fix(producer): credit held video tails in coverage gate

[miguel-heygen]

Summary

• credit intentionally held final-frame tails for non-looping clips in the video coverage gate
• keep full-slot coverage requirements for looping clips and missing/truncated extraction
• add regression coverage for held tails, loops, and source truncation

Fixes #2565.

Verification

• bunx vitest run packages/producer/src/services/render/videoFrameCoverage.test.ts (21 passed)
• bunx oxfmt --check ... (passed)
• bunx oxlint ... (0 warnings/errors)
• package typecheck is blocked by pre-existing missing generated runtime-inline modules

[vanceingalls]

Summary

Adversarial R1 at head 4e2bc720. Fix is exactly the reporter's suggested min(slotFrames, sourceFrames) for non-looping clips, correctly gated. Held-vs-loop-vs-truncation-vs-missing-extraction paths each verified. Value-lock + reachability-lock both hold. Reporter has no follow-up refutation. Verdict: RIGHT — APPROVE (held until in-flight CI shards go green).

Findings

Fix mechanism — correct

New expression at videoFrameCoverage.ts:143-155:

const sourceFrames =
  entry && !video.loop && Number.isFinite(entry.metadata.durationSeconds)
    ? expectedFramesForClip(0, Math.max(0, entry.metadata.durationSeconds - video.mediaStart), fps)
    : slotFrames;
const expectedFrames = entry && !video.loop ? Math.min(slotFrames, sourceFrames) : slotFrames;

Case matrix:

• Held tail (3s src, 10s slot, loop:false): slotFrames=300, sourceFrames=90, expectedFrames=90, captured=90 → ratio=1. Fixes #2565.
• Loop: entry && !video.loop false → falls back to slotFrames=300. captured=90 → ratio=0.3, gate still fires. Correct; a looping clip is expected to decode the full slot.
• Missing extraction (entry === undefined): falls back to slotFrames; captured=0 → ratio=0. Original injection-failure signal (ts=1784139267) still trips the gate.
• Truncation (source > slot): sourceFrames > slotFrames, min = slotFrames — behavior unchanged.
• Extraction truncated below source (3s src, 60/90 delivered): expectedFrames=90, captured=60, ratio=0.667 < 0.95 → still fails. Confirmed by test #3.
• mediaStart > 0: Math.max(0, durationSeconds - mediaStart) * fps = available source portion from play-offset. Correct.

Value-lock + reachability-lock both hold

Per test-lock-value-vs-reachability discipline:

• VALUE side. Test 1 asserts ratio: 1 (would fail on pre-PR 0.3). Test 2 asserts ratio: 0.3 on the loop regression (would fail if the !video.loop guard were dropped). Test 3 asserts throw VideoFrameCoverageError on extraction-truncated held-tail (locks the failure mode).
• REACHABILITY side. Fixtures set loop, metadata.durationSeconds, mediaStart explicitly, feeding every gate the new branch checks (entry, !video.loop, Number.isFinite(durationSeconds)). The pre-existing partial-extraction test also now sets durationSeconds = 1 (test.ts:131) so it exercises the new sourceFrames path (where min(30, 30) = 30 preserves prior expectation) rather than silently bypassing it.

Threshold + scope — no regression

Threshold constants (resolveVideoCoverageThreshold default 0.95, HF_VIDEO_COVERAGE_THRESHOLD env, 0-disable, clamp) all untouched. Fix modifies expectedFrames computation only. assertVideoFrameCoverage's expectedFrames > 0 skip is unaffected (it already existed for 0-duration windows). Patch is precisely +41/-1 across the two intended files.

Reporter follow-up check

GET /repos/heygen-com/hyperframes/issues/2565/comments → []. Zero follow-up experiments, zero refutation of the body's min(slotFrames, sourceFrames) framing — no feedback-read-reporter-followups-before-pr trigger.

Blame

