Skip to content

refactor(document): consolidate five forked PDF extractors onto one canonical path (#13893) - #13924

Merged
mrveiss merged 3 commits into
Dev_new_guifrom
issue-13893
Aug 10, 2026
Merged

mrveiss merged 3 commits into
Dev_new_guifrom
issue-13893

Conversation

@mrveiss

@mrveiss mrveiss commented Aug 10, 2026

Copy link
Copy Markdown
Owner

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 — so
every 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:23
pins pypdf>=6.15.0 with the comment "SECURITY UPDATE (improved permission
checks)". 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 single
implementation. Callers adapt its result to their own error shape (an
HTTPException for the API, a ProcessingResult for the media pipeline, an empty
string for the connectors), but nobody re-implements extraction.

All five call sites now delegate:

Call site Before After
api/knowledge.py:1065 own pypdf loop extract_pdf(...)
media/document/pipeline.py own pypdf + docx loops extract_document(...)
knowledge/connectors/content_extraction.py PyPDF2 extract_pdf(...)
utils/document_parser.py own pypdf loop extract_pdf(...)
utils/document_extractors.py own pypdf loop extract_pdf(...)

Three design decisions worth flagging:

Removed: the PyPDF2 dependency. It had no remaining importers and no test
users (verified before removal).

Verification

One reader remains, and the forks cannot grow back silently:

$ grep -rn "PdfReader\|import pypdf" --include=*.py autobot-backend/ | grep -v tests
autobot-backend/media/document/extraction.py:148:        from pypdf import PdfReader
autobot-backend/media/document/extraction.py:153:        return PdfReader(io.BytesIO(raw))

$ grep -rn "PyPDF2" --include=*.py autobot-backend/ | grep -v "^.*:.*#\|docstring"
(only doc-comment references explaining the removal)

Tests — 40 passing across the new suite and the rewritten pipeline suite:

$ pytest media/document/ tests/test_document_extraction_canonical_13893.py -q
30 passed
$ pytest media/document/pipeline_test.py media/pipeline_constructor_compat_test.py \
         tests/test_document_extraction_canonical_13893.py -q
40 passed

Mutation-tested rather than merely re-run — the anti-fork guards were each
provoked to prove they can fail:

### MUTATION: reintroduce a PyPDF2 fork in the connectors
1 failed, 14 passed
### MUTATION: re-declare PyPDF2 as a dependency
1 failed, 14 passed
### RESTORED
15 passed

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):

base branch:  6 failed, 1349 passed, 63 skipped
this branch:  6 failed, 1349 passed, 63 skipped

Startup imports clean (352 passed); flake8 --max-line-length=120 exit 0; no
print(; closure gate check-new-module-callers.sh exit 0.

Test quality note: the old pipeline tests mocked PdfReader. That mock would
have 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.py and utils/document_extractors.py
no 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
DocumentParser uniquely supports (XLSX, PPTX, ODT, ODS, ODP, ODG). It was built
for 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 am
not 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.

@github-actions

Copy link
Copy Markdown
Contributor

✅ SSOT Configuration Compliance: Passing

🎉 No hardcoded values detected that have SSOT config equivalents!

@mrveiss

mrveiss commented Aug 10, 2026

Copy link
Copy Markdown
Owner Author

code-quality was red on 7 hardcoded-value violations in autobot-backend/api/knowledge.py — all of them pre-existing. git blame puts every one on an older commit, and this PR's diff in that file only touches lines ~1078–1120. The guard scans changed files, not changed lines, so editing a legacy file inherits its whole backlog.

Fixed rather than bypassed (Rule 6). Each substitution was checked to be behaviour-preserving:

line was now equal?
1915 limit: int = 10 QueryDefaults.DEFAULT_SEARCH_LIMIT 10 == 10
199, 558, 2354, 2756, 2758 "general" CategoryDefaults.GENERAL 'general' == 'general'
984 timeout=30.0 config.timeout.default_request 30.0 == 30.0

Two things worth recording:

An eighth instance the guard does not catch. Line 2835 (doc.get("category", "general") inside a set() comprehension) is the same defect in a shape the pattern misses — it was never reported. Fixed as well; 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, so a module-level import would be silently shadowed there. It sits outside the try so an ImportError surfaces as itself rather than as a URL-fetch failure.

Verification: test_document_extraction_canonical_13893.py 15 passed; ast.parse clean; zero "general" literals remain in the file.

…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.
mrveiss added a commit that referenced this pull request Aug 10, 2026
)

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.
mrveiss added a commit that referenced this pull request Aug 10, 2026
…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.
@mrveiss
mrveiss merged commit 0b913d1 into Dev_new_gui Aug 10, 2026
63 of 64 checks passed
@mrveiss
mrveiss deleted the issue-13893 branch August 10, 2026 19:00
mrveiss added a commit that referenced this pull request Aug 11, 2026
…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.
mrveiss added a commit that referenced this pull request Aug 11, 2026
…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.
@mrveiss

mrveiss commented Aug 11, 2026

Copy link
Copy Markdown
Owner Author

Follow-up filed from this PR: #14005 — the hardcoded-value guard misses literals inside comprehensions.

Line 2835 of api/knowledge.py (len(set(doc.get("category", "general") for doc in all_docs))) was never reported by the guard; I found it by grep while fixing the 7 it did report, and fixed it here. Reproduced the gap in isolation:

def a(docs): return len(set(doc.get("category", "general") for doc in docs))   # NOT reported
def b(req):  category = req.get("category", "general")                          # reported

Same literal, same rule — caught or missed by statement shape.

mrveiss added a commit that referenced this pull request Aug 11, 2026
…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>
mrveiss added a commit that referenced this pull request Aug 12, 2026
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant