Review debt on merged PRs, and the two structural causes behind it

[lamemustafa]

Eleven review findings are open on already-merged code. This issue aggregates them so they stop being invisible, and records the two structural causes, both measured rather than asserted.

The debt

PR	open	what
#303	3	P1 restrict X preservation to real mask shapes · P2 uniqueness on downstream party keys · P2 decode pdftotext output as UTF-8
#305	2	P2 preserve POSIX output symlinks during replacement · P2 keep staged replacements inside the rollback
#306	4	P1 exclude the unverified case direction · P1 remove NFC from the active Phase 2 prompt · P1 remove the surviving fingerprint prescription · P2 plan cites the wrong sections
#307	1	P2 mark composed transformations as unverified
#312	1	P2 restore distinct wrong-tails per header field

In flight: #314 covers #306's four. #316 covers two of #303's three; the third is recorded in the test file as a measured limitation with its reproduction, because a correct fix needs the fabricator to be uniquely decodable after whitespace removal and that is a redesign, not a guard.

Now answerable with evidence that did not exist when it was raised: #307's composed-transformations wording. The master-binding lane has since measured eight composed variants against licensed 7.1 — all matched — plus a poisoning test showing an unmeasured axis (en dash) still refuses however much measured folding surrounds it.

Not started: #305's two, #312's one.

Cause 1 — merging before the reviewer had answered

The direct cause, and it is entirely a process failure on the merging side.

PR	merged after opening	threads that arrived after the merge
#309	2.4 min	0 — no review had run at all
#312	5.8 min	1 — 3 seconds after the merge
#300	6.1 min	4
#304	7.6 min	2

The pre-merge check counted unresolved review threads, got zero, and read that as reviewed clean. On a six-minute-old PR it means not reviewed yet. Codex takes roughly six minutes.

Fixed. The gate now requires positive evidence that a review names the current head SHA. Note the subtlety that cost a second iteration: Codex submits a review object only when it has findings — a clean pass produces no object, only a 👍 reaction and an updated summary comment naming the commit. A gate that waits for a review object deadlocks every clean PR. The signal is the summary table plus the reaction.

Cause 2 — changes and files large enough that a correction cannot reach the whole claim

Merged PR sizes from #280:

#280  3504 lines  13 files
#311  1350 lines  19 files
#291  1089 lines  13 files
#303   884 lines   5 files

And the files that keep needing correction:

2796  docs/tally/TALLY_PROTOCOL_REFERENCE.md
1893  scripts/bank_statement_import.py
1482  scripts/bank_statement_import.test.py
1392  docs/tally/IMPLEMENTATION_GUIDE.md
 989  docs/tally/PROMPT_PLAYBOOK.md

Size alone does not predict findings — #306 was 244 lines and drew four. What size predicts is a correction failing to reach everywhere the claim lives, which is the single most repeated defect in this set:

• the NFC withdrawal reached §9.4b's table and item 5 of the playbook, and left step 4, the adversarial review prompt, and IMPROVEMENT_PLAN_2026H2.md §8.6 still prescribing normalisation;
• the case-direction qualification was written into a note while the predicate beside it stayed symmetric;
• §3.4 kept the fingerprint alive as duplicate prevention while §3.4a of the same patch withdrew it;
• the _key Unicode fix landed in the importer and never reached the sanitiser, where getting it wrong publishes data.

A 2,796-line document that corrects itself in four places is a document where "fix the section the finding points at" is reliably insufficient.

Proposed

1. Work the debt down — in flight above; fix(scripts): an ACH reference can wrap, and a Windows overwrite keeps its ACL #305 and fix(fixtures): regenerate both bbox captures, clearing the X-run defect #312 next.
2. Keep the head-SHA gate, and write it down rather than leaving it in one session's habits.
3. Split TALLY_PROTOCOL_REFERENCE.md. It is the file every correction has failed to fully reach. Splitting by topic would make "grep the whole claim" tractable.
4. Smaller PRs. One finding or one behaviour per PR, so a post-merge finding lands against a diff small enough to re-read.
5. Standing habit: when correcting a claim, grep the whole docs/tally/ tree for other statements of it before opening the PR — and report what was found even where nothing changed.

