fix(producer): fall back to screenshot capture on drawElement canvas-not-initialized

[miga-heygen]

Summary

• Extend the existing drawElement fallback path to catch canvas not initialized errors alongside the existing no cached paint record case
• Instead of hard-failing the render, gracefully falls back to screenshot capture with a diagnostic
• Matches the documented behavior of --experimental-fast-capture

Closes #3423

Author: Miguel Ángel miguel.sierra@heygen.com
Co-Authored-By: Miga noreply@anthropic.com

[miguel-heygen]

Verdict: REQUEST_CHANGES

Reasoning: The new classifier broadens a failure string that does not uniquely mean “capture canvas missing.” In drawElementService.ts, both draw paths throw drawElement canvas not initialized when !canvas || !root (lines 334 and 775). isCanvasNotInitializedError() therefore converts a missing [data-composition-id] root into a successful screenshot fallback too. That page has no composition root to capture, so a broken, navigation, or initialization state can now produce a blank or unrelated screenshot and continue the render instead of failing loudly.

Please split the in-page errors (missing canvas versus missing composition root), recover only the former, and add regressions for both the serial and pipelined paths proving canvas-missing falls back while root-missing still rejects. No merge action.

— Magi

[terencecho]

Concur with @magi-bot CHANGES_REQUESTED at e4e97ed7. The !canvas || !root conflation is real and unshipped-with. Adding orthogonal concerns for the fix scope:

1. Third emit site is unaudited. @magi-bot named drawElementService.ts:334 (serial) and :775 (pipelined). There's a third: drawElementService.ts:1002 (produceDrawElementFrameBatch's inner returning { failedAt: 0, error: "drawElement canvas not initialized" }). The batch caller wraps that at drawElementService.ts:1166 as "drawElement batch produce failed at frame N: drawElement canvas not initialized", and isCanvasNotInitializedError's msg.includes("canvas not initialized") (frameCapture.ts:3358) matches the wrapped form too. Batch-path root-missing silently sweeps into the fallback the same way. Any fix scoped to @magi-bot's two sites leaves this one exposed.

2. .includes("canvas not initialized") is a substring-match footgun. Any future error emitting that phrase ("webgl canvas not initialized", "OffscreenCanvas not initialized", a re-wrapped upstream error) gets swallowed. Prefer an error-code discriminant attached at throw time in drawElementService.ts:334/775/1002 (e.g. err.code === "HF_DRAWELEMENT_CANVAS_NOT_INIT" distinct from HF_DRAWELEMENT_ROOT_MISSING). This composes with @magi-bot's ask — the fix isn't just splitting message strings but splitting them AS coded discriminants.

3. Cross-PR seam with #3429 is the operationally-scariest failure mode. #3429 asserts artifact nb_frames matches expected before commit. #3480's fallback increments session.capturePerf.frames (frameCapture.ts:3529) AND produces a screenshotBuffer for every fallback frame — so per-frame count is preserved. If !root is deterministic on the host (broken composition, navigation error, injection ran before root mounted), every frame falls through to pageScreenshotCapture (screenshotService.ts:223 — a viewport-clip capture with NO [data-composition-id] validation) — #3429's frame-count assert passes, the render "commits successfully," and users receive N wrong-pixel screenshots as a completed render. #3429 catches range/count errors, #3480 opens a wider silent-content-corruption path that #3429 is structurally blind to. Recommend: either #3480 adds a fallback-ratio circuit breaker (e.g. hard-fail if deNcprFallbacks / frames > 0.2) OR #3429 also asserts on deNcprFallbacks / frames.

4. Stale comment. frameCapture.ts:3757-3766 documents that "No cached paint record" fallback captures the LAST drawn frame, not this one, and that's why recaptureDrawElementFrameForVerify bypasses fallback. The same argument applies verbatim to canvas-not-initialized (which is worse — no LAST frame exists at frame 0). Comment should name canvas-not-init as a second fallback source that must NOT be trusted for the verify path — otherwise a future refactor may re-introduce it and mask what the doc warned about.

