Closes #1930
This is a draft that writes new facts into signed records, so it merges only after the release that carries the verifier for them; it is stacked on #1999 and merges after it. A background capture now also stores a PDF of the page, printed last, after the screenshot, the saved page, and the record of the page's network exchanges, so anything the page fetches only for printing stays out of that record. The capture's signed record holds the PDF's hash and says it was printed after recording stopped; if the print fails or runs out of time, the page or the file is over a size limit, or the page navigates again after it was saved, even to the same address, or while it prints, the capture is still stored and its record says the PDF is missing. The capture keeps the address and title of the page it saved, which closes a gap the network-record step opened for the address and this step had widened to the PDF, and the signed TLS details of the page follow the same saved page. Tests read the stored PDF with an independent reader, check it between two members of a shared case, and verify it in an exported evidence package with both shipped verifiers.
src/main/services/backgroundRenderer.ts (printPageToPdf, module-private, run after the Transaction Record's end phases with printToPDF({}) under PRINT_TIMEOUT_MS 5 s cut to the time left; end reserve 35 s to 40 s; RECORD_END_MARGIN_MS renamed END_MARGIN_MS, since the print uses it too; the PDF part parsed with the strict schema), src/shared/constants.ts (MAX_PDF_CONTENT_HEIGHT_PX 50,000 and MAX_PDF_SIZE 104,857,600).src/main/services/recapture.ts (PdfOutcome on RenderedPage, the inventory's PDF part, backgroundRenderer.pdf_failed logged once the Capture is stored), src/main/services/captureLifecycle.ts (signs pdfHash, writes {id}.pdf through step 3's writePdf; a Duplicate's row mirrors the pdfHash its entry re-anchors), src/main/services/db/captureRepo.ts (pdf_hash insert parameter).src/shared/types.ts and src/renderer/lib/notify.ts.pdfjs-dist 5.6.205, exact-pinned devDependency (package.json, pnpm-lock.yaml, with its optional @napi-rs/canvas and node-readable-to-web-readable-stream), a Dependabot ignore for >=5.7 beside the dependency-cruiser entry, and an audit-exceptions.json entry for GHSA-hq66-cqwq-w95j until 2026-12-31.backgroundRenderer.test.ts (order and options, a request made while printing, the bound and the cut, the 40 s reserve, both size bounds at and past their values, each outcome stored through the Recapture queue and ingest), recapture.test.ts, captureLifecycle.test.ts, captureDuplicate.test.ts, verifyRunbookExecution.test.ts (both shipped verifiers), sharing/syncPeers.test.ts and its helper (two peers), e2e/recapture-transaction-record.spec.ts (the probe's print-only image, pdf.js, an exported package), and e2e/recapture.spec.ts (step 6's case expected the PDF not-taken).CONTEXT.md (Capture), website/content/docs/features.mdx (Capture, Integrity, Export), the plan's Progress, and the validation record (step 10 in every section, with the print measurements behind the three values).manifest.ts gains an opt-in check on the unsigned entry, which ingest uses to parse the whole entry with the strict schema whenever the capture engine's fields are given (finding 6); the audit exception (finding 1); the print-style false negative, the uncancelled print, and the two rulings in the validation record and plan (findings 3 and 4); MiB, the glossary's commas, and the unused export and duplicate type (findings 7, 8 and 11). The maintainer's rulings of 2026-10-26 on the 40 s reserve and the test-only packages are recorded there.4e18f025, which signs Bound TLS Details only when the main document is the page the window last committed (did-navigate), and main at a6e392f2 with Electron 45. One did-navigate listener keeps both the navigation count the PDF check reads and that committed URL. Because this step stores the Capture as the page the save took, the committed URL is read at the save too and passed to the Transaction Record's end phases (d807ff8e), with unit cases for a page that navigates after the save and one that returns after the save from the blank page. The PDF check runs again when the print settles and discards the PDF on a navigation (1cf7e08b, the pre-pass reviews at 39ae663a), with unit cases for the three kinds. The features page, the log code comment and the validation record name a reload and the navigation refusal among the PDF's failed causes, and the record states the Electron 45 end-to-end run (23b11681).MAX_BLOB_BYTES (200 MiB, sharing/messages.ts), not by MAX_PDF_SIZE (finding 10); the receive path (receivedEvidence.ts) writes none of the capture engine mirror columns; and the text is read after the URL in its own call, so a navigation committed between the two leaves the later page's text beside the saved MHTML (validation record, Known limitations).Yes
background Capture gains {id}.pdf, pdfHash, and the PDF part of inventory ({ present: true, afterCollection: true }, or absent as failed), and its row mirrors pdf_hash; a Duplicate's row now mirrors the pdfHash its entry re-anchors. The Capture's url, finalUrl, title and httpStatus now come from the page at the save, where step 9 read them after its end phases, so a page that navigates after the save is stored as the page saved and gets no PDF; a navigation the window reports when the print settles discards the PDF too. Bound TLS Details are compared with the URL the window had committed at the save, where #1999 reads it after the text, so a page that navigates after the save keeps its details, and one that left for the blank page before the save and came back after records them absent. The end reserve grows to 40 s, so the load, the idle waits and Scroll-to-load share 80 s where step 9 left 85 s. An ingest whose capture engine entry fails the strict schema is refused before signing, where it was signed. The gate fired on the issue's evidence-affecting label and on blocking-tier paths (backgroundRenderer.ts, recapture.ts, captureLifecycle.ts, manifest.ts, db/captureRepo.ts).tampered), in Package Verification and verify.sh (step 5), and between two peers (hash-mismatch). It does not prove the PDF shows what the screenshot or MHTML shows: it is the page as Chromium prints it with its print styles, and afterCollection states only the order. failed does not say whether the print failed, ran out of time, met a size bound, or was refused after a navigation; the log entry's error does.pdfHash, and the not-taken entries of step 6 and step 9 builds, keep their bodies and chain hashes, and the full suite, frozen digests included, runs in the block below.pdfHash, an exported package failing its pdf check on a changed byte, and a page that navigates after the save stored under its first address with no PDF. With the print moved before collection stops, and with the save-time reads reverted, each case failed (local edits, reverted). The two committed-page unit cases failed with the URL read after the text, and the three during-print cases failed with the check after the print removed (local edits, reverted).pnpm preflight at 23b116813db6f5c87a7da3699c3dbaf9ce3fcd14 on Node v20.20.2; diff scored against origin/main (merge base a6e392f2f). 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) - 6 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) - 8523 passed, 25 skipped (8548), thresholds metpnpm coverage:diff - pass (exit 0) - 98.50% of 400 changed lines covered (floor 90%); 2 changed source files not instrumentedCreated with Claude Opus 5.5 in Claude Code (T3 Code)
Added 7 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
The rest of what you asked about held up: the print runs after recording stops on every path, a failed or slow print keeps the capture, and copies, exports and both verifiers agree on the PDF.
Reviewed commit: 3f5b2a477ef6c0a3cbe5c5ac5852573a70f4f50a (PR #2000, feat/1930-store-pdf), diffed against base origin/feat/1929-record-transaction-record at 50f9a3593e8f47284570d60a4e3c697bef509fc1. The PR head was unchanged at the end of the review. The base branch moved during the review to 11fcd9c2d2647dfd36f28f894757d60158cc0226 (#1999's fix round: parseMirroredHash on the captures read path in db/captureRepo.ts, plus stack merges). git merge-tree --write-tree HEAD 11fcd9c2 merges cleanly. Nothing here was re-run against the new base.
Live CI at the head commit: Dependency audit failure; merge-gate failure, as expected for an evidence-affecting PR with no maintainer sign-off at head; e2e, test, build, lint, typecheck, docs-build, and Test reliability (probe) all success. agent/pre-pass is pending.
| # | severity | file:line | finding |
|---|---|---|---|
| 1 | blocking | package.json:223, audit-exceptions.json | pdfjs-dist 5.6.205 carries GHSA-hq66-cqwq-w95j (high, range >= 5.6.83, < 6.2.108), and the required Dependency audit check fails on it. The patched 6.2.108, like 5.7.284, declares engines.node >=22.13.0 || >=24, so no upgrade fixes it on Node 20. Reproduced locally: node scripts/audit-check.mjs exits 1 with BLOCKING (no accepted exception): [.] high pdfjs-dist GHSA-hq66-cqwq-w95j. |
| 2 | blocking | src/main/services/backgroundRenderer.ts:773, :802-815; src/main/services/recapture.ts:256, :269 | The PDF is printed from whatever document the window holds after the end phases, and finalUrl/title are read after the print. A page that navigates after savePage is stored under the new URL, with a PDF of the new document and an MHTML and screenshot of the old one. The entry carries pdf: { present: true, afterCollection: true } and nothing that states a navigation happened. |
| 3 | advisory | src/main/services/backgroundRenderer.ts:526-545; src/shared/constants.ts:23-29; validation record :150, :227-264, :321 | MAX_PDF_CONTENT_HEIGHT_PX compares the screen layout's cssContentSize.height (:398, :437), and print styles can lay out far more. Measured: a page 479 CSS px tall on screen printed to 108,106,623 bytes in 6,091 ms. The size check runs only after the whole Buffer is in the main process. The 5 s race stops waiting but does not cancel printToPDF, which keeps running until rendered.cleanup() destroys the window after ingest (recapture.ts:348). The validation record's false-negative list names only inner scrolling panels. |
| 4 | advisory | validation record :217-219; plan Progress, step 10 entry | The 40 s reserve is still written up as a "Deviation" from the step 9 ruling, and the dev-only package acceptance is not recorded. Both were ruled on 2026-10-26, and only the run log records them. The validation record is the artifact the maintainer accepts (D16). |
| 5 | advisory | src/main/services/backgroundRenderer.ts:601-606, :689-691 | The deadline timer starts before render(), but remainingMs counts from startedAt, which comes after await session.setProxy(...) on a proxy Egress. The 1 s END_MARGIN_MS absorbs that gap unless setProxy takes longer than 1 s. If it does, a print run to its bound settles after the outer deadline, and the whole Capture is lost (ruling E). This predates the PR for step 9's phases, and the print now joins them. Not reproduced. |
| 6 | advisory | src/main/services/manifest.ts:1040-1083; src/main/services/captureLifecycle.ts:135-138 | No strict parse of the whole entry runs before it is signed. Only the PDF part is parsed (backgroundRenderer.ts:545), and the pairing of pdfHash and inventory.pdf.present holds by construction (recapture.ts:280-287). The new IngestParams comment, "the schema refuses a hash the inventory does not mark present", describes the verifier reading the entry back. Ingest itself would sign a mismatched pair, which would then break the Case's chain. The wording is the same as step 9's comment. |
| 7 | advisory | website/content/docs/features.mdx:60 | The page says "larger than 100 MB", but MAX_PDF_SIZE is 104,857,600 bytes (100 MiB), while line 38 of the same page uses decimal MB for 10,000,000 bytes. A 102,000,000-byte PDF is larger than 100 MB and is still stored. |
| 8 | advisory | CONTEXT.md:54 | "a Transaction Record unless recording it failed and a PDF of the page, printed after everything else, unless ..." parses as one compound condition. Commas around the first unless clause fix it. |
| 9 | advisory | PR body, Changes, "For the maintainer" | This bullet still puts the 40 s reserve to the maintainer as an open question, which the ruling has since settled. |
| 10 | advisory | src/main/services/sharing/messages.ts:22 | MAX_PDF_SIZE applies only on the print path. A PDF received from a Shared Case peer is bounded by MAX_BLOB_BYTES (200 MiB), the screenshot cap's pattern. Noted against the bounded-work rule, not confirmed as a problem. |
| 11 | advisory | src/main/services/backgroundRenderer.ts:504, :520; src/main/services/recapture.ts:21 | printPageToPdf is exported with no caller outside its module, and PresentPdf is declared twice. |
Failure scenario for 2. An interstitial at https://a.example/ runs setTimeout(() => location.href = 'https://b.example/', 5000). The render finishes its idle waits and saves the MHTML at about 4 s. The page navigates during finishTransactionRecord or the print. printToPDF prints B, wc.getURL() returns B, and recapture.ts:256 stores the Capture with url B. redirectedFinalUrl then records B as the redirect target, while the MHTML and screenshot hold A. A hostile page can widen the window on purpose by keeping a body fetch open into the 5 s collect phase.
A standalone Electron 44.9.1 run (offscreen, sandboxed window on a fresh partition) showed the Chromium behaviour. The script loaded /nav-a, saved the MHTML at 200 ms, and the page navigated at 1,500 ms. At print time getURL() was /nav-b, the MHTML holds "Page A the saved page", and pdf.js reads the PDF as "Page B navigated after the save".
The URL half was opened by step 9, which moved getURL() after finishTransactionRecord; on main it is read right after the save. This PR widens the window by up to 5 s and adds the PDF half. One fix: read finalUrl and title at the save, and record the PDF failed when wc.getURL() differs at print time. The Evidence impact section, features.mdx:56 ("a PDF of the page") and the validation record do not cover this case.
Failure scenario for 3. The page uses #p { height: 0; overflow: hidden } with @media print { #p { height: auto; overflow: visible } } around 150 random-pixel 600x400 images. The screen content height is 479 CSS px, so the layout bound passes. printToPDF({}) returned 108,106,623 bytes in 6,091 ms in the standalone run. In the render, the 5 s race records failed ("did not settle"), but Chromium keeps printing, and its Buffer reaches the main process if the print finishes before ingest ends.
The PDF is never signed or stored, so the evidence stays truthful: inventory failed, no pdfHash, no file. What the validation record misses is a false negative of the layout bound. Its line 150, "a page as dense as the fixture is not printed only to be discarded", holds only for content laid out the same on screen and in print.
Answers to the six checks
activeRecorder.stop() runs unconditionally at backgroundRenderer.ts:769, before the screenshot (:771), savePage (:773) and the text read (:775). finishTransactionRecord never throws (transactionRecorder.ts:478-525), and the print follows at :802. After stop(), onMessage drops every Network.* event (transactionRecorder.ts:368) and resumes, without recording, any target that attaches (:361-363). An early stop on overflow (:368, collect throwing at :396-398) and a failed or absent record (:795-799) both leave the print after the stop. The unit test backgroundRenderer.test.ts:1461 would catch a reorder, and the e2e at e2e/recapture-transaction-record.spec.ts:673-680 holds the order in a real print.Math.min(PRINT_TIMEOUT_MS, remainingMs(END_MARGIN_MS)) (:805). When boundMs <= 0 the print never starts (:534), and the reserve is 20+5+10+5 = 40 s (:61-62). Timeout, throw and size failures all return failed, and ingest stores the Capture (recapture.ts:280-287). Tests: backgroundRenderer.test.ts:1482, :1491, :1364, :1647. The one exception is finding 5.:526-531) and size after (:540-544). Either way the entry records pdf: { present: false, reason: 'failed' }, with no pdfHash, no .pdf file, a null pdf_hash on the row, and one backgroundRenderer.pdf_failed log naming the bound (recapture.ts:308-315), as D4 defines failed. The validation record states this truthfully. Its gap is finding 3.ManifestEntrySchema. The Duplicate re-checks the copied PDF's hash (captureLifecycle.ts:1656-1659), re-anchors pdfHash (:1714), and its row mirrors pdfHash (:1761) and inventory (:1757). Export ships the PDF only when the signed entry anchors it (export.ts:1591-1599). Both verifiers' PASS and FAIL cases (verifyRunbookExecution.test.ts, "a PDF the ingest stored") ran, not skipped, with jq and openssl required."pdfjs-dist": "5.6.205" is in devDependencies (package.json:223), and Dependabot ignores >=5.7 (.github/dependabot.yml:87-92); npm view confirms 5.7.284's engines. pnpm why places @napi-rs/canvas and node-readable-to-web-readable-stream only under pdfjs-dist in devDependencies. pnpm list --prod names none of the three, and out/ holds no reference to them. An electron-builder --linux dir build packed 15,586 app.asar entries with no match for any of them (better-sqlite3: 76 entries, as a control).RECORD_END_MARGIN_MS reference remains. The reserve test moved from 34/37.5 s to 39/42.5 s, with its reason stated (backgroundRenderer.test.ts:1361-1369), and e2e/recapture.spec.ts:306-313 now expects the printed part and the mirrored hash. recapture.test.ts:221 also changed: the fixture's default render now prints, and its not.toHaveProperty('pdfHash') assertion was dropped. The new positive test replaces it. All of these changes are deliberate.Evidence gate. Blocking-tier hits: recapture.ts, backgroundRenderer.ts, captureLifecycle.ts, db/captureRepo.ts. The PR and #1930 both carry evidence-affecting. Advisory-tier hits:
src/shared/types.ts, the LOG_CODES list: no evidence behaviour.src/shared/constants.ts, the two PDF bounds: these decide whether a PDF is stored, so they do reach evidence behaviour, and the PR is already labelled.package.json, devDependencies: no shipped change.pnpm-lock.yaml, dev-only importers: the production tree is unchanged.The Evidence impact section is substantive but misses finding 2. The known-answer coverage grows through the ingest, both verifiers and the e2e. No verifier, schema or layout code changed, and the full suite passed.
Completion controls. auto_merge is null. The PR body's last line is the maintainer byline, and post-pr-body/scripts/check.sh exits 0 on it. None of the 7 commits carries a Co-authored-by trailer.
Commands and exit codes (scratch worktree at the head commit, Node 20.20.2, Linux, run in bash)
| 1 | gh pr checks 2000 1 (Dependency audit, merge-gate fail) |
| 2 | node scripts/audit-check.mjs 1 |
| 3 | pnpm lint 0 (0 errors, 47 warnings) |
| 4 | pnpm typecheck 0 |
| 5 | pnpm build 0 |
| 6 | env -u PKG_CONFIG_PATH pnpm build:test-sharp 0 |
| 7 | BIRDBRAIN_REQUIRE_OPENSSL=1 BIRDBRAIN_REQUIRE_JQ=1 BIRDBRAIN_TEST_SYSTEM_SHARP=1 pnpm test:coverage |
| 8 | 0 (482 files, 8170 passed, 24 skipped) |
| 9 | node scripts/diff-coverage.mjs --base origin/feat/1929-record-transaction-record |
| 10 | 0 (27/27 changed lines, 100.00%) |
| 11 | pnpm test tests/main/services/verifyRunbookExecution.test.ts tests/main/services/backgroundRenderer.test.ts |
| 12 | 0 (129 passed) |
| 13 | xvfb-run -a npx playwright test e2e/recapture-transaction-record.spec.ts e2e/recapture.spec.ts |
| 14 | 0 (12 passed) |
| 15 | .claude/hooks/vale-prose.sh --base origin/feat/1929-record-transaction-record 0 |
| 16 | .claude/skills/post-pr-body/scripts/check.sh <PR body> 0 |
| 17 | pnpm build:extension && pnpm build:notices && electron-builder --linux dir 0 |
| 18 | xvfb-run -a electron <standalone print probe> 0 |
Not checked. I did not reproduce the PR's claim that moving the print before collection stops made the e2e fail. The unit test at :1461 covers that order. I did not check Windows or macOS packaging, or whether destroying the window cancels an in-flight printToPDF (finding 3 infers it). I re-ran nothing against the moved base.
Added 6 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.
3dd81428 (finding 1), 32add6e8 and 39ae663a (findings 2 and 11), 39ae663a (findings 3, 4, 7 and 8), ee255c5c (finding 5), 7797b7fc (finding 6). The description was regenerated at 39ae663a for finding 9.
Review verdict: approve for human review
Reviewed commit: 39ae663ad60bcb1226e40c8a70ddc1de74793409 (PR #2000, feat/1930-store-pdf), delta from 3f5b2a477ef6c0a3cbe5c5ac5852573a70f4f50a. Base origin/feat/1929-record-transaction-record at 11fcd9c2d2647dfd36f28f894757d60158cc0226. Head re-read at the end: unchanged.
Delta. 13da4b1d is a clean merge of 3f5b2a47 and 11fcd9c2: git merge-tree --write-tree 3f5b2a47 11fcd9c2 gives a tree identical to 13da4b1d (git diff --stat empty), so it brings in only #1999's fix round. The reviewed delta is 3dd81428, 32add6e8, ee255c5c, 7797b7fc, 39ae663a. Trial merge against the base: the base is an ancestor of head, so the merge is head itself (clean). For information only: a trial merge into origin/main conflicts in CONTEXT.md, the plan, the charset spike and pnpm-lock.yaml, which is the stack's to resolve when #1999 lands.
Earlier findings, each verified by mutation (unit runs under BIRDBRAIN_TEST_SYSTEM_SHARP=1, Node 20.20.2, run in bash)
| # | fix | mutant | result |
|---|---|---|---|
| 1 | audit-exceptions.json entry for GHSA-hq66-cqwq-w95j | entry removed | audit-check.mjs exit 1, BLOCKING (no accepted exception): [.] high pdfjs-dist GHSA-hq66-cqwq-w95j; restored: exit 0 |
| 2 | backgroundRenderer.ts:542-545 navigation check, :799-804 save-time reads, :846-848 | check replaced by if (false) | 4 unit tests fail (other page, reload, in place, through ingest) |
| 2 | same | URL compare only, navigation count dropped | the reload case fails |
| 2 | same | title/finalUrl/httpStatus read live again | 3 unit tests fail |
| 2 | same | check and save-time reads both reverted, app rebuilt | e2e "navigates after the save" fails: entry.url is /nav-b, expected /nav-a |
| 5 | backgroundRenderer.ts:620-624 startedAt before the deadline timer | moved back after setProxy | "cuts the print to the deadline when the proxy setup was slow" fails |
| 6 | captureLifecycle.ts:325-340, :519; manifest.ts:987, :1070 | check option dropped | "refuses, and writes nothing, an entry the strict schema refuses" fails |
| 6 | same | opts.check moved after the write | same test fails |
Findings 3, 4, 7, 8, 9 and 11 are applied as the dispositions say (validation record Known limitations and Rulings paragraph, plan Progress 2026-10-26, features.mdx:60 MiB, CONTEXT.md:54 commas, body, PresentPdf exported once from recapture.ts:21 and printPageToPdf module-private at backgroundRenderer.ts:530). Finding 10 is left as a named follow-up in the body, which is acceptable for an advisory.
Findings this round
| # | severity | file:line | finding |
|---|---|---|---|
| 1 | advisory | src/main/services/backgroundRenderer.ts:542-558 | The navigation check runs once before printToPDF and not after it. A main-frame commit the renderer handles before the print request, but whose did-navigate has not yet reached the main process, passes the check and is printed. Not reproduced; inferred from the ordering. CONTEXT.md:54 ("would have shown a page the window navigated to after the save") and features.mdx:60 assume the check is complete. Fix: after printToPDF resolves, compare wc.getURL() and navigations with saved again and return failed on a change. |
| 2 | advisory | website/content/docs/features.mdx:60-61; PR body Summary | "or the page moves to another address after it is saved" leaves out the reload case, which the code also refuses (backgroundRenderer.ts:543, the reload unit case). Corrected wording: "or the page navigates again after it is saved, even to the same address". The body's "or the page has moved to another address since it was saved" needs the same change. Nothing it states is false. |
| 3 | advisory (informational) | validation record :68-74 | Step 10's print sizes and times (90,084,033 bytes; 108,106,623 bytes in 6,091 ms and 2,365 ms), the 612 by 792 Letter page and the print-only image behaviour were measured on Electron 44.9.1 with Chromium 152.0.7977.167, and the record says so at :68. Main moved to Electron 45 (#1994); the base still pins ^44.9.1. The e2e pins (one Letter page, print-only image absent from the record, the navigation case) re-run on Electron 45 once the stack is rebased. The measurements behind MAX_PDF_SIZE and MAX_PDF_CONTENT_HEIGHT_PX will not, and stay labelled 44.9.1. |
Advisory exception (3dd81428). GHSA-hq66-cqwq-w95j (CVE-2026-16633, high, pdfjs-dist >= 5.6.83, < 6.2.108, first patched 6.2.108) is about script execution when a host loads a malicious PDF with enableScripting on. npm view pdfjs-dist@6.2.108 engines gives >=22.13.0 || >=24; .nvmrc is 20 on the branch and on main. The only import is e2e/recapture-transaction-record.spec.ts:726 (getDocument, getPage, getTextContent) on PDFs the app printed from fixture pages. pnpm why --prod pdfjs-dist is empty, and pnpm why lists it under devDependencies only. The acceptance is recorded in audit-exceptions.json with expires: 2026-12-31, in the validation record's Rulings paragraph and in the plan. #1992 (npm group) and #1993 (vite) do not touch pdfjs-dist, which is not on main, or audit-exceptions.json, or the Node pin (git diff base..main on those files), so they neither resolve nor change it. audit-check.mjs also reports a stale exception for GHSA-vfj7-8cjw-p6xm, which predates this PR.
Evidence gate. Touched blocking-tier paths: backgroundRenderer.ts, recapture.ts, captureLifecycle.ts, manifest.ts (new this round), db/captureRepo.ts. The PR and #1930 both carry evidence-affecting. Advisory-tier hits: src/shared/types.ts (log codes, no evidence behaviour), src/shared/constants.ts (the PDF bounds, which do decide storage; already labelled), package.json and pnpm-lock.yaml (devDependencies only). The Evidence impact section is substantive, names manifest.ts, and covers the save-time reads and the strict pre-sign parse. Known-answer coverage: the ingest, both shipped verifiers and the e2e exercise the stored PDF; the strict parse changes no entry bytes. Backward verification: no verifier or schema file changed; manifest.test.ts and verifyRunbookExecution.test.ts pass with jq and openssl required. Auto-merge is null.
CI at head. All pass except merge-gate (fail, no maintainer sign-off at head, expected) and agent/pre-pass (pending, this review). Dependency audit now passes. The test job reports diff coverage vs the base at 44/44 lines, 100.00%. Test reliability (probe): 0 mutants survived, 25 caught, 9 not run for time; two tests pass with the source reverted ("ingest writes no PDF file and signs no hash when given no PDF", "notice ... not logged for a printed PDF, or when the ingest fails"), both negative guards that hold on the base by construction, not findings.
Completion controls. auto_merge null. PR body passes post-pr-body/scripts/check.sh (exit 0) and ends with the maintainer byline the repo check accepts. No Co-authored-by in any of the 12 non-merge commits (git log origin/feat/1929-record-transaction-record..HEAD).
Commands and exit codes (scratch worktree at head, Node 20.20.2 via mise, Linux, bash)
| 1 | gh pr checks 2000 merge-gate fail, pre-pass pending, rest pass |
| 2 | node scripts/audit-check.mjs 0 (1 with the new exception removed) |
| 3 | env -u PKG_CONFIG_PATH pnpm build:test-sharp 0 |
| 4 | pnpm test <9 files: backgroundRenderer, captureDuplicate, captureLifecycle, recapture, |
| 5 | sharing/syncPeers, verifyRunbookExecution, captureEngineColumnsMigration, |
| 6 | reportCaptureEngine, manifest> (REQUIRE_OPENSSL/JQ=1) 0 (500 passed) |
| 7 | pnpm build 0 |
| 8 | xvfb-run -a npx playwright test e2e/recapture-transaction-record.spec.ts e2e/recapture.spec.ts |
| 9 | 0 (13 passed) |
| 10 | mutants above each killed (exit 1) |
| 11 | post-pr-body/scripts/check.sh <body> 0 |
Not checked. I did not run lint, typecheck or the full coverage suite locally; CI's lint, typecheck, build and test jobs are green at this commit. I did not reproduce finding 1's race, run anything on Electron 45, or check Windows or macOS.
Review verdict: approve for human review
Reviewed commit: 39ae663ad60bcb1226e40c8a70ddc1de74793409
Review of PR #2000 (feat/1930-store-pdf), diffed against its base origin/feat/1929-record-transaction-record at 11fcd9c2 (the merge base, so the base is an ancestor of head). I re-read the head at the end of the review and it had not moved. The base and auto_merge (null) had not changed either. I ran in a scratch worktree at /tmp/wt-2000, which I left clean.
Live checks at head (gh pr checks 2000): every required check passes. merge-gate fails with blocked: evidence-affecting with no sign-off at head, which is expected until the maintainer signs off (ADR-0041). probe passes and is advisory. agent/pre-pass already holds an approve verdict at this commit.
Verify loop (bash, Node v20.20.2, Ubuntu runner, no system-sharp workaround needed):
| step | result |
|---|---|
pnpm lint | exit 0 |
pnpm typecheck | exit 0 |
pnpm build | exit 0 |
BIRDBRAIN_REQUIRE_OPENSSL=1 BIRDBRAIN_REQUIRE_JQ=1 pnpm test:coverage | exit 0; 482 files passed, 2 skipped; 8177 tests passed, 24 skipped (8201), the same as the PR body |
pnpm coverage:diff (vs origin/main) | exit 0; 624/630 changed lines (99.05%). The PR's own files are at 100%: backgroundRenderer.ts 84/84, captureLifecycle.ts 31/31, recapture.ts 11/11, manifest.ts 6/6, db/captureRepo.ts 9/9. The 6 uncovered lines are in warcReader.ts, which belongs to #1999 |
node scripts/audit-check.mjs | exit 0; GHSA-hq66-cqwq-w95j accepted until 2026-12-31 |
verifyRunbookExecution.test.ts -t 'step 10' | the 2 PDF cases ran (not skipped by RUNS) and passed |
Findings
| # | severity | file:line | finding |
|---|---|---|---|
| 1 | advisory | src/main/services/backgroundRenderer.ts:546-551 (layout bound), :558-563 (race, then size check); src/shared/constants.ts:23-29 | Bounded work. MAX_PDF_SIZE is tested against a Buffer that printToPDF has already built in full. The pre-print bound reads contentHeight from the screen layout, captured at screenshot time, up to about 15 s and both Transaction Record end phases before the print. Print-only styles bypass it: the record measured a 479 px page printing 108,106,623 bytes. A print that outlasts its 5 s bound is not cancelled, so its Buffer, of any size, can still reach the main process after the render stops waiting. Issue #1930 specifies this design, Electron's printToPDF has no streaming or cancel option, and the validation record's Known limitations and False positives sections describe each part. I found no defect beyond what is disclosed. |
| 2 | advisory | src/shared/types.ts:821-823; validation record, False positives and negatives, Step 10, first bullet | Two lists of what failed covers predate the fix round. They name the print failing, running out of time, and the size bounds, but not the refusal after a navigation (backgroundRenderer.ts:542-545). The record's bullet also says the log error "names the bound", but for this case it names the later address. The record's Step 10 summary and the PR body both include the navigation case. |
| 3 | advisory | website/content/docs/features.mdx:59-61 | "the page moves to another address after it is saved" misses a reload at the same address. The count check at backgroundRenderer.ts:543 also skips the PDF for a reload, and CONTEXT.md ("a page the window navigated to") covers it. The previous pre-pass raised this too. |
| 4 | advisory (not reproduced) | backgroundRenderer.ts:540-545, :558 | The navigation check runs once, synchronously, before printToPDF. A navigation that commits during the print is not re-checked. I reasoned that a commit in the same renderer process is blocked while it prints, and that a cross-process commit leaves the old document printing, but I did not verify either. A second check after the print would close the gap. |
There are no blocking or gate findings.
Correctness traces
activeRecorder.stop() (:792), then screenshot (:794), then savePage (:796), then the save-time snapshot saved (:799-804, with no await between it and the save), then text, then finishTransactionRecord (:812-830), then printPageToPdf (:833-838), then the inner finally { cleanup() }. The consent blocker and the Egress proxy are still on the session during the print.startedAt now precedes the deadline timer (:620-624). The print bound is min(5000, remaining - 1000) (:837), and boundMs <= 0 refuses to start. FINAL_ARTIFACT_RESERVE_MS = 20 + 5 + 10 + 5 = 40 s (:62-63). With DEFAULT_TIMEOUT_MS = 120,000 (recapture.ts:86), the earlier phases share 80 s, as the body states.Promise.race attaches a handler to the losing printToPDF promise, so a late rejection is not unhandled, and .finally clears the timer.pdfHash is the SHA-256 of the same Buffer writePdf stores (captureLifecycle.ts:376, :474-476). The row mirrors it (:514; captureRepo.ts:254, read back through parseMirroredHash at :534). On failure, deleteArtifacts iterates CAPTURE_ARTIFACT_TYPES, which includes pdf (captureStore.ts:32-39, :453-460).manifest.ts:1070) runs before signEntryHash and before the manifest file is opened, so a refusal writes nothing to the manifest, and the outer catch removes the MHTML. The schema is ManifestEntrySchema, the same one the app's manifest reader uses (manifest.ts:563). Its refine (schemas.ts:727-730) enforces D2, that pdfHash is present exactly when inventory.pdf.present.copyArtifacts copies every artifact type (captureStore.ts:434-443), a PDF whose hash differs from the signed one is refused (captureLifecycle.ts:1678), and the entry and row re-anchor sourceEntry.pdfHash (:1736, :1783).pdfHash (export.ts:1591-1599), which matches the features page's Export bullet.Targeted mutation (mine, in the scratch worktree, then reverted): I set the Duplicate row's pdfHash (captureLifecycle.ts:1783) to undefined, and 2 tests in captureDuplicate.test.ts failed. The previous pre-pass already mutation-checked the navigation check, the save-time reads, the start of the time count, and the strict check.
Test reliability probe at head: 25 mutants caught, 0 survived, 9 not run because time ran out. Two tests pass with the source reverted: captureLifecycle.test.ts:410 (no PDF given, so no file and no hash) and recapture.test.ts:414 (no notice for a printed PDF or a failed ingest). Both are negative-case guards that the base, which has no PDF code, trivially satisfies. The behaviours they guard cannot regress through a revert, so neither is a finding.
Evidence gate
backgroundRenderer.ts, recapture.ts, captureLifecycle.ts, manifest.ts, db/**. The PR's labels include evidence-affecting and so do issue #1930's, so the backstop holds.package.json: devDependencies pin only. Not evidence-reaching: no pdfjs or @napi-rs/canvas string in out/.pnpm-lock.yaml: adds pdfjs-dist, @napi-rs/canvas and node-readable-to-web-readable-stream, all dev-only. Not evidence-reaching.src/shared/constants.ts: adds MAX_PDF_SIZE and MAX_PDF_CONTENT_HEIGHT_PX. These do reach evidence, because they decide whether a signed inventory marks the PDF present. They are covered by the blocking-tier review above.src/shared/types.ts: LOG_CODES only. Not evidence-reaching.pdfHash, the inventory part, the stored URL, title and status now read at the save, the 40 s reserve, and refusal before signing) and the limits of what verification proves (print styles, the meaning of afterCollection, and failed not naming its cause).verifyRunbookExecution.test.ts:1065-1137 runs an ingest-stored PDF through both shipped verifiers: it passes, and a changed byte fails at step 5. The e2e case confirms against the real print that the print-only image is absent from the record, that pdf.js reads one 612 by 792 page, that the hash matches, and that an exported package fails on a changed byte.src/shared/verify/, schemas.ts or evidence-package-layout/ changes, and the full suite, frozen digests included, passes.Docs claims checked against code
MAX_PDF_SIZE equals MAX_SCREENSHOT_SIZE at 104,857,600 (constants.ts:9, :28).MAX_SCROLL_CONTENT_HEIGHT_PX = 32,000,000 / 1280 = 25,000 (backgroundRenderer.ts:34, :71).printToPDF({}) defaults, and the e2e asserts 612 by 792.> comparison.npm view gives pdfjs-dist@5.7.284 engines >=22.13.0 || >=24, and 5.6.205 allows >=20.19.0. pdf.js is imported only at e2e/recapture-transaction-record.spec.ts:726.Scope: the diff maps onto #1930's six acceptance criteria. The fix-round additions (save-time reads, the time-count start, the strict pre-sign check) fix defects the previous review attributed to this step, or that this step widened. The PR body records them, so I do not count them as drive-by changes.
Completion controls
git log origin/feat/1929-record-transaction-record..HEAD --format=%B | grep -i co-authored-by finds nothing (exit 1) across the 13 PR commits.null.Created with Claude Opus 5.5 in Claude Code (T3 Code) byline, which lint-body.mjs:13-15 accepts. check.sh --any-author exits 0.Not checked
e2e check at head is green and I treat it as authoritative..claude/hooks/vale-prose.sh did not run. CI's docs-build and lint are green.pnpm build:extension is not applicable, because extension/ is unchanged.Added 4 commits
Applied.
1cf7e08b checks the page again when the print settles (both verdicts at 39ae663a). 23b11681 names a reload and the navigation refusal on the features page, in the log code comment and in the validation record, and records the Electron 45 end-to-end run; the print measurements stay labelled Electron 44.9.1. The description carries the reload wording at 23b11681.