fill: fix label-collision silent overwrite + pikepdf handle leak - #1
Conversation
Two verified audit fixes on the shared engine's hottest fill path, both backward-compatible (four form repos pin this by git tag). 1. HIGH — label-collision silent overwrite (fill_via_mapping.py). The label-keyed fill dict let two different field_ids resolving to the same widget label overwrite each other (last wins); only one value reached the PDF, yet `resolved`/`coverage` counted both. Now: detect the collision, make `resolved` truthful (subtract shadowed field_ids), and surface the dropped value via an additive `label_conflicts` key. No collision => shape and counts unchanged. Last-write-wins byte output is preserved. 2. MED — pikepdf handle leak (field_split.py:split_to_copy). Converted the bare pikepdf.open() to a context manager so the handle is released even on error — matters in the long-lived MCP server every sibling repo runs. Regression tests for both; the _wrap_across_widgets gap is recorded as a strict xfail follow-up (a clean fix would reorder text out of reading order). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1339f7ea6c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if prev_fid is not None: | ||
| shadowed_fids.add(prev_fid) | ||
| field_data[label] = v | ||
| label_owner[label] = fid |
There was a problem hiding this comment.
Track all same-value claimants per label
When one widget label is claimed by A and B with the same value, this assignment makes B the only owner even though A is still counted as resolved. If a later field C writes a different value to that label, the conflict path shadows only B, so resolved_written still includes A even though the final PDF no longer contains A's value; the reverse sequence (A=x, B=y, C=x) similarly undercounts A after its value is restored. Please track all field_ids/value claimants for a label rather than a single owner.
Useful? React with 👍 / 👎.
Part of the 2026-07-06 suite-wide audit. Two verified fixes on the shared engine's hottest fill path. Both are backward-compatible and additive — the four form repos (court @v0.5.0; probate/corp/tax @v0.4.0) pin this engine by git tag, so nothing ships until a new tag is cut.
1. HIGH — label-collision silent overwrite (
fill/fill_via_mapping.py)Finding. The fill dict handed to
fill_formis keyed by widget label, but it is built while iteratingfield_id → value. Two different field_ids that resolve to the same widget label collide.Failure scenario. A mapping where
name_fieldandaddr_fieldboth point at one widget label (a real shape when a schema reuses a label across field_ids). Both resolve from the case, but the second write silently overwrites the first — only one value reaches the PDF. Meanwhileresolvedcounted both, socoveragereported1.0on a fill that left a box holding the wrong value and dropped the other entirely. The data loss is invisible to every downstream consumer.Fix.
resolvedtruthful: subtract field_ids whose written value was entirely shadowed by a later collision. With no collisionsresolvedis unchanged (== res["resolved"]), so existing consumers see no drift.label_conflictskey (label,kept_field_id/kept_value,dropped_field_id/dropped_value), present on both thecourtandtaxresult styles, and only when a conflict actually occurs.Not routed through
field_split.py: that machinery handles the inverse case (one field_id, many appearances) by detaching an appearance. Here two distinct field_ids carry genuinely conflicting values for one box, so there is nothing to split — the right move is to report it.2. MED — pikepdf handle leak (
fill/field_split.py,split_to_copy)Finding / scenario.
pikepdf.open(str(src_pdf))was opened without a context manager, leaking one file handle per fill. In the long-lived MCP server every sibling repo runs, these accumulate until exhaustion.Fix. Wrapped in a
withblock so the handle is released even ifsplit()/save()raises.Tests
test_label_collision_makes_resolved_truthful— collision →resolveddrops,coveragehalves,label_conflictsrecords the dropped value.test_label_collision_same_value_is_not_a_conflict— identical value on a shared label is not flagged and does not undercount.test_no_collision_leaves_resolved_and_result_shape_unchanged— backward-compat guard for the tag-pinned consumers.test_split_to_copy_closes_the_pdf_handle— asserts the openedpikepdf.Pdfis closed on return._wrap_across_widgetsgap (audit finding Fix __version__ drift by deriving from package metadata #3) is recorded as a strict xfail follow-up, not fixed: a wide word that jumps ahead leaves the skipped widget blank, but packing a later narrow word into it would reorder text out of top-to-bottom reading order — a design decision, not a clean bugfix.Baseline: 123 passed. After: 127 passed, 1 xfailed.
Recommended follow-up
Version skew across consumers: court pins v0.5.0 while probate/corp/tax pin v0.4.0. Cutting the tag that carries this fix is a good moment to converge them so the collision/handle-leak fixes land everywhere.
🤖 Generated with Claude Code