fix(acp): address review of the terminal-card alignment (exit parse, background/error, capability snapshot)
Codex + an independent review pass found three real defects in the prior commit: 1. parseExitStatus could misreport a SUCCESSFUL command as a failure: a clean exit 0 appends no marker, so output ending in "[exit code: 5]" (no trailing newline) was read as the marker. Anchor the parse to a LEADING newline — renderResult always inserts one before a real marker, so a body that merely ends in marker-like text no longer matches. A narrow residual (a clean exit 0 whose final line is exactly the marker) is inherent to the replay-only-sees- text design and documented; the complete fix (a structured exit on the event) is the RFC's named escape hatch. 2. A run_in_background start and an isError result were rendered as exited terminal cards with a false exit-0 pill. A background start returns a task-id ack (not a streamed terminal) and is no longer marked terminal; an isError result (spawn failure / abort) carries no exit pill. 3. The terminal capability was re-read live on the result path, so a second initialize between a call and its result could desync them (orphan terminal_output or clobbered card). Snapshot the capability per session at creation (SessionRecord.terminalEnabled) so call and result always agree. Also reword the reference-parity claim: keeping the description as a content block in terminal mode is a DELIBERATE divergence (claude-agent-acp drops it). Tests added for each; with-key e2e still green.
This commit is contained in:
@@ -141,6 +141,17 @@ interface SessionRecord {
|
||||
* tool state.
|
||||
*/
|
||||
presenter: ToolPresenter
|
||||
/**
|
||||
* Whether THIS session renders shell tools as terminal cards — snapshotted
|
||||
* from the client's `_meta.terminal_output` capability at session creation
|
||||
* (`session/new`/`session/load`), NOT re-read live. A capability snapshot per
|
||||
* session means the `tool_call` (which registers the terminal) and the matching
|
||||
* `tool_call_update` (which streams its output) ALWAYS agree, even if a later
|
||||
* `initialize` mutates the connection-level capability between them — otherwise
|
||||
* a re-`initialize` mid-call could orphan a `terminal_output` (call non-terminal,
|
||||
* result terminal) or clobber the card (call terminal, result non-terminal).
|
||||
*/
|
||||
terminalEnabled: boolean
|
||||
/**
|
||||
* The in-flight `session/prompt`, or `undefined` when none is pending. A
|
||||
* prompt resolves with a {@link StopReason} or rejects with an Error (a
|
||||
@@ -290,7 +301,7 @@ export function apply(ctx: Context, config: AcpConfig): void {
|
||||
const rec = sessions.get(session.header.id)
|
||||
if (rec === undefined) return
|
||||
streamSessionEventUpdate(rec.sessionId, event, notify, rec.presenter, {
|
||||
enabled: terminalOutputCap,
|
||||
enabled: rec.terminalEnabled,
|
||||
cwd: session.header.cwd,
|
||||
})
|
||||
const inflight = rec.inflight
|
||||
@@ -422,7 +433,7 @@ export function apply(ctx: Context, config: AcpConfig): void {
|
||||
agentOptions: agentOptions(config),
|
||||
})
|
||||
bySession.set(agent, sessionId)
|
||||
sessions.set(sessionId, { sessionId, agent, presenter: makePresenter(), inflight: undefined })
|
||||
sessions.set(sessionId, { sessionId, agent, presenter: makePresenter(), terminalEnabled: terminalOutputCap, inflight: undefined })
|
||||
return Promise.resolve({ sessionId })
|
||||
},
|
||||
|
||||
@@ -475,7 +486,13 @@ export function apply(ctx: Context, config: AcpConfig): void {
|
||||
throw invalidParams('connection closed during session/load')
|
||||
}
|
||||
bySession.set(agent, params.sessionId)
|
||||
const record: SessionRecord = { sessionId: params.sessionId, agent, presenter: makePresenter(), inflight: undefined }
|
||||
// Snapshot the terminal capability ONCE for this session (used by both
|
||||
// the replay below and the post-load live stream) so a later
|
||||
// `initialize` can't desync the call/result of a tool card.
|
||||
const terminalEnabled = terminalOutputCap
|
||||
const record: SessionRecord = {
|
||||
sessionId: params.sessionId, agent, presenter: makePresenter(), terminalEnabled, inflight: undefined,
|
||||
}
|
||||
sessions.set(params.sessionId, record)
|
||||
// Replay the persisted event log to the client as session/update. Use
|
||||
// the raw event log (NOT deriveMessages, which drops assistant/chunk
|
||||
@@ -492,7 +509,7 @@ export function apply(ctx: Context, config: AcpConfig): void {
|
||||
// so the record's presenter starts clean for the post-load live stream.
|
||||
const replayPresenter = makePresenter()
|
||||
const replayTerminal: TerminalRendering = {
|
||||
enabled: terminalOutputCap,
|
||||
enabled: terminalEnabled,
|
||||
cwd: agent.session.header.cwd,
|
||||
}
|
||||
for (const event of agent.session.events) {
|
||||
|
||||
Reference in New Issue
Block a user