fix(skills): retain audio metadata file identity during generation

[jrusso1020]

Problem and change

Fixes CodeQL alert #735 (js/file-system-race). The audio engine reads existing metadata, awaits generation, then writes the output pathname. Replacing that pathname during generation could redirect the write.

Keep an existing metadata file open from the merge-base read through the final write. Write at byte zero and truncate through that descriptor, preserving existing file modes, hard links, existing symlink targets, JSON formatting, and --only merge behavior. When the merge base is absent, defer creation until saving and use exclusive wx creation.

Behavior boundaries

• A file or symlink appearing at a previously missing output now raises EEXIST instead of being overwritten or followed. A pre-existing dangling output symlink is likewise rejected. Existing symlinks to valid metadata remain supported.
• Existing output write permission is checked before generation. If generation exits before saving, process exit releases the CLI-owned descriptor; malformed JSON and final-write paths explicitly close it.
• This pins file identity, not file contents. Same-inode concurrent edits and hostile parent-directory renames are outside this fix; no atomic snapshot or filesystem sandbox is claimed.
• Only the media-use manifest fingerprint/file count changes. No workflow changes.

Validation

• Eight focused tests pass: Unicode/truncation/mode preservation, pathname replacement, symlink retargeting, deferred creation, racing files/links, malformed input, and a real CLI partial-update run.
• Full skills suite: 608 passed, 2 skipped, 0 failures (parser prerequisite built).
• oxlint, oxfmt, manifest generation and signed commit hooks pass.

[miguel-heygen]

Exact-head security/behavior review at 47813960ae922c69ef821c783d650f57ea4dbd58.

• skills/media-use/audio/scripts/lib/audio-meta.mjs:7-20 opens an existing merge base once, reads through that descriptor, and closes it on parse failure. :23-36 writes from byte zero, handles short writes, truncates the same inode, and closes on every save exit.
• The state boundary is consistent: existing valid file/symlink/hard-link → update the opened inode; malformed existing file → preserve bytes and fail; absent file → defer all output mutation until save and create once with wx; racing leaf or dangling link → EEXIST, no overwrite/follow. audio.mjs:119-120,283-284 has one owner for the handle, and CLI process exit releases it on pre-save exits.
• The focused filesystem cases cover replacement, symlink retarget, deferred creation, file/link races, mode/truncation, malformed input, and partial --only merge. They pass 8/8 with an isolated credential directory, and skills-manifest.json reproduces exactly. The disclosed same-inode edit and hostile-ancestor limits match the mechanism.

Important (nonblocking): audio-meta.test.mjs:110-124 spawns the real CLI with the entire ambient environment and default shared credential directory. On this reviewer machine, an expired ~/.heygen/credentials makes the new test fail before it reaches the metadata write (HeyGen OAuth token expired); setting an empty HEYGEN_CONFIG_DIR makes the same eight tests pass. Please make the child hermetic by overriding HEYGEN_CONFIG_DIR and blanking HEYGEN_API_KEY / HYPERFRAMES_API_KEY in its env, so contributor auth state cannot redden an otherwise unrelated filesystem test.

All relevant exact-head GitHub checks are green, including the full skills suite, manifest, Codex package, Windows bootstrap, CodeQL, and regression.

Verdict: APPROVE
Reasoning: The patch follows CodeQL’s descriptor-based recommendation and preserves the requested merge, identity, permission, mode, link, and failure semantics; the remaining issue affects test isolation, not runtime correctness.

— Magi

[jrusso1020]

Addressed the credential-isolation finding in e9d3aa8. The child now points HEYGEN_CONFIG_DIR at its private fixture and blanks HEYGEN_API_KEY/HYPERFRAMES_API_KEY. All eight tests pass with deliberately expired ambient OAuth credentials. Format/lint/manifest and signed hooks pass. Fresh review requested.

