Closes #1843
This is a draft: it changes what a capture's signed record says, so it merges only after the release tag that brings the new record format, and it is stacked on #2000 and merges after that pull request. When the app captures a page in the background, the browser's page saver drops any image offered as a set of resolutions, or files it under an address the browser never fetched, so the app now gives such images up to 5 seconds to finish loading, takes the screenshot, and points each one the page displayed at the source it displayed before saving the page, while the network is still being recorded. Each capture's signed record states how many images that covered and how many were still loading their new source, or had failed to, covering only images that had settled and displayed; an image still loading when the wait ended, one that failed, one shown from an in-memory address, and one in a frame or shadow root are left as they were, where the earlier step (#1996) wrote zeros that read the same as a pass that found nothing, so this change must ship in the same release as that step. If the pass itself fails, the capture fails with an error and nothing is stored, because the record has no way to state a failed pass. Pointing an image at its source makes the browser ask the server for it a second time, so the saved page holds that second answer, and the network record holds both.
src/main/services/backgroundRenderer.ts), per the maintainer's rulings (comment of 2026-10-29 on #1843): settleImageSets polls until no considered image is loading, up to IMAGE_SET_SETTLE_MAX_MS (5 s), before captureFullPageScreenshot, and its last check keeps the images still loading in the isolated world; pinImageSources skips those, and pins only an image that is complete with a natural width and height above 0 and a non-blob: currentSrc, polls the pinned loads up to PIN_LOAD_MAX_MS (5 s), and recounts; a pin with no settle record throws, as a pin with no pinned record already did; both waits are clampPhaseMs(remainingMs(FINAL_ARTIFACT_RESERVE_MS), …) on the main process's clock; activeRecorder.stop() moved from before the screenshot to after the pin. From the review rounds, on the orchestrating session's instructions: the four scripts run through executeJavaScriptInIsolatedWorld(PIN_WORLD_ID, …), the final count is retaken over the DOM, and printPageToPdf reads Page.getLayoutMetrics when it prints instead of screenshotRecord.contentHeight.src/main/services/reportHtml.ts, src/shared/schemas.ts): step 5's artifact inventory sentence defines the second count and says what the counts cover, naming picture elements and the end of the settle wait. It stands in place of the brief's separate report row, shown at 0 too, as the maintainer accepted on 2026-10-29 (ruling 5).tests/main/services/backgroundRenderer.test.ts, tests/main/services/reportCaptureEngine.test.ts): the scripts over fake images in a persistent world, including an image still loading at the settle wait's last check that loads before the pin, an image with natural width 1 and height 0, and a pin with no settle record; the waits under fake time; the render order, an image that loads during the screenshot left out of the pin and the counts, a request during the pin recorded and one during the save not; the print bound after the pin; failure paths; ingest and Duplicate. Step 9's stop test now emits its late request during the save, because the ruling moved where collection stops.e2e/recapture-image-sources.spec.ts): the 400w fixture image is a 400 by 400 file; one page holds a failed image the server would answer a second time, a revoked blob: image, one never answered and one answered after 1 s; each image response is tagged with its path and request number, and the spec asserts each pinned image is requested twice, both requests are in the Transaction Record, and its MHTML part holds the second response, while an image left alone is requested once.features.mdx; plan D4 and Progress; the validation record's #1843 parts, with the three choices recorded as the maintainer's rulings of 2026-10-29, and its standalone probe's "no request during the pin" corrected for the app.origin/feat/1930-store-pdf at 39ae663a, both sides kept in CONTEXT.md, the plan, recapture.ts and the renderer test harness.Yes
img[srcset] is not complete, so the screenshot is taken later and can show images that earlier builds missed; a lazy srcset image below where scrolling stopped holds the full 5 s. The pin runs after the screenshot and does not change it. Each img in the page's own document with srcset, whose parent is not a picture, that had settled and displayed an image (natural size above 0, not blob:) is saved with src set to its currentSrc and no srcset or sizes, so the MHTML holds a part for an image it used to drop, filed under the address that served it. Setting src makes the browser request each pinned image a second time while the network is recorded: the MHTML part holds the second response, and the Transaction Record holds both requests (the e2e tags each response with its request number and checks this). If the server answers the second request with different bytes, the saved page differs from what the page displayed and from the screenshot. An image still loading when the settle wait ends is left out of the pin even if it finishes during the screenshot; failed ones, blob: ones, and those in frames or shadow roots are also unchanged and not counted. The signed inventory.mhtml counts become true values over that set. The Transaction Record now covers the time up to just before the save, so it also holds any request between the screenshot and the save, the pin's included. The PDF shows the pinned page: an image sized by its srcset density or sizes lays out at its natural size, which in the probe took a page from 3,112 to 12,112 CSS pixels and its print from 7 to 16 pages; the 50,000-pixel bound is read when the page prints. The gate fired on the issue's label and on three blocking-tier paths (backgroundRenderer.ts, recapture.ts, reportHtml.ts).verify.sh prove the MHTML and the entry are the bytes and fields that were signed. They do not prove the MHTML is what the serializer would have written from the page unmodified; the signed counts state that it was modified. The counts are computed in an isolated world the page's JavaScript cannot reach and retaken over the DOM; a page script that changes a pinned src between that count and the save is not caught, and the strict schema refuses only counts that are not whole numbers or contradict each other.verifyRunbookExecution.test.ts already runs verify.sh over an entry with counts 12 and 10.pnpm preflight at 6c70ddafdd055037d8c2b3c587adced501a58bb6 on Node v20.20.2; diff scored against origin/main (merge base 50966b79b). Result: pass.
pnpm lint - pass (exit 0)pnpm format:check - pass (exit 0)pnpm lint:boundaries - pass (exit 0)pnpm lint:agents-md - pass (exit 0) - CLAUDE.md and AGENTS.md matchpnpm lint:adr - pass (exit 0)vale-prose --base - pass (exit 0) - 7 changed Markdown files checkedpnpm typecheck - pass (exit 0)pnpm build - pass (exit 0)pnpm build:extension - skipped - no extension/ changespnpm test:coverage - pass (exit 0) - 8208 passed, 24 skipped (8232), thresholds metpnpm coverage:diff - pass (exit 0) - 99.09% of 662 changed lines covered (floor 90%); 2 changed source files not instrumentedCreated with Claude Opus 5.5 in Claude Code (T3 Code)
Added 4 commits
Important
Draft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration:
| 1 | reviews: |
| 2 | auto_review: |
| 3 | drafts: true |
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Written by claude-opus-5-5 via Claude Code on behalf of Matt.
Review verdict: request changes
Reviewed commit: c9d1f27fef487f909d4babda74dbaa87a0430e31 (still head at the end of the review). Base feat/1930-store-pdf at 3f5b2a477. Diff read with git diff origin/feat/1930-store-pdf...c9d1f27f.
| # | severity | file:line | finding |
|---|---|---|---|
| 1 | blocking | src/main/services/backgroundRenderer.ts:517, :830 vs :835; docs/adr/0055-a-background-render-pins-image-sources.md:40-42, :62-64; website/content/docs/features.mdx:30-31, :38-39, :44-46; CONTEXT.md:98; docs/specs/2026-10-26-capture-engine-first-slice-validation-assessment.md:277-280, :397-398; PR body, Evidence impact | Selection is by non-empty currentSrc only, and a failed image has one, so it is pinned after activeRecorder.stop(); its retry is unrecorded, and when the retry succeeds the MHTML stores the retry's bytes and the count calls it loaded. Every text that says a failed image is "counted as not loaded", or that the render pins images "to the one the page displayed", is false for that case. |
| 2 | blocking | backgroundRenderer.ts:517; src/shared/schemas.ts:502-505; src/main/services/reportHtml.ts:1868-1872; #1843 ruling of 2026-10-21 | The ruling defines pinnedImagesNotLoaded as corrected images still loading at the save. A still-loading image has an empty currentSrc (confirmed below), so it is neither pinned nor counted and the MHTML lacks it; a signed 0/0 is the same as a page with no srcset image. In practice the count measures failed images whose retry also failed, plus images the pin itself broke (finding 8). The report sentence beside the counts states none of the limits. |
| 3 | blocking | backgroundRenderer.ts:835-872; PR body, Evidence impact; features.mdx:73-78 | printPageToPdf runs on the pinned DOM, and its 50,000 px bound reads screenshotRecord.contentHeight, taken before the pin. Evidence impact says only "the screenshot, taken first, is unaffected". The features page says a page taller than 50,000 px is not printed, which no longer holds for a page the pin makes taller. |
| 4 | blocking | docs/adr/0055-a-background-render-pins-image-sources.md:3, :40-42, :62-64 | Accepted citing the ruling follows ADR-0051's precedent, and the ruling's own draft said Accepted. The text adds "An image that displayed nothing is left as it is" (false: failed images are pinned) and a consequence that records the implementer's choice to pin failed images as accepted, and that consequence's "counted as not loaded" is false when the retry succeeds. Cut the ADR back to the ruling's text, or mark it Proposed until the maintainer accepts the additions. |
| 5 | gate | gh pr checks 2002 | 11 checks failed in 1-2 s and 17 were skipped. The check-run annotation reads "The job was not started because your account is locked due to a billing issue." There is no CI result at this head, including test, e2e, diff coverage, adr-numbers and Test reliability. |
| 6 | advisory | backgroundRenderer.ts:827-830 | The comment says stopping before the screenshot records the same events as stopping after the save, citing the spike. The pin now sits between the two and sends requests, so its premise is false, and the comment was not updated. |
| 7 | advisory | backgroundRenderer.ts:524-528 | The wait loop uses the page's own Date.now and setTimeout. A page that replaces either holds the render to the 120 s deadline; that fails closed and stores nothing. waitForDocumentComplete (:156-170) has the same shape. Running the script in an isolated world would also stop a page forging the counts, a limit the validation record (:326-328) states and the features page, CONTEXT, ADR and report do not. |
| 8 | advisory | backgroundRenderer.ts:517-521 | An image shown from a blob: URL that the page revoked after load is broken by the pin (probe: natural width 1 before, 0 after). It is counted as not loaded although it had loaded, and the live page and the PDF then show it broken. |
| 9 | advisory | reportHtml.ts:1868-1872; #1843 brief, acceptance criteria 2 and 4 | The brief's row (Image sources: N pinned ..., absent at 0) is replaced by step 5's sentence, which also shows at 0. That is recorded as a deviation in the validation record (:274-276) but not yet accepted by the maintainer. "pinned to the source displayed" is untrue for failed images. |
| 10 | advisory | CONTEXT.md:98 | "in the page's own document" reads as including images in shadow roots, whose node document is the page's own document, and those are not pinned. The features page and plan D4 state this correctly. |
Probe evidence (standalone Electron 44.9.1 script under xvfb-run -a, offscreen sandboxed window, fresh partition, the branch's pinImageSourcesScript extracted verbatim, wait 2,000 ms, then savePage(..., 'MHTML')):
<img srcset="/retry.png 1x">, where the server sends 404 on the first request and a PNG tagged #2 on the second. Before the pin: currentSrc set, complete, natural width 0. Counts {pinnedImages: 1, pinnedImagesNotLoaded: 0}. The MHTML has a part at /retry.png whose bytes end TAG:/retry.png#2. The page and the screenshot showed a broken image.srcset images inserted after load, answered after 6 s. Before the pin both had an empty currentSrc. Counts 0/0, the MHTML has no image part, and one of the two had src="/late-fallback.png".img with its own srcset inside a picture whose source does not match is saved at the right address with or without a src, so the picture skip is sound.srcset set to a blob: URL that is revoked on load. Counts 1/1, natural width 1 before the pin and 0 after, and no part.<img srcset="/tall.svg 4x"> (400 by 4000 SVG) with no width or height. scrollHeight went from 3,112 to 12,112 after the pin, and printToPDF({}) from 7 to 16 pages.The six asks.
img in the top document with srcset, a non-empty currentSrc and a parent other than picture, the saved markup has src set to the absolute currentSrc and no srcset or sizes, so the page's original src and candidate list appear nowhere in it. The image part holds the bytes Chromium has for that address at the save: the displayed bytes for a loaded image, the retry's bytes for a failed image whose retry succeeded, and nothing for a revoked blob:. An image without its own width and height that was sized by density or sizes lays out at its natural size. Frames, shadow roots, picture and still-loading images are unchanged, and the last are missing if offered only through srcset. The texts that claim more are listed in findings 1, 3, 4 and 10. The Content Hash and verification hold: savePage (:840) writes the pinned page, ingest hashes the stored bytes, no hash, signing, schema or verifier code changed, the e2e package passes Package Verification and verify.sh, and a Duplicate re-anchors the entry. They prove the bytes are the bytes signed, not that the MHTML is what the page displayed.clampPhaseMs(remainingMs(FINAL_ARTIFACT_RESERVE_MS), PIN_LOAD_MAX_MS) at :835-838, computed after the screenshot, with a 40 s reserve (:65-66, 20 + 5 + 10 + 5). The unit test gives 0 under a 39 s limit and 5,000 under 60 s. A throwing script or a schema refusal rejects at :536-559 before savePage; the job emits failed and listCaptures is empty in the unit test. The only producer is pinImageSources, which feeds RenderedPage.imageSourcePinning (required, src/main/services/recapture.ts:44) and then backgroundInventory (:133-147). Grep finds no other source of the MHTML counts in src/, and a Duplicate copies its source's signed inventory (src/main/services/captureLifecycle.ts:1711, :1756). No path stores counts the pass did not produce. The pass's own counts can still be wrong (findings 1, 7 and 8).picture exclusion and the document scope, but not shadow roots or still-loading images.appendManifestEntry (src/main/services/manifest.ts:1040-1080) does not re-parse the whole entry, which predates this PR and applies to every part. The unit tests parse the written entry and the Duplicate's with ManifestEntrySchema. The Duplicate's row equals its inventory in the unit test and the e2e.Evidence gate. Two blocking-tier hits: backgroundRenderer.ts and recapture.ts (assessment :232-233). The PR and #1843 both carry evidence-affecting, and auto_merge is null. The Evidence impact section is substantive but incomplete (findings 1 and 3). The new e2e is a sound known answer for the saved page (each part's bytes are tagged with the address that served them), but it does not cover a retry that succeeds. Backward verification is unaffected: no schema or verifier change, and the exported package verifies.
Completion controls. Head is unchanged at the end. auto_merge is null. There are no Co-authored-by trailers in the 4 commits from base to head. The PR body passes the post-pr-body check.sh (exit 0), whose rule accepts the global byline as the attribution line.
Commands run (tool shell is bash, at the head in a scratch worktree under the cache folder):
| 1 | gh pr checks 2002 -> 11 fail, 17 skipping (billing lock annotation) |
| 2 | scripts/setup-worktree.sh -> 0 |
| 3 | pnpm lint -> 0 (0 errors, 47 warnings, none in changed files) |
| 4 | pnpm typecheck -> 0 |
| 5 | pnpm build -> 0 |
| 6 | pnpm test <renderer + recapture tests> -> 1 (libOpenEXR at setup), then after |
| 7 | env -u PKG_CONFIG_PATH pnpm build:test-sharp -> 0 |
| 8 | BIRDBRAIN_TEST_SYSTEM_SHARP=1 pnpm test <same two> -> 0 (143 passed) |
| 9 | pnpm test:coverage (run 1) -> 1: one timeout in derivedFileStates.test.ts (15 s); |
| 10 | that file alone -> 0 (12 passed), so load noise; no coverage file written |
| 11 | pnpm test:coverage (run 2) -> 0 (8191 passed, 24 skipped) |
| 12 | node scripts/diff-coverage.mjs --base origin/feat/1930-store-pdf -> 0 (10/10 lines, 100%) |
| 13 | pnpm coverage:diff -> 0 (615/621 vs main, 99.03%) |
| 14 | pnpm test:reliability --base origin/feat/1930-store-pdf -> 0 (revert: 0 of 21 still pass; |
| 15 | mutation: 1 of 6 survived, backgroundRenderer.ts:546 userGesture true->false, equivalent) |
| 16 | xvfb-run -a npx playwright test e2e/recapture-image-sources.spec.ts -> 0 (3 passed) |
| 17 | xvfb-run -a npx playwright test e2e/recapture.spec.ts -> 0 (4 passed) |
| 18 | .claude/hooks/vale-prose.sh --base origin/feat/1930-store-pdf -> 0 |
| 19 | pnpm lint:adr -> 0; node scripts/check-adr-numbers.mjs -> 0 |
| 20 | Electron probes (retry, slow, late, picture, blob, pdf) -> 0 each |
Not checked. CI at this head (it never ran). The other 15 Recapture e2e specs the PR body counts. The extension path, which has no diff here. How common it is for real pages to replace Date.now or setTimeout.
Added 8 commits
Written by claude-opus-5-5 via Claude Code on behalf of Matt.
Dispositions for the review verdict above, at the latest commit, after the maintainer's rulings of 2026-10-26.
03123021 (merge of the base), 517c4828 (3), 7f822091 (1, 2, 3 test, 6, 7, 8), a3cc3105 (2, 9), edc71455 (1, 2, 8), d7115cd7 (4), 55871201 (1, 2, 3, 7, 9, 10), d48dae2d (validation record count)
Review verdict: request changes
Reviewed commit: d48dae2db57011d7f1468e007f43f24147435917 (still head at the end). Base feat/1930-store-pdf at 39ae663a. Delta round from c9d1f27f. The PR's own delta is git diff 03123021..d48dae2d (all first-parent commits after the merge are the PR's); the merge's remerge diff resolves four conflicts by keeping both sides, checked by hand.
| # | severity | file:line | finding |
|---|---|---|---|
| 1 | blocking | docs/adr/0055-a-background-render-pins-image-sources.md:3, :7-10, :50-62; PR comment 6434962348 | The ADR is Accepted on "two rulings of 2026-10-26 on the review of #2002, as the orchestrating session relayed them". Neither has a GitHub record: #1843 has two comments (2026-10-15, 2026-10-21), #2002 has CodeRabbit's draft notice, the round-1 verdict and the dispositions, which cite the rulings without their text; no comment repo-wide since 2026-10-26T12:00Z mentions them. The 2026-10-21 ruling is on record (comment 6315229188) and the ADR's "Ruling of 2026-10-21" bullets match its brief and draft ADR. Fix: the maintainer posts the two rulings on #1843 and the ADR links that comment. |
| 2 | advisory | validation record :336-342; backgroundRenderer.ts:552, :576-584; ADR :38-41, :58-59 | Two behaviours rest on the orchestrating session, not a ruling: pinnedImagesNotLoaded also counts a pinned source that finished broken (ruling 1 says "had not finished loading"; disclosed in the body as for the maintainer), and a displayed blob: image is not pinned, although the ADR's 10-21 bullet says each img that "displayed an image has its src set". Merging on approve ratifies both silently. |
| 3 | advisory | backgroundRenderer.ts:547-552 vs :601, :917-924; ADR:59; features.mdx:44; validation record:477; PR body, Evidence impact | Selection happens at pin time by shown(img); settleImageSets records nothing about which images were still loading when it ended. An image still loading when the 5 s settle wait times out, that completes during captureFullPageScreenshot, is pinned and counted. The screenshot does not show it. The texts saying "still loading after the first wait/the settle wait ... left as they are" are false for that window. Its request is in the Transaction Record and its part is filed correctly, so no wrong-origin bytes result. Corrected wording: "An image still loading when the pin runs ...", or snapshot the still-loading set at the end of the settle wait. |
| 4 | advisory | PR body, Evidence impact ("The screenshot, taken before the pin, is unaffected") | The new settle wait before the screenshot (backgroundRenderer.ts:917-920) delays it by up to 5 s whenever an img[srcset] is not complete (a lazy image below the scroll stop always holds it the full 5 s), so the screenshot can show images that earlier builds missed. Corrected wording: "The pin does not touch the screenshot, but the settle wait before it can take the screenshot up to 5 s later, after images offered as sets have finished loading." The section also names two blocking-tier paths; reportHtml.ts is a third (assessment :303). |
| 5 | advisory | backgroundRenderer.ts:533 | Mutant naturalWidth > 0 && naturalHeight > 0 -> naturalWidth > 0 survives all 110 renderer tests. features.mdx:37 claims width and height above zero. A wide image whose density makes its natural height below 1 px would be pinned under the mutant. A fake image with width 1 and height 0 would cover it. |
| 6 | advisory | reportHtml.ts:1868-1876 | The report sentence says what the counts cover but does not say a picture's image is excluded, though "offered as a set of sources" describes it too. CONTEXT.md and features.mdx do say it. |
| 7 | advisory | backgroundRenderer.ts:591-596 | CI mutant at :595 (- to +) survived; it lets one poll overshoot the wait by under 50 ms, inside the reserve. Equivalent for the deadline claim. Separately, a page that blocks its main thread stalls executeJavaScriptInIsolatedWorld, so the main-process clock does not end the wait; the outer render deadline does, as for the scroll scripts (pre-existing shape). |
Earlier blockers, each fix mutated (bash tool shell; BIRDBRAIN_TEST_SYSTEM_SHARP=1 pnpm test tests/main/services/backgroundRenderer.test.ts, source restored after each):
|| !shown(img) -> 5 tests fail. Same mutant built into the app -> e2e leaves a failed image, a blob: image and one still loading as they were fails on the counts. Moved activeRecorder.stop() back before the screenshot -> records a request the page makes during the pin, and none made during the save fails.settleImageSets call -> 2 fail; settle max set to 0 -> 1 fails; settle script returns 0 -> 3 fail.reads the print's height bound after the pin, not from the screenshot fails. features.mdx:84-89 and Evidence impact now state the PDF shows the pinned page.blob: exclusion removed -> 5 fail; count recheck removed -> 1 fails; not-loaded as !complete only -> 2 fail; picture filter removed -> 6 fail; page world instead of isolated -> 58 fail (mock-level; isolation itself is shown only by the validation record's standalone probe).Bot findings. CodeRabbit posted only its draft-skip notice. There are no review threads or inline comments (pulls/2002/reviews and pulls/2002/comments, paginated, are empty).
Test reliability (CI, run 40458393688). Revert probe: 2 of 30 pass with the source reverted. leaves ... as they were passes vacuously at the base, which has no pin, and the mutants above kill it. stops collection before the save is step 9's regression guard and holds at the base by design. Mutation: 16 of 17 caught, survivor at :595 (finding 7).
Evidence gate. Blocking-tier hits: backgroundRenderer.ts, recapture.ts, reportHtml.ts (assessment :232, :233, :303). Advisory: schemas.ts (:377); the diff there is a comment above MhtmlInventorySchema and changes no evidence behaviour. The PR and #1843 both carry evidence-affecting. The Evidence impact section is substantive; findings 3 and 4 correct two of its claims. Known answer: the e2e tags every image response with its path and checks each MHTML part's bytes against its address, with fixed counts. Backward verification: no hash, signing, verifier or schema-shape change (schema 6 already has both fields); the e2e package passes Package Verification and verify.sh. auto_merge is null. The body says the PR merges only after the release tag that brings the new record format; nothing in the repo enforces that, so it rests on the maintainer.
Trial merges. Into origin/feat/1930-store-pdf: already up to date. Into origin/main (aa87bd36, with rerere off): 19 conflicted files, 18 of which the base branch alone also conflicts on. The one this PR adds is reportHtml.ts: main carries step 5's sentence, and resolving it is mechanical. node scripts/check-adr-numbers.mjs on that merged tree passes; main's highest ADR is 0054, and no remote branch holds another 0055.
Completion controls. Head re-read at the end: d48dae2d, unchanged. auto_merge null. No Co-authored-by in the 12 commits from base to head. The PR body passes post-pr-body/scripts/check.sh (exit 0); its last authored line is the global byline that the checker accepts.
Commands run (tool shell is bash; scratch worktree under the cache root; Node v20.20.2):
| 1 | gh pr checks 2002 -> all pass at d48dae2d except merge-gate (expected, sign-off); agent/pre-pass pending |
| 2 | CI test job -> 482 files passed; diff coverage 34/34 changed lines vs base (100%) |
| 3 | scripts/setup-worktree.sh -> 0 |
| 4 | env -u PKG_CONFIG_PATH pnpm build:test-sharp -> 0 |
| 5 | pnpm test <backgroundRenderer, reportCaptureEngine, captureLifecycle, |
| 6 | captureEngineColumnsMigration, recapture> -> 0 (324 passed) |
| 7 | 12 source mutants, backgroundRenderer.test.ts -> 11 killed, 1 survived (finding 5) |
| 8 | pnpm build -> 0 |
| 9 | xvfb-run -a npx playwright test e2e/recapture-image-sources.spec.ts -> 0 (3 passed) |
| 10 | same, failed-image mutant built in, left-alone case -> 1 (counts differ), source restored and rebuilt |
| 11 | xvfb-run -a npx playwright test e2e/recapture.spec.ts e2e/recapture-transaction-record.spec.ts -> 0 (13 passed) |
| 12 | trial merges and check-adr-numbers -> as above |
Not checked. Full local coverage run (CI's test job is green at this head and was used instead). The other Recapture e2e specs locally (CI's e2e job is green). Whether the 26 October rulings were made as the ADR states them.
Review verdict: request changes
Reviewed commit: d48dae2db57011d7f1468e007f43f24147435917
The commit was still head at the end. Base feat/1930-store-pdf at 39ae663a, which is the merge base, so git diff origin/feat/1930-store-pdf...d48dae2d is the PR's own change. The tool shell was bash on a GitHub Actions runner. This is an independent re-review of the sha the 2026-10-27 pre-pass already reviewed. Findings 1, 3 and 4 agree with it, finding 2 rates its finding 3 higher, and findings 6 and 8 are new.
| # | severity | file:line | finding |
|---|---|---|---|
| 1 | blocking | docs/adr/0055-a-background-render-pins-image-sources.md:3, :7-10, :50-62 | The ADR is Accepted on "two rulings of 2026-10-26 ... as the orchestrating session relayed them". At this review neither ruling has a record: #1843 has only its 2026-10-15 and 2026-10-21 comments, and a search of every issue and PR comment since 2026-10-25 finds the rulings cited (dispositions 6434962348) but never stated. An accepted ADR on an evidence-affecting path that rests on rulings nobody can check cannot be verified. Fix: the maintainer posts the two rulings on #1843 and the ADR links that comment. |
| 2 | blocking | src/main/services/backgroundRenderer.ts:552 vs :601, :917-925; ADR-0055:59; website/content/docs/features.mdx:44-45; validation record:477; PR body, Evidence impact | settleImageSets returns nothing and keeps no record of which images were still loading when the wait ended. The pin re-tests shown(img) after the screenshot, so an image still loading at the end of the settle wait that completes during captureFullPageScreenshot is pinned and counted. The ADR states as a ruling that "Any still loading after that wait are left out", and the features page, validation record and Evidence impact say the same. The code does not do that. No wrong-origin bytes result: the request is recorded and the part is filed under its address. But the pinned image can be absent from the screenshot, and the docs say it cannot happen. Fix: snapshot the still-loading set when the settle wait ends and skip it in the pin, or reword to "an image still loading when the pin runs". |
| 3 | blocking | PR body, Evidence impact ("The screenshot, taken before the pin, is unaffected"); backgroundRenderer.ts:917-922 | The settle wait now runs before the screenshot and holds it up to 5 s whenever any considered img[srcset] is not complete. A lazy srcset image below where the scroll stopped always holds the full 5 s (validation record :221-222, 98 polls). The screenshot is therefore taken later, after a wait meant to change which images it shows. The gate artifact's "unaffected" is untrue. |
| 4 | advisory | validation record :330, :339, :340; src/main/services/reportHtml.ts:1868-1876; #1843 acceptance criteria 2 and 4 | The record marks three decisions as not the maintainer's: counting a pinned source that finished without an image as not loaded ("This addition awaits the maintainer"); leaving displayed blob: images alone (orchestrating session's instruction); and replacing the brief's Image sources: N pinned ... row, absent at 0, with step 5's sentence, which also shows at 0 (orchestrating session's decision). Criteria 2 and 4 are not met as written. Each is defensible, but merging ratifies all three without a ruling. |
| 5 | advisory | backgroundRenderer.ts:533; features.mdx:37-38 | I applied the mutant img.complete && img.naturalWidth > 0 (dropping naturalHeight > 0) and all 110 renderer tests passed (source restored). The features page claims "a natural width and height above zero". A fake image with width 1 and height 0 would cover it. This confirms the 2026-10-27 pre-pass's finding 5. |
| 6 | advisory | e2e/recapture-image-sources.spec.ts (png() tags bytes with TAG:<path>; five-image test) | The known-answer tag names the path, not the request, so a second fetch of a pinned image with different bytes would still pass. The five-image and q2-savepage cases never assert each pinned path was served once (only the failed-retry case counts requests). My probe shows Chromium does not refetch today, so this is a coverage gap rather than a defect. Asserting a served count of 1 for each pinned path would let the e2e back the claim that the MHTML holds the bytes the page displayed. |
| 7 | advisory | PR body, Evidence impact, last sentence | It names two blocking-tier paths. src/main/services/reportHtml.ts is a third (assessment :303). |
| 8 | advisory | PR body, Summary, last sentence | "The repository's automated checks did not start on the previous push ... so every result below is a local run" is stale. CI ran at this head and every required job is green. |
| 9 | advisory | backgroundRenderer.ts:595 | CI's surviving mutant (- to + in the poll's sleep) lets one poll overshoot the wait by under 50 ms, inside the reserve. It is equivalent for the deadline claim. |
Probes (standalone Electron from the branch's node_modules, under xvfb-run -a in bash; offscreen sandboxed window on a fresh partition with the debugger attached and Network.enable; the branch's five pin scripts extracted verbatim from backgroundRenderer.ts and run in world 1843; 2 s load wait, settle poll, pin, 5 s load poll, count, savePage(..., 'MHTML'); each response tagged with TAG:<path>#<request number>):
srcset-only images served with no cache headers, no-store, no-cache, Vary: * and max-age=3600, plus a src+srcset image with no-store. The pin returned 6 and the counts were 6/0. Every path was served once before the pin and once in total, and every MHTML part held request #1. /fallback.png was never requested. So pinning sends no second request for a loaded image in Electron 44, and the hypothesis is refuted.viewBox-only SVG: natural size 150 by 150, so it is pinned and has a part. A sized SVG gives 10 by 10. The hypothesis that a displayed SVG would be skipped is refuted.Evidence gate. Blocking-tier hits: backgroundRenderer.ts (assessment :233), recapture.ts (:232) and reportHtml.ts (:303). Advisory-tier hit: src/shared/schemas.ts (:377). Its diff is a comment above MhtmlInventorySchema and changes no evidence behaviour. The PR's labels (evidence-affecting, agent-authored, size:XL) and #1843's both include evidence-affecting. The Evidence impact section is substantive, but findings 2, 3 and 7 correct it. Known answer: the e2e checks each MHTML part's bytes against its address with fixed counts, which is sound for the address, with finding 6's gap for the bytes. Backward verification: no hash, signing, verifier or schema-shape change, since schema 6 already carries both fields with the not-loaded-at-most-pinned refine (schemas.ts:512-518). The full suite, including the schema-6 and verify.sh tests, passed, and tests/helpers/captureEnginePackage.ts:45 carries counts 12/10 into verifyRunbookExecution.test.ts. auto_merge is null.
Acceptance criteria (#1843). Five-image e2e with 3/0 and no part under an unserved address: met. Report row: not met as written (finding 4). Order test: met (backgroundRenderer.test.ts:2012). Zero counts: met in code and unit test, but the report shows a 0 sentence rather than no row. Not-loaded within deadline: met (:2077). Package verifies in the app and in verify.sh: met in the e2e. Extension unchanged: no extension/ diff.
Verify loop (scratch worktree at d48dae2d, Node v20.20.2, removed afterwards):
| 1 | gh pr checks 2002 -> all required jobs pass; merge-gate fail and agent/pre-pass fail (prior verdict); none pending |
| 2 | scripts/setup-worktree.sh -> 1 (vale missing on this runner; pnpm install itself done) |
| 3 | pnpm lint -> 0 |
| 4 | pnpm typecheck -> 0 |
| 5 | pnpm build -> 0 |
| 6 | pnpm build:test-sharp -> 1 (no system libvips); suite ran without it |
| 7 | BIRDBRAIN_REQUIRE_OPENSSL=1 BIRDBRAIN_REQUIRE_JQ=1 pnpm test:coverage -> 0 (482 files; 8204 passed, 24 skipped) |
| 8 | pnpm coverage:diff -> 0 (656/662, 99.09%, vs origin/main) |
| 9 | node scripts/diff-coverage.mjs --base origin/feat/1930-store-pdf -> 0 (34/34, 100%) |
| 10 | naturalHeight mutant, backgroundRenderer.test.ts -> 0 (110 passed: survives) |
| 11 | Electron probes (refetch, SVG) -> 0 each |
Test reliability (CI probe job, run 40458393688): 2 of 30 tests pass with the source reverted. "leaves a picture image ... as they were" passes vacuously at the base, and the earlier pre-pass's mutants kill it. "stops collection before the save" is step 9's guard. Mutation: 16 of 17 caught, with the one survivor at :595 (finding 9).
Completion controls. Head re-read at the end: d48dae2d, unchanged. auto_merge: null. Co-authored-by trailers in the 12 commits base..head: 0. The PR body passes post-pr-body/scripts/check.sh --any-author (exit 0). Its last line is a T3 Code byline with no mid-body attribution line, which is acceptable on this maintainer-written body.
Not checked. Vale locally (not installed; CI's checks are green). The e2e specs locally (CI's e2e job is green at this sha). Whether the 2026-10-26 rulings were made as the ADR states. The extension path, which has no diff.
Added 6 commits
Answers to the two review verdicts on this pull request, at the latest commit.
| answer | verdict 6483456936 | verdict 6452789490 | change |
|---|---|---|---|
| 1 | 1 | 1 | a684c0be |
| 2 | 2 | 3 | 659b8feb |
| 3 | 3 | 4, first half | PR body, Evidence impact |
| 4 | 4 | 2 | a684c0be |
| 5 | 5 | 5 | b9227103 |
| 6 | 6 | none | b9227103, then 6267408b and 6c70ddaf |
| 7 | 7 | 4, second half | PR body, Evidence impact |
| 8 | 8 | none | PR body, Summary |
| 9 | 9 | 7, first half | none |
| 10 | none | 6 | 36c066ba |
| 11 | none | 7, second half | none |
Answer 6: with each image response tagged TAG:<path>#<n>, the three pinned images of the five-image page, the q2-savepage srcset image and the left-alone page's 1 s image each had two requests; the MHTML parts end in #2, the WARC has two request records for each pinned five-image path, and picture-source.png and plain.png had one request each. The renderer as it was at d48dae2d did the same (rebuilt and rerun with request logging). The validation record's standalone probe, which saw no second request, now carries the app's result beside it.
Added 1 commit