From 6dc9ff40d5c368f109b106e7c6add97779810d4d Mon Sep 17 00:00:00 2001 From: Anmol <91365474+anmol0b@users.noreply.github.com> Date: Tue, 27 Oct 2026 18:27:48 +0530 Subject: [PATCH] fix(acp): report exit status and keep stderr as a log detail (#33190) An unexpected ACP agent exit reported only the whole-run stderr tail, so a startup warning read as the cause, and the observed exit code/signal was dropped. Add the exit code or signal to the ended reason and Details, file the stderr tail as a log diagnostic like the Claude and Codex lanes, and read the exit status from ManagedProviderProcess.exitStatus through a shared formatter that Claude reuses. Fixes #33035 Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Co-authored-by: Neil --- src/main/acp/acp-agent-connection.test.ts | 4 +- src/main/acp/acp-agent-connection.ts | 5 ++ .../acp-scripted-connection.test-fixture.ts | 25 ++++++---- src/main/acp/acp-structured-connection.ts | 1 + .../acp/acp-structured-host-crash.test.ts | 5 +- ...ructured-session-adapter-lifecycle.test.ts | 50 +++++++++++++++++-- src/main/acp/acp-structured-session.ts | 15 ++++-- .../claude/claude-stream-json-connection.ts | 10 ++-- .../claude/claude-stream-json-exit-error.ts | 17 +++---- ...aude-structured-session-exit-blame.test.ts | 4 +- .../managed-provider-process.ts | 5 ++ .../provider-process-exit-status.ts | 12 +++++ 12 files changed, 112 insertions(+), 41 deletions(-) create mode 100644 src/main/provider-process/provider-process-exit-status.ts diff --git a/src/main/acp/acp-agent-connection.test.ts b/src/main/acp/acp-agent-connection.test.ts index bec2bc119c0..3709b925bc9 100644 --- a/src/main/acp/acp-agent-connection.test.ts +++ b/src/main/acp/acp-agent-connection.test.ts @@ -101,7 +101,8 @@ describe('ACP process-owning connection', () => { }) it('settles prompts and permission signals on proven exit with stdout still open', async () => { - const onExit = vi.fn() + const statusDuringExit: unknown[] = [] + const onExit = vi.fn(() => statusDuringExit.push(connection.exitStatus)) const onClose = vi.fn() const permission = deferred() const { connection, child, agent } = fixture({ @@ -132,6 +133,7 @@ describe('ACP process-owning connection', () => { expected: false, exit: { code: 7, signal: null, processless: false } }) + expect(statusDuringExit).toEqual([{ code: 7, signal: null, processless: false }]) const late = vi.fn() connection.onExit(late) expect(late).toHaveBeenCalledOnce() diff --git a/src/main/acp/acp-agent-connection.ts b/src/main/acp/acp-agent-connection.ts index e5504561f46..0bac131d562 100644 --- a/src/main/acp/acp-agent-connection.ts +++ b/src/main/acp/acp-agent-connection.ts @@ -177,6 +177,11 @@ export class AcpAgentConnection extends AcpSessionRuntime { return this.managed.stderrTail().trim() } + /** First managed exit observation, including processless spawn failure. */ + get exitStatus(): ProviderProcessExit | null { + return this.managed.exitStatus + } + onExit(listener: (exit: ProviderProcessExit) => void): void { this.managed.onExit((exit) => { try { diff --git a/src/main/acp/acp-scripted-connection.test-fixture.ts b/src/main/acp/acp-scripted-connection.test-fixture.ts index 402fd8411ec..09a1295b6dd 100644 --- a/src/main/acp/acp-scripted-connection.test-fixture.ts +++ b/src/main/acp/acp-scripted-connection.test-fixture.ts @@ -15,9 +15,9 @@ export class FakeAcpChild extends AcpSessionRuntime implements AcpStructuredConn stderr = '' processTreeUnproven = false private exitListeners: ((exit: ProviderProcessExit) => void)[] = [] - private gone = false private closing = false private lostWith: Error | undefined + private exitedWith: ProviderProcessExit | null = null closes = 0 constructor( @@ -36,11 +36,11 @@ export class FakeAcpChild extends AcpSessionRuntime implements AcpStructuredConn } get exited() { - return this.gone + return this.exitedWith !== null } onExit(listener: (exit: ProviderProcessExit) => void): void { - if (this.gone) { - listener(FAKE_EXIT) + if (this.exitedWith) { + listener(this.exitedWith) } else { this.exitListeners.push(listener) } @@ -48,6 +48,9 @@ export class FakeAcpChild extends AcpSessionRuntime implements AcpStructuredConn stderrTail(): string { return this.stderr } + get exitStatus(): ProviderProcessExit | null { + return this.exitedWith + } pauseReading(): void { this.agent.stdout.pause() } @@ -61,26 +64,26 @@ export class FakeAcpChild extends AcpSessionRuntime implements AcpStructuredConn } override close(error?: Error): Promise { this.closes += 1 - this.closing ||= !this.gone + this.closing ||= this.exitedWith === null this.drainNotifications(error) return this.proveClose().finally(() => super.close(error)) } - exit(): void { - if (this.gone) { + exit(status: ProviderProcessExit = FAKE_EXIT): void { + if (this.exitedWith) { return } - this.gone = true + this.exitedWith = status const error = this.lostWith ?? new AcpConnectionClosedError(this.stderr || `${this.launch.command} exited`) super.close(error) - this.connectionOptions.onExit?.(error, { expected: this.closing, exit: FAKE_EXIT }) + this.connectionOptions.onExit?.(error, { expected: this.closing, exit: status }) for (const listener of this.exitListeners.splice(0)) { - listener(FAKE_EXIT) + listener(status) } } private lost(error: Error): void { this.lostWith ??= error - if (this.closing || this.gone) { + if (this.closing || this.exitedWith) { return } this.connectionOptions.onClose?.(error) diff --git a/src/main/acp/acp-structured-connection.ts b/src/main/acp/acp-structured-connection.ts index c943272615f..c112e1754cb 100644 --- a/src/main/acp/acp-structured-connection.ts +++ b/src/main/acp/acp-structured-connection.ts @@ -17,6 +17,7 @@ export type AcpStructuredConnection = Pick< | 'pid' | 'exited' | 'stderrTail' + | 'exitStatus' | 'onExit' | 'pauseReading' | 'resumeReading' diff --git a/src/main/acp/acp-structured-host-crash.test.ts b/src/main/acp/acp-structured-host-crash.test.ts index 541b8757a92..4038930e3a2 100644 --- a/src/main/acp/acp-structured-host-crash.test.ts +++ b/src/main/acp/acp-structured-host-crash.test.ts @@ -71,7 +71,10 @@ function expectOneTurnScopedCutShortRow(settled: Awaited item.body.kind === 'tool-call')?.body).toMatchObject({ state: 'failed', endedAs: 'interrupted' diff --git a/src/main/acp/acp-structured-session-adapter-lifecycle.test.ts b/src/main/acp/acp-structured-session-adapter-lifecycle.test.ts index 8489b83203d..588cdd62f90 100644 --- a/src/main/acp/acp-structured-session-adapter-lifecycle.test.ts +++ b/src/main/acp/acp-structured-session-adapter-lifecycle.test.ts @@ -209,8 +209,50 @@ describe('ACP connection loss', () => { expect(rig.ended[0]).toMatchObject({ type: 'ended', cause: 'unexpected-exit', - reason: 'grok ACP agent exited: panic: out of memory', - failure: { kind: 'providerExited', detail: { text: 'panic: out of memory' } } + reason: 'grok ACP agent exited (code 0): panic: out of memory', + failure: { kind: 'providerExited', detail: { text: '(code 0)\npanic: out of memory' } } + }) + }) + + it('reports how a crash ended and keeps a startup warning in stderr as a log, not its cause', async () => { + const rig = await openAcpAdapterRig() + await rig.acquire() + const warning = '[pi-web-access] Dynamic tool activation requires Pi 0.86.0 or newer' + rig.child().stderr = warning + rig.child().exit({ code: null, signal: 'SIGTERM', processless: false }) + await waitFor(() => expect(rig.ended).toHaveLength(1)) + expect(rig.ended[0]).toMatchObject({ + cause: 'unexpected-exit', + reason: `grok ACP agent exited (signal SIGTERM): ${warning}`, + failure: { + kind: 'providerExited', + detail: { text: `(signal SIGTERM)\n${warning}`, audience: 'log' } + } + }) + }) + + it('adds the exit status to the stream loss that came before a silent exit', async () => { + const rig = await openAcpAdapterRig() + await rig.acquire() + const child = rig.child() + child.proveClose = async () => false + child.agent.close() + await rig.settle() + child.exit({ code: null, signal: 'SIGKILL', processless: false }) + await waitFor(() => expect(rig.ended).toHaveLength(1)) + expect(rig.ended[0]?.reason).toBe( + 'grok ACP connection closed: ACP connection closed (signal SIGKILL)' + ) + }) + + it('keeps the exit code of a crash that printed nothing to stderr', async () => { + const rig = await openAcpAdapterRig() + await rig.acquire() + rig.child().exit({ code: 1, signal: null, processless: false }) + await waitFor(() => expect(rig.ended).toHaveLength(1)) + expect(rig.ended[0]).toMatchObject({ + reason: 'grok ACP agent exited (code 1)', + failure: { kind: 'providerExited', detail: { text: '(code 1)', audience: 'log' } } }) }) @@ -243,8 +285,8 @@ describe('ACP connection loss', () => { await waitFor(() => expect(rig.ended).toHaveLength(1)) expect(rig.ended[0]).toMatchObject({ cause: 'unexpected-exit', - reason: 'grok ACP agent exited: panic: late', - failure: { kind: 'providerExited', detail: { text: 'panic: late' } } + reason: 'grok ACP agent exited (code 0): panic: late', + failure: { kind: 'providerExited', detail: { text: '(code 0)\npanic: late' } } }) }) diff --git a/src/main/acp/acp-structured-session.ts b/src/main/acp/acp-structured-session.ts index 9bece67dfb1..a738208ed93 100644 --- a/src/main/acp/acp-structured-session.ts +++ b/src/main/acp/acp-structured-session.ts @@ -8,6 +8,7 @@ import { UNVERIFIABLE_TURN_VERDICT, type StructuredAgentSessionTurnVerdict } from '../native-chat/agent-session-wire/structured-agent-session-stale-turn-verdict' +import { providerExitStatusText } from '../provider-process/provider-process-exit-status' import type { AcpLaunchSpec } from './acp-launch-specs' import type { AcpChildStops } from './acp-dialects/acp-dialect' import { acpAgentName } from './acp-structured-agent-definitions' @@ -113,12 +114,15 @@ export function endAcpStructuredSession( session.events?.observeExit?.(undefined, observedAt) session.exitObservedAt = observedAt const stderr = session.connection.stderrTail() - // Process exit can follow stream loss; final stderr explains the crash. + const how = providerExitStatusText(session.connection.exitStatus) + // Process exit can follow stream loss; keep that earlier reason when stderr is absent. + const lead = + session.journalClosed !== null && !stderr + ? session.journalClosed + : `${session.spec.agent} ACP agent exited` const reason = session.closeRequested ? `${session.spec.agent} ACP agent closed by Orca` - : session.journalClosed !== null && !stderr - ? session.journalClosed - : `${session.spec.agent} ACP agent exited${stderr ? `: ${stderr}` : ''}` + : `${lead}${how ? ` ${how}` : ''}${stderr ? `: ${stderr}` : ''}` // Spontaneous exit interrupts now; requested close leaves the host to revise an unverifiable turn. closeAcpSessionJournal( session, @@ -129,7 +133,8 @@ export function endAcpStructuredSession( : { state: 'interrupted', completedAt: observedAt }, observedAt ) - const detail = stderr ? providerDiagnostic(stderr, 'person') : undefined + // The tail spans the whole run, so a startup warning can still be in it: a log, not the cause. + const detail = providerDiagnostic([how, stderr].filter(Boolean).join('\n'), 'log') onEvent?.({ type: 'ended', sessionId: session.sessionId, diff --git a/src/main/claude/claude-stream-json-connection.ts b/src/main/claude/claude-stream-json-connection.ts index 0ccbb3464eb..59b81675614 100644 --- a/src/main/claude/claude-stream-json-connection.ts +++ b/src/main/claude/claude-stream-json-connection.ts @@ -20,7 +20,7 @@ import { } from './claude-agent-sdk-user-message-queue' import type { ClaudeStructuredSdkOptions } from './claude-structured-launch-resolution' import { withMissingProviderExecutable } from '../provider-process/provider-executable-missing' -import { claudeStreamJsonExitError, type ClaudeExitStatus } from './claude-stream-json-exit-error' +import { claudeStreamJsonExitError } from './claude-stream-json-exit-error' export { ClaudeControlRequestError } @@ -125,7 +125,6 @@ export async function openClaudeStreamJsonConnection( if (!child || !managed) { throw new Error('the claude agent SDK returned without spawning a child') } - let exitStatus: ClaudeExitStatus | null = null let closing = false let terminalError: Error | null = null let faultReported = false @@ -170,7 +169,7 @@ export async function openClaudeStreamJsonConnection( const handleUnexpectedEnd = (cause?: Error): void => { resumeReading() if (!terminalError) { - const error = claudeStreamJsonExitError(managed.stderrTail(), exitStatus, cause) + const error = claudeStreamJsonExitError(managed.stderrTail(), managed.exitStatus, cause) terminalError = managed.executableMissing ? withMissingProviderExecutable(error) : error } inbox.fail(terminalError) @@ -216,10 +215,7 @@ export async function openClaudeStreamJsonConnection( } })() - managed.onExit((exit) => { - exitStatus = exit - handleUnexpectedEnd() - }) + managed.onExit(() => handleUnexpectedEnd()) child.on('error', (error) => { if (!closing && managed.rootVerdict !== 'exited') { void tree.reap() diff --git a/src/main/claude/claude-stream-json-exit-error.ts b/src/main/claude/claude-stream-json-exit-error.ts index d095ed9b6a3..5807164d2d2 100644 --- a/src/main/claude/claude-stream-json-exit-error.ts +++ b/src/main/claude/claude-stream-json-exit-error.ts @@ -1,27 +1,22 @@ import { providerDiagnostic, withProviderDiagnostic } from '../../shared/agent-session-failure' +import type { ProviderProcessExit } from '../provider-process/managed-provider-process' +import { providerExitStatusText } from '../provider-process/provider-process-exit-status' import { providerStderrForDisplay } from '../provider-process/provider-spawn-failure-report' import { AgentSessionAcquisitionRefusal } from '../native-chat/agent-session-wire/structured-agent-session-adapter' -export type ClaudeExitStatus = { code: number | null; signal: NodeJS.Signals | null } - // Claude Code's own startup refusal: as root it exits before initialize rather than skip permissions. const CLAUDE_ROOT_FULL_ACCESS_REFUSAL = /--dangerously-skip-permissions cannot be used with root/ export function claudeStreamJsonExitError( stderrTail: string, - status: ClaudeExitStatus | null, + status: ProviderProcessExit | null, cause?: Error ): Error { const detail = providerStderrForDisplay(stderrTail).trim() // Retain exit status and stderr through wrappers for refused-start diagnostics. - const how = - status?.signal !== null && status?.signal !== undefined - ? ` (signal ${status.signal})` - : status?.code !== null && status?.code !== undefined - ? ` (code ${status.code})` - : '' - const message = `claude stream-json exited${how}${detail ? `: ${detail}` : ''}` - const diagnostic = providerDiagnostic([how.trim(), detail].filter(Boolean).join('\n'), 'log') + const how = providerExitStatusText(status) + const message = `claude stream-json exited${how ? ` ${how}` : ''}${detail ? `: ${detail}` : ''}` + const diagnostic = providerDiagnostic([how, detail].filter(Boolean).join('\n'), 'log') // The typed refusal replaces this error in failure facts, so it carries the diagnostic too. const refusal = CLAUDE_ROOT_FULL_ACCESS_REFUSAL.test(detail) ? withProviderDiagnostic( diff --git a/src/main/claude/claude-structured-session-exit-blame.test.ts b/src/main/claude/claude-structured-session-exit-blame.test.ts index 14ac3c8e631..16f771fa02e 100644 --- a/src/main/claude/claude-structured-session-exit-blame.test.ts +++ b/src/main/claude/claude-structured-session-exit-blame.test.ts @@ -46,7 +46,9 @@ describe('what a started Claude session says ended it', () => { const refusal = '--dangerously-skip-permissions cannot be used with root/sudo privileges for security reasons' - connection.handlers.onExit?.(claudeStreamJsonExitError(refusal, { code: 1, signal: null })) + connection.handlers.onExit?.( + claudeStreamJsonExitError(refusal, { code: 1, signal: null, processless: false }) + ) expect(await ended()).toMatchObject({ cause: 'unexpected-exit', diff --git a/src/main/provider-process/managed-provider-process.ts b/src/main/provider-process/managed-provider-process.ts index b0bd860c5fb..9db341679a5 100644 --- a/src/main/provider-process/managed-provider-process.ts +++ b/src/main/provider-process/managed-provider-process.ts @@ -61,6 +61,8 @@ export type ManagedProviderProcess = { readonly rootExitObserved: boolean /** The last close that ran the ladder; the already-exited answer only when none did. */ readonly lastCloseResult: ProviderProcessCloseResult | null + /** First exit observation, including processless spawn failure; null before either is observed. */ + readonly exitStatus: ProviderProcessExit | null readonly exitPromise: Promise /** Direct or supervisor-reported ENOENT for the provider executable. */ readonly executableMissing: boolean @@ -166,6 +168,9 @@ export function spawnManagedProviderProcess( child, supervised: spec.supervised, exitPromise, + get exitStatus() { + return observed + }, get processless() { return observed?.processless ?? false }, diff --git a/src/main/provider-process/provider-process-exit-status.ts b/src/main/provider-process/provider-process-exit-status.ts new file mode 100644 index 00000000000..f39a8f92483 --- /dev/null +++ b/src/main/provider-process/provider-process-exit-status.ts @@ -0,0 +1,12 @@ +import type { ProviderProcessExit } from './managed-provider-process' + +/** How a provider process ended, e.g. `(signal SIGTERM)` or `(code 1)`; empty when nothing was observed. */ +export function providerExitStatusText(exit: ProviderProcessExit | null): string { + if (!exit) { + return '' + } + if (exit.signal !== null) { + return `(signal ${exit.signal})` + } + return exit.code === null ? '' : `(code ${exit.code})` +}