diff --git a/packages/cli/src/services/prompt-processors/shellProcessor.test.ts b/packages/cli/src/services/prompt-processors/shellProcessor.test.ts index c47758574e7..0b3d15e50ef 100644 --- a/packages/cli/src/services/prompt-processors/shellProcessor.test.ts +++ b/packages/cli/src/services/prompt-processors/shellProcessor.test.ts @@ -524,6 +524,25 @@ describe('ShellProcessor', () => { ]); }); + it('should not report PTY signal 0 as a termination', async () => { + const processor = new ShellProcessor('test-command'); + const prompt: PromptPipelineContent = + createPromptPipelineContent('!{cmd}'); + mockShellExecute.mockReturnValue({ + result: Promise.resolve({ + ...SUCCESS_RESULT, + output: 'output', + stderr: '', + exitCode: 0, + signal: 0, + }), + }); + + const result = await processor.process(prompt, context); + + expect(result).toEqual([{ text: 'output' }]); + }); + it('should throw a detailed error if the shell fails to spawn', async () => { const processor = new ShellProcessor('test-command'); const prompt: PromptPipelineContent = diff --git a/packages/cli/src/services/prompt-processors/shellProcessor.ts b/packages/cli/src/services/prompt-processors/shellProcessor.ts index 679e1d0c6e3..6ea440b693e 100644 --- a/packages/cli/src/services/prompt-processors/shellProcessor.ts +++ b/packages/cli/src/services/prompt-processors/shellProcessor.ts @@ -10,6 +10,7 @@ import { escapeShellArg, getShellConfiguration, ShellExecutionService, + isSignalTermination, flatMapTextParts, checkArgumentSafety, } from '@qwen-code/qwen-code-core'; @@ -218,7 +219,7 @@ export class ShellProcessor implements IPromptProcessor { executionResult.exitCode !== null ) { processedPrompt += `\n[Shell command '${injection.resolvedCommand}' exited with code ${executionResult.exitCode}]`; - } else if (executionResult.signal !== null) { + } else if (isSignalTermination(executionResult.signal)) { processedPrompt += `\n[Shell command '${injection.resolvedCommand}' terminated by signal ${executionResult.signal}]`; } } diff --git a/packages/cli/src/ui/hooks/shellCommandProcessor.test.ts b/packages/cli/src/ui/hooks/shellCommandProcessor.test.ts index c85ce69d255..fc3c88d24ef 100644 --- a/packages/cli/src/ui/hooks/shellCommandProcessor.test.ts +++ b/packages/cli/src/ui/hooks/shellCommandProcessor.test.ts @@ -282,6 +282,29 @@ describe('useShellCommandProcessor', () => { expect(setShellInputFocusedMock).toHaveBeenCalledWith(false); }); + it('should treat PTY clean-exit signal 0 as a successful command', async () => { + const { result } = renderProcessorHook(); + + act(() => { + result.current.handleShellCommand( + 'pty-clean-exit', + new AbortController().signal, + ); + }); + const execPromise = onExecMock.mock.calls[0][0]; + + act(() => { + resolveExecutionPromise(createMockServiceResult({ signal: 0 })); + }); + await act(async () => await execPromise); + + const finalHistoryItem = addItemToHistoryMock.mock.calls[1][0]; + expect(finalHistoryItem.tools[0].status).toBe(ToolCallStatus.Success); + expect(finalHistoryItem.tools[0].resultDisplay).not.toContain( + 'terminated by signal', + ); + }); + describe('UI Streaming and Throttling', () => { beforeEach(() => { vi.useFakeTimers({ toFake: ['Date'] }); diff --git a/packages/cli/src/ui/hooks/shellCommandProcessor.ts b/packages/cli/src/ui/hooks/shellCommandProcessor.ts index 99336692623..beeb92f4dba 100644 --- a/packages/cli/src/ui/hooks/shellCommandProcessor.ts +++ b/packages/cli/src/ui/hooks/shellCommandProcessor.ts @@ -19,6 +19,7 @@ import type { import { compactToolResultDisplayForHistory, createDebugLogger, + isSignalTermination, isBinary, ShellExecutionService, } from '@qwen-code/qwen-code-core'; @@ -288,7 +289,7 @@ export const useShellCommandProcessor = ( } else if (result.aborted) { finalStatus = ToolCallStatus.Canceled; finalOutput = `Command was cancelled.\n${finalOutput}`; - } else if (result.signal) { + } else if (isSignalTermination(result.signal)) { finalStatus = ToolCallStatus.Error; finalOutput = `Command terminated by signal: ${result.signal}.\n${finalOutput}`; } else if (result.exitCode !== 0) { diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index fb5f84a3ac9..fbf1ea62ff9 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -377,6 +377,15 @@ describe('ShellExecutionService', () => { }); }); + it('normalizes node-pty clean-exit signal 0 to null', async () => { + const { result } = await simulateExecution('echo clean', (pty) => { + pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: 0 }); + }); + + expect(result.exitCode).toBe(0); + expect(result.signal).toBeNull(); + }); + it('disposes PTY terminal resources on natural exit', async () => { const terminalDisposeSpy = vi.spyOn(Terminal.prototype, 'dispose'); const removeListenerSpy = vi.spyOn(mockPtyProcess, 'removeListener'); @@ -1051,13 +1060,14 @@ describe('ShellExecutionService', () => { ); expect(result.promoted).toBe(true); // After promote, drive the PTY's onExit to simulate natural - // completion. The service attaches a new exit listener for + // completion with its raw clean-exit signal metadata. The service + // attaches a new exit listener for // post-promote settle — find the most-recently-registered. const onExitRegistrations = mockPtyProcess.onExit.mock.calls; expect(onExitRegistrations.length).toBeGreaterThanOrEqual(2); const postPromoteExitHandler = onExitRegistrations[onExitRegistrations.length - 1][0]; - postPromoteExitHandler({ exitCode: 0, signal: undefined }); + postPromoteExitHandler({ exitCode: 0, signal: 0 }); expect(settleCalls).toHaveLength(1); expect(settleCalls[0].exitCode).toBe(0); expect(settleCalls[0].signal).toBeNull(); diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index a078641abc4..839dd66744a 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -150,6 +150,18 @@ export type ShellAbortReason = | { kind: 'cancel' } | { kind: 'background'; shellId?: string }; +/** + * Returns true only for a real process-signal termination. + * node-pty reports signal 0 for a clean exit; the service normalizes that + * value to null at its boundary, while this predicate remains defensive for + * legacy or mocked result objects. + */ +export function isSignalTermination( + signal: number | NodeJS.Signals | null, +): boolean { + return signal !== null && signal !== 0; +} + /** A structured result from a shell command execution. */ export interface ShellExecutionResult { /** @@ -161,9 +173,15 @@ export interface ShellExecutionResult { rawOutput: Buffer; /** The combined, decoded output as a string. */ output: string; - /** The process exit code, or null if terminated by a signal. */ + /** + * The process exit code. Child-process signal termination reports null; + * PTY signal termination may still carry a numeric exit code. + */ exitCode: number | null; - /** The signal that terminated the process, if any. */ + /** + * The non-zero signal that terminated the process, if any. A node-pty + * clean-exit signal of 0 is normalized to null at the service boundary. + */ signal: number | null; /** An error object if the process failed to spawn. */ error: Error | null; @@ -296,8 +314,11 @@ export interface ShellPostPromoteHandlers { onData?: (event: ShellOutputEvent) => void; /** * Fired exactly once when the post-promote child settles — natural - * exit (`exitCode` set, `signal: null`), signal kill (`exitCode: - * null`, `signal` set), or spawn-side error (`error` set). NOT + * child-process exit (`exitCode` set, `signal: null`), natural PTY + * exit (`exitCode` set, clean-exit signal normalized to `null`), signal kill (which may carry + * `exitCode: 0` with a non-zero signal on PTY, or `exitCode: null` + * with a string signal from `child_process`), or spawn-side error + * (`error` set). NOT * fired for the promote-time resolve itself (that's the * `result.promoted` Promise resolution). Callers wire this to the * registry's `complete` / `fail` transitions. @@ -1888,7 +1909,7 @@ export class ShellExecutionService { rawOutput: finalBuffer, output: fullOutput, exitCode, - signal: signal ?? null, + signal: signal === 0 ? null : (signal ?? null), error, aborted: abortSignal.aborted, pid: ptyProcess.pid, @@ -2132,7 +2153,7 @@ export class ShellExecutionService { }) => { firePostSettle({ exitCode, - signal: signal ?? null, + signal: signal === 0 ? null : (signal ?? null), endTime: Date.now(), }); }, diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index a13d7156647..10750530f34 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -21,6 +21,8 @@ const mockDebugLogger = vi.hoisted(() => ({ })); vi.mock('../services/shellExecutionService.js', () => ({ ShellExecutionService: { execute: mockShellExecutionService }, + isSignalTermination: (signal: number | NodeJS.Signals | null) => + signal !== null && signal !== 0, getShellAbortReasonKind: (reason: unknown) => typeof reason === 'object' && reason !== null && @@ -2878,6 +2880,92 @@ describe('ShellTool', () => { expect(result.error?.message).toContain('failed output'); }); + it('reports a foreground signal termination as a tool error', async () => { + const invocation = shellTool.build({ + command: 'signal-terminated-command', + is_background: false, + }); + const promise = invocation.execute(mockAbortSignal); + resolveShellExecution({ + output: '', + exitCode: null, + signal: 15, + error: null, + aborted: false, + }); + + const result = await promise; + + expect(result.error).toEqual({ + message: expect.stringContaining('Signal: 15'), + type: ToolErrorType.SHELL_EXECUTE_ERROR, + }); + expect(result.returnDisplay).toContain( + 'Command terminated by signal: 15', + ); + }); + + it('keeps a successful PTY exit code successful with signal 0 metadata', async () => { + const invocation = shellTool.build({ + command: 'pty-cleanup-command', + is_background: false, + }); + const promise = invocation.execute(mockAbortSignal); + resolveShellExecution({ + output: 'completed', + exitCode: 0, + signal: 0, + aborted: false, + }); + + const result = await promise; + + expect(result.error).toBeUndefined(); + expect(result.llmContent).toContain('Output: completed'); + }); + + it('reports a PTY signal termination as a tool error', async () => { + const invocation = shellTool.build({ + command: 'pty-signal-terminated-command', + is_background: false, + }); + const promise = invocation.execute(mockAbortSignal); + resolveShellExecution({ + output: '', + exitCode: 0, + signal: 15, + error: null, + aborted: false, + }); + + const result = await promise; + + expect(result.error).toEqual({ + message: expect.stringContaining('Signal: 15'), + type: ToolErrorType.SHELL_EXECUTE_ERROR, + }); + }); + + it('does not report a user-cancelled signal as a tool error', async () => { + const invocation = shellTool.build({ + command: 'cancelled-command', + is_background: false, + }); + const promise = invocation.execute(mockAbortSignal); + resolveShellExecution({ + output: '', + exitCode: null, + signal: 15, + error: null, + aborted: true, + }); + + const result = await promise; + + expect(result.error).toBeUndefined(); + expect(result.llmContent).toContain('Command was cancelled'); + }); + it.each([ 'grep pattern file', 'rg pattern file', @@ -3211,6 +3299,23 @@ describe('ShellTool', () => { expect(result.llmContent).not.toContain('foreground command ran for'); }); + it('appends the hint when PTY reports a clean exit with signal 0', async () => { + const invocation = shellTool.build({ + command: 'echo hi', + is_background: false, + }); + const promise = invocation.execute(mockAbortSignal); + await vi.advanceTimersByTimeAsync(60_000); + resolveShellExecution({ + output: 'hi', + exitCode: 0, + signal: 0, + aborted: false, + }); + const result = await promise; + expect(result.llmContent).toContain('foreground command ran for 60s'); + }); + it('off-by-one: omits the hint at threshold − 1ms', async () => { // Pin the boundary so a regression that flips `>=` to `>` would // fail loudly. Pairs with the existing 60_000ms-exactly test @@ -5990,9 +6095,10 @@ describe('ShellTool', () => { ); }); - it('natural child exit transitions the registry entry to "completed" (exitCode 0)', async () => { + it('clean PTY exit transitions the registry entry to "completed" (exitCode 0, signal 0)', async () => { // Pin the PR-2.5 settle path: after promote, when the - // service's post-promote exit listener fires with exitCode=0, + // service's post-promote exit listener fires with exitCode=0 and + // node-pty's clean-exit signal=0, // `registry.complete(shellId, 0, ...)` is called and the // stream closes. const writeStreamMock = { @@ -6037,7 +6143,7 @@ describe('ShellTool', () => { postPromote?: { onSettle?: (info: { exitCode: number | null; - signal: number | null; + signal: number | NodeJS.Signals | null; error?: Error; endTime: number; }) => void; @@ -6046,7 +6152,7 @@ describe('ShellTool', () => { expect(opts?.postPromote?.onSettle).toBeDefined(); opts.postPromote!.onSettle!({ exitCode: 0, - signal: null, + signal: 0, endTime: 1700000000000, }); @@ -6083,7 +6189,7 @@ describe('ShellTool', () => { postPromote: { onSettle: (info: { exitCode: number | null; - signal: number | null; + signal: number | NodeJS.Signals | null; error?: Error; endTime: number; }) => void; @@ -6108,6 +6214,14 @@ describe('ShellTool', () => { 2, ); + // node-pty can preserve exitCode 0 alongside a non-zero signal. + onSettle({ exitCode: 0, signal: 15, endTime: 2.5 }); + expect(registry.fail).toHaveBeenCalledWith( + entry.shellId, + 'Terminated by signal 15', + 2.5, + ); + // Spawn-side error → fail with err.message. onSettle({ exitCode: null, @@ -6118,6 +6232,100 @@ describe('ShellTool', () => { expect(registry.fail).toHaveBeenCalledWith(entry.shellId, 'ENOENT', 3); }); + it('treats a child-process signal string as a failed settle', async () => { + const registry = mockConfig.getBackgroundShellRegistry(); + const invocation = shellTool.build({ + command: 'cmd', + is_background: false, + }); + const promise = invocation.execute(mockAbortSignal); + resolveShellExecution({ + output: '', + exitCode: null, + signal: null, + aborted: false, + promoted: true, + pid: 33334, + }); + await promise; + const serviceCall = mockShellExecutionService.mock.calls[0]; + const onSettle = ( + serviceCall[6] as { + postPromote: { + onSettle: (info: { + exitCode: number | null; + signal: number | NodeJS.Signals | null; + error?: Error; + endTime: number; + }) => void; + }; + } + ).postPromote.onSettle; + const entry = (registry.register as Mock).mock.calls[0][0]; + + onSettle({ exitCode: null, signal: 'SIGTERM', endTime: 3.5 }); + + expect(registry.fail).toHaveBeenCalledWith( + entry.shellId, + 'Terminated by signal SIGTERM', + 3.5, + ); + }); + + it('keeps a task_stop cancellation from being reclassified as a signal failure', async () => { + vi.useFakeTimers(); + const processKillSpy = vi + .spyOn(process, 'kill') + .mockImplementation(() => true); + try { + const registry = mockConfig.getBackgroundShellRegistry(); + const invocation = shellTool.build({ + command: 'sleep 1', + is_background: false, + }); + const promise = invocation.execute(mockAbortSignal); + resolveShellExecution({ + output: '', + exitCode: null, + signal: null, + aborted: false, + promoted: true, + pid: 12345, + }); + await promise; + + const serviceCall = mockShellExecutionService.mock.calls[0]; + const onSettle = ( + serviceCall[6] as { + postPromote: { + onSettle: (info: { + exitCode: number | null; + signal: number | NodeJS.Signals | null; + error?: Error; + endTime: number; + }) => void; + }; + } + ).postPromote.onSettle; + const entry = (registry.register as Mock).mock.calls[0][0]; + + // `task_stop` aborts the fresh registry controller before the + // child reports its SIGTERM/SIGKILL settle event. + entry.abortController.abort(); + await Promise.resolve(); + expect(processKillSpy).toHaveBeenCalledWith(-12345, 'SIGTERM'); + await vi.advanceTimersByTimeAsync(250); + expect(processKillSpy).toHaveBeenCalledWith(-12345, 'SIGKILL'); + onSettle({ exitCode: 0, signal: 15, endTime: 4 }); + + expect(registry.cancel).toHaveBeenCalledWith(entry.shellId, 4); + expect(registry.fail).not.toHaveBeenCalled(); + } finally { + processKillSpy.mockRestore(); + vi.useRealTimers(); + } + }); + it('queued-settle race: onSettle fires BEFORE handlePromotedForeground completes — entry settles + llmContent reflects final status', async () => { // Pin the queued-settle path: a very fast command can exit // between the service-side promote-resolve and the diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 7fcc1776a4f..c7dece30138 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -45,6 +45,7 @@ import type { } from '../services/shellExecutionService.js'; import { getShellAbortReasonKind, + isSignalTermination, ShellExecutionService, } from '../services/shellExecutionService.js'; import { @@ -2809,10 +2810,12 @@ export class ShellToolInvocation extends BaseToolInvocation< // cancel). Their own messaging is enough; a "should have // been background" reminder when the agent already knows // the command didn't complete is noise. - // * Suppressed on external signal kills (`result.signal != - // null` with `aborted: false`, e.g. SIGTERM from container - // shutdown, k8s eviction, OOM killer, sibling reaping the - // process group). `shellExecutionService` only sets + // * Suppressed on external signal kills + // (`isSignalTermination(result.signal)` with `aborted: false`, + // e.g. SIGTERM from container shutdown, k8s eviction, OOM + // killer, or sibling reaping the process group). node-pty's + // clean-exit signal 0 is normalized to null and does not + // suppress this hint. The service only sets // `aborted` when the AbortSignal we passed was triggered, // so external signals fall through to the non-aborted // branch — same rationale as timeout. @@ -2836,7 +2839,7 @@ export class ShellToolInvocation extends BaseToolInvocation< const shouldAppendLongRunHint = longRunThreshold !== null && !result.aborted && - result.signal === null && + !isSignalTermination(result.signal) && elapsedMs >= longRunThreshold; // Observability: the hint decision is otherwise invisible. If a // user reports "my 65s command didn't get the hint" or "5s command @@ -2877,7 +2880,7 @@ export class ShellToolInvocation extends BaseToolInvocation< : wasPromoteRefused ? 'Command finished before background-promote could be honoured.' : 'Command cancelled by user.'; - } else if (result.signal) { + } else if (isSignalTermination(result.signal)) { returnDisplayMessage = `Command terminated by signal: ${result.signal}`; } else if (result.error) { returnDisplayMessage = `Command failed: ${getErrorMessage( @@ -3016,7 +3019,8 @@ export class ShellToolInvocation extends BaseToolInvocation< type: ToolErrorType.SHELL_EXECUTE_ERROR, }, } - : isShellExitError(this.params.command, result.exitCode) + : (!result.aborted && isSignalTermination(result.signal)) || + isShellExitError(this.params.command, result.exitCode) ? { error: { // Schedulers use error.message as the model-facing response. @@ -3389,21 +3393,23 @@ export class ShellToolInvocation extends BaseToolInvocation< const classifySettle = ( info: ShellPostPromoteSettleInfo, ): { status: 'completed' | 'failed'; failMsg: string | null } => { - // Decision table: `error` → fail (spawn-side failure); `exitCode - // === 0` → complete; non-zero exitCode → fail; signal-killed - // (no exitCode, signal set) → fail with descriptive message; - // everything-null → fail with generic message. + // Decision table: `error` → fail (spawn-side failure); a non-zero + // signal means the process was killed (including node-pty's + // `exitCode: 0, signal: N` shape), then `exitCode === 0` → complete; + // non-zero exitCode → fail; everything-null → fail with a generic + // message. if (info.error) return { status: 'failed', failMsg: info.error.message }; - if (info.exitCode === 0) return { status: 'completed', failMsg: null }; - if (info.exitCode !== null) + if (isSignalTermination(info.signal)) { return { status: 'failed', - failMsg: `Exited with code ${info.exitCode}`, + failMsg: `Terminated by signal ${info.signal}`, }; - if (info.signal !== null) + } + if (info.exitCode === 0) return { status: 'completed', failMsg: null }; + if (info.exitCode !== null) return { status: 'failed', - failMsg: `Terminated by signal ${info.signal}`, + failMsg: `Exited with code ${info.exitCode}`, }; // PR-2.5 wave-3: this branch is meant to // be unreachable — the service always populates one of @@ -3426,6 +3432,13 @@ export class ShellToolInvocation extends BaseToolInvocation< }; }; const transitionRegistry = (info: ShellPostPromoteSettleInfo) => { + // `task_stop` aborts the entry before the child necessarily reports + // its signal. Preserve the user-intended `cancelled` state instead of + // allowing the later signal settle to overwrite it as `failed`. + if (entryAc.signal.aborted) { + registry.cancel(shellId, info.endTime); + return; + } const cls = classifySettle(info); if (cls.status === 'completed') { registry.complete(shellId, info.exitCode as number, info.endTime); @@ -3715,7 +3728,7 @@ export class ShellToolInvocation extends BaseToolInvocation< } else if ( result.error || (result.exitCode !== null && result.exitCode !== 0) || - result.signal !== null + isSignalTermination(result.signal) ) { // Non-zero exit / killed by signal / spawn error all count as failed. // Treating them as `completed` would let `/tasks` (and any future @@ -3723,7 +3736,7 @@ export class ShellToolInvocation extends BaseToolInvocation< // `false` command as a success. const reason = result.error ? result.error.message - : result.signal !== null + : isSignalTermination(result.signal) ? `terminated by signal ${result.signal}` : `exited with code ${result.exitCode}`; registry.fail(shellId, reason, endTime);