fix(test): reject empty code get --out and strip code-file BOM (#34)

* fix(test): reject empty code get --out and strip code-file BOM

When test code get --out receives an empty inline body, closeOutputFile left a zero-byte file with exit 0 — scripts and agents treated that as a successful download. Abort the sink, unlink any artifact, and surface VALIDATION_ERROR instead.

Plan/steps JSON already strip a leading UTF-8 BOM from PowerShell 5.1 files; apply the same strip to --code-file reads so uploaded test source is not corrupted by an invisible U+FEFF prefix.

* fix(test): rebase onto v0.2.0 and harden empty-code --out cleanup

- Resolve rebase conflicts: keep atomic temp-file --out writes from main
  while rejecting empty inline code with VALIDATION_ERROR (exit 5)
- abortOutputFile: wait for stream close before unlinking tmpPath
- BOM test: use .py code-file (assertPythonCodeFile from v0.2.0)
- CI: build before test + fileParallelism false (dist/ race flake)

Author: SahilRakhaiya05

.github/workflows/ci.yml

@@ -63,6 +63,7 @@ jobs:
           cache: 'npm'
 
       - run: npm ci
+      - run: npm run build
       - run: npm test
         env:
           CI: true

src/commands/test.test.ts

@@ -1591,22 +1591,66 @@ describe('runCodeGet', () => {
     expect(leftovers).toEqual([]);
   });
 
-  // Regression: the "no code generated yet" branch writes nothing but
-  // previously still closed (and thus truncated) the opened file.
+  it('--out (text mode) rejects empty inline code with VALIDATION_ERROR and leaves no artifact', async () => {
+    const { credentialsPath } = makeCreds();
+    const emptyCode: CliTestCode = { ...TEST_CODE_INLINE, code: '' };
+    const fetchImpl = makeFetch(() => ({ body: emptyCode }));
+    const dir = mkdtempSync(join(tmpdir(), 'cli-test-code-empty-out-'));
+    const target = join(dir, 'empty.ts');
+    await expect(
+      runCodeGet(
+        {
+          profile: 'default',
+          output: 'text',
+          debug: false,
+          testId: 'test_fe',
+          out: target,
+        },
+        { credentialsPath, fetchImpl },
+      ),
+    ).rejects.toMatchObject({
+      code: 'VALIDATION_ERROR',
+      exitCode: 5,
+      details: expect.objectContaining({ field: 'out' }),
+    });
+    expect(existsSync(target)).toBe(false);
+  });
+
+  // Regression: empty inline code with --out must reject (exit 5) without
+  // truncating or replacing a pre-existing destination 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 },
-    );
+    await expect(
+      runCodeGet(
+        { profile: 'default', output: 'text', debug: false, testId: 'test_fe', out: target },
+        { credentialsPath, fetchImpl, stderr: () => undefined },
+      ),
+    ).rejects.toMatchObject({
+      code: 'VALIDATION_ERROR',
+      exitCode: 5,
+      details: expect.objectContaining({ field: 'out' }),
+    });
     expect(readFileSync(target, 'utf-8')).toBe('PRE-EXISTING CONTENT');
     const leftovers = readdirSync(dir).filter(f => f !== 'existing.ts');
     expect(leftovers).toEqual([]);
   });
