From 75d6eb3ef69362de7b6908a03f9adb1e54b42e11 Mon Sep 17 00:00:00 2001 From: "jinye.djy" Date: Sun, 30 Aug 2026 21:32:55 +0800 Subject: [PATCH 1/2] fix(serve): surface provider error detail in turn_error messages Daemon turns that failed because the model provider rejected the request published a generic "Internal error" as the turn_error message, hiding the upstream reason (e.g. engine_overloaded_error) from the Web Shell. Teach the bridge's error-message extractor to read the nested data.error shape the ACP SDK produces when the agent's error message is itself a JSON string, with the same precedence the desktop client uses. Fixes #10564 --- .../2026-08-30-turn-error-provider-detail.md | 137 ++++++++++++++++++ docs/developers/daemon/09-event-schema.md | 20 +-- .../cli/qwen-serve-streaming.test.ts | 94 ++++++++++++ integration-tests/fake-openai-server.test.ts | 37 +++++ integration-tests/fake-openai-server.ts | 15 ++ packages/acp-bridge/src/bridge.test.ts | 56 +++++++ packages/acp-bridge/src/bridge.ts | 15 +- 7 files changed, 363 insertions(+), 11 deletions(-) create mode 100644 docs/design/2026-08-30-turn-error-provider-detail.md diff --git a/docs/design/2026-08-30-turn-error-provider-detail.md b/docs/design/2026-08-30-turn-error-provider-detail.md new file mode 100644 index 00000000000..dceea3886fa --- /dev/null +++ b/docs/design/2026-08-30-turn-error-provider-detail.md @@ -0,0 +1,137 @@ +# Surface provider error detail in `turn_error` messages + +Date: 2026-08-30 +Status: proposed + +## Problem statement + +When a daemon-hosted prompt turn fails because the model provider rejects the +request, the Web Shell shows the bare text `Internal error`. The actual +provider reason (e.g. `The engine is currently overloaded, please try again +later`) never reaches any user-visible surface, so users cannot distinguish a +transient provider overload from a genuine daemon bug. Observed in production +on session `eeab9c1c-305d-4259-8a37-2ef8d07ca934`: four turn failures, all +displayed as `Internal error`, all caused by the upstream +`engine_overloaded_error` body. + +## Current state + +The error travels through five layers; the detail is dropped at the third: + +1. **core (agent child).** The provider returns the error as a stream chunk + (`finish_reason: "error_finish"`, JSON body in `delta.content`). The + pipeline throws `StreamContentError` whose `.message` is the raw upstream + JSON string + (`packages/core/src/core/openaiContentGenerator/pipeline.ts`). +2. **ACP SDK (agent child).** The catch-all in `#tryCallRequestHandler` + serializes a non-`RequestError` throw as + `RequestError.internalError(JSON.parse(error.message))` — i.e. wire shape + `{code: -32603, message: 'Internal error', data: }`. + For a JSON message like ours this yields + `data: {error: {message, type}}`; for a plain-text message that fails + `JSON.parse` it yields `data: {details: }` + (`@agentclientprotocol/sdk`, `acp.js`). +3. **acp-bridge (daemon).** `broadcastTurnError` + (`packages/acp-bridge/src/bridge.ts`) builds the `turn_error` SSE event + with `message = extractErrorMessage(err)`. `extractJsonRpcErrorDetail` + today reads a string `data`, `data.details`, and `data.message` — but + **not** the nested `data.error.message` the JSON-parsed shape produces. + It therefore falls back to `err.message`, the generic `Internal error`. +4. **sdk-typescript.** `turn_error` validates with only + `sessionId` + `message` required and normalizes to a + `DaemonUiErrorEvent` whose `text` is that message. +5. **webui.** `turn_error` stays in the transcript (never routes to notices) + and renders as a `system_error` block whose text is the event message. + +Because the plain-text shape (`data.details`) already surfaces through the +existing extractor, only the JSON-parsed shape loses its detail. The gap is a +missing branch in one helper, not an absent wire field. + +`extractErrorMessage` has exactly five call sites, all in `acp-bridge`, and +all are display or log surfaces with no string-matching behavioral consumer: + +| Call site | Surface | +| ------------------------------------------- | ---------------------------------------------------------------------------------- | +| `bridge.ts` `broadcastTurnError` | `turn_error` event message, `entry.turnError`, refresh replay, turn-status overlay | +| `bridge.ts` `sendPrompt` forward-failed log | daemon stderr | +| `bridge.ts` cancel-forward-failed log | daemon stderr | +| `bridge.ts` approval-mode restore log | daemon stderr | +| `bridge.ts` channel quarantine log | daemon stderr | +| `transcript-replay.ts` tool-result replay | restored transcript error text | + +`classifyTurnErrorKind` exact-matches the message against `terminated`; that +error arrives via the plain-text `data.details` shape and is unaffected by +adding a nested-`error` branch. + +## Proposed change + +Extend `extractJsonRpcErrorDetail` in `packages/acp-bridge/src/bridge.ts` +with one more fallback: after the existing `data` string / `data.details` / +`data.message` checks, read a nested `data.error` — accepting either a plain +string or an object with a string `message` (mirroring the desktop client's +`getAcpErrorDetail`, which has shipped the same precedence for ACP internal +errors). Update the `extractErrorMessage` doc comment to name the +`data.error.message` shape as the ACP SDK's JSON-parsed-message artifact. + +That is the entire production change. `broadcastTurnError` then publishes the +provider's own message as `turn_error.data.message`, and every downstream +surface — the Web Shell transcript error block, the live-state `turnError` +summary, refresh replay, `DaemonClient`'s `DaemonHttpError`, and the daemon +stderr logs — shows the real reason with no wire schema change and no +SDK/webui/desktop edits. + +Result for the motivating failure: the transcript shows +`The engine is currently overloaded, please try again later` instead of +`Internal error`. + +## Key decisions + +- **Fix at message extraction, not an additive wire field.** An alternative + considered was adding `turn_error.data.detail` and plumbing it through + `DaemonTurnErrorData`, the UI normalizer, transcript block types, and the + webui adapter. That preserves the generic `message` for hypothetical + string matchers but costs four packages of churn for the same user-visible + result. Every actual `message` consumer is enumerated above and is a + display/log surface, so enriching `message` directly is safe and minimal. +- **Precedence matches desktop.** Top-level `details`/`message` win over the + nested `error.message`, identical to `formatQwenAcpErrorMessage`'s + `getAcpErrorDetail` in + `packages/desktop/packages/shared/src/agent/qwen-agent.ts`. One consistent + de-facto contract for ACP `-32603` data across clients. +- **No new length bound.** The existing `data.details` path is already + unbounded, so a pathological provider blob flows today; this change keeps + parity instead of inventing a second policy. Render-side control-character + sanitization (`sanitizeDaemonTerminalText`) and event-bus frame byte + accounting already apply. A bound can be revisited as its own change. +- **No new `errorKind`.** `DaemonErrorKind` stays closed; this change only + improves human-readable text. A structured kind (e.g. provider-overload) + belongs with retry-classification work, not text plumbing. + +## Files affected + +| File | Change | +| ---------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `packages/acp-bridge/src/bridge.ts` | nested `data.error` branch in `extractJsonRpcErrorDetail`; doc comment | +| `packages/acp-bridge/src/bridge.test.ts` | new `extractErrorMessage` cases: nested `error.message`, string `error`, precedence over/absence of top-level keys, non-JSON fallback unchanged | +| `integration-tests/fake-openai-server.ts` | test infrastructure: `errorContent` choice field emitting a single `error_finish` chunk (error body and finish reason on the same chunk, matching real gateways) | +| `integration-tests/fake-openai-server.test.ts` | self-test pinning the same-chunk invariant | +| `integration-tests/cli/qwen-serve-streaming.test.ts` | regression case: fake OpenAI server emits an `error_finish` chunk with a JSON error body; assert the session SSE `turn_error` `message` carries the provider text | +| `docs/developers/daemon/09-event-schema.md` | `turn_error` row: note that `message` may carry provider-supplied detail for `-32603` agent failures | + +## Scope boundaries + +- No retry-behavior change. Classifying `engine_overloaded_error` as + retryable (503-equivalent) is a separate, independent follow-up. +- No change to `matchTurnEvent`, `normalizeTurnResultError`, the prompt + ledger record shape, or the Java SDK. +- TUI (non-daemon) error rendering is untouched; it does not traverse the + bridge extractor. +- No `DaemonErrorKind` additions and no Web Shell UI restructuring. + +## Open questions + +- Should `normalizeTurnResultError` (turn-status polling overlay) also carry + the provider `type` (e.g. `engine_overloaded_error`) as a structured code? + Deferred — current overlay consumers only display `message`. +- If provider blobs prove unwieldy in practice, where should the length cap + live — the bridge extractor or the event publisher? Deferred until observed. diff --git a/docs/developers/daemon/09-event-schema.md b/docs/developers/daemon/09-event-schema.md index 431d5a2abe0..59b0b6ee500 100644 --- a/docs/developers/daemon/09-event-schema.md +++ b/docs/developers/daemon/09-event-schema.md @@ -126,16 +126,16 @@ These events are workspace-keyed, not session-keyed. The session reducer treats ### Turn lifecycle / assistant pushes -| Type | Direction | Trigger | Key payload fields | -| --------------------- | --------- | ------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `prompt_cancelled` | S->C | Prompt was cancelled through explicit `cancelSession` route **or** originator SSE disconnect | Envelope stamps `originatorClientId` for the canceling client. This means "cancellation requested", not "cancellation confirmed". Peer subscribers learn that the prompt has ended. | -| `turn_complete` | S->C | A turn completed successfully | `sessionId, stopReason, promptId?, branchPoint?`. `promptId` links to non-blocking prompt responses (`202`). Eligible completed turns include `branchPoint: { assistantRecordUuid, checkpointUuid }`. | -| `turn_error` | S->C | A turn failed | `sessionId, message, code?, promptId?`; same `promptId` correlation mechanism. | -| `session_rewound` | S->C | `POST /session/:id/rewind` succeeded | `sessionId, promptId, targetTurnIndex, filesChanged[], filesFailed[], originatorClientId?` | -| `session_branched` | S->C | Legacy compatibility event; the current branch endpoint returns its result directly and does not publish this event | `sourceSessionId, newSessionId, displayName, originatorClientId?`. Readers retain support for older producers. | -| `followup_suggestion` | S->C | ACP child generated ghost-text follow-up suggestions after `end_turn`, forwarded over per-session SSE | `sessionId, suggestion, promptId`; wire only carries suggestions whose `getFilterReason()===null`. Clients render them as input-placeholder ghost text and invalidate them on next `sendPrompt`. | -| `user_shell_command` | S->C | User started a shell command through `POST /session/:id/shell`; fanned out to other subscribers in the same session | `sessionId, command, shellId, originatorClientId?`. There is no typed `DaemonXxxData` interface yet; `asKnownDaemonEvent` returns `undefined` and the UI normalizer parses it ad hoc. | -| `user_shell_result` | S->C | Result of the shell command above | `sessionId, shellId, exitCode, output, aborted`. Same ad hoc parsing note as `user_shell_command`. | +| Type | Direction | Trigger | Key payload fields | +| --------------------- | --------- | ------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `prompt_cancelled` | S->C | Prompt was cancelled through explicit `cancelSession` route **or** originator SSE disconnect | Envelope stamps `originatorClientId` for the canceling client. This means "cancellation requested", not "cancellation confirmed". Peer subscribers learn that the prompt has ended. | +| `turn_complete` | S->C | A turn completed successfully | `sessionId, stopReason, promptId?, branchPoint?`. `promptId` links to non-blocking prompt responses (`202`). Eligible completed turns include `branchPoint: { assistantRecordUuid, checkpointUuid }`. | +| `turn_error` | S->C | A turn failed | `sessionId, message, code?, promptId?`; same `promptId` correlation mechanism. For agent-side `-32603` failures `message` carries the provider's own detail when the agent supplied one (`data.details` / `data.message` / nested `data.error.message`), not the generic JSON-RPC text. | +| `session_rewound` | S->C | `POST /session/:id/rewind` succeeded | `sessionId, promptId, targetTurnIndex, filesChanged[], filesFailed[], originatorClientId?` | +| `session_branched` | S->C | Legacy compatibility event; the current branch endpoint returns its result directly and does not publish this event | `sourceSessionId, newSessionId, displayName, originatorClientId?`. Readers retain support for older producers. | +| `followup_suggestion` | S->C | ACP child generated ghost-text follow-up suggestions after `end_turn`, forwarded over per-session SSE | `sessionId, suggestion, promptId`; wire only carries suggestions whose `getFilterReason()===null`. Clients render them as input-placeholder ghost text and invalidate them on next `sendPrompt`. | +| `user_shell_command` | S->C | User started a shell command through `POST /session/:id/shell`; fanned out to other subscribers in the same session | `sessionId, command, shellId, originatorClientId?`. There is no typed `DaemonXxxData` interface yet; `asKnownDaemonEvent` returns `undefined` and the UI normalizer parses it ad hoc. | +| `user_shell_result` | S->C | Result of the shell command above | `sessionId, shellId, exitCode, output, aborted`. Same ad hoc parsing note as `user_shell_command`. | ## Architecture diff --git a/integration-tests/cli/qwen-serve-streaming.test.ts b/integration-tests/cli/qwen-serve-streaming.test.ts index 8a425edfcdd..4f5822f9b3f 100644 --- a/integration-tests/cli/qwen-serve-streaming.test.ts +++ b/integration-tests/cli/qwen-serve-streaming.test.ts @@ -197,6 +197,27 @@ beforeAll(async () => { return { content: 'The test Todo remains unfinished.' }; } + // Gate out the memory-selection side query (its system prompt starts + // with "You are selecting memories"): it replays the prompt text, so a + // bare marker match would error it too and muddy the request count. + if ( + messages.includes('turn-error-provider-detail-e2e') && + !messages.includes('You are selecting memories') + ) { + // Reproduces the production failure shape: the gateway reports an + // upstream overload as a single `error_finish` chunk whose content is + // the provider's JSON error body instead of an HTTP error status. + return { + errorContent: JSON.stringify({ + error: { + message: + 'The engine is currently overloaded, please try again later', + type: 'engine_overloaded_error', + }, + }), + }; + } + if (messages.includes('turn-final-answer-boundary-e2e')) { const toolCallId = 'call_turn_final_answer_boundary'; if (!messages.includes(toolCallId)) { @@ -537,6 +558,79 @@ describePOSIX('qwen serve — pollable turn results', () => { }, 60_000); }); +describePOSIX('qwen serve — turn_error provider detail', () => { + it('publishes the provider message as the turn_error message', async () => { + const marker = 'turn-error-provider-detail-e2e'; + const session = await client.createOrAttachSession({ + workspaceCwd: workspaceDir, + sessionScope: 'thread', + }); + + const events: DaemonEvent[] = []; + const ac = new AbortController(); + let promptId: string | undefined; + const subscriber = (async () => { + try { + for await (const event of sseFrames(session.sessionId, { + signal: ac.signal, + })) { + events.push(event); + const data = event.data as { promptId?: string } | undefined; + if (event.type === 'turn_error' && data?.promptId === promptId) { + break; + } + } + } catch { + /* aborted */ + } + })(); + + const requestStart = fakeServer.requests.length; + try { + await new Promise((resolve) => setTimeout(resolve, 200)); + const accepted = asAccepted( + await client.promptNonBlocking(session.sessionId, { + prompt: [{ type: 'text', text: marker }], + }), + ); + expect(accepted).toBeDefined(); + if (!accepted) return; + promptId = accepted.promptId; + + const findTurnError = () => + events.find((event) => { + if (event.type !== 'turn_error') return false; + const data = event.data as { promptId?: string } | undefined; + return data?.promptId === promptId; + }); + await expect.poll(findTurnError, { timeout: 30_000 }).toBeDefined(); + + const data = findTurnError()!.data as { message?: string }; + expect(data.message).toBe( + 'The engine is currently overloaded, please try again later', + ); + // The JSON error body carries no numeric code, so no rate-limit retry + // fires: the turn must fail after exactly one model request. The + // memory-selection side query also carries the marker text (it replays + // the prompt), so exclude it from the count the same way the fake + // server callback does. + const modelRequests = fakeServer.requests + .slice(requestStart) + .map((request) => JSON.stringify(request.body['messages'] ?? [])) + .filter( + (messages) => + messages.includes(marker) && + !messages.includes('You are selecting memories'), + ); + expect(modelRequests).toHaveLength(1); + } finally { + ac.abort(); + await subscriber; + await client.closeSession(session.sessionId).catch(() => undefined); + } + }, 60_000); +}); + describePOSIX('qwen serve — child-crash recovery (real SIGKILL)', () => { it('publishes session_died after the qwen --acp child is SIGKILL-ed', async () => { const session = await client.createOrAttachSession({ diff --git a/integration-tests/fake-openai-server.test.ts b/integration-tests/fake-openai-server.test.ts index 495705f6c77..e5fcfe2297c 100644 --- a/integration-tests/fake-openai-server.test.ts +++ b/integration-tests/fake-openai-server.test.ts @@ -89,6 +89,43 @@ describe('fake OpenAI server', () => { expect(server.requests).toHaveLength(2); }); + it('streams errorContent as a single error_finish chunk', async () => { + server = await startFakeOpenAIServer(() => ({ + errorContent: '{"error":{"message":"overloaded"}}', + })); + + const response = await fetch(`${server.baseUrl}/chat/completions`, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ + model: 'fake-model', + stream: true, + messages: [{ role: 'user', content: 'hi' }], + }), + }); + + expect(response.status).toBe(200); + const streamText = await response.text(); + expect(streamText).toContain('"finish_reason":"error_finish"'); + expect(streamText).toContain('overloaded'); + expect(streamText).toContain('data: [DONE]'); + // The error body and finish reason must ride the SAME chunk: the CLI + // pipeline reads delta.content off the chunk that carries + // finish_reason === 'error_finish'. + const errorFrame = streamText + .split('\n\n') + .find((frame) => frame.includes('error_finish')); + expect(errorFrame).toBeDefined(); + const payload = JSON.parse( + errorFrame!.replace(/^data: /, ''), + ) as unknown as { + choices: Array<{ delta: { content?: string } }>; + }; + expect(payload.choices[0]?.delta.content).toBe( + '{"error":{"message":"overloaded"}}', + ); + }); + it('can close response connections for isolated latency measurements', async () => { server = await startFakeOpenAIServer( () => ({ content: 'no connection reuse' }), diff --git a/integration-tests/fake-openai-server.ts b/integration-tests/fake-openai-server.ts index 7ba74a20930..a6c692380d3 100644 --- a/integration-tests/fake-openai-server.ts +++ b/integration-tests/fake-openai-server.ts @@ -29,6 +29,7 @@ export type FakeOpenAIResponse = { model?: string; content?: string; contentChunks?: string[]; + errorContent?: string; disconnectAfterContentChunks?: number; toolCalls?: FakeOpenAIToolCall[]; finishReason?: 'stop' | 'tool_calls' | 'length'; @@ -47,6 +48,13 @@ export type FakeOpenAIChoice = { index: number; content?: string; contentChunks?: string[]; + /** + * Streaming-only mid-stream provider error: emits one chunk carrying this + * string as `delta.content` together with `finish_reason: 'error_finish'`, + * then ends the stream. Mirrors gateways that report upstream failures as + * stream content instead of an HTTP error status. + */ + errorContent?: string; toolCalls?: FakeOpenAIToolCall[]; finishReason?: 'stop' | 'tool_calls' | 'length'; }; @@ -298,6 +306,12 @@ function writeStreamed( const choices = responseChoices(message); for (const [choicePosition, choice] of choices.entries()) { send(chunk(choice.index, { role: 'assistant' })); + if (choice.errorContent !== undefined) { + send( + chunk(choice.index, { content: choice.errorContent }, 'error_finish'), + ); + continue; + } for (const [contentIndex, content] of ( choice.contentChunks ?? [] ).entries()) { @@ -373,6 +387,7 @@ function responseChoices(message: FakeOpenAIResponse): FakeOpenAIChoice[] { index: 0, content: message.content, contentChunks: message.contentChunks, + errorContent: message.errorContent, toolCalls: message.toolCalls, finishReason: message.finishReason, }, diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 511f17d9c58..14c677a9287 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -29579,6 +29579,62 @@ describe('extractErrorMessage', () => { ).toBe('<503> model serving is throttled'); }); + it('extracts nested provider messages from data.error.message', () => { + // Shape the ACP SDK produces when the agent throws an error whose + // message is a JSON string: internalError(JSON.parse(message)). + expect( + extractErrorMessage( + new RequestError(-32603, 'Internal error', { + error: { + message: + 'The engine is currently overloaded, please try again later', + type: 'engine_overloaded_error', + }, + }), + ), + ).toBe('The engine is currently overloaded, please try again later'); + }); + + it('extracts string data.error from JSON-RPC error data', () => { + expect( + extractErrorMessage({ + code: -32603, + message: 'Internal error', + data: { error: 'plain string detail' }, + }), + ).toBe('plain string detail'); + }); + + it('prefers top-level data.details over nested data.error.message', () => { + expect( + extractErrorMessage({ + code: -32603, + message: 'Internal error', + data: { details: 'top', error: { message: 'nested' } }, + }), + ).toBe('top'); + }); + + it('prefers top-level data.message over nested data.error.message', () => { + expect( + extractErrorMessage({ + code: -32603, + message: 'Internal error', + data: { message: 'top', error: { message: 'nested' } }, + }), + ).toBe('top'); + }); + + it('falls back to message when data.error has no usable string', () => { + expect( + extractErrorMessage( + new RequestError(-32603, 'Internal error', { + error: { type: 'engine_overloaded_error' }, + }), + ), + ).toBe('Internal error'); + }); + it('extracts details from Error subclasses with JSON-RPC data', () => { expect( extractErrorMessage( diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 05f2761ca5b..649669b0629 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -1765,7 +1765,10 @@ function broadcastTurnComplete( * objects with a `message` property. * JSON-RPC internal errors carry the generic `"Internal error"` as * `message`; the actual detail often lives in `data.details` or - * provider-specific `data.message`. + * provider-specific `data.message`. When the agent throws an error whose + * message is itself a JSON string (e.g. a provider error body surfaced as + * stream content), the ACP SDK ships `JSON.parse(message)` as `data`, which + * nests the provider text at `data.error.message` — read that shape too. */ export function extractErrorMessage(err: unknown): string { if (err instanceof Error) { @@ -1790,6 +1793,16 @@ function extractJsonRpcErrorDetail(data: unknown): string | undefined { if (typeof details === 'string' && details.length > 0) return details; const message = (data as Record)['message']; if (typeof message === 'string' && message.length > 0) return message; + return extractNestedErrorDetail((data as Record)['error']); + } + return undefined; +} + +function extractNestedErrorDetail(error: unknown): string | undefined { + if (typeof error === 'string' && error.length > 0) return error; + if (typeof error === 'object' && error !== null) { + const message = (error as Record)['message']; + if (typeof message === 'string' && message.length > 0) return message; } return undefined; } From c9d1a8b7ed4fa5a064c38c225a0551098a7798a2 Mon Sep 17 00:00:00 2001 From: "jinye.djy" Date: Mon, 31 Aug 2026 03:13:28 +0800 Subject: [PATCH 2/2] codex: address PR review feedback (#10569) Co-authored-by: Qwen-Coder --- .../2026-08-30-turn-error-provider-detail.md | 62 ++++++++++--------- packages/acp-bridge/src/bridge.test.ts | 18 ++++++ 2 files changed, 51 insertions(+), 29 deletions(-) diff --git a/docs/design/2026-08-30-turn-error-provider-detail.md b/docs/design/2026-08-30-turn-error-provider-detail.md index dceea3886fa..4f5f5073bc7 100644 --- a/docs/design/2026-08-30-turn-error-provider-detail.md +++ b/docs/design/2026-08-30-turn-error-provider-detail.md @@ -47,17 +47,20 @@ Because the plain-text shape (`data.details`) already surfaces through the existing extractor, only the JSON-parsed shape loses its detail. The gap is a missing branch in one helper, not an absent wire field. -`extractErrorMessage` has exactly five call sites, all in `acp-bridge`, and -all are display or log surfaces with no string-matching behavioral consumer: - -| Call site | Surface | -| ------------------------------------------- | ---------------------------------------------------------------------------------- | -| `bridge.ts` `broadcastTurnError` | `turn_error` event message, `entry.turnError`, refresh replay, turn-status overlay | -| `bridge.ts` `sendPrompt` forward-failed log | daemon stderr | -| `bridge.ts` cancel-forward-failed log | daemon stderr | -| `bridge.ts` approval-mode restore log | daemon stderr | -| `bridge.ts` channel quarantine log | daemon stderr | -| `transcript-replay.ts` tool-result replay | restored transcript error text | +`extractErrorMessage` has eight call sites, all in `bridge.ts`: + +| Call site | Surface | +| ------------------------------------------- | --------------------------------------------------- | +| `broadcastTurnError` | `turn_error`, `entry.turnError`, and refresh replay | +| timed-out `newSession` cleanup quarantine | daemon stderr | +| two `model_switch_failed` publishers | SSE event message | +| approval-mode restore | daemon stderr | +| timed-out session-action cleanup quarantine | daemon stderr | +| `sendPrompt` forward failure | daemon stderr | +| pending-prompt cancel-forward failure | daemon stderr | + +`transcript-replay.ts` defines a separate local helper with the same name; +it is not a call site of the bridge helper and is untouched by this change. `classifyTurnErrorKind` exact-matches the message against `terminated`; that error arrives via the plain-text `data.details` shape and is unaffected by @@ -68,17 +71,16 @@ adding a nested-`error` branch. Extend `extractJsonRpcErrorDetail` in `packages/acp-bridge/src/bridge.ts` with one more fallback: after the existing `data` string / `data.details` / `data.message` checks, read a nested `data.error` — accepting either a plain -string or an object with a string `message` (mirroring the desktop client's -`getAcpErrorDetail`, which has shipped the same precedence for ACP internal -errors). Update the `extractErrorMessage` doc comment to name the +string or an object with a string `message` while preserving the existing +top-level precedence. Update the `extractErrorMessage` doc comment to name the `data.error.message` shape as the ACP SDK's JSON-parsed-message artifact. That is the entire production change. `broadcastTurnError` then publishes the -provider's own message as `turn_error.data.message`, and every downstream -surface — the Web Shell transcript error block, the live-state `turnError` -summary, refresh replay, `DaemonClient`'s `DaemonHttpError`, and the daemon -stderr logs — shows the real reason with no wire schema change and no -SDK/webui/desktop edits. +provider's own message as `turn_error.data.message`. The Web Shell transcript +error block, live-state `turnError` summary, refresh replay, and SDK consumers +of that `turn_error` (including `DaemonHttpError`) then receive the real reason +with no wire schema change and no SDK or webui edits. The separate +turn-status overlay normalization path is unchanged. Result for the motivating failure: the transcript shows `The engine is currently overloaded, please try again later` instead of @@ -91,13 +93,13 @@ Result for the motivating failure: the transcript shows `DaemonTurnErrorData`, the UI normalizer, transcript block types, and the webui adapter. That preserves the generic `message` for hypothetical string matchers but costs four packages of churn for the same user-visible - result. Every actual `message` consumer is enumerated above and is a - display/log surface, so enriching `message` directly is safe and minimal. -- **Precedence matches desktop.** Top-level `details`/`message` win over the - nested `error.message`, identical to `formatQwenAcpErrorMessage`'s - `getAcpErrorDetail` in - `packages/desktop/packages/shared/src/agent/qwen-agent.ts`. One consistent - de-facto contract for ACP `-32603` data across clients. + result. The eight bridge call sites above are display, log, or event + surfaces. The only behavioral check on the extracted message recognizes + `terminated`, which already arrives through `data.details`, so enriching + the nested shape is safe and minimal. +- **Preserve existing precedence.** A string `data`, top-level `details`, or + top-level `message` continues to win over the new nested `error.message` + fallback. - **No new length bound.** The existing `data.details` path is already unbounded, so a pathological provider blob flows today; this change keeps parity instead of inventing a second policy. Render-side control-character @@ -130,8 +132,10 @@ Result for the motivating failure: the transcript shows ## Open questions -- Should `normalizeTurnResultError` (turn-status polling overlay) also carry - the provider `type` (e.g. `engine_overloaded_error`) as a structured code? - Deferred — current overlay consumers only display `message`. +- Should the separate `normalizeTurnResultError` path used by turn-status + polling also extract provider detail or carry the provider `type` (e.g. + `engine_overloaded_error`) as a structured code? Deferred — this PR only + changes bridge extraction for `turn_error`; the terminal overlay may still + retain the generic `Internal error`. - If provider blobs prove unwieldy in practice, where should the length cap live — the bridge extractor or the event publisher? Deferred until observed. diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 14c677a9287..c96cf253d55 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -29635,6 +29635,24 @@ describe('extractErrorMessage', () => { ).toBe('Internal error'); }); + it('falls back to message when data.error is an empty string', () => { + expect( + extractErrorMessage( + new RequestError(-32603, 'Internal error', { error: '' }), + ), + ).toBe('Internal error'); + }); + + it('falls back to message when data.error.message is an empty string', () => { + expect( + extractErrorMessage( + new RequestError(-32603, 'Internal error', { + error: { message: '' }, + }), + ), + ).toBe('Internal error'); + }); + it('extracts details from Error subclasses with JSON-RPC data', () => { expect( extractErrorMessage(