From 45a92e54f368f61f30d4063fd0abac128922de97 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 8 Oct 2026 16:03:17 -0700 Subject: [PATCH] fix(acp): show the agent's own reason when a prompt fails Agents often answer a failed session/prompt with a generic JSON-RPC message ("Internal error") and keep the real reason in error.data. Only OMP and Grok had hooks that read it, so OpenCode and future ACP agents showed the bare message. The prompt error detail now reads, for every ACP agent: the agent's hook, else string data, else data.details, else the message. Other structured data is metadata and is ignored. OMP's details hook and Grok's string-data branch are now covered by that rule and removed. --- .../scripts/vitest-sqlite-runtime-files.mjs | 1 + src/main/acp/acp-dialects/grok-dialect.ts | 5 +- src/main/acp/acp-dialects/omp-dialect.ts | 1 - .../acp/acp-structured-prompt-failure.test.ts | 109 ++++++++++++++++++ src/main/acp/acp-turn-failures.ts | 16 ++- 5 files changed, 125 insertions(+), 7 deletions(-) create mode 100644 src/main/acp/acp-structured-prompt-failure.test.ts diff --git a/config/scripts/vitest-sqlite-runtime-files.mjs b/config/scripts/vitest-sqlite-runtime-files.mjs index ee5701d1159f..8275fe853d9e 100644 --- a/config/scripts/vitest-sqlite-runtime-files.mjs +++ b/config/scripts/vitest-sqlite-runtime-files.mjs @@ -5,6 +5,7 @@ export const SQLITE_RUNTIME_INCLUDE = [ ...SQLITE_DATABASE_RUNTIME_INCLUDE, 'src/main/acp/acp-structured-sign-in.test.ts', 'src/main/acp/acp-structured-auth.test.ts', + 'src/main/acp/acp-structured-prompt-failure.test.ts', 'src/main/acp/acp-timeline-turn-failures.test.ts', 'src/main/acp/acp-structured-session-adapter.test.ts', 'src/main/pi/rpc-turn-races.test.ts', diff --git a/src/main/acp/acp-dialects/grok-dialect.ts b/src/main/acp/acp-dialects/grok-dialect.ts index 81d07fa34bc5..609a9c313c5e 100644 --- a/src/main/acp/acp-dialects/grok-dialect.ts +++ b/src/main/acp/acp-dialects/grok-dialect.ts @@ -177,10 +177,7 @@ export const GROK_ACP_DIALECT: AcpDialect = { toolBackgroundTasks: grokToolBackgroundTasks, notification, contextWindow, - promptErrorDetail: (error) => - typeof error.data === 'string' - ? error.data - : promptErrorDataSchema.safeParse(error.data).data?.message, + promptErrorDetail: (error) => promptErrorDataSchema.safeParse(error.data).data?.message, failedTurnText: (stopReason) => stopReason === 'rate_limit' ? 'Grok usage limit reached.' diff --git a/src/main/acp/acp-dialects/omp-dialect.ts b/src/main/acp/acp-dialects/omp-dialect.ts index 0e38a7c2db56..d09c455a7292 100644 --- a/src/main/acp/acp-dialects/omp-dialect.ts +++ b/src/main/acp/acp-dialects/omp-dialect.ts @@ -96,7 +96,6 @@ function normalizeToolUpdate(update: ToolCallUpdate): ToolCallUpdate { export const OMP_ACP_DIALECT: AcpDialect = { normalizeToolUpdate, - promptErrorDetail: (error) => promptErrorDataSchema.safeParse(error.data).data?.details, authenticationRequired: (error) => { const details = promptErrorDataSchema.safeParse(error.data).data?.details return ( diff --git a/src/main/acp/acp-structured-prompt-failure.test.ts b/src/main/acp/acp-structured-prompt-failure.test.ts new file mode 100644 index 000000000000..44c452504803 --- /dev/null +++ b/src/main/acp/acp-structured-prompt-failure.test.ts @@ -0,0 +1,109 @@ +import { afterEach, describe, expect, it } from 'vitest' +import { z } from 'zod' +import { closeProviderTimelineRigs } from '../native-chat/agent-session-timeline/provider-timeline-assembler-test-support' +import { ACP_LAUNCH_SPECS, type AcpLaunchSpec } from './acp-launch-specs' +import { + openAcpAdapterRig, + PROVIDER_SESSION, + sendHello +} from './acp-structured-adapter.test-support' + +const promptMeta = z.object({ _meta: z.object({ promptId: z.string() }).optional() }) +afterEach(closeProviderTimelineRigs) + +function specFor(agent: string): AcpLaunchSpec { + const spec = ACP_LAUNCH_SPECS.find((entry) => entry.agent === agent) + if (!spec) { + throw new Error(`${agent} launch specification missing`) + } + return spec +} + +/** Fails the first prompt with `code`, after the turn started unless `beforeTurn`. */ +async function failFirstPrompt( + spec: AcpLaunchSpec, + error: { code: number; message: string; data?: unknown }, + beforeTurn = false +) { + const rig = await openAcpAdapterRig({ + spec, + script: (agent) => + agent.on('session/prompt', (frame) => { + if (!beforeTurn) { + agent.notify('session/update', { + sessionId: PROVIDER_SESSION, + _meta: promptMeta.parse(frame.params)._meta, + update: { + sessionUpdate: 'agent_message_chunk', + content: { type: 'text', text: 'started' } + } + }) + } + agent.fail(frame, error.code, error.message, error.data) + }) + }) + await rig.acquire() + await sendHello(rig, 'first') + await rig.settle() + const failures = (await rig.rig.rows()).flatMap(({ body }) => + body.kind === 'status' && body.tone === 'error' ? [body] : [] + ) + return { rig, failures } +} + +const WORDS = 'Model quota exceeded for this workspace; try again in 5 minutes.' +// OpenCode 1.18.31's recorded answer when its model provider fails: data is only metadata. +const OPENCODE_PROVIDER_ERROR = { + code: -32603, + message: 'Internal error: CAPTURE_PROVIDER_400', + data: { service: 'session', errorName: 'APIError' } +} + +describe.each(['opencode', 'grok'])('%s prompt failure detail', (agent) => { + const spec = specFor(agent) + + it.each([ + ['data.details', { code: -32603, message: 'Internal error', data: { details: WORDS } }, WORDS], + ['string data', { code: -32603, message: 'Internal error', data: WORDS }, WORDS], + ['no data', { code: -32603, message: 'Internal error' }, 'Internal error'], + ['metadata-only data', OPENCODE_PROVIDER_ERROR, 'Internal error: CAPTURE_PROVIDER_400'] + ])('shows the agent words from %s', async (_label, error, text) => { + const { failures } = await failFirstPrompt(spec, error) + expect(failures).toHaveLength(1) + expect(failures[0]?.text).toBe(text) + expect(failures[0]?.failure?.kind).not.toBe('notSignedIn') + }) + + it('quotes data.details after the sign-in sentence', async () => { + const { failures } = await failFirstPrompt(spec, { + code: -32000, + message: 'Authentication required', + data: { details: WORDS } + }) + expect(failures).toHaveLength(1) + expect(failures[0]?.failure).toMatchObject({ kind: 'notSignedIn' }) + expect(failures[0]?.text).toMatch(/^Sign in to .+\./) + expect(failures[0]?.text).toContain(WORDS) + expect(failures[0]?.text).not.toContain('Authentication required') + }) +}) + +describe('prompt refused before its turn began', () => { + const spec = specFor('grok') + + it.each([ + [{ code: -32603, message: 'Internal error', data: { details: WORDS } }, WORDS], + [OPENCODE_PROVIDER_ERROR, 'Internal error: CAPTURE_PROVIDER_400'] + ])('quotes the agent words: %j', async (error, text) => { + const { rig, failures } = await failFirstPrompt(spec, error, true) + expect(failures).toHaveLength(0) + const rejected = rig.settled.find((item) => 'state' in item && item.state === 'rejected') + expect(rejected).toMatchObject({ + clientMessageId: 'first', + rejection: expect.objectContaining({ + kind: 'providerRejected', + detail: { text, audience: 'person' } + }) + }) + }) +}) diff --git a/src/main/acp/acp-turn-failures.ts b/src/main/acp/acp-turn-failures.ts index f5f5f5dd463a..ba87ec6a5050 100644 --- a/src/main/acp/acp-turn-failures.ts +++ b/src/main/acp/acp-turn-failures.ts @@ -17,9 +17,21 @@ function acpStopReasonFailed(stopReason: string): boolean { return FAILED_STOP_REASONS.includes(stopReason) } -/** The provider's words in its error answer to `session/prompt`. */ +/** The provider's words in its error answer to `session/prompt`. Agents often answer a generic + * message ("Internal error") and keep their own words in `data`. */ export function acpPromptErrorDetail(dialect: AcpDialect, error: AcpAgentError): string { - return dialect.promptErrorDetail?.(error) ?? error.message + return dialect.promptErrorDetail?.(error) ?? acpErrorDataWords(error.data) ?? error.message +} + +// Other structured data is metadata (service, error class names), not words for a person. +function acpErrorDataWords(data: unknown): string | undefined { + const words = + typeof data === 'string' + ? data + : typeof data === 'object' && data !== null && 'details' in data + ? data.details + : undefined + return typeof words === 'string' && words.trim() ? words : undefined } export function acpAuthenticationRequired(dialect: AcpDialect, error: unknown): boolean {