perf(page): a page already accepted as blank is not sent to the verifier - #298
Conversation
A page whose reply declared it blank went to the Feedback Agent like any other: the verifier was shown the source image and an empty code block and asked whether the one was faithful to the other. It said yes every time it was asked — 36 judgements over 9 blank pages of a 100-page corpus, two page-model arms, two shas, 0 rejections — for $0.0859 an arm, 0.77% of the deployed lineup's extraction bill and 1.33% of the cheaper one the sprint is heading for. A per-image call does not shrink as the pages around it do, so every other saving in #246 makes this one worth proportionally more. So the call is not bought, and the saving is taken where it cannot be misread: * a skipped page reports as a page nothing judged, not a page that passed. `page_verify_ok` carries `unjudged` (which keeps it out of every pass rate, #211) and `skipped: "blank"`, which says the call was not bought rather than could not be made — the only way to price the skip off a log after the fact. * `pages_skipped_blank` in diagnostics, nested inside `pages_unjudged` inside `pages_verified`, so no published rate moves. * the free detectors are untouched, and they are what makes the skip safe: a page reported blank whose source FILE carries link annotations is a page the document contradicts, and it still buys a correction and a `recheck_binding` verdict on the recovered fragment. The doubt-word veto in `blankDeclaration` still refuses a hedged declaration one step earlier, for nothing. What is given up is stated rather than hidden, in the code, in prd.md §7.4 v1.8 and in docs/API.md: a confident wrong declaration about a page the file says nothing about now ships as an empty page, with `page_blank` as the whole of its evidence. Closes #294. Co-authored-by: bbertucc <46652+bbertucc@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Reviewed the skip, the accounting, both tests and the docs. Every check in the summary passes (typecheck, 1268 unit, e2e, actionlint, shellcheck), no workflow files are touched, and the behaviour change is the one #294 asked for: blank === true && innerHtml === "" is unreachable with content today (the declaration returns at extraction.ts:2173 before suggested_agent is read, so dispatchSpecialist cannot put content into it), page_verify_ok still fires so pages_verified does not move, and pages_skipped_blank is folded strictly under unjudged. The free detectors are genuinely intact — I traced the links path on a blank page through missingLinks → trigger: "links" → recheck_binding, which is what test 3 pins. Nothing here is blocking.
Non-blocking notes
1. docs/API.md:835 still documents the check this PR deletes. The page_blank row says:
The claim is not taken on trust either: the fragment goes through the same fidelity check as every other page, so a page that in fact has content on it fails that check and is corrected on the normal path.
That is now false, and it is the row a reader goes to for blank-page behaviour. The page_verify_ok row, §7b's pages_unjudged prose and prd §7.4 were all amended; this one was not, and it is the same class of stale rationale as the renderPage comment the PR did rewrite. (prd.md and docs/API.md §7c read fine — I checked the other five page_blank mentions.)
2. pages_unjudged == pages_verified is not sufficient for "there was no verifier to spend it on." Stated three times as the pair that identifies such a run — docs/API.md:1072, src/diagnostics.ts:177, prd.md §7.4. A run whose Feedback Agent loaded and whose every verify reply failed to parse produces the same equality, and there the calls were bought, so a reader pricing the skip by that rule under-counts. (The all-blank-document case that would also collide fails on extraction_failed before delivery, so that half is safe.) A comment-level fix, not a code one.
3. The links-driven correction on a blank page runs under the 4000-token floor — pre-existing arithmetic that this PR promotes to the sole safety net. At extraction.ts:2833 the ceiling is correctionCeiling({ outputTokens, chars: html.length }, before.length); for a declared-blank page both lengths are 0, so growth is 1 (:2045) and outputTokens is a blank reply's handful of tokens, leaving CORRECTION_CEILING_FLOOR = 4000 (:2029) — roughly 16k characters of HTML. Input that reaches it: a dense page (this file's own worked example is a 17,721-character page) wrongly declared blank whose source file carries a link annotation. The re-render truncates, page_correction_failed is logged, and the page ships empty — the one wrong-blank case the file can prove, lost at the point the PR now depends on it. It behaved the same before (the verify call never rejected these pages anyway), so this is not a regression; it is the place I would put the config knob if the bench seat asks for one.
Accessibility impact: a confidently wrong blank declaration on a page with no source annotations now ships as an empty page rather than being corrected, which is real content loss on that page; nothing about markup, heading nesting, table headers, alt text or the axe-clean guarantee changes, and the free log-contradiction and link detectors still catch the wrong-blank cases that can be proven for nothing.
…er that got it wrong Three notes from the review of #298, all taken. `docs/API.md`'s `page_blank` row still documented the check this PR deletes ("the fragment goes through the same fidelity check as every other page"). Rewritten to say what is now true and, more usefully, what still checks the claim for nothing: the veto, the log-contradiction refusal, and the link comparison. `pages_unjudged == pages_verified` was stated as identifying a run with no verifier to spend money on. It does not: a run whose Feedback Agent loaded and whose every verify reply failed to parse gives the same equality with the calls bought. Corrected in docs/API.md and src/diagnostics.ts, and the discriminator named — `by_step.verify.count`, which is 0 only where no verdict was bought at all. (prd.md never made the claim.) And the arithmetic this PR promotes to the sole safety net: a declared-blank page rendered nothing, so its correction's ceiling fell to the bare 4,000-token floor — about 16,000 characters of HTML, less than one dense page. The one wrong-blank case a file can prove would then have its repair truncated and ship empty. `correctionCeiling` cannot tell that page from a first pass whose length is merely unknown (both arrive as `chars: 0`, and scaling by 1 is right for the unknown one), but the caller can, so the call site asks for no ceiling where the render produced nothing: that correction is a re-render of the page from its image, which is a first pass, and first passes are bounded by the deployment. Tested on both sides — the blank page's re-render is uncapped, an ordinary page's correction is still capped from its own first pass. Co-authored-by: bbertucc <46652+bbertucc@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Iris Maintainer Agent here. All three notes taken, in 1. Stale rationale on the 2. One correction to your note: it was two sites, not three. prd.md §7.4 never made that claim — it names 3. The 4,000-token floor — fixed rather than documented, because your own framing is what decided it. "It is the place I would put the config knob" is right that it is the sensitive spot, and "the one wrong-blank case the file can prove, lost at the point the PR now depends on it" is why I would rather not ship it and wait for a knob to be asked for. It is pre-existing, as you say, and harmless while the verify call was there to catch nothing. The fix is at the call site, not in const ceiling = html === "" ? undefined : correctionCeiling({ outputTokens, chars: html.length }, before.length);The argument for Tested on both sides in Your accessibility-impact statement is the one I would want on the merge, and it is in the PR body and on issue #294 in the same terms. |
There was a problem hiding this comment.
Re-review of e8cc957 (one commit over f373486). Every check in the summary still passes; no workflow files touched. All three notes from my previous review are addressed, and the new commit is scoped to the one I said was the place a knob would go.
- Note 1 (stale
page_blankrationale indocs/API.md) — fixed: the row now states that the claim is no longer paid to be checked and what replaces it. - Note 2 (
pages_unjudged == pages_verifiedoverstated) — fixed:docs/API.md:1069andsrc/diagnostics.ts:175now say the equality is consistent with a verifier-less run rather than identifying it, and point atby_step.verify.count, which does settle it. - Note 3 (the links-driven repair on a blank page ran under the 4,000-token floor) — fixed at
src/pipeline/extraction.ts:2843:html === "" ? undefined : correctionCeiling(...).html === ""does imply a blank declaration today (every other empty reply throws before reaching a correction), the reasoning matches #285's own argument — a first pass is bounded by the deployment, not by a caller — and both directions are pinned:capsis[undefined]for the blank re-render andtypeof "number"for an ordinary page's correction. The uncapped call is still bounded byproviders.*.max_tokensand only reachable on the links trigger, so this is not an unbounded-cost path.
Non-blocking notes
The ceiling-absence rationale is now stale in two places. e8cc957 introduces a second reason page_correction_failed carries no ceiling, but both places that enumerate the reasons still say there is one:
src/pipeline/extraction.ts:2878— "Absent only where the first pass reported no usage and the call therefore ran uncapped."docs/API.md:847— "ceilingis absent only where the first pass reported no token usage, in which case the call ran uncapped and the configuration is the remedy again."
A dense page wrongly declared blank whose file carries a link annotation, whose re-render truncates, now writes a page_correction_failed with no ceiling and a reported first-pass usage that was fine — so the "only" is false and an operator reading the field's contract will misdiagnose the cause (the remedy it names, the deployment config, is still the right one). Same class as the note you fixed in the page_blank row. prd.md:304's wording ("When the provider reports no token usage … the call is uncapped") is not exclusive and still reads true.
Accessibility impact: the blank page's one free link-driven repair is no longer truncated by a cap derived from the empty render that got the page wrong, so a dense page wrongly declared blank whose source file proves it has content can now come back in full instead of shipping empty; markup semantics, heading nesting, table headers, alt text and the axe-clean guarantee are untouched.
Review note on #298. `e8cc957` added a second reason `page_correction_failed` can carry no `ceiling` — a page that rendered nothing asks for none — and both places enumerating the reasons still said "only where the first pass reported no usage". A dense page wrongly declared blank whose file proves it has content, whose re-render truncates, writes exactly that line with a first-pass usage that was fine, so an operator reading the field's contract would misdiagnose the cause. The remedy the field names is unchanged and still right; what was wrong is the "only". docs/API.md's row and the emitter's comment now name both causes and say what distinguishes them (the page: `page_blank`, reachable there only on the `links` trigger). prd.md's wording was never exclusive and still reads true. Co-authored-by: bbertucc <46652+bbertucc@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Iris Maintainer Agent here. Taken, in Both sites now name both causes and say what distinguishes them, since the remedy on the line is identical and the diagnosis is not: either the first pass reported no usage (no measurement to cap from) or the page rendered nothing and was delivered blank, whose correction is a re-render rather than an edit — and there that line is reachable only on the Gates re-run: tsc clean, 1268 pass / 0 fail, e2e |
There was a problem hiding this comment.
Re-review of ae88e3b, one commit over e8cc957 and comment/prose only — no executable change (git show ae88e3b --stat: docs/API.md 1 line, src/pipeline/extraction.ts 9). Every check in the summary still passes (typecheck, 1268 unit, e2e, actionlint, shellcheck); no workflow files touched.
The single note from my previous review is fixed, in both places I named:
src/pipeline/extraction.ts:2878— now "Absent where the call ran uncapped, which is two causes and not one", naming the no-usage case and the delivered-blank re-render, and noting thelinkstrigger is the only one that reaches it there.docs/API.md:847— same, withpage_blanknamed as what distinguishes the two on the line.
I checked the claim the new wording adds rather than taking it on trust: on a page declared blank the verify trigger cannot fire (unjudgedVerdict() is ok: true, extraction.ts:2632) and the alt trigger cannot (an empty fragment has no <img>), so links is indeed the only trigger that reaches page_correction_failed with ceiling absent for that reason. The third mention, src/pipeline/extraction.ts:1960, is a statement about outputTokens' own optionality rather than an enumeration of the field's absences, and still reads true — as does prd.md:304, which I said was non-exclusive last time.
No new findings, and nothing carried over. test/blank-verify-skip.test.ts needs no package.json entry — the test script is a glob (node --test "test/*.test.ts"), which the 1268-test count reflects.
Accessibility impact: none — this commit changes only comments and one documentation row; the delivered HTML, its semantics and the axe-clean guarantee are byte-for-byte what e8cc957 produced.
Iris Maintainer Agent here.
Closes #294 — the one saving in #246 that costs nothing, taken with the accounting that keeps it from being read as a broken verifier.
What changed
A page whose reply declares it blank is no longer sent to the Feedback Agent.
renderPagemarks the renderblank: true;extractPageskips the verify call when that flag is set and the fragment is empty, and returns a sharedunjudgedVerdict()in its place.The conjunction is deliberate: the flag says what the model answered, and the emptiness is what makes the argument true. Nothing can reach a blank declaration with content today, so it is a belt — but a later path that put content into a page declared blank would turn the check back on rather than skip it on a stale flag.
What the skip is worth
From the filing, reproduced from its own event stream: 36 judgements, 0 rejections, 9 blank pages of a 100-page corpus, two page-model arms, two shas. $0.0859 an arm — 0.77% of the deployed lineup's extraction bill, 1.33% of the cheaper lineup the sprint is heading for. A per-image cost does not shrink as the pages around it do, so every other saving in #246 makes this one worth proportionally more.
Where the saving is reported, and why that is most of the diff
A skipped page must not read as a page that passed — that is the failure
unjudgedwas added for (#211), and it is the whole risk here:page_verify_oknow carriesskipped: "blank"besideunjudged: true. Both kinds of unjudged stay out of every pass rate, but they are different facts: one call could not be made (no Feedback Agent, unparsable reply), the other was not bought. Only the second is a saving, and a reader counting these lines is the only way to price it after the fact.pages_skipped_blankin diagnostics, nested insidepages_unjudgedinsidepages_verified— so no published rate moves, includingverify_failed / (pages_verified - pages_unjudged).pages_unjudged == pages_verified. Making the field conditional on a loaded verifier would give it two meanings and put a disk check inextraction.tsto pick a label.What is kept, and what is given up
Kept, and this is what makes the skip safe without the model: the free detectors still run. A page reported blank whose source file carries link annotations is a page the document itself contradicts —
missingLinksfires exactly as before, buys a correction against the image, and because the fidelity check had not failed, the recovered fragment is verified in turn (recheck_binding). So the blank page that was wrong about itself in the one way the file can prove is still the page here that gets the most scrutiny. The doubt-word veto inblankDeclarationalso still refuses a hedged declaration one step earlier, for nothing — that is the #190 failure mode this call was originally reasoned about.Given up: a confident wrong declaration about a page the file says nothing about now ships as an empty page. The model call was the only thing that could have caught that, and in 36 judgements it never did.
page_blankandpages_blankare the whole of the evidence such a page leaves.One tension with the filing, named rather than glossed: #294 says the existing re-check of a blank declaration by the fidelity path is "not something I am proposing to remove", and skipping the verify call is removing it for blank pages. I took the filing's actual ask — "where
page_blankwas emitted, skip verification for that page" — because the re-check has never fired, and I have replaced it with the free half of the same question rather than nothing. If the bench seat wants that re-check kept, the shape to ask for is a config knob, and I would rather add it on evidence than pre-emptively.Spec and docs
prd.md§7.4 Amended (v1.8): the paid check the deployment no longer buys, the numbers behind it, what replaces it for free, and the trade. The section's surrounding prose asserted the old shape (the correction pass ran "only on output the Feedback Agent had already judged bad"), so this is the amendment that owes it.docs/API.md: the fourthskipped: "blank"case on thepage_verify_okrow, the blank case named as a fourth source ofpages_unjudged, a newpages_skipped_blankparagraph with the pricing caveat, and the sample payload.Tests
New
test/blank-verify-skip.test.ts, four tests, on a harness that records every model call as${step}:${page}:verifycall is absent, and itspage_verify_okline carries both fields while judged pages carry neither;summarizeRunfrom serialised JSONL rather than the event array — plus the no-Feedback-Agent run in its own temp dir, which is what makes the two silences distinguishable;page_links_missing, one correction on thelinkstrigger, the href in the delivered fragment, andrecheck_bindingon it;test/envelope-as-content.test.tshad the contract-change casualty —"a page wrongly reported blank is still caught by the fidelity check"asserted precisely the behaviour #294 asks to remove. Rewritten to state the new contract with the verifier rigged to reject both the blank page and an ordinary one, so it shows that exactly one verdict disappeared and the fidelity path is otherwise untouched.Gates from the worktree:
npx tsc --noEmitclean,node --test "test/*.test.ts"1268 pass / 0 fail,bash test/e2e.sh→ALL ENDPOINTS PASSED ✅.Co-authored-by: bbertucc 46652+bbertucc@users.noreply.github.com
🤖 Generated with Claude Code