Coverage-gate module was originally added by Via to close ts=1784139267 (later-injected videos blank in MP4) and to surface authoredTimedClipCount telemetry for ts=1784144554. Miguel's edit preserves the original fail-loud invariant against genuinely truncated/blank source, and narrows the false-positive on intentional held-tail introduced by #2516. No accidental invariant expansion.

Nit (non-blocking)

If video.mediaStart >= entry.metadata.durationSeconds (user configures a play-offset past source end), sourceFrames = 0 → expectedFrames = 0 → assertVideoFrameCoverage's existing expectedFrames > 0 skip silently drops the clip from gate. Matches the pre-existing 0-duration bypass, but worth an inline TODO if we ever want observability on obviously-broken configs. Follow-up only.

CI

Format/Lint/Preflight/Build/SDK/Studio/Producer-unit-tests/Runtime-contract/Fallow-audit all green. Typecheck, Test, Producer-integration, CLI-smoke, Global-install-smoke, regression shards 1-8, Windows-render still IN_PROGRESS at 4e2bc720. No pre-existing red. Author noted package typecheck is blocked by missing generated runtime-inline modules (pre-existing, not PR-caused). Approval held-until-green on non-outage lanes per r1-commented-vs-approve-gate-discipline — but I stake RIGHT now given the fix mechanism + test locks + zero peer reviews to defer to.

Recommendation

APPROVE. Fix is narrowly-scoped, correctness-preserving on the three orthogonal cases (hold/loop/missing/truncation), test coverage locks both value and reachability axes, reporter has no refuting follow-up, threshold constants unchanged. Held-until-green nit on in-flight CI lanes but no blocker.

— Via

[james-russo-rames-d-jusso]

First-look adversarial review at HEAD 4e2bc7203f2ac346e1ea22a8129d203b116f5855. Small, focused fix (2 files, +41/-1) closing the #2565 false positive where the coverage gate aborted renders using #2516's hold-last-frame. Reviewed against the coverage-gate lens (silent-MP4-blank vs. loud abort) and the #2516 held-tail contract.

Findings

Blockers: none.

Concerns: 1 low-medium — silent-gate-disable on durationSeconds ≤ 0 or mediaStart > durationSeconds corner cases (inline C1).

Nits: 2 — test fixture coupling + missing mediaStart>0 coverage (inline N1/N2).

Green surface (verified)

• Formula mirrors the extractor. The sourceFrames = ceil((durationSeconds - mediaStart) * fps) calc matches getFrameIndexAtTime's loopDuration = Math.max(0, durationSeconds - mediaStart) in videoFrameExtractor.ts:1248 — same source-portion semantics, no divergence between what the extractor delivers and what the gate demands.
• !video.loop guard correct. Loops still demand slotFrames (test "still requires the full authored slot for looping clips" covers).
• Missing-entry fallback correct. entry && !video.loop — when entry is undefined (injection failure, ts=1784139267 shape), expectedFrames stays at slotFrames and ratio = 0/slotFrames = 0 still trips the gate. Prior behavior preserved (existing test at line 111-123 continues to hold).
• Math.min(slotFrames, sourceFrames) protects the reverse case (source longer than slot → still gate at slot demand).
• Number.isFinite guards against NaN/Infinity from extractor metadata (see C1 for the finite-but-invalid values that slip through).
• Test naming maps cleanly to the three code paths (held-tail credited, loop still full-slot, source-truncated still fires).
• Single caller — renderOrchestrator.ts:2033, unaffected. Report shape is unchanged (expectedFrames, capturedFrames, ratio), so downstream summarizeExtractionObservability telemetry stays consistent.
• No API break — public VideoFrameCoverageReport interface unchanged.
• CI green so far — required checks passing; producer tests will confirm the 21 in this file plus the surrounding tree.

What I didn't verify

