fix(skills): preserve caption skin contrast states

[miguel-heygen]

What

• keep caption word-state CSS owned by the selected caption skin in all three shared caption builders
• preserve byte-identical caption builders and mirrored behavior tests across product-launch, faceless-explainer, and pr-to-video
• verify all 13 caption skins retain their active/spoken rules on both light and dark caption canvases
• clarify root-relative CSS asset paths and frame-prefixed selectors for parallel product-launch workers

Why

The removed heuristic keyed on --cap-canvas luminance, but that token is not necessarily the caption pill background: skins such as broadside use --cap-ink for the pill. Appending a generic dark-canvas override therefore targeted the wrong signal and could replace the skin-owned state-color contract. Each skin already owns its active/spoken styling through semantic tokens, so the shared builder now preserves those rules unchanged.

Verification

• bun run test:skills — 388 tests passed
• bun run lint:skills
• bun packages/cli/scripts/gen-skills-manifest.ts --check
• byte identity verified for all three captions.mjs and captions.test.mjs copies
• git diff --check

[vanceingalls]

⚠️ COMMENTED · position: RIGHT direction, CLOSED-out on landing. The premise (skin owns caption word-state colors) is defensible — the removed heuristic keyed on the wrong signal (--cap-canvas luminance is not the pill background for skins like broadside where background: var(--cap-ink)), and every one of the 13 skins under skills/hyperframes-creative/frame-presets/*/caption-skin.html already drives its .caption-word.is-active / .is-spoken colors through --cap-canvas / --cap-ink / --cap-accent with color-mix, so token-driven dark-canvas resolution is already the skin's job. Two P1 gaps below.

Findings (against head 5694557)

P1 · Cross-skill drift. The same 15-line dark-ground override still exists in the sibling skills at head:

• skills/faceless-explainer/scripts/captions.mjs L279–292
• skills/pr-to-video/scripts/captions.mjs L279–292

The three product-video skills share tokens.mjs and are cross-verified in scripts/lib/tokens.test.mjs (which imports brandRolesFromStats from both siblings and asserts alignment). Nothing in scripts/lint-skills.ts grep-checks the captions.mjs pair, so this drift won't fail CI. If the design conclusion is "skin owns state colors," mirror the removal in both siblings — either stacked in this PR or as a follow-up in the same batch. As-is, the three skills disagree about who owns caption state contrast.

P1 · Regression test is a source-scan, not a behavior test. captions.test.mjs:

assert.doesNotMatch(source, /caption-word\.is-(?:active|spoken)/);

This asserts two literal selector substrings are absent from captions.mjs. It would also pass if buildFromSkin's comment-strip / fillOnce transforms silently stripped the skin's own .caption-word.is-active rule while the source string test coincidentally still passed — the test's success is orthogonal to the actual invariant ("the generated composition preserves the SKIN's word-state selectors"). The scope is broader than one skin: parameterize across the 13 frame-presets/*/caption-skin.html fixtures at BOTH a light and a dark --cap-canvas value, then either (a) assert the skin's .caption-word.is-active block survives buildFromSkin intact, or (b) resolve the token values and assert computed active/spoken contrast ≥ 4.5:1. As-written the test is aspirational — it can never fail on a real behavior regression because it doesn't observe behavior.

P2 · PR body asserts the "contrast-safe" premise without showing it. The removed comment claimed the OPPOSITE premise ("skins tuned for a LIGHT pill (cream)… on a dark brand ground the pill goes near-black, the skin's faint upcoming-word mix and light highlight block turn unreadable"). The actual rationale for removal — that the override triggered on --cap-canvas luminance but skins like broadside use --cap-ink as pill bg, so the override was mis-targeted for those skins — is the right story and belongs in the PR body / commit body so the next agent doesn't restore the old block. As-is, both the deleted-code comment and the new PR text can't both be true; readers get to guess which won.

nit · Scope drift. frame-worker.md addenda (root-relative asset paths + frame-id-prefixed selectors) are unrelated to caption contrast. Reviewable in isolation, no objection to the content, but they belong in a separate commit or PR next time — the caption-skin fix and the parallel-worker doc updates want independent audit trails.

Envelope audit. No Co-Authored-By: Claude / 🤖 Generated with [Claude Code] in commit message body or PR body. Clean.

CI / mergeability. All checks green. mergeStateStatus = BLOCKED on review; mergeable = MERGEABLE.

— Via

[james-russo-rames-d-jusso]

Reviewed at b4c986db. Refetched after Magi's skills-manifest.json regen restack from 56945570 — manifest is in sync; the source change I flag below is unchanged from the earlier head.

Blockers

🔴 Byte-identity break: shared caption builder now diverges across the three skills. At origin/main all three captions.mjs copies are byte-identical (md5 f194c51e97900f7cf713c5e8f83b6dcf), and this PR deletes the dark-ground contrast override + its lum import from skills/product-launch-video/scripts/captions.mjs only:

• skills/faceless-explainer/scripts/captions.mjs — md5 f194c51e…, still emits the override.
• skills/pr-to-video/scripts/captions.mjs — md5 f194c51e…, still emits the override.
• skills/product-launch-video/scripts/captions.mjs — md5 2884464d…, fixed here.

The PR-body rationale ("the generated supplementary style could silently override a caption skin's contrast-safe active colors and create WCAG failures") applies identically to all three skills — same code, same call sites. Landing the fix in one of three copies leaves two skills continuing to emit the exact WCAG regression the PR is trying to eliminate, and future drift between the three copies becomes harder to reason about.

Two ways out (either is fine):

1. Apply the same deletion to the other two captions.mjs. Restores byte-identity — they'll share a new md5 that diverges from origin/main together. Also mirror captions.test.mjs to the sibling skill directories so the regression guard travels with the file it protects.
2. State explicitly which skills are impacted and why the other two are intentionally left alone. If (say) faceless-explainer and pr-to-video genuinely don't ship any dark-caption preset in production, the delta is defensible — but that needs an in-code comment or PR-body note so the next captions edit knows not to re-unify.

This is the same shape as #2484 (CJK grouping change) and #2485 (nested asset paths), where the fix DID land byte-identically across the three copies. No CI enforces the invariant, so it's easy to miss.

Concerns

(none)

Nits

🟡 Test placement asymmetry. captions.test.mjs is added only under skills/product-launch-video/scripts/; faceless-explainer and pr-to-video don't get a copy. Same shape as HF#2484 — worth mirroring while you're in the file so drift in the other two skills' copies would trip a test.

🟡 Test asserts on source text, not on emitted output. assert.doesNotMatch(source, /caption-word\.is-(?:active|spoken)/) pins the fix by the CODE PATH being deleted, not by the behavior. Stable and cheap, but if a future edit re-introduced a similar override under a different class name (say .caption-word.is-current), the guard would silently pass. If it's worth extending, a small tokens fixture + snapshot on the emitted <style> block count would give a shape-level pin. Not urgent.

Green notes

🟢 lum import cleanly removed. The only caller inside captions.mjs was the removed override; no other module imports lum from ./lib/tokens.mjs — safe delete.

🟢 frame-worker.md guidance additions (project-root-relative asset paths, frame_id-prefixed ids/classes) are worker-agent guidance and don't affect the runtime. Consistent with the shared-composition-worker parallelization pattern already codified in the same doc.

🟢 Manifest regen at b4c986db — skills-manifest.json is in sync after Magi's follow-up commit; the "Skills: manifest in sync" gate should clear on rerun.

What I didn't verify

• Whether any currently-shipping caption skin produces an unreadable active-state on a dark canvas without the removed override. If one exists, this PR moves the fix responsibility onto the skin author — reasonable, but nothing automated would catch a regression there.

— Review by Rames D Jusso

[vanceingalls]

⚠️ COMMENTED — R2 verification at head b4c986db · position holds: RIGHT direction, CLOSED-out on landing. Refetched after Magi's manifest-regen fixup (b4c986db = chore(skills): refresh caption contrast manifest, touches only skills-manifest.json +2 -2, empty body). None of the R1 findings were addressed by the fixup — all three sustain.

R1 disposition at b4c986db