🤖 Generated with Claude Code

[lamemustafa]

The rectification queue has progressed, with root retaining single-writer publication/merge authority:

• Rectify output replacement recovery and header evidence #320 repairs output ownership, rollback metadata, hard-link checks and cleanup diagnostics. A fresh new-output pathname-identity P1 is being fixed before acceptance.
• Rectify ambiguous mask preservation and recovery guidance #316 repairs ambiguous short-mask preservation and now makes Journal unknown-outcome guidance read-only in the MCP description, generated warnings and README.
• docs(tally): four findings left open when #306 was merged too early #314 separates measured protocol observations from mutation policy. Its runtime binding prerequisite is tracked in Bug: folded ledger binding lacks qualified scope #329 and implemented in Rectify unqualified folded ledger binding #331, stacked on Rectify catalogue candidate precision and lookup work #290.
• The remaining downstream sanitizer equality/uniqueness gap has a concrete generation-gate fix in Rectify sanitized statement party equivalence #332, closing Bug: sanitizer does not preserve parsed party equivalence #330 after its Rectify ambiguous mask preservation and recovery guidance #316 prerequisite. It compares complete parsed party classes and refuses incomplete/sentinel evidence; existing captures stay unchanged.
• Rectify incomplete merge-gate evidence #321 has current-head regression/CI evidence and a further batch for complete protected-check, metadata and privacy evidence; review findings are being dispositioned against the actual pushed heads.

This umbrella remains open until these reviewed changes land. Live/source completeness and missing Windows interactive evidence are not replaced by local tests or feature-base cleanliness.

[lamemustafa]

Queue update: six PRs have landed after current-head review, required CI, independent acceptance and immediate head/base checks: #290, #310, #315, #326, #327 and #334. The current master tip is ba1742cf0ec8f17c3f10d537a5bfa845739b946f.

• Rectify incomplete merge-gate evidence #321 is published at 7ea1235396bb1ddf2cab0e9f5cdda079c65a2f17 on that master. Its last eight threads are dispositioned, with the production-regression branch/label responsibility explicitly retained in independent review. All 118 controls passed on the preceding immutable gate batch; the final PowerShell/SQL extension passed its focused controls. The current provider review and hosted full suite are pending.
• Rectify output replacement recovery and header evidence #320 has 124 passing local controls and a passing Linux workflow at 8f922b994b2e2675293de7f4f1ecfa267f16e202. The completed provider review found four additional recovery issues; fresh registration interrupts, typed path refusal, staged descriptor-close reporting and restrictive-umask behavior are being repaired before publication. Current master has been integrated locally.
• Rectify unqualified folded ledger binding #331 remains held for the actual native source-draft/catalogue DTO capture. docs(tally): four findings left open when #306 was merged too early #314 and Rectify ambiguous mask preservation and recovery guidance #316 depend on that binding acceptance; Rectify sanitized statement party equivalence #332 follows Rectify ambiguous mask preservation and recovery guidance #316.
• Decide which proposed vouchers are already in the book #294 and Identify a voucher by the marker Bridge wrote into its narration #308 retain their nonempty live-completeness holds. Restored endpoint connectivity does not establish source completeness. Platform-specific qualification gaps remain explicit.

This issue remains open. Passing local tests and provider completion are inputs to acceptance; neither closes unresolved findings or replaces the missing live evidence.

[lamemustafa]

#305's two items are done — both fixed and both tested. Verified against origin/master rather than inferred, because the PR threads are still open and read as outstanding.

"Preserve POSIX output symlinks during replacement" — implemented and covered.

The staging sibling and the final rename both target os.path.realpath(path), not path (scripts/bank_statement_import.py:2555 and :2788), so a symlinked destination is written through to its target instead of being replaced by a regular file. Covered by test_write_outputs_writes_through_a_symlinked_destination (bank_statement_import.test.py:1453), whose docstring gives the same reasoning the finding did.

