Skip to content

Commit 461ea7a

Browse files
committed
fix: include signal info in exec error when process is killed
The close handler only read the exit code, producing a misleading "exit code null" message when a child process was killed by a signal. Now handles (code, signal) and reports the signal name in the error.
1 parent 52d4361 commit 461ea7a

File tree

4 files changed

+141
-4
lines changed

4 files changed

+141
-4
lines changed

AGENTS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ Key directories:
6363
- **Run tests:** `pnpm test` / `pnpm test:coverage`
6464
- **Structure:** `source/__tests__/` mirrors `source/` layout. Operations tests live in `source/__tests__/operations/`
6565
- **What to test:** Non-interactive agentic flow (validation, JSON output), operations (correct shell commands), config, utils
66-
- **What not to test:** React/Ink components, `exec.ts` internals (mocked in all consumers)
66+
- **What not to test:** React/Ink components
6767
- **Mocking pattern:** Operations tests mock `exec`/`execFile` from `source/operations/exec.js`. Non-interactive tests mock the entire operations layer
6868
- **Coverage:** Focus on the agentic interface. Test files and `source/components/` are excluded from coverage
6969

architecture.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ Two helpers with different security profiles:
9494
- **`execFile(file, args, options)`** — wraps `child_process.spawn` without a shell. Arguments are passed as an array, so user input cannot be interpreted as shell metacharacters. Use this whenever user-provided values (e.g., `projectName`) appear in the command.
9595
- **`exec(command, options)`** — wraps `child_process.spawn` to run `/bin/sh -c <command>` (spawns a shell). Only for commands that require shell features like `$(...)` substitution. Never interpolate user input into the command string.
9696

97-
Both helpers use `spawn` with stdout ignored and stderr piped. They do not capture or return stdout — output is not buffered for the caller. They throw on non-zero exit codes with the stderr message.
97+
Both helpers use `spawn` with stdout ignored and stderr piped. They do not capture or return stdout — output is not buffered for the caller. They throw on non-zero exit codes with the stderr message, or report the signal name when the process is killed by a signal.
9898

9999
## Data Flow
100100

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
import { type ChildProcess, spawn } from 'node:child_process'
2+
import { EventEmitter } from 'node:events'
3+
import { beforeEach, describe, expect, it, vi } from 'vitest'
4+
5+
vi.mock('node:child_process', () => ({
6+
spawn: vi.fn(),
7+
}))
8+
9+
const { exec, execFile } = await import('../../operations/exec.js')
10+
11+
type StderrEmitter = EventEmitter & { on: (event: string, cb: (data: Buffer) => void) => void }
12+
13+
function createMockChild(): ChildProcess {
14+
const child = new EventEmitter() as ChildProcess
15+
child.stderr = new EventEmitter() as StderrEmitter
16+
return child
17+
}
18+
19+
function mockSpawnWith(child: ChildProcess): void {
20+
vi.mocked(spawn).mockReturnValue(child)
21+
}
22+
23+
describe('exec', () => {
24+
beforeEach(() => {
25+
vi.clearAllMocks()
26+
})
27+
28+
it('resolves when process exits with code 0', async () => {
29+
const child = createMockChild()
30+
mockSpawnWith(child)
31+
32+
const promise = exec('echo hello')
33+
child.emit('close', 0, null)
34+
35+
await expect(promise).resolves.toBeUndefined()
36+
})
37+
38+
it('rejects with stderr when process exits with non-zero code', async () => {
39+
const child = createMockChild()
40+
mockSpawnWith(child)
41+
42+
const promise = exec('false')
43+
child.stderr?.emit('data', Buffer.from('something went wrong'))
44+
child.emit('close', 1, null)
45+
46+
await expect(promise).rejects.toThrow('something went wrong')
47+
})
48+
49+
it('rejects with exit code message when stderr is empty', async () => {
50+
const child = createMockChild()
51+
mockSpawnWith(child)
52+
53+
const promise = exec('false')
54+
child.emit('close', 1, null)
55+
56+
await expect(promise).rejects.toThrow('Command failed with exit code 1')
57+
})
58+
59+
it('rejects with signal name when process is killed by a signal', async () => {
60+
const child = createMockChild()
61+
mockSpawnWith(child)
62+
63+
const promise = exec('sleep 999')
64+
child.emit('close', null, 'SIGTERM')
65+
66+
await expect(promise).rejects.toThrow('Process killed by signal SIGTERM')
67+
})
68+
69+
it('rejects with stderr over signal message when both are available', async () => {
70+
const child = createMockChild()
71+
mockSpawnWith(child)
72+
73+
const promise = exec('sleep 999')
74+
child.stderr?.emit('data', Buffer.from('Terminated'))
75+
child.emit('close', null, 'SIGTERM')
76+
77+
await expect(promise).rejects.toThrow('Terminated')
78+
})
79+
80+
it('rejects when spawn emits an error', async () => {
81+
const child = createMockChild()
82+
mockSpawnWith(child)
83+
84+
const promise = exec('nonexistent')
85+
child.emit('error', new Error('spawn ENOENT'))
86+
87+
await expect(promise).rejects.toThrow('spawn ENOENT')
88+
})
89+
90+
it('spawns /bin/sh -c with the command string', async () => {
91+
const child = createMockChild()
92+
mockSpawnWith(child)
93+
94+
const promise = exec('echo hello', { cwd: '/tmp' })
95+
child.emit('close', 0, null)
96+
await promise
97+
98+
expect(spawn).toHaveBeenCalledWith('/bin/sh', ['-c', 'echo hello'], {
99+
cwd: '/tmp',
100+
stdio: ['ignore', 'ignore', 'pipe'],
101+
})
102+
})
103+
})
104+
105+
describe('execFile', () => {
106+
beforeEach(() => {
107+
vi.clearAllMocks()
108+
})
109+
110+
it('spawns the file directly with args array', async () => {
111+
const child = createMockChild()
112+
mockSpawnWith(child)
113+
114+
const promise = execFile('rm', ['-rf', 'dist'], { cwd: '/project' })
115+
child.emit('close', 0, null)
116+
await promise
117+
118+
expect(spawn).toHaveBeenCalledWith('rm', ['-rf', 'dist'], {
119+
cwd: '/project',
120+
stdio: ['ignore', 'ignore', 'pipe'],
121+
})
122+
})
123+
124+
it('rejects with signal name when killed', async () => {
125+
const child = createMockChild()
126+
mockSpawnWith(child)
127+
128+
const promise = execFile('sleep', ['999'])
129+
child.emit('close', null, 'SIGKILL')
130+
131+
await expect(promise).rejects.toThrow('Process killed by signal SIGKILL')
132+
})
133+
})

source/operations/exec.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,15 @@ function run(command: string, args: string[], options: SpawnOptions): Promise<vo
1212
stderr += data.toString()
1313
})
1414

15-
child.on('close', (code) => {
15+
child.on('close', (code, signal) => {
1616
if (code === 0) {
1717
resolve()
1818
} else {
19-
const message = stderr.trim() || `Command failed with exit code ${code}`
19+
const fallback =
20+
signal !== null
21+
? `Process killed by signal ${signal}`
22+
: `Command failed with exit code ${code}`
23+
const message = stderr.trim() || fallback
2024
reject(new Error(message))
2125
}
2226
})

0 commit comments

Comments
 (0)