P1a · Cross-skill drift — STILL STANDS (aligned with Rames' P1 blocker).
Fetched via /contents?ref=b4c986db:

• skills/faceless-explainer/scripts/captions.mjs — md5 f194c51e97900f7cf713c5e8f83b6dcf, override intact at L279–292 (comment + if (capCanvas && (lum(capCanvas) ?? 255) < 90) block emitting .caption-word.is-active / .caption-word.is-spoken).
• skills/pr-to-video/scripts/captions.mjs — same md5, byte-identical override.
• skills/product-launch-video/scripts/captions.mjs — md5 2884464dd4f823f86beddaaf145b007f, override removed.

Byte-identity across the three shared copies is broken (matches origin/main md5 f194c51e… for two, diverges on the fixed one). @james-russo-rames-d-jusso raised this at the same head with the md5s explicitly — I concur with his blocker severity on this one; the three product-video skills share tokens.mjs, are cross-verified in scripts/lib/tokens.test.mjs, and no CI grep-check catches captions.mjs drift. Recommended remediation: mirror the deletion in the two siblings (restores byte-identity + mirrors the guard test into their scripts/ dirs).

P1b · Test is source-scan, not behavior — STILL STANDS.
skills/product-launch-video/scripts/captions.test.mjs at b4c986db is 9 lines, unchanged from R1:

const source = readFileSync(new URL("./captions.mjs", import.meta.url), "utf8");
test("preserves caption-skin word-state colors", () => {
  assert.doesNotMatch(source, /caption-word\.is-(?:active|spoken)/);
});

It does not import buildFromSkin, does not run against any frame-presets/*/caption-skin.html fixture, and does not observe emitted output — it grep-checks the source file. Passes for the wrong reason (source-string absence), fails for the wrong reason (any refactor that keeps the same runtime behavior under a different literal). Rames flagged the same shape as a 🟡 nit and pointed at HF#2484 as a precedent for tokens-fixture + snapshot on the emitted <style>. Same recommendation from me — parameterize over the 13 fixtures at light + dark --cap-canvas, either assert the skin's .is-active block survives buildFromSkin, or resolve tokens and assert computed contrast ≥ 4.5:1.

P2 · PR body rationale mismatch — STILL STANDS.
Pulled /pulls/2486: body still reads "The generated supplementary style could silently override a caption skin's contrast-safe active colors and create WCAG failures" — unchanged. 5694557 commit body is empty; b4c986db commit body is empty. The mechanistically-correct rationale (the override keyed on --cap-canvas luminance, but skins like broadside use --cap-ink as pill background — so the override was mis-targeted for those skins) still isn't captured anywhere in the merge history. Next agent editing captions.mjs won't have that context.

nit · frame-worker.md scope drift — STILL STANDS.
Files at head still include skills/product-launch-video/sub-agents/frame-worker.md (+2 −0), unchanged from R1. Reviewable on its own merits, but wants an independent audit trail from the caption-skin fix.

R2-novel findings

• Fixup scope clean. b4c986db touches only skills-manifest.json; no accidental collateral edits piggybacked on the regen. Skills: manifest in sync gate now green.
• CI green at b4c986db (all required checks SUCCESS; Tests on windows-latest still IN_PROGRESS but non-blocking).
• Envelope clean. Neither commit body nor PR body contains Co-Authored-By: Claude or 🤖 Generated with [Claude Code].
• Position vs Rames. Rames requests remediation on P1a as a blocker with md5 receipts; I hold ⚠️ COMMENTED (findings same, block responsibility already carried by his review). If Miguel takes P1a as a follow-up rather than in-PR, the fix should ship stacked in the same batch since the invariant lives across all three captions.mjs.

— Via

[miguel-heygen]

Cross-skill drift / byte-identity break, and the regression test is a source scan rather than a behavior test.

Addressed at 411e562e8:

• applied the caption-state ownership fix to all three shared builders and restored byte identity;
• mirrored byte-identical tests into all three skills;
• replaced the source scan with 78 behavior checks (13 skins × light/dark canvases × three builders) asserting each skin's active/spoken rules survive exactly once;
• updated the PR rationale to document why --cap-canvas luminance was the wrong override signal.

Validation: bun run test:skills (388 passed), bun run lint:skills, manifest sync, and diff check all pass.

[miguel-heygen]

Current-head follow-up: formatted all three byte-identical caption builders and regenerated the skills manifest. Focused behavior suite remains 78/78; all three builders have identical md5 c9b31850c9241b02cb6be557a4b1e81d. This resolves the Format/Preflight failure at the prior head. CI rerun is in progress.

[james-russo-rames-d-jusso]

Re-verified at 2b3578840. Δ from b4c986db: fix propagated to all three skills + test-file structure rewritten to loop-over-presets on both canvases.

Blockers

(none — R1 blocker resolved)

Concerns

(none)

Nits

(none)

Green notes

🟢 Byte-identity restored across all three skills. md5 of skills/{faceless-explainer,pr-to-video,product-launch-video}/scripts/captions.mjs at 2b3578840 = c9b31850c9241b02cb6be557a4b1e81d (all three). Same for the sibling captions.test.mjs files (967716b2ea4a94b930f49d2cf57a7b01). No divergence left behind. Closes the R1 blocker.

🟢 Regression test now loops the preset directory on BOTH canvases. For each frame-preset/*/caption-skin.html that ships .caption-word.is-active, the test builds the skin on --cap-canvas: #f7f3e8 (light) AND --cap-canvas: #111827 (dark) and asserts both .caption-word.is-active and .caption-word.is-spoken rules survive in the built output. The R1 dark-ground override (strip .is-active / .is-spoken when luminance < 90) would fail this test on the dark canvas — real red-first pin, not a rewritten assertion looking for the same string it just emitted.

🟢 Symmetric coverage — all three skill trees run the same test. Since the three test files are byte-identical, adding a new preset (or removing state rules in one skin) exercises the pin from every skill's test suite. Closes the "test placement asymmetric" nit from R1 as a side effect.

Verdict framing

Blocker cleanly closed. LGTM from my side.

— Review by Rames D Jusso

[vanceingalls]

✅ APPROVED — R3 at head 2b3578840 · position: RIGHT direction, LANDED cleanly. Both R1/R2 blockers resolved end-to-end at the new head. Independent verification below matches @james-russo-rames-d-jusso's R3 receipts.

R2 findings at 2b3578840

P1a · Cross-skill drift — ✅ RESOLVED.
Refetched via /contents?ref=2b3578840:

• skills/faceless-explainer/scripts/captions.mjs — md5 c9b31850c9241b02cb6be557a4b1e81d, 499 lines.
• skills/pr-to-video/scripts/captions.mjs — md5 c9b31850c9241b02cb6be557a4b1e81d, 499 lines.
• skills/product-launch-video/scripts/captions.mjs — md5 c9b31850c9241b02cb6be557a4b1e81d, 499 lines.

Byte-identical across all three. diff -q clean pairwise. Direction of resolution is REMOVAL (the two siblings caught up to the primary, not the other way): the 15-line dark-ground override + its lum-luminance branch are gone from all three; file now runs straight from export { buildFromSkin } at L283 into brandFontFaces() at L289. Same as Rames' md5.

P1b · Test is source-scan, not behavior — ✅ RESOLVED.
captions.test.mjs at 2b3578840 (byte-identical across three skills, md5 967716b2ea4a94b930f49d2cf57a7b01, 50 lines each) now:

1. Reads every hyperframes-creative/frame-presets/<name>/caption-skin.html fixture.
2. Filters to skins that define .caption-word.is-active.
3. For each such skin × each --cap-canvas in ["#f7f3e8" /* light */, "#111827" /* dark */]:
   • Invokes buildFromSkin(skin.source, [], 1, 1920, 1080, ":root { --cap-canvas: <color>; --cap-ink: #111111; --cap-accent: #ffcc00; }", die) with concrete tokens.
   • Asserts the skin's .caption-word.is-active and .caption-word.is-spoken rule bodies appear in output verbatim AND appear exactly once (no duplicate override injected).

That's a red-first pin on the actual invariant — the R1 dark-ground override would fail this test on the dark canvas (it emitted a second .caption-word.is-active { … } block, breaking the ?.length === 1 assertion). Real behavior test, not source-scan. Test: skills gate reports SUCCESS.

P2 · Rationale — ✅ ADDRESSED (via PR body, which lands as squash-merge body).
The PR body ## Why now carries the mechanistically-correct rationale verbatim:

"The removed heuristic keyed on --cap-canvas luminance, but that token is not necessarily the caption pill background: skins such as broadside use --cap-ink for the pill. Appending a generic dark-canvas override therefore targeted the wrong signal and could replace the skin-owned state-color contract."

Commit bodies (5694557, b4c986db, 411e562e, 2b357884) remain empty headlines, but hyperframes squash-merges with the PR body as the merge-commit body, so this rationale reaches git log on merge. Closes the R1 "next agent won't have context" concern.

nit · frame-worker.md scope drift — 🟡 PERSISTS (non-blocking).
Still in the diff at 2b3578840 (+2 −0): one line on project-root-relative asset paths in HTML/CSS, one on <frame_id>- prefixed ids/classes for parallel-worker collision safety. Both are worker-agent guidance, no runtime impact, and reviewable on their own merits. Same scope-drift note as R1 — belongs in a separate audit trail next time, but not worth blocking on.

R3-novel findings

• Δ from R2 head b4c986db: two new commits — 411e562e fix(skills): keep caption skin ownership aligned (propagates the override deletion + test to faceless-explainer and pr-to-video, restoring byte-identity) and 2b357884 style(skills): format shared caption builders (formatting-only pass). No unexpected collateral.
• CI at 2b3578840. All required checks SUCCESS (Preflight (lint + format), Format, Fallow audit, Typecheck, Test, Test: skills, Skills: manifest in sync, Producer: unit tests, Producer: integration tests, Tests on windows-latest, Render on windows-latest, Preview parity, regression, preview-regression, player-perf, Build, Lint, Semantic PR title, CLI smoke (required), SDK: unit + contract + smoke, Test: runtime contract, Studio: load smoke). Only SKIPPED items are duplicate Preflight matrix legs, Perf: ${{ matrix.shard }} shards, regression-shards, and Mintlify Deployment — all expected skips for this diff surface.
• Envelope clean. All four commits authored+committed by Miguel Ángel <miguel.sierra@heygen.com>. No Co-Authored-By: Claude or 🤖 Generated with [Claude Code] anywhere in commit messages or PR body. Miguel's own hands throughout.
• Peer position. Rames' R3 at 2b3578840 (state COMMENTED, LGTM prose) concurs — his md5 receipts match mine byte-for-byte on both source and test files.
• Merge state. mergeable = MERGEABLE, mergeStateStatus = BLOCKED on reviewDecision = REVIEW_REQUIRED. This APPROVE should unblock.

— Via