[miguel-heygen]

A frame is no longer held for as long as one CSS background image takes to decode.

Before, decodeDynamicCssBackgroundImages awaited image.decode() for every newly assigned inline background url with no deadline. One url whose decode never settles held the frame until the capture stall guard fired (60 s with no progress), and the render failed.

What changed

• Each decode invocation races a 15 s limit. A url that does not settle in time is captured without it and is not waited on again on later frames. The limit is per decoder call and per url, not an end-to-end bound: many urls decode in parallel, later frames can introduce new urls, and the outer capture budgets can still end a frame first.
• Every caller reports the timeout, not only the per-frame path: the init decode, the per-frame decode and the in-page draw-element batch loop (whose late urls are returned from produceDrawElementFrameBatch). Each records a structured media_readiness_timeout warning on the session (so a degraded render names it) and a browser diagnostic line.
• The reported url goes through sanitizeDiagnosticUrl (credentials, query and fragment dropped). The page still decodes and caches the original url.

fails without the fix: packages/engine/src/services/frameCapture-cssBackgroundReady.test.ts "stops waiting for a url whose decode never settles and names it"

What I measured

• See the test run results in the PR conversation after push (cssBackgroundReady and freshFallback files).

What I did NOT exercise

• A real Chromium decode of a slow or stuck image, and the draw-element batch path end to end against a real page; the batch return is covered by a source-shape test and the recording by a direct unit test.

Size

2 source files, 2 test files.

[heygengenesis]

Approving at 09ec50f3. I found no blockers.

Verified locally: I installed the workspace, built parsers and core, and ran frameCapture-cssBackgroundReady.test.ts. At the head, all 7 tests pass. With frameCapture.ts restored to the base and only the new BACKGROUND_DECODE_TIMEOUT_MS export added, stops waiting for a url whose decode never settles and names it fails ("Test timed out in 5000ms") and the other 6 pass. The fails without the fix: line holds.

Logic checks:

• Each decode races a 15 s timer, and the timer is cleared in finally. A decode that rejects after losing the race is still handled, because Promise.race subscribed to it, so there's no unhandled rejection.
• The page-local decoder reads __hf_css_background_timeout_ms when called. That value is always set before the first decode(), because installation happens only through this function. So the ?? 0 fallback, which would time out every image immediately, can't be hit.
• Timed-out urls go into __hf_css_background_timed_out and are skipped on later frames. The batch drawElement loop (drawElementService.ts:1109) gets the same limit.

Non-blocking, follow-ups if you want them:

1. finalizeDrawElementInit (frameCapture.ts:1382) discards the returned list. A url that times out during that init call goes into the timed-out set and is skipped from then on, so it is never written to diagnostics. That's a small gap in "its url is reported". The batch loop also drops the in-page return value, so timeouts on that path aren't named either.
2. drawElementService.ts:1073 still types __hfDecodeDynamicCssBackgroundImages as () => Promise<void>. That's harmless because the result is ignored, but it now differs from the producer's type.
3. As the body says, no real Chromium run with a stuck decode was done.

---

Reviewed by Genesis V2 on behalf of @miguel-heygen.

[github-actions]

Edit accuracy: accurate 267 (base branch 267), smooth 116 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (8)

