Repository navigation
refactor(document): consolidate five forked PDF extractors onto one canonical path (#13893) - #13924
Conversation
✅ SSOT Configuration Compliance: Passing🎉 No hardcoded values detected that have SSOT config equivalents! |
|
Fixed rather than bypassed (Rule 6). Each substitution was checked to be behaviour-preserving:
Two things worth recording: An eighth instance the guard does not catch. Line 2835 (
Verification: |
…anonical path (#13893) Five independent PDF text extractors existed - three live, two orphaned with zero callers. They disagreed on page markers (## Page N, --- Page N ---, and none at all on the live KB upload path), so every fix to the document path had to ship five times or be forgotten in four places. One of the live forks ran PyPDF2, which is unmaintained, while the repo pins pypdf>=6.15.0 as a security update - so Drive/OneDrive ingestion used the library the rest of the codebase had deliberately moved off. Adds media/document/extraction.py as the single implementation and repoints all five call sites at it. Page text is kept structured rather than pre-joined, so per-page provenance can reach retrieval as metadata instead of as marker text baked into the embedding (#13894). Splits DocumentDependencyError from DocumentExtractionError so a missing library stays reportable as 'unavailable' rather than collapsing into 'error' - a deployment gap must not be diagnosed as a bad upload. Removes the PyPDF2 dependency; it had no remaining importers or test users. The pipeline tests mocked PdfReader, which would have passed against all five old implementations without proving which one ran. They now exercise real generated PDFs through the public pipeline entry point.
…th SSOT constants (#13893) The consolidation touches api/knowledge.py, and the hardcoded-value guard scans changed *files* rather than changed lines — so seven pre-existing violations in that file became this PR's failing check. Fixed rather than worked around, per Rule 6. All seven substitutions are behaviour-preserving; each constant was checked to equal the literal it replaces (DEFAULT_SEARCH_LIMIT=10, GENERAL='general', timeout.default_request=30.0). An eighth instance at line 2835 the guard does not match was fixed too — the same defect in a shape its pattern misses, and leaving one behind is how the debt survives a cleanup. config is imported at the use site, not module scope: add_watch_folder already binds 'config' as a local WatchFolderConfig, which would silently shadow a module-level import. It sits outside the try so an ImportError surfaces as itself rather than as a URL-fetch failure.
) Same shape as #13924: the hardcoded-value guard scans changed *files* rather than changed lines, so touching context_overflow.py and compression.py made this PR responsible for three pre-existing violations in them. git blame puts all three on unrelated commits (5ac5ee7, 1d38dcd, 1238951); this branch has exactly one commit and none of them is it. Behaviour-preserving: ROLE_ASSISTANT == 'assistant' and ROLE_USER == 'user' were checked against the values they replace, not assumed. Imported from autobot_shared.ssot_constants rather than through the constants.threshold_constants shim — that shim's own docstring directs new code to the canonical module, and both files already import from autobot_shared.
…ant per concept (#13933) Same inheritance as #13924/#13797: the hardcoded-value guard scans changed files rather than changed lines, so touching loop.py and tool_registry.py made this PR responsible for three pre-existing violations. All three blame to adbf666, which is none of this branch's three commits. The guard's suggested fix is right for two of the three and wrong for the third. tool_registry.py's 'user_role' is an RBAC role — worker_node reads it in _validate_user_role and _check_task_permission — not a chat message role. It uses Role.USER from autobot_shared.auth.permissions rather than CategoryDefaults.ROLE_USER, which is the chat vocabulary and merely happens to hold the same string. Taking the suggestion literally would tie an authorization decision to a presentation constant, and the two are free to diverge. Behaviour-preserving: ROLE_ASSISTANT == 'assistant' and Role.USER.value == 'user' were checked against the values they replace. tool_registry.py now imports autobot_shared.auth.permissions; that module's own docstring warns about an import cycle through auth_middleware, so both modules were import-smoked to confirm none is introduced. tests/agent_loop: 230 passed.
…13950) (#13962) * chore: claim worktree for #13950 * ci(code-quality): scope the hardcoded-value guard to changed lines (#13950) The guard reads each changed FILE whole, so editing one line of a legacy module makes a PR answerable for that file's entire backlog. Four PRs in one session failed on violations git blame attributes to other commits: #13924 (7), #13797 (3), #13934 (3), #13945 (1). None touched an offending line. The rule is good and all 14 were worth fixing — they are fixed. The cost is that each occurrence looks like a genuine regression until a per-line blame plus a check of the branch's own commit list proves otherwise, and it recurs on every touch of a large legacy module. It also makes avoiding old files the cheapest way to stay green. Worse, in two of the four the constant the message SUGGESTED was the wrong concept: #13934's 'user_role' is an RBAC input, not the chat-vocabulary CategoryDefaults.ROLE_USER, and #13945's le=100 is a ceiling, not KNOWLEDGE_DEFAULT_LIMIT. Both hold the same value today, so the mechanical fix the message invites goes green while encoding a different meaning. Advice that is unsafe to follow is a reason to only give it where the author is in the code. Opt-in via --changed-lines-only rather than a default, so every other hook that runs through this wrapper keeps its behaviour byte for byte. Only code-quality's hardcoded-values step passes it; pre-commit and a direct run still scan whole files, so the backlog stays measurable rather than invisible. Fails closed. If the hook exits non-zero and the filter parses no 'file:line' violations, the output is not one it understands, so it prints everything and exits with the hook's status — suppressing an unparsed failure would turn a real regression green, which is #13880's defect reached from the other direction. Suppressed violations are still printed under their own heading; the hook's own 'COMMIT BLOCKED' trailer is dropped in scoped mode because it counts what the hook found across whole files, not what this wrapper is about to decide. 3 mutations killed: filter suppressing everything (guard becomes an off switch), filter keeping everything (no scoping), and the default silently gaining scoping. A first version also counted the hook's banner and trailer as kept and reported '6 violations on lines this PR added' for a PR that added one clean line — the summary contradicting the correct suppression directly above it.
…sweep touching operator-chosen roots (#13933) (#13934) * chore: claim worktree for #13933 * fix(agent): bound classification by serialised size, and stop the sweep touching operator-chosen roots (#13933) * fix(agent): measure the replacement instead of estimating it, and release the resume binding (#13933) * fix(agent): replace hardcoded role literals with the right SSOT constant per concept (#13933) Same inheritance as #13924/#13797: the hardcoded-value guard scans changed files rather than changed lines, so touching loop.py and tool_registry.py made this PR responsible for three pre-existing violations. All three blame to adbf666, which is none of this branch's three commits. The guard's suggested fix is right for two of the three and wrong for the third. tool_registry.py's 'user_role' is an RBAC role — worker_node reads it in _validate_user_role and _check_task_permission — not a chat message role. It uses Role.USER from autobot_shared.auth.permissions rather than CategoryDefaults.ROLE_USER, which is the chat vocabulary and merely happens to hold the same string. Taking the suggestion literally would tie an authorization decision to a presentation constant, and the two are free to diverge. Behaviour-preserving: ROLE_ASSISTANT == 'assistant' and Role.USER.value == 'user' were checked against the values they replace. tool_registry.py now imports autobot_shared.auth.permissions; that module's own docstring warns about an import cycle through auth_middleware, so both modules were import-smoked to confirm none is introduced. tests/agent_loop: 230 passed.
|
Follow-up filed from this PR: #14005 — the hardcoded-value guard misses literals inside comprehensions. Line 2835 of Same literal, same rule — caught or missed by statement shape. |
…elper (#13694) (#13797) * fix(memory): count tokens at decision boundaries through one shared helper (#13694) Context-fitting decisions ran on len(text.split()) * 1.3, whose own docstring conceded it held 'for English prose' — which is exactly where large prompts do not come from. Measured divergence against the character estimate: case words*1.3 chars/4 ratio English prose 19 20 1.1x Code 11 35 3.2x JSON 1 19 19.0x CJK 1 26 26.0x text.split() returns ONE element for a non-space-delimited script, so CJK under-counted ~26x. That number gates whether compression runs at all, so the threshold was reached only after the real window was blown. Investigation first, and it changed the fix. The issue proposed a local exact counter; tiktoken is importable in some dev environments but is in no requirements file, so building on it would be green locally and broken in CI. Meanwhile the 80/90% overflow trigger ALREADY consumes authoritative provider usage — api/chat.py:797 -> handle_message_completion -> check_and_protect (usage=llm_response.usage) — so that AC was already satisfied and did not need a tokenizer at all. What was actually wrong: - services/memory/compression.py used words*1.3 for the compression gate. - context_overflow.py re-added post-summary tokens with an inline len//4 — a fourth estimator, and the numbers the trigger runs on until the next provider response supplies a real count. autobot_shared/token_count.py now holds the fast/exact distinction, delegating the fast path to the canonical doc_chunking estimator rather than deriving a new ratio. Both call sites use it. Why no local exact tokenizer exists is documented there so the next reader does not re-litigate it. Also restores _CONFIG_PATH, which my first edit to compression.py deleted — caught by flake8 F821 before it left the branch. * fix(memory): replace hardcoded role literals with SSOT constants (#13694) Same shape as #13924: the hardcoded-value guard scans changed *files* rather than changed lines, so touching context_overflow.py and compression.py made this PR responsible for three pre-existing violations in them. git blame puts all three on unrelated commits (5ac5ee7, 1d38dcd, 1238951); this branch has exactly one commit and none of them is it. Behaviour-preserving: ROLE_ASSISTANT == 'assistant' and ROLE_USER == 'user' were checked against the values they replace, not assumed. Imported from autobot_shared.ssot_constants rather than through the constants.threshold_constants shim — that shim's own docstring directs new code to the canonical module, and both files already import from autobot_shared. * Auto-fix: code formatting Applied Black, isort, and autoflake to match code-quality checks. Triggered by workflow auto-fix-formatting.yml. * fix(memory): restore the estimate_fast import essential_story lost (#13694) flake8 F821: essential_story.py:208 calls estimate_fast with no import, so the module would raise NameError the first time _estimate_tokens ran. Self-inflicted. Earlier in this branch's history I started a rebase, it stopped part-way, and 'git rebase --abort' discarded a working-tree change that was adding exactly this import. The call site survived in an earlier commit; the import did not. Caught by flake8 rather than by any test, which is the point worth noting: the branch's own suite never exercises _estimate_tokens, so nothing here would have failed until the code ran in production. Verified by mutation, not by re-reading: removing the import reproduces 'F821 undefined name estimate_fast' at 208:16, restoring it returns exit 0, and the module imports with estimate_fast resolving to autobot_shared.token_count. * test(memory): pin agreement with the canonical estimator, not the old ratio (#13694) python-suite shard 9/12: test_multiple_words asserted 13, the shared helper returns 12. The test pinned the behaviour this PR deliberately removes — '10 * 1.3 = 13', the word-based formula whose own docstring conceded it held 'for English prose'. Checked before changing the number, because updating an expected value is exactly how a real regression gets waved through: old: len(text.split()) * 1.3 -> 10 words = 13 new: character-based canonical -> 49 chars = 12 They are different estimators, and 12 is the canonical one. The change is the point of the PR — the word formula under-counted JSON by ~19x and CJK by ~26x, and that number gates whether compression runs at all, so under-counting disabled compression on exactly the payloads that needed it. Rather than swapping 13 for 12, the test now asserts _estimate_tokens(text) == estimate_fast(text): agreement with the canonical estimator, not a second copy of its arithmetic that would drift the moment the ratio is tuned. Added test_a_non_space_delimited_script_is_not_under_counted, which pins the DEFECT rather than the arithmetic: text.split() returns one element for CJK, so the old estimator scored a 200-character Chinese string as 1 token. A value-only test goes green again on any ratio tweak; this one fails for the whole class of input the previous formula could not see. Verified by mutation: reverting to int(len(text.split()) * 1.3) fails both. * test(memory): sweep the remaining assertions pinned to the removed ratio (#13694) python-suite shard 8/12: test_essential_story.py::test_ten_words asserted int(10 * 1.3), the same defect as the shard 9 failure I fixed one commit ago in test_context_compression.py. My mistake was fixing the instance CI reported instead of the class. The estimator change touches three test files; I found the third only because a second shard failed. Swept properly this time: tests/memory/test_essential_story.py test_ten_words -> agreement assert + CJK under-count test tests/memory/test_essential_story.py:221 stale '10 words * 1.3 = 13' comment tests/services/test_context_compression.py:137 stale '6 words * 1.3' comment tests/test_token_count_boundaries.py keeps len(text.split()) * 1.3 on purpose — it is the baseline the new estimator is compared against there, not a stale assertion. Left alone. Both new assertions pin agreement with the canonical helper rather than a literal, so they cannot drift when the ratio is tuned, and the CJK test fails for the whole class of input the old formula could not see rather than for one value. Verified by mutation: reverting to int(len(text.split()) * 1.3) fails both. 59 passed across the three affected files. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…CategoryDefaults (#14047) (#14128) * chore: claim worktree for issue-14047 * fix(backend,frontend): replace hardcoded category/mode defaults with CategoryDefaults (#14047) The widened hardcoded-values guard (#14005/#14048) surfaced 33 genuine category/mode literal defaults across autobot-backend, autobot-infrastructure shared scripts, and autobot-frontend that predate the guard. Each `.get("category", "general")`-style default now routes through the existing CategoryDefaults SSOT (autobot_shared/ssot_constants.py) instead of repeating the bare string, matching the precedent already established in api/knowledge.py (#13924). Frontend sites had no existing TS mirror of CategoryDefaults, so one was added to src/config/ssot-config.ts (CATEGORY_DEFAULTS) following the same mirror-plus-parity-test pattern already used for PermissionMode/PortConfig, with a new parity assertion in ssot-parity.spec.ts checking it byte-for-byte against the Python source. Every replaced literal is confirmed identical to the constant it now uses: CategoryDefaults.GENERAL == "general", CategoryDefaults.UNKNOWN == "unknown", CategoryDefaults.SEARCH_MODE_HYBRID == "hybrid" -- verified against autobot_shared/ssot_constants.py and cross-checked site by site against the diff. Two sites already had full pre-existing test coverage (specialized_agent_service.py, skills/registry.py) and one had partial coverage (essential_story.py) needing no new tests; the remaining 30 sites get new default-applies/override tests, each verified to fail when the CategoryDefaults reference is mutated to a bogus sentinel. * style: apply black/isort formatting to #14047 changes (#14047) * fix(tests): explicit-path imports + CategoryDefaults value pin (#14047) Review of #14047 found three of its new autobot-infrastructure/shared test files bare-import their subject module, which fails under real pytest invocation (--import-mode=importlib does not add a test's own directory to sys.path, per pytest.ini's own documented pipeline-scripts trap) and, for index_documentation specifically, is ambiguous -- two unrelated files share that bare name in the repo. All three now load their subject via explicit importlib.util.spec_from_file_location, verified to collect and pass with no PYTHONPATH workaround. Also adds a value-pin test for CategoryDefaults (autobot_shared) mirroring what ssot-parity.spec.ts already does on the TypeScript side: every other new assertion in this issue compares against the CategoryDefaults symbol itself, so a value rename (e.g. GENERAL -> "generic") would move production code and every assertion together and stay green. Mutation-tested: changing GENERAL reddened this new pin while leaving the symbol-comparison tests green, confirming it catches a defect class nothing else in the PR does. autobot-infrastructure remains outside pytest.ini testpaths (tracking gap: 14+ pre-existing collection errors and 2 fatal sys.exit() crashes found while investigating wiring it in, filed separately) -- these three files are individually correct and will start running the moment that gap closes.
Thinking Path
Five independent PDF text extractors existed. Three were live, two were fully
orphaned (728 lines, zero callers, not even tests). They disagreed on page markers
—
## Page N,--- Page N ---, and none at all on the live KB upload path — soevery fix to the document path had to ship five times, or ship once and be
forgotten in four places. That is precisely what blocked the rest of #13892.
One live fork ran PyPDF2, which is unmaintained, while
requirements.txt:23pins
pypdf>=6.15.0with the comment "SECURITY UPDATE (improved permissionchecks)". Google Drive and OneDrive ingestion was running on the library the rest
of the codebase had deliberately moved off.
What Changed
New:
autobot-backend/media/document/extraction.py— the singleimplementation. Callers adapt its result to their own error shape (an
HTTPExceptionfor the API, aProcessingResultfor the media pipeline, an emptystring for the connectors), but nobody re-implements extraction.
All five call sites now delegate:
api/knowledge.py:1065extract_pdf(...)media/document/pipeline.pyextract_document(...)knowledge/connectors/content_extraction.pyextract_pdf(...)utils/document_parser.pyextract_pdf(...)utils/document_extractors.pyextract_pdf(...)Three design decisions worth flagging:
ExtractedDocument.pagesholdsper-page text;
render_pages()produces the flat string. That split is what letsfix(knowledge): KB upload loses page provenance — facts cannot cite a source page #13894 carry page numbers to retrieval as metadata rather than as marker text
baked into the embedding.
DocumentDependencyErrorsplit fromDocumentExtractionError. A missinglibrary is a deployment gap; a corrupt file is the user's problem. The media
pipeline's existing
unavailablevserrordistinction is preserved, so one cannever be diagnosed as the other.
""with a truthful
page_count, and unreadable pages are kept inpagesrather thandropped — that is the signal fix(document): media pipeline reports confidence 0.95 success on a zero-text extraction #13884 detects and feat(document): rasterize-then-OCR fallback for PDFs with no text layer #13896 acts on.
Removed: the
PyPDF2dependency. It had no remaining importers and no testusers (verified before removal).
Verification
One reader remains, and the forks cannot grow back silently:
Tests — 40 passing across the new suite and the rewritten pipeline suite:
Mutation-tested rather than merely re-run — the anti-fork guards were each
provoked to prove they can fail:
No new failures anywhere. The full knowledge suite is identical on base and on
this branch — the 6 failures are pre-existing (4 are order-dependent and pass in
isolation, 2 are environmental):
Startup imports clean (
352 passed);flake8 --max-line-length=120exit 0; noprint(; closure gatecheck-new-module-callers.shexit 0.Test quality note: the old pipeline tests mocked
PdfReader. That mock wouldhave passed happily against all five old implementations and proved nothing about
which one actually ran — the exact blind spot that let five forks coexist. They now
generate real PDFs and assert through the public pipeline entry point.
Known gap — not claimed as done
The issue's acceptance criterion "every orphan is wired to a live caller" is
not met by this PR.
utils/document_parser.pyandutils/document_extractors.pyno longer carry forked implementations — they delegate — but they still have zero
callers.
Wiring them in properly means letting the KB upload endpoint accept the formats
DocumentParseruniquely supports (XLSX, PPTX, ODT, ODS, ODP, ODG). It was builtfor exactly that — its docstring reads "Universal Document Parser for Knowledge
Base" — but
ALLOWED_EXTENSIONS(api/knowledge.py:124) has never included them.That is a product decision with real blast radius: it puts three more parsers
(
odfpy,openpyxl,python-pptx) on the untrusted-upload path. Per Rule 6 I amnot making that call unilaterally. Raised for a decision on #13893; this PR is
complete for the consolidation itself.
Model Used
Claude Opus 5 (1M context)
Part of #13892 — Wave 1. Unblocks #13884, #13894, #13895, #13896, #13897.