feat(engine,producer,cli): capture failing dB/frame index on drawElement verify fallback

[vanceingalls]

What

DrawElementVerificationError now carries structured frameIndex/failedDb/verifyThresholdDb fields instead of only a formatted message string. Two new telemetry properties flow from them: de_fallback_failed_db and de_fallback_frame_index, on both render_complete (perfSummary path) and the crash-survival RenderCaptureObservability mirror (so a hard failure that happens right after a fallback attempt — before perfSummary is ever built — still reports the failing dB/frame).

• New getDrawElementVerificationDetails(err) helper in frameCapture.ts (engine), same chain-walk pattern as the existing isDrawElementVerificationError, so it survives duplicated module instances and CaptureStageError wrapping.
• All 3 throw sites in captureStreamingStage.ts (2 blank-frame, 1 PSNR) now pass the detail through.
• renderOrchestrator.ts's catch block extracts details via the helper instead of regexing the error message, threads two new locals (deFallbackFailedDb/deFallbackFrameIndex) through both DrawElementPerfInput (perfSummary.ts) and RenderCaptureObservability (observability.ts).
• render.ts + telemetry/events.ts map both fields through to PostHog on render_complete and render_error.

Why

Working the current DE parallel-router soak (v0.7.52+, de_parallel_router telemetry), de_fallback_reason already discriminates psnr | blank | oom | capture_error, but a psnr revert carries no score — the failing dB only ever existed as text inside the thrown error's message, which the orchestrator never captured. Can't currently tell "32.1dB just under the 32dB threshold, maybe tune it" from "12dB real corruption, investigate the bug" — that distinction should drive whether the next step on the soak is a threshold tweak or a fix.

How

Structural discriminant fields on the error class (mirrors the existing deVerificationFailure boolean pattern) rather than re-parsing the message — getDrawElementVerificationDetails walks the same .cause chain as isDrawElementVerificationError since CaptureStageError wraps the original error before the orchestrator sees it. failedDb gets the same rounding/clamp treatment as the existing verifyMinDb field for consistency (round to 1 decimal, clamp at 999).

Test plan

• Unit tests added: frameCapture.test.ts (engine) — the new class fields + extraction helper, including a cause-chain-wrapped case; renderObservability.test.ts — the crash-survival mirror carries both fields for a psnr fallback and leaves them undefined for blank/oom/capture_error; events.test.ts — both render_error and render_complete emit the new PostHog properties
• Full typecheck clean on engine/producer/cli
• renderOrchestrator.test.ts (141/141), captureStreamingStage.test.ts (6/6, bun test), frameCapture.test.ts (23/23), events.test.ts + renderObservability.test.ts (49/49) all pass
• Confirmed 5 unrelated pre-existing test failures (macOS resource-fork/tmpdir flakiness in lintProject.test.ts and render.test.ts) reproduce identically on a clean origin/main checkout — not caused by this change

🤖 Generated with Claude Code

[vanceingalls]

• feat(engine,producer,cli): capture failing dB/frame index on drawElement verify fallback #2411 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[miga-heygen]

One-line: Structured observability fields on DrawElementVerificationError — replaces message-text regex with typed data. Clean throughout.

Strengths:

1. frameCapture.ts:201-256 — This is an SSOT win. The failing dB and frame index previously existed only as formatted text inside the thrown error's message. The orchestrator had to either regex that text or discard it. Structural discriminant fields on the error class are the right shape — the data is born structured and stays structured all the way to PostHog.

2. getDrawElementVerificationDetails (frameCapture.ts:242-260) mirrors the existing isDrawElementVerificationError cause-chain walker exactly: same depth cap, same structural discriminant check, same duplicated-module-instance resilience. Good discipline — one pattern for "is it?" and one for "what's in it?", both walking the same chain.

3. Test coverage of the cause-chain-wrapped case (frameCapture.test.ts:247-254) is the non-obvious edge that would bite in production — the producer's CaptureStageError wraps the original before the orchestrator sees it. Testing that the helper finds the details through the wrapper is the right test.

