Repository navigation
Remove a dead constant, ground the rest in measurement - #22
Merged
Merged
Conversation
Follow-up to #21, which named the remaining constants but left several asserted rather than justified. Each one here was probed: perturb the value, run the suite, and diff the extracted text over a 161-message corpus captured from the tests. Zero corpus outputs change across this commit. Removed: - DETECT_MIN_CONFIDENCE could not affect anything. The gated branch and the fallback under it both returned `detected.encoding`, so every value of the threshold produced identical output — confirmed by enumerating the truth table and by the suite passing at 0, 0.4, 0.9, 0.999 and 2. Deleted, with the ungated behaviour stated where the gate used to be. - The three HTML marker windows (200 / 250 / 80) are one idea, and the closing tag lookahead, not the number, is what confines a marker to its block. Now a single HTML_MARKER_SPAN. The widest real span in the corpus is 184 characters (Outlook's inline CSS between "From:" and the address); all three windows are identical on every corpus input at any shared value from 200 to 400. The 80 was the least grounded: it fires on one input, with a 9-character match. Corrected: - The DETECT_MIN_CONFIDENCE rationale added in #21 was wrong. It claimed the bar was high enough to fall back to latin1 on an ambiguous file; that never happened, because the fallback returned the encoding the gate had rejected. - MAX_PDF_PAGES said it bounds parse work. The per-page deadline check already does that. What the cap actually buys is determinism — a declared page count is a fixed property of the file, so the same attachment extracts identically on a fast and a slow host. Restated; the value is unchanged. Named and justified with their measured bounds — the signature-rescue thresholds flagged as follow-up in #21: - QUOTED_BLOCK_RATIO (0.5) and SIG_VISIBLE_CHARS (400) are the two with a demonstrated bound on BOTH sides, so each comment now cites the case that fails below it and the case that fails above it. - SIG_BODY_LINES (2) and MORE_QUOTE_SPAN (200) are floors; nothing in the corpus constrains them from above, and the comments say so. - The three line lookaheads had an upper-bound argument that measurement does not support: raising ATTRIBUTION_WRAP_LINES to 50, DIVIDER_LOOKAHEAD_LINES to 1000 or HEADER_BLOCK_LINES to 1000 changes no output. Reworded to present the low side as evidence and the high side as judgement. Also recorded where a constant is pinned by a test that does not import it (STREAM_SLICE_UNITS, which fails at both 64 and 1 GB), and where a value has no test coverage at all (SNIFF_TEXT_RATIO passes at 0 through 0.95; DETECT_SAMPLE_BYTES passes at 4 bytes and at 1 GB). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CAP_STORAGE_SLACK existed only so the central trim could notice that a handler had overshot its cap, because the trailing-surrogate check was gated on that overshoot. That made a correctness guarantee depend on every incremental handler agreeing to store two units more than it can ever return. Check it unconditionally instead: a trailing high surrogate is half a character whether we cut there or a handler stopped there, and stripping it is right either way — left in place it serialises as U+FFFD. The docx reader can then store exactly maxOutputChars and the constant goes away. Truncation is still reported, from the handler's own capExceeded rather than inferred from length. Coverage is unchanged, not reduced: 'never splits a surrogate pair when the cap lands mid-character' now exercises the new path. With storage flush against the cap that document ends on a lone high surrogate WITHOUT overshooting, so the old gated check would miss it — verified by running the two changes separately, where zero slack against the old check fails exactly that test. This is a behaviour change, narrowly: a document that legitimately ends in an unpaired high surrogate now has it stripped rather than emitted as U+FFFD. That input is malformed in every format we read, and both answers lose the half — so this is a better failure, but it is a different one, which is why it is split out from the pure-cleanup commit before it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review caught a real regression in the previous commit: unifying the three HTML marker windows at 250 widened the foreign-verb scan from 80, and ordinary French prose then matches. "Paul a écrit une proposition détaillée ... Voici mon avis :" is a plain reply whose own later colon corroborates the verb, so everything below the greeting was cut away as quoted history. The evidence behind that unification was too weak to carry it, and the shape of the mistake is worth recording. The marker fires on ONE of 49 HTML inputs in the corpus, with a 9-character match — so the corpus had no power to detect the change, and "0 outputs changed" measured its own blind spot rather than safety. I stated that caveat on the method and then did not apply it to this decision. The three windows are not one number, because how far a scan may wander depends on how much evidence it is looking for. "On ... wrote:" needs a literal "wrote:" and the From: scan needs a full email address; ordinary prose supplies neither, so they can afford a wide window. The foreign-verb scan needs only a COLON, and its verbs are ordinary words — the window IS the guard there, and the closing-tag lookahead does not help, because the trailing colon is inside the same <div>. Restored as three named spans sharing the notPast helper, so the readability of the refactor survives without the behaviour change. All twelve compiled markers are now byte-identical to main, verified by comparing source and flags. Regression test pinned with a ~125-character gap — inside a 250-char window and outside the 80-char one — and confirmed to fail when the span is set back to 250. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
josephfeleke
approved these changes
Sep 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #21, which named the remaining constants but left several asserted rather than justified. Every constant here was probed rather than reasoned about: 134 runs that perturb one value, run the suite, and diff the extracted text over a 161-message corpus captured by instrumenting the two entry points — so each comment now cites the case that breaks below a value and, where one exists, the case that breaks above it.
Two constants are gone.
DETECT_MIN_CONFIDENCEcould never change its function's answer — the gated branch and the fallback beneath it both returneddetected.encoding, confirmed by truth table and by the suite passing at 0, 0.4, 0.9, 0.999 and 2.CAP_STORAGE_SLACKdisappears once the trailing-surrogate check is made unconditional instead of gated on overshoot, which also stops a correctness guarantee depending on every handler storing two units it can never return.Two earlier rationales were wrong and are corrected. The
DETECT_MIN_CONFIDENCEcomment added in #21 claimed a fallback to latin1 that never happened.MAX_PDF_PAGESclaimed to bound parse work that the per-page deadline check already bounds — what it actually buys is determinism, so the same attachment extracts identically on a fast and a slow host.A third removal was attempted, reverted, and is now a test. Collapsing the three HTML marker windows (200/250/80) into one looked safe — zero corpus outputs changed at any shared value from 200 to 400 — but review caught that widening the foreign-verb scan from 80 lets ordinary French prose (
Paul a écrit une proposition détaillée … Voici mon avis :) match its own later colon and delete the reply. That marker fires on one of 49 HTML inputs with a 9-character match, so the corpus had no power to detect the change; "0 outputs changed" measured its own blind spot. The three are now separately named spans sharing one helper, all twelve compiled markers are byte-identical tomain(source and flags compared), and the case is pinned by a regression test verified to fail at 250.The signature-rescue thresholds flagged as follow-up in #21 are now named and bounded (
QUOTED_BLOCK_RATIOandSIG_VISIBLE_CHARShave demonstrated failures on both sides;SIG_BODY_LINESandMORE_QUOTE_SPANare floors only, and say so), and the three line lookaheads lost an upper-bound argument that measurement contradicts. The commits are separable: the first is pure cleanup, the second carries one narrow behaviour change (a document legitimately ending in an unpaired high surrogate now has it stripped rather than emitted as U+FFFD) and can be dropped on its own, and the third is the regression fix. 530 tests, typecheck and build pass, with zero corpus drift against pre-auditmain.🤖 Generated with Claude Code
Open workspace in Conductor