fix(test): atomic write for code get --out, no truncate before fetch (#7)

test code get --out <path> opened (truncated) the destination file
before the network request. If the GET then failed, or hit the
"no code generated yet" branch which writes nothing, the user's
pre-existing --out file was left emptied with no way to recover it.

Write to a sibling temp file instead and rename it onto the real
path only after a successful, complete write. Mirrors the atomic
rename contract bundle.ts already uses for multi-file bundles.

Add regression tests for both failure modes: a failing fetch and
the no-code-yet branch. Both reproduce the truncation on the old
code and pass on the fix.

Author: crypticsaiyan

src/commands/test.test.ts

@@ -1,4 +1,11 @@
-import { existsSync, mkdirSync, mkdtempSync, readFileSync, writeFileSync } from 'node:fs';
+import {
+  existsSync,
+  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';
@@ -1556,6 +1563,46 @@ describe('runCodeGet', () => {
       ),
     ).rejects.toMatchObject({ code: 'VALIDATION_ERROR', exitCode: 5 });
   });
+
+  // Regression: --out used to open (truncate) the destination file
+  // before the network request. A failed fetch left a pre-existing
+  // file emptied. The fix writes to a sibling temp file and renames it
+  // into place only on success, so a failure must never touch the
+  // operator's existing --out file.
+  it('--out: a failed fetch leaves a pre-existing file untouched, not truncated', async () => {
+    const { credentialsPath } = makeCreds();
+    const dir = mkdtempSync(join(tmpdir(), 'cli-test-code-out-fail-'));
+    const target = join(dir, 'existing.ts');
+    writeFileSync(target, 'PRE-EXISTING CONTENT', 'utf8');
+    const fetchImpl = (() => Promise.reject(new Error('ENETUNREACH'))) as typeof globalThis.fetch;
+    await expect(
+      runCodeGet(
+        { profile: 'default', output: 'text', debug: false, testId: 'test_fe', out: target },
+        { credentialsPath, fetchImpl },
+      ),
+    ).rejects.toBeDefined();
+    expect(readFileSync(target, 'utf-8')).toBe('PRE-EXISTING CONTENT');
+    // No leftover temp file in the directory.
+    const leftovers = readdirSync(dir).filter(f => f !== 'existing.ts');
+    expect(leftovers).toEqual([]);
+  });
+
+  // Regression: the "no code generated yet" branch writes nothing but
+  // previously still closed (and thus truncated) the opened file.
+  it('--out: "no code generated yet" leaves a pre-existing file untouched', async () => {
+    const { credentialsPath } = makeCreds();
+    const dir = mkdtempSync(join(tmpdir(), 'cli-test-code-out-empty-'));
+    const target = join(dir, 'existing.ts');
+    writeFileSync(target, 'PRE-EXISTING CONTENT', 'utf8');
+    const fetchImpl = makeFetch(() => ({ body: { ...TEST_CODE_INLINE, code: '' } }));
+    await runCodeGet(
+      { profile: 'default', output: 'text', debug: false, testId: 'test_fe', out: target },
+      { credentialsPath, fetchImpl, stderr: () => undefined },
+    );
+    expect(readFileSync(target, 'utf-8')).toBe('PRE-EXISTING CONTENT');
+    const leftovers = readdirSync(dir).filter(f => f !== 'existing.ts');
+    expect(leftovers).toEqual([]);
+  });
 });
 
 describe('runCodePut', () => {

src/commands/test.ts

@@ -1,6 +1,6 @@
 import { createWriteStream, readFileSync, readdirSync, statSync, type WriteStream } from 'node:fs';
-import { stat } from 'node:fs/promises';
-import { dirname, extname, isAbsolute, join, resolve } from 'node:path';
+import { rename, stat, unlink } from 'node:fs/promises';
+import { basename, dirname, extname, isAbsolute, join, resolve } from 'node:path';
 import { randomUUID } from 'node:crypto';
 import { Command } from 'commander';
 import {
@@ -3169,11 +3169,14 @@ interface CodeGetOptions extends CommonOptions {
  * directly; presigned URLs are dereferenced via the same fetch impl
  * (without API-key headers — the URL is the bearer of authority).
  *
- * `--out <path>` redirects the same bytes into a file. We open the file
- * before issuing the network request so a permission/dir error fails
- * fast (exit 5 / VALIDATION_ERROR) without spending an API call. On
- * any error after open we tear down the sink so we don't leak a
- * half-written artifact.
+ * `--out <path>` redirects the same bytes into a file. We validate the
+ * path and open a sibling temp file before issuing the network request
+ * so a permission/dir error fails fast (exit 5 / VALIDATION_ERROR)
+ * without spending an API call. The temp file is renamed onto the real
+ * `--out` path only after a successful, complete write; on any error
+ * (or the "no code generated yet" branch, which writes nothing) the
+ * temp file is discarded and the user's pre-existing `--out` file, if
+ * any, is left untouched.
  */
 export async function runCodeGet(opts: CodeGetOptions, deps: TestDeps = {}): Promise<CliTestCode> {
   // Dry-run: no fetch, no fs. Print the canned shape to stdout and, if
@@ -3204,9 +3207,11 @@ export async function runCodeGet(opts: CodeGetOptions, deps: TestDeps = {}): Pro
 
   try {
     const code = await client.get<CliTestCode>(`/tests/${encodeURIComponent(opts.testId)}/code`);
+    let wroteContent = false;
 
     if (opts.output === 'json') {
       out.print(code);
+      wroteContent = true;
     } else if (isPresignedCodeUrl(code.code)) {
       // Text mode: dump the source body. JSON consumers want the wire
       // shape; humans (and agents shelling out via `> file.ts`) want
@@ -3217,20 +3222,24 @@ export async function runCodeGet(opts: CodeGetOptions, deps: TestDeps = {}): Pro
       // or a piped `gzip`) pauses the upstream reader rather than
       // letting chunks accumulate in V8's heap.
       await streamPresignedBody(code.code, out, deps);
+      wroteContent = true;
     } else if (code.code === '' || code.code === null) {
       // P2-10: draft test with no code yet — empty body would produce
       // silent empty stdout. Print a friendly hint to stderr instead so
-      // the operator knows what happened, and keep exit 0.
+      // the operator knows what happened, and keep exit 0. Nothing was
+      // written, so the temp file is discarded below without touching
+      // a pre-existing `--out` file.
       const stderrFn = deps.stderr ?? ((line: string) => process.stderr.write(`${line}\n`));
       stderrFn('(no code generated yet — run the test first)');
     } else {
       await out.writeChunk(code.code);
+      wroteContent = true;
     }
 
-    if (fileSink) await closeOutputFile(fileSink);
+    if (fileSink) await closeOutputFile(fileSink, wroteContent);
     return code;
   } catch (err) {
-    if (fileSink) await closeOutputFile(fileSink).catch(() => undefined);
+    if (fileSink) await closeOutputFile(fileSink, false).catch(() => undefined);
     throw err;
   }
 }
@@ -7733,22 +7742,29 @@ function makeOutput(mode: OutputMode, deps: TestDeps): Output {
  * Internal handle for `--out <path>` writes. Wraps a Node WriteStream
  * with a tracked `error` field so `closeOutputFile` can re-raise an
  * async stream error (EACCES on a write, ENOSPC mid-stream, etc.) that
- * was emitted between writes.
+ * was emitted between writes. The stream writes to `tmpPath`, a sibling
+ * of the real `path`; `closeOutputFile` renames it into place only on
+ * a successful, complete write, so a forged or failed response never
+ * modifies (or empties) the operator's pre-existing `--out` file.
  */
 interface FileSink {
   readonly stream: WriteStream;
   readonly path: string;
+  readonly tmpPath: string;
   error: Error | null;
 }
 
 /**
- * Open the `--out` target before any network I/O so a permission/dir
- * error fails fast. Synchronous open via `createWriteStream` doesn't
- * actually open the descriptor until first write, so we don't surface
- * EACCES/ENOENT here — instead the stream emits `'error'`, which we
- * remember on the sink and re-throw at close time. The benefit of
- * opening early is still real: invalid path strings (empty, `/dev/null`
- * on a sandboxed fs, etc.) are caught before the API request goes out.
+ * Open a temp file next to the `--out` target before any network I/O so
+ * a permission/dir error fails fast. Synchronous open via
+ * `createWriteStream` doesn't actually open the descriptor until first
+ * write, so we don't surface EACCES/ENOENT here, instead the stream
+ * emits `'error'`, which we remember on the sink and re-throw at close
+ * time. The benefit of opening early is still real: invalid path
+ * strings (empty, `/dev/null` on a sandboxed fs, etc.) are caught
+ * before the API request goes out. Writing to a temp path rather than
+ * `resolved` directly means the real `--out` file is never truncated
+ * up front, see `closeOutputFile` for the commit step.
  */
 function openOutputFile(rawPath: string): FileSink {
   if (typeof rawPath !== 'string' || rawPath.length === 0) {
@@ -7777,8 +7793,9 @@ function openOutputFile(rawPath: string): FileSink {
   if (!parentStat.isDirectory()) {
     throw localValidationError('out', `parent path is not a directory: ${parent}`);
   }
-  const stream = createWriteStream(resolved, { encoding: 'utf8' });
-  const sink: FileSink = { stream, path: resolved, error: null };
+  const tmpPath = join(parent, `.${basename(resolved)}.tmp-${randomUUID()}`);
+  const stream = createWriteStream(tmpPath, { encoding: 'utf8' });
+  const sink: FileSink = { stream, path: resolved, tmpPath, error: null };
   stream.on('error', err => {
     sink.error = err instanceof Error ? err : new Error(String(err));
   });
@@ -7808,25 +7825,37 @@ function makeFileOutput(mode: OutputMode, sink: FileSink): Output {
 }
 
 /**
- * Flush + close the file sink. Called on the success path after the
- * last write and on the error path inside a `.catch(() => undefined)`
- * so the original error isn't masked by a teardown failure.
+ * Flush + close the file sink, then either commit or discard the temp
+ * file. Called on the success path after the last write (`commit:
+ * true` when content was actually written) and on the error / "no code
+ * yet" paths (`commit: false`) inside a `.catch(() => undefined)` so a
+ * teardown failure doesn't mask the original error.
+ *
+ * `commit: true` renames `tmpPath` onto the real `--out` path, the
+ * only point at which the operator's file is touched. `commit: false`
+ * discards the temp file and leaves any pre-existing `--out` file
+ * exactly as it was, this is what prevents a failed/empty response
+ * from silently truncating the operator's filesystem (mirrors the
+ * atomic-rename contract `bundle.ts` uses for multi-file bundles).
  *
  * Re-raises any async stream error captured by the `'error'` listener.
  * Without this re-raise, an EACCES on first write would leave a
- * zero-byte file behind and exit 0 — a false-success surface that is
- * exactly the failure mode `--out` exists to avoid.
+ * zero-byte temp file behind and exit 0, a false-success surface that
+ * is exactly the failure mode `--out` exists to avoid.
  */
-function closeOutputFile(sink: FileSink): Promise<void> {
-  return new Promise((resolve, reject) => {
-    sink.stream.end(() => {
-      if (sink.error) {
-        reject(new TransportError(`Failed to write --out ${sink.path}: ${sink.error.message}`));
-        return;
-      }
-      resolve();
-    });
+async function closeOutputFile(sink: FileSink, commit: boolean): Promise<void> {
+  await new Promise<void>(resolveStream => {
+    sink.stream.end(() => resolveStream());
   });
+  if (sink.error) {
+    await unlink(sink.tmpPath).catch(() => undefined);
+    throw new TransportError(`Failed to write --out ${sink.path}: ${sink.error.message}`);
+  }
+  if (!commit) {
+    await unlink(sink.tmpPath).catch(() => undefined);
+    return;
+  }
+  await rename(sink.tmpPath, sink.path);
 }
 
 /** A presigned `code` body is any `https://` URL — never anything else. */