Blockers: none.

Important: none.

Nits: none.

Notes:

• All three throw sites in captureStreamingStage.ts correctly differentiate: blank-frame (2 sites) passes { frameIndex } only (no PSNR score exists), PSNR breach (1 site) passes { frameIndex, failedDb, verifyThresholdDb }. The telemetry tests verify this distinction — renderObservability.test.ts:97 confirms captureDeFallbackFailedDb is undefined for blank/oom/capture_error. Contract is complete at all throw sites (Rule 2).
• The perfSummary path rounds/clamps fallbackFailedDb (perfSummary.ts:119-121, same Math.round(Math.min(x, 999) * 10) / 10 as existing verifyMinDb), while the crash-survival RenderCaptureObservability path passes the raw value. Both are correct — perfSummary normalizes for analysis, the crash mirror captures maximum fidelity before potential death.
• Constructor change is backward-compatible (details? is optional) — existing new DrawElementVerificationError(msg) callers outside this diff continue to work unchanged.
• regression-shards shard-2 still pending at review time; all other required checks green.

Verdict: COMMENT (LGTM, no blockers — clean addition)
Reasoning: Well-scoped SSOT improvement with complete throw-site coverage, proper cause-chain resilience, and test coverage on the non-obvious wrapped-error edge. Ship it once shard-2 clears.

— Miga

[james-russo-rames-d-jusso]

Reviewed at b44a34ae.

