test(dev): cover Windows npm invocation safety and supervisor cleanup

tests/dev-supervisor-cleanup.test.ts

@@ -0,0 +1,232 @@
+/**
+ * Dev supervisor cleanup umbrella (SW-065, #166).
+ *
+ * Pulls the failure-path cleanup cases together into one regression suite. The
+ * existing `dev-orchestrator.test.ts` covers a single-child spawn failure and a
+ * two-child sibling exit, so two properties of cleanup were still unverified:
+ *
+ *   - **No child is orphaned.** With more than two children, a failure partway
+ *     through the spawn loop must still terminate every child that was already
+ *     started. The existing case only proves the second child is signalled, which
+ *     cannot distinguish "all children killed" from "the next one killed".
+ *
+ *   - **The failure cause survives.** `fail()` wraps the original error as the
+ *     `cause` of a `SubprocessCliError` so the underlying reason is not lost.
+ *     The existing case asserts only the error *type* and message; nothing
+ *     checked that the cause is actually attached.
+ *
+ * Cleanup is also idempotent — but the repeated-signal half of that contract is
+ * covered separately by `dev-orchestrator-signal-idempotency.test.ts`, so this
+ * file stays focused on failure paths.
+ */
+
+import { PassThrough } from 'node:stream';
+import { describe, expect, it } from 'vitest';
+import {
+  superviseDevProcesses,
+  type DevSignalSource,
+  type ManagedChildProcess,
+} from '../src/dev/orchestrator.js';
+import type { DevProcessSpec } from '../src/dev/project.js';
+import { SubprocessCliError } from '../src/errors/errors.js';
+import { createCapturedTerminalOutput } from './helpers/index.js';
+
+class FakeChild implements ManagedChildProcess {
+  readonly stdout = new PassThrough();
+  readonly stderr = new PassThrough();
+  readonly killedSignals: NodeJS.Signals[] = [];
+  #errorListener: ((error: Error) => void) | undefined;
+  #exitListener:
+    | ((code: number | null, signal: NodeJS.Signals | null) => void)
+    | undefined;
+
+  once(event: 'error', listener: (error: Error) => void): this;
+  once(
+    event: 'exit',
+    listener: (code: number | null, signal: NodeJS.Signals | null) => void,
+  ): this;
+  once(
+    event: 'error' | 'exit',
+    listener:
+      | ((error: Error) => void)
+      | ((code: number | null, signal: NodeJS.Signals | null) => void),
+  ): this {
+    if (event === 'error') {
+      this.#errorListener = listener as (error: Error) => void;
+    } else {
+      this.#exitListener = listener as (
+        code: number | null,
+        signal: NodeJS.Signals | null,
+      ) => void;
+    }
+    return this;
+  }
+
+  kill(signal: NodeJS.Signals = 'SIGTERM'): boolean {
+    this.killedSignals.push(signal);
+    queueMicrotask(() => this.emitExit(null, signal));
+    return true;
+  }
+
+  emitError(error: Error): void {
+    this.#errorListener?.(error);
+  }
+
+  emitExit(code: number | null, signal: NodeJS.Signals | null): void {
+    this.#exitListener?.(code, signal);
+  }
+}
+
+class FakeSignalSource implements DevSignalSource {
+  readonly #listeners = new Map<NodeJS.Signals, () => void>();
+
+  once(signal: NodeJS.Signals, listener: () => void): void {
+    this.#listeners.set(signal, listener);
+  }
+
+  removeListener(signal: NodeJS.Signals, listener: () => void): void {
+    if (this.#listeners.get(signal) === listener) {
+      this.#listeners.delete(signal);
+    }
+  }
+
+  emit(signal: NodeJS.Signals): void {
+    const listener = this.#listeners.get(signal);
+    this.#listeners.delete(signal);
+    listener?.();
+  }
+}
+
+function specs(...labels: string[]): DevProcessSpec[] {
+  return labels.map((label) => ({
+    label,
+    command: 'npm',
+    args: ['run', 'start'],
+    cwd: '/workspace',
+  }));
+}
+
+describe('supervisor cleanup never orphans a child (#166)', () => {
+  it('terminates every started child when one fails to spawn', async () => {
+    const spawned: FakeChild[] = [];
+
+    const running = superviseDevProcesses(
+      specs('database', 'api', 'worker'),
+      {
+        spawnProcess(planned) {
+          if (planned.label === 'api') {
+            throw new Error('spawn EACCES');
+          }
+          const child = new FakeChild();
+          spawned.push(child);
+          return child;
+        },
+        signalSource: new FakeSignalSource(),
+      },
+    );
+
+    await expect(running).rejects.toBeInstanceOf(SubprocessCliError);
+
+    // Only 'database' was started before the failure, and it must not be left
+    // running. 'worker' was never spawned, so there is nothing to clean up.
+    expect(spawned).toHaveLength(1);
+    for (const child of spawned) {
+      expect(child.killedSignals).toEqual(['SIGTERM']);
+    }
+  });
+
+  it('terminates all siblings when one exits non-zero', async () => {
+    const children = [new FakeChild(), new FakeChild(), new FakeChild()];
+    let index = 0;
+
+    const running = superviseDevProcesses(
+      specs('database', 'api', 'worker'),
+      {
+        spawnProcess: () => children[index++]!,
+        signalSource: new FakeSignalSource(),
+      },
+    );
+
+    children[1]?.emitExit(3, null);
+
+    await expect(running).rejects.toThrow('exited unexpectedly with code 3');
+
+    for (const child of children) {
+      expect(child.killedSignals).toEqual(['SIGTERM']);
+    }
+  });
+
+  it('preserves the original error as the cause of a spawn failure', async () => {
+    const original = new Error('spawn EACCES on /usr/bin/npm');
+    const captured = createCapturedTerminalOutput();
+
+    const error = await superviseDevProcesses(specs('api'), {
+      output: captured.output,
+      spawnProcess() {
+        throw original;
+      },
+      signalSource: new FakeSignalSource(),
+    }).then(
+      () => undefined,
+      (reason: unknown) => reason,
+    );
+
+    expect(error).toBeInstanceOf(SubprocessCliError);
+    // The wrapper is user-facing; the cause is what a maintainer needs.
+    expect((error as SubprocessCliError).userMessage).toContain(
+      "development process 'api'",
+    );
+    expect((error as { cause?: unknown }).cause).toBe(original);
+  });
+
+  it('preserves the non-zero exit code and signal in the failure message', async () => {
+    const children = [new FakeChild(), new FakeChild()];
+    let index = 0;
+
+    const running = superviseDevProcesses(specs('api', 'worker'), {
+      spawnProcess: () => children[index++]!,
+      signalSource: new FakeSignalSource(),
+    });
+
+    children[0]?.emitExit(9, 'SIGKILL');
+
+    await expect(running).rejects.toThrow(
+      'Development process \'api\' exited unexpectedly with code 9 after SIGKILL.',
+    );
+  });
+
+  it('removes signal handlers after a failure', async () => {
+    const children = [new FakeChild()];
+    const signals = new FakeSignalSource();
+
+    const running = superviseDevProcesses(specs('api'), {
+      spawnProcess: () => children[0]!,
+      signalSource: signals,
+    });
+
+    children[0]?.emitExit(1, null);
+    await expect(running).rejects.toBeInstanceOf(SubprocessCliError);
+
+    // A leaked handler would try to kill an already-terminated child on the next
+    // signal.
+    expect(signals.has('SIGINT')).toBe(false);
+    expect(signals.has('SIGTERM')).toBe(false);
+  });
+
+  it('reports a child error event as a failure and still cleans up', async () => {
+    const children = [new FakeChild(), new FakeChild()];
+    let index = 0;
+
+    const running = superviseDevProcesses(specs('api', 'worker'), {
+      spawnProcess: () => children[index++]!,
+      signalSource: new FakeSignalSource(),
+    });
+
+    children[1]?.emitError(new Error('EPIPE'));
+
+    await expect(running).rejects.toThrow(
+      "Development process 'worker' failed to start.",
+    );
+    expect(children[0]?.killedSignals).toEqual(['SIGTERM']);
+  });
+});

