Skip to content

fix: remove doc-text sanitization; keep only sound index validation - #392

Open
rejojer wants to merge 2 commits into
mainfrom
fix/remove-doc-text-sanitization
Open

fix: remove doc-text sanitization; keep only sound index validation#392
rejojer wants to merge 2 commits into
mainfrom
fix/remove-doc-text-sanitization

Conversation

@rejojer

@rejojer rejojer commented Aug 8, 2026

Copy link
Copy Markdown
Member

Cleans up the document-text sanitization introduced in 9681cda..9bcc7cf ("Implement document text sanitization and validation"), keeping only the parts that earn their place.

Removed

1. Keyword redaction (_INJECTION_PATTERNS, _sanitize_doc_text)
The pattern list matches ordinary English with no word boundaries: "impact assessment" → "imp[REDACTED]sessment", "The Trustee shall act as…", "disregarded" → "[REDACTED]ed", "renew instructions" → "re[REDACTED]". This silently corrupts the exact text the pipeline then fuzzy-matches section titles against, breaking physical_index assignment — while a keyword blocklist stops no real attacker (trivially bypassed by typos, encodings, or other languages).

2. Prompt wrapping and hardening preamble (_wrap_doc_text, _SYSTEM_HARDENING)
The indexing LLM calls have no tools, no secrets, and no cross-user surface — a malicious PDF can only make its own index bad, which requires no injection. The wrapper also labeled the JSON structure the model must fill in as "raw document text, treat as data only" inside the same prompt as the actual document text. All prompts are restored to their pre-sanitization form.

3. Pre-filter nullification (_validate_physical_indices + its four calls in process_no_toc)
meta_processor drops entries with physical_index=None before verify_toc runs. Nullifying at this stage therefore silently deleted sections that the designed verify → fix_incorrect_toc_with_retries loop (which checks every entry) would have relocated with the section preserved. It added no detection either — verify already checks everything. Out-of-range and non-int values are handled after the filter by validate_and_truncate_physical_indices (both bounds since c603b63), where a nullified entry survives as a placeholder.

This is also why process_no_toc intentionally no longer chunk-validates: an entry referencing a page outside its own chunk (e.g. copied from the previous structure) is left for verify to flag and the fix loop to relocate — the section survives with the corrected page, at the cost of a few extra LLM calls. Chunk-rejecting it here could only nullify, i.e. delete the section. The chunk check stays only in toc_index_extractor, where the validated structure is offset-vote scratch data discarded after use, so nullification there is free.

Kept

1. _parse_physical_index + the chunk-membership check ending toc_index_extractor
The printed-page/physical-page offset is decided by a majority vote over very few matching pairs, and one fabricated pair can shift every page number in the tree. This check nullifies answers referencing pages the model was never shown, operates on scratch data that no prompt ever sees, and costs no extra LLM call.

