fix(media-use): never replace a voice, music or sound file the person put in the project

[miguel-heygen]

Stacked on #4648 (its base branch); review that first.

What

media-use's audio engine wrote to fixed names in the project and replaced whatever was there: a voice line to assets/voice/<line id>.wav, retrieved music to assets/bgm/track.mp3, locally made music to assets/bgm/track.wav, and a HeyGen sound effect to assets/sfx/<name>.mp3. A person's own assets/sfx/whoosh.mp3 or assets/voice/intro.wav was silently overwritten.

Now the engine never replaces a file the person put there:

• If the target name is free, or the project's media manifest records the file there as made by the agent (generated, search or bundled), the engine writes there as before, so a rerun still replaces its own takes.
• Otherwise it writes to the first free name, whoosh-2.mp3, whoosh-3.mp3, and so on, records that file, and reports the switch as an anomaly: assets/sfx/whoosh.mp3: kept, because the file there is yours ...; writing assets/sfx/whoosh-2.mp3 instead (audio_meta.json has the path used). A second run reuses the same -2 file, because it is now recorded as the agent's.
• Voice paths are all picked before lines synthesize in parallel, so two lines never land on one free name (ids hook and hook-2 with a person's hook.wav get hook-2.wav and hook-2-2.wav).
• One effect named by two cues shares one file; two different effects never do, even when one's own name is another's -2 (glitch and glitch 2 beside a person's glitch.mp3 get glitch-2.mp3 and glitch-2-2.mp3).
• A copy from the bundled sound library with no manifest record is reused when it is byte-for-byte the library file (an engine older than fix(media-use): record agent-made voice, music and sound effects in the media manifest #4648 made it). A file recorded as the person's own is never reused, even with the library's bytes.

The rule that decides what counts as agent-made (AGENT_SOURCES) now lives in the manifest library next to the records, and resolve --source uses the same list. The tts, bgm and sfx references say where a file lands when the fixed name is taken.

Also fixed: generating music again over the engine's own track.wav left the old file in place until the new one was written, and wait-bgm treats any file there as finished, so it reported the old track as ready at once. The old track is now removed right before the generator starts.

Upgrade note

Files written by an engine older than #4648 carry no manifest record, so they count as the person's own: the first run after upgrading writes -2 names beside them, leaves the old files in place and says so in the anomalies. The run's audio_meta.json names the files it used.

Known limits

• Ownership is read from the manifest only. A person who drops their own file over an engine-recorded name gets it replaced on the next run, since records carry no size or hash.
• Regenerating music while an earlier generator is still running is not handled here; wait-bgm can still report that run's output.
• With the library file missing, a HeyGen download at the library name stands in and is re-recorded as bundled.

Tests

• skills, sound effects: a person's own assets/sfx/whoosh.mp3 survives unchanged, the cue lands on assets/sfx/whoosh-2.mp3 with the library's bytes, and a second run reuses that file. Fails on the base.
• skills, the Gemini engine pipeline test (the real engine with fixture providers): with a person's own assets/voice/intro.wav present, the voice line is written to assets/voice/intro-2.wav, recorded as generated, and the person's file survives. Fails on the base.
• skills, agentWritePath: a free name is used; a person's file, and a file recorded as existing, move the write to -2 with one anomaly each; a recorded agent file and a reusable file are replaced silently; a person's -2 moves it to -3; an agent-made -2 is reused.
• skills, voicePaths: two lines never share a file when one's name is taken by the person.
• skills, HeyGen sound effects (fake fetch): two effects never share a file when one's name is taken by the person, on the first run and on a rerun where the -2 is already recorded as the engine's; a repeated effect reuses its file. The bundled branch uses the same rule without a test of its own (it needs a library file literally named <other>-2).
• skills, bundled sound effects: the engine's earlier copy stands in for a library file this install lacks; a person's file with the library's bytes, recorded as their own, gets -2.
• skills, music: generating again removes the engine's old track.wav before the generator runs (a fake python3 stands in for the model; skipped on Windows).

Each rule's test fails with that rule removed (8 mutations, all caught).

[somanshreddy]

Verdict: COMMENT (holding, not approving) — see gate note at the bottom.

Reviewed at de003bcc (base: fix/media-use-audio-records @ bf463a0c, i.e. #4648's current head).

What I did

• Read the full diff against its actual base (base-4648...pr-4665), not against main — this PR is stacked on fix(media-use): record agent-made voice, music and sound effects in the media manifest #4648.
• Read agentWritePath, voicePaths, resolveSfx, generateBgmDetached/retrieveBgm, and recordInManifest in full, and traced every caller of the collision-avoidance path.
• Ran the full relevant test surface locally (Node's built-in node:test, not vitest/bun:test — these files use node:test + node:assert): media-record.test.mjs, sfx.test.mjs, bgm.test.mjs, audio.test.mjs, gemini-pipeline.test.mjs, check-media-use-copy-parity.test.mjs. 25 + 8 + 3 = all green.
• Mutation-tested independently (not just trusting the PR body's "8 mutations, all caught" claim): gutted agentWritePath's free() guard down to !taken.has(path) (i.e. reverted to "always allowed to overwrite"). 7 of 25 tests in media-record.test.mjs/sfx.test.mjs failed, including the two I'd have picked myself (voicePaths collision, and the byte-identical-library-copy reuse case) — confirms the suite actually pins the protection logic rather than mock-theater. Restored the real code after.
• Traced the collision math by hand for the PR body's own examples (hook/hook-2 beside a person's hook.wav; glitch/glitch 2 beside a person's glitch.mp3) against the actual taken-set threading in voicePaths and resolveSfx — both check out exactly as described.
• Verified wait-bgm.mjs already reads audioMeta.bgm?.path (not a hardcoded track.wav), so the new agentWritePath-diverted BGM path threads through correctly without needing a change there.
• Verified the rmSync(abs, {force:true}) fix for the stale-track.wav race operates on the (possibly-diverted) rel computed after the ownership check, so it can never touch a person's own file — only the engine's own prior take at whatever path it was actually assigned.
• Confirmed both manifest.mjs copies (packages/cli/src/media-use/lib/ and skills/media-use/scripts/lib/) got the identical AGENT_SOURCES addition, and the copy-parity test passes.
• As a side effect, this PR also appears to resolve fix(media-use): record agent-made voice, music and sound effects in the media manifest #4648 review finding 5 (SFX source label disagreeing with the manifest when a later offline run's file differs from the library) — since ownership is now resolved before the write, the own ? "project" : "local" branch that produced the mismatch is gone entirely in favor of unconditional "local". Not claimed in the PR body; worth a one-line mention there but not blocking.

Findings

None blocking on this PR's own diff. The design (manifest-recorded ownership, -2-name fallback, pre-computed taken sets for the two must-not-collide code paths, reusable() for the byte-identical-library-copy case) is sound and the tests genuinely exercise it.

Gate — why I'm not approving right now

This PR's base is fix/media-use-audio-records, which is #4648, and #4648 currently carries an open, unresolved CHANGES_REQUESTED from @jrusso1020 (resolve --candidates and .media/index.md still surface a replaced record's stale text/duration pointing at the current audio — reviewed twice, still blocking as of the bf463a0c re-review). This PR doesn't touch candidates.mjs or index-gen.mjs, so it doesn't fix or worsen that issue, but "one approval on this head clears the gate; the loop presses merge" would merge this PR's commits into #4648's branch — moving #4648's head out from under an in-progress, still-blocking review. I'd rather hold this one approval than have it trigger that.

Once #4648 clears Rames's finding 1, I'll re-review at whatever new head results (should be fast — my findings here don't change) and can approve then.

[somanshreddy]

Upgrading my earlier comment to a formal REQUEST_CHANGES — not because the diff has a defect (it doesn't; see my full review above), but because on a "one approval clears the gate, the loop presses merge" lane, a plain comment doesn't hold the gate against a different reviewer approving without seeing this thread. That's exactly the failure mode from hyperframes#4054: a comment classified "hold, don't merge" still lets a stray approval clear an auto-merge gate.

The hold, restated: #4665's base branch is #4648, which carries an open, unresolved CHANGES_REQUESTED (Rames, bf463a0c). Merging #4665 now would move #4648's head mid-review. Once #4648 clears, I'll switch this to APPROVE immediately — my technical review above already stands and won't need to be redone.

[somanshreddy]

Verdict: APPROVE — hold cleared.

My hold on this PR was never about its own content (see my earlier full review, thoroughly verified and mutation-tested) — it was that its base, #4648, carried an open CHANGES_REQUESTED, and merging this PR would have moved #4648's head mid-review. Rames has since cleared that CR at #4648's dd78781e (reviewDecision: APPROVED).

Re-verified at this exact head (ac0edee9) rather than just trusting the dispatch label:

• #4665's own production source is byte-identical to the de003bcc head I already fully reviewed and mutation-tested (git diff de003bcc...ac0edee9 on every file in this PR's own scope, excluding one test file, is empty).
• The only change within #4665's own scope is a small, positive test strengthening in media-record.test.mjs: a new assertion that a re-recorded take with a new duration is a new record, and a new assertion that .media/index.md reflects the current take's text, not a stale one. This directly exercises the integration with #4648's now-fixed currentRecords/index-gen behavior — a genuine strengthening, not a regression.
• Everything else in the raw head-to-head diff (candidates.mjs, both index-gen.mjs copies, mediaIndex.ts) is inherited from the rebased base, not authored by this PR — confirmed against gh pr view --json files's base-relative scope before attributing anything to this PR.

CI green on all 15 checks. Approving.

The base branch was changed.

[somanshreddy]

Verdict: APPROVE — re-verified at the rebased head, not just re-stamped.

At c512878e (rebased onto main after #4648 merged, resolving the conflict I flagged earlier). Diffed every file in this PR's own scope (gh pr view --json files) between my last-approved head (ac0edee9) and this one: the only difference is skills-manifest.json's auto-generated hashes for hyperframes, hyperframes-cli, hyperframes-creative and hyperframes-studio — all unrelated skills whose content shifted from picking up other main-merged PRs during the rebase. media-use's own hash (the actual subject of this PR) is byte-identical: 0744050e3114e60a on both heads. No production or test code changed.

CI green on 57 of 60 checks, 3 still in progress (Windows engine-cli/studio-2, CodeQL), none failed.