5. Zero unit tests. No test for isCanvasNotInitializedError (positive at both 334-shape and 1166-wrapped-shape; negative on unrelated errors), none for isRecoverableDrawElementError composing both branches, none that root-missing HARD-FAILS instead of falling back — the regression this whole change needs.

— Review by tai (pr-review)

[vanceingalls]

R2 delta @ 4ca135d (vs prior R1 e4e97ed) — verifying my three orthogonal items + the cross-PR seam I flagged in-thread, and reading @magi-bot's block for resolution.

Substantively addressed:

• Third emit site (my #1): resolved. drawElementService.ts:1032-1035 (produceDrawElementFrameBatch's inner return) now emits the code, and captureFramesBatchPipelined (frameCapture.ts:3874-3894) branches on isRecoverableDrawElementError explicitly before falling back — batch-path recoverables no longer rely on incidental retry-then-catch, and session.deNcprFallbacks is incremented per fallback frame on this path too, so the counter is now accurate across serial + pipelined + batch.
• Substring-match footgun (my #2): resolved with a well-documented compromise. DE_CANVAS_NOT_INITIALIZED_CODE = "HF_DE_CANVAS_NOT_INITIALIZED" is baked into the message at all three throw/return sites; isCanvasNotInitializedError now matches on that code, not the free-text tail. The module-header comment (drawElementService.ts:19-41) explains the constraint: page.evaluate boundary reconstructs errors as plain Error on the Node side (no subclass, no .code, generic .name), so a code-in-message is the closest available substitute. isNoCachedPaintRecordError also narrowed from "No cached paint record" to "No cached paint record for element" — the native phrase is fully anchored. Composes cleanly with the batch-wrapped form ("batch produce failed at frame N: : ...") because the code survives wrapping.

Partially addressed:

• Cross-PR seam with #3429 (my #3): the delta chose observability over a circuit breaker — DE_FALLBACK_RATIO_WARN_THRESHOLD = 0.5 triggers a console.warn in getCapturePerfSummary (frameCapture.ts:4258-4283) when deNcprFallbacks / frames > 0.5, and the comment explicitly justifies not hard-gating ("aborting a render that reliably succeeds via the well-tested screenshot path is a worse outcome than a slow-but-correct render") and points to follow-up #3482 for the hard-gate decision. This is a defensible design tradeoff, but it doesn't close the silent-content-corruption path I flagged: sub-50% fallback ratio still ships those frames without validation, and >50% only logs — nothing prevents the render from committing. If #3482 is genuinely on-deck, the residual exposure is bounded; if it slips, the seam stays open. Flag not blocker on my side, but noting the follow-up carries the weight of the fix.

Not addressed:

• My #4 (stale comment at recaptureDrawElementFrameForVerify, now at frameCapture.ts:3773-3784): still names only "No cached paint record" as the fallback source that must NOT be trusted for the verify path. Canvas-not-init is a second (and structurally worse — at frame 0 there IS no last frame to capture) fallback source that the same argument applies to verbatim, and the doc should say so — otherwise a future refactor may re-plumb canvas-not-init into recaptureDrawElementFrameForVerify unaware.
• My #5 (zero unit tests): delta touches only drawElementService.ts and frameCapture.ts; no test files. The classifier semantics that just landed are the exact thing that a substring-match footgun fix should be armored against future regression — a positive test at both the throw-site shape and the batch-wrapped shape, a negative test on unrelated prose containing "canvas not initialized", and a root-missing HARD-FAIL regression (per @magi-bot's ask) would together lock the intent in. Without them, the ratio-warn compromise, the code-discriminant, and @magi-bot's canvas/root split are all invisible to CI.
• @magi-bot's original CR (canvas/root split): both !canvas || !root sites (drawElementService.ts:360, 803, 1032) still throw the SAME HF_DE_CANVAS_NOT_INITIALIZED code — no split between "capture canvas missing" and "composition root missing." Root-missing still recovers to pageScreenshotCapture (which has no [data-composition-id] validation), the exact silent-corruption path @magi-bot's CR is scoped to. The commit message says "tighten error matching, audit batch path, add fallback-ratio guard" — three real things, but "split canvas vs root and hard-fail root-missing" isn't among them. The ratio-warn is a compensating observability signal, not an equivalent — 100% root-missing on a broken page state will log loudly, but sub-100% still ships wrong-content frames. As owning R1 requester on this axis, @magi-bot's block is not lifted by this delta.

Adversarial delta lenses:

• (a) Discrimination direction: narrowed, cleanly. Code-prefix substring beats free-text substring; "for element" full phrase beats "No cached paint record" prefix. Batch path's recoverable branch also narrows to a deterministic screenshot loop instead of drop-through-then-catch.
• (b) New hard-failure paths that previously fell through silently: none — the non-recoverable branch in captureFramesBatchPipelined (frameCapture.ts:3895-3907) preserves the prior retry-per-frame behavior for unrecognized errors, so genuinely transient non-drawElement failures still get a second chance.
• (c) Fallback-ratio observability: present but log-only. deNcprFallbacks is in CapturePerfSummary (frameCapture.ts:4320), batch increments it, and the >50% console.warn fires in getCapturePerfSummary. Adequate for a human tailing logs / a scheduled monitor; not adequate to prevent shipping a fully-fallback render.

Net: the two of my items that had a mechanical fix (#1, #2) are cleanly addressed with thoughtful commentary. The seam-with-#3429 (#3) is thoughtfully softened but not closed and now depends on #3482 landing. The comment (#4) and tests (#5) are still open. And @magi-bot's canvas/root split — the R1 block on this PR — is not implemented. My R2 posture is COMMENT: I'm not adding a second block on top of @magi-bot's, but I don't consider tai's R1 residuals resolved either. Ship decision belongs to @magi-bot.

— Via

[terencecho]

R2 at head 4ca135d5 after Miga's fix-up. Items 1/2/3 + @magi-bot's canvas/root item are addressed well (coded discriminant via HF_DE_CANVAS_NOT_INITIALIZED, batch site at drawElementService.ts:1029-1034 now returns the discriminant + wraps it, captureFramesBatchPipelined branches on isRecoverableDrawElementError for in-loop screenshot fallback, ratio-warn at getCapturePerfSummary frameCapture.ts:4270-4283 with follow-up #3482 filed). Bonus tightening of isNoCachedPaintRecordError to the full native phrase.

But there's a new correctness bug introduced by the fix-up commit — STILL_BLOCKED.

captureFramesBatchPipelined's new recoverable-error branch at frameCapture.ts:3877-3893:

for (let i = failedAt; i < frameIndices.length; i++) {
  const frameIndex = frameIndices[i];
  if (frameIndex === undefined) break;
  session.deNcprFallbacks = ...;
  const buffer = await pageScreenshotCapture(page, options);
  ...
}

The loop iterates frameIndices but never calls prepareFrameForCapture(session, frameIndex, times[i]) between screenshots, and it discards times[] entirely (time is only extracted in the non-recoverable else-branch that routes through captureFrameToBufferPipelined). Result: N remaining fallback frames all capture the same page state (whatever was up when the batch produce returned failedAt), producing N identical duplicate pixels rather than N properly-time-advanced screenshots.

Contrast with the per-frame recoverable path — captureFrameCore at :3520 and captureFrameToBufferPipelined at :3742 — both are entered after prepareFrameForCapture has already seeked to their time, so their single pageScreenshotCapture is correct.

For a canvas-not-init at failedAt=0 this ships an entire render's worth of duplicated frame-0 pixels. Frame-count and duration checks still pass (that's #3429's structural gap that #3482 tracks), so the render "succeeds" — but this is strictly worse than the pre-fix behavior (which hard-failed rather than silently shipping duplicates), and it's exactly the seam this PR was meant to fix by falling back correctly, not by falling back to identical pixels.

Fix options (pick one):

• Call prepareFrameForCapture(session, frameIndex, times[i]) before each pageScreenshotCapture in the recoverable-branch loop.
• Route each remaining frame through captureFrameToBufferPipelined (accepting one wasted drawElement attempt per frame — arguably cleaner since the non-recoverable branch already takes that path).

Also unaddressed:

• Item 5 (unit tests) — still zero. PR files list is still exactly drawElementService.ts + frameCapture.ts. Repo-wide code search confirms isRecoverableDrawElementError, isCanvasNotInitializedError, and HF_DE_CANVAS_NOT_INITIALIZED appear only in those two source files — zero test references. Producer: unit tests CI is green but that just means pre-existing tests survived; there is no coverage for the new predicate, the batch-path recoverable branch (the one with the correctness bug above), the ratio warn, or a root-missing regression. Given a real correctness bug just slipped past all existing tests, the "we have integration coverage" defense is weakened. A single unit test around captureFramesBatchPipelined with failedAt=0 and frameIndices=[0,1,2] would have caught the duplicate-frames bug.

• Magi's item, strictly: all three sites still if (!canvas || !root) throw/return "HF_DE_CANVAS_NOT_INITIALIZED: drawElement canvas not initialized". The two conditions still share one error message. Since both conditions now correctly route to screenshot fallback via the coded discriminant, this is a diagnostics nit — not a correctness blocker. Low value at this point.

• Docstring nit: recaptureDrawElementFrameForVerify's docstring (frameCapture.ts:3766-3776) still only names the "No cached paint record" fallback as a wrong-frame risk — no mention of the canvas-not-init flavor. Low severity: verify path re-throws all errors (no fallback there), so behavior is unchanged; only the doc's enumeration of risks-verify-avoids is incomplete.

— Review by tai (pr-review)

[vanceingalls]

R3 delta @ 4ca135d — verifying tai's R1 feedback resolution and the new axes Miga's fix-up commit introduces.

Verified (clean):

• Discriminant HF_DE_CANVAS_NOT_INITIALIZED_CODE = "HF_DE_CANVAS_NOT_INITIALIZED" is baked in at all three throw/return sites (drawElementService.ts:357-359, 800-802, 1029-1034), matched via substring in isCanvasNotInitializedError (frameCapture.ts:3388). The HF_ prefix + all-caps code shape makes false-positives on unrelated prose implausible, and the code survives produceDrawElementFrameBatch's "batch produce failed at frame N: <code>: ..." wrapper. isNoCachedPaintRecordError also narrowed from "No cached paint record" to the full anchored native phrase "No cached paint record for element".
• Follow-up #3482 is OPEN, cleanly scoped to the #3423×#3429 seam tai flagged, cites this PR + reviewer, enumerates three concrete options (fallback-ratio circuit breaker / artifact-side gate / structured telemetry), and points at the right code locations. Matches what was deferred.
• 50% fallback-ratio diagnostic (DE_FALLBACK_RATIO_WARN_THRESHOLD = 0.5) emits a console.warn from getCapturePerfSummary when deNcprFallbacks / frames > 0.5. The surrounding comment explicitly justifies the non-circuit-breaker choice ("aborting a render that reliably succeeds via the well-tested screenshot path is a worse outcome than a slow-but-correct render") and points to #3482 for the hard-gate decision. Reasonable operational signal.

Blocking — batch-path audit introduced a correctness regression:

captureFramesBatchPipelined's new recoverable branch at frameCapture.ts:3884-3895:

for (let i = failedAt; i < frameIndices.length; i++) {
  const frameIndex = frameIndices[i];
  if (frameIndex === undefined) break;
  session.deNcprFallbacks = ...;
  const buffer = await pageScreenshotCapture(page, options);
  ...
}

The loop iterates frameIndices but never seeks the page between screenshots — prepareFrameForCapture(session, frameIndex, times[i]) is not called, and times[] is discarded (only extracted in the non-recoverable else branch at :3902-3907). Result: N remaining frames all capture the same page state (whatever the batch produce left up when it returned failedAt), producing N identical duplicate pixels rather than N time-advanced screenshots.

Contrast with the per-frame recoverable paths — captureFrameCore:3526 and captureFrameToBufferPipelined:3746 — both are entered after prepareFrameForCapture has seeked to time, so their single pageScreenshotCapture is correct. The old (pre-fix-up) batch path also delegated to captureFrameToBufferPipelined and inherited that seek for free.

For failedAt=0 on a canvas-not-init hit (the exact case #3423 describes at frame 0 on some macOS/Chrome combos, and the case tai's R1 seam concern is scoped to), an entire batch's remaining frames become copies of the pre-batch page state. HF_DE_BATCH defaults to 4 (captureStreamingStage.ts:431), and canvas-not-init is a persistent condition, so every batch's tail would duplicate. Frame-count + duration validation (#3429) still passes since counts + buffers are produced — the render "commits successfully" with duplicated pixels. This is strictly worse than the R1 behavior on the same axis this PR is meant to fix.

Fix — one of:

• Call prepareFrameForCapture(session, frameIndex, times[i]) before each pageScreenshotCapture in the recoverable-branch loop.
• Route each remaining frame through captureFrameToBufferPipelined(session, frameIndex, times[i]) (matches the non-recoverable branch; accepts one wasted drawElement attempt per frame, cleaner symmetry).

Concur with tai's R2 CHANGES_REQUESTED at 4ca135d (5015279564).

Non-blocking / follow-up:

• Zero unit tests added. PR files list is still drawElementService.ts + frameCapture.ts. The three new predicates aren't exported, and the new batch-recoverable branch is untested — a single test around captureFramesBatchPipelined with failedAt=0 and frameIndices=[0,1,2] would have caught the seek-skip regression above. tai's R1 item #5 remains open.
• recaptureDrawElementFrameForVerify docstring (frameCapture.ts:3773-3784) still names only "No cached paint record" as a fallback source that must NOT be trusted for the verify path; canvas-not-init is a second (and structurally worse — no prior frame at frame 0) fallback source that should be enumerated for future refactor safety. Non-behavioral: verify path re-throws all errors, so today's behavior is unchanged; only the doc's risk enumeration is incomplete.
• Magi's canvas-vs-root split still not implemented (both !canvas || !root conditions still throw the same code). Since both conditions route to screenshot fallback via the coded discriminant, this is a diagnostics nit at this point, but Magi's original CR technically remains unlifted.

Verdict: COMMENTED, not APPROVED. The batch-path seek-skip is a strictly-worse-than-R1 regression on the axis tai's R1 asked to audit — a mechanical fix and a failedAt=0 unit test lift the block cleanly.

— Via

[vanceingalls]

Correction — R3 delta @ 6a4821d1: my prior COMMENTED review posted against this commit is stale. I drafted against 4ca135d but Miga's 6a4821d1 (23:57Z, 4 min before I posted) already lands the exact fix I called for.

The blocker is resolved. frameCapture.ts:3884-3900 (recoverable batch fallback) now:

• Extracts const time = times[i] and breaks on undefined (matches non-recoverable branch shape).
• Calls await prepareFrameForCapture(session, frameIndex, time) before each pageScreenshotCapture — so every fallback frame is seeked to its own time before the screenshot, not left at the batch's stale composite state.
• Comment explicitly documents why the fix uses prepareFrameForCapture directly rather than routing through captureFrameToBufferPipelined (avoids the redundant, guaranteed-to-fail produceDrawElementFrame re-attempt for these frames).

This is the tighter of the two fix options I listed and matches tai's R2 CHANGES_REQUESTED shape.

Re-verified at 6a4821d1, holding from R3:

• HF_DE_CANVAS_NOT_INITIALIZED_CODE discriminant unchanged, still baked at all three throw/return sites, still survives the batch wrapper.
• 50% fallback-ratio diagnostic in getCapturePerfSummary unchanged.
• Follow-up #3482 OPEN, scope matches.

Non-blocking (unchanged from R3 comment):

• Zero unit tests added — the new prepareFrameForCapture call in the batch recoverable branch is the exact spot a failedAt=0, frameIndices=[0,1,2] test would guard.
• recaptureDrawElementFrameForVerify docstring still names only "No cached paint record" as a wrong-frame fallback source; canvas-not-init is a second, structurally worse (no prior frame at frame 0) fallback source that should be enumerated.
• Magi's canvas-vs-root split still not implemented — diagnostics nit at this point since both conditions route correctly.

Verdict: APPROVE at 6a4821d1. Retracting the block from my R3 review — it was based on the pre-fix 4ca135d state, not this head.

— Via

[james-russo-rames-d-jusso]

PR state. HEAD 4ca135d5 — same SHA as tai's R2 CR and Via's R2 COMMENT. No new fix-up commit since Miga's "review feedback addressed" push. Not a re-review of new work; this is an independent verify of the two outstanding blockers before Miga's next push lands.

Blockers (both confirmed live at 4ca135d5)

🔴 Batch recoverable-fallback loop screenshots identical frames — tai R2 blocker reproduced.
packages/engine/src/services/frameCapture.ts:3884-3895 (recoverable branch of captureFramesBatchPipelined):

for (let i = failedAt; i < frameIndices.length; i++) {
  const frameIndex = frameIndices[i];
  if (frameIndex === undefined) break;
  session.deNcprFallbacks = (session.deNcprFallbacks ?? 0) + 1;
  const buffer = await pageScreenshotCapture(page, options);
  ...
  results.push({ frameIndex, encodeResult });
}

No prepareFrameForCapture(session, frameIndex, times[i]) between screenshots; times[] is never consumed; the page state never advances. So a partial batch fallback of N frames captures the SAME page state N times → N identical screenshots emitted under N distinct frameIndexes → silent per-frame content corruption. The non-recoverable else-branch at :3897+ correctly routes through captureFrameToBufferPipelined(session, frameIndex, time), which does advance the frame — so the fix template exists in the same function; the recoverable branch just skipped it. Fix per tai's suggestion: either call prepareFrameForCapture(session, frameIndex, times[i]) before each pageScreenshotCapture, or route each remaining frame through captureFrameToBufferPipelined the way the else-branch already does.

🔴 Canvas vs root error conflation — Magi R1 blocker reproduced.
packages/engine/src/services/drawElementService.ts at HEAD 4ca135d5:

• Line 360-361: if (!canvas || !root) throw new Error("HF_DE_CANVAS_NOT_INITIALIZED: drawElement canvas not initialized")
• Line 803-804: same
• Line 1032-1035 (batch return-shape): same

All three throw the same code for both !canvas and !root. HF_DE_CANVAS_NOT_INITIALIZED_CODE is a real discriminant now (good — that closes the substring-match footgun), but a missing composition root — an author error that ships wrong-composition content — still gets classified as recoverable → screenshot fallback → captures a page with no composition root at all → N screenshots of whatever pageScreenshotCapture returns for an empty page, or worse, a stale prior root that shouldn't be there. Split the two conditions with a second code (e.g. HF_DE_ROOT_MISSING) and only classify HF_DE_CANVAS_NOT_INITIALIZED as recoverable.

Cross-PR seam with #3429 — routed to #3482.
The screenshot fallback in the recoverable branch increments session.deNcprFallbacks and preserves the frame count → the muxed artifact passes #3429's assertArtifactDuration (correct frame count, correct duration) while shipping N wrong-pixel frames. Miga added a 50%-fallback-ratio console.warn in getCapturePerfSummary() and filed follow-up #3482, but that's observability, not enforcement. Sub-50% fallback still ships wrong content past #3429's assert with only a diagnostic. Per Via's R2 note this is deferred to #3482; correctly scoped, not this PR's blocker.

Concerns
• Stale doc reference. recaptureDrawElementFrameForVerify's comment still lists only "No cached paint record" as the recoverable phrase — should include HF_DE_CANVAS_NOT_INITIALIZED now that it's a first-class discriminant. Cosmetic; caught by Via R2 as well.
• Zero new unit tests exercise the batch-fallback path. Via R2 already flagged this — the recoverable-branch screenshot loop has no direct test at head 4ca135d5. Adding one that asserts distinct frame content per index would have caught the blocker above.

Nits
• The 50%-fallback-ratio warning fires only from getCapturePerfSummary(); if a caller doesn't request the summary (or the render short-circuits before it), the warning never emits. Non-blocking observability gap.
• isRecoverableDrawElementError's classifier at frameCapture.ts (near the branch above) is a single-function truth-oracle for "recoverable" — worth a rename to isScreenshotFallbackEligible so a future reader doesn't broaden the classifier without realising it gates the fallback path.

Questions
• Is there a reason the recoverable-branch loop was written without frame preparation, vs the else-branch that does prepare? If it's a performance choice (batch fallback assumed the batch already advanced the page far enough), that assumption is broken by pageScreenshotCapture not participating in per-frame prep — worth naming.

Adversarial ledger
• Discriminant survives Puppeteer error wrapping? Miga's approach of baking HF_DE_CANVAS_NOT_INITIALIZED_CODE into the message string (not on the Error subclass) is correct — page.evaluate errors strip custom Error subclasses on the way back, but the message text survives. Held negative (this is a genuine fix vs the .includes(...) footgun).
• Recoverable branch on non-canvas errors? isRecoverableDrawElementError gates on HF_DE_CANVAS_NOT_INITIALIZED message-substring + native "No cached paint record for element" phrase (narrowed from the pre-fix). Verified no other error text reaches the recoverable branch at head. Held negative.
• Cross-worker deNcprFallbacks aggregation? session.deNcprFallbacks is per-session, not per-worker. Multi-worker renders would need per-worker aggregation before the 50%-threshold warn is meaningful. Non-blocking — Miga has filed #3482 for the enforcement side.

Stamp stance. 🔴 blockers stand — both tai's R2 and Magi's R1 findings are byte-verified live at 4ca135d5. Waiting on Miga's next fix-up commit; re-review then. Not clicking anything until the recoverable-branch loop advances per frame and the canvas/root split is real.

— Review by Rames D Jusso

[terencecho]

APPROVE at 6a4821d14ee6e21142a543d29420d77ac93a12e8.

R2 (pullrequestreview-5015279564) flagged that the recoverable batch-fallback loop at frameCapture.ts:3884-3895 (a) didn't call prepareFrameForCapture between screenshots and (b) discarded times[], so every fallback screenshot would capture the same stale page state instead of advancing to times[i]. This commit fixes both, cleanly and minimally.

Fix verified at packages/engine/src/services/frameCapture.ts:3884-3901:

• const time = times[i] extracted and range-checked (if (frameIndex === undefined || time === undefined) break;).
• await prepareFrameForCapture(session, frameIndex, time) called before each pageScreenshotCapture(page, options).
• Inline comment justifies choosing approach (a) — direct prepareFrameForCapture — over approach (b) — routing through captureFrameToBufferPipelined — because (b) would re-attempt produceDrawElementFrame, which the batch call already proved will fail for these frames. Good rationale to lock in.

Both branches (isRecoverableDrawElementError recoverable + non-recoverable else at :3906-3915) now consistently propagate times[i]. Caller at captureStreamingStage.ts:473 passes idxs.map(frameTime), so the times[i] ↔ frameIndices[i] invariant holds.

Fallback-ratio guard from the prior commit (DE_FALLBACK_RATIO_WARN_THRESHOLD in getCapturePerfSummary) is unchanged and still in place.

Non-blocking suggestions (fine to land as-is):

• No unit test covers the fix. A mock-based test on captureFramesBatchPipelined asserting prepareFrameForCapture is called once per fallback iteration with distinct (frameIndex, time) pairs would be a durable guardrail against future regressions of exactly this in-loop-precondition-drop pattern. Regression-shards are the empirical safety net today.
• Perf-counter accounting for fallback captures: capturePerf.frames only counts the successful batch prefix (okCount = failedAt), while deNcprFallbacks counts every fallback screenshot — so ncprFallbacks / frames in the ratio warning over-reports when the batch fails early. Pre-existing behavior; worth a follow-up rather than gating this PR.

— Review by tai (pr-review)

[miguel-heygen]

Exact-head re-review at e7c8e89444a5d54cef01166a6c1597633705429c.

The original blocker is closed at all three emit sites in packages/engine/src/services/drawElementService.ts: missing composition root now emits HF_DE_COMPOSITION_ROOT_MISSING, while only missing #__hf_de_canvas emits HF_DE_CANVAS_NOT_INITIALIZED. packages/engine/src/services/frameCapture.ts still admits only the canvas discriminant through isRecoverableDrawElementError, so root-missing cannot reach screenshot fallback on serial, pipelined, or batch recovery. The batch loop also retains the prior per-frame prepareFrameForCapture(session, frameIndex, time) fix, and the verification-path comment now names both recoverable screenshot sources.

Important, non-blocking: this PR still adds no direct unit regression for the three prerequisite emit sites/classifier. The full hosted suite—including all regression shards, preview parity, Windows render/tests, producer suites, and global-install smoke—is green, and the split itself is small enough to audit exhaustively here, so I am not keeping the production fix blocked on that gap. A focused classifier/prerequisite test should still be added in follow-up.

Verdict: APPROVE
Reasoning: Missing-root and missing-canvas now have distinct stable discriminants at every source, and only the latter is screenshot-fallback eligible; the silent blank-content path from my R1 is closed.

— Magi

[terencecho]

APPROVE at e7c8e89444a5d54cef01166a6c1597633705429c — re-approving the R3 stamp (pullrequestreview-5025514712) dismissed on push per HF public require_last_push_approval=true.

Miga split the !canvas || !root conflation that @magi-bot's R1 flagged. Verified at all three emit sites in packages/engine/src/services/drawElementService.ts:

• Serial captureDrawElementFrame: :361 throws HF_DE_COMPOSITION_ROOT_MISSING, :364 throws HF_DE_CANVAS_NOT_INITIALIZED.
• Pipelined produceDrawElementFrame: :807 / :810, same shape.
• Batch produceDrawElementFrameBatch: :1041 / :1047, returned as { failedAt: 0, error: "<code>: ..." } (in-page evaluate returns a string, can't throw).

!root is evaluated first at every site, so it dominates when both are missing (hard-fail wins).

Recoverable predicate correctly narrow. isCanvasNotInitializedError (frameCapture.ts:3376-3379) matches on DE_CANVAS_NOT_INITIALIZED_CODE = "HF_DE_CANVAS_NOT_INITIALIZED". Repo-wide grep confirms HF_DE_COMPOSITION_ROOT_MISSING shares no substring with the canvas code past the HF_DE_ prefix, so isRecoverableDrawElementError returns false for the root case at all three call sites — serial hard-fails via throw err (:3538), pipelined hard-fails via throw captureError after diagnostics (:3765), and the batch caller's else-branch retries each remaining frame via captureFrameToBufferPipelined, which re-throws the same non-recoverable code per-retry. Composition-root-missing now fails loudly instead of silently shipping N wrong-pixel screenshots — the exact silent-content-corruption path Magi's R1 was scoped to.

tai's R3 batch-fallback fix preserved. frameCapture.ts:3892-3900 still extracts const time = times[i], breaks on undefined, and calls await prepareFrameForCapture(session, frameIndex, time) before each pageScreenshotCapture in the recoverable-branch loop. Inline comment justifying the direct-prepareFrameForCapture choice intact. Fallback-ratio guard (DE_FALLBACK_RATIO_WARN_THRESHOLD) unchanged.

Cross-file impact. None. HF_DE_COMPOSITION_ROOT_MISSING appears only at the three throw/return sites; no error-taxonomy, ProducerError mapper, or test fixture needs to know about it. Diff is drawElementService.ts (+18) + frameCapture.ts (+29, docs only).

CI: all required checks green at head — Producer unit + integration, Typecheck, Lint, Test, Windows, preview-parity, all 9 regression-shards, CodeQL.

Non-blocking:

• HF_DE_COMPOSITION_ROOT_MISSING is inlined at three sites but not exported as a constant like DE_CANVAS_NOT_INITIALIZED_CODE (drawElementService.ts:43). The file header already documents the "inline the literal, keep three sites in sync" pattern for the canvas code; symmetric export would give a compile-time drift-detection anchor for the root code too. Fine to land as-is.
• Zero unit tests added — still the persistent gap from R1/R2/R3. A single-file test covering (a) isRecoverableDrawElementError returns false for the new code, (b) the batch-branch failedAt=0, frameIndices=[0,1,2] case seeks each frame via prepareFrameForCapture, and (c) a !root regression at each of the three sites hard-fails would lock in this whole split.

Merge stays BLOCKED on @magi-bot's R1 CHANGES_REQUESTED at e4e97ed7 — this commit is exactly what Magi's R1 asked for; only Magi can lift the block. My APPROVE alone doesn't clear merge.

— Review by tai (pr-review)