fix(engine): give the group sub-mix the failure contract, and sanitize its path

Review finding 4 plus the workdir-traversal tail item.

**The group loop had no try/catch.** `parseAudioFxChain`, `parseAutomation` and
`applyAudioFxChain` were called bare, so a malformed `data-fx-chain` on a BUS —
hand-authored, or written by a newer studio carrying an effect id this engine
does not know — threw straight out of `processCompositionAudio`. That bypassed
the MixResult/`failures[]` shape every caller handles, and skipped `bail()`, so
the temp dir leaked with it. The per-element loop has always wrapped the
identical calls. Now both do, with the same rule: an `AudioFxRenderError` stays
fatal, because substituting the dry signal for a processed one ships a render
that sounds plausible and is not what was authored.

**The traversal is real, and narrower than it looks.** `group-${groupId}.wav`
defuses a bare `../` — the segment is `group-..`, not `..` — but an id holding a
slash BEFORE the dots escapes: `a/../../escaped` normalizes to
`<workDir>/../escaped.wav`, outside the tree `bail()`'s rmSync can reach.
Verified: without `safePathSegment` the test finds `escaped.wav` sitting beside
workDir. `data-audio-group` reaches this file straight from the document; the
studio's `GROUP_ID_PATTERN` guards only ids the studio itself mints.

Two tests, each verified against a revert of its own fix. engine: 66 files.

packages/engine/src/services/audioMixer.grouping.test.ts

@@ -1,5 +1,5 @@
 import { spawnSync } from "node:child_process";
-import { mkdtempSync, rmSync } from "node:fs";
+import { mkdirSync, mkdtempSync, readdirSync, rmSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import { afterEach, describe, expect, it } from "vitest";
@@ -401,3 +401,66 @@ describe.skipIf(!HAS_FFMPEG)(
   },
   60_000,
 );
+
+describe.skipIf(!HAS_FFMPEG)("group sub-mix failure contract", () => {
+  afterEach(() => {
+    for (const dir of tempDirs.splice(0)) rmSync(dir, { recursive: true, force: true });
+  });
+
+  // A malformed chain on a BUS threw straight out of processCompositionAudio,
+  // bypassing the MixResult/failures[] shape the caller handles and skipping
+  // bail()'s workDir cleanup. The per-element loop has always wrapped the
+  // identical calls.
+  it("reports an unparseable group fx-chain as a failure instead of throwing", async () => {
+    const projectDir = mkdtempSync(join(tmpdir(), "hf-grp-bad-"));
+    const workDir = mkdtempSync(join(tmpdir(), "hf-grp-bad-work-"));
+    tempDirs.push(projectDir, workDir);
+    writeTone(join(projectDir, "a.wav"), 440, 1, 0.4);
+
+    const result = await processCompositionAudio(
+      [
+        {
+          ...track("a", 1),
+          groupId: "vo",
+          groupFxChain: '{"version":1,"nodes":[{"type":"tapestop"}]}',
+        },
+      ],
+      projectDir,
+      workDir,
+      join(projectDir, `bad-${MIXED_AUDIO_FILENAME}`),
+      1,
+    );
+
+    expect(result.success).toBe(false);
+    const failure = result.failures?.find((f) => f.elementId === "vo");
+    expect(failure?.stage).toBe("mix");
+    expect(failure?.detail).toContain("tapestop");
+  });
+
+  // `data-audio-group` arrives unvalidated from the document. `group-${id}.wav`
+  // defuses a bare `../` (the segment is `group-..`, not `..`), but an id
+  // holding a slash BEFORE the dots does escape: `a/../../x` normalizes to
+  // `<workDir>/../x.wav`, outside the tree `bail()`'s rmSync can reach.
+  it("keeps a traversal-shaped group id inside the work directory", async () => {
+    const parent = mkdtempSync(join(tmpdir(), "hf-grp-parent-"));
+    tempDirs.push(parent);
+    const projectDir = join(parent, "project");
+    const workDir = join(parent, "work");
+    mkdirSync(projectDir);
+    mkdirSync(workDir);
+    writeTone(join(projectDir, "a.wav"), 440, 1, 0.4);
+
+    const result = await processCompositionAudio(
+      [{ ...track("a", 1), groupId: "a/../../escaped" }],
+      projectDir,
+      workDir,
+      join(projectDir, `esc-${MIXED_AUDIO_FILENAME}`),
+      1,
+    );
+
+    expect(result.success).toBe(true);
+    // workDir is cleaned up on success, so what matters is what is left BESIDE
+    // it: an escaped  would survive there. Only the project remains.
+    expect(readdirSync(parent).sort()).toEqual(["project"]);
+  });
+});

packages/engine/src/services/audioMixer.ts

@@ -72,6 +72,23 @@ function clampVolume(volume: number): number {
   return clampAudioGain(volume);
 }
 
+/**
+ * An author-controlled id, made safe to put in a filename.
+ *
+ * `data-audio-group` reaches this file straight from the document — the
+ * studio's `GROUP_ID_PATTERN` guards only ids the studio itself mints, and a
+ * hand-authored or agent-written one is unvalidated. Interpolated raw it could
+ * carry `/` or `..`, and `mkdirSync(recursive)` inside ffmpeg's own path
+ * handling would then write outside `workDir`, where `bail()`'s `rmSync` never
+ * cleans it up. Everything outside [A-Za-z0-9_-] collapses to `_`, and an id
+ * that sanitizes to nothing gets a stable positional fallback rather than an
+ * empty segment that two groups would share.
+ */
+function safePathSegment(id: string, fallbackIndex: number): string {
+  const cleaned = id.replace(/[^A-Za-z0-9_-]/g, "_");
+  return cleaned.length > 0 ? cleaned : `group-${fallbackIndex}`;
+}
+
 function formatFilterNumber(value: number): string {
   return Number(value.toFixed(6)).toString();
 }
@@ -1336,92 +1353,117 @@ export async function processCompositionAudio(
   // own, and members are already delayed to their composition positions
   // inside the sub-mix, so the group WAV's t=0 IS composition time.
   const groupsDegradedAutomation: string[] = [];
+  let groupIndex = -1;
   for (const [groupId, memberTracks] of groupTracks) {
+    groupIndex += 1;
     const meta = groupMeta.get(groupId);
     if (!meta) continue;
-    const groupWavPath = join(workDir, `group-${groupId}.wav`);
-    const subMix = await mixGroupMembers(
-      memberTracks,
-      groupWavPath,
-      totalDuration,
-      effectiveSignal,
-      config,
-    );
-    if (!subMix.success) {
+    const pathId = safePathSegment(groupId, groupIndex);
+    const groupWavPath = join(workDir, `group-${pathId}.wav`);
+    // Same contract the per-element loop above gives: a malformed `data-fx-chain`
+    // or `data-automation` on a BUS threw straight out of processCompositionAudio,
+    // bypassing the MixResult/failures[] shape the caller handles and skipping
+    // `bail()`, so the temp dir leaked too. An AudioFxRenderError stays fatal
+    // for the same reason it is fatal per element: shipping the dry signal in
+    // place of a processed one sounds plausible and is not what was authored.
+    try {
+      const subMix = await mixGroupMembers(
+        memberTracks,
+        groupWavPath,
+        totalDuration,
+        effectiveSignal,
+        config,
+      );
+      if (!subMix.success) {
+        failures.push({
+          stage: "mix",
+          reason: "ffmpeg_failed",
+          owner: "system",
+          retryable: false,
+          elementId: groupId,
+          detail: boundedDetail(
+            `Group sub-mix failed for group ${groupId}: ${subMix.error ?? "unknown"}`,
+          ),
+        });
+        continue;
+      }
+      if (subMix.degradedAutomation) groupsDegradedAutomation.push(groupId);
+
+      // Composition-time automation (offset 0, duration totalDuration) — same
+      // resolve/lane/bake path a member uses, just anchored at the group clock
+      // instead of a clip's own start.
+      const automation = meta.automation
+        ? resolveAutomation(
+            parseAutomation(meta.automation),
+            meta.fxChain ? parseAudioFxChain(meta.fxChain) : undefined,
+          )
+        : null;
+      const laneKeyframes = automation ? volumeLaneKeyframes(automation, 0, totalDuration) : null;
+      const envelope =
+        laneKeyframes && laneKeyframes.length > 0
+          ? { keyframes: laneKeyframes, trackStart: 0, baseVolume: meta.volume }
+          : null;
+
+      let groupSrcPath = groupWavPath;
+      let bakedEnvelope = false;
+      if (meta.fxChain) {
+        const chain = parseAudioFxChain(meta.fxChain);
+        const fxResult = await applyAudioFxChain(
+          groupSrcPath,
+          chain,
+          join(workDir, `group-${pathId}-fx.wav`),
+          {
+            trackId: groupId,
+            signal: effectiveSignal,
+            ...(automation ? { automation } : {}),
+            ...(envelope ? { envelope } : {}),
+          },
+        );
+        groupSrcPath = fxResult.path;
+        bakedEnvelope = fxResult.envelopeBaked;
+      }
+      if (envelope && !bakedEnvelope) {
+        bakedEnvelope = applyVolumeEnvelopeToWav(
+          groupSrcPath,
+          envelope.keyframes,
+          envelope.trackStart,
+          envelope.baseVolume,
+        );
+      }
+
+      // `end` is already the render's totalDuration, so the outer mix's own
+      // atrim-to-totalDuration clips this track exactly the same way it would
+      // an ungrouped one whose clip ran to the end of the render — a group's FX
+      // tail gets the same "never past the end of the video" treatment member
+      // tails get, for free, with no separate tailSeconds bookkeeping needed.
+      tracks.push({
+        id: groupId,
+        srcPath: groupSrcPath,
+        start: 0,
+        end: totalDuration,
+        mediaStart: 0,
+        duration: totalDuration,
+        volume: bakedEnvelope ? 1.0 : meta.volume,
+        // Same fallback an ungrouped track gets: when the envelope could not be
+        // baked into the samples, hand the keyframes to the outer mix's volume
+        // expression instead of dropping the group's automation on the floor.
+        ...(bakedEnvelope || !laneKeyframes?.length ? {} : { volumeKeyframes: laneKeyframes }),
+      });
+    } catch (err: unknown) {
+      if (err instanceof AudioFxRenderError) throw err;
       failures.push({
         stage: "mix",
-        reason: "ffmpeg_failed",
+        reason: "internal",
         owner: "system",
         retryable: false,
         elementId: groupId,
         detail: boundedDetail(
-          `Group sub-mix failed for group ${groupId}: ${subMix.error ?? "unknown"}`,
+          `Audio processing failed for group ${groupId}: ${
+            err instanceof Error ? err.message : String(err)
+          }`,
         ),
       });
-      continue;
     }
-    if (subMix.degradedAutomation) groupsDegradedAutomation.push(groupId);
-
-    // Composition-time automation (offset 0, duration totalDuration) — same
-    // resolve/lane/bake path a member uses, just anchored at the group clock
-    // instead of a clip's own start.
-    const automation = meta.automation
-      ? resolveAutomation(
-          parseAutomation(meta.automation),
-          meta.fxChain ? parseAudioFxChain(meta.fxChain) : undefined,
-        )
-      : null;
-    const laneKeyframes = automation ? volumeLaneKeyframes(automation, 0, totalDuration) : null;
-    const envelope =
-      laneKeyframes && laneKeyframes.length > 0
-        ? { keyframes: laneKeyframes, trackStart: 0, baseVolume: meta.volume }
-        : null;
-
-    let groupSrcPath = groupWavPath;
-    let bakedEnvelope = false;
-    if (meta.fxChain) {
-      const chain = parseAudioFxChain(meta.fxChain);
-      const fxResult = await applyAudioFxChain(
-        groupSrcPath,
-        chain,
-        join(workDir, `group-${groupId}-fx.wav`),
-        {
-          trackId: groupId,
-          signal: effectiveSignal,
-          ...(automation ? { automation } : {}),
-          ...(envelope ? { envelope } : {}),
-        },
-      );
-      groupSrcPath = fxResult.path;
-      bakedEnvelope = fxResult.envelopeBaked;
-    }
-    if (envelope && !bakedEnvelope) {
-      bakedEnvelope = applyVolumeEnvelopeToWav(
-        groupSrcPath,
-        envelope.keyframes,
-        envelope.trackStart,
-        envelope.baseVolume,
-      );
-    }
-
-    // `end` is already the render's totalDuration, so the outer mix's own
-    // atrim-to-totalDuration clips this track exactly the same way it would
-    // an ungrouped one whose clip ran to the end of the render — a group's FX
-    // tail gets the same "never past the end of the video" treatment member
-    // tails get, for free, with no separate tailSeconds bookkeeping needed.
-    tracks.push({
-      id: groupId,
-      srcPath: groupSrcPath,
-      start: 0,
-      end: totalDuration,
-      mediaStart: 0,
-      duration: totalDuration,
-      volume: bakedEnvelope ? 1.0 : meta.volume,
-      // Same fallback an ungrouped track gets: when the envelope could not be
-      // baked into the samples, hand the keyframes to the outer mix's volume
-      // expression instead of dropping the group's automation on the floor.
-      ...(bakedEnvelope || !laneKeyframes?.length ? {} : { volumeKeyframes: laneKeyframes }),
-    });
   }
   if (failures.length > 0) return bail();