mirror of
https://github.com/stablyai/orca.git
synced 2026-10-30 16:47:34 +00:00
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 <neil@stably.ai>
This commit is contained in:
co-authored by
Brennan Benson
Neil
parent
4583236a24
commit
6dc9ff40d5
@@ -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<AbortSignal>()
|
||||
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()
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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<boolean> {
|
||||
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)
|
||||
|
||||
@@ -17,6 +17,7 @@ export type AcpStructuredConnection = Pick<
|
||||
| 'pid'
|
||||
| 'exited'
|
||||
| 'stderrTail'
|
||||
| 'exitStatus'
|
||||
| 'onExit'
|
||||
| 'pauseReading'
|
||||
| 'resumeReading'
|
||||
|
||||
@@ -71,7 +71,10 @@ function expectOneTurnScopedCutShortRow(settled: Awaited<ReturnType<typeof settl
|
||||
expect(row?.turnScope).toEqual({ kind: 'turn', turnItemId: turn?.itemId })
|
||||
expect(
|
||||
readAgentSessionFailureFact(row?.body.kind === 'status' ? row.body.failure : undefined)
|
||||
).toEqual({ kind: 'providerExited', detail: { text: LAST_WORDS, audience: 'person' } })
|
||||
).toEqual({
|
||||
kind: 'providerExited',
|
||||
detail: { text: `(code 0)\n${LAST_WORDS}`, audience: 'log' }
|
||||
})
|
||||
expect(rows.find((item) => item.body.kind === 'tool-call')?.body).toMatchObject({
|
||||
state: 'failed',
|
||||
endedAs: 'interrupted'
|
||||
|
||||
@@ -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' } }
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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',
|
||||
|
||||
@@ -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<void>
|
||||
/** 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
|
||||
},
|
||||
|
||||
@@ -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})`
|
||||
}
|
||||
Reference in New Issue
Block a user