Repository navigation
Conversation
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 45.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 170 functions across 34 files. (1 skipped… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly summarises the main knowledge-base changes: add_document chunking, partial-success ingestion, and stable upload rejection codes. Linked Issues check The PR addresses all active direct issue requirements. For [ #17830],add_documentuses shared chunking, assigns shared document provenance and deterministic chunk IDs, and adds multi-fact tests. For…Out of Scope Changes check The reviewed changes remain connected to the direct issue objectives. Ingest budgets, environment variables, resumable records, and upload rejection handling support the chunked upload requirements. T…
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 45.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 170 functions across 34 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
-
📝 Generate docstrings 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks 💡
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
|
Review (Sonnet) at 6144390: blocked on two MEDIUMs. Every AC of #17830 is met as written:
The chunker reuses the canonical one and is not a fork.
For the closing decision: the ACs of #17830 are met literally, but the issue's own failure scenario (a manually uploaded PDF stored as one vector) persists until #18407 lands, because upload, /facts and /url still bypass |
|
Delta review (Sonnet) at 8d54a90: approve.
LOWs:
Queued for the next merge train. |
|
Delta review (Sonnet, plus a merger check) at ddc0ae0: blocked on one High introduced by the #18129 fold.
Verified fine:
|
…e one indexer writes, and a re-index drops the file's prior chunks (#18129)
|
Delta check at 551d91b: approve. The docs-browser High is fixed.
Nit: the
|
|
Delta review @ 567ab9c (main merge): approve.
|
|
Ejected from merge train #9 (#18507 went red; it is rebuilt as 9c without this PR). These were reproduced by CI on the merged tree:
Back with the owner; it rejoins a train once fixed (delta review). |
…t, env_registry_knowledge, redaction guard follows add_document (#17530)
|
Delta review @ 5719de5: one blocker left; everything else checks out.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
eb9d96d4-e4c0-40bf-ab3d-fb324bdf5639
⛔ Files ignored due to path filters (3)
pipeline-scripts/hardcoded_values_baseline.txtis excluded by!**/*_baseline.txt,!pipeline-scripts/hardcoded_values_baseline.txtrepo_tests/python_file_size_ratchet_baseline.pyis excluded by!repo_tests/python_file_size_ratchet_baseline.pyscripts/python_file_size_known_large.pyis excluded by!scripts/python_file_size_known_large.py
📒 Files selected for processing (38)
autobot-backend/api/kb_ingestion_sites_system_16693_test.pyautobot-backend/api/kb_upload_chunking_18407_test.pyautobot-backend/api/knowledge.pyautobot-backend/api/knowledge_office_upload.pyautobot-backend/api/knowledge_upload_no_text_layer_test.pyautobot-backend/api/knowledge_upload_redaction_test.pyautobot-backend/api/knowledge_upload_rejection_codes_test.pyautobot-backend/app_factory.pyautobot-backend/knowledge/chunk_provenance.pyautobot-backend/knowledge/document_ingest.pyautobot-backend/knowledge/document_ingest_test.pyautobot-backend/knowledge/documents.pyautobot-backend/knowledge/schemas/documents.pyautobot-backend/knowledge/upload_pages.pyautobot-backend/knowledge/upload_rejections.pyautobot-backend/services/knowledge/doc_indexer.pyautobot-backend/services/knowledge/doc_listing.pyautobot-backend/services/knowledge/doc_listing_18129_test.pyautobot-backend/services/knowledge/doc_taxonomy.pyautobot-backend/services/knowledge/test_doc_indexer_scope_18129.pyautobot-backend/tests/test_page_provenance_13894.pyautobot-backend/utils/slug_equivalence_18093_test.pyautobot-infrastructure/shared/scripts/utilities/index_documentation.pyautobot-infrastructure/shared/tools/index_documentation.pyautobot-infrastructure/shared/tools/index_documentation_category_default_test.pyautobot_shared/doc_chunking.pyautobot_shared/doc_chunking_test.pyautobot_shared/env_registry_backend_services.pyautobot_shared/env_registry_knowledge.pydocs/developer/ENV_VARS.mdrepo_tests/_reach_policy.pyrepo_tests/connector_redaction_guard_test.pyrepo_tests/doc_index_cli_behaviour_18129_test.pyrepo_tests/doc_index_cli_flags_18129_test.pyrepo_tests/infra_backend_imports_allowlist_18288.pyrepo_tests/one_doc_indexer_18129_test.pyrepo_tests/reach_scope_claim_17844_test.pytools/index_documentation.py
💤 Files with no reviewable changes (3)
- autobot-backend/utils/slug_equivalence_18093_test.py
- repo_tests/infra_backend_imports_allowlist_18288.py
- autobot-infrastructure/shared/tools/index_documentation_category_default_test.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
|
|
||
| # The knowledge-component registrations live in their own module (file-size ceiling). | ||
| from autobot_shared import env_registry_knowledge # noqa: E402,F401 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Import env_registry_knowledge from env_registry.py instead.
The repository documentation requires new component modules to be imported from autobot_shared/env_registry.py. This import is in a sibling registry module. The PR description reports that the extensibility test fails for this reason. Move the import to env_registry.py and remove it here.
As per path instructions (ENV_VARS.md): "if creating a component module, import it from autobot_shared/env_registry.py".
Source: Path instructions
| # The one ingest path (#18407): long input is chunked, with per-chunk page_spans. | ||
| result = await kb.add_document(content, metadata) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not report a partial or failed ingest as success.
add_document can now return partial, timeout or error. _store_fact_in_kb ignores status and returns only fact_id. On a partial result, upload_file_to_knowledge returns success=True with the full word_count, and the audit record says "success". Some chunks are missing, and the response does not tell the user. On a timeout or error result, the upload returns success=True with document_id=None. Check result["status"] in _store_fact_in_kb. Raise an error for timeout/error. Expose partial together with chunks_stored/chunks_total so the client can retry with the same id.
| KB_CHUNK_MAX_TOKENS = env_int("AUTOBOT_KB_CHUNK_MAX_TOKENS", 800) | ||
| KB_INGEST_RECORD_TTL_SECONDS = env_int("AUTOBOT_KB_INGEST_RECORD_TTL_SECONDS", 30 * 24 * 3600) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Read AUTOBOT_KB_CHUNK_MAX_TOKENS with a clamped reader.
env_int falls back only on a malformed value. It does not enforce the registered range=(50, 100000). If an operator sets the value to 0 or a negative number, _split_oversized uses a 1-word window and _hard_cap uses a 1-character limit. A long document then becomes thousands of chunks, and the code stores and embeds each one. Use the clamped integer reader from autobot_shared/env_utils.py with the registered bounds. Also give the TTL a lower bound. A non-positive expire deletes the resume record at once.
As per path instructions: "an EnvVarSpec range= is documentation that enforces nothing" and new numeric env reads must "Use the clamped readers in autobot_shared/env_utils.py".
Sources: Path instructions, Learnings
| def _select_files(root_dir: Path, tier: int | None = None, category: str | None = None) -> List[Tuple[str, int]]: | ||
| """Discover files for ``tier`` (None = all), keeping only those in ``category`` when given (#18129).""" | ||
| files = _discover_files(root_dir, tier) | ||
| if category: | ||
| files = [(p, t) for p, t in files if detect_category(Path(p)) == category] | ||
| return files |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Match categories against the repository-relative path.
_discover_files returns absolute paths (root_dir / rel). detect_category then searches the full absolute path for directory patterns such as /developer/, /testing/, /api/ and /features/. Suppose the checkout sits under a directory with one of those names, for example ~/developer/AutoBot or /tmp/testing/repo. Every file then gets that category.
The result:
--category developerselects the whole tree.--category apiselects no files.
The documentation browser (doc_listing.group_indexed_documents) calls detect_category on the stored relative file_path. The CLI filter and the browser categories therefore disagree. Pass the path relative to root_dir.
🐛 Proposed fix
files = _discover_files(root_dir, tier)
if category:
- files = [(p, t) for p, t in files if detect_category(Path(p)) == category]
+ files = [(p, t) for p, t in files if detect_category(Path(os.path.relpath(p, root_dir))) == category]
return files| # Ids hash the section name: drop the file's prior chunks so a renamed section leaves no orphan. | ||
| await asyncio.to_thread(self._collection.delete, where={"file_path": rel_path}) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Delete the old chunks only after the new chunks are stored.
Line 929 deletes every row for rel_path before any new chunk is embedded. If embedding fails for every chunk, the file has no rows left in autobot_docs. A transient Ollama outage or a dimension error can cause this. Before this change, the old chunks stayed searchable after such a failure.
The hash cache prevents a retry:
index_allsavesnew_hashesfor every selected file, including files that failed.index_fileupdates the cache in_check_hash_and_update_cachebefore indexing.
The next incremental run therefore skips the file until its content changes. That documentation stays missing from search and from the documentation browser.
Do the cleanup after the upserts instead. One way is to give each run a token in the chunk metadata. After a run that stored at least one chunk, delete only the rows for rel_path that carry a different token. This still removes chunks left behind by a renamed section. A complete failure leaves the previous index in place.
🛡️ Proposed fix
--- "a/autobot-backend/services/knowledge/doc_indexer.py"
+++ "b/autobot-backend/services/knowledge/doc_indexer.py"
@@ -925,14 +925,21 @@
file_tags = _extract_tags(body, file_str)
file_tags = list(dict.fromkeys(fm_tags + fm_aliases + file_tags))[:20]
- # Ids hash the section name: drop the file's prior chunks so a renamed section leaves no orphan.
- await asyncio.to_thread(self._collection.delete, where={"file_path": rel_path})
+ run_token = uuid.uuid4().hex
indexed = 0
for i, chunk in enumerate(chunks):
- ok = await asyncio.to_thread(self._index_chunk, chunk, i, len(chunks), rel_path, file_tags, tier)
+ ok = await asyncio.to_thread(
+ self._index_chunk, chunk, i, len(chunks), rel_path, file_tags, tier, run_token
+ )
if ok:
indexed += 1
+ if indexed:
+ # Ids hash the section name: drop rows from earlier runs so a renamed section leaves no orphan.
+ await asyncio.to_thread(
+ self._collection.delete,
+ where={"$and": [{"file_path": rel_path}, {"index_run": {"$ne": run_token}}]},
+ )
return indexed, len(chunks)
async def index_file(self, file_path: Path, tier: int = 3, force: bool = False) -> IndexResult:
"""Index a single markdown file at the given priority tier; force skips the hash check."""Add a run_token parameter to _index_chunk and write "index_run": run_token into its metadata. The split sub-chunks inherit that key through {**metadata, ...}. Update the _FakeCollection.delete test double so that it supports the $and/$ne filter.
| if args.verbose: | ||
| logging.getLogger().setLevel(logging.DEBUG) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make --verbose lower the level of the loggers that get_logger creates.
main sets the root logger to DEBUG. run_index then calls get_logger(__name__). LoggingManager.get_logger (autobot_shared/logging_manager.py:197-271) calls logger.setLevel(...) on each named logger, using the configured logging.log_level (default INFO). A logger's own level overrides the root level. As a result, debug records from this CLI and from services.knowledge.doc_indexer are still dropped, and --verbose has no effect.
Lower the level after the loggers exist, or route the setting through the configured log level.
🐛 Proposed fix
--- "a/tools/index_documentation.py"
+++ "b/tools/index_documentation.py"
@@ -180,10 +180,11 @@
def main(argv: list[str] | None = None) -> int:
"""Entry point. `--incremental` is the default when no mode is given."""
args = parse_args(argv)
if args.verbose:
- logging.getLogger().setLevel(logging.DEBUG)
+ os.environ["AUTOBOT_LOG_LEVEL"] = "DEBUG" # read by LoggingManager when it configures each logger
+ logging.getLogger().setLevel(logging.DEBUG)
return asyncio.run(run_index(force=args.force, rebuild=args.rebuild, args=args))
if __name__ == "__main__":Check that AUTOBOT_LOG_LEVEL maps to logging.log_level before the first get_logger call. If it does not, call setLevel(logging.DEBUG) on every logger in logging.root.manager.loggerDict after the service import.
This branch has not been deployed
1 participant
Closes #17830
Closes #18407
Refs #17986
Refs #17577
Refs #17530
Refs #17527
Thinking Path
add_documentembedded a whole transcript/PDF as one vector.autobot_shared/doc_chunking.pyalready chunks by paragraph budget but hardcoded 800 and could not split a single oversized paragraph (an unbroken transcript). Extended it (max_tokensparameter,chunk_plain_text) instead of adding a chunker.kb_timeouts.document_add, SSOT timeouts) is enforced inside the chunk loop; status reusesOperationOutcome(success/partial/timeout) rather than a new status enum.detail_code+detail_paramsvia anUploadRejection(HTTPException)and a registered handler;detailkeeps its English text./entriesobservation, so no backend change is made for it.What Changed
autobot_shared/doc_chunking.py:chunk_large_content(max_tokens=800),chunk_plain_text.knowledge/document_ingest.py(new): chunked, budgeted, resumable ingest. Chunk 0 keepsfact_id == doc_id; later chunks are<doc_id>:chunk:<n>; chunk metadata carriesdocument_id/chunk_index/chunk_count. Per-document Redis record (kb:ingest:<doc_id>) holds status, chunks stored and a content digest; a retry with the same id and content resumes after the last stored chunk, changed content restarts. Single-chunk documents behave exactly as before.knowledge/documents.py:add_documentdelegates to it. Resultstatusissuccess|partial|timeout|error, pluschunks_stored/chunks_total.knowledge/upload_rejections.py(new):UploadRejectionCodeenum,UploadRejection, handler (registered inapp_factory.py),no_text_rejection,extract_within_deadline(moved out ofapi/knowledge.py, which shrinks 3414 -> 3380; both ceilings lowered).Env vars
AUTOBOT_KB_CHUNK_MAX_TOKENS,AUTOBOT_KB_INGEST_RECORD_TTL_SECONDSregistered +ENV_VARS.mdregenerated.No response schema changed, so
api.tsis not regenerated (detail_codeis emitted by the exception handler, not a response model).Review round: (1)
doc_chunking._hard_capsplits a single token larger than the budget by characters; (2)document_ingest._drop_orphansdeletes the document's own chunks past the new count when shorter content is re-added under the same id (and the record when it shrinks to one chunk); (3) bug(kb): the upload, /facts and /url routes bypass add_document — a PDF uploaded in the UI is still stored as one vector #18407:_store_fact_in_kb(upload, /facts, /url, add_text, audio) now stores throughadd_document;knowledge/chunk_provenance.pygives each chunk only thepage_spansof the pages it covers, re-based to the chunk (a chunk that cannot be located gets none rather than a wrong one). The knowledge upload: rejection reasons reach the user in English only — the API composes prose where it should emit a code #17577 codes are unchanged.page_spansdrift fixed at the root:knowledge/upload_pages.pyclean_upload_textcleans a paginated PDF page by page and re-renders text and spans together, so spans index the text actually stored (previously computed before redaction/sanitizing).Verification
Review-round tests:
knowledge/document_ingest_test.pynow 11 (oversized token, orphans, shrink-to-one, per-chunk pages),api/kb_upload_chunking_18407_test.py3 (long upload / PDF-style pages / facts+url helper: more than one fact sharing the returned document id; contrast: with the helper back onstore_fact, all 3 fail). Existing redaction and security(kb): backfill facts with no owner or visibility as SYSTEM before chat grounding is scoped #16693 tests updated to assert onadd_document.KB selection after the change: 2208 passed, 263 skipped, 1 failed (
api/file_upload_comprehensive_test.py::test_knowledge_base_integration, asserts a cwd-relativedata/file_manager_rootpath; touches nothing changed here; not verified against main). Ratchets andreach_scope_claim_17844pass.New tests:
knowledge/document_ingest_test.py(7),api/knowledge_upload_rejection_codes_test.py(5, AST contract: no bareHTTPExceptionon the upload path, every raise names an enum member, every member is used).Contrast: with
documents.pyandapi/knowledge.pyrestored toorigin/main, 9 of the 12 new tests fail; with the change, 38 pass across them plusconfig/timeout_configuration_test.pyandapi/knowledge_upload_no_text_layer_test.py.Broader KB selection (
knowledge,api/knowledge*_test.py,config,-k "knowledge or document or upload or chunk or fact", e2e ignored): 1996 passed, 65 skipped.symbol_fork_ratchet_17312,python_file_size_ratchet,ceiling_record_parity_17872,guard_reach_metaandreach_scope_claim_17844: 93 passed.Final-tree runs: KB selection 2225 passed, 267 skipped, 1 failed (
file_upload_comprehensive_test::test_knowledge_base_integration, reproduced on a detachedorigin/maincheckout, so pre-existing; filed bug(tests): file_upload_comprehensive_test::test_knowledge_base_integration fails on main: it asserts a cwd-relative data/file_manager_root exists #18419); symbol_fork_ratchet_17312, python_file_size_ratchet, ceiling_record_parity_17872, guard_reach_meta: 83 passed; reach_scope_claim_17844 alone: 10 passed.Contrast: bug(kb): the upload, /facts and /url routes bypass add_document — a PDF uploaded in the UI is still stored as one vector #18407 tests now count facts through a fake KB whose every path ends in
store_fact: on the old helper all 3 fail with "stored as a single fact"; with spans taken before cleaning the redaction test fails with "span names the wrong page".Folded in
From branch
issue-18129-doc-indexers(handed over), cherry-picked with -x:DocIndexerService. Conflicts:_reach_policy.pyandreach_scope_claim_17844_test.py(kept this branch's entries and addedone-doc-indexerbeside them; kept this branch'sreach_declarations_testimport in place of the hand list);autobot-infrastructure/shared/tools/index_documentation.py(took the shim; the tech-debt(backend): six regex concepts are forked across 2–8 files each, and one fork is a no-op #18093sanitize_tokenedit it conflicted with is inside the code the shim removes, and the canonical service already usessanitize_token).chunk_large_content, but a header-less or long-intro document was kept as ONE over-budget chunk (it relied on the embedder rejecting it and_split_and_embedhalving it). The intro is now cut withchunk_plain_text; the test failed before (1 chunk vs 3). Not routed throughadd_document: the indexer writes its own ChromaDB docs collection with doc-level metadata, a different store from KB facts.docs/browse,docs/categoriesanddocs/statsscanned Redisdoc_hash:*, whose only writer was the retired script, so a fresh install's browser was empty. They now read theautobot_docscollection (services/knowledge/doc_listing.py, grouped byfile_path; category is the taxonomy id,content_hasha stable per-path id); an unopenable index is a 503. One store, no Redis mirror; response shapes unchanged. A re-index now deletes the file's prior chunks first, so a renamed section leaves no orphan. New tests:doc_listing_18129_test.py(browse, categories, stats, 503, orphan, and a contrast where the old Redis reader returns empty with nodoc_hashkeys) andrepo_tests/doc_index_cli_behaviour_18129_test.py(--dry-run,--output,--search,--stats, scoping).#18129 acceptance criteria (verified against this branch): one implementation, both infra scripts are 21-line
runpyshims, tool path resolvesDocIndexerService(one_doc_indexer_18129_test); tags from one helper (sanitize_token, single list indoc_indexer._extract_tags); no infra script re-derives front matter; guardtest_only_the_canonical_service_chunks_markdown. The flag-by-flag comparison is now recorded on the issue (#18129 (comment)), so every criterion is met.Closes #18129
Verification (folded work)
Model Used
Claude Sonnet 5.5
🤖 Generated with Claude Code
Summary by CodeRabbit