fix(cli): align video output boundaries

[miguel-heygen]

Summary

• scaffold init --video compositions from the video stream duration instead of the longer container/audio duration
• treat authored clip windows as half-open intervals so a clip is hidden and inactive at its exact end timestamp
• cover both behaviors with CLI and runtime regression tests

Tests

• bun run --cwd packages/cli test -- src/commands/init.test.ts
• bun run --cwd packages/core test -- src/runtime/init.test.ts src/runtime/media.test.ts
• bun run typecheck in packages/cli and packages/core
• focused oxfmt --check and oxlint on changed files

[james-russo-rames-d-jusso]

Reviewed at 450a357a. Refetched after Magi's test-harness restack from 7e299073 — init.test.ts now honors CI's FFMPEG_BIN env fixture; product-code behavior unchanged since the earlier head.

Blockers

(none)

Concerns

(none)

Nits

(none)

Green notes

🟢 probeVideo duration precedence — stream duration → nb_frames/fps → format duration → 5s fallback. videoStream.duration reflects only the video track's own timeline, so an audio-outlasts-video source no longer stretches the scaffolded composition past the last video frame. parseFloat("N/A") → NaN → Number.isFinite falls through cleanly to the next candidate, so containers that emit stream duration=N/A (some VFR muxes) still resolve via nb_frames / fps or format fallback.

🟢 Half-open interval [start, end) change is consistent across the three sites AND preexisting code.

• init.ts:1444 (element isActive guard): <= computedEnd → < computedEnd.
• init.ts:1936 (audio activation): <= end → < end.
• media.ts:167 (media clip isActive): <= clip.end → < clip.end.
• Preexisting init.ts:2001 (timeSeconds < start || timeSeconds >= end) is already half-open — this PR converges on that convention rather than fighting it.
• Post-PR grep confirms no <=\s*(computedEnd|end) remains under packages/core/src/runtime/.

🟢 Boundary tests pin exact-end behavior in both layers. runtime/init.test.ts seeks to 2.5 on a clip with duration=2.5 and asserts visibility === "hidden". runtime/media.test.ts seeks to 2.5 on a clip with end=2.5 and asserts play was NOT called. Both would fail on pre-PR closed-interval behavior.

🟢 Looping-clip edge preserved. For clip.loop=true with an infinite end (POSITIVE_INFINITY), Number.isFinite(computedEnd) is false and the guard short-circuits to true — no regression on the common infinite-loop case. Only the rare "loop with finite window" wraps at end - ε instead of end, which is the intended half-open behavior.

🟢 CI fixture fix (450a357a) — findFFmpeg() returns FFMPEG_BIN when the env var is set, so the fixture-video build in init.test.ts no longer misses the toolchain on the CI runner. Product-code path is untouched.

Verdict framing

Duration source picks the right stream, half-open convention applied consistently across the runtime, both boundaries pinned by focused tests. LGTM from my side.

— Review by Rames D Jusso

[vanceingalls]

Position: RIGHT thesis on both changes. Mitigation for the initially-failing test is in flight at HEAD — worth reviewing the fix pattern.

Freshness pull @ 450a357ab4b0b5eba9043848779c67b9be311c2c: mergeStateStatus=BLOCKED, mergeable=MERGEABLE, reviews=[]. Test = IN_PROGRESS (rerunning against the fixup commit); Typecheck, Build, Lint, Format, Producer: unit tests, Producer: integration tests, SDK: unit + contract + smoke, Preflight lanes = SUCCESS. Several regression-shards + Windows render + CLI smoke (required) still IN_PROGRESS. Not-CLEAN → 🟡 pending green from the still-running lanes; second look recommended once Test lands.

Rames-differentiation: no other reviews present at HEAD (reviews=[]) — this is a fresh R1 with no prior lens to differentiate from.

---

Context: the initial R1 blocker was the PR's own new test failing in CI

At the previous SHA 7e29907342bca6439d067d57f1132a9ff4652e9d, the Test job failed on the new regression test this PR introduces — not the "Puppeteer ECONNRESET" the PR body suggested:

FAIL src/commands/init.test.ts > uses the video stream duration when audio outlasts the final video frame
AssertionError: expected undefined to be defined
❯ src/commands/init.test.ts:187:22
      const ffmpeg = findFFmpeg();
      expect(ffmpeg).toBeDefined();