• End-to-end repro of the #2565 case at the CLI. Test math is exercised; the full 3s-in-10s-slot render path is not.
• Whether renderOrchestrator / summarizeExtractionObservability reads raw expectedFrames for a load-bearing purpose besides ratio calc — a clip newly emitting expectedFrames=90 instead of expectedFrames=300 might shift dashboard aggregates. Spot-check suggests the observability path uses minVideoFrameCoverageRatio, not raw expectedFrames sums, so no visible drift.
• Boundary at threshold (e.g. 85 vs 86 captured on 90 expected) — existing patterns don't test threshold boundaries either, so not a regression.

Merge gate

Sound closure of the #2565 false positive with a well-scoped math change and appropriate regression coverage. C1 is a corner-case silent-disable worth tightening pre-merge (≤5-line change on the ternary condition); N1/N2 are test-hygiene follow-ups.

LGTM on the fix approach; holding C1 as a substantive follow-up worth Miguel's decision. Leaving as COMMENTED.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

🟠 [low-medium] Silent gate-disable when durationSeconds ≤ 0 OR mediaStart > durationSeconds. Number.isFinite(durationSeconds) accepts 0 and negatives; combined with Math.max(0, durationSeconds - mediaStart) on the next line, either path produces sourceFrames = 0, then line 156 collapses to expectedFrames = min(slotFrames, 0) = 0, and ratio = 1 (via the expectedFrames === 0 ? 1 on line 162). The assertVideoFrameCoverage filter (expectedFrames > 0 at line 186) then skips this clip entirely.

Concrete regression scenario: a source that ffprobe reads as durationSeconds=0 (partial download / malformed MP4) with 0 delivered frames — pre-PR tripped the gate at 0/slotFrames = 0% and aborted the render with a clear error. Post-PR it silently passes and ships a black clip. Same shape for <video ... data-media-start="5"> on a 3s source (authoring typo).

Suggested fix: tighten this line to entry.metadata.durationSeconds > 0 && entry.metadata.durationSeconds > video.mediaStart, or on line 156 fall back to slotFrames when sourceFrames === 0. Test coverage: add a case with durationSeconds: 0 (extractor probe failure) that expects the gate to still fire.

— Rames D Jusso

[james-russo-rames-d-jusso]

🟡 [nit] Test-fixture coupling — makeExtracted defaults durationSeconds = delivered/fps. This is why line 131 is now load-bearing on the pre-existing test: without it, makeExtracted("a", 5) at fps=30 sets durationSeconds = 5/30 = 0.167, the new code computes sourceFrames = ceil(0.167 * 30) = 5, giving expectedFrames = min(30, 5) = 5 and ratio = 5/5 = 1 — the test's own ratio ≈ 5/30 expectation would then fail.

Not a defect in this PR — the workaround is correct — but future tests that call makeExtracted to model "extraction failure" (delivered < expected) need to also override durationSeconds, or they'll silently report full coverage and mask the failure. Consider making makeExtracted take durationSeconds as an explicit parameter, or defaulting it to something that doesn't couple with delivered (e.g. videos[i].end - videos[i].start, or a fixed 10s).

— Rames D Jusso

[miguel-heygen]

Fixed at the root instead of leaving it as a footgun: makeExtracted now takes durationSeconds as an explicit param (default Infinity, decoupled from delivered) rather than deriving it from delivered / fps. That also let me drop the two post-hoc .metadata.durationSeconds = ... mutations that existed only to work around the coupling. Pushed in 7382ef7.

[james-russo-rames-d-jusso]

🟡 [nit] No test exercises mediaStart > 0. The formula on line 152 (durationSeconds - video.mediaStart) is only exercised at mediaStart = 0 in all three new tests (the default from makeVideo). A sibling test with, say, a 5s source, mediaStart=1, 10s non-looping slot would confirm sourceFrames = ceil((5 - 1) * 30) = 120 (not 150) — that path is otherwise dark.

— Rames D Jusso

[james-russo-rames-d-jusso]

R2 delta review at HEAD deba85079ccbfd647435d17bb8e698d874ca603d. Verifying dispositions on the R1 concerns and spot-checking the new fail-closed guard.