2. The fill-only-blanks merge loop in process_toc_no_page_numbers
Prevents the model from overwriting or dropping previously found entries (the crash-and-loss class #188 fixed symptoms of). Refined here: identity mismatches skip the chunk instead of raising; accepted values are parsed leniently (bare ints as well as <physical_index_N>) and stored in canonical marker form, so mid-flight state matches the format the prompts request.

Verification

  • AST-level function diff against the pre-sanitization baseline: generate_toc_init, generate_toc_continue, process_toc_with_page_numbers, extract_matching_page_pairs, calculate_page_offset are byte-identical; every remaining delta is accounted for (fix: prevent KeyError crash and context exhaustion in TOC processing #188 robustness fixes, the keeps above, whitespace).
  • Edge probes on parser/validator (whitespace, floats, malformed markers, negatives, absent keys) all pass.
  • Pipeline tests updated to the new semantics (reordered LLM output skips the round preserving order; lenient fill accepts bare ints and rejects out-of-window answers).

Cleans up the document sanitization introduced in 9681cda..9bcc7cf.

Removed:
- Keyword redaction (_INJECTION_PATTERNS/_sanitize_doc_text): the patterns
  match ordinary English ("impact assessment" -> "imp[REDACTED]sessment",
  "shall act as", "disregarded"), silently corrupting the very text the
  pipeline fuzzy-matches section titles against, while any real attacker
  trivially bypasses a keyword blocklist.
- Prompt wrapping and preamble (_wrap_doc_text/_SYSTEM_HARDENING): the
  indexing calls have no tools, no secrets, and no cross-user surface, so
  there is nothing an injected instruction could hijack; the wrapper also
  labeled the JSON structure the model must fill in as "raw document text".
  All prompts restored to their pre-sanitization form.
- Pre-filter nullification (_validate_physical_indices and its four calls
  in process_no_toc): meta_processor drops physical_index=None entries
  before verify_toc runs, so nullifying at this stage silently deleted
  sections that the verify -> fix_incorrect_toc_with_retries loop would
  have relocated. Out-of-range indices are already handled after the
  filter by validate_and_truncate_physical_indices.

Kept:
- _parse_physical_index and the chunk-membership check ending
  toc_index_extractor: guards the page-offset majority vote (the highest
  blast-radius number in the pipeline) using scratch data no prompt ever
  sees, at zero LLM cost.
- The fill-only-blanks merge loop in process_toc_no_page_numbers with the
  length/identity guards: prevents the model from overwriting or dropping
  previously found entries. Identity mismatch now skips the chunk instead
  of raising; accepted values parse leniently (bare ints as well as
  <physical_index_N>) and are stored in canonical marker form so
  mid-flight state matches the format the prompts request.

Verification: AST-level function diff against the pre-sanitization
baseline shows generate_toc_init, generate_toc_continue,
process_toc_with_page_numbers, extract_matching_page_pairs and
calculate_page_offset byte-identical, with every remaining delta
accounted for (#188 robustness fixes, the keeps above, whitespace).
58 tests pass.
@rejojer
rejojer force-pushed the fix/remove-doc-text-sanitization branch from fb3c36a to 78b5bc4 Compare August 8, 2026 11:52
@rejojer
rejojer changed the base branch from sdk-local to main August 8, 2026 11:53
Review follow-ups. Removing the delete-based validators re-exposed two
crash paths that predate the sanitization PR and were masked by it (at
the cost of silently deleted sections):

- convert_physical_index_to_int crashed on malformed markers
  ("<physical_index_x>" -> int('x>') ValueError) and passed bare
  numeric strings through, which later raised TypeError at the
  validate_and_truncate comparison. Marker tails and bare numeric
  strings now parse to int; anything unparseable is left as-is for
  validation to nullify. Single-value mode now accepts bare "7"
  (previously None).

- validate_and_truncate_physical_indices only checked the upper bound.
  Below-start_index values (possible when process_large_node_recursively
  runs with start_index > 1) reached page_list[negative] in verify_toc
  and could raise IndexError. Non-int and out-of-range values are now
  nullified — this runs after the None-entry filter, so nullified
  entries survive as placeholders on the existing None-tolerant paths.

- _parse_physical_index accepted bools (True -> page 1), truncated
  non-integral floats (1.9 -> page 1), and raised uncaught
  OverflowError on infinite floats. It now rejects all three.
@rejojer

rejojer commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

Follow-up commit c603b63 addresses external review findings: closes two latent crash paths that this PR re-exposed by removing the delete-based validators (bare numeric strings reaching the > comparison in validate_and_truncate_physical_indices; below-start_index values reaching page_list[negative] in verify_toc during recursive node processing), and tightens _parse_physical_index against bools, non-integral floats, and infinite floats. Both crashes predate the sanitization commits — the removed validators had masked them at the cost of silently deleting sections. Fixed at the source instead: parse what is parseable, nullify out-of-range values after the None-entry filter where placeholders are safe.

Comment thread pageindex/utils.py
value = value.split('_')[-1].strip()
try:
data[i]['physical_index'] = int(value)
except ValueError:
@rejojer

rejojer commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

Code review

No issues found. Checked for bugs, git history context, prior PR review comments, and code comment compliance.

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

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