+
+  it('text mode without --out still hints on stderr when inline code is empty', async () => {
+    const { credentialsPath } = makeCreds();
+    const emptyCode: CliTestCode = { ...TEST_CODE_INLINE, code: '' };
+    const fetchImpl = makeFetch(() => ({ body: emptyCode }));
+    const stderr: string[] = [];
+    const got = await runCodeGet(
+      { profile: 'default', output: 'text', debug: false, testId: 'test_fe' },
+      { credentialsPath, fetchImpl, stderr: line => stderr.push(line) },
+    );
+    expect(got.code).toBe('');
+    expect(stderr.join('\n')).toContain('no code generated yet');
+  });
 });
 
 describe('runCodePut', () => {
@@ -1661,6 +1705,30 @@ describe('runCodePut', () => {
     expect(sent.headers.get('content-type')).toBe('application/json');
   });
 
+  it('strips a UTF-8 BOM from --code-file before uploading (Windows PowerShell 5.1 default)', async () => {
+    const { credentialsPath } = makeCreds();
+    const dir = mkdtempSync(join(tmpdir(), 'cli-p4-bom-'));
+    const codeFile = join(dir, 'updated.py');
+    writeFileSync(codeFile, '\uFEFF' + 'updated body', 'utf8');
+    let seenBody: unknown;
+    const fetchImpl = makeFetch((_url, init) => {
+      seenBody = init.body ? JSON.parse(init.body as string) : undefined;
+      return { body: SAMPLE_RESPONSE };
+    });
+    await runCodePut(
+      {
+        profile: 'default',
+        output: 'json',
+        debug: false,
+        testId: 'test_alpha',
+        codeFile,
+        expectedVersion: 'v3',
+      },
+      { credentialsPath, fetchImpl, stdout: () => undefined, stderr: () => undefined },
+    );
+    expect(seenBody).toEqual({ code: 'updated body' });
+  });
+
   it('forwards --language in the body when set', async () => {
     const { credentialsPath } = makeCreds();
     const codeFile = writeCodeFile('print("hi")');

src/commands/test.ts

@@ -911,7 +911,7 @@ function readCodeFileGuarded(path: string): string {
 
 function readCodeFile(path: string): string {
   try {
-    return readFileSync(resolveAbsolute(path), 'utf8');
+    return stripBom(readFileSync(resolveAbsolute(path), 'utf8'));
   } catch (err) {
     const code = (err as NodeJS.ErrnoException).code;
     if (code === 'ENOENT') {
@@ -3292,7 +3292,7 @@ export async function runCodeGet(opts: CodeGetOptions, deps: TestDeps = {}): Pro
     return code;
   }
 
-  const fileSink = opts.out !== undefined ? openOutputFile(opts.out) : null;
+  let fileSink = opts.out !== undefined ? openOutputFile(opts.out) : null;
   const out = fileSink ? makeFileOutput(opts.output, fileSink) : makeOutput(opts.output, deps);
   const client = makeClient(opts, deps);
 
@@ -3317,9 +3317,20 @@ export async function runCodeGet(opts: CodeGetOptions, deps: TestDeps = {}): Pro
     } 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. Nothing was
-      // written, so the temp file is discarded below without touching
-      // a pre-existing `--out` file.
+      // the operator knows what happened, and keep exit 0 when no `--out`.
+      //
+      // With `--out`, refuse to leave a zero-byte artifact behind: agents
+      // and scripts that check file size would otherwise treat exit 0 as
+      // a successful download. Discard the temp sink without touching a
+      // pre-existing destination file.
+      if (fileSink) {
+        await abortOutputFile(fileSink);
+        fileSink = null;
+        throw localValidationError(
+          'out',
+          'test has no generated code yet — run the test first (refusing to write an empty --out file)',
+        );
+      }
       const stderrFn = deps.stderr ?? ((line: string) => process.stderr.write(`${line}\n`));
       stderrFn('(no code generated yet — run the test first)');
     } else {
@@ -7997,6 +8008,19 @@ async function closeOutputFile(sink: FileSink, commit: boolean): Promise<void> {
   await rename(sink.tmpPath, sink.path);
 }
 
+/** Tear down an opened `--out` sink without leaving a zero-byte artifact. */
+async function abortOutputFile(sink: FileSink): Promise<void> {
+  await new Promise<void>(resolve => {
+    if (sink.stream.destroyed) {
+      resolve();
+      return;
+    }
+    sink.stream.once('close', () => resolve());
+    sink.stream.destroy();
+  });
+  await unlink(sink.tmpPath).catch(() => undefined);
+}
+
 /** A presigned `code` body is any `https://` URL — never anything else. */
 export function isPresignedCodeUrl(code: string): boolean {
   return code.startsWith('https://');

vitest.config.ts

@@ -4,6 +4,9 @@ export default defineConfig({
   test: {
     include: ['src/**/*.{test,spec}.ts', 'test/**/*.{test,spec}.ts'],
     exclude: ['test/dev-e2e/**', 'test/e2e/**', 'node_modules/**', 'dist/**'],
+    // Subprocess/snapshot suites each run `npm run build` in beforeAll; parallel
+    // file workers can race on dist/ and produce a stale binary (exit 1 vs 5 flakes).
+    fileParallelism: false,
     coverage: {
       provider: 'v8',
       reporter: ['text', 'text-summary', 'json-summary', 'html'],