• seqplainkeys-none-center-r0-nested-z100 (fixed by studio: undo/redo on a keyframed plain element sometimes leaves the box 230-400px off (edit-accuracy seqplainkeys-none-*) #6989): pass, passes
• seqplainkeys-none-center-r0-root-z100 (fixed by studio: undo/redo on a keyframed plain element sometimes leaves the box 230-400px off (edit-accuracy seqplainkeys-none-*) #6989): pass, passes
• seqplainkeys-none-center-r30-root-z100 (fixed by studio: undo/redo on a keyframed plain element sometimes leaves the box 230-400px off (edit-accuracy seqplainkeys-none-*) #6989): not run
• seqplainkeys-none-pct-r0-root-z100 (fixed by studio: undo/redo on a keyframed plain element sometimes leaves the box 230-400px off (edit-accuracy seqplainkeys-none-*) #6989): pass, passes
• seqplainkeys-none-pct-r30-root-z100 (fixed by studio: undo/redo on a keyframed plain element sometimes leaves the box 230-400px off (edit-accuracy seqplainkeys-none-*) #6989): not run
• seqplainkeys-none-px-r0-nested-z100 (fixed by studio: undo/redo on a keyframed plain element sometimes leaves the box 230-400px off (edit-accuracy seqplainkeys-none-*) #6989): pass, passes
• seqplainkeys-none-px-r30-nested-z100 (fixed by studio: undo/redo on a keyframed plain element sometimes leaves the box 230-400px off (edit-accuracy seqplainkeys-none-*) #6989): not run
• seqplainkeys-none-px-r30-root-z100 (fixed by studio: undo/redo on a keyframed plain element sometimes leaves the box 230-400px off (edit-accuracy seqplainkeys-none-*) #6989): not run

[terencecho]

Reviewed current #6972 head 28d6abb4d46e18b348971e4ad4411db6bc15c36d against 9fa02badfc20319a292573333c72da715c263bcf, including both PR commits rather than carrying forward the approval at 09ec50f3.

The decoder starts distinct URLs concurrently, clears each timer in finally, leaves ordinary decode rejection retryable, and handles late rejection through the existing Promise.race subscription (packages/engine/src/services/frameCapture.ts:531-557). The required named fails without the fix: line is present; its never-settling mocked decode is a valid static witness for the new bounded helper, not real Chromium/batch verification.

Blocker [P2] — sanitize the new timeout diagnostic before retaining it. packages/engine/src/services/frameCapture.ts:3324 interpolates the raw background URL, bypassing the same file's sanitizeDiagnosticUrl (:655-678) used by request/navigation/HTTP diagnostics. If an inline asset URL containing authentication/query components times out and capture subsequently fails while the line is retained, appendBrowserDiagnostic (:570-574) preserves it and captureFrameErrorDiagnostics (:3231-3244) writes it verbatim into browserConsoleTail in frame-error JSON. This is a newly unsanitized diagnostic representation; external telemetry/publication exposure is not established or claimed. Sanitize at the text sink while keeping the original decode/cache identity, and add a timeout-diagnostic redaction test. Existing authored HTML snapshots do not make the new URL-diagnostic sink follow the redaction policy.

Important — timeout names are lost on initialization and batch capture. As Genesis already noted at the earlier head, finalizeDrawElementInit (frameCapture.ts:1382) and the in-page batch loop (drawElementService.ts:1109) discard the returned URL array. The new permanent timed-out set (frameCapture.ts:527,547) prevents later normal frames from rediscovering that event, so a successfully degraded render on those paths can never name it through the new diagnostic. Preserve a sanitized timeout event at every caller, preferably in shared structured render warnings; the test's returned-list assertion and batch ordering regex do not cover this. This is credited prior observability feedback, not a newly discovered duplicate finding or the reason for the security blocker above.

Boundaries, not additional proved regressions: the limit is per decoder invocation/URL, not end-to-end frame/render. Existing outer 15s drawElement, prestarted 30s batch encode and 60s no-progress budgets can win; distinct later seeks/motion-blur samples can accumulate waits. The implementation captures the DOM as it stands—it does not remove an image or guarantee it never appears after late decoding. I have not proved a newly broken formerly-successful base render, deterministic fallback pixels, or universal continuation in those modes. The PR description's broader claims should be qualified.

Scope/evidence: complete changed helper/test and real init/main/batch/cache/diagnostic/watchdog integration paths were inspected; a fresh source adversary also completed. The large frame-capture/producer integrations were bounded in the primary recovery, not a claimed end-to-end whole-repository audit. The original primary failed; its completion was not adopted. Automatic memory-index/context exposure is disclosed. No tests, replay, browser/native/render or CI execution was performed; old peer runtime claims are not tai evidence. The current successor only adds nullish-list tolerance; that does not repair the diagnostic sink. Check color is not the basis of this verdict. No merge action.

— tai

Verdict: REQUEST CHANGES
Reasoning: The new raw-URL timeout diagnostic bypasses the established sanitizer and can persist sensitive URL components in frame-error JSON. The bounded-wait implementation does not require abandoning that redaction invariant.

[heygengenesis]

Approving at dcc3da71 after the rebase onto #6984. I found no blockers.

What changed since 09ec50f3: I compared the two patches. The only content difference is one line in prepareFrameForCapture: the loop now reads (await decodeDynamicCssBackgroundImages(page)) ?? [], with a comment about mocked pages. The test file and the rest of frameCapture.ts are the same patch.

Why the guard is needed: I ran vitest run src/services/frameCapture at this head: 31 files and 339 tests pass, 11 skipped. When I removed the ?? [] and ran the same suites again, about 10 existing tests failed (the motionBlur seek display, serial/pipelined untrusted capture, boundary screenshots, pipelined diagnostics). Their mocked page.evaluate returns undefined, and iterating undefined throws. So the guard is load-bearing for the existing suite, and it is harmless with a real page. tsc --noEmit is clean in packages/engine.

Earlier checks still hold: The patch is otherwise identical, so the red/green check on stops waiting for a url whose decode never settles and names it from my previous review still applies.

Non-blocking:

• My earlier notes still apply: urls that time out in finalizeDrawElementInit or the batch loop are not named in diagnostics, and the type in drawElementService.ts is stale.
• A cleaner long-term option is for the shared mock to return [], so the production code doesn't have to allow for test pages.

---

Reviewed by Genesis V2 on behalf of @miguel-heygen.

[terencecho]

Re-reviewed #6972 at 7540d84ae07b719a699a7ec4543bd0bd65bb4cce against true merge-base 87a42e7470c28aa689934a28a0a53784459b99f2. The absolute-URL redaction case and normally returning init/batch warning paths are fixed, but a supported relative-URL input still bypasses redaction.

What improved: packages/engine/src/services/frameCapture.ts:1388,3408,5264-5271 now records timeout warnings from init, ordinary preparation and returned batches. The batch collects and drains late URLs instead of discarding them (packages/engine/src/services/drawElementService.ts:1114-1117,1244-1248). The per-URL timeout also retains separate success/timeout caches, retryable ordinary decode rejection and timer cleanup (packages/engine/src/services/frameCapture.ts:535-558).

Blocker P2 — redact path-relative background URLs before the new diagnostic sinks. packages/engine/src/services/frameCapture.ts:2505-2516 passes the decoder's strings through sanitizeDiagnosticUrl, but that helper only provides a base for slash-leading values. For a valid bare or dot-relative CSS asset reference carrying a private query/fragment—for example, illustrative url("./assets/bg.webp?example=REDACT_ME#fragment")—new URL(input) throws and the catch returns the complete input (:660-683).

This input is reachable: the decoder reads the specified element.style.backgroundImage, extracts the original URL text and returns that same string on timeout; assigning image.src does not replace the local string with the resolved URL (:519-553). Dynamic seek-time inline styles are not restricted to absolute URLs. Consequently the new reporter retains that query/fragment in both browserConsoleBuffer and warning.details.sources; recordCaptureWarnings immediately prints those details through console.warn (:2483-2494). On a later frame failure the buffered line can also enter local diagnostics JSON (:3318-3327, subject to buffer retention and successful writes). This is a source/control-flow finding, not an observed credential exposure or executed browser reproduction.

Sanitize supported relative references with a safe base or a redacted relative-path representation, and do not return raw input on parse failure. Keep the original decode/cache identity unchanged; redact only diagnostic copies. Extend the recorder regression to bare and dot-relative query/fragment inputs and assert removal from both text and warning sources. The new test at packages/engine/src/services/frameCapture-cssBackgroundReady.test.ts:147-169 covers only an absolute URL, so it does not discriminate this branch.

Prior concern disposition: my earlier absolute credential/query example is resolved, not unchanged; the narrower relative-input case leaves the redaction invariant incomplete. Genesis's init/batch observability concern is resolved on normal returns. I am not claiming every failed/abandoned capture necessarily publishes its warnings. The existing outer frame/batch deadlines can still win; the description now appropriately limits the 15-second claim rather than promising end-to-end completion or absent pixels.

Important test gap, not a second proved code blocker: the batch source-pattern assertion (packages/engine/src/services/frameCapture-cssBackgroundReady.test.ts:190-202) and direct reporter test do not exercise init recording, batch drain/host recording, failed-batch fallback or production deadline composition. Add connected nonempty-timeout propagation cases; the currently inspected wiring itself is present.

Scope: full four-file delta and relevant decoder/caller/warning/lifecycle source, plus a fresh independent source adversary. The required named fails without the fix: line is present. No target tests, mutants, browser/render/native/provider execution or CI verification; peer runs are not my execution evidence. No merge or peer-review dismissal.

— tai

Verdict: REQUEST CHANGES
Reasoning: Valid path-relative CSS URLs can still carry private query/fragment material into the new timeout diagnostics despite the redaction fix. Resolve that remaining input-domain gap before merge.

[terencecho]

Re-reviewed #6972 at 194f9a6078c2be505db668d4a2b61a389c588455 against true merge-base 87a42e7470c28aa689934a28a0a53784459b99f2, including the incremental change from 7540d84ae07b719a699a7ec4543bd0bd65bb4cce. The bare/dot-relative query-and-fragment P2 from my prior review is resolved; no new blocker was established in this source-only review.

What is fixed:

• packages/engine/src/services/frameCapture.ts:660-679 now handles bare and dot-relative references with a safe-base pathname fallback. Their query/fragment no longer reaches either the diagnostic buffer or structured warning sources through the shared reporter (:2496-2516). Original asset URLs still drive decoding/cache identity; only diagnostic copies are normalized.
• The assertions in keeps only the path of bare and dot-relative URLs (packages/engine/src/services/frameCapture.test.ts:456-462) and reports a timed-out relative url without its query or fragment (packages/engine/src/services/frameCapture-cssBackgroundReady.test.ts:171-181) distinguish the previous implementation by source inspection. I did not execute them or demonstrate RED/GREEN results.
• The earlier absolute credential/query finding remains resolved. Genesis's init/batch warning-loss concern is also resolved on normal returns: init and preparation call the recorder, while the batch collects/drains late URLs and its host caller records them (packages/engine/src/services/frameCapture.ts:1385,3405,5268; packages/engine/src/services/drawElementService.ts:1114-1117,1244-1248).

Important follow-up — connected propagation coverage: the source-pattern batch assertion (packages/engine/src/services/frameCapture-cssBackgroundReady.test.ts:202-214) and direct recorder tests do not execute initialization recording or the batch queue drain → returned lateBackgrounds → host recorder chain. The fresh-fallback fixtures supply only empty lists. Add nonempty connected cases; the production wiring is present in the inspected source, so this is not a second proved code blocker.

Redaction boundary, not a confirmed new-path exposure: the existing data:/blob: guards remain case-sensitive (packages/engine/src/services/frameCapture.ts:662-671). For a mixed-case opaque scheme supplied directly to the sanitizer, the inspected control flow permits bypassing those guards and retaining payload/identity after URL parsing. Those guards predate this PR; the potentially new route is the timeout reporter receiving original inline-CSS spelling (:519-553). I have not established that the browser used for capture preserves that spelling through CSSOM and that such an image reaches the timeout recorder. I am therefore not presenting this as a reproduced disclosure or a newly confirmed blocker. Classifying normalized protocols and adding mixed-case recorder cases would harden this boundary. Likewise, the last-resort split (:677) is not a universal credential scrubber. This approval resolves the source-established relative-input finding, not a certification that every possible string is safe.

Timing and evidence limits: distinct URLs decode concurrently; success and timeout caches remain separate, ordinary rejection is retryable, and timers are cleared in finally (packages/engine/src/services/frameCapture.ts:537-558). The 15-second limit is per call/URL, not an end-to-end guarantee. Existing outer deadlines can win, failed/abandoned paths need not publish warnings, and the code captures the DOM as it stands rather than enforcing absent pixels. Best-effort warnings and strict-mode rejection were source-traced through current producer consumers, not exercised end to end.

Scope: all five current changed files and both full merge-base/incremental diffs were read by the source reviewers, with current decoder/caller/warning/output contracts traced and an independent adversarial pass adjudicated. The required named fails without the fix: line is present. No target tests, mutants, imports, builds, typechecks, browser/render/native/provider execution or CI verification were performed; historical author/peer runs are not my results. No merge or peer-review dismissal.

— tai

Verdict: APPROVE
Reasoning: The prior source-traced inclusion of bare/dot-relative query and fragment is repaired at both new diagnostic representations, with test assertions that distinguish the previous implementation by inspection. The retained coverage gap and conditional adjacent hardening candidate do not establish another introduced blocker within this review's stated evidence bounds.

[heygengenesis]

Approving at bb149a88. I found no blockers.

This head is more than a branch update from main. Compared with dcc3da71, which I last approved, the branch adds three of its own commits besides the merge from main: 7540d84ae0, 194f9a6078 and 014b4b87f2. I reviewed the new content:

• Timeouts reported at every caller: recordBackgroundDecodeTimeouts now covers prepareFrameForCapture, finalizeDrawElementInit and the drawElement batch. The batch loop collects late urls in window.__hf_css_background_late, and produceDrawElementFrameBatch drains them with splice(0) after the batch. This closes both gaps from my earlier review, and the stale Promise<void> type in drawElementService.ts is fixed.
• Warning shape: it reuses the existing media_readiness_timeout code with mediaType: "image", which types.ts already allows. recordCaptureWarnings dedupes on code and details, as the new test exercises.
• URL redaction: sanitizeDiagnosticUrl now reduces any relative reference to its path. Before, a relative url that didn't start with / came back raw, so this tightens the existing navigation and request-failure diagnostics too. Absolute, data: and blob: handling is unchanged. The new tests cover credentials, query and fragment for both absolute and relative urls.
• Verified locally at this head: vitest run src/services/frameCapture src/services/drawElement gives 33 files and 366 tests passing, 14 skipped. tsc --noEmit is clean in packages/engine. CI is green on this SHA. The decode/timeout logic, and so the fails without the fix: test, is unchanged from what I red/green-checked before.

Non-blocking:

1. The drain page.evaluate after the batch also runs when the batch returned a failure outcome. If that evaluate rejects, for example because the target closed right after, it replaces the batch's own failedAt/error. Wrapping it in .catch(() => []) would keep the original failure as the one reported.
2. The PR body still says "2 files, +65 -14" and doesn't mention the reporting or redaction changes. It's worth updating before merge.

---

Reviewed by Genesis V2 on behalf of @miguel-heygen.

[terencecho]

Post-merge advisory review (already merged into main). Findings are follow-ups, not gating.

Reviewed bb149a880257d3947d632472dc3d1ce7edfaf5b8 against true merge-base 2679bcdee71aa0b1d706fd845f399fa07fc2c48f.

Strengths / prior concerns resolved

• frameCapture.ts:537–560 bounds each new URL's decode wait, clears its timer and keeps ordinary decode rejection retryable. Original URLs remain decode/cache identities.
• frameCapture.ts:660–679,2496–2515 sanitizes diagnostic copies before buffered text and structured sources. The earlier absolute-URL and relative query/fragment concerns are fixed; init, ordinary preparation and normally returned batch reporting are now wired (:1385,3405,5261–5268).

Important — preserve timeout warnings from every session whose frames remain in output.

The new media_readiness_timeout is stored on the capturing session (frameCapture.ts:2480–2515), but some contributing sessions never export it:

1. Risk screenshot session: captureStreamingStage.ts:1114–1124 supplies risk-frame bytes from riskSession; :1171 summarizes only the main session and :1191–1192 closes both. This route requires sequential streaming, eligible nontransparent worker encode, an accepted minority risk plan/memory budget and no forced drawElement override (frameCapture.ts:1150–1153,1223–1246,1306–1311). A background assigned only in a risk interval, absent at initialization and ordinary main-session times, can therefore warn only on riskSession. If its 15s decode timeout is followed by successful screenshot/encode within the remaining budgets, its frame contributes output but its warning does not. The screenshot branch bypasses the separate drawElement-only 15s host deadline (frameCapture.ts:5030–5060); the default producer no-progress guard is 60s, refreshed before risk capture (captureStreamingStage.ts:535–548). This is a permitted conditional source schedule, not an observed browser completion.
2. Retained disk prefix: captureStage.ts:519–546,559–576 can retain already-written frames after a retryable browser death and resume at the first missing frame. Its warning/perf export occurs only after the entire loop succeeds (:665). If an earlier retained frame timed out and its background is absent from the resumed suffix, the old session closes without forwarding that timeout warning. I have not established warning-free recovery, so this route alone is not proof of an unqualified final status.

The final policy receives only collected summaries/layered warnings (renderOrchestrator.ts:5516–5519). For the risk-only schedule above, with no other warnings and otherwise successful completion, strict mode can miss this timeout (:824–871), and the job can be classified completed rather than completed_with_warnings (render/shared.ts:263–272). A host console.warn still exists; the gap is structured job qualification/policy, not total log silence or proof of missing pixels.

These collector omissions predate this PR: complete source bytes match the true base, including captureStreamingStage.ts:1171–1192 and captureStage.ts:665. This PR newly converts the never-settling decode into a returned frame plus warning, so that outcome needs connected integration. Preserve correctness warnings when their frames survive; discard them when the associated output is replaced. Add risk-only and retained-prefix retry cases, plus a discarded/replaced-output case proving abandoned warnings do not contaminate surviving output. Exercise strict rejection and best-effort qualification for the retained-warning cases.

Coverage / notes

• Two independent source passes read all five changed files; I adjudicated the load-bearing source and full diff. Prior-head integration was 40 commits/242 paths: relevant consumer/dependency contexts were inspected, not all 242 files end-to-end. Five-file parity with 194f9a60 does not establish runtime parity. The five PR files and cited producer consumers also match the merge commit's blobs.
• The required fails without the fix: line names the real never-settling-decode test (frameCapture-cssBackgroundReady.test.ts:130–145): its fake decode never resolves and it expects a bounded return, whereas the old helper awaited indefinitely. Actual RED/GREEN remains unverified: I ran no tests, typecheck, build or browser/render. Batch coverage at :202–214 is source-shape only; empty lateBackgrounds fixtures do not establish connected nonempty propagation.
• As @heygengenesis[bot] noted in review 5775595901, the separate batch-drain evaluate can mask an original returned failure (drawElementService.ts:1244–1248). I verified that boundary, not a second demonstrated successful-output defect. The URL-readiness timer is per invocation/URL, distinct from host deadlines and not an end-to-end render guarantee.

— tai

Verdict: COMMENT (post-merge: With fixes)
Reasoning: The prior diagnostic-redaction and init/ordinary/returned-batch reporting concerns are repaired, but contributing-session boundaries can still drop timeout warnings for retained output. Follow-up warning retention and connected policy tests are warranted; this advisory does not gate or reverse the merge.