The fixup commit 450a357 (test(cli): honor CI ffmpeg fixture path) fell back to process.env.FFMPEG_BIN ?? findFFmpeg(), piggy-backing on the existing .github/actions/prepare-ffmpeg-bin composite action which does cp $(which ffmpeg) $RUNNER_TEMP/hf-ffmpeg. This works because ubuntu-latest GitHub runners ship with ffmpeg preinstalled and the Test job at ci.yml:230 already runs that action.

One residual concern with the fix pattern: prepare-ffmpeg-bin has an explicit fallback branch — when which ffmpeg returns empty, it writes a stub script (#!/bin/sh\nexec ffmpeg "$@"\n) and still exports FFMPEG_BIN pointing at that stub. If the runner image ever loses its preinstalled ffmpeg, expect(ffmpeg).toBeDefined() will still pass (env var is set), then execFileSync(ffmpeg, [...]) invokes the stub, which re-execs a missing ffmpeg binary and dies with a less-legible ENOENT rather than the current legible "expected undefined to be defined" error. Non-blocking but worth an execFileSync(ffmpeg, ["-version"], { stdio: "ignore" }) sanity probe before the test proceeds, or (better) a checked-in fixture MP4 that removes the runner-image dependency entirely.

---

✅ Verified — half-open dispatch chain is now internally consistent

Full audit of every currentTime | timeSeconds | state.currentTime vs end | clip.end | computedEnd comparison in both edited files at HEAD:

Site	Convention	Status
packages/core/src/runtime/init.ts:586 (isTimedElementVisibleAt)	currentTime < computedEnd	✅ changed by this PR
packages/core/src/runtime/init.ts:2816 (audio active detection)	state.currentTime < end	✅ changed by this PR
packages/core/src/runtime/init.ts:2891 (hardSyncAllMedia)	timeSeconds >= end → inactive	pre-existing half-open, now consistent
packages/core/src/runtime/media.ts:176 (syncRuntimeMedia)	params.timeSeconds < clip.end	✅ changed by this PR

No straggler sites in these files. The three edited comparisons now match the pre-existing convention at init.ts:2891 — so the full-dispatch-chain audit clears. Missing-duration case (computedEnd = POSITIVE_INFINITY) still evaluates to currentTime < Infinity → true, so the "no data-duration attribute" path is unchanged.

---

✅ Verified — CLI duration-selection fallback semantics preserved

packages/cli/src/commands/init.ts:129-139 chain streamDuration → frameCount/fps → formatDuration → 5. Fallback preserved when all sources are NaN (parseFloat("") === NaN, parseInt("", 10) === NaN, Number.isFinite(NaN) === false).

Minor edge: parseFloat("0") === 0 and Number.isFinite(0) === true, so a corrupt videoStream.duration === "0" will now short-circuit at 0 instead of falling through to formatDuration. Not blocking — pre-existing degenerate-ffprobe territory — but a defensive > 0 guard on each isFinite branch would harden it.

---

🟡 Weak — regression tests assert the boundary point only, not the neighborhood

The three new tests each pin behavior at time === end exactly. To catch a future off-by-epsilon or comparator-flip, they should assert the neighborhood too:

// packages/core/src/runtime/media.test.ts (analogous shape for the other two)
it.each([
  [2.499, /* active */ true],
  [2.5,   /* active */ false],
  [2.501, /* active */ false],
])("boundary transitions cleanly at t=%s", (t, expected) => {
  const clip = createMockClip({ start: 0, end: 2.5 });
  Object.defineProperty(clip.el, "readyState", { value: 4, writable: true });
  (clip.el.play as Mock).mockClear();
  syncRuntimeMedia({ clips: [clip], timeSeconds: t, playing: true, playbackRate: 1 });
  expect((clip.el.play as Mock).mock.calls.length > 0).toBe(expected);
});

Same shape for the initSandboxRuntimeModular visibility test (t=2.499 visible / t=2.5 hidden / t=2.501 hidden) and the CLI probe test (assert data-duration="1" under stream-duration path AND a second fixture under the frame-count fallback path, so the middle branch of the ternary isn't dead-covered). Miguel's parameterization directive lands here: a single-point test at the boundary point is what the previous <= implementation would also have passed if flipped incorrectly — the neighborhood assertions are what discriminate the half-open convention from a closed one.

Non-blocking on its own; worth folding into a follow-up.

---

Extrapolation-blocker checks

Case	Behavior at HEAD	Verdict
Single-frame clip (fps=30, duration=1/30)	Renders [0, 1/30); hidden at exact t=1/30.	✅ intended
Boundary exactly at frame boundary	Same as above.	✅
Boundary between frames (fractional)	Test at t=2.5 covers it.	✅
Multi-track selection (video vs audio duration mismatch)	CLI picks streamDuration (video-only).	✅ addressed by this PR
Overlapping clips	Not touched by this PR; pre-existing behavior.	out of scope
Clip past end of source	Media-decode territory; not touched.	out of scope
Very short clip (< 1 tick period)	Pre-existing render-tick flicker risk unchanged.	out of scope
Very long / multi-hour clip	Half-open convention scales; no change.	✅
duration <= 0 (degenerate)	computedEnd = POSITIVE_INFINITY → always active. Pre-existing.	out of scope

---

Envelope + sensitive-paths audit

• No Co-Authored-By: Claude / "🤖 Generated with [Claude Code]" trailer in commit body or PR body. Clean.
• No touch on share-URL construction, route handlers, apple-app-site-association, or redirect/routing. Sensitive Paths from CLAUDE.md → not implicated. No mobile-team coordination required.

---

Verdict: approve-with-comments once Test lands green, otherwise request-changes.

Positioning explicitly: the two theses (video-stream duration selection + half-open runtime clip boundaries) are RIGHT, the dispatch chain is clean, and the CI test failure was already correctly identified and mitigated by 450a357. What remains:

1. Neighborhood-parameterization of the three new regression tests (weak, non-blocking).
2. Optional: swap FFMPEG_BIN env-var pattern for a checked-in fixture MP4 to decouple the CLI test from GitHub runner-image contents (nit).
3. Optional: > 0 guard on each isFinite branch in probeVideo to handle degenerate "0" ffprobe output (nit).

Holding this review as COMMENT state — not stamping approve while Test is still IN_PROGRESS and multiple regression-shards haven't landed, but the code position is clear.

— Via

[miguel-heygen]

Current-head CI follow-up: replaced the ffmpeg-dependent duration-precedence integration fixture with a pure resolver regression, avoiding the CI FFMPEG_BIN postinstall stub while preserving the video-stream-over-container contract. Added coverage for zero/unusable stream duration fallback. Focused CLI init suite: 24/24; CLI typecheck, format, and lint pass.

[miguel-heygen]

Resolved the style-7 regression at the current head. The half-open boundary intentionally changes frame 90 (3.0s), so I regenerated that fixture baseline and added explicit end−ε / end / end+ε coverage for both timed-element visibility and media playback. Verification: core runtime tests 123/123; style-7-prod compilation, 100/100 visual checkpoints, stream parity, and audio correlation all passed; pre-commit lint/format/fallow/typecheck gates passed.

[james-russo-rames-d-jusso]

Re-verified at 491ed0235. Δ from 450a357a: probeVideo duration precedence extracted into resolveVideoDurationSeconds + tightened with a > 0 filter; runtime tests moved from single-point end-boundary to epsilon-perturbed half-open pins.

Blockers

(none)

Concerns

(none)

Nits

(none)

Green notes

🟢 resolveVideoDurationSeconds is a subtle correctness improvement on top of R1. R1's Number.isFinite(streamDuration) ? streamDuration : … accepted streamDuration === 0 (finite but useless — would seed a zero-length composition). R2's .find(d => Number.isFinite(d) && d > 0) rejects zero/negative and falls through to frameDuration → formatDuration. Directly pinned by falls through unusable stream durations before using the container duration (streamDuration=0, frameDuration=NaN → picks formatDuration=1.2). Nice pickup during the extraction.

🟢 ?? DEFAULT_META.durationSeconds instead of magic 5. Reads from the module's canonical default constant, so the fallback stays in sync if someone bumps DEFAULT_META.

🟢 Extracted helper is exported for direct testability. Both the positive case and the zero-stream fallthrough hit the helper directly rather than round-tripping through the ffprobe / spawn path — fast unit tests, no fixture required.

🟢 Runtime half-open tests tightened to true boundary pins. Both init.test.ts and media.test.ts now assert:

• t = end - 1e-9 → active / play (before end)
• t = end → inactive / no-play (at end)
• t = end + 1e-9 → inactive / no-play (after end)

That's a proper half-open [start, end) pin at the exact discontinuity, not just a single-point sample. Rename of the test titles to uses a half-open interval around … matches the new coverage shape.

🟢 CI fixture rebuilt (output/output.mp4 updated). No product-code drift caused it — just the corresponding fixture regenerating under the new FFMPEG_BIN-aware harness.

Verdict framing

Small helper extraction with a subtle correctness improvement pinned by a focused unit test; runtime pins are tighter. LGTM from my side.

— Review by Rames D Jusso

[vanceingalls]

✅ R2 verdict: APPROVE at 491ed0235. Held from approve at R1 pending nit follow-ups + CI health; both landed.

Nit disposition (from R1 at 450a357a)

• ✅ (a) neighborhood parameterization — pin half-open video boundaries (6fca3d5) rewrites both runtime tests (packages/core/src/runtime/{init,media}.test.ts) to assert t = end − 1e-9 (active), t = end (inactive), t = end + 1e-9 (inactive). That is a true half-open [start, end) pin at the discontinuity, not a single-point sample — exactly the fractional-boundary flush I flagged.
• ✅ (b) runner-image ffmpeg dependency — decouple duration precedence from ffmpeg (09cb2ef) deletes the execFileSync(ffmpeg, [...])-driven test and pins the same behavior via a pure-unit call into the extracted helper. prepare-ffmpeg-bin's stub-script fallback is now off the critical path for this assertion.
• ✅ (c) probeVideo degenerate-"0" — same commit extracts resolveVideoDurationSeconds({streamDuration, frameDuration, formatDuration}) with .find(d => Number.isFinite(d) && d > 0). Old code accepted streamDuration === 0 (finite-but-useless) and would have short-circuited on it; new code falls through zero/negative/non-finite alike, pinned by the added streamDuration: 0, frameDuration: NaN → formatDuration: 1.2 test. Fallback also moved from magic 5 to ?? DEFAULT_META.durationSeconds (verified still 5 at 491ed0235, so no drift; earns future-proofing).

Adversarial re-verify on the new mechanism

Per follow-up-upgraded-to-in-scope discipline, I re-ran an adversarial pass against resolveVideoDurationSeconds itself (not just the prior boundary thesis):

• All 3 durations non-finite / all zero / all negative → fallthrough to DEFAULT_META.durationSeconds. ✅
• streamDuration = Infinity → Number.isFinite rejects, falls through. ✅
• Priority order preserved (stream → frame → format), matching prior semantics. ✅
• Fallback default read from single source of truth. ✅

CI status (rollup at 491ed0235)

• 48 SUCCESS (including Test, Producer: unit tests, Producer: integration tests, Tests on windows-latest, CLI smoke (required), preview-regression, Fallow audit, Typecheck, all Perf lanes, CLI: npx shim × 3 OS, SDK: unit + contract + smoke, and regression-shards 1, 3, 4, 5, 6, 8).
• 2 SKIPPED (Test: skills, Skills: manifest in sync — expected).
• 2 IN_PROGRESS (regression-shards shard-2, regression-shards shard-7). No red anywhere.

Shard-8 flipped green since Magi's 2/7/8-pending snapshot, and 6 of 8 shards have already landed clean on this branch — the trajectory on the remaining two reads as "still running the queue", not "shape trouble". mergeStateStatus is BLOCKED on reviewDecision, not on CI.

Green notes

• Fixture regeneration (packages/producer/tests/style-7-prod/output/output.mp4, +2/-2) is header/metadata drift from re-encoding under the FFMPEG_BIN-aware harness, not a product-behavior change — matches the commit message and the golden-refresh intent.
• Rames independently arrived at LGTM-shape at the same head via a different lens; that peer alignment is a soft corroborator, not a substitute.

— Review by Via