Skip to content

fix(formats): reject inspection-only formats before doing the work - #39

Open
mazzasaverio wants to merge 1 commit into
ma2za:mainfrom
mazzasaverio:fix/inspection-only-routing
Open

fix(formats): reject inspection-only formats before doing the work#39
mazzasaverio wants to merge 1 commit into
ma2za:mainfrom
mazzasaverio:fix/inspection-only-routing

Conversation

@mazzasaverio

Copy link
Copy Markdown
Contributor

Summary

Three rough edges in the new 0.3.0 format plumbing, all reachable from normal use. None of them changes what the adapters extract; they change what happens on paths that cannot work.

1. process_file on a PDF or Office document did all the work, then failed obscurely. _processing_adapters returned the inspection adapter as both input and output adapter, so the engine extracted the document, ran detection over every block, and only then hit render(), whose AdapterContractError was swallowed by _render_document and re-raised as:

AdapterExecutionError: output adapter failed during rendering

Nothing tells the caller that these formats are inspection-only, or that inspect_file is what they want. Now the format is rejected up front:

UnsupportedFormatError: pdf supports inspection only; use inspect_file or supply custom adapters

2. inspect_file(..., encoding=...) silently ignored the encoding for PDF and Office. Elsewhere the engine is strict about meaningless combinations (custom adapters cannot be combined with format or encoding), so this now raises rather than pretending the argument had an effect.

3. BuiltinFileAdapter accepted the new binary formats. FileFormat gained PDF/DOCX/XLSX/PPTX, but the adapter's dispatch chain ends in an unconditional else that parses CSV, so BuiltinFileAdapter(FileFormat.PDF).extract(path) tried to decode a PDF as UTF-8 text and died with UnicodeDecodeError. It now refuses at construction. The engine no longer routes those formats there, so this is defence in depth rather than an active bug.

A shared INSPECTION_ONLY_FORMATS frozenset in formats.py is the single place that lists them, so adding a format to the enum no longer requires remembering three separate call sites.

Note on the two updated tests

test_process_pdf_fails and test_process_docx_fails asserted the old message, so they now assert the new one plus the absence of a partial output file. The exception type changes from AdapterExecutionError to UnsupportedFormatError on that path; both derive from PseudonymizeError, so except PseudonymizeError callers are unaffected, and this surface shipped hours ago in 0.3.0.

Verification

  • Tests use synthetic data only
  • Ruff, mypy, and pytest pass (218 passed, coverage 99.62%, mypy clean on 70 files)
  • Documentation and changelog are updated when required
  • Security and privacy impact has been considered (failing before extraction means a document that cannot be rewritten is never read into memory or processed)
  • No compatibility shim was added for an alpha-only API without an explicit roadmap reason
  • The base package remains dependency-free and network-denied by default

Related to #38, which covers the larger question of which document regions the adapters read.

🤖 Generated with Claude Code

process_file returned the inspection adapter as both input and output
adapter for PDF and Office documents, so the engine extracted the whole
document and ran detection over every block before render() failed and
was reported as a generic 'output adapter failed during rendering'.
Nothing indicated that these formats are inspection-only. They are now
rejected up front with UnsupportedFormatError naming inspect_file.

Two related edges: inspect_file silently ignored 'encoding' for those
formats, and BuiltinFileAdapter accepted them and fell through to the
CSV branch, decoding binary documents as UTF-8 text. Both now raise.

A shared INSPECTION_ONLY_FORMATS set keeps the list in one place.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mazzasaverio
mazzasaverio requested a review from ma2za as a code owner August 22, 2026 12:26
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