Skip to content

Commit f0cc9b1

Browse files
vanceingallsclaude
andauthored
fix(skills): make the carve CLI work against the published core, and honour its own group invariant (#3416)
* fix(skills): make the carve CLI work against the published core, and honour its own group invariant Two defects found by using the shipped feature end to end on a real project rather than inside this repo. **It could not load core at all.** `loadCore` resolved `./audio-carve` and `./audio-fx` with `require.resolve`. The workspace manifest declares a `node` condition, so that resolved fine here — but the PUBLISHED manifest (`publishConfig.exports`) carries only `import` + `types`, so every consumer of the released package got ERR_PACKAGE_PATH_NOT_EXPORTED for a package that ships those files perfectly well. The script was broken everywhere except where it was developed, and its error text blamed a missing/outdated package, which no install can fix. It now keeps the project anchor and falls back to the manifest's declared `import` target. **It violated the invariant its own SKILL.md sets.** SKILL.md is explicit: "A carve against more than one clip id is wrong. Group the clips and carve against the group. This is an invariant, not a tip." The script wrote `sources: voices.map((v) => v.id)` unconditionally, so every run against grouped voices produced output that tripped the repo's own `audio_carve_ungrouped_sources` lint rule, and a voice added to the group later would silently play outside the carve's awareness. When every voice shares one group it now records the group; mixed, partially grouped or ungrouped voices keep their ids so the lint rule still fires on the case it is meant to catch. `main()` moves behind an entry guard so the pure helper can be imported and tested; `node carve.mjs` is unaffected (verified against a real composition). Six tests, and the manifest hash is regenerated for the changed skill. * fix(skills): run the carve CLI through symlinks, and keep the bed out of its own sources Two blockers from review, both of the class this PR's first fix was about: correct where it was developed, broken for the audience it ships to. **The entry guard silently skipped `main()` through any symlinked path.** `process.argv[1]` keeps the spelling the caller typed while `import.meta.url` is derived from the realpath, because node resolves the main module's symlinks. So the raw compare added to make the helpers importable turned the CLI into a no-op that wrote nothing and exited 0. Reachable with no symlink of one's own: on macOS `/tmp` is a link to `/private/tmp`, and `SKILL.md` documents the entry point as `node <SKILL_DIR>/scripts/carve.mjs`, so any install placed behind a link breaks too. Reproduced against the published core by a reviewer, not only inferred. Fixed by realpathing the left side. This repo already documents and solves the same trap in three scripts (`frame-packets-core.mjs`, `preflight.mjs`, `project-dir.mjs`); the canonical comment is carried over verbatim. A local copy rather than an import, because skills install independently — `hyperframes-audio` has no dependency on `hyperframes-core` being present. **`carveSources` could make the bed its own carve source.** It decided from the voices alone, so a bed sharing their group (`mix`) got `sources: ["mix"]` written onto it. `resolveCarveSourceIds` expands a group id to every current member and takes no host element to exclude, so the next analysis in Studio hands the bed to itself and the duck envelope fights the bed's own content instead of speech — the "never carve a track against itself" invariant, arriving one re-analysis after a first pass that was genuinely correct (`main()` sums the detected voices directly and never round-trips through group resolution, which is why the PR's own end-to-end check could not catch it). The fix is at the call site, not in the resolver: neither `resolveCarveSourceIds` nor `resolveCarveVoices` receives the host, so "make the resolver skip the target" would be a signature change on shared core. `carveSources(voices, bed)` declines the group form when the bed is a member and records clip ids, which is exactly what `audio_carve_ungrouped_sources` exists to raise — plus a stderr note saying why, so the lint message does not read as "group clips you already grouped". Scoped to `<audio>` beds: group membership is audio-only, so a `<video>` bed cannot be pulled in by an expansion and declining there would be a false positive. SKILL.md now states the constraint next to the group invariant it belongs to. Tests: six added, closing both gaps review named. The bed-in-group regression and a symlinked CLI invocation both fail on the previous commit (silent exit 0 vs the usage error) and pass now; three more pin the cases that must NOT decline (different group, ungrouped bed, video bed). `loadCore` is now exported and covered by a fixture package carrying an import-only export map — the published manifest's shape — so this PR's first fix is pinned without depending on npm. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(skills): refuse the carve group when a non-voice member would widen it Closes the second branch of the original blocker, which the bed fix did not cover: detected voices sharing `voiceover` with an existing SFX or music member. Detection correctly leaves that member out, but the persisted `sources: ["voiceover"]` resolves wider on the next Studio analysis — `resolveCarveSourceIds` expands the group to every current member and `resolveCarveVoices` keeps any audio with a src — so the extra clip enters the sidechain and the bed starts ducking under a whoosh. Same shape as the bed case: the first pass is genuinely correct because `main()` sums the voice list `detectTracks` returned and never round-trips through group resolution. Taking the first of the two suggested fixes (membership + classification in the collapse decision) rather than deriving the first pass from the resolved group: analysing whatever the group happens to hold would make the CLI measure clips it classified as non-voice, which is the arrangement problem rather than a licence to sidechain them. `groupSourceRefusal(voices, bed, members)` replaces `bedInVoiceGroup` and returns `{group, reason, ids}` or null, so the decision and the stderr note come from one place. `members` is every `<audio>` in the composition as `{id, group, nameKind}` with `nameKind` from core's `classifyAudioName`, so this and Studio's picker classify identically. `detectTracks` now returns the media list it already built. Classification, not membership, is what makes this safe. A member classified `music` or `sfx` blocks the group; a member classified `voice` or `unknown` does not. That distinction is load-bearing: `detectTracks` only analyses voices that overlap the bed, so an outro line that starts after the bed ends is routinely a group member this run did not measure — and covering it on a later analysis without editing `sources` is the entire reason SKILL.md says to name the group. Refusing on "any member the run did not analyse" would collapse the group form into clip ids for every ordinary narration sequence. `unknown` follows detection's own loose-in-the-safe-direction rule, since detection treats an unknown name as a possible voice. The note now names the blocking member, for either reason, since "sources are clip ids" plus `audio_carve_ungrouped_sources` reads as nonsense to an author who did group their clips. Tests: six added, 18 in the file. The two regressions (sfx member, music member) and the refusal shape fail with the mixed branch ablated and pass with it; three more pin the cases that must NOT refuse — a non-overlapping voice member, an `unknown` member, and an sfx member of a different group. SKILL.md states both refusals and the voice-member exemption next to the group invariant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(skills): make `members` required so dropping it cannot undo the widening fix Review finding, and the one link no test covered. `carveSources` and `groupSourceRefusal` defaulted `members = []`, and with an empty list the `mixed` refusal cannot fire — so a refactor that dropped the third argument at the call site would return the group form again with the entire suite green. That is the same signature as the bug the argument exists to prevent: `main()` sums the detected voice list directly, so the first CLI pass is correct either way and only a later Studio re-analysis reads the widened attribute. Nothing goes red. `main()` is also the only code that BUILDS `members`, and no test runs it — the symlink test stops at the usage error and a real run needs ffmpeg. Both defaults are gone, so a missing argument throws on `members.filter`. The nine cases that predate the membership check now pass `[]` explicitly, which also documents that they are about the bed and the group attributes alone, and a new test asserts both functions throw when the argument is omitted. Verified it fails when the defaults are restored. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 8612cfd commit f0cc9b1

