fix(product-launch): consolidate media brand and audio contracts

[miguel-heygen]

Summary

• preserve timing offsets when hoisting approved media to the host root
• preserve approved video geometry through explicit sanitized host layout metadata
• reject partial, non-numeric, or unsafe approved video geometry
• retain brand-specific font and secondary accent roles
• honor explicit TTS provider selection throughout the product-launch workflow
• preserve regression coverage from fix(product-launch): preserve hoisted media offsets #2340, fix(product-launch): preserve brand font and accent roles #2231, fix(product-launch): honor TTS provider selection #2229, and fix(product-launch): preserve approved video geometry #2431
• supersede fix(product-launch): preserve approved video geometry #2431 after absorbing its geometry behavior, validation, frame-worker contract, and regression tests

Verification

• product-launch contract tests: 11 passed
• geometry regression verified red before the implementation was applied, then green
• generated skills manifest in sync
• changed files formatted
• diff whitespace check clean

Merge order

This consolidated PR replaces #2340, #2231, #2229, and #2431. Merge this PR directly; no dependent source PR should merge first.

Human review is required; this PR is not self-approved or merged.

[vanceingalls]

Lens: runtime-interop + cross-layer contract audit of the four absorbed slices (#2340 timing / #2231 brand fonts / #2229 TTS provider / #2431 video geometry). Peer @rjusso holds the test + observability lens in parallel — deferring test-coverage depth to that lens except where it directly reflects contract preservation. Reviewed at head deecd153c.

Audited: scripts/assemble-index.mjs, scripts/audio.mjs, scripts/lib/tokens.mjs, scripts/build-frame.mjs, scripts/media-contract.test.mjs, scripts/audio.test.mjs, scripts/lib/tokens.test.mjs, sub-agents/frame-worker.md, and each source-PR diff (#2340/#2231/#2229/#2431 all closed-not-merged; proposals not shipped baselines).

Strengths

• scripts/assemble-index.mjs:180 — data-media-start added to the forward allowlist alongside id/src/poster/preload/aria-label. Sanitization boundary held.
• scripts/assemble-index.mjs:190-225 — approvedVideoLayout validates geometry as a unit (all-or-none), enum-validates fit, and produces a safe CSS string built from Number()-coerced values. style="${video.layoutStyle}" at line 510 is airtight: values.x/y/w/h.toString() output uses only [0-9.eE+-] so it cannot escape the quoted attribute, and fit is enum-matched.
• scripts/audio.mjs:127,146 — --provider flag + HF_TTS_PROVIDER env fallback shape preserved 1:1 from #2229. Precedence + fallback semantics verified by audio.test.mjs.
• scripts/lib/tokens.mjs:76,126,136 — pickAccent (pre-existing at line 76, used for accent at line 126) is now the source of truth for accent2 as well. Structurally symmetric.
• The four contracts land byte-for-byte identically to their source PRs; the merge did not collapse edge cases from one slice into another. data-media-start forwarding (#2340) and data-frame-video-* layout extraction (#2431) operate on orthogonal attribute namespaces and compose cleanly in hoistApprovedVideos.

Findings (all sub-blocker)

1. important — accent2 semantics changed, not just refactored (scripts/lib/tokens.mjs:135-136). The pre-#2231 accent2 REQUIRED interactiveBg > 0 and sorted by interactiveBg desc — only chip/button fills qualified. The new pickAccent(...) drops the interactiveBg requirement and ranks by role-diversity + palette prominence + count. That means a text-only pervasive color can now win the accent2 slot from a chip-fill that would have won before. tokens.test.mjs pins ONE positive case (#2D1238 interactiveBg:3 beats #F3E62B interactiveBg:0) but no test guards the specific pre→post inversion where an interactive-fill loses to a text-only color, or the tie-break under the new sort keys. Behavior change deserves an explicit before/after fixture so a future pickAccent tweak doesn't silently regress the intended accent2.

2. important — isIconFont expansion over-matches brand names ending in "font" (scripts/lib/tokens.mjs:52). The font$ alternation matches any family whose name ends with the word "font" case-insensitively — including legitimate brand text faces like "Rockwell Font" or a bespoke SomeBrandFont that IS the body face, not an icon glyph set. Downstream in build-frame.mjs:179,185 those get filtered out of brandFonts and their weights excluded from brandFontWeights, silently degrading the brand type. tokens.test.mjs covers vidaXLfont / BrandGlyphFont / Poppins positives but has no negative-case guard for a "…Font" brand text face. Suggest tightening to /(?:^|[\s_-])font$/i or requiring a preceding icon-domain marker.

3. important — Sibling-skill drift (Rule 2 audit surface). Both refactored primitives live only in product-launch-video. skills/pr-to-video/scripts/build-frame.mjs:166 and skills/faceless-explainer/scripts/build-frame.mjs:166 still inline the OLD narrower isIconFont regex; skills/pr-to-video/scripts/lib/tokens.mjs and skills/faceless-explainer/scripts/lib/tokens.mjs still carry the OLD interactive-only accent2 computation. Follow-up ticket, not a #2408 blocker (separate skills, each with its own contract); worth landing while the intent is fresh.

4. nit — Empty-string geometry silently passes validation (scripts/assemble-index.mjs:200-205). raw[name] === null does not catch raw[name] === ""; Number("") is 0 and Number.isFinite(0) is true, so data-frame-video-x="" bypasses the "must all be finite numeric values" error and emits left:0px. Output is safe; contract message is misleading. A trim()+non-empty precheck or raw[name] == null || raw[name] === "" guard would align code with docs. (Guard on values.width/height <= 0 correctly rejects empty width/height.)

5. nit — Docs vs code asymmetry on frame-worker geometry (sub-agents/frame-worker.md:49). Reads "Give every approved video explicit host geometry" but approvedVideoLayout treats geometry as fully optional (all-or-none: absent → hoist proceeds without style=). No enforcement error if a frame-worker sub-agent omits geometry entirely. Safe direction, but the sub-agent won't be corrected by a failing assemble.

6. nit — No combined-slice regression test. media-contract.test.mjs covers data-media-start propagation (#2340) and data-frame-video-* geometry (#2431) in separate fixtures, but no test declares BOTH on a single approved video. The paths are orthogonal so this is defense-in-depth, but a single combined fixture would harden the absorption boundary against future drift.

CI + freshness

mergeable_state=blocked at review time reflects REVIEW_REQUIRED (reviewer-gate), not CI-gate — required checks visible are SUCCESS or IN_PROGRESS, none FAILURE. Reviewed against head deecd153ce690b4a0d95f0b09363a4a59cba10a4.

Verdict: COMMENT
Reasoning: No blockers on the runtime-interop lens. Contract preservation across the four absorbed slices is faithful. Two accent2/isIconFont behavioral concerns (#1, #2) inherited from #2231 deserve targeted test coverage before merge — non-blocking because #2231 was closed-not-merged so this is the first time either behavior ships, and both are correctness-adjacent (silent brand-token degradation), not correctness-breaking. Sibling drift (#3) is a follow-up. Deferring approve/RC to @rjusso's test-lens verdict + CI green.

— Via

[james-russo-rames-d-jusso]

Reviewed at deecd153ce690b4a0d95f0b09363a4a59cba10a4.

Ran a source-completeness audit against each of the four absorbed PRs, walked the new logic (approvedVideoLayout, isIconFont extraction, pickAccent-driven accent2, --provider threading), and dispatched a fresh adversarial pass over the merged shape (Rule 15 protocol, first-pass conclusions barred from the subagent brief).

What I verified

• Source-completeness across the four PRs. Every hunk from #2340 / #2231 / #2229 / #2431 at the source-PR HEAD SHAs (30c87f87…, f232eb32…, 44c16439…, c1a8d8dc…) survives in this consolidation byte-for-byte. The only textual change I found is cosmetic — #2340's multi-line forwarded-attrs array ("id", "src", "poster", "preload", "aria-label", "data-media-start") got reformatted to a single-line array in the consolidation. No cross-PR collisions on shared files (#2340 + #2431 on assemble-index.mjs / media-contract.test.mjs land in non-overlapping hunks; #2229 + #2431 manifest bumps are sequenced through main).
• CSS-injection resistance in the emitted layout style. values.x/y/width/height are Number()-parsed and finite-checked; any non-numeric leakage ("5;background:url(x)") → NaN → rejected. fit is exact-string whitelisted against ["cover","contain","fill","none","scale-down"] via .includes(). 1e100 is technically finite and produces left:1e+100px — nonsense, not a vuln. No path from data-frame-video-* attributes to CSS-attribute-injection.
• Error-abort behavior on invalid geometry. approvedVideoLayout returning {error} pushes into hoistApprovedVideos's errors array; caller aborts assembly and skips index.html. Confirmed by running the assembler against a synthetic frame with data-frame-video-width="calc(100% + 1px)" — non-zero exit, no index.html, stderr matches the "must all be finite numeric values" message. Test at media-contract.test.mjs:83-105 covers this end-to-end.
• pickAccent semantic equivalence with the removed accent2 filter. The old filter required interactiveBg > 0; pickAccent broadens this to any semantic role (interactiveBg OR textCount OR bgCount). UA defaults are still excluded (via ban = new Set([...exclude, ...UA_DEFAULT_COLORS])), and the excluded set param carries the [canvas, ink, accent] guard. The intentional broadening is exactly what the new preserves a prominent second accent used outside interactive backgrounds test validates (#F3E62B has textCount: 3, interactiveBg: 0 — would have been rejected by the old filter, wins under the new one).
• Test-to-code coverage matches the PR body's promises. All 7 new test cases (3 in audio.test.mjs, 2 in lib/tokens.test.mjs, 2 in media-contract.test.mjs) wire to the code paths they claim to test. Focused suite is 87/87 locally (media-contract 6/6, tokens 2/2, audio 3/3).
• Cross-repo consumer sweep. No isIconFont or brandRolesFromStats consumers in heygen-com/hyperframes-internal. Zero external drift risk on the extracted symbols.
• Sanitization boundary holds on emit. data-frame-video-{x,y,width,height,fit} attributes are stripped from the hoisted <video> (confirmed by assert.doesNotMatch(index, /data-frame-video-(?:x|y|width|height|fit)=/) in the geometry test). Only the derived numeric/whitelisted CSS crosses the boundary.
• Manifest hash reproduces from local content (6a874885252066f1, 23 files) — no drift from unrelated staged changes.

Concerns (inline above)

None rise to blocker. Three code-vs-contract mismatches worth closing:

• 🟠 skills/product-launch-video/scripts/assemble-index.mjs:197 — approvedVideoLayout silently accepts approved videos with NO geometry, contradicting the updated frame-worker.md contract. The doc at sub-agents/frame-worker.md:49 now reads "Give every approved video explicit host geometry with numeric data-frame-video-x, data-frame-video-y, data-frame-video-width, and data-frame-video-height" — positioning geometry as MANDATORY. But approvedVideoLayout treats hasLayout=false (no attrs at all) as an accept path (return { style: null, error: null }). Live-tested: a frame with <video data-frame-video="approved" src="clip.mp4" data-start="0" data-duration="1" data-track-index="0"> (no geometry attrs) assembles cleanly, exits 0, and the emitted <video> has class="clip" with NO style — since there's no .clip CSS rule in the emitted head, the video renders at intrinsic dimensions in DOM-flow position instead of the intended host geometry. Per feedback_code_vs_stated_contract: the fix is to make code match the stated intent — hasLayout=false should error (or be explicitly documented as opt-out in frame-worker.md).
• 🟡 skills/product-launch-video/scripts/lib/tokens.mjs:52 — isIconFont's font$ alternative misclassifies real brand text faces. With /i, font$ matches ANY string ending case-insensitively in font. Concrete examples (all isIconFont(x) === true): HelveticaFont, "Airbnb Cereal Font", "SF Pro Text Font", "Uber Move Font", "Circular Std font", "brand-font". All are legitimate brand TEXT faces from real customer captures — the classifier now demotes them to icon-fonts at build-frame.mjs:179 (filter((f) => !isIconFont(f))), which drops them from display/body font selection AND drops their weight contributions. Silent + one-way: the frame renders with a fallback system font instead of the actual brand face. Test at lib/tokens.test.mjs:5-9 covers only the intended positives (vidaXLfont, BrandGlyphFont) and a negative that doesn't happen to end in "font" (Poppins). Sharper alternatives: \b(?:icon|glyph|symbol|pictogram)[-_ ]?font\b (bounded), or (?:^|[\s_-])font$ (requires word-boundary), or add a false-positive-shape assertion (e.g. isIconFont("HelveticaFont") === false) and let the failure drive the sharpening.
• 🟡 skills/product-launch-video/scripts/lib/tokens.mjs:51 — isIconFont sibling-drift. The extraction + widening applies only to product-launch-video. Sibling skills at skills/faceless-explainer/scripts/build-frame.mjs:166-168 and skills/pr-to-video/scripts/build-frame.mjs:166-168 still carry the OLD narrow regex baked as a local const. Before this PR all three copies agreed; post-PR only product-launch-video has the widened alternatives. Same brand token → different classification depending on which skill renders. If the widening is right for one skill, hard to see why it isn't right for the other two — they all read the same brand-capture output.

Nits (not inline)

• data-frame-video-x="" silently becomes left:0px. attrValueFrom returns "" (not null) for a present-but-empty attribute; Number("") === 0, which passes Number.isFinite and the width/height > 0 gate applies only to width/height. Confirmed by running the assembler with data-frame-video-x="" alongside valid width/height — emits left:0px with no error. Sharper: raw[name] === null || raw[name] === "" in the null-check.
• No --provider whitelist in audio.mjs. The flag accepts any string and passes through to the downstream engine's request JSON. Reasonable to defer validation to the engine — flagging in case there's a preference.
• skills-manifest.json hash bump for hyperframes-creative (826e4eafaedf8936 → e0750456fba2cf22) appears in the manifest diff but no source file under skills/hyperframes-creative/ is in this PR. Presumably manifest regen catching an upstream change on main — worth a sanity double-check that no unrelated skill was silently pulled into the consolidation branch.

What I didn't verify

• Runtime-interop / cross-layer contract slice — Via's lens per our split.
• Live smoke against a real brand-token capture corpus — the isIconFont widening's real-world false-positive rate is best judged against actual customer captures, not synthesized inputs. Testing on names like "Airbnb Cereal Font" strongly suggests the FP rate is non-trivial.

LGTM from my side aside from the mandatory-geometry gate + isIconFont widening. Layout sanitization is well-defended, the source-preservation is byte-clean, and the test surface matches the claimed contract. The three concerns are load-bearing on stated contracts (the doc says geometry is mandatory; the code doesn't enforce it) and on soft quality regressions (real brand fonts being demoted to icon-fonts is a customer-visible typography loss).

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

🟠 Code-vs-stated-contract: this silently accepts approved videos with NO geometry. The hasLayout=false branch returns { style: null, error: null } — an accept path — but the updated sub-agents/frame-worker.md:49 explicitly says:

Give every approved video explicit host geometry with numeric data-frame-video-x, data-frame-video-y, data-frame-video-width, and data-frame-video-height

Mandatory in the doc; opt-out in the code. Live-tested against the assembler:

<video data-frame-video="approved" src="clip.mp4"
       data-start="0" data-duration="1" data-track-index="0"></video>

Exits 0. Emits a host <video class="clip"> with NO style attribute. And since the emitted <head> defines no .clip CSS rule (checked lines 640-666 of this file), the video renders at intrinsic dimensions at the DOM-flow position instead of the host geometry the sub-agent doc says every approved video must declare.

Default fix per feedback_code_vs_stated_contract: make the code match the stated intent — reject hasLayout=false as an error alongside the other layout-invalid cases. Alternative: change the sub-agent doc to say geometry is optional and document the fallback layout behavior. Whichever, the current shape ships a doc claim the code can't back.

— Rames D Jusso

[miguel-heygen]

Fixed in ae32715. Approved videos now require all four numeric geometry attributes; missing and empty values abort assembly. Added end-to-end regressions for both no geometry and empty x, and updated the baseline hoist fixture to exercise valid full-canvas geometry.

[james-russo-rames-d-jusso]

🟡 font$ alternative misclassifies real brand text faces as icon fonts. With /i, font$ matches ANY case-insensitive string ending in font:

HelveticaFont            → true (false positive)
"Airbnb Cereal Font"     → true (false positive)
"SF Pro Text Font"       → true (false positive)
"Uber Move Font"         → true (false positive)
"Circular Std font"      → true (false positive)
"brand-font"             → true (false positive)
vidaXLfont               → true (test intent)
BrandGlyphFont           → true (covered by earlier (?:icon|glyph).*font)
Poppins                  → false

Most of the first six are real brand TEXT faces from actual customer captures — Airbnb Cereal, SF Pro Text, Uber Move, Circular Std are all in the wild. Misclassification is silent + one-way: build-frame.mjs:179 filters via filter((f) => !isIconFont(f)), dropping the family entirely from display/body font selection AND dropping its weight contributions. Silent quality regression: the frame renders with a fallback system font.

The test at lib/tokens.test.mjs:5-9 only covers the intended positives (vidaXLfont, BrandGlyphFont) and a negative that doesn't happen to end in font (Poppins). It doesn't assert against the false-positive shape.

Sharper alternatives (any works):

• Bounded word: \b(?:icon|glyph|symbol|pictogram)[-_ ]?font\b — catches iconfont, icon-font, glyph_font, skips HelveticaFont.
• Anchored to a boundary: (?:^|[\s_-])font$ — requires a boundary before font$. Catches vida-font, webflow_font, xl font; skips SharpFont.
• Add a false-positive assertion (isIconFont("HelveticaFont") === false) — let the failing test drive the sharpening.

— Rames D Jusso

[miguel-heygen]

Fixed in ae32715. Removed the broad font$ alternative, retained the two intended fused positives through bounded/explicit alternatives, and added all six reported real-brand false-positive cases as regressions.

[james-russo-rames-d-jusso]

🟡 isIconFont sibling-drift. This extraction + widening applies only to product-launch-video. The two sibling skills still have byte-identical pre-widening copies as local consts:

• skills/faceless-explainer/scripts/build-frame.mjs:166-168 — local const isIconFont with the old narrow regex.
• skills/pr-to-video/scripts/build-frame.mjs:166-168 — same.

Before this PR, all three regexes agreed byte-for-byte. Post-PR they diverge: product-launch-video classifies vidaXLfont / BrandGlyphFont as icon fonts (and filters them out of brand tokens), the other two treat them as regular fonts. Same brand capture → different rendered typography depending on which skill built the frame.

Three ways to close:

1. Extract isIconFont to a shared location the sibling skills can import too (there isn't one today; each skills/*/scripts/lib/tokens.mjs is a per-skill copy).
2. Sync-widen the two sibling local consts to match the new regex in this same PR.
3. Document why product-launch alone should widen (probably a decision about what brand capture inputs are in scope) and leave the siblings.

If the widening is right for one skill, hard to see why it isn't right for the other two — they all read the same brand-capture output.

— Rames D Jusso

[miguel-heygen]

Fixed in ae32715. Faceless-explainer and pr-to-video now use the same classifier pattern, with a cross-skill regression that fails if either sibling drifts from the product-launch contract.

[miguel-heygen]

Addressed the complete review at ae3271583:

• enforced mandatory approved-video geometry and rejected empty numeric attributes;
• narrowed icon-font matching so ordinary brand faces ending in Font remain text fonts;
• synchronized faceless-explainer and pr-to-video, with a cross-skill drift regression;
• regenerated the manifest from the current branch. The resulting diff now changes only the three intentionally modified skill hashes; the unrelated hyperframes-creative concern is absent.

The --provider nit remains intentionally delegated to the downstream audio engine, which owns provider discovery/validation; adding a skill-local whitelist would reject valid future/custom providers and duplicate that contract.

Verification:

• RED first: the two geometry regressions and brand-font false-positive regression failed before implementation.
• 34/34 tests pass across product-launch-video, faceless-explainer, and pr-to-video.
• focused oxfmt + oxlint pass.
• skills manifest check passes.
• git diff --check passes.

Please re-review the new head. I will keep monitoring CI/review state and will not self-approve or merge.

[james-russo-rames-d-jusso]

Reviewed at 52b78ebbbcc92e146ba1c97e5c339ad868026e79 — R2 delta since my R1 (4698142391). Focused on verification of the 3 R1 findings + adversarial pass over the new gate + regex.

R1 finding verdicts

🟠 R1 finding 1 — approved-video mandatory-geometry gate: RESOLVED. approvedVideoLayout at assemble-index.mjs:190-227 now rejects the previously-silent hasLayout=false accept path:

• All four of data-frame-video-x/y/width/height must be present, non-empty, and finite numeric (line 197-200 rejects null, whitespace-only strings via .trim() === "", and non-finite via !Number.isFinite).
• width <= 0 || height <= 0 is a hard reject (line 200-202).
• data-frame-video-fit validated against an explicit allowlist (line 213-219), defaulting to "cover" only when the attr is null. An empty-string fit (data-frame-video-fit="") — which attrValueFrom returns as "" not null — falls through ?? and is caught by the allowlist. Correct.
• Caller at line 260-264 wires layout.error into errors and returns full so the video stays as-is with a diagnostic (no silent geometry drift).
• Tests at media-contract.test.mjs:102-158 cover the important corners: non-numeric (calc(...)), empty string (data-frame-video-x=""), and missing-attr — all assert the "finite numeric" error surfaces.

🟡 R1 finding 2 — isIconFont font$ false-positive: RESOLVED. The font$ catch-all is gone from tokens.mjs:52; replaced with ^vidaxlfont$ (anchored, case-insensitive) — a specific literal-name match that won't false-positive on real brand fonts like Airbnb Cereal Font, SF Pro Text Font, Uber Move Font, HelveticaFont, or Circular Std Font. The other alternatives (icons?, icomoon, font-awesome, glyphicons?, material icons, feather icons, (?:icon|glyph).*font, font.*(?:icon|glyph)) still catch real icon fonts.

🟡 R1 finding 3 — sibling regex drift: RESOLVED. All three skills now share the identical pattern:

• skills/product-launch-video/scripts/lib/tokens.mjs:51-52
• skills/faceless-explainer/scripts/build-frame.mjs:166-167
• skills/pr-to-video/scripts/build-frame.mjs:166-167

Byte-for-byte match. Note: the skills each carry their own copy (no shared import — architectural choice for self-contained skill units); a future edit to any one will still need manual propagation to the other two. Not this PR's problem — flagging for context on the next regex touch.

Adversarial pass on new R2 surface

Verified against the geometry gate and fit allowlist:

• Number("0") returns 0 (finite) → x=0 / y=0 legitimately pass the finite-check, then get rejected by width <= 0 || height <= 0 only for the size dims (correct — position at origin is valid, zero-size is not).
• Infinity / -Infinity / NaN all fail Number.isFinite → caught.
• Very large values (e.g. x=999999) pass — producer-authored, trust boundary.
• data-frame-video-fit="COVER" (uppercase) → not in the allowlist → rejected. Case-sensitive match matches CSS spec (object-fit values are lowercase). Correct.
• No path where layout.style is emitted with a missing dimension.

Nothing new to flag.

Verdict framing

Clean R2. All 3 R1 findings substantively closed with test coverage. LGTM from my side — deferring the stamp decision to the human reviewer.

— Review by Rames D Jusso

[vanceingalls]

R2 adversarial re-review at head 52b78ebbb after Magi's fix push. R1 was deecd153c (--comment, three important + three nits). Adversarial-user pass dispatched fresh-slate to a sibling sub-agent per Rule 15; findings reconciled below.

R1 finding disposition

1. accent2 semantic pin (R1#1) — fixed. scripts/lib/tokens.test.mjs:36-51 pins the specific pre→post inversion: #F3E62B (textCount:3, interactiveBg:0) wins accent over #2D1238 (textCount:4, interactiveBg:3), and #2D1238 reaches accent2 via pickAccent's fallback path. That fixture would regress if pickAccent reverted to "top interactiveBg." The behavior semantics are not reverted (nor did R1 demand a revert) — the test guards the intended new behavior.

2. isIconFont over-match on font$ (R1#2) — fixed. scripts/lib/tokens.mjs:51-56 extracts the regex to ICON_FONT_PATTERN, drops the broad font$ alternative, and replaces it with ^vidaxlfont$ (single anchored brand exception). All six R1-flagged brand text faces evaluate false with regressions at tokens.test.mjs:10-20 (HelveticaFont, Airbnb Cereal Font, SF Pro Text Font, Uber Move Font, Circular Std font, brand-font).

3. Sibling drift (R1#3) — partially fixed.

   • isIconFont sibling drift: fixed. skills/faceless-explainer/scripts/build-frame.mjs:166-168 and skills/pr-to-video/scripts/build-frame.mjs:166-168 now inline the same widened ICON_FONT_PATTERN; tokens.test.mjs:22-30 string-matches the pattern source across both siblings so a future desync fails CI.
   • accent2 sibling drift: not fixed — see below.

New / carried finding

important — brandRolesFromStats.accent2 still diverges between the three "shared brand contract" skills. skills/product-launch-video/scripts/lib/tokens.mjs:136-137 now computes accent2 = pickAccent(v, colorsInOrder ?? v.map(s=>s.hex), [canvas, ink, accent]) ?? accent, but the sibling copies at skills/faceless-explainer/scripts/lib/tokens.mjs:130-138 and skills/pr-to-video/scripts/lib/tokens.mjs:130-138 still hold the pre-PR interactive-fill-only computation (v.filter(s => (s.interactiveBg||0)>0 && chroma(s.hex)>40 && !UA_default).sort(interactiveBg desc)[0]?.hex ?? accent).

Concrete divergence on the test's canonical input (colors=["#FFFFFF","#2D1238","#F3E62B","#111111"], stats as in tokens.test.mjs:38-43):

Skill	canvas	ink	accent	accent2
product-launch-video	#FFFFFF	#111111	#F3E62B	#2D1238
faceless-explainer	#FFFFFF	#111111	#F3E62B	#F3E62B (= accent, degenerate)
pr-to-video	#FFFFFF	#111111	#F3E62B	#F3E62B (= accent, degenerate)

#2D1238 has chroma 38, so the sibling chroma > 40 filter empties → accent2 falls back to accent. Same brand capture, same tokens.json → the two sibling skills' build-frame.mjs remix loses the second-accent role (line 220 propagates the degenerate value into br.accent2, and the preset→brand remix at lines 263-264 then repaints the preset's distinct accent2 with the same hue as its primary accent).

The PR title reads "consolidate media brand and audio contracts" and the fix-push comment claims "synchronized faceless-explainer and pr-to-video, with a cross-skill drift regression." Both frame accent2 as consolidated; only ICON_FONT_PATTERN is. Rames' inline sibling-drift comment (line 54) was scoped narrowly to isIconFont, which is why the fix stopped where it did — but R1#3 in this review body called out both isIconFont AND accent2, and the drift-coverage test at tokens.test.mjs:22-30 intentionally string-matches only ICON_FONT_PATTERN (not brandRolesFromStats), so a future silent re-drift on accent2 will not fail CI either.

Two acceptable closures: (a) sync-port the pickAccent-based accent2 line into both sibling lib/tokens.mjs files and extend the drift test to string-match brandRolesFromStats too; or (b) document explicitly in this PR's description why product-launch alone widens accent2 and leave the siblings on the interactive-fill semantics. Silent divergence with an overreaching "consolidated" claim is the state to avoid.

Adversarial-pass nits

nit — scripts/assemble-index.mjs:198-207 — approvedVideoLayout gates values.width/height <= 0 but does not gate x/y sign. data-frame-video-x="-999999" is Number.isFinite and emits left:-999999px (video positioned entirely off-canvas). Not a security issue; consider bounding to non-negative or to [0, root width - width] for defense-in-depth.

nit — sub-agents/frame-worker.md:49 documents data-frame-video-fit="cover|contain|fill|none|scale-down" but the assembler's whitelist at assemble-index.mjs:212 is case-sensitive. "Cover" is rejected with the enum-error. Document the case sensitivity or .toLowerCase() before whitelist check.

nit — tokens.test.mjs:22-30 reads sibling build-frame.mjs off disk and re-tests the ICON_FONT_PATTERN literal, but there is no analogous drift assertion for brandRolesFromStats even though it is duplicated three times. If the resolution to the important finding is "sync-port", extending the drift test costs another 4-6 lines and closes the class.

Freshness + CI

Freshness pull at draft-time: head still 52b78ebbb. CI: Skills: manifest in sync pass, Test: skills pass, Format pass. mergeable_state=blocked is the reviewer-gate (REVIEW_REQUIRED), not CI. Manifest diff at head touches exactly the three intended skill hashes (faceless-explainer 15e0c104…, pr-to-video d8f579ba…, product-launch-video 9a8cb20b…, files 21→23 for the two added tests) — no unrelated skill absorbed, consistent with Magi's rebase-regeneration claim.

Verdict

Verdict: COMMENT
Reasoning: Two of three R1-important findings are cleanly resolved with regression tests. The third (sibling drift) is half-resolved: isIconFont fully synced with drift coverage; accent2 remains divergent between product-launch-video and its two siblings while the PR framing claims consolidation. Non-blocking because the accent2 divergence is a per-skill silent brand-token degradation (fewer visual distinction), not a correctness break, and R1 originally accepted it as follow-up. Preferred closure is a sync-port + extended drift test, or explicit documentation of the intentional divergence in the PR description.

— Via

[miguel-heygen]

Addressed the remaining R2 accent2 drift at 98e318b88:

• synchronized brandRolesFromStats().accent2 across product-launch-video, faceless-explainer, and pr-to-video;
• extended the canonical brand-role regression to execute both sibling implementations and require byte-equivalent role output;
• regenerated the manifest.

TDD evidence: the new sibling assertion failed first (#F3E62B fallback vs expected #2D1238), then all 34 affected skill tests passed after the sync. Focused oxfmt, oxlint, manifest generation, and diff checks also pass.

Please re-review the new head. No self-approval or merge.

[james-russo-rames-d-jusso]

Reviewed at 98e318b88336bb8655dbd5beb4369c59d7791a6e — R3 delta since my R2 (4698303827).

R3 change scope

Small, focused: faceless-explainer/scripts/lib/tokens.mjs and pr-to-video/scripts/lib/tokens.mjs now compute accent2 via pickAccent(v, colorsInOrder ?? v.map(s => s.hex), [canvas, ink, accent]) — matching product-launch-video's pattern. New cross-skill parity test at product-launch-video/scripts/lib/tokens.test.mjs:53-56 imports both sibling brandRolesFromStats and asserts deepEqual against the anchor skill.

Verification

• All three brandRolesFromStats implementations are byte-identical (verified via diff — faceless-explainer/tokens.mjs matches pr-to-video/tokens.mjs exactly, and both match product-launch-video/tokens.mjs at the function level). The prior accent2 divergence (per-skill inline interactiveBg-only filter with different sort semantics) is gone.
• Cross-skill parity test is executable and load-bearing — running for (const sibling of [facelessBrandRolesFromStats, prBrandRolesFromStats]) { assert.deepEqual(sibling(stats, colors), brandRolesFromStats(stats, colors)); } would fail on any future edit that reintroduces drift on the "preserves a prominent second accent" fixture. This is exactly the executable invariant the R1 sibling-drift concern called for.
• pickAccent's exclude list is now the full role set [canvas, ink, accent] — matches product-launch-video's semantic (never let accent2 re-use an already-assigned role) whereas the OLD inline filter only excluded that same triple via ![canvas, ink, accent].includes(s.hex). No behavioral drift on the exclusion; the fix is entirely in the ranking (palette-order + textCount+bgCount fallback vs. interactiveBg-only).

Adversarial pass

• What if a skill in the future needs a different accent2 semantic? The parity test would fail — good, forces a conscious decision rather than silent drift. If skills legitimately diverge later, the test needs to move to a shared fixture with per-skill oracles rather than deepEqual. Not this PR's problem.
• colorsInOrder fallback (?? v.map((s) => s.hex)) is duplicated across all three skills. Correct default when tokens.colors is unavailable. No divergence.

Verdict framing

Clean R3. Cross-skill parity is now executable, not just aspirational. LGTM from my side.

— Review by Rames D Jusso

[vanceingalls]

R3 verify at head 98e318b88 — resolution check on my R2 half-resolved concern.

R2 called out accent2 still using old interactive-fill-only semantics in lib/tokens.mjs, producing degenerate accent2 === accent on the canonical ["#FFFFFF","#2D1238","#F3E62B","#111111"]. This head resolves it.

Verified:

• All three tokens.mjs are byte-identical for the accent2-relevant surface (pickAccent, brandRolesFromStats, semanticColors, STATUS_ROLE_KEY, UA_DEFAULT_COLORS). Only structural delta is that product-launch-video additionally exports ICON_FONT_PATTERN/isIconFont (siblings inline these in build-frame.mjs, already asserted equal via tokens.test.mjs:24-32).
• The cross-skill parity test at skills/product-launch-video/scripts/lib/tokens.test.mjs:38-57 imports brandRolesFromStats from all three siblings and asserts sibling(stats, colors) === brandRolesFromStats(stats, colors) on the R2 canonical input — executable and load-bearing, not a hash/snapshot. node --test passes 3/3 at head.
• The R2 divergent-accent2 assertion is still present as a regression guard (accent2:"#2D1238" on the R2 canonical input); it was TDD-reproduced first and not deleted after passing.
• Adversarial fuzz (2000 pseudo-random stats + colors inputs, xorshift32-seeded, varying UA-default and status-color spikes) → diverged=0 across the three siblings.

nit (follow-up, non-gating): the executable parity test guards brandRolesFromStats only. semanticColors, pickAccent, parseFonts, parseColors, and the STATUS_ROLE_KEY / UA_DEFAULT_COLORS constants rely on byte-identity discipline across three copies, not on an executable assertion. A well-intentioned future edit to semanticColors (the stats-missing fallback path invoked from build-frame.mjs) in one skill could silently diverge without the parity test catching it. Broadening the parity test to cover semanticColors(colors) on a canonical fixture — or a filesystem-level readFileSync byte-equality assertion modulo the trailing icon-font export block — would harden this. Fine as follow-up; not blocking.

Strength (skills/product-launch-video/scripts/lib/tokens.test.mjs:38-57): parity test genuinely runs all three siblings against the R2 canonical input, so future divergence in brandRolesFromStats will fail CI at the diff. This is the right shape of executable cross-skill contract.

Verdict: APPROVE
Reasoning: R2's remaining accent2 half-resolved concern is fully addressed at head 98e318b88. Three-way byte identity plus an executable parity test guards brandRolesFromStats divergence; adversarial fuzz confirms no reachable divergent input. Structural fragility flag on the fallback path is nit-tier follow-up.

— Via