feat(recapture): record the Scroll-to-load outcome and screenshot facts#1996

merged50+

main

feat/1928-scroll-and-screenshot-record

View on GitHubTwo-way sync enabledPR last imported 7h ago

Closes #1928, #1667

Summary

Do not merge this before the maintainer tags a release whose verifier reads the new fields it signs, and do not ship it in a release without #1843: until that pinning lands, the zero image-pinning counts it signs cannot be told apart from "pinning ran and found nothing" except by the tool version. A background capture's signed record now says whether the scroll that loads lazy content ran and how it ended, and what happened to the screenshot: the page size, the image size, and whether the image was scaled down, cut short, or trimmed. The same record starts the list of files a capture holds, which states the network record and the PDF as not taken, and the capture's database row and any copy of the capture carry both facts. Testing found that the scroll used to end at the bottom of every page as "nothing to scroll"; it now ends once the page or inner panel it scrolls has sat at its bottom, or stopped moving, with no change in height for three steps in a row, keeps going on a feed whose next chunk takes a second to arrive, lifts a scroll lock only when it finds nothing it can scroll up or down, and fails the capture if it cannot restore a scroll lock it lifted. The pull requests it was built on, #1985 and the ones below it, have all merged, and this branch now carries the main branch with them; the session that started this review round posts this description and the review replies.

Changes

Files changed, grouped by purpose

