fix(skills): extend transition roots without explicit duration

[miguel-heygen]

What

Allow transition injection to extend an outgoing composition root even when the root does not already declare data-duration.

Why

The documented frame-root contract permits a composition root without a static duration, but the transition injector checked data-duration before confirming the matching root. Valid generated frames could therefore fail with a misleading missing-root diagnostic.

How

• Recognize the outgoing composition root before requiring a duration attribute.
• Compute and inject the transition-tail duration when the root omits it.
• Preserve existing timed-descendant and audio extension behavior.

Test plan

• RED: focused regression exited 1 with the misleading missing-root diagnostic.
• GREEN: node --test skills/faceless-explainer/scripts/transitions.test.mjs — 1 passed.
• node --test skills/faceless-explainer/scripts/*.test.mjs — 35 passed.
• bunx oxfmt --check skills/faceless-explainer/scripts/transitions.mjs skills/faceless-explainer/scripts/transitions.test.mjs.
• bunx oxlint skills/faceless-explainer/scripts/transitions.mjs skills/faceless-explainer/scripts/transitions.test.mjs.
• git diff --check.

[vanceingalls]

R1 review @ 4c65dfd

Verdict: APPROVE — targeted fix for APPS-977 + APPS-985. Composition-root detection is correctly reordered to run before the data-duration gate; injection path is regex-safe; both roots-with-duration (old) and roots-without-duration (new) end at the same shape; the missing-root die() invariant is preserved.
Grade: A Rubric: CORRECT

Approach. skills/faceless-explainer/scripts/transitions.mjs::extendFrameTail line 119-126: swaps the order so compositionMatch is checked first, so a root tag that lacks data-duration is still recognized as the root. When it is the root, the ternary picks between (a) in-place attribute replace (existing behavior for roots WITH data-duration) and (b) end-of-tag injection via tag.replace(/(\s*\/?>)$/, ' data-duration="…"$1') for roots WITHOUT it. The old !durationMatch early-return still gates descendant clip rewriting so non-root elements without duration are untouched.

Findings.

• P1: none.
• P2: none.
• Non-blocker (test symmetry): the new test only covers the NEW path (root omits data-duration). The OLD path (root with data-duration) is exercised only by the pre-existing suite ("35 passed" per body). A one-line assertion adding a variant fixture with data-duration="4" on the root — same expected output — would lock in the ternary's behavior-preserving claim explicitly so a future refactor of the branch can't silently regress it.
• Non-blocker (regex-anchoring nit): assert.match(outgoing, /data-composition-id="01-a" data-duration="2.5"/) requires the attributes to be adjacent. Injection appends at end of tag, so with the current fixture (<div id="root" data-composition-id="01-a">) the test passes. If a future author adds any other attribute after data-composition-id to the fixture, the assertion breaks even though production behavior is correct. Prefer /data-composition-id="01-a"[^>]*data-duration="2.5"/.

Correctness — end-of-tag regex.
/(\s*\/?>)$/ matches:

• > → replaces with data-duration="…"> ✓
• /> (self-closing) → replaces with data-duration="…"/> ✓
• > (trailing whitespace) → replaces with data-duration="…" > ✓
• attribute values containing > are already excluded by the outer [^>]* in the enclosing regex, so the tag boundary was fixed before the inner replace fires — pre-existing limitation, not introduced here.

Correctness — composition-root reordering. For the root WITH data-duration: (old) durationMatch → compositionMatch → replace vs (new) compositionMatch → ternary picks replace branch. Same output. For the root WITHOUT data-duration: (old) early-returned with foundRoot=false, which then tripped the die("no root") invariant → misleading diagnostic; (new) enters the compositionMatch branch, sets foundRoot=true, and injects. Correct.

Sibling / parity. Grep confirms extendFrameTail is the only extend-* helper. Line 283 (data-composition-id="main"[^>]*?data-duration="…" in index.html) is a different code path — it reads the pre-authored root duration from the top-level file, doesn't extend it — and the index.html root is contract-guaranteed to declare data-duration. Not the same bug. The foundRoot invariant (die at line 142) is preserved: injection sets it, and a legitimately missing composition-id root still fails loudly.

Test coverage. New transitions.test.mjs builds a real fixture project (STORYBOARD.md + index.html + two frame HTML files where the root omits data-duration), runs the actual transitions.mjs inject CLI via spawnSync(process.execPath, …), and asserts both (a) the root gets data-duration="2.5" injected and (b) the descendant class="clip" also gets extended to 2.5. Integration-shaped, exercises the exact bug, self-cleans via t.after(rmSync). Good.

Skills-manifest hash. faceless-explainer hash bumps 72acdcb3… → 261a9740… and files count 23→24 (new test file). Consistent.

CI. Detect-changes across all workflows correctly routed to no-op (skills-only diff); CI Format ✓, semantic PR title ✓, file-size ✓, player-perf ✓, preview-regression ✓, regression ✓, CodeQL actions/python ✓ (javascript-typescript in progress at time of review — non-blocking, expected to pass). No blocking checks.

Verdict rationale. Fix cleanly implements the documented frame-root contract Miguel cites (root without data-duration is legal). Reorder is behavior-preserving on the pre-existing case, actively repairs the reported case, and preserves the die() invariant that keeps the missing-root error path meaningful. Non-blockers are minor test-hygiene nits.

— Via

[james-russo-rames-d-jusso]

Reviewed at 4c65dfd0.

The fix is genuine root-cause: the predicate is reordered so the composition root is identified before data-duration is required, then injected when absent. Both Linear tickets (APPS-977 frame-worker roots + APPS-985 injector rejection) collapse to the same underlying failure and are covered by this one reorder. Regression test at skills/faceless-explainer/scripts/transitions.test.mjs exercises the exact APPS-985 shape (root omitting data-duration) end-to-end via the CLI and asserts non-zero-exit is gone. Clean, small, well-scoped.

Concerns

• Sibling skills carry the identical pre-fix bug — skills/pr-to-video/scripts/transitions.mjs:101-145 and skills/product-launch-video/scripts/transitions.mjs:101-145 contain extendFrameTail that is byte-identical to the pre-fix faceless-explainer version (verified: sha256(pr-to-video/...transitions.mjs) == sha256(product-launch-video/...transitions.mjs)). Both hit the same if (!durationMatch) return tag; before the composition-root check and would die("has no data-composition-id=... root") on frame-worker outputs that emit a root without data-duration. Since APPS-977 is framed as a general "frame-worker roots" problem, and any of these three skills could ingest frame-worker output, the fix likely wants to propagate — or the PR title should narrow to fix(skills/faceless-explainer) so the remaining exposure is explicit. Note there's no CI enforcement of cross-skill file identity here (the mirror lane passed cleanly even with the divergence), so this won't get caught automatically.

Nits

• The regression test asserts result.status === 0 and inspects the outgoing frame, but doesn't verify the incoming frame (02-b.html) was left untouched. The invariant "extend outgoing only, never touch incoming" is core to the docstring at lines 13-18; a one-line assert.doesNotMatch(readFileSync('.../02-b.html','utf8'), /data-duration/) (or asserts the incoming still carries only its authored duration) would nail that down.

Questions

• Was the scope-narrowing to just faceless-explainer intentional (e.g. because pr-to-video and product-launch-video frame-workers currently always emit data-duration and the failure has never fired in prod there), or is the plan to follow this up with the same reorder in the sibling skills? Happy to defer if the answer is "known, tracked separately."

What I didn't verify

• I didn't trace whether skills/pr-to-video/ or skills/product-launch-video/ currently have frame-worker prompts that could emit a root without data-duration. The exposure could be latent-but-unreachable rather than latent-and-firing, depending on their frame-worker contracts.
• I didn't reproduce the RED case locally — I trusted the PR's node --test output ("35 passed") and the CI Test: skills job's SUCCESS.

— Review by Rames D Jusso