"Keep staged replacements inside the rollback boundary" — implemented, covered, and the specific scenario is now unreachable.

test_a_failed_swap_rolls_back_every_staged_replacement covers the rollback. And the example the finding used — --out an existing file, --manifest an existing directory — no longer reaches the replacement stage at all: preflight refuses it with output_not_regular: … an existing output must be a regular file. I confirmed that by constructing exactly that case and watching it refuse before anything was written.

So both threads on #305 can be resolved, and the table above is two rows shorter. I have not resolved them myself since they are not on a PR of mine.

How I nearly got this wrong, since it is the same failure this issue is about. I started writing both tests, having grepped the test file for symlink and found nothing. The grep was correct; my head -12 was not. The pattern also contained .part, which matched .party dozens of times earlier in the file, and the cap consumed the output before the symlink matches appeared. A capped view of a search is not a search result — the same shape as a gate that reports a cap as a total, which #413 fixed this morning.

Running the tests I had written is what caught it: one printed a refusal proving its own premise impossible, and the other duplicated an existing test verbatim. Neither was committed.

Remaining on this issue by my reading: #312's one item. #303's third is recorded as a measured limitation rather than a fix, and #307's is answerable on evidence gathered since.

[lamemustafa]

The debt table is now empty. Every one of the eleven findings is dispositioned, and the last not-started item turned out to be covered by a control nobody had linked to it.

PR	was	now
#303	3	2 fixed in #316; the third recorded as a measured limitation with its reproduction (a correct fix needs a uniquely-decodable fabricator — a redesign, not a guard)
#305	2	both verified done against master; one scenario is now unreachable, refused at preflight by output_not_regular
#306	4	covered by #314
#307	1	answerable on evidence gathered since — eight composed variants measured against licensed 7.1, all matched, plus a poisoning test showing an unmeasured axis still refuses
#312	1	dispositioned below — #431

#312's item: correct finding, coverage already present, nothing pointed at it

The finding is right on its facts. require_account_match(pages, bank, "HDFC CA xx1111") does not discriminate between header lines: the sanitised Cust ID (111111111) and IFSC (1111111) both end 1111.

What it asked for — "structurally faithful negative cases for each affected header line" — already exists as test_real_hdfc_capture_binds_the_account_no_geometry_only, which pins the selected label geometry and repoints account_anchors at each neighbouring header line. Measured by regressing the production anchors:

regression	what fails
("Cust", "ID")	production refuses ambiguous_account_match
("RTGS/NEFT", "IFSC")	the negative cases stop refusing
("MICR",)	the capture test fails
all three	the geometry test fails its label assertion

Caught three independent ways, none of them the positive check the finding names — which is precisely why it reads as unaddressed while the behaviour is covered. #431 adds the pointer so it is not re-raised.

On this issue's own two causes, measured again today