tests/npm-invocation-metacharacters.test.ts

@@ -0,0 +1,129 @@
+/**
+ * Windows npm invocation safety (SW-064, #165).
+ *
+ * `createNpmInvocation` is the only place the CLI decides how to reach npm, and on
+ * Windows it does so through `cmd.exe /d /s /c npm <args>`. That form is a wrapper
+ * around a command interpreter, so the argument vector has to stay an argv: the
+ * moment a caller-supplied string is joined into one command line, a value
+ * containing `&`, `|`, `;` or backticks stops being data and becomes a command
+ * fragment.
+ *
+ * Nothing tested this. `critical-path.test.ts` re-implements the same wrapper
+ * inline in its own helper rather than importing the function, so it asserts a
+ * copy of the behaviour — and `npm.ts` itself has no direct coverage.
+ *
+ * The cases below pin two things: the fixed npm prefix always leads, and an
+ * untrusted argument always arrives as exactly one argv element with its
+ * metacharacters intact and unexpanded.
+ */
+
+import { afterEach, describe, expect, it } from 'vitest';
+import { createNpmInvocation } from '../src/process/npm.js';
+
+const ORIGINAL_PLATFORM = process.platform;
+
+/** Stubs `process.platform` for one case and restores it afterwards. */
+function onPlatform(platform: NodeJS.Platform): void {
+  Object.defineProperty(process, 'platform', {
+    value: platform,
+    configurable: true,
+  });
+}
+
+afterEach(() => {
+  Object.defineProperty(process, 'platform', {
+    value: ORIGINAL_PLATFORM,
+    configurable: true,
+  });
+});
+
+describe.each([
+  { platform: 'win32', label: 'Windows' },
+  { platform: 'linux', label: 'POSIX' },
+] as const)(
+  'npm invocation on $label',
+  ({ platform, label }) => {
+    it('runs a safe fixed command successfully', () => {
+      onPlatform(platform);
+
+      const invocation = createNpmInvocation(['run', 'test']);
+
+      expect(Array.isArray(invocation.args)).toBe(true);
+      expect(invocation.args).toContain('npm');
+      expect(invocation.args.slice(-2)).toEqual(['run', 'test']);
+      if (platform === 'win32') {
+        // `/d /s /c` is the documented safe wrapper: it disables AutoRun
+        // autorestart and treats the remainder as a single command.
+        expect(invocation.command).toBe('cmd.exe');
+        expect(invocation.args.slice(0, 3)).toEqual(['/d', '/s', '/c']);
+      } else {
+        expect(invocation.command).toBe('npm');
+      }
+    });
+
+    it.each([
+      'value & calc.exe',
+      'value | more',
+      'value; del important.txt',
+      '$(whoami)',
+      '`id`',
+      'value > C:/Windows/System32/out.txt',
+      'value && echo pwned',
+      'a"b',
+      "a'b",
+      'value\nsecond',
+    ])('keeps %j a single, inert argv element', (hostile) => {
+      onPlatform(platform);
+
+      const invocation = createNpmInvocation(['run', 'test', '--', hostile]);
+      const forwarded = invocation.args[invocation.args.length - 1];
+
+      // Present exactly once, byte-for-byte: never split, quoted or concatenated
+      // into the command line.
+      expect(forwarded).toBe(hostile);
+      expect(invocation.args.filter((arg) => arg === hostile)).toHaveLength(1);
+
+      // The fixed npm prefix is untouched, so nothing can be prepended to it.
+      const npmIndex = invocation.args.indexOf('npm');
+      expect(npmIndex).toBeGreaterThanOrEqual(0);
+      expect(invocation.args.slice(0, npmIndex)).toEqual(
+        platform === 'win32' ? ['/d', '/s', '/c'] : [],
+      );
+    });
+
+    it('never produces a single joined command string', () => {
+      onPlatform(platform);
+
+      const invocation = createNpmInvocation(['run', 'test', '--', 'a && b']);
+
+      // The whole defence is that `command` and `args` stay separate so the
+      // spawner can pass `shell: false`.
+      expect(typeof invocation.command).toBe('string');
+      expect(Array.isArray(invocation.args)).toBe(true);
+      for (const arg of invocation.args) {
+        expect(typeof arg).toBe('string');
+      }
+      expect(invocation.args.join(' ')).toContain('a && b');
+    });
+  },
+);
+
+describe('npm invocation argument handling', () => {
+  it('keeps the fixed prefix when no arguments are supplied', () => {
+    onPlatform('win32');
+
+    const invocation = createNpmInvocation([]);
+
+    expect(invocation.command).toBe('cmd.exe');
+    expect(invocation.args).toEqual(['/d', '/s', '/c', 'npm']);
+  });
+
+  it('forwards an empty argument list unchanged on POSIX', () => {
+    onPlatform('linux');
+
+    const invocation = createNpmInvocation([]);
+
+    expect(invocation.command).toBe('npm');
+    expect(invocation.args).toEqual([]);
+  });
+});