fix(skills): pipeline fixes from prompt-guide validation (BGM, caption accent, voice, PR version)#2110
fix(skills): pipeline fixes from prompt-guide validation (BGM, caption accent, voice, PR version)#2110vanceingalls wants to merge 1 commit into
Conversation
…n accent, voice, PR version) Behavior fixes surfaced by the prompt-guide validation campaign (Tier 1+2 of the upstream bug list; Tier 3 tracked in #2107). Split out from the doc-only updates, which follow in a separate PR. - BGM level: default bed volume under narration was 0.8 linear (~-2 dB, ~16 dB too hot vs voice). Now 0.12 (~-18 dB) via shared bgmDefaultVolume() in media-use bgm.mjs + assemble-index fallbacks in faceless-explainer / pr-to-video / product-launch-video. Explicit volume still wins; silent-film 0.9 and music-to-video unchanged. Adds bgm.test.mjs (3 cases); bgm.md reference updated to match. - Caption accent: semanticColors() ranked accents purely by chroma, so a preserved status red (#dc2626) outranked the brand accent and captions highlighted in error-red. Status-keyed colors now excluded via shared STATUS_ROLE_KEY regex consumed by both tokens.mjs and build-frame.mjs (all three skill copies kept in sync). - Voice threading: workflow SKILL.md Step 3.1 blocks now instruct choosing the narration voice from the user's ask and passing --voice <id>; previously "a male voice" was silently ignored and the default (Marcia/am_michael) always won. - fetch-pr shipping version: MERGED PRs get best-effort shipped_version + version_source in pr.json (first release published at/after merge, else default-branch package.json marked unreleased); ingest surfaces it as a 'Shipped in:' brief line; story-design.md forbids inventing versions when absent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
miga-heygen
left a comment
There was a problem hiding this comment.
LGTM with nits
Clean set of pipeline fixes — root causes are well-identified and consistently applied across all three skill copies. The STATUS_ROLE_KEY extraction is a textbook SSOT improvement. Four notes:
SSOT: BGM volume inlined in assemblers (medium)
bgm.mjs now exports BGM_BED_VOLUME / BGM_SILENT_VOLUME / bgmDefaultVolume() as the canonical owner of the bed-vs-silent decision. But the three assemble-index.mjs files hardcode 0.12 / 0.9 inline instead of importing from bgm.mjs:
// skills/{faceless-explainer,pr-to-video,product-launch-video}/scripts/assemble-index.mjs
const vol = audio.bgm.volume != null ? audio.bgm.volume : voiceCount > 0 ? 0.12 : 0.9;This means the "default BGM volume" decision is now owned in two places. If someone later tweaks BGM_BED_VOLUME in bgm.mjs, the assembler fallbacks silently drift. Consider importing bgmDefaultVolume (or at least the two constants) from ../../media-use/audio/scripts/lib/bgm.mjs.
Dead field: mergeCommit (low)
fetch-pr.mjs adds "mergeCommit" to FIELDS (line ~86), but the version-resolution logic never reads it — it uses mergedAt for timestamp comparison. If this was planned for a git tag --contains-style check and abandoned, remove it from FIELDS to keep the contract clean. If it's forward-looking, a comment would help.
Everything else
STATUS_ROLE_KEYshared constant: correct SSOT improvement, properly consumed by bothsemanticColors()and the preset→brand remix inbuild-frame.mjs.- Voice threading in SKILL.md: instructions are thorough, provider-aware, and consistent across all three skills.
bgm.test.mjs: solid coverage — bed volume, silent-film volume, and dB separation assertions are all correct (20 * log10(0.12) ≈ -18.4 dB).story-design.mdversion guidance: clear, correctly distinguishes "shipped in" vs "unreleased" vs "no version resolvable."ingest.mjssurfacingShipped in:line: type checks are correct, null handling is clean.
— Miga
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at b208d06.
Behavior-fixes bundle split out of #2109, sane and well-tested — the BGM level fix ships a shared bgmDefaultVolume() + regression test that pins the constant against a dB check, the caption accent fix consolidates the status-role regex so build-frame.mjs and tokens.mjs can't drift, the voice-threading fix documents the flag consistently across the 3 SKILL.md copies, and the version-invention fix makes the fact-source deterministic + docs it explicitly in story-design.md. Fetch-pr's version-resolution shape (first release published at/after merge → default-branch package.json unreleased → null) is honest about what it doesn't know, and story-design.md's "no version → cite the URL only" rule closes the invention loop cleanly.
Inline findings below. Plus one meta finding that isn't line-anchorable at HEAD:
🟠 Same bug class untouched in the 4th skill copy (skills/music-to-video/scripts/assemble-index.mjs:159 — not in this PR's diff). That file still reads const vol = voiceCount > 0 ? 0.8 : 0.9; — the exact −2 dB pre-fix constant. PR body handwaves it ("music-to-video untouched"), but the code here still tracks voiceCount and gates BGM off it — so if a music-to-video piece ever ships with a voice track (SKILL.md says no narration, but the code allows it), the same −16 dB BGM regression hits here. Two options: apply the same fix (fastest, keeps parity), or delete the voiceCount > 0 ? branch entirely if music-to-video genuinely can't have voices (the cleaner narrow-titled-bugfix root-cause fix). Current middle-ground leaves a repro-in-waiting.
CI green.
| v !== ink && | ||
| v !== canvas && | ||
| !UA_DEFAULT_COLORS.has(String(v).toUpperCase()) && | ||
| !STATUS_ROLE_KEY.test(k), // a status red/green carries meaning by hue — never an accent |
There was a problem hiding this comment.
🟠 pickAccent (line 99, sibling function not in this diff) has the same status-color-wins-by-chroma bug that this PR fixes here in semanticColors. pickAccent's fallback path at .sort((x, y) => chroma(y) - chroma(x)) (no colorStats provided) ranks by raw chroma with exclude accepting only palette values, not role-keys — so a preset carrying #dc2626 in a status-red slot still wins over a lower-chroma brand accent through that path.
With colorStats present the roles(s) - roles(x) primary sort partially mitigates (a status red in only a single interactive slot loses to a brand accent in 3+ roles), but the fallback is exactly the shape semanticColors had before this PR.
Suggest either (a) accept a keyed palette in pickAccent and apply STATUS_ROLE_KEY.test(k) to the same filter, or (b) have callers pre-filter the palette before passing. Applies identically across all 3 skill copies of tokens.mjs.
— Review by Rames D Jusso
| const vol = audio.bgm.volume != null ? audio.bgm.volume : voiceCount > 0 ? 0.8 : 0.9; | ||
| // An explicit volume from audio_meta always wins. Otherwise BGM under | ||
| // narration is a bed (0.12 ≈ -18 dB); a silent film sits it forward at 0.9. | ||
| const vol = audio.bgm.volume != null ? audio.bgm.volume : voiceCount > 0 ? 0.12 : 0.9; |
There was a problem hiding this comment.
🟡 Bare 0.12 / 0.9 literals in the fallback path duplicate the shared constants. BGM_BED_VOLUME / BGM_SILENT_VOLUME / bgmDefaultVolume(hasVoice) live in ../../../media-use/audio/scripts/lib/bgm.mjs and were introduced by this same PR. When audio_meta.json doesn't provide volume, this fallback synthesizes the same default with literal duplicates — so a future tune of the shared constant (say -18 dB → -20 dB) drifts silently in the fallback path.
Suggest importing bgmDefaultVolume (or the BGM_BED_VOLUME/BGM_SILENT_VOLUME constants) here and calling audio.bgm.volume ?? bgmDefaultVolume(voiceCount > 0). Applies identically at line 393 in all 3 skill copies (faceless-explainer / pr-to-video / product-launch-video).
— Review by Rames D Jusso
| // chromatic color in a palette (e.g. #dc2626 chroma 182 beats a deep-blue accent #1E40AF chroma | ||
| // 145) and would otherwise win a pure chroma ranking, painting captions/highlights the error red. | ||
| // build-frame.mjs uses this same key set to protect status colors during the preset→brand remix. | ||
| export const STATUS_ROLE_KEY = |
There was a problem hiding this comment.
🟡 STATUS_ROLE_KEY regex scope — worth confirming the intentional exclusions. The regex covers positive|negative|success|error|warning|danger|good|bad|up|down, but a few common design-token status roles aren't in there: info, neutral, alert, caution, critical. These are also hue-carries-meaning colors (blue info, amber caution, red critical) that a preset would encode as design tokens and that the fix's premise says shouldn't be repainted as brand accents.
Question: are these excluded on purpose (they don't show up in your validation corpus so scope-narrow the regex), or oversight? If oversight, extending the regex is cheap and consistent with the fix's thesis.
— Review by Rames D Jusso
| ``` | ||
|
|
||
| `volume` is 0.8 under narration, 0.9 for a silent film (no voice). `bgm_pending` is `false` — the file is on disk when the engine returns. | ||
| `volume` is 0.12 (≈ -18 dB — a bed under the voice) under narration, 0.9 for a silent film (no voice). An explicit `volume` in `audio_meta.json` always overrides this default. `bgm_pending` is `false` — the file is on disk when the engine returns. |
There was a problem hiding this comment.
🟡 Doc has bare 0.12 / 0.9 values — same drift class as the fallback literals in assemble-index.mjs. When you tune the shared constants in bgm.mjs, this prose goes stale silently.
Optional: phrase as "the pipeline's BGM_BED_VOLUME default (currently 0.12 ≈ -18 dB)" so the linked-constant framing survives future tuning without another docs PR.
— Review by Rames D Jusso
Summary
The behavior fixes from the prompt-guide validation campaign, split out of #2109 (which now carries only the doc-text updates). Tier 3 block bugs remain tracked in #2107.
BGM level (~16 dB too hot)
Default BGM under narration was
0.8linear (~−2 dB) — measured 1.9 dB separation from voice. Now0.12(~−18 dB bed, 18.4 dB separation) via a sharedbgmDefaultVolume()in the media-use engine plus theassemble-index.mjsfallbacks in faceless-explainer / pr-to-video / product-launch-video. Explicitvolumeinaudio_meta.jsonstill wins; silent-film stays 0.9; music-to-video untouched. Addsbgm.test.mjs;references/bgm.mdupdated to match.Caption accent mapped to status red
semanticColors()ranked accent candidates by pure chroma, so a preset's preserved status red (#dc2626, chroma 182) beat a deep-blue brand accent (#1E40AF, chroma 145) and--cap-accentpainted caption highlights error-red. Status-keyed colors (positive/negative/error/warning/…) are now excluded via a sharedSTATUS_ROLE_KEYregex consumed by bothtokens.mjsandbuild-frame.mjs, so the two mappings can't drift. Fixed identically across all three duplicated skill copies.--voicenever threaded from the user's askThe engine already threads
--voiceend-to-end, but the three workflow SKILL.md command templates never passed it — "a male voice" was silently ignored and the default (Marcia /am_michael) always won. Step 3.1 now instructs choosing the voice from the request and passing--voice <id>, with provider-specific lookup instructions. Live-verified against HeyGen (default vs explicit voice both confirmed).PR videos invented version numbers
fetch-pr.mjsnow resolves a best-effortshipped_version+version_sourcefor MERGED PRs (first non-draft release published at/after the merge; fallback: default-branchpackage.json, marked unreleased).ingest.mjssurfaces it as aShipped in:brief line, andstory-design.mdforbids naming a version when none is resolvable. Verified: #1928 → v0.7.38, #2092 → v0.7.45.Test plan
node --test skills/media-use/audio/scripts/lib/bgm.test.mjs— 3/3semanticColors()re-run against the exact remix scenario — accent now#1E40AF, not#dc2626volume=filter (1.9 dB before → 18.4 dB after; explicit override honored)fetch-pr.mjs→ingest.mjspipeline against merged PRs perf(engine,producer): batch N drawElement frames per CDP round-trip (HF_DE_BATCH) #1928 / feat(sdk): export getRootElements/isNewHostBoundary/bareId, fix relative data-start #2092🤖 Generated with Claude Code