Cause 1 — merging before the reviewer answered. The gate holds. Four PRs merged today (#426, #427, #430, and #425 earlier) each carried an independent review naming the current head, and each review found something:

• Stop naming a reclaimed path once the created inode is gone #426 — an assertion message claimed two _fd_identity calls prove which branch ran. The retry is unconditional; both branches reach two.
• Close the duplicated descriptor if the handoff never completes #427 — the guard double-closed on exactly the scenario it was written for: os.fdopen is io.open, which closes the descriptor on its own error path, so the operator's KeyboardInterrupt arrived as Bad file descriptor. A real defect, in a fix for a real defect.
• Cover the staged-digest pin inspection with an injected fstat failure #430 — "the arm is consumed on first use, so it cannot drift" was false; cleanup re-installed it.

Cause 2 has a sharper shape than "changes too large". In all three the code was serviceable and the claim about the code was wrong — comments, assertion messages, PR bodies. Those are exactly the artefacts nothing executes. The pattern that matters:

A control written for the bug you were told about does not test the change you actually made.

#427 is the clean example. The leak test's fake fdopen raised without touching the descriptor — the half of the failure space a real late io.open failure never produces — so it passed identically with and without the double close. And when review then asked for _defer_sigint_during_claim(), replacing that wrapper with a null context left the whole suite green. The headline change of that revision was pinned by nothing until someone thought to neutralise it.

Cheap countermeasure, applied to all four merges: before claiming a change is safe, delete or neutralise the line that makes it safe and confirm the suite goes red. It cost minutes and caught two unpinned controls that reviewers had not asked about.

Leaving this issue open: the debt is cleared, but Cause 2 is a standing hazard, not a fixed one, and this is where its evidence is accumulating.

[lamemustafa]

Item 3 (split TALLY_PROTOCOL_REFERENCE.md): measurements and a proposed shape, not implemented

Measured on master d2c65d88 on 2026-09-17. Recorded here so that whoever does the split starts from numbers rather than re-deriving them. Nothing below is implemented.

Where the size and the churn are

The file is 3,256 lines. git blame author dates, by top-level heading, over the last 14 days:

section	lines	lines changed in the last 14 days
§9 Writes (import)	1,106	907
§9.10 Company creation (a ## heading, with 9.10a–d, 9.11d and 9.12–9.12b under it)	557	323
§9.11 Company pinning	111	29
§8 FETCH semantics	226	142
§5 Dates	229	124
§12a Bill-wise	383	60
§3 Request template	72	39
§0–2, 4, 6, 7, 10, 11, 11a, 11b, 12, 13, 14 and 1.2	546 combined	6

The write material, §9 plus the §9.10 and §9.11 blocks, is 1,774 lines, and 1,259 of them changed in 14 days. It is also where this issue's "a correction does not reach everywhere the claim lives" findings happened. The rest of the document is largely static.

Order problems a split should fix, independent of size

• ## 1.2 (modal dialog blocks the gateway) sits at line 1902, between §9.6 and §9.11.
• ## 9.11 comes before ## 9.10, and ### 9.11d sits under §9.10.
• Within §9 the order is 9.5, 9.8, 9.13, 9.9, 9.7, 9.6.
• ## 11b comes before ## 11a, which comes before ## 11.

What depends on the single file

• scripts/check-protocol-section-numbers.mjs (and its test) reads headings from this one path, including git show <ref>: of it. A split must teach the gate to read the set of files, or the duplicate-number guarantee silently stops covering the moved sections.
• 16 Markdown files and 36 code, script or config files name the document. At least 49 code references have the form "TALLY_PROTOCOL_REFERENCE… §N". Section numbers are the stable address, so a split must keep every number unchanged.
• The file is pinned on the compatibility surface. Splitting it changes the pin set and needs MAX_SURFACE_FILES reasons for the new parts.

Proposed shape (for the owner to accept or change)

1. Keep TALLY_PROTOCOL_REFERENCE.md as a short index mapping each § number to its file, so all 52 referring files still resolve "§9.3" in one hop and no code comment needs editing.
2. Parts along existing number families, numbers unchanged:
   • §0–2 environment and transport
   • §3–4 requests
   • §5 dates
   • §6–8 reads
   • §9 writes, split further: 9.1–9.2 response semantics; 9.3, 9.5–9.7 identity and idempotency; 9.4–9.4d masters; 9.8, 9.9, 9.12–9.13 voucher shapes; 9.10–9.11d company; plus §1.2 moved beside §6
   • §10–12 scale and verification
   • §12a bill-wise
   • §13–14
3. The section-number gate reads every part and fails on a duplicate across parts. Add a test that a number defined in two different parts is caught.
4. Generate the split with a script from a named master SHA, in a quiet window for the write lanes. A branch that edited the old file then re-runs the script instead of hand-merging.

Why not tonight

Several overnight lanes are editing §9 right now: 1,259 changed lines in 14 days, and open PRs touch the compatibility JSONs. A 3,000-line move would force every one of them into a conflict. The pin question (pin every part, or the index plus the parts that hold qualification evidence) is also the owner's call under the seal criterion.

🤖 Generated with Claude Code