Repository navigation
fix(document): document_parser and document_extractors still have zero callers after the consolidation #14333
Description
Part of #13892. Carries the two acceptance criteria left unmet when #13893 was closed.
State
#13893 (PR #13924) removed all five forked PDF implementations — that part shipped and is in base. But two of its ACs were explicitly flagged as unmet at merge time, and the issue was closed anyway:
- Directory-batch capability from
document_extractors.pyreachable - Every orphan wired to a live caller
Still true today:
$ git grep -n "document_parser|document_extractors" origin/Dev_new_gui -- '*.py' \
| grep -v 'utils/document_parser.py|utils/document_extractors.py'
origin/Dev_new_gui:autobot-backend/tests/test_document_extraction_canonical_13893.py:46: "utils/document_parser.py",
origin/Dev_new_gui:autobot-backend/tests/test_document_extraction_canonical_13893.py:47: "utils/document_extractors.py",
Both hits are string literals in the anti-fork guard's path list — not imports. utils/document_parser.py (311 lines) and utils/document_extractors.py (417 lines) delegate to the canonical extractor and are called by nothing.
What they carry that nothing else does
document_parser.py— format breadth the live paths lack: DOC, XLSX, PPT/PPTX, ODT, ODS, ODP, ODG, with a dispatch table and thread-pool offload.document_extractors.py— directory-batch extraction and format discovery.
Per the no-deletion rule these are wired in, not removed.
The blocking decision (unchanged from #13893)
Wiring DocumentParser in means letting the KB upload endpoint accept the formats it uniquely supports. ALLOWED_EXTENSIONS (api/knowledge.py:124) is {.txt, .md, .pdf, .docx, .json, .csv, .html} and has never included them, though the module's own docstring reads "Universal Document Parser for Knowledge Base" — it was built for this endpoint and never connected.
Options:
- Widen
ALLOWED_EXTENSIONSto the fullDocumentParserset. Real capability gain, wires the orphan for real. Cost: putsodfpy,openpyxlandpython-pptxon the untrusted-upload path — three more parsers reachable by anyone who can upload. - Keep the upload surface as-is and find the orphans a different live caller (a batch-ingest path is the natural fit for
document_extractors). - Decide the breadth is not wanted, and record that — at which case these modules need an explicit disposition, not indefinite limbo.
Recommend (1) for DocumentParser and (2) for document_extractors, but this is a product/security call.
Acceptance criteria
- Each module has at least one live, non-test caller — verified by grep for imports, not string occurrences
- Whichever option is chosen is recorded on this issue with its reasoning
- If (1): the new parsers are exercised against real fixtures through the upload endpoint
- No file deleted
Note on the closure
Recording this because #13893 closed with these ACs open. The consolidation genuinely shipped; only the wire-in is outstanding, which is why this is a fresh issue rather than a reopen.
Activity
mrveiss commentedon Aug 18, 2026
Decision (owner): wire via a batch-ingest caller — do NOT widen
ALLOWED_EXTENSIONSThe upload endpoint keeps its current format set.
odfpy,openpyxlandpython-pptxstay off the path reachable by anyone who can upload a file.Consequence accepted: the KB upload endpoint will still reject XLSX/PPTX/ODT/ODS/ODP/ODG. That capability remains unavailable to users through upload; it becomes reachable only through the batch path.
Scope now fixed:
document_extractors.py— wire its directory-batch extraction into a real batch-ingest caller. This is what the module was built for, so it is a genuine wire-in rather than a contrived one.document_parser.py— its format breadth reaches the batch path through the same route.Implementing.
fix(knowledge): wire the orphaned extractors into directory ingest (#…
github-actions commentedon Aug 18, 2026
PR #14532 (merged to
Dev_new_gui) references this issue with a close keyword.If this issue is fully resolved, close it manually. If work remains, no action is needed.
mrveiss commentedon Aug 18, 2026
✅ Closed — merged and verified in base.
PR #14532 → commit
8c87aeb18. The wire-in verified by grepping for imports, not string occurrences — which is how #13893 was mistakenly read as complete:Chain:
documents.py(live) →DocumentExtractor→DocumentParser. Both orphans reachable from a live, non-test path.Acceptance criteria:
Two live defects fixed on the way in:
add_document_from_filecould never read a PDF (read_text(encoding="utf-8")against a docstring claiming PDF support), andadd_documents_from_directorydefaulted topattern="*.txt"so a directory of PDFs ingested nothing and returnedtotal_files: 0.python-suiteconfirmed run, not skipped — all 12 shards success.mrveiss commentedon Aug 18, 2026
Closed by PR #14532 (
8c87aeb18, now in base). One correction for the record, so the next person reading this does not over-read the closure.The extractors do now have a live non-test caller — the AC is genuinely met — but it is
add_document_from_file, reached frompopulate_knowledge_base.py:131andfresh_kb_setup.py:113.add_documents_from_directorystill has zero live callers. Its only caller iskb_optimization_test.py:121, which is itself broken (it passes afile_extensionsparameter that has never existed and asserts on three keys the function does not return) and is markedintegration, so it does not run.So the
pattern="*.txt"default fixed by that PR was a correctness fix in code that is still dormant. Worth stating plainly here, because #14333 exists precisely because #13893's closure read as more complete than it was.Follow-ups filed: #14561 (the
.pptdeclared-but-unparseable suffix, and the brokenkb_optimization_testcall) and #14564 (a supported-but-corrupt file reporting as 'unsupported file type').