fix(skills): harden music-to-video Step 3 gate, align product-launch-video with pr-to-video#1672
Conversation
A bare Step-2 skeleton (frames with `### Groups: TBD (Step 3)`) satisfied
every hard check in validate-plan.mjs and exited 0, so the Step-3 gate
("validate-plan exits 0") was passable without ever filling the plan — an
orchestrator could go straight from skeleton to building frames.
- validate-plan.mjs: a frame with no filled group is now a HARD error
(exit 1), not a warning. A skeleton can no longer clear the Step-3 gate.
- frame-worker.md: add a one-line hard stop — a TBD/empty `### Groups`
means Step 3 was skipped; report back and write nothing, never improvise.
- SKILL.md: lengthen Step 2/3/4 titles so the plan reads as a required gate
(structure-only skeleton -> fill the plan -> build from the plan).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…hecks Port the fixes already landed in pr-to-video: - frame-worker no longer told to run the hyperframes CLI — the frame isn't assembled yet, so lint/validate/inspect report on other files (a false green); the self-check is now "re-read against the contract" and the orchestrator runs the CLI at Step 6 after assembly. - add the `#root` root-styling rule (`subcomposition_root_styled_by_class`): a class on the composition-id element scopes to a non-matching descendant selector, so the whole scene renders unstyled. - drop `--strict-layout` from the orchestrator's `inspect` step. - document the caption `text_box_overflow` false-positive (~1-4px on `#caption-word-*`) so it isn't chased. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jrusso1020
left a comment
There was a problem hiding this comment.
Read all 5 files end-to-end. Posting as COMMENT per the customer-partner-PR discipline; stamp eligibility routes through James.
What I verified
music-to-video Step 3 hard gate (validate-plan.mjs):
The fix is doubly load-bearing — both the warn→error severity flip AND a subtle empty-title bug closure:
// pre-fix
if (![...framesWithGroups].some((s) => s.includes(String(f.title ?? "")))) {
warns.push(...);
}
// post-fix
const title = String(f.title ?? "").trim();
const hasGroup = title !== "" && [...framesWithGroups].some((s) => s.includes(title));
if (!hasGroup) {
errors.push(...);
}The pre-fix code had a sleeper false-positive: an empty/undefined f.title produced String(...).trim() === "", and s.includes("") is always true for any string. So a frame with an empty title would falsely pass the check (hasGroup = true) — masking the missing-groups case the rule is meant to catch. The new title !== "" guard closes that hole as well as turning the warn into a hard error. ✓ Both load-bearing parts of the fix are present.
Error message names the fix path: "Run Step 3: fill its groups (`- gN — template|free_design|asset …`) before building." — calls out the exact next-step + the syntax pattern. Good DX. ✓
music-to-video frame-worker hard stop: one-line "No plan = stop" instruction in the prelude. Matches the validate-plan.mjs gate hardening at the orchestrator side. ✓
music-to-video SKILL.md step titles: Step 2/3/4 titles now spell out the sequence ("Frame skeleton (structure only)" / "Fill the plan (user-gated)" / "Build frames from the plan"). Cosmetic but useful — makes the dependency visible in the section headings.
product-launch-video — alignment with pr-to-video:
- frame-worker
OUTPUTline: now lists "run the CLI" in the terminal-action exclusion list. ✓ - Self-check section reframed: title changes from "Self-check (fix before finishing)" to "Self-check before finishing (you do NOT run the CLI)", with the why spelled out in the body — "they operate on the assembled project... your frame isn't wired in yet... they report on other files (a false green)." The reframe is the right corrective: it tells the worker WHY the CLI is wrong, not just that it is. ✓
subcomposition_root_styled_by_classrule reference added to the self-check checklist, with the failure-mode explanation: "at render a class on the root gets scoped to a descendant selector that can't match it, so the whole scene renders unstyled (Studio preview still looks right — trust this rule, not the preview)." This matches the rule shipped in HF#1665. ✓- Step 3 of orchestration: adds the
#rootcallout inline. ✓ SKILL.mdStep 6: drops--strict-layoutfrominspect. ✓text_box_overflowcaption false-positive documented: ~1-4px ink-spill on#caption-word-*due to deliberately snug line-height inscripts/captions.mjs+ nooverflow:hidden. Names the selector pattern so it can't be confused with a frame-element overflow (which IS still actionable). Tells the orchestrator not to inflate line-height (cascading worse) and not to re-dispatch a frame for it. Good defensive doc. ✓
Body claims verified against the diff
- "5 files,
.md/.mjsonly — no package code" — confirmed via file list. - "Two independent skill-authoring robustness fixes" — verified: music-to-video changes touch only
music-to-video/*paths; product-launch-video changes touch onlyproduct-launch-video/*paths. The bundling is the right grain (both are skill-authoring robustness drifts) per the body's framing. - "pr-to-video already fixed all of this; product-launch-video had drifted" — implicit cross-reference, takes the body at its word. The shape of the changes matches what a port-from-pr-to-video would look like.
CI
Re-pulled at post-time: 13 SUCCESS / 1 IN_PROGRESS / 21 SKIPPED / 0 FAILURE. Most checks correctly skipping since this PR is .md / .mjs only — no package source. Mergeable from a CI perspective.
Stamp posture
Per team discipline on customer-partner PRs, stamp eligibility routes through <@U08E7PV788Z>. From my read: clean, surgical, both fixes are real (the music-to-video one closes both a severity-flip and a subtle empty-title false-positive in one go), and the product-launch-video port-from-pr-to-video shape is sound. Same hand-back routing as the recent batch.
Review by Jerrai
miga-heygen
left a comment
There was a problem hiding this comment.
Review — harden music-to-video Step 3 gate, align product-launch-video with pr-to-video
5 files, +26/−13 — All .md/.mjs, no package code. Two independent fixes bundled cleanly.
music-to-video: validate-plan.mjs hardening
Fully agree with Rames' assessment that this is doubly load-bearing:
-
Severity flip (warn → error): Step-2 skeletons with
TBD/empty groups can no longer sneak past the gate. The process now exits 1 instead of 0 with a warning. Essential. -
Empty-title false-positive closure: The old
s.includes(String(f.title ?? ""))always returnedtruefor empty/missing titles because"anything".includes("")is universallytruein JS. The newtitle !== ""guard closes this hole. Without it, the severity flip alone would be incomplete — empty-titled frames would still pass.
Error message is actionable ("Run Step 3: fill its groups...") and names the exact syntax pattern. The frame-worker hard-stop instruction and SKILL.md step-title clarifications are good companions.
product-launch-video: pr-to-video port
Verified against pr-to-video's frame-worker on main — all three changes are faithful ports:
- CLI exclusion from terminal actions ✓
subcomposition_root_styled_by_classguidance ✓text_box_overflowcaption false-positive documentation ✓ (well-written — explains the trigger, the selector pattern to distinguish real overflows, and what NOT to do)
--strict-layout removal from Step 6 inspect is clean.
Minor note (non-blocking)
Title-based group matching is still substring-based (s.includes(title)), so frames with titles where one is a substring of another (e.g. "Hero" vs "Hero Reveal") could cross-match. Pre-existing, not introduced here — and this PR makes it less dangerous by closing the degenerate empty case.
No issues found. LGTM.
— Miga
miguel-heygen
left a comment
There was a problem hiding this comment.
Verified current head f4aeff6faf: all non-skipped checks are green, mergeable, not draft. I also ran the load-bearing validate-plan.mjs behavior on the PR head: Step-2 skeleton exits 1, filled plan exits 0, and the empty-title includes("") case exits 1. oxfmt --check passed on all 5 changed files.
Verdict: APPROVE
Reasoning: The Step 3 gate is now enforced at the validator and worker-instruction layers, and the product-launch-video guidance is a faithful port of the already-accepted pr-to-video behavior. No blockers found.
— Magi
What
Two independent skill-authoring robustness fixes, each closing a path where the
orchestrator or a frame-worker could silently take a degraded route. Both skills already
shipped (music-to-video in #1665, product-launch-video in #1635); this tightens their
author flow. 5 files,
.md/.mjsonly — no package code.fix(music-to-video)make Step 3 plan a hard gatefix(product-launch-video)stop frame-worker CLI runs, relax layout checkspr-to-videoBranches off current
main.Why
### Groupsis stillTBD (Step 3)) passed every hard check invalidate-plan.mjsand exited 0. Since theStep-3 gate is "validate-plan exits 0", the plan could be approved and frames built
without the plan ever being filled — straight from skeleton to build.
yourself first", but a frame isn't wired into
index.htmlwhen the worker runs, so theCLI reports on other files (a false green) while the worker's terminal action is just
writing the file. Separately, Step 6's
inspect --strict-layoutover-policed layout andflagged a known ~1–4px caption false-positive.
pr-to-videoalready fixed all of this;product-launch-video had drifted.
How
music-to-video — Step 3 is now a real gate (
bc9c8dab)validate-plan.mjs— a frame with no filled group is now a HARD error (exit 1),not a warning. A
TBD/empty### Groupsis a Step-2 skeleton, not an approved plan, soit can't clear the gate.
frame-worker.md— one-line hard stop: TBD/empty### Groupsmeans Step 3 wasskipped → report back and write nothing, never improvise groups/templates/copy.
SKILL.md— Step 2/3/4 titles spell out the sequence (structure-only skeleton → fillthe plan → build from the plan) so the plan reads as required.
product-launch-video — align with pr-to-video (
f4aeff6f)against the contract" with the why (unassembled frame → false green; the orchestrator
runs lint/validate/inspect at Step 6 after assembly), and "run the CLI" is added to the
terminal-action exclusion list.
#rootroot-styling rule (subcomposition_root_styled_by_class) — a class on thedata-composition-idelement scopes to a non-matching descendant selector, so the wholescene renders unstyled; style the root via
#root.--strict-layoutfrom Step 6'sinspect.text_box_overflowfalse-positive (~1–4px on#caption-word-*)so it isn't chased.
Test plan
Skill content is
.md/.mjsonly — no package code changed.oxfmt --checkclean on all 5 changed files.mainconflict-free.validate-plan.mjs(exit 1) instead of building from a skeleton.
inspect(no--strict-layout) passes apart from the documented caption false-positive.Notes
fixes" branch. Happy to split if you'd rather review them separately.
pr-to-video'sframe-worker; product-launch-specific phrasing (cutout/background roles,
[video]clips)is kept intact.
🤖 Generated with Claude Code