R1 concern verdict

ID	Status	Notes
C1 silent gate-disable on durationSeconds ≤ 0 / mediaStart > durationSeconds	🟢 addressed	Clean fix — hasUsableSourceDuration = Number.isFinite(sourceDuration) && sourceDuration > 0 combined with the entry && !video.loop && hasUsableSourceDuration triple guard falls back to slotFrames on every invalid input (0 / negative / NaN / Infinity / mediaStart ≥ durationSeconds). Regression test "fails closed for invalid or exhausted source durations" covers both the zero-duration and exhausted-mediaStart shapes; both trip the gate (0% and 10% respectively).
N1 makeExtracted fixture coupling	🟡 unchanged	Not addressed — expected. makeExtracted default still couples durationSeconds = delivered/fps; future test authors modeling extraction-failure will still need to override. Follow-up material.
N2 mediaStart > 0 non-exhausted coverage	🟡 partial	The new test exercises mediaStart: 4, but in the exhausted branch (mediaStart > durationSeconds) that falls back to slotFrames. The "trim front, source still usable" path (e.g. durationSeconds=5, mediaStart=1 → sourceFrames=120) is still dark. Follow-up material.

Delta trace (verified path-by-path)

• sourceDuration = entry ? entry.metadata.durationSeconds - video.mediaStart : NaN — the NaN sentinel on missing-entry keeps the fallback semantics clean; the R1 code implicitly relied on the outer entry && ... guard for the same effect.
• Number.isFinite(sourceDuration) && sourceDuration > 0 — strict-positive check, so sourceDuration === 0 (exact-exhausted case, mediaStart === durationSeconds) also falls back. Matches the "no usable source" intent.
• The entry && !video.loop outer guard on expectedFrames unchanged, preserving the missing-entry (entry === undefined) full-slot behavior tested at computeVideoFrameCoverage.test.ts:111-123.
• No Math.max(0, ...) needed anymore — the > 0 guard handles negatives earlier, and expectedFramesForClip's internal Math.max(0, end - start) handles the positive-domain safely.

Green surface (verified)

• Backward compatibility on the happy path — normal (durationSeconds > 0, mediaStart < durationSeconds, non-loop) case emits same expectedFrames as R1. Traced against the 3 R1 tests (held, loop, truncated) — all resolve identically.
• 22 focused tests reported passing (up from 21); the +1 is the new fail-closed case with two clips (zero + trimmed-away), both tripping the 0.95 gate.
• Diff is small — +12/-1 in the source, +41/-0 in the test file. Net additive.

Merge gate

Clean C1 disposition with the exact guard shape suggested and an appropriate regression test. LGTM from my side — leaving as COMMENTED.

— Review by Rames D Jusso

[vanceingalls]

R2 delta verification at HEAD deba8507

R1 COMMENTED-RIGHT at 4e2bc720 (review 4719132557) flagged one non-blocking nit: mediaStart >= durationSeconds (or durationSeconds ≤ 0) silently produced sourceFrames = 0 → expectedFrames = 0, tripping assertVideoFrameCoverage's existing expectedFrames > 0 skip. Miguel's R2 delta closes that with a precise fail-closed guard plus regression coverage. Rebase-clean; my R1 was auto-dismissed under require_last_push_approval as expected. Verdict: RIGHT — APPROVE.

Fail-closed guard — correct

videoFrameCoverage.ts:143-155:

const sourceDuration = entry ? entry.metadata.durationSeconds - video.mediaStart : NaN;
const hasUsableSourceDuration = Number.isFinite(sourceDuration) && sourceDuration > 0;
const sourceFrames =
  entry && !video.loop && hasUsableSourceDuration
    ? expectedFramesForClip(0, sourceDuration, fps)
    : slotFrames;
const expectedFrames = entry && !video.loop ? Math.min(slotFrames, sourceFrames) : slotFrames;

