diff --git a/tests/dev-supervisor-cleanup.test.ts b/tests/dev-supervisor-cleanup.test.ts new file mode 100644 index 0000000..261f087 --- /dev/null +++ b/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 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']); + }); +}); \ No newline at end of file diff --git a/tests/npm-invocation-metacharacters.test.ts b/tests/npm-invocation-metacharacters.test.ts new file mode 100644 index 0000000..e537fbe --- /dev/null +++ b/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 `. 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([]); + }); +}); \ No newline at end of file