4 files changed

Lines changed: 464 additions & 20 deletions

File tree

skills-manifest.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,8 @@
2626
"files": 121
2727
},
2828
"hyperframes-audio": {
29-
"hash": "819ab4e70d0f1cc9",
30-
"files": 6
29+
"hash": "5564aeda7725f455",
30+
"files": 7
3131
},
3232
"hyperframes-cli": {
3333
"hash": "3fa884269c43d7df",

skills/hyperframes-audio/SKILL.md

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,34 @@ A `sources` list naming two or more plain clip ids instead of a group is caught
261261
by the `audio_carve_ungrouped_sources` lint rule — it still works, but it is the
262262
version that silently rots when a clip is added.
263263

264+
**Keep the carve group a voice group: no bed, no SFX, no music.** A group id in
265+
`sources` resolves to every _current_ member on _every_ analysis, so the group
266+
you name is the group you get later — not the tracks that were measured when it
267+
was written. Two ways that bites:
268+
269+
- **The bed in its own source group.** It is handed to itself as a voice and
270+
carved against its own content — the "never carve a track against itself" rule
271+
arriving one re-analysis later.
272+
- **An SFX or music clip in the voice group.** It enters the sidechain on the
273+
next analysis and the bed starts ducking under a whoosh, even though the run
274+
that wrote the attribute never measured it.
275+
276+
Both are invisible at the moment the carve is written: the analysis sums the
277+
voices it detected and never round-trips through group resolution, so the first
278+
pass is genuinely correct and only the next one is wrong. So give each role its
279+
own group — `music` for the bed, `voiceover` for the narration, `sfx` for the
280+
hits — and keep the group named in `sources` holding nothing but voices.
281+
282+
`carve.mjs` refuses to write the group form when it sees either case, records
283+
clip ids, and says on stderr which member blocked it. The
284+
`audio_carve_ungrouped_sources` rule then points at the arrangement instead of
285+
the CLI quietly persisting a wider carve than it measured.
286+
287+
A voice that this run left out is **not** one of these cases and does not block
288+
the group form: `carve.mjs` only analyses voices that overlap the bed, and
289+
picking up a clip that plays later without an edit to `sources` is the whole
290+
reason to name the group.
291+
264292
**One knob.** `strength` is 0..1 and derives everything: how deep to cut, how
265293
many bands, how wide, how far to favour intelligibility over raw voice energy,
266294
how far the level may drop, how far under the voice to aim. Those six move

skills/hyperframes-audio/scripts/carve.mjs

Lines changed: 195 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626

2727
import { execFileSync } from "node:child_process";
2828
import { createRequire } from "node:module";
29-
import { readFileSync, writeFileSync } from "node:fs";
29+
import { readFileSync, realpathSync, writeFileSync } from "node:fs";
3030
import { dirname, resolve } from "node:path";
3131
import { pathToFileURL } from "node:url";
3232

@@ -81,28 +81,160 @@ function fail(message, code = 1) {
8181
* the skill was installed — a sibling of the composition is what has the
8282
* dependency.
8383
*/
84-
async function loadCore(fromDir) {
84+
export async function loadCore(fromDir) {
8585
const require = createRequire(pathToFileURL(resolve(fromDir, "package.json")));
86-
const load = (subpath) => {
87-
const file = require.resolve(`@hyperframes/core/${subpath}`);
88-
return import(pathToFileURL(file).href);
86+
87+
/*
88+
* Two constraints at once, and satisfying either alone is broken:
89+
*
90+
* 1. Anchored at the PROJECT, not at this script. This file lives wherever
91+
* the skill was installed, which has no @hyperframes/core; the
92+
* composition's project is what holds the dependency. So a bare
93+
* `import("@hyperframes/core/audio-carve")` from here cannot work — bare
94+
* specifiers resolve relative to the importing module.
95+
* 2. Honouring the package's export CONDITIONS. `require.resolve` asks for
96+
* "require"/"node". The workspace manifest declares `node`, so this
97+
* resolved fine inside the monorepo — but the PUBLISHED manifest carries
98+
* only `import` + `types`, so every consumer of the released package got
99+
* ERR_PACKAGE_PATH_NOT_EXPORTED for a package that ships the file. That
100+
* is the audience this skill is shipped to, so the script was broken
101+
* everywhere except where it was developed.
102+
*
103+
* Keep the project anchor; fall back to the package's declared `import`
104+
* target when no require-resolvable condition exists.
105+
*/
106+
const load = async (subpath) => {
107+
const spec = `@hyperframes/core/${subpath}`;
108+
try {
109+
return await import(pathToFileURL(require.resolve(spec)).href);
110+
} catch (error) {
111+
if (error?.code !== "ERR_PACKAGE_PATH_NOT_EXPORTED") throw error;
112+
// `./package.json` is exported by every manifest, so this always resolves
113+
// and gives us the package root without guessing at node_modules layout.
114+
const pkgPath = require.resolve("@hyperframes/core/package.json");
115+
const pkg = JSON.parse(readFileSync(pkgPath, "utf-8"));
116+
const entry = pkg.exports?.[`./${subpath}`];
117+
const target = typeof entry === "string" ? entry : (entry?.import ?? entry?.default ?? null);
118+
if (!target) {
119+
fail(
120+
`@hyperframes/core does not export ./${subpath}\n` +
121+
` found at: ${pkgPath} (version ${pkg.version})\n` +
122+
` update it: npm i -D @hyperframes/core`,
123+
);
124+
}
125+
return import(pathToFileURL(resolve(dirname(pkgPath), target)).href);
126+
}
89127
};
128+
90129
try {
91130
return {
92131
carve: await load("audio-carve"),
93132
fx: await load("audio-fx"),
94133
};
95134
} catch (error) {
96135
fail(
97-
`cannot resolve @hyperframes/core from ${fromDir}\n` +
98-
` install or update it: npm i -D @hyperframes/core\n` +
99-
` (the audio-carve export needs a version that ships the carve analysis)\n` +
136+
`cannot load @hyperframes/core from ${fromDir}\n` +
137+
` is it installed there? npm i -D @hyperframes/core\n` +
100138
` or point at one: --core <dir containing node_modules/@hyperframes/core>\n` +
101-
` (${error.message})`,
139+
` (${error.code ?? "error"}: ${error.message.split("\n")[0]})`,
102140
);
103141
}
104142
}
105143

144+
/**
145+
* The `sources` a carve should record for these voices, on this bed.
146+
*
147+
* SKILL.md states the invariant: "A carve against more than one clip id is
148+
* wrong. Group the clips and carve against the group." Naming the group lets
149+
* `resolveCarveSourceIds` resolve membership at analysis time, so a voice added
150+
* later is covered without editing `sources` — whereas a list of clip ids rots
151+
* silently the moment a fourth narration clip appears. The lint rule
152+
* `audio_carve_ungrouped_sources` enforces exactly this.
153+
*
154+
* This script was writing clip ids unconditionally, so it violated its own
155+
* skill's invariant and tripped its own lint rule on every run. When every
156+
* voice shares one group, record the group. Mixed or ungrouped voices keep
157+
* their ids, and the lint rule then correctly tells the author to group them.
158+
*
159+
* The bed has to be part of the decision, because the group form resolves
160+
* LATER and wider than it looks. If the bed is itself a member of the voices'
161+
* group, `resolveCarveSourceIds` expands that id to every current member on the
162+
* next analysis — including the bed — and the bed ends up carved against
163+
* itself, which SKILL.md calls a bug rather than a mix choice. This run cannot
164+
* see it: `main()` sums the voice list it detected and never round-trips
165+
* through group resolution, so the first pass is correct and only the next
166+
* re-analysis in Studio is wrong. So decline the group form there and fall back
167+
* to clip ids, which is exactly the case `audio_carve_ungrouped_sources` exists
168+
* to put in front of the author.
169+
*
170+
* Only an `<audio>` bed can trip it: group membership is audio-only
171+
* (`audioGroupOf`), so `data-audio-group` on a `<video>` bed is ignored by core
172+
* and expanding a group can never pull it in.
173+
*/
174+
export function carveSources(voices, bed, members) {
175+
const group = sharedVoiceGroup(voices);
176+
return group && !groupSourceRefusal(voices, bed, members) ? [group] : voices.map((v) => v.id);
177+
}
178+
179+
/** The one group every voice belongs to, or null if they do not share exactly one. */
180+
function sharedVoiceGroup(voices) {
181+
const groups = voices.map((v) => attrOf(v.tag, "data-audio-group"));
182+
const first = groups[0];
183+
return Boolean(first) && groups.every((g) => g === first) ? first : null;
184+
}
185+
186+
/**
187+
* Why naming the voices' shared group would persist something this run did not
188+
* analyse — or null when the group is safe to name.
189+
*
190+
* `members` is every `<audio>` in the composition as `{id, group, nameKind}`,
191+
* with `nameKind` from core's `classifyAudioName`, so this and Studio's picker
192+
* classify the same way.
193+
*
194+
* Required, deliberately not defaulting to `[]`. With an empty list the `mixed`
195+
* refusal below cannot fire, so a call that forgot the argument would return the
196+
* group form and restore the exact behaviour this function exists to prevent —
197+
* silently, because the first CLI pass is correct either way and only a later
198+
* Studio re-analysis is wrong. A missing argument throws on `members.filter`
199+
* instead.
200+
*
201+
* Two refusals, and both exist because the group form resolves LATER and WIDER
202+
* than the analysis: `resolveCarveSourceIds` expands a group id to every current
203+
* member on every analysis, and `resolveCarveVoices` keeps any audio member with
204+
* a src. `main()` meanwhile sums the voice list `detectTracks` returned, so the
205+
* first pass looks correct however wrong the persisted attribute is.
206+
*
207+
* `bed` — the bed is a member, so it would be handed to itself as a voice
208+
* and carved against its own content.
209+
* `mixed` — a member classified music or sfx is not a voice this run measured,
210+
* so it would enter the sidechain on the next analysis and duck the
211+
* bed under a whoosh.
212+
*
213+
* Deliberately NOT a refusal: a member classified `voice` or `unknown` that this
214+
* run left out. That is the group form working as designed — `detectTracks` only
215+
* takes voices that overlap the bed, and picking up a clip that starts playing
216+
* later without an edit to `sources` is the whole reason SKILL.md says to name
217+
* the group. Refusing there would collapse the group form into clip ids for
218+
* every ordinary narration sequence.
219+
*/
220+
export function groupSourceRefusal(voices, bed, members) {
221+
const group = sharedVoiceGroup(voices);
222+
if (!group) return null;
223+
if (bed?.kind === "audio" && attrOf(bed.tag, "data-audio-group") === group) {
224+
return { group, reason: "bed", ids: [bed.id] };
225+
}
226+
const analysed = new Set(voices.map((v) => v.id));
227+
const strays = members
228+
.filter(
229+
(m) =>
230+
m.group === group &&
231+
!analysed.has(m.id) &&
232+
(m.nameKind === "music" || m.nameKind === "sfx"),
233+
)
234+
.map((m) => m.id);
235+
return strays.length > 0 ? { group, reason: "mixed", ids: strays } : null;
236+
}
237+
106238
/** Mono float PCM for one media file, via ffmpeg. */
107239
function decode(path) {
108240
let raw;
@@ -234,7 +366,7 @@ function detectTracks(html, given, classify, overlaps) {
234366
` name one with --voice`,
235367
);
236368
}
237-
return { bed, voices: usable };
369+
return { bed, voices: usable, all };
238370
}
239371

240372
const startOf = (tag) => {
@@ -249,12 +381,21 @@ async function main() {
249381
const { carve: carveApi, fx: fxApi } = await loadCore(args.core ? resolve(args.core) : compDir);
250382

251383
const html = readFileSync(compPath, "utf-8");
252-
const { bed: bedEl, voices } = detectTracks(
253-
html,
254-
args,
255-
carveApi.classifyAudioName,
256-
carveApi.clipsOverlap,
257-
);
384+
const {
385+
bed: bedEl,
386+
voices,
387+
all: media,
388+
} = detectTracks(html, args, carveApi.classifyAudioName, carveApi.clipsOverlap);
389+
// Group membership + name classification for every audio track, so the source
390+
// decision can see what the group will resolve to later and not just what this
391+
// run analysed.
392+
const members = media
393+
.filter((el) => el.kind === "audio")
394+
.map((el) => ({
395+
id: el.id,
396+
group: attrOf(el.tag, "data-audio-group"),
397+
nameKind: carveApi.classifyAudioName(el.id, unescapeAttr(attrOf(el.tag, "src") ?? "")),
398+
}));
258399
const bedTag = bedEl.tag;
259400
const bedSrc = attrOf(bedTag, "src");
260401
process.stdout.write(
@@ -354,7 +495,26 @@ async function main() {
354495
: [];
355496
const lanes = [...carriedLanes, ...carvedLanes];
356497

357-
const settings = { enabled: true, sources: voices.map((v) => v.id), strength: args.strength };
498+
const settings = {
499+
enabled: true,
500+
sources: carveSources(voices, bedEl, members),
501+
strength: args.strength,
502+
};
503+
// Say why the group form was declined, or the lint rule tells the author to
504+
// group clips they have already grouped.
505+
const refusal = groupSourceRefusal(voices, bedEl, members);
506+
if (refusal) {
507+
process.stderr.write(
508+
refusal.reason === "bed"
509+
? `note bed ${bedEl.id} is in group "${refusal.group}" with the voices, so\n` +
510+
` sources are clip ids: naming that group would carve the bed\n` +
511+
` against itself on the next analysis. Move the bed to its own group.\n`
512+
: `note group "${refusal.group}" also holds ${refusal.ids.join(", ")}, which this run\n` +
513+
` did not analyse (music/sfx by name), so sources are clip ids: naming\n` +
514+
` the group would pull them into the sidechain on the next analysis.\n` +
515+
` Move them out of the voice group.\n`,
516+
);
517+
}
358518
const written =
359519
` data-fx-carve="${escapeAttr(JSON.stringify(settings))}"` +
360520
` data-fx-chain="${escapeAttr(fxApi.serializeAudioFxChain(chain))}"` +
@@ -389,4 +549,21 @@ async function main() {
389549
process.stdout.write(`wrote ${args.comp} (id="${bedEl.id}")\n`);
390550
}
391551

392-
await main();
552+
// Only run as a CLI. Guarded so the pure helpers above can be unit-tested by
553+
// importing this module (`skills/**/*.test.mjs`, run by `bun run test:skills`).
554+
//
555+
// realpath both sides: on macOS /tmp → /private/tmp, and node resolves the main
556+
// module's symlinks in import.meta.url while argv[1] keeps the invoked spelling —
557+
// a raw compare silently skips main() when invoked through any symlinked path.
558+
function isMainModule(importMetaUrl) {
559+
if (!process.argv[1]) return false;
560+
try {
561+
return pathToFileURL(realpathSync(process.argv[1])).href === importMetaUrl;
562+
} catch {
563+
return false;
564+
}
565+
}
566+
567+
if (isMainModule(import.meta.url)) {
568+
await main();
569+
}

0 commit comments

Comments
 (0)