Renderer (src/main/services/backgroundRenderer.ts): scrollToLoadLazyContent returns the D5 record; captureFullPageScreenshot and trimTrailingBackground return the D6 facts beside the image. The step script picks a root that can still move down (the document, else the inner panel with most overflow) and, only when none can, reports the root at its end with the most overflow, so the stall detector ends the phase at a real bottom; the stall count restarts on any change of height, so a second pane does not inherit the first's stalls. The unlock script makes the same root choice (pickRoot(true)), so a root at its end, such as a page loaded at a fragment near its bottom, is not read as locked. The comment above SCROLL_HELPERS now says Chromium moves a standards-mode overflow: hidden document from script. A failed reset that leaves a lifted scroll lock in place now fails the render (D5 has no field for it); a reset with no lock to restore is logged under the new code backgroundRenderer.scroll_reset_failed (src/shared/types.ts, src/renderer/lib/notify.ts).
Writer (src/main/services/recapture.ts, src/main/services/captureLifecycle.ts, src/main/services/db/captureRepo.ts): RenderedPage carries the two records; the Recapture builds the D4 inventory (MHTML and screenshot present, pin counts 0, Transaction Record and PDF not-taken); IngestParams and InsertCaptureParams gain inventory and scrollToLoad; ingest signs and mirrors both, and Duplicate's row copies both from the source's signed entry (the #1982 carry). manifest.ts is unchanged (D2).
Report (src/main/services/reportHtml.ts): the Scroll-to-load note says the end height is the last height read of the document or inner panel scrolled, not the document's height.
Tests: renderer tests drive each D5 ending, the stall count (including a height that falls, or moves down and up, at every step), the restore failures, a last height that is not a whole number after a whole one, and the D6 cases including content ending inside the trim padding, a first-row-only page, and each PNG header check on its own; the step script runs against a simulated page for the document, app-shell, two-pane and grown-panel cases; the unlock, step and reset scripts run together against a simulated page for a page at its end and a locked page whose document cannot move from script; recapture tests parse the entries a Recapture and its Duplicate sign with the strict ManifestEntrySchema and check both rows; ingest tests cover the fields present and absent.
End to end (e2e/recapture.spec.ts): Recaptures of a page that grows to 4001 px (stopped-growing, the height, the screenshot facts in entry and row), an app-shell page whose inner panel grows to 10001 px, and a two-pane page whose panes each load a chunk. Their shared queueUrl helper opens /cases/new?blank=true, since #1972 made a bare /cases/new open the start step. e2e/recapture-egress.spec.ts now expects schema 6 on a background entry.
Docs: CONTEXT.md (Capture, Scroll-to-load), website/content/docs/features.mdx (Capture), plan D5's endHeight definition, including a last height that is left out, the D16 validation record docs/specs/2026-10-26-capture-engine-first-slice-validation-assessment.md with the Electron runs comparing the scroll before step 6, the first fix and this one, the scroll-lock runs on pages with and without a doctype with each page's compatMode, the body scroller the step never considers in standards mode as a known limitation, and the stall rule's effect on a height that changes every step, and a plan Progress note.
Pin counts: #1978's schema requires pinnedImages and pinnedImagesNotLoaded on the MHTML part. This build pins no image, so 0 and 0 are true of it and pass the schema's refine; the report renders "0 images were pinned". The release sequencing is with the maintainer.
Hot-file order: the plan puts #1499 (persona phase 3) ahead of this in backgroundRenderer.ts and recapture.ts, and the link-capture writer ahead of it in recapture.ts. Neither has merged; whichever lands second pays a rebase.
Not done here: Shared Case sync inserts a received Capture's row (receivedEvidence.ts) without the inventory and scroll_to_load mirrors, so that row leaves them null while its signed entry has them. The issue names ingest and Duplicate only.

Evidence-affecting

Yes

What could change for evidence, and how that is checked

Evidence impact

Evidentiary result or interpretation that could change: acquisition and the signed entry. A background Capture's entry gains scrollToLoad and inventory, stamped schema 6, and a Duplicate of one re-anchors both. A build between this step and step 9 states the Transaction Record and PDF as not-taken and both pin counts as 0, which is true of that build and stays in the chain; once #1843 ships the same zeros mean that pinning ran and found nothing. The scroll changes alter what is acquired, against the build before step 6, in Electron runs recorded in the validation record: the phase now takes one to three more half-second steps at the end of the pages run; an app-shell page and a two-pane page are scrolled as before and now end stopped-growing rather than nothing-to-scroll; an endless feed whose next chunk arrives 1 s after its end, which the old build stopped at its first bottom with no chunk loaded, now runs on (to the 30 s limit of the run, ten chunks), which in a Recapture means to the height cap or the phase's time limit; and a page loaded at a fragment near its end, which the old build treated as locked and set to overflow: visible for the phase, now keeps its own overflow and records scrollLockLifted: false. That includes a page with a doctype whose html and body are full height with overflow: hidden: loaded at a fragment its document starts at its 8 px end, so the old build lifted the lock and scrolled the 5000 px block to 5042, and this step lifts nothing, scrolls none of it, and ends stopped-growing at 487; loaded at its top, that page stops at 487 in both builds. The record's scroll-lock runs give each page with and without a doctype. A page whose height changes at every step, down as well as up, runs to the height cap or the time limit; the old build counted a fall in height as a stall step. A render whose lifted scroll lock cannot be restored now fails instead of being stored with the lock lifted.
What verification proves and does not prove after this change: package and chain verification prove the entry is signed as written and passes the strict schema. They do not prove the page reported its height or scroll position honestly: the step script runs in the page's own world, and a height that is not a non-negative safe integer is left out rather than signed. stopped-growing means three half-second steps without a change in height, not that the page had no more content, and endHeight is the height of the root the last step reported, which on an app-shell page is the panel's and on a two-pane page the larger pane's. scrollLockLifted: true means that when the phase started the document overflowed and neither it nor any inner panel at least half the window tall could move up or down; it does not say what set the lock. scrollLockLifted: false does not mean the page had no lock: the full-height page above, which scrolls its content inside a hidden body, records false.
Backward verification preserved (existing evidence packages still verify): yes. Entries that do not carry the fields are written as before (an ingest test pins that an ingest passing neither omits both and stays below schema 6), and step 1's verifier reads the new ones. A verifier older than that release stops at a schema-6 entry as too old, which is why this waits for the tag.
Known-answer test added or extended, or why none applies: the renderer tests pin each D5 ending, the root each step scrolls on simulated pages, the unlock decision for a page at its end and a locked page, and each D6 case to exact values from fixed inputs; the end-to-end Recaptures pin stopped-growing with 4001, 10001 and 6001 px on three invented pages. No hash, signature, or canonical-form method changes here, so no chain known-answer test needs extending.

Verification

All pre-merge checks passed

pnpm preflight at c2d35f668d24780f3c9cf89ddfcff25b6fb4866b on Node v20.20.2; diff scored against origin/main (merge base 2d5f15940). 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 match
pnpm lint:adr - pass (exit 0)
vale-prose --base - pass (exit 0) - 4 changed Markdown files checked
pnpm typecheck - pass (exit 0)
pnpm build - pass (exit 0)
pnpm build:extension - skipped - no extension/ changes
pnpm test:coverage - pass (exit 0) - 8380 passed, 25 skipped (8405), thresholds met
pnpm coverage:diff - pass (exit 0) - 100.00% of 40 changed lines covered (floor 90%); 1 changed source file not instrumented

Created with Claude Opus 5.5 in Claude Code (T3 Code)

Checks

1 failing · 20 successful · 10 skipped

Some checks failed

Failing1

CodeRabbitGitHub
Cancelled

Added 18 commits4d ago

feat(export): hand the report the capture engine fields054e044
fix(recapture): count a scroll root at its end as scrollable0b0709b
feat(report): state the capture engine fields from the signed entry1438738
test(archive): pin archive version 13 in the records migration test3ae44ae
chore(merge): take step 4 into the report branch3f01014
chore(merge): take the step 1 fix round into the report branch4ce8575
feat(verify): bind the Transaction Record and the PDF in verify.sh61af817
feat(recapture): sign the Scroll-to-load record and the inventory6d0cc9c
fix(export): ship the two files only when the signed entry anchors them71cce8b
docs(capture-engine): carry the Duplicate row's mirror fields forward7dc8392
fix(capture): stream the Transaction Record and PDF when verifying8a6c628
feat(export): ship the Transaction Record and the PDF in packages95444c0
fix(report): quote capture engine fields only over a verified chain976e4a5
test(export): find capture engine fields for an import-renamed rowb4bd9b7
docs(verify): state who carries request bodies and qualify the recordb7e5e2a
docs(capture-engine): state the scroll record and start validationbdb25c9
chore(merge): take step 4's fix round into the report branchdfd8677
chore(merge): take the step 3 fix round from its branchec0e1c8

CcoderabbitaiBot4d ago

Caution

Review failed

The pull request is closed.

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.

Share

MMatt Donovan4d ago

_{Written by claude-opus-5-5 via Claude Code on behalf of Matt.}

Review verdict: request changes

The scroll fix stops background captures from scrolling pages whose content sits in an inner scrolling panel when the page itself overflows by a few pixels, so their lazy content never loads and the signed record still says the page stopped growing.
The change's own evidence notes describe the fix as a few extra half-second steps at the bottom of a page, and leave out that loss, as well as the longer scroll runs it causes on pages that keep growing.
The most common screenshot case, a page with content close to its bottom edge, has no test, so a wrong "trimmed" value there would pass every test.
The zero image-pinning counts are true of this build but read in the report as though a pinning step ran, and later entries will be indistinguishable from them; holding the release until real pinning lands avoids this.
Everything else asked about checks out: each record parses, the failure and zero-time paths are right, copies carry the fields, and shared copies show the fields as missing rather than wrong.

Full report

Reviewed commit: bdb25c969bafcd6a777af51ca3e7fe11bfb391df (head re-read at the end, unchanged). Base: feat/1927-report-capture-engine-fields at b4bd9b715.

#	severity	file:line	finding
1	blocking	src/main/services/backgroundRenderer.ts:147-161	`canScroll` now returns true for a root at its end, and `pickRoot` returns `doc` whenever `canScroll(doc)` holds, so once the document reaches its end it is picked on every step and an inner scroller is never reached. Before the fix, the document failing the down probe at its end is what handed the loop to the inner scroller.
2	blocking	docs/specs/2026-10-26-capture-engine-first-slice-validation-assessment.md:54, :62, :74; PR body, Evidence impact; CONTEXT.md:102	The validation record and the Evidence impact describe the fix's acquisition effect as "up to three more steps at the bottom", and omit the inner-scroller loss in finding 1. They also leave out that a page whose next chunk takes longer than 0.5 s now runs to the height cap or time limit where it used to stop at the first bottom. With finding 1, CONTEXT.md's "the phase never silently yields a static one" is untrue for app-shell pages.
3	blocking	src/main/services/backgroundRenderer.ts:434; tests/main/services/backgroundRenderer.test.ts:240	The `cropHeight >= h` branch (content ends within the 8 px padding of the bottom on a different background, the usual real page) has no assertion-bearing test. The test titled "leaves a page that fills to the bottom untouched" builds a single-colour image, so it takes the `lastContentRow < 0` branch at :432. The CI probe reports `return undefined` surviving at :410 and :434. I reproduced :434 locally: 39/39 pass with the mutant. The signed `trimmed: false` on that path is unpinned.
4	advisory	src/main/services/recapture.ts:110; src/main/services/reportHtml.ts:1868-1872 (from #1985)	`pinnedImages: 0, pinnedImagesNotLoaded: 0` is true of a build with no pinning. The report renders "0 images were pinned to the source displayed before the save, and 0 of them had not loaded", which reads as a pinning step that ran. After #1843 the same signed values mean "pinning ran and found nothing", and nothing in the entry except `toolVersion` tells the two apart. No surface claims anything about `srcset` or image sets: the verifier reads only `inventory.transactionRecord` (src/shared/verify/evidencePackage.ts:174), and MCP `get_capture` passes the row's zeros through raw (src/mcp/tools/captures.ts:87). Recommend that the tag waits for #1843, as the PR body already offers.
5	advisory	src/main/services/backgroundRenderer.ts:160-173 (two-pane)	Same root cause as 1: on a page with two inner scrollers (sidebar plus main), the base build scrolled both in turn and the head scrolls only the one with the larger overflow.
6	advisory	src/main/services/backgroundRenderer.ts:295 (pre-existing)	The reset that restores a lifted scroll lock swallows its own failure (`.catch(() => {})`), and the function comment still says the lock "is always restored". If the reset rejects, the record says `scrollLockLifted: true` while the screenshot and MHTML are taken with the lock still lifted. Not introduced here, but the PR now signs a claim beside it.

Failure scenario for 1 (reproduced in real Chromium). I ran Electron 44 under xvfb with SCROLL_HELPERS and SCROLL_STEP_SCRIPT extracted verbatim from the base and head sources, driving the loop with the renderer's stall logic. The page is <body style="height:100vh"> with the default 8 px margin (16 px of document overflow) and a <main style="height:100%;overflow:auto"> that grows by 2000 px through an IntersectionObserver.

Base: 26 steps, inner scroller at 9522 of 10001, four lazy chunks loaded (the base build would have ended on nothing-to-scroll).
Head: 4 steps, outcome stopped-growing, endHeight 495, inner scrollTop 0, no lazy chunk loaded.

The signed entry would state that the page stopped growing at 495 px while its content grows to 10001 px when scrolled. The e2e fixture scrolls the document itself, so the suite cannot see this. A fix direction that keeps the D5 ending: pick a root that can still advance first (document, then inner), and fall back to a root at its end only when nothing can advance. Scripts: ~/.cache/rev1996/scratch/main.js and loop.js.

The questions asked.

Scroll fix correctness and capture effect. The fix is right for a page that scrolls the document, and it makes stopped-growing reachable at the bottom. It is wrong for app-shell layouts, as in finding 1.
Timing. The phase stays bounded by clampPhaseMs(remainingMs(20 s), 75 s) (:635) and the deadline check before each step (:253), so the 20 s final-artifact reserve and the 120 s Recapture budget hold, with the same one-step overshoot as before. Pages now spend about 1 to 1.5 s more at each bottom, and growing feeds can use the full scroll budget, so screenshots are taller and more often truncated: true.
Unlock and restore pairing. The unlock runs at the top, where the up-probe is skipped (prev > 0), so its decision is unchanged. The restore runs in finally with the same priorOverflow, so the two still pair (finding 6 aside).
D5 outcomes. All five are reachable and pinned by mocked tests (backgroundRenderer.test.ts, scrollToLoadLazyContent). stopped-growing at a real page bottom is pinned only by the e2e.
Strict schema. Every scroll record parses ScrollToLoadRecordSchema (the scrollUnderFakeTime helper). Whole entries parse ManifestEntrySchema for the stopped-growing, not-run and Duplicate cases (recapture.test.ts). A step that throws rejects the render, runJob emits failed (recapture.ts:260-285), and nothing is ingested. A zero budget records ran: false with no outcome, which is true.
Duplicate and Shared Case. Duplicate copies both fields from the signed entry (captureLifecycle.ts:1675-1676, 1720-1721). receivedEvidence.ts:405 leaves the mirror null. Readers: the report reads the signed entry (reportHtml.ts:148-179), the renderer reads neither field, and MCP get_capture shows them absent. Absent only, never wrong.
Schema stamp. fieldMinReaderSchemaVersion (manifest.ts:1018-1035) stamps 5 for Egress-only. captureLifecycle.test.ts:435-458 pins 5 for a background ingest with an Egress and no engine fields. The e2e change to 6 is correct, because recapture.ts:236-237 always passes inventory.

Evidence gate. Touched blocking-tier paths: backgroundRenderer.ts, recapture.ts, captureLifecycle.ts, and db/**. The PR and #1928 both carry evidence-affecting. The Evidence impact section is substantive but incomplete (finding 2). No hash or signing method changed, so no chain KAT applies, and the renderer tests act as known answers except for finding 3. Backward verification: an ingest with neither field omits both and stamps below 6 (captureLifecycle.test.ts:286). The reliability probe flags that test as passing with the source reverted, which is expected for an omission guard.

Completion controls. Attribution: the body ends with the Created with Claude Opus 5.5 in Claude Code (T3 Code) byline, which lint-body.mjs:15 accepts. Co-authored-by trailers: 0 across the 3 commits. auto_merge: null. The fix is its own commit (0b0709b3, backgroundRenderer.ts only). merge-gate is red only for "evidence-affecting with no sign-off at head", as expected.

CI at head. lint, typecheck, build, test, e2e, docs-build and probe are green. The reliability probe reports 6 surviving mutants: :225 and :314 are equivalent in effect, and :410 and :434 are finding 3.

Commands (exit codes).

pnpm lint 0; pnpm typecheck 0; pnpm build 0
BIRDBRAIN_TEST_SYSTEM_SHARP=1 BIRDBRAIN_REQUIRE_OPENSSL=1 BIRDBRAIN_REQUIRE_JQ=1 pnpm test:coverage 0: 8011 passed, 24 skipped
node scripts/diff-coverage.mjs --base origin/feat/1927-report-capture-engine-fields 0: 31/31 lines, 100%
unshare -rn ... xvfb-run -a npx playwright test e2e/recapture.spec.ts e2e/recapture-egress.spec.ts 0: 5 passed
.claude/hooks/vale-prose.sh --base origin/feat/1927-... 0
Mutant at :434 with pnpm test tests/main/services/backgroundRenderer.test.ts 0: 39 passed, tree restored
Electron scroll probes (main.js, loop.js) 0

Not checked. Windows and macOS. pnpm test:reliability locally (CI's probe read instead). The test-reliability-reviewer agent (not spawned, as instructed).

Added 6 commits4d ago

test(recapture): pin the untrimmed screenshot at the padding edgea30c96f
fix(recapture): fail a render whose scroll lock cannot be restored5c465a4
fix(recapture): scroll inner panels once the document is at its end5154935
docs(capture-engine): state what the scroll fix changes in acquisition45d7236
fix(logging): register the Scroll-to-load reset failure code7d30955
fix(db): check the two mirrored hashes on readb3a68d4

MMatt Donovan4d ago

_{Written by claude-opus-5-5 via Claude Code on behalf of Matt.}

Dispositions for the review verdict above, at the latest commit.

An inner panel never scrolled once the document reached its end: applied; the review's reproduction now scrolls the inner panel to its full height and loads every lazy chunk, and an app-shell page joins the end-to-end tests.
The evidence notes understated the change: applied; the validation record, the description and the glossary state the real effects, including that a slow-growing feed now runs to the height cap or the time limit.
The usual uncropped screenshot case had no test: applied; the surviving change at the padding check now fails two tests, and the unreachable guard above it is removed.
Zero pinned-image counts: not applied in code, since this build pins no image; the maintainer ruled that no release carries this step without #1843, and the description says so.
Two scrolling panes: applied with the first item; both panes are scrolled and both load their chunks.
A failed scroll-lock restore was swallowed: applied; the render now fails, because the signed record has no field that could state the failure.

Commits

5154935d (findings 1 and 5), 45d72361 (finding 2), a30c96fd (finding 3), 5c465a46 and 7d309557 (finding 6)

Added 12 commits3d ago

feat(verify): count requests that finished with no response30605f8
docs(capture-engine): define the finished-without-response count083dba1
chore(merge): take the finished-without-response count from #19789b9ec9e
chore(merge): take the finished-without-response count from #1980fd542de
chore(merge): take the finished-without-response count from #1982144340c
chore(merge): take the finished-without-response count from #1984e46f9d3
feat(report): list the finished-without-response count19267c2
chore(merge): take the finished-without-response count from #1985f7a6b98
test(export): pin the per-file gate for an entry anchoring one file74a1b8a
chore(merge): take the per-file gate test from #19848a2bae8
test(export): pin the chain gate on capture engine fields in an exporte1dcee4
test(report): pin capture engine fields from Shared Case member chainsc7219ba

Bbirdbrain-agent3d ago

Review verdict: request changes

A background capture of a page that opens already at its bottom, such as a link to a section near the end, now signs that a scroll lock was lifted when the page had none, and the report repeats the claim; the version reviewed last round did not do this.
The earlier findings are fixed: inner panels and both panes scroll and load their content again, the evidence notes describe the real effects, the screenshot edge case is tested, and a lock that cannot be put back fails the capture. I broke each fix on purpose and a test failed every time.
The summary that becomes the merge commit says the change sits on five open drafts, but the lowest of them has merged, and it reads as if inner panels are newly scrolled when the build before this change already scrolled them.
Checks are green at this commit apart from the expected missing sign-off; the change merges cleanly onto its parent branch, and its conflicts with main come from the drafts below it.

Full report

Reviewed commit: f7a6b98fb9b670c5de1e63bac84fb771d216441c (delta from bdb25c969bafcd6a777af51ca3e7fe11bfb391df). Head re-read at the end: unchanged.

#	severity	file:line	finding
1	blocking	src/main/services/backgroundRenderer.ts:189, :153, :325	5154935d made `canScroll` down-only again, and `SCROLL_UNLOCK_SCRIPT` still calls `pickRoot()` without `allowAtEnd`. A document that overflows and sits at its end when the phase starts reads as locked, the script sets `overflow: visible` on html and body, and the entry signs `scrollLockLifted: true`. reportHtml.ts:1958 then says "It lifted the page's scroll lock", and D5 (plan :387) defines the field as a consent wall's lock being lifted.
2	advisory	PR body, Summary	"five open drafts ... (#1984, #1982, #1980, #1978)": #1978 merged at 2026-10-27T18:19:51Z. "it now ... scrolls inner panels and both panes of a two-pane page" reads as new behaviour; the build before step 6 already scrolled both (Electron probe below, and the body's own Evidence impact says "scrolled as before").
3	advisory	src/main/services/reportHtml.ts:1955 (from #1985); plan D5 :386	The report says "The last document height it read", and D5 calls `endHeight` the last document height. On an app-shell or two-pane page it is the panel's height. CONTEXT.md is now right, but the report sentence and D5 are not.
4	advisory	src/main/services/backgroundRenderer.ts:286	With `===`, a page whose height keeps shifting down and up (estimated virtual lists) never stalls and runs to the time limit. Shrinking used to count as no growth. It is bounded by the phase budget and the record says "a change in height", but the acquisition-effect paragraph names only the slow feed.
5	advisory	backgroundRenderer.ts:232, :335-336, :451	Probe survivors at head. :232 (`>=0` to `>0`) has the same effect for an honest page. :335 and :336 mean the PNG signature and IHDR checks are not each pinned. :451 (`<0` to `<=0`) changes `trimmed` only for an image whose sole content row is row 0.

Failure scenario for 1, reproduced in real Chromium. Electron from the tree under xvfb-run, an offscreen 1280x900 window, and the step and unlock scripts extracted verbatim from each source. The stall logic is the renderer's: === for head, <= for the others. The page is <body><div style="height:5000px"> followed by <p id="end">, loaded with hash #end, so it starts at scrollTop 4579. This is the state wc.loadURL(url) (:639) leaves when the operator's URL carries a fragment near the end, because recapture.ts:166 passes job.url raw. A page that scrolls itself to the bottom on load gives the same state.

Base b4bd9b71: scrollLockLifted would be true (base signs no record).
First fix bdb25c96: false.
Head: true, stopped-growing at 5058.
Head with the one-line change if (pickRoot(true)) return null in the unlock script: false. On real locks (html,body{height:100%;overflow:hidden}, at the top and with #end) it still lifts (true), with stopped-growing at 5042.

No test covers the unlock decision: the scrollingContents mock returns a canned unlock result (backgroundRenderer.test.ts:352). Script: ~/.cache/pr1996/scratch/probe.js.

Earlier blockers, each mutated (pnpm test tests/main/services/backgroundRenderer.test.ts, tree restored after each run).

Finding 1 (inner panel): the mutation returns the document whenever it is at its end. 2 tests fail: the app-shell panel and the grown panel. Dropping || end fails 3.
Finding 3 (trim edge): cropHeight >= h to >. 2 tests fail.
Finding 6 (restore): the throw at :316 disabled. 1 test fails.
New stall rule: === back to <=. 1 test fails (the second pane).
Electron probe at head against the validation record's table: app shell stopped-growing 10001, 27 steps, 4 chunks; two panes stopped-growing 6000, 22 steps, both scrolled. Both match the record. A two-pane page of equal heights also scrolls both panes (26 steps), which rules out a stall-inheritance worry I had. I did not re-run the 30 s endless-feed row.

Merge resolution (f7a6b98f). The only conflict was the plan's decision log, and both entries are kept. Per file, the PR's own diff against the new base matches the diff of its own commits, so the merge dropped nothing.

Evidence gate. Blocking-tier paths touched in this round: backgroundRenderer.ts. Across the PR: recapture.ts, captureLifecycle.ts and db/**. The PR, #1928 and #1667 all carry evidence-affecting. Advisory-tier: src/shared/types.ts, where the delta adds a LOG_CODES entry only and does not reach evidence. The Evidence impact section is substantive and now states the scroll effects, but it omits finding 1. KAT: renderer tests pin root choice, stall and D6 values; no hash or signing method changed. Backward verification: manifestSchema6, manifestSchema6TooOld, captureLifecycle and captureEngineColumnsMigration all pass.

Bot findings. CodeRabbit posted only its draft-skip notice. There are no review threads and no other bot comments.

Completion controls. The body passes post-pr-body/scripts/check.sh (exit 0) and ends with the Created with ... byline. Co-authored-by trailers: 0 across the 9 PR-own commits. auto_merge: null.

CI at f7a6b98f. lint, typecheck, build, test, e2e, docs-build, probe and hunks pass. merge-gate is red only for "evidence-affecting with no sign-off at head", as expected. agent/pre-pass is pending (this review).

Commands run locally (exit codes). Touched unit files under BIRDBRAIN_TEST_SYSTEM_SHARP=1:

backgroundRenderer: 50 passed
recapture: 34
captureLifecycle: 120
captureDuplicate: 40
captureEngineColumnsMigration: 6
reportCaptureEngine: 40
manifestSchema6: 39
manifestSchema6TooOld: 5

All exited 0. The full loop (lint, typecheck, build, coverage, e2e) is left to CI once the PR retargets to main.

Trial merges (git merge-tree).

Against origin/feat/1927-report-capture-engine-fields at c7219ba3: clean. The base has moved 4 test-only commits past the head's merge.
Against origin/main at f684808a: conflicts in the plan, src/shared/verify/manifestChain.ts and tests/verifier/binary.test.ts. The base conflicts with main in the same way, so these are inherited from the stack after #1978's squash.

Not checked. Windows and macOS. The local e2e run (CI e2e is green at head). The endless-feed row of the validation record. The test-reliability-reviewer agent.

Added 6 commits3d ago

chore(merge): take the capture engine field tests from #198584cf6c2
fix(recapture): lift no scroll lock on a page that starts at its end10b4aa9
test(recapture): pin the PNG checks, a first-row trim and a zero height51e32bb
docs(capture-engine): say a height that changes every step never stalls92b6453
fix(report): say the end height is the scrolled root's height7621203
docs(capture-engine): record the scroll lock run before step 67b449b2

Bbirdbrain-agent3d ago

Dispositions for the review verdict above, at the latest commit.

A page that opens at its bottom read as scroll-locked: applied. The lock check now makes the same root choice as the scroll step, and a new test runs the real lock, scroll and restore scripts against a simulated page at its end; it fails without the fix. In a real Chromium run the page loaded at its end fragment now records no lock lifted, and a locked page still has its lock lifted and put back.
The summary's list of open drafts and the inner-panel sentence: applied in the description. It names the four open pull requests below this one, and says the scroll now ends when the page or inner panel stops growing rather than implying inner panels are newly scrolled.
The report and the plan called the end height the document's: applied. The report sentence, the plan's definition and the features page now say it is the height of the document or inner panel scrolled.
A page whose height keeps moving down and up never stalls: applied. The validation record says so, with the comparison to the earlier rule, and two tests pin that such a page runs to the time limit.
The mutations that survived the probe: applied. New tests cover a page reporting a height of zero, a screenshot header cut short, a wrong signature byte, a first chunk that is not the header, and a page whose only content is its first row; I re-ran each surviving change and each now fails one of them.

Commits

10b4aa9c and 7b449b26 (finding 1), 7621203c (finding 3), 92b64534 (finding 4), 51e32bb0 (finding 5). Finding 2 is in the description only. 84cf6c27 merges the base branch at c7219ba3, with no conflicts.

Bbirdbrain-agent3d ago

Review verdict: request changes

The fix works: a page that opens at its bottom no longer signs that a scroll lock was lifted, in a real browser run and in a new test that fails when the fix is undone.
The written test record says a locked page opened near its end still has its lock lifted, which holds only for an old-style page; on the same page in the modern style the earlier build lifted the lock and scrolled all 5,000 pixels, and this change lifts nothing and stops after 8, which the evidence notes do not say.
The merge summary says the change sits on four open pull requests, but one of them merged today, so three remain.
The unit-test check is red only because image-processing test files crashed on the newer CI machine, with no test failing; I ran the changed test files here and all passed.

Full report

Reviewed commit: 7b449b26c2daf7e549af94429c7653894d260a38, a delta from f7a6b98fb9b670c5de1e63bac84fb771d216441c. I read git diff 84cf6c27 7b449b26, which leaves out the base merge. Head re-read at 18:58Z: unchanged.

#	severity	file:line	finding
1	blocking	docs/specs/2026-10-26-capture-engine-first-slice-validation-assessment.md:97-102; PR body, Evidence impact; tests/main/services/backgroundRenderer.test.ts:617-618	The record's lock runs used a page with no doctype and `height: 100%` on `html` and `body`, where the body is the scroller. It describes them as plain "`overflow: hidden` on `html` and `body`". With a doctype, Chromium scrolls an `overflow: hidden` document from script. On a full-height locked page loaded at the fragment, the document's 8px margin then puts it at its end: b4bd9b71 and f7a6b98f lifted the lock and scrolled to 5042, while head lifts none and ends `stopped-growing` at 487. That acquisition change is unstated, and the test comment's "as Chromium's scroll-locked viewport does not" is false for a standards-mode document.
2	advisory	PR body, Summary	"four open pull requests ... (#1984, #1982, #1980)": #1980 merged at 2026-10-27T18:47:07Z.
3	advisory	PR body, Summary	"no longer treats a page that opens at its bottom as scroll-locked" also covers finding 1's locked page. "ends when the page or inner panel it scrolls stops growing" does not hold for a height that falls at every step.
4	advisory	docs/plans/2026-10-10-capture-engine-first-slice.md:388	"absent when the loop read none": `endHeight` is also absent when the last step's height is not a non-negative integer, because backgroundRenderer.ts:274 overwrites it each step. The clause predates this round, but the round rewrote the paragraph around it.
5	advisory	src/main/services/backgroundRenderer.ts:134	The comment that a viewport locked by `overflow: hidden` "clamps even programmatic scrolling" predates the PR, and in Chromium standards mode it is false. The unlock lifts only when the document cannot move from script, as in quirks mode.

Finding 1, Electron runs. I used Electron 44.9.1 from the tree under xvfb-run: an offscreen window with a 479px viewport, the unlock and step scripts extracted verbatim from each source, and the renderer's stall rule. Script: ~/.cache/pr1996r3/scratch/probe.js.

The page is a 5000px block and a #end paragraph:

page	b4bd9b71	f7a6b98f	head
unlocked, at `#end`	lifted, nothing-to-scroll	lifted, stopped-growing 5058	not lifted, stopped-growing 5058
no doctype, `html`/`body` `height:100%;overflow:hidden` (the record's page), top	lifted, 5042	lifted, 5042	lifted, 5042
same, at `#end` (document scrollTop 0, body scrollTop 4571)	lifted, 5042	lifted, 5042	lifted, 5042
doctype, `html`/`body` `overflow:hidden`, top	not lifted, document scrolled from script to 5066	not lifted, 5066	not lifted, 5066
same, at `#end`	lifted, nothing-to-scroll	lifted, 5058	not lifted, 5066
doctype, `html`/`body` `height:100%;overflow:hidden`, top	not lifted, 487	not run	not lifted, 487
same, at `#end` (document at its 8px end)	lifted, scrolled, 5042	lifted, 5042	not lifted, stopped-growing 487

My round-2 report called the no-doctype page a "real lock" loaded "with #end". That description fed the record's sentence, and it was wrong on both counts: the page is quirks mode, and the fragment moves the body rather than the document. A prose-only fix will do. Describe the run's page in full. Add the doctype rows. State the last row as an acquisition change in Evidence impact. Correct the test comment so it says the fake models a document that cannot move from script.

Blocker from round 2: fixed.

With the unlock reverted to pickRoot(), pnpm test tests/main/services/backgroundRenderer.test.ts fails 1 test, "lifts no lock on a page that starts at its end". It passes at head.
atEnd mutated to drop moves(el, -1): 3 tests fail, including both locked cases.
The tree was restored after each run.
In Chromium the unlocked page at #end now records no lift, as the table shows.

vm-context simulation.

It runs the real unlock, step and reset strings through runInNewContext, so the decision under test is the shipped code.
Gaps: no inner boxes, because querySelectorAll returns []. window.scrollTo is a no-op. The body is not a scroller.
Its overflow: hidden model matches the quirks page and does not match a standards-mode document.
It pins the at-end and doc-locked branches, and does not pin finding 1's page.

Claim against b4bd9b71. The body says the build before step 6 lifted a lock on a page loaded at a fragment near its end. My runs confirm it: lifted, nothing-to-scroll in one step.

Other delta items.

Report sentence, D5 wording and features.mdx match the step script, which reports root.scrollHeight.
Oscillating-height paragraph and tests: correct. Note that the "moves down and up" case also never stalls under the old <= rule, so only the "falls" case tells the two rules apart.
The tests for the round-2 probe survivors are right.

Bot findings and review surface. All fetched with --paginate:

0 review comments and 0 reviews.
Issue comments: CodeRabbit's draft-skip notice, which raises no finding. The round-1 and round-2 verdicts and their dispositions, all addressed.

Evidence gate.

Blocking-tier paths touched this round: backgroundRenderer.ts and reportHtml.ts. The PR, #1928 and #1667 all carry evidence-affecting.
Evidence impact is substantive apart from finding 1.
KAT: no hash or signing method changed, and the renderer and report tests pin exact values.
Backward verification: manifestSchema6 (39), manifestSchema6TooOld (5) and captureEngineColumnsMigration (6) pass.

Completion controls.

auto_merge is null.
Co-authored-by trailers: 0 across 13 PR-own commits (git log origin/feat/1927-report-capture-engine-fields..7b449b26).
The body passes post-pr-body/scripts/check.sh (exit 0) and ends with the byline.

CI at 7b449b26.

Pass: lint, typecheck, build, e2e, docs-build, hunks, probe.
test is red. Workers hit SIGSEGV on the ubuntu-26.04 image in export, exhibitModel, backgroundRenderer, thumbnails and burnAnnotations. Counts: 7842 passed, 24 skipped, 0 failed, 5 errors. This is the sharp infrastructure crash, but backgroundRenderer's tests and diff coverage did not run in CI.
Test reliability: 0 mutants survived. backgroundRenderer.ts was not probed because its tests crashed. Two revert survivors are absence checks whose behaviour other tests pin.
merge-gate is red only for "evidence-affecting with no sign-off at head".

Local runs. BIRDBRAIN_TEST_SYSTEM_SHARP=1, Node 20, all exit 0:

backgroundRenderer 60
reportCaptureEngine 41
recapture 34
captureLifecycle 120
captureDuplicate 40

The full loop is left to CI once the PR retargets to main.

Trial merges (git merge-tree).

Against base c7219ba3: clean, and the base is an ancestor of head.
Against main 07ffcea6: conflicts in the plan, captureRepo.ts, manifestChain.ts, caseArchiveRoundTrip.test.ts and binary.test.ts. The base conflicts on four of these. captureRepo.ts comes from #1980's squash against the stack's copy.

Not checked.

Windows and macOS.
The endless-feed row.
Whether a quirks-mode lock with a tall inner panel at its end now goes unlifted.
The test-reliability-reviewer agent.

Added 2 commits3d ago

docs(capture-engine): record the scroll lock on pages with a doctypefbe12b3
test(recapture): pin no end height after an unrecordable last height8c61fd8

Bbirdbrain-agent3d ago

Dispositions for the review verdict above, at the latest commit.

The written record overstated when a locked page opened near its end still has its lock lifted: applied. I re-ran the lock in a real browser on pages with and without a doctype, reading each page's mode, and the record now has a table of every page loaded at its top and at its end. It states that a modern full-height locked page opened at its end now keeps its lock and stops at 487 pixels, where the build before this step scrolled to 5042, and the description's evidence notes say the same. The test comment now says the simulated page models a page that cannot be scrolled from a script, which in Chromium is an old-style page.
The summary counted four open pull requests: applied in the description, which now names three.
The summary's lock and scroll-ending wording: applied in the description. It says the scroll ends once the page or panel has sat at its bottom, or stopped moving, with no change in height for three steps, and that a lock is lifted only when nothing can be scrolled up or down.
The plan's account of when the end height is absent: applied. It now covers a last height that is not a whole number, and a new test fails if an earlier height were kept instead.
The code comment saying a hidden overflow stops scrolling from a script: applied. It now says Chromium still moves such a page in standards mode, and does not move a full-height one in quirks mode.

Commits

fbe12b34 (findings 1 and 5), 8c61fd85 (finding 4). Findings 2 and 3 are in the description only. The base branch is still at c7219ba3, so no merge was needed. No acquisition code changed: the renderer diff this round is the comment above SCROLL_HELPERS, outside the script strings.

Bbirdbrain-agent3d ago

Review verdict: request changes

The earlier problems are fixed: the test record's new table matches my own browser runs on every row, the change on a modern full-height locked page opened at its end is now stated, and this round's code change is a comment plus one new test that fails when its behaviour is broken.
A new limitation note in the test record says the scroll never moves the page body, but on old-style pages without a doctype the body is the page's own scroller and the scroll moves it, as the record's own table shows.
The record and the evidence notes say a modern full-height locked page opened at its top ends "stopped growing" in both builds, but the earlier build ended it as "nothing to scroll"; only the 487 pixel stop is shared.
The summary says this sits on three open pull requests, but one of them merged at 19:23 today, so two remain.
The unit-test check is red only because image-processing test files crashed on the newer CI machine, with no test failing; the five changed test files pass here.

Full report

Reviewed commit: 8c61fd85c6ecfff0f008f3539bf609aa19ace0f5, a round from 7b449b26c2daf7e549af94429c7653894d260a38 (fbe12b34 docs and a comment, 8c61fd85 one test and the plan). Head re-read at 2026-10-27T19:35Z: unchanged.

#	severity	file:line	finding
1	blocking	docs/specs/2026-10-26-capture-engine-first-slice-validation-assessment.md:151	"The step never scrolls `body` itself" is false in quirks mode. `doc` is `document.scrollingElement` (src/main/services/backgroundRenderer.ts:156), which is `body` on a `BackCompat` page, and the step scrolls `doc` first. The record's own table rows (no doctype, 5058 and 5042) are runs that scrolled `body`.
2	advisory	same file:122-124; PR body, Evidence impact	"ends `stopped-growing` at 487, as both builds do for the page loaded at its top": the table row for that page gives the build before step 6 `nothing-to-scroll` at 487.
3	advisory	PR body, Summary	"three open pull requests, #1985 and the two below it (#1984, #1982)": #1982 merged at 2026-10-27T19:23:06Z, after the body was written.
4	advisory	docs/plans/2026-10-10-capture-engine-first-slice.md:388	"not a non-negative integer" omits that `recordableHeight` also drops an integer above 2^53 (`Number.isSafeInteger`, backgroundRenderer.ts:235). It does not change the decision.

Corrected wording.

Record line 151: "In standards mode the step never scrolls body: the document is html, and its inner roots are the descendants of body. A page with a doctype and ..." (the rest stands).
Record lines 122-124 and the Evidence impact sentence: "... lifts nothing, and ends stopped-growing at 487; loaded at its top, that page stops at 487 in both builds."
Summary: "It is built on two open pull requests, #1985 and the one below it (#1984), and the session that started this review round posts this description and the review replies."

Finding 1, trace. backgroundRenderer.ts:156 sets doc = document.scrollingElement || ...; pickRoot returns doc when it can move down (:163), and inner candidates come from body * (:169). With no doctype and body at overflow: visible, scrollingElement is body. After the unlock sets overflow: visible on a quirks full-height page, scrollingElement turns from null to body. My probe printed the step's chosen root: BODY(doc) on quirks-open, quirks-lockFull and quirks-bodyScroller, in both builds. The statement is only true for a standards-mode document, which is what line 120 already says correctly ("its inner roots are the descendants of body").

Electron runs (reviewer's own). Electron from the tree under xvfb-run, a 479px viewport. The unlock and step scripts came from git show of c7219ba3 and 8c61fd85, each with its own stall rule (<= before, === head). It printed compatMode, scrollingElement and the chosen root. Script: ~/.cache/pr1996r4rev/probe/probe.js, output probe.out beside it. Every one of the record's ten rows matched, as did the implementer's probe.out.

c7219ba3 and b4bd9b71 have the same renderer file (sha256 8205f970...). The implementer's head copy equals 7b449b26.
7b449b26..8c61fd85 in src/ changes only the comment above const SCROLL_HELPERS, at lines 133-137. No script string changes.
An extra case, doctype with html height:100% and body height:100%; overflow:auto: both builds scroll the document to 5042. body's overflow propagates to the viewport, so it is not a body scroller. This is consistent with the record.

Earlier findings (comment 6450961525).

Lock claim, the 487px change, the test comment: resolved. The table is right, the Evidence impact states the 487 case, and the test comment at backgroundRenderer.test.ts:635 is accurate. The new limitation bullet introduces finding 1 above.
Four open PRs: changed to three; stale again (finding 3).
Summary lock and ending sentences: resolved. "with no change in height for three steps in a row" matches the stall rule at backgroundRenderer.ts:284-296. "lifts a scroll lock only when it finds nothing it can scroll up or down" matches the unlock at :192-193.
Plan end height: resolved. Mutating :275 to recordableHeight(state.height) ?? endHeight fails "records no end height when the last height read is not recordable" (1 failed, 60 passed). Restored afterwards.
Code comment: resolved. It agrees with the probe: the standards-mode lock moves, the quirks full-height one does not.

Review surface (--paginate). 0 reviews and 0 review comments. Issue comments: CodeRabbit's draft-skip notice (no finding), the earlier verdicts and their dispositions. The round-3 disposition matches the commits, except that its "three open pull requests" went stale after it was posted.

Evidence gate. The PR, #1928 and #1667 carry evidence-affecting. Blocking-tier paths touched: backgroundRenderer.ts, recapture.ts, captureLifecycle.ts, reportHtml.ts. Advisory: src/shared/types.ts, the log-code union only. Evidence impact is substantive apart from finding 2. KAT: no hash or signing change, and the renderer tests pin exact values. Backward verification passes locally: manifestSchema6 39, manifestSchema6TooOld 5, captureEngineColumnsMigration 6.

Completion controls. auto_merge null. Co-authored-by: 0 across 15 non-merge PR commits. The body passes post-pr-body/scripts/check.sh (exit 0) and ends with the byline that check accepts.

CI at 8c61fd85.

Pass: lint, typecheck, build, e2e, docs-build, hunks, probe.
test failed: SIGSEGV in export, exhibitModel, backgroundRenderer, thumbnails and burnAnnotations. 7843 passed, 24 skipped, 0 failed, 5 errors. That is the base branch's runner, so CI ran no backgroundRenderer tests and computed no diff coverage. The PR body's preflight reports 100% of 226 lines; I did not re-run coverage.
Test reliability: 0 mutants survived. backgroundRenderer.ts and captureRepo.ts were not probed (their tests crash). The two revert survivors are absence checks, as last round.
merge-gate is red only for "evidence-affecting with no sign-off at head". agent/pre-pass is pending on this review.

Local runs. Node 20, BIRDBRAIN_TEST_SYSTEM_SHARP=1, all exit 0: backgroundRenderer 61, captureDuplicate 40, captureLifecycle 120, recapture 34, reportCaptureEngine 41. eslint is clean on the two changed TS files.

Trial merges (git merge-tree).

Against the base c7219ba3: clean, and the base is an ancestor.
Against main 14bf7977: 11 files conflict. The base alone conflicts on 8 of them. The PR adds captureRepo.ts, captureDuplicate.test.ts and captureLifecycle.test.ts, from #1982's squash against the stack's copy.

Not checked. The full suite and coverage, which were out of scope for this round. Windows and macOS. The test-reliability-reviewer agent.

Added 3 commits3d ago

docs(capture-engine): limit the body claim to standards mode15024fe
docs(capture-engine): say an unsafe integer height is left out too038d6c1
chore(merge): take main after #1984 and #1985 squash-mergedf56233b

Bbirdbrain-agent3d ago

Dispositions for the review verdict above, at the latest commit.

The record said the scroll never moves the page body, which is false on pages with no doctype: applied. The limitation now says that in standards mode the step never scrolls the body, in the wording the review gave, and keeps the rest of the note.
The record and the evidence notes said both builds end the full-height locked page "stopped growing" when it opens at its top: applied in both. They now say only the 487 pixel stop is shared.
The summary named open pull requests the branch sits on: applied in the description. Both remaining ones have merged, and this branch now carries the main branch, so the summary says that instead.
The plan's rule for when the end height is left out missed whole numbers too large to hold exactly: applied. The plan now names the safe-integer check, and the evidence notes say "safe integer" to match.

Commits

15024fe8 (findings 1 and 2, the validation record), 038d6c1b (finding 4, plan D5). f56233be merges main at aa87bd36, after #1984 and #1985 squash-merged; its content diff against main equals this branch's diff against c7219ba3. Findings 2 and 3 also change the description, and the description's Changes line for the record now says the body limitation is for standards mode. No acquisition code changed this round.

Bbirdbrain-agent3d ago

The coordinator checked this round and passed it at the latest commit. The two documentation commits apply the review's corrected wording verbatim: the standards-mode limit on the body claim, and the 487 pixel sentence. They also add the safe-integer note to the plan, and they change no code. The merge of main that follows comes after every pull request below this one squash-merged. The coordinator rebuilt it independently from the commit this branch last took from its base and got a tree identical to this commit's, so the resolution adds no line of its own. The full local check run passed at this commit.

Bbirdbrain-agent3d ago

Review verdict: request changes

The end-to-end CI check fails at this commit: the three end-to-end tests this PR adds to the Recapture spec time out waiting for the new-case form on every retry, so the reviewer was not run.

Full report

Reviewed commit: f56233be9095ea88b5a5162fca7cbe8e7fc319ed

#	severity	file:line	finding
1	blocking	e2e/recapture.spec.ts:66	CI check `e2e` fails: the three end-to-end tests this PR adds time out waiting for the new-case form, on every retry.

CI is authoritative over any review, so the reviewer was not spawned and this verdict covers CI only. Every other check passed, and the most recent CI run on main (aa87bd36) succeeded, so the e2e failure belongs to this PR.

The failing tests are records how Scroll-to-load ended and the screenshot facts, signed and mirrored (line 234) and the app-shell and two-pane cases (line 339). Each fails at its first step: queueUrl sets the hash to /cases/new, and page.waitForSelector('[data-testid="case-name-input"]') at line 66 times out after 10 s. The test that runs first in the same describe (line 134) passes, so the failure happens before any scroll or screenshot code runs. That points at the page state the earlier test leaves, or at how the helper navigates, rather than at the renderer change; the error-context.md files in the Playwright artifact of run 40577486809 show what the page held at the timeout.

Added 3 commits2d ago

Merge branch 'main' into feat/1928-scroll-and-screenshot-record7de1262
chore(merge): take main after the Shared Cases top bar and dashboard63ea04c
fix(e2e): open the blank new-Case form in the Recapture helperc2d35f6

Bbirdbrain-agent2d ago

Applied.

Commit

c2d35f66. The failure predates the merge that brought in the Case settings fix: CI run 40577486809 at f56233be fails the same three tests. It came in with f56233be's merge of main, which brought #1972. Since #1972 a bare /cases/new opens the start step, and only /cases/new?blank=true shows the new-Case form. The PR's queueUrl helper used the bare route, so case-name-input never appeared. The first test in the describe has its own copy of the flow with ?blank=true, which is why it passed. Locally at the merged head, with the app built, xvfb-run -a npx playwright test e2e/recapture.spec.ts --retries=0 failed those three tests at recapture.spec.ts:66 (exit 1) before the fix. After it, the two Recapture specs passed 7 of 7 (exit 0).

Bbirdbrain-agent2d ago

Review verdict: approve for human review

Hold the merge until a release carries the new verifier, or set that rule aside: the change's own summary and the approved plan both say this capture step merges only after such a release, and the newest release (12 October) is older than the verifier.
The earlier blocker is fixed: the end-to-end check passes in CI, and when I undid the one-line fix the three new end-to-end tests failed again at the same step.
Both merges of the main branch went through with no conflicts and left the change's own edits identical apart from that one line, and the branch already contains the latest main, so it merges cleanly.
Two sentences in the summary that becomes the merge commit need rewording: it says every copy of a capture carries the new facts, but a copy received by a Shared Case member leaves them out of its database row, and its last sentence about which session posts the description means nothing in the history.

Full report

Reviewed commit: c2d35f668d24780f3c9cf89ddfcff25b6fb4866b (previous review at f56233be9095ea88b5a5162fca7cbe8e7fc319ed)

#	severity	file:line	finding
1	advisory (merge sequencing, maintainer call)	PR body, Summary, first sentence; docs/plans/2026-10-10-capture-engine-first-slice.md:965-966, :1443; issue #1928, Order	The plan says steps 6, 9 and 10 "each merges only after the release" carrying step 1's verifier, and the Summary opens "Do not merge this before the maintainer tags a release whose verifier reads the new fields it signs". Step 1 (#1978) merged 2026-10-27T18:19Z; the newest tag and release is v1.0.1-beta.23 (2026-10-12, `gh api repos/.../tags` and `.../releases`). The condition is unmet at this sha. No change to the PR fixes it, so this is not a code finding, but an approve here is not permission to merge under the plan as written.
2	advisory	PR body, Summary, second-to-last sentence; src/main/services/sharing/receivedEvidence.ts:248-279	"the capture's database row and any copy of the capture carry both facts" overclaims: a Capture received through Shared Case sync is inserted with no `inventory` or `scrollToLoad` in its params, so its row mirrors stay null while its signed entry carries them. The Changes section discloses this, but Changes is not in the squash body.
3	advisory	PR body, Summary, last sentence	"the session that started this review round posts this description and the review replies" is review-process text that will sit in the squash commit body, where it describes nothing.
4	advisory (environment, not the PR)	e2e/recapture-egress.spec.ts:150, :183	Locally two Egress tests failed at head and one of them failed identically on origin/main 2d5f1594. I instrumented the fixture server: the stray direct request is `GET /` with `user-agent: node`, `host: localhost:<port>` and `b3`/`traceparent` headers, which nothing in the repository sends, arriving once at each fixture server, on that server's port. CI's `e2e` job passes both tests at this sha.
5	advisory	test reliability probe, src/main/services/backgroundRenderer.ts:307, :338	Two surviving mutants: the `userGesture` flag on the reset `executeJavaScript`, which no test can observe, and `png.length < 24` changed to `<= 24`, which only changes the verdict on a 24-byte buffer that cannot be a complete PNG. I judge both equivalent. The two tests that pass with the source reverted are absence checks (`ingest OMITS ...`, `states a phase that ran with no outcome`); the positive report sentence is pinned at tests/main/services/reportCaptureEngine.test.ts:259.

Corrected wording for the Summary (squash body):

Finding 1, the first sentence becomes: "Ship this only in a release that also carries #1843: until that pinning lands, the zero image-pinning counts it signs cannot be told apart from "pinning ran and found nothing" except by the tool version." The merge-order instruction belongs in the PR conversation, not in the commit history.
Finding 2: "..., and the capture's database row and the row of a Duplicate of it carry both facts; a Capture received through Shared Case sync carries them in its signed entry but not in its row."
Finding 3: "It builds on #1985 and the pull requests below it, all merged."

Earlier blocker (CI e2e at f56233be): fixed. queueUrl now sets /cases/new?blank=true (e2e/recapture.spec.ts:64). This matches the 38 other e2e uses of that route, and the route reads blank at src/renderer/components/dashboard/cases/NewCaseWizard.tsx:48-58,123. As a mutation check I reverted line 64 to /cases/new, rebuilt, and ran xvfb-run -a npx playwright test e2e/recapture.spec.ts --retries=0: 3 failed at queueUrl (recapture.spec.ts:66) and 1 passed, exit 1. At head (before the mutation), all 4 recapture.spec.ts tests passed; recapture-egress.spec.ts passed 1 of 3 locally, for the environmental reason in finding 4. The reply's claim that #1972 came in through f56233be's merge checks out: 200a8f08 (#1972) is in parent aa87bd36 and not in 038d6c1b.

Delta review: git diff f56233be..c2d35f66 minus the main merges. I compared git diff aa87bd36 f56233be with git diff origin/main c2d35f66: both have 18 files, +1451/-66, and the only difference is the queueUrl line. Main's 81 changed files since aa87bd36 (#2007 to #2011, all sharing UI and docs) overlap none of the PR's 18. git show --remerge-diff is empty for 7de12628 and for 63ea04ce, so neither merge resolved anything by hand.

Trial merge: origin/main 2d5f1594 is an ancestor of head, and git merge --no-commit --no-ff origin/main reports "Already up to date". Clean.

Stacked PRs: #2000 and #2002 have branch bases. #1999 has base main and carries this branch's commits, so after this squash-merges #1999 needs a rebase onto main to drop them.

CI at c2d35f66 (gh pr checks 1996): lint, typecheck, build, test (includes diff coverage), e2e, probe, docs-build, hunks, adr-numbers, security checks all pass. merge-gate fails with "evidence-affecting with no sign-off at head", which is expected and needs the maintainer's approval at head. agent/pre-pass is pending on this review.

Local verification (Node 20 via mise, scratch worktree): touched unit files (backgroundRenderer, captureDuplicate, captureLifecycle, recapture, reportCaptureEngine) 5 files, 297 tests passed, exit 0. pnpm build exit 0. I left the full coverage suite and diff coverage to CI's test job at this sha.

Evidence gate: the PR, #1928 and #1667 all carry evidence-affecting. The blocking-tier paths backgroundRenderer.ts, recapture.ts and captureLifecycle.ts are touched. The Evidence impact section is substantive and its KAT justification stands (no hash, signature or canonical-form change). The delta adds no evidence-path change, so the earlier rounds' evidence review carries forward.

Completion controls: auto_merge is null. git log origin/main..HEAD --format=%B | grep -i co-authored finds nothing (exit 1). The body ends with the maintainer byline, which lint-body.mjs accepts (exit 0). Review surface (paginated): 0 reviews, 0 inline comments, 12 issue comments. The only bot comment is CodeRabbit's "Draft PR not reviewed", which has no findings to judge.

Head sha re-read at the end: c2d35f668d24780f3c9cf89ddfcff25b6fb4866b, unchanged.

Bbirdbrain-agent1d ago

Review verdict: approve for human review

Hold the merge until a release carries the verifier that reads these new fields; no released version does yet, and the description asks for the same wait.
On merge, GitHub will close only the scroll-recording issue; the older scroll-fallback issue this change also promises to close stays open and needs closing by hand.
A scrolling panel shorter than the window is never scrolled to its last screen, and while it is chosen no other panel is scrolled; this is not new, but the validation record should list it before the maintainer accepts it.
Every check, test and coverage figure the description states reproduces at this commit, and older evidence still verifies.

Full report

Reviewed commit: c2d35f668d24780f3c9cf89ddfcff25b6fb4866b

The commit was still the head at the end of the review, and auto-merge was null.

Verdict: approve for human review. No blocking or gate finding. The hold in finding 1 is a release-sequencing decision for the maintainer, not a code defect.

#	severity	file:line	finding
1	advisory (merge hold)	PR body, Summary line 1; plan Rulings ruling 30	The latest tag `v1.0.1-beta.23` (2026-10-12) does not contain step 1's verifier `327116b4` (#1978, 2026-10-27): `git merge-base --is-ancestor 327116b4 v1.0.1-beta.23` is false. A merge before the next tag puts a schema-6 writer on `main` ahead of any released reader. The body says not to merge before that tag.
2	advisory (acceptance criterion)	PR body line 1	`Closes #1928, #1667` links only #1928: GraphQL `closingIssuesReferences` returns `[1928]`. GitHub needs a keyword per issue. The #1928 criterion "the PR closes #1667" is therefore not met on merge. `merge.sh` only warns afterwards, and the composed squash body drops line 1. The `post-pr-body` linter rejects `Closes #1928, closes #1667` (line 1 rule), so the practical fix is to close #1667 by hand after merge.
3	advisory (pre-existing, missing from the validation record)	src/main/services/backgroundRenderer.ts:209-211; docs/specs/2026-10-26-capture-engine-first-slice-validation-assessment.md, Known limitations	The step uses `viewportHeight = max(root.clientHeight, innerHeight)` and `maxY = scrollHeight - viewportHeight`, so it stops an inner panel shorter than the window `innerHeight - clientHeight` px short of its end. That panel can still move down, so `pickRoot(true)` keeps choosing it, and no other pane is ever scrolled. The phase ends `stopped-growing` at the panel's height. The validation record and Evidence impact describe app-shell and two-pane scrolling from fixtures whose panes are exactly the window's height (`height:100%` of a `100vh` body), which masks this.
4	advisory (pre-existing, unclaimed)	src/main/services/backgroundRenderer.ts:215-220	`scrollResetScript` calls `pickRoot()` without `allowAtEnd`. After a phase that ends with every root at its end it resets only the document, so an inner panel is left at its bottom when the screenshot is taken. Simulated: after reset, doc 0 and main 9101. The behaviour is identical on `main`. The validation row "the page's scroll position and lock are restored" concerns a step that throws, where the render fails anyway.
5	advisory (scope, disclosed)	src/main/services/backgroundRenderer.ts:145-181, 192, 207, 289, 319-321	#1928's scope says "outcome maps to where the loop ends today". The PR also changes acquisition: the root choice, the stall rule (`<=` to `===`), the unlock decision, and a render that fails on a lock restore failure. These were needed for the `stopped-growing` e2e criterion. The Evidence impact, the plan Progress note and the validation record all disclose them. No action needed.
6	advisory	src/main/services/receivedEvidence.ts (not in diff)	A received Shared Case row leaves the `inventory` and `scroll_to_load` mirrors null while its signed entry has them. The body discloses this. The issue names ingest and Duplicate only.
7	advisory (test reliability)	backgroundRenderer.ts:338, :307	Two mutants survived. `png.length < 24` to `<= 24` is a near-equivalent boundary: a 24-byte buffer cannot hold a whole PNG header and IHDR, and the 23-byte test still fails. `executeJavaScript(..., true)` to `false` on the reset is equivalent under the mock, and no behaviour is claimed for it. Revert probe: the absent-fields ingest test and the report's no-height test pass with the source reverted, which is expected because both pin behaviour that is unchanged. No finding.

Failure scenario for 3: take an app shell with a 64 px top bar, a 900 px window, and <main> 836 px tall holding 3000 px plus a lazy sentinel at its foot, next to a <nav> that also overflows. I extracted the step script and ran it with node in a vm context, for both the branch source and origin/main, with identical output. Each step scrolls main to 2100 and stops, although its real max is 2164. nav.scrollTop stays 0 for all 8 steps. The stall loop then ends stopped-growing with endHeight 3000. The sentinel in the last 64 px never intersects, so its chunk never loads, and the signed record reads like a page scrolled to its end. The fix is in the PR's own area: use root.clientHeight for a non-document root. Failing that, list it under Known limitations and False negatives before the D16 acceptance decision.

Evidence gate

Touched paths on the blocking tier: backgroundRenderer.ts, recapture.ts, captureLifecycle.ts, db/captureRepo.ts (src/main/services/db/**) and reportHtml.ts. The PR carries evidence-affecting, and so do #1928 and #1667.
Advisory tier: src/shared/types.ts. The diff adds one entry to LOG_CODES (backgroundRenderer.scroll_reset_failed) and touches no capture, provenance or export type. It does not reach evidence behaviour.
The Evidence impact section is substantive and checked against the code. scrollLockLifted: true is set only when pickRoot(true) is null and the document overflows (backgroundRenderer.ts:192-193). endHeight comes from recordableHeight on the last step's state (:234-238, :275). A lock restore failure throws (:319-321). An ingest with neither field omits both and stays below schema 6 (tests/main/services/captureLifecycle.test.ts:286).
Known-answer tests: manifest.ts is unchanged, so no hash, signing or canonical-form method moved. The renderer tests pin each D5 ending and D6 case to exact values. Recapture and Duplicate entries are parsed with the strict ManifestEntrySchema.
Backward verification: the full suite passes, including the verify, chain and package tests. Entries without the fields are written as before.
Merge gate: merge-gate fails by design with "evidence-affecting with no sign-off at head". That needs the maintainer's approval at head. It is not a code failure.

Verify loop (tool shell bash, Node v20.20.2, at the pinned sha)

pnpm lint: exit 0, with 48 warnings, none in changed files.
pnpm typecheck: exit 0.
pnpm build: exit 0.
BIRDBRAIN_REQUIRE_OPENSSL=1 BIRDBRAIN_REQUIRE_JQ=1 pnpm test:coverage: exit 0. 490 files passed and 2 skipped; 8380 tests passed and 25 skipped (8405), matching the body.
pnpm coverage:diff: 40/40 changed lines (100%). src/shared/types.ts is not instrumented.
CI at the sha: green. merge-gate is the expected human gate, and the advisory probe reports 2 surviving mutants and 2 revert survivors, covered in row 7.

Completion controls

Attribution: the body ends with the Created with ... (T3 Code) byline. post-pr-body/scripts/check.sh accepts it in both default and --any-author modes.
Commit trailers: no Co-authored-by on any of the 63 commits in origin/main..HEAD.
Auto-merge: null.

Acceptance criteria (#1928): the unit tests for each D5 ending and each D6 case are present. The e2e stopped-growing fixture with entry and row passed in CI. e2e/recapture.spec.ts passes in CI. Closing #1667 is not met on merge (row 2).

Could not check

Vale on the changed docs: not installed in this runner, and CI does not run it. The body's preflight reports pass.
The e2e suite locally: I relied on CI e2e passing at this sha.
The validation record's Electron scroll-lock runs on real pages, for example a standards-mode overflow: hidden document moving from script. I did not reproduce them. The unit tests and the CI e2e fixtures cover the root choice on simulated and invented pages only.