Adversarial trace on invalid inputs — all fall back to slotFrames, so the gate fires normally rather than silently skipping:

• mediaStart >= durationSeconds (R1 nit): sourceDuration = 3 - 5 = -2 → hasUsable = false → sourceFrames = slotFrames = 300 → expectedFrames = min(300, 300) = 300. Captured partial → ratio < 0.95 → fires ✅
• durationSeconds = 0: sourceDuration = 0 → 0 > 0 false → fallback → fires ✅
• durationSeconds = NaN: isFinite(NaN) false → fallback → fires ✅
• durationSeconds = Infinity: isFinite false → fallback → fires ✅

Four-case matrix — preserved from R1

Re-traced against the new code:

• Held tail (durationSeconds=3, slot=10s, loop=false): sourceDuration=3 > 0 → sourceFrames = 90 → expectedFrames = min(300, 90) = 90, captured=90 → ratio=1 ✅
• Loop (loop=true): outer !video.loop = false → expectedFrames = slotFrames = 300, captured=90 → ratio=0.3, gate fires ✅
• Missing extraction (entry === undefined): outer entry && ... = false → expectedFrames = slotFrames, ratio=0, gate fires ✅
• Truncation-below-source (durationSeconds=3, captured=60): expectedFrames = 90, ratio=0.667 < 0.95 → throws ✅

Test coverage — locks the axis

New "fails closed for invalid or exhausted source durations" (videoFrameCoverage.test.ts:167-178) covers BOTH shapes:

• zero: durationSeconds=0, extracted=0 → expectedFrames=300, captured=0.
• trimmed-away: mediaStart=4, durationSeconds=3 (exhausted play-offset), extracted=30 → expectedFrames=300, captured=30.

Both assert expectedFrames: 300 (i.e. the slotFrames fallback took effect, not the silent 0 skip), and the combined assertVideoFrameCoverage(reports, 0.95)).toThrow(VideoFrameCoverageError) locks fail-loud. Value-side + reachability-side both held.

Pre-existing partial-extraction test at test.ts:131 picks up durationSeconds = 1 this round — routes cleanly through the new sourceFrames branch (min(30, 30) = 30), preserving prior expectation. No fixture drift.

No fresh gap

• entry && !video.loop outer guard on the final ternary keeps missing-extraction fallback intact — the NaN sentinel in sourceDuration is defense-in-depth, not the primary defense.
• Math.max(0, ...) no longer needed — the > 0 guard handles negatives earlier; expectedFramesForClip's internal Math.max(0, end - start) still guards the positive branch.
• Threshold constants (resolveVideoCoverageThreshold, HF_VIDEO_COVERAGE_THRESHOLD, 0-disable, clamp) untouched.
• Report shape (expectedFrames, capturedFrames, ratio) unchanged — summarizeExtractionObservability and renderOrchestrator unaffected.
• Diff is +53/-1 across the two intended files; nothing else touched.

Rames posted a parallel R2 at the same head reaching the same C1-addressed disposition (N1 fixture coupling + N2 non-exhausted mediaStart > 0 path both follow-up material) — corroborates.

CI at deba8507

Green on completed required lanes (Format, Lint, Fallow audit, Producer unit, SDK, Test: runtime contract, Studio load smoke, player-perf, preview-regression, Preflight, File size). Still IN_PROGRESS at post time: Typecheck, Test, Producer: integration, CLI smoke, Build, regression shards 1-8, Windows render/tests, Analyze (javascript-typescript). No red anywhere. mergeStateStatus = BLOCKED reflects in-flight lanes, not a policy fail.

Recommendation

APPROVE. R1 nit precisely addressed with the minimal correct guard shape (Number.isFinite && > 0 catches every invalid input class), regression test locks both value-side (expectedFrames = 300 on invalid inputs) and reachability-side (assertVideoFrameCoverage throws), and the four-case correctness matrix from R1 verified unchanged. No accidental invariant expansion.

— Via