From 71e45c093cce4d87deded7c02b4103060fc6a609 Mon Sep 17 00:00:00 2001 From: codesailor4 Date: Sun, 18 Oct 2026 13:29:42 +0100 Subject: [PATCH] test(dev): cover Windows npm invocation safety and supervisor cleanup SW-064 (#165): createNpmInvocation is the only place the CLI decides how to reach npm, and on Windows it goes through cmd.exe /d /s /c npm - a wrapper around a command interpreter. Nothing tested it: 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 had no direct coverage. Pins that the fixed /d /s /c prefix always leads, and that a hostile argument (value & calc.exe, $(whoami), backticks, quotes, newlines) arrives as exactly one inert argv element on both platforms. SW-065 (#166): the existing supervisor suite covers a one-child spawn failure and a two-child sibling exit, leaving two cleanup properties unverified. With three children, a mid-loop failure must terminate every already-started child - the two-child case cannot distinguish 'all children killed' from 'the next one killed'. And fail() wraps the original error as the cause so the reason is not lost, which nothing asserted. Covers no-orphans across three children, cause preservation, exit-code and signal in the failure message, handler teardown after failure, and the child 'error' event path. --- tests/dev-supervisor-cleanup.test.ts | 232 ++++++++++++++++++++ tests/npm-invocation-metacharacters.test.ts | 129 +++++++++++ 2 files changed, 361 insertions(+) create mode 100644 tests/dev-supervisor-cleanup.test.ts create mode 100644 tests/npm-invocation-metacharacters.test.ts 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