Structural improvement over the regex-the-error-message shape, and it composes cleanly with the observability rebuild (crash-survival mirror pattern from #2331/#2231 preserved). Verified the mechanism claims: getDrawElementVerificationDetails uses the same discriminant + chain-walk pattern as isDrawElementVerificationError so it survives duplicated module instances + CaptureStageError wrapping (test "finds details through a wrapping cause chain" pins the one-level wrap), all three captureStreamingStage.ts throw sites pass an appropriate details literal (blank-frame sites omit failedDb as the JSDoc claims), and both the perfSummary and RenderCaptureObservability paths extract via the same helper — so a hard failure right after a psnr revert (before perfSummary is built) still surfaces the failing dB on render_error alongside render_complete.

Concerns

• Precision asymmetry between the two dB paths (renderOrchestrator.ts:2644 vs perfSummary.ts:342-345). The perfSummary aggregation rounds to 1 decimal + clamps to 999 (Math.round(Math.min(fallbackFailedDb, 999) * 10) / 10), but the crash-survival mirror passes the raw deFallbackFailedDb straight into updateCaptureObservability unrounded. When both events fire for a single job (soak scenario: render_complete follows a psnr revert successfully, and a hard failure also produces render_error on a sibling run), PostHog sees 28.4 on one event and something like 28.437 on the other for the same underlying dB — consumers joining across the two events will see mismatches. Cheapest fix is mirroring the round/clamp into the observability path (single-line helper, or inline the expression). Either that or documenting the asymmetry so soak analysts don't chase phantom differences.

Nits

• verifyThresholdDb is captured on the engine's DrawElementVerificationDetails interface + on the PSNR throw site, but not threaded through to producer / CLI / PostHog. The engine surface is one field wider than the consumers use. Two reasonable directions: (a) add de_fallback_threshold_db on both events — lets ops see "28.4dB failed a 32dB threshold" directly in PostHog without a config lookup, which is exactly the "should I tweak the threshold or investigate a bug" call this PR is enabling; or (b) drop verifyThresholdDb from the engine interface if it's not planned for downstream propagation. Either is fine, but the current shape has an unused hook.
• Math.min(fallbackFailedDb, 999) at perfSummary.ts:344 — if db ever comes through as NaN (unlikely — PSNR calculation is well-defined for the compared frames), Math.min(NaN, 999) returns NaN and the field ships as NaN to PostHog. Not worth guarding today, just noting the edge.

What I didn't verify

• Whether soak dashboards / BI queries already exist keyed on the naming — de_fallback_failed_db / de_fallback_frame_index are new fields so this is greenfield.
• Cross-repo consumers of the new engine exports (getDrawElementVerificationDetails, DrawElementVerificationDetails) — grepped the workspace, no external consumers yet.

LGTM from my side aside from the precision-asymmetry decision.

— Review by Rames D Jusso

[miguel-heygen]

Reviewed at b44a34ae523e5e66b09b1b5703624027b68ab461 after shard-2 and every required check completed green.

Finding

• [BLOCKER] The fallback reason still depends on parsing the human-readable error message. packages/producer/src/services/renderOrchestrator.ts:2618-2621 still classifies a structurally recognized DrawElementVerificationError with /blank/i.test(err.message). That leaves the new cross-module structured contract incomplete: a blank failure whose wording changes, a duplicated-module error with a different message, or a JSON-round-tripped structural error (the custom numeric/discriminant fields serialize, while Error.message does not) is silently reported as psnr. This directly contradicts the stated move away from message regexes and corrupts the new telemetry taxonomy. Put the failure kind ("blank" | "psnr") in DrawElementVerificationDetails at the three throw sites (captureStreamingStage.ts:290-344), derive deFallbackReason from the helper, and pin both real blank and PSNR paths so changing message prose cannot change telemetry.

Additional evidence

• All three constructor sites were enumerated: the two blank guards carry only frameIndex, while the PSNR guard carries frameIndex, failedDb, and verifyThresholdDb. All consumers of the new detail helper and both telemetry mirrors were traced. The cause walker is bounded, cycle-safe, structural rather than instanceof, and handles undefined/partial numeric fields without throwing.
• The new focused tests pass: 71 Vitest assertions across frameCapture.test.ts, events.test.ts, and renderObservability.test.ts, plus 6/6 Bun tests in captureStreamingStage.test.ts. However, the producer test mocks DrawElementVerificationError as an empty class and does not exercise either real verification throw site; the current blank/PSNR tests construct synthetic errors/payloads, so they cannot catch the remaining message-classification dependency.
• As Rames-D noted, dB normalization is asymmetric. There is an additional consumer consequence: CLI render_complete explicitly overrides the raw observability mirror with rounded perfSummary.drawElement.fallbackFailedDb, but Studio's perfPayload (packages/cli/src/server/studioRenderTelemetry.ts:69-92) only spreads renderObservabilityTelemetryPayload, so Studio success events retain raw precision while CLI success events round to one decimal. Normalize at the producer observability boundary (or thread the authoritative perf field through Studio too) to keep the same PostHog property stable across source/event paths.

Strengths: replacing detail extraction with a structural cause-chain helper is the right architecture; the crash-survival mirror is correctly updated before the retry; the new telemetry keys remain numeric and optional; and CI, including regression shard-2, is fully green.

Verdict: REQUEST CHANGES
Reasoning: The structured detail pipeline is sound, but the verification subtype still comes from mutable message text, so blank failures can be mislabeled as PSNR across the exact module/serialization boundaries this PR is intended to harden. Move the subtype into the structured contract and add message-independent blank/PSNR path tests before approval.

— Deepwork

[vanceingalls]

Fixed both points from Rames's review at `24d8a68`:

• Precision asymmetry: extracted the existing inline round/clamp expression (was duplicated for `verifyMinDb` and `fallbackFailedDb` in `perfSummary.ts`) into a shared `roundDb` helper, now applied once at the single point `deFallbackFailedDb` is derived from the thrown error in the orchestrator — both the `render_complete` perfSummary path and the crash-survival `RenderCaptureObservability` mirror now see the identical rounded value.
• Unused `verifyThresholdDb` hook: went with option (a) — threaded it through as `de_fallback_threshold_db` on both `render_complete` and `render_error`, so a psnr fallback now reports "28.4dB failed a 32dB threshold" directly.

Left the NaN edge as noted (not worth guarding).

New test: `perfSummary.test.ts` for the shared `roundDb` helper (rounding, clamp-at-999, idempotence). Extended the existing psnr-fallback tests in `events.test.ts` and `renderObservability.test.ts` to cover the threshold field. All 3 packages typecheck clean, 145 producer + 48 cli telemetry tests pass.

[miguel-heygen]

Reviewed at 24d8a6836443e3a3f08f2ea891f58753714efc60.

Prior blocker remains

• [BLOCKER] renderOrchestrator.ts:2626-2629 still derives blank versus psnr from /blank/i.test(err.message). The new head does not add a subtype to DrawElementVerificationDetails (frameCapture.ts:208-212) or to any of the three real throw sites (captureStreamingStage.ts:290-344). Consequently, changing the human-facing message, crossing a module boundary with a differently worded structural error, or serializing the structured fields without Error.message still silently relabels a blank failure as psnr. This is the exact contract-drift path from the previous requested-changes review, and the b44a34ae..24d8a683 delta does not touch the engine detail type or throw sites.

Required fix: add a message-independent failure kind ("blank" | "psnr") to the structured details, set it at all three throw sites, derive deFallbackReason from getDrawElementVerificationDetails, and add tests proving message changes/serialization cannot change the blank/PSNR telemetry classification.

Delta verified

• The precision asymmetry is fixed correctly: roundDb is now the shared normalization boundary, and both the crash-survival observability mirror and perf summary receive the same rounded/clamped dB.
• verifyThresholdDb is now threaded through both render_complete and render_error telemetry as de_fallback_threshold_db.
• Focused verification passed: 75 Vitest assertions across the engine/perf/CLI telemetry suites and 6/6 producer capture-stage tests.
• CI is otherwise green at this snapshot, but the eight regression shards are still running. That would independently prevent an approval until completion.

Verdict: REQUEST CHANGES
Reasoning: The precision and threshold follow-ups are sound, but the original blocker is unchanged: fallback subtype still comes from mutable error prose rather than the structured contract, so blank failures remain vulnerable to silent PSNR misclassification across the boundaries this PR is intended to harden.

— Deepwork

[vanceingalls]

Fixed the blocker at `3789a6a` — good catch, this was the actual bug the PR was supposed to eliminate and I'd only closed it for the numeric fields, not the discriminant itself.

• `DrawElementVerificationDetails` now carries a required `kind: "blank" | "psnr"` field. Making it required (not optional) means TypeScript fails the build on any throw site — present or future — that omits it; confirmed this by adding the field first and letting the compiler point at the 3 real call sites in `captureStreamingStage.ts` that needed updating.
• The orchestrator now derives `deFallbackReason` from `getDrawElementVerificationDetails(err)?.kind`, not `/blank/i.test(err.message)`.
• New adversarial tests in `frameCapture.test.ts` prove message-independence directly: a reworded message containing neither "blank" nor "psnr" still classifies correctly, and a psnr failure whose message happens to contain the literal substring "blank" (quoting a neighboring log line) still reports `kind: "psnr"` — exactly the case a regex would get wrong.

Also re-checked your Studio `perfPayload` note from the first review: since `deFallbackFailedDb` is now rounded once at the single source (the `roundDb` fix from the previous commit), Studio's path — which only spreads `renderObservabilityTelemetryPayload` — inherits the already-rounded value with no separate fix needed. Confirmed by reading `studioRenderTelemetry.ts:69-92` again.

All 3 packages typecheck clean; 25 engine + 145 producer + 6 bun-test + 48 cli telemetry tests pass.

[miguel-heygen]

Reviewed at 3789a6a04c281c515f944260a90af5c130bf4b30.

Prior blocker closed

• packages/engine/src/services/frameCapture.ts:211-235 makes kind: "blank" | "psnr" a required part of DrawElementVerificationDetails and the constructor, so future in-repo throw sites fail typecheck if they omit the subtype.
• All three production throw sites are correctly pinned: the two blank guards set kind: "blank" (captureStreamingStage.ts:290-310), while the PSNR breach sets kind: "psnr" with the score and threshold (captureStreamingStage.ts:342-345).
• renderOrchestrator.ts:2626-2639 now derives deFallbackReason solely from the structural helper and uses the same extracted record for rounded dB, frame index, and threshold. The old /blank/i dependency is gone from the render path.
• The message-independent tests cover both directions: a blank failure whose message contains neither keyword remains blank, and a PSNR failure whose message contains the word "blank" remains PSNR (frameCapture.test.ts:286-309).

Contract audit

• Audited all 13 changed files and enumerated every DrawElementVerificationError construction and every helper/telemetry consumer. The cause walk remains bounded and structural across CaptureStageError/duplicated-module boundaries; partial numeric fields are ignored safely; class fields plus deVerificationFailure remain enumerable for JSON-style structural transport.
• Telemetry stays bounded to the existing four reason values plus three optional numeric properties. roundDb is still the single normalization point, so CLI and Studio mirrors now agree on precision and type.
• Crash-survival ordering remains correct: observability is updated with the extracted fields before the screenshot retry, while the successful path later threads the same locals into perfSummary.
• Focused verification passed locally: 77/77 Vitest assertions across engine, perfSummary, and CLI telemetry, plus 6/6 Bun capture-stage tests.

No new blockers found in the current-head delta.

Verdict: APPROVE
Reasoning: The original message-parsing bug is closed at the source: subtype is now a required structural field at every throw site, the orchestrator consumes that field directly, adversarial message tests pin blank/PSNR stability, and the telemetry/precision/crash-survival contracts remain consistent.

— Deepwork

[jrusso1020]

Approved. I ran a fresh, context-isolated adversarial pass on this final head (3789a6a) — independently tracing all five failure vectors to ground truth rather than stamping on the review alone — and it came back clean:

• Failure-kind completeness: kind: "blank" | "psnr" is type-required on DrawElementVerificationDetails (non-optional), so any throw site omitting it is a compile error. All 3 production construction sites (captureStreamingStage.ts:290/308/342) set it correctly; the deVerificationFailure discriminant is set only in the constructor, so detection can only fire on a real instance that always carries kind. Green Typecheck confirms no un-updated site.
• No residual prose-parsing: the orchestrator now reads verifyDetails?.kind (renderOrchestrator.ts:2631); the old /blank/i message match is gone, and no other blank-vs-psnr text-matching exists on the failure path.
• Cross-component contract: the error never crosses a serialization boundary (the parallel-drain rethrows the original object by reference before the worker pool flattens it to a string; CaptureStageError preserves .cause by reference; both detectors walk .cause). Telemetry travels as plain JSON-safe number fields with names aligned at every engine→producer→cli hop.
• Precision mirror: a single roundDb (idempotent) feeds both the observability mirror and perfSummary — no raw dB reaches any telemetry number field, so render_complete/render_error agree. The earlier raw-vs-rounded asymmetry is resolved at HEAD.
• Crash-survival: the kind is assigned synchronously and mirrored into observability before the fallback re-render — no window where the fallback fired but the classification is lost.

One non-blocking, pre-existing observation (not introduced here): on the studio render_complete path, explicit perfSummary-sourced de_* keys clobber the observability spread with undefined for fallback renders — identical to the whole existing DE-telemetry cohort (de_worker_inversion, de_parallel_router, …), so #2411 just extends the established shape. The crash-survival render_error path (the one this PR's precision goal targets) is correct. Worth a follow-up note to the studio-telemetry owner, not a blocker.

Combined with Magi's rigorous re-review (which found and verified the closure of the original prose-classification blocker), this is solid. Approving.

Review by Rames Jusso