The completed CodeQL scan raised #897 (intended network metadata JSON persistence) and #898 (assertion-only fixture read). Independent classification was requested from Magi in the existing Slack review thread; neither has been dismissed and the PR will remain unmerged while these are unresolved.

[miguel-heygen]

Re-review at exact head e9d3aa8c2342467016e60e338f168337c0210a0d.

The auth-isolation note is resolved. audio-meta.test.mjs:123-131 gives the spawned CLI a fixture-local credential directory and explicitly blanks both API-key variables, so ambient files and .env values cannot steer the test. It now passes 8/8 on this machine with the same deliberately expired shared OAuth state that failed the prior head; the manifest reproduces exactly.

CodeQL #898 — recommend dismissal as used in tests. The flagged statSync / readFileSync pair at audio-meta.test.mjs:29-34 operates only on a test-authored file under an unpredictable, test-owned mkdtempSync directory (:18-21). There is no untrusted caller/path or privilege boundary; the second read is an assertion of mode/content preservation, not an authorization check followed by a sensitive pathname operation.

CodeQL #897 — recommend dismissal as false positive (benign intended persistence). audio.mjs:82-84 resolves the output solely from local CLI arguments before any network call, and :119 opens that caller-selected metadata file before generation. Network responses can supply metadata values, never the destination: TTS keeps only filtered word strings plus finite numeric times (tts.mjs:334-339), BGM/SFX keep fixed local paths and numeric durations (bgm.mjs:52-65, sfx.mjs:45-70), and audio-meta.mjs:24 encodes the final object with JSON.stringify before descriptor-only writing. A hostile quote/newline/path-like string round-tripped as JSON data and created no sibling path. This is the documented audio_meta.json output contract, not arbitrary upload or executable persistence.

I did not dismiss either alert. The current JavaScript CodeQL analysis is still running; leave that scan and any resulting review state as merge gates.

Verdict: APPROVE
Reasoning: The new head fully isolates the regression test, while both new alerts are non-security query matches: one is test-only assertion code and the other is intentional JSON metadata persistence to a destination the network cannot influence.

— Magi

[miguel-heygen]

CodeQL #737 audit at exact head e9d3aa8c2342467016e60e338f168337c0210a0d.

Source ledger

• audio.mjs:102-110 reads the explicitly supplied audio_request.json. Only the requested line text / voice / language / speed reaches the fixed /voices/speech POST (audio.mjs:127-158, tts.mjs:298-310); BGM queries and SFX cue names reach the fixed catalog search as URLSearchParams values (audio.mjs:192-213,257-266, heygen.mjs:143-146). This is the documented audio-generation contract.
• audio.mjs:119-126 reads prior audio_meta.json only as the --only merge base. If TTS runs, provider and voice are overwritten before the network call; otherwise prior values affect local retention, hasVoice, and duration, not an outbound request body.
• heygen-tts.mjs:85-103 reads a .txt only when the user explicitly supplies that file as the standalone TTS input, then sends exactly that text to speech synthesis by design.

Destination/control audit

heygen.mjs:11,109-116 hard-codes the HTTPS origin to https://api.heygen.com/v3. All heygenJSON call sites use fixed route literals; catalog search interpolates only URLSearchParams, and speech data is encoded with JSON.stringify into the POST body. File data cannot select the origin/path, method, output destination, or headers. A hostile path-like/query-bearing string probe stayed on /v3/voices/speech and /v3/audio/sounds, round-tripping only as a body/query value.

Disposition: recommend dismissing #737 as false positive with the explanation “documented user-requested TTS/catalog transmission to a fixed HeyGen HTTPS origin; explicit file inputs only, encoded body/query values, no destination control.” The query sees a real file-to-network flow, but there is no unauthorized or uncontrolled disclosure. I did not dismiss the alert.

Verdict: COMMENT
Reasoning: All traced file sources are explicit inputs to the feature they invoke, and every outbound destination is fixed and encoding-safe; no payload can redirect the request or broaden which local data is read.

— Magi