fix(bundle): preserve prior bundle on failed re-commit swap

Replace in-place commitBundle file renames with a directory-level atomic swap. The complete staging tree is moved aside, the prior bundle is only deleted after a successful rename, and failed swaps roll back to the previous meta.json and steps/ contents.

Adds commitBundle regression test in bundle.commit.test.ts.

Author: SahilRakhaiya05

src/lib/bundle.commit.test.ts

@@ -0,0 +1,64 @@
+import type * as NodeFsPromises from 'node:fs/promises';
+import { mkdirSync, mkdtempSync, readdirSync, readFileSync, writeFileSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+
+const renameMock = vi.hoisted(() => vi.fn());
+
+vi.mock('node:fs/promises', async importOriginal => {
+  const actual = (await importOriginal()) as typeof NodeFsPromises;
+  return {
+    ...actual,
+    rename: renameMock,
+  };
+});
+
+const { commitBundle } = await import('./bundle.js');
+
+describe('commitBundle', () => {
+  let realRename: NodeFsPromises.rename;
+
+  beforeEach(async () => {
+    const actual = (await vi.importActual('node:fs/promises')) as typeof NodeFsPromises;
+    realRename = actual.rename;
+    renameMock.mockImplementation(realRename);
+  });
+
+  afterEach(() => {
+    renameMock.mockReset();
+  });
+
+  it('rolls back to the prior complete bundle when the staging swap fails', async () => {
+    const parent = mkdtempSync(join(tmpdir(), 'bundle-commit-parent-'));
+    const dir = join(parent, 'bundle');
+    const tmpDir = join(dir, '.tmp');
+
+    mkdirSync(join(dir, 'steps'), { recursive: true });
+    writeFileSync(join(dir, 'meta.json'), '{"snapshotId":"snap_old"}\n', 'utf8');
+    writeFileSync(join(dir, 'steps', '01-evidence.json'), '{"step":1}\n', 'utf8');
+
+    mkdirSync(join(tmpDir, 'steps'), { recursive: true });
+    writeFileSync(join(tmpDir, 'meta.json'), '{"snapshotId":"snap_new"}\n', 'utf8');
+    writeFileSync(join(tmpDir, 'result.json'), '{}\n', 'utf8');
+    writeFileSync(join(tmpDir, 'steps', '01-evidence.json'), '{"step":9}\n', 'utf8');
+
+    renameMock.mockImplementation(async (oldPath, newPath) => {
+      const src = String(oldPath);
+      const dest = String(newPath);
+      if (src.includes('.staging.') && !dest.includes('.prior.')) {
+        throw Object.assign(new Error('simulated atomic swap failure'), { code: 'EACCES' });
+      }
+      return realRename(oldPath, newPath);
+    });
+
+    await expect(commitBundle(tmpDir, dir)).rejects.toThrow('simulated atomic swap failure');
+
+    expect(readFileSync(join(dir, 'meta.json'), 'utf8')).toBe('{"snapshotId":"snap_old"}\n');
+    expect(readFileSync(join(dir, 'steps', '01-evidence.json'), 'utf8')).toBe('{"step":1}\n');
+    const leftovers = readdirSync(parent).filter(
+      name => name.includes('.staging.') || name.includes('.prior.'),
+    );
+    expect(leftovers).toEqual([]);
+  });
+});

src/lib/bundle.ts

@@ -11,9 +11,11 @@
  *      `snapshotId`. We refuse a forged response where
  *      `bundle.snapshotId !== result.snapshotId` or steps disagree on
  *      `runIdIfAvailable` (`assertContextIntegrity`).
- *   2. **Atomic on disk** — we write to `<dir>/.tmp/...` first and
- *      `rename()` each file into place. `meta.json` is renamed last so
- *      the presence of `<dir>/meta.json` ⇔ "bundle is complete and
+ *   2. **Atomic on disk** — we write to `<dir>/.tmp/...` first, then
+ *      swap the complete staging tree into place via directory rename.
+ *      The prior bundle is moved aside and only deleted after the swap
+ *      succeeds, so a failed re-commit never destroys a complete bundle.
+ *      `meta.json` in the final tree ⇔ "bundle is complete and
  *      self-consistent".
  *   3. **`.partial` on failure** — any download or fs failure leaves
  *      a `<dir>/.partial` marker (with `requestId`, error summary,
@@ -33,12 +35,13 @@
  *     when the agent re-runs. M3 may add resume.
  */
 
-import { mkdir, mkdtemp, readdir, rename, rm, stat, unlink, writeFile } from 'node:fs/promises';
+import { randomUUID } from 'node:crypto';
+import { mkdir, mkdtemp, rename, rm, stat, unlink, writeFile } from 'node:fs/promises';
 import type { Writable } from 'node:stream';
 import { pipeline } from 'node:stream/promises';
 import type { ReadableStream as NodeReadableStream } from 'node:stream/web';
 import { createWriteStream } from 'node:fs';
-import { dirname, isAbsolute, join, resolve } from 'node:path';
+import { basename, dirname, isAbsolute, join, resolve } from 'node:path';
 import type { CliFailureContext, CliTestStep } from '../commands/test.js';
 import { ApiError, TransportError } from './errors.js';
 import type { FetchImpl } from './http.js';
@@ -465,10 +468,10 @@ export async function writeBundle(
     await writeFile(join(tmpDir, 'meta.json'), JSON.stringify(meta, null, 2) + '\n', 'utf8');
     filesWritten.push('meta.json');
 
-    // Atomic rename: move every file from <dir>/.tmp/* into <dir>/*.
-    // Steps subdir gets renamed wholesale; meta.json renames last so
-    // its visibility implies "bundle is complete and atomic on disk."
-    await commitBundle(tmpDir, dir, filesWritten);
+    // Atomic directory swap: the complete bundle in <dir>/.tmp/ replaces
+    // <dir> only after the staging tree is fully built. A failed swap
+    // rolls back to the prior bundle instead of leaving meta.json gone.
+    await commitBundle(tmpDir, dir);
 
     return { meta, dir, files: filesWritten };
   } catch (err) {
@@ -492,83 +495,43 @@ async function freshTmpDir(dir: string): Promise<string> {
 }
 
 /**
- * Rename `<tmp>/<file>` → `<dir>/<file>` for every file in `files`.
+ * Atomically replace `<dir>` with the complete bundle staged in `tmpDir`.
  *
- * Critical ordering for atomicity (the §3 "agent-safe" contract):
- *
- *   1. **Remove the OLD `meta.json` first.** The bundle's completion
- *      signal is `meta.json`'s presence; an agent reading `<dir>` while
- *      we're mutating it must see "no meta → bundle absent or
- *      mid-write" rather than "meta points at a snapshot that's already
- *      been partially overwritten." Removing the old meta is what
- *      makes the rest of the swap safe to do in place.
- *   2. Wipe stale top-level files (e.g. an old `video.mp4` when the new
- *      bundle has no video). Without this, a fresh bundle could ship
- *      with a stale video lingering at the top level.
- *   3. Replace `<dir>/steps/` wholesale.
- *   4. Rename top-level files into place.
- *   5. **Rename `meta.json` LAST.** Its visible presence is the atomic
- *      completion signal; until step 5 lands, agents see "incomplete."
- *
- * The window between (1) and (5) is bounded by a handful of `rename`
- * syscalls — small enough that a SIGKILL there is rare, and any agent
- * caught reading the dir during it sees no meta and refuses to consume
- * (per §7.3). That's what we want.
+ * `tmpDir` already contains the full §7 layout (including `meta.json`).
+ * We move it to a sibling staging directory, swap it into place via
+ * directory rename, and only delete the prior bundle after the swap
+ * succeeds. If the swap fails, the prior bundle is restored so a
+ * re-commit never leaves `meta.json` deleted with no replacement.
  */
-async function commitBundle(
-  tmpDir: string,
-  dir: string,
-  files: ReadonlyArray<string>,
-): Promise<void> {
-  // (1) Remove the prior bundle's completion signal FIRST.
-  await unlink(join(dir, 'meta.json')).catch(() => undefined);
-
-  // (2) Sweep stale top-level files that the new bundle won't write.
-  // If the prior run wrote `video.mp4` and the new run has no video,
-  // an in-place rename leaves the old video lingering. Enumerate
-  // current top-level entries and remove anything that isn't being
-  // freshly renamed in.
-  const topLevel = files.filter(f => !f.startsWith('steps/'));
-  const newTopLevelSet = new Set(topLevel);
-  newTopLevelSet.add('meta.json'); // about to land last, do not delete
-  const existing = await readdir(dir).catch(() => [] as string[]);
-  for (const entry of existing) {
-    // Preserve the writer's own scratch dir + the .partial marker
-    // (we'll re-evaluate .partial at the end of commit). Anything else
-    // not-listed in the new bundle is stale.
-    if (entry === '.tmp' || entry === '.partial') continue;
-    if (newTopLevelSet.has(entry)) continue;
-    if (entry === 'steps') continue; // handled below
-    await rm(join(dir, entry), { recursive: true, force: true });
-  }
-
-  // (3) Replace `<dir>/steps/` with `<tmp>/steps/`.
-  const stepsTmp = join(tmpDir, 'steps');
-  const stepsDir = join(dir, 'steps');
-  await rm(stepsDir, { recursive: true, force: true });
-  if (await dirExists(stepsTmp)) {
-    await rename(stepsTmp, stepsDir);
-  }
+export async function commitBundle(tmpDir: string, dir: string): Promise<void> {
+  const parent = dirname(dir);
+  const base = basename(dir);
+  const stagingDir = join(parent, `.${base}.staging.${randomUUID()}`);
+  const priorDir = join(parent, `.${base}.prior.${randomUUID()}`);
 
-  // (4) Top-level files (result/failure/code/video). meta.json renames
-  // LAST; track it separately.
-  const metaIdx = topLevel.indexOf('meta.json');
-  const beforeMeta = metaIdx >= 0 ? topLevel.filter((_, i) => i !== metaIdx) : topLevel;
-  for (const file of beforeMeta) {
-    await rename(join(tmpDir, file), join(dir, file));
-  }
+  // Move the complete new bundle out of <dir>/.tmp/ without mutating
+  // the existing bundle tree yet.
+  await rename(tmpDir, stagingDir);
 
-  // (5) meta.json LAST → atomic completion signal.
-  if (metaIdx >= 0) {
-    await rename(join(tmpDir, 'meta.json'), join(dir, 'meta.json'));
+  try {
+    if (await dirExists(dir)) {
+      await rename(dir, priorDir);
+    }
+    await rename(stagingDir, dir);
+    await rm(priorDir, { recursive: true, force: true });
+    await unlink(join(dir, '.partial')).catch(() => undefined);
+  } catch (err) {
+    const priorPresent = await dirExists(priorDir);
+    const dirPresent = await dirExists(dir);
+    if (priorPresent && !dirPresent) {
+      await rename(priorDir, dir);
+    } else if (priorPresent && dirPresent) {
+      await rm(dir, { recursive: true, force: true });
+      await rename(priorDir, dir);
+    }
+    await rm(stagingDir, { recursive: true, force: true }).catch(() => undefined);
+    throw err;
   }
-
-  // .partial from a prior aborted run is now stale. Remove it so an
-  // agent inspecting the dir sees only the fresh bundle.
-  await unlink(join(dir, '.partial')).catch(() => undefined);
-
-  // Clean up the now-empty tmp dir.
-  await rm(tmpDir, { recursive: true, force: true });
 }
 
 async function dirExists(path: string): Promise<boolean> {
@@ -796,7 +759,5 @@ function bundleIntegrityError(
   });
 }
 
-// Avoid unused-import lint warning for the `mkdtemp` and `readdir`
-// helpers reserved for forensics + future resume work.
+// Avoid unused-import lint warning for `mkdtemp` (reserved for resume work).
 void mkdtemp;
-void readdir;