From 428e850ca2e6b3c0d582b645199823ec0e6115b8 Mon Sep 17 00:00:00 2001 From: Jang Date: Fri, 21 Aug 2026 16:01:13 +0800 Subject: [PATCH] fix(acp-server): surface provider errors on session/prompt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `session/prompt` used to silently resolve with `{stopReason: 'end_turn'}` when the model provider returned a transport / status failure (rate limit, overload, 5xx, connection loss, etc.). The engine already classifies these as born-coded `Error2` instances with codes like `provider.api_error` / `provider.overloaded` / `provider.connection_error` / `context.overflow` and surfaces them on `turn.ended{ reason: 'failed' }`, but the ACP layer only knew how to map auth errors — every other failure was swallowed into `end_turn` and clients had no way to tell the model service was unavailable. Add `isProviderError()` in events-map.ts (sibling to `isAuthError()`) and a third branch in both `AcpSession.onTurnEnded` and `mapPromptLaunchError`: provider / context failures now reject with `RequestError.internalError({ code, message }, "model provider reported an error")`. The engine's full text rides in the JSON-RPC `data` payload so clients can log it; the wire message stays generic to keep provider-supplied PII out of every error toast. Auth failures keep their existing `auth_required` mapping; content-filter failures keep the legacy `refusal` mapping in `turnEndReasonToStopReason`. The scripted provider helper gains `mockNextProviderError(code, message)` so end-to-end tests can throw a coded provider error without going through the network layer. Resolve #3107 --- .changeset/surface-provider-errors.md | 5 ++ packages/acp-server/src/events-map.ts | 30 +++++++ packages/acp-server/src/session.ts | 28 +++++++ .../test/_helpers/scriptedProvider.ts | 29 ++++++- packages/acp-server/test/e2e-turn.test.ts | 79 +++++++++++++++++++ packages/acp-server/test/events-map.test.ts | 75 ++++++++++++++++++ 6 files changed, 244 insertions(+), 2 deletions(-) create mode 100644 .changeset/surface-provider-errors.md create mode 100644 packages/acp-server/test/events-map.test.ts diff --git a/.changeset/surface-provider-errors.md b/.changeset/surface-provider-errors.md new file mode 100644 index 0000000000..24dd15d924 --- /dev/null +++ b/.changeset/surface-provider-errors.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/kimi-code": patch +--- + +`session/prompt` now fails with a JSON-RPC error carrying the provider code and message when the model provider reports a failure, instead of silently resolving with an empty turn. \ No newline at end of file diff --git a/packages/acp-server/src/events-map.ts b/packages/acp-server/src/events-map.ts index cb549b7361..73627d672e 100644 --- a/packages/acp-server/src/events-map.ts +++ b/packages/acp-server/src/events-map.ts @@ -92,6 +92,36 @@ export function isAuthError(error?: { readonly code: string }): boolean { return error !== undefined && AUTH_ERROR_CODES.has(error.code); } +/** + * Error codes that indicate a model-side failure the client should see as a + * JSON-RPC `internal_error` rather than a silent `end_turn`. Drawn from + * `kosong/contract/errors.ts` (provider.status / context.overflow), + * `kosong/model/errors.ts` (provider.not_found) and + * `agent/loop/errors.ts` (loop.max_steps_exceeded). `provider.filtered` is + * deliberately omitted: content-filter failures keep the legacy refusal + * mapping in `turnEndReasonToStopReason` and surface as `refusal`, not an + * error. + */ +const PROVIDER_ERROR_CODES: ReadonlySet = new Set([ + 'provider.api_error', + 'provider.rate_limit', + 'provider.connection_error', + 'provider.overloaded', + 'provider.not_found', + 'context.overflow', + 'loop.max_steps_exceeded', +]); + +/** + * Whether the given error is a provider / context failure that should + * propagate as a JSON-RPC error instead of being swallowed into `end_turn`. + * Auth errors are deliberately excluded — those are handled by `isAuthError` + * and surface as `auth_required`. + */ +export function isProviderError(error?: { readonly code: string }): boolean { + return error !== undefined && PROVIDER_ERROR_CODES.has(error.code); +} + /** * Build the ACP `toolCallId` for a wire-level tool call. * diff --git a/packages/acp-server/src/session.ts b/packages/acp-server/src/session.ts index 66741fe42b..89efa6b126 100644 --- a/packages/acp-server/src/session.ts +++ b/packages/acp-server/src/session.ts @@ -79,6 +79,7 @@ import { turnEndReasonToStopReason, usageUpdateNotification, isAuthError, + isProviderError, stringifyArgs, } from './events-map'; import { AcpInteractionBridge } from './interaction-bridge'; @@ -111,6 +112,10 @@ const TURN_AGENT_BUSY_CODE = 'turn.agent_busy'; * re-auth flow (same mapping as the `turn.ended` auth path). * - `turn.agent_busy` maps to `invalidRequest` (-32600), matching the legacy * adapter's busy-prompt semantics. + * - Provider / context failures surface as `internalError` (-32603) with a + * generic wire message; the engine's full text and the code ride in the + * JSON-RPC `data` payload so the client can log them without forcing PII + * into every error toast. * - Everything else becomes a fixed-message `internalError` (-32603): the * raw engine message and stack are logged server-side but NEVER cross the * wire, so internal details cannot leak into the JSON-RPC channel. @@ -129,6 +134,17 @@ export function mapPromptLaunchError(error: unknown, sessionId: string): Request log.warn('acp: prompt rejected because another turn is active', { sessionId }); return RequestError.invalidRequest({ code }, message); } + if (typeof code === 'string' && isProviderError({ code })) { + log.warn('acp: prompt launch rejected with a provider error; surfacing to client', { + sessionId, + code, + error: message, + }); + return RequestError.internalError( + { code, message }, + 'model provider reported an error', + ); + } log.error('acp: prompt launch failed', { sessionId, error: error instanceof Error ? { message: error.message, stack: error.stack } : String(error), @@ -915,6 +931,18 @@ export class AcpSession { driver.reject(RequestError.authRequired(undefined, error?.message)); return; } + // Provider / context failures propagate as JSON-RPC `internalError`; + // the engine's full text rides in `data` so clients can log it + // without forcing PII into every error toast. + if (event.reason === 'failed' && isProviderError(error)) { + driver.reject( + RequestError.internalError( + { code: error?.code, message: error?.message }, + 'model provider reported an error', + ), + ); + return; + } driver.resolve({ stopReason: turnEndReasonToStopReason(event.reason, error) }); }); void this.emitUsageUpdate(); diff --git a/packages/acp-server/test/_helpers/scriptedProvider.ts b/packages/acp-server/test/_helpers/scriptedProvider.ts index cb25e06103..246a2b93ce 100644 --- a/packages/acp-server/test/_helpers/scriptedProvider.ts +++ b/packages/acp-server/test/_helpers/scriptedProvider.ts @@ -19,6 +19,8 @@ */ import { + type ErrorCode, + Error2, type FinishReason, IProtocolAdapterRegistry, type IProtocolAdapterRegistry as IProtocolAdapterRegistryType, @@ -36,6 +38,15 @@ interface ScriptedResponse { readonly rawFinishReason?: string | null; } +/** + * A scripted `generate()` rejection. `throwError` lets the test pick the + * concrete class (`Error2` to carry a typed code, plain `Error` to simulate + * an un-coded engine failure). + */ +interface ScriptedError { + readonly throwError: Error; +} + const ZERO_USAGE: TokenUsage = { inputOther: 0, output: 0, @@ -81,7 +92,7 @@ class ScriptedChatProvider { readonly thinkingEffort = null; constructor( - private readonly queue: ScriptedResponse[], + private readonly queue: Array, private readonly calls: Array, ) {} @@ -99,6 +110,10 @@ class ScriptedChatProvider { `queue exhausted. Push another response via mockNextResponse().`, ); } + if ('throwError' in response) { + this.calls.push(history); + throw response.throwError; + } this.calls.push(history); return new ScriptedStream(response.parts, response, this.calls.length); } @@ -125,6 +140,13 @@ export interface ScriptedProvider { readonly finishReason?: FinishReason | null; readonly rawFinishReason?: string | null; }): void; + /** + * Push a coded provider error for the next `generate()` call. The thrown + * error is an `Error2` so the engine translates it into a `turn.ended` + * event carrying `error.code` verbatim, letting the ACP layer branch on + * `isProviderError` / `isAuthError` without extra wiring. + */ + mockNextProviderError(code: ErrorCode, message: string): void; /** Number of `generate()` calls the engine has made so far. */ callCount(): number; /** The `history` argument of every `generate()` call so far, in order. */ @@ -132,7 +154,7 @@ export interface ScriptedProvider { } export function createScriptedProvider(): ScriptedProvider { - const queue: ScriptedResponse[] = []; + const queue: Array = []; const calls: Array = []; // Single shared provider so every ModelImpl in the process (main agent, // sub-agents) draws from the same FIFO queue. @@ -170,6 +192,9 @@ export function createScriptedProvider(): ScriptedProvider { rawFinishReason: response.rawFinishReason, }); }, + mockNextProviderError: (code, message) => { + queue.push({ throwError: new Error2(code, message) }); + }, callCount: () => calls.length, callHistory: () => calls, }; diff --git a/packages/acp-server/test/e2e-turn.test.ts b/packages/acp-server/test/e2e-turn.test.ts index 5aea540424..e6bc693704 100644 --- a/packages/acp-server/test/e2e-turn.test.ts +++ b/packages/acp-server/test/e2e-turn.test.ts @@ -567,15 +567,55 @@ describe('mapPromptLaunchError', () => { }); }); + it('surfaces a provider-coded rejection as internalError with the engine text in data', () => { + const error = Object.assign(new Error('custom gateway returned 500'), { + code: 'provider.api_error', + }); + const mapped = mapPromptLaunchError(error, 'sess-x'); + expect(mapped.code).toBe(-32603); + // The wire message stays generic so providers cannot echo PII into every + // error toast; the engine's full text and the provider code ride on the + // JSON-RPC `data` payload for clients that want to log them. + expect(mapped.message).toBe('Internal error: model provider reported an error'); + expect(JSON.stringify(mapped)).toContain('custom gateway returned 500'); + expect(JSON.stringify(mapped)).toContain('provider.api_error'); + }); + + it('surfaces a context.overflow rejection as internalError with its engine text in data', () => { + const error = Object.assign(new Error('context_length_exceeded'), { + code: 'context.overflow', + }); + const mapped = mapPromptLaunchError(error, 'sess-x'); + expect(mapped.code).toBe(-32603); + expect(mapped.message).toBe('Internal error: model provider reported an error'); + expect(JSON.stringify(mapped)).toContain('context_length_exceeded'); + }); + + it('routes a provider.filtered launch rejection to the fixed generic path', () => { + // `provider.filtered` is deliberately excluded from `PROVIDER_ERROR_CODES` + // — content-filter failures keep the legacy refusal mapping on the turn + // path and fall through to the fixed generic message at launch, where + // there is no turn yet to attach a refusal to. + const error = Object.assign(new Error('request was blocked by content safety'), { + code: 'provider.filtered', + }); + const mapped = mapPromptLaunchError(error, 'sess-x'); + expect(mapped.code).toBe(-32603); + expect(mapped.message).toBe('Internal error: session prompt failed'); + expect(JSON.stringify(mapped)).not.toContain('request was blocked'); + }); + describe('acp-server prompt error hygiene', () => { let homeDir: string | undefined; let client: TestClient | undefined; + let scripted: ScriptedProvider | undefined; afterEach(async () => { if (client !== undefined) { await client.close(); client = undefined; } + scripted = undefined; if (homeDir !== undefined) { await rm(homeDir, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); homeDir = undefined; @@ -611,6 +651,45 @@ describe('acp-server prompt error hygiene', () => { expect(serialized).not.toContain('session not found'); expect(serialized).not.toContain(created.sessionId); }, 30_000); + + it('a mid-turn provider.api_error fails the prompt with internalError; engine text rides in data', async () => { + // Real engine + ACP wire + scripted LLM that throws a coded provider + // error on the first `generate()` call. Before this change the prompt + // silently resolved with `end_turn`; now it must reject with a JSON-RPC + // `internalError` whose data payload carries the engine's failure text + // and the provider code, while the wire message stays generic so PII + // cannot leak through every error toast. + homeDir = await mkdtemp(join(tmpdir(), 'acp-provider-error-')); + await writeFakeModelConfig(homeDir); + scripted = createScriptedProvider(); + client = await createTestClient({ homeDir, extraSeeds: [scripted.seed] }); + const c = client; + await c.send('initialize', { protocolVersion: 1, clientCapabilities: {} }); + scripted.mockNextProviderError('provider.api_error', 'custom gateway returned 500'); + + const created = (await c.send('session/new', { cwd: homeDir, mcpServers: [] })) as { + sessionId: string; + }; + await c.waitForSessionUpdate('available_commands_update', 10_000); + + let captured: unknown; + try { + await c.send('session/prompt', { + sessionId: created.sessionId, + prompt: [{ type: 'text', text: 'run anything' }], + }); + } catch (error) { + captured = error; + } + const serialized = JSON.stringify((captured as Error)?.message ?? String(captured)); + expect(serialized).toContain('-32603'); + // Wire message stays generic. + expect(serialized).toContain('Internal error: model provider reported an error'); + // Engine text and provider code ride on the JSON-RPC `data` payload. + expect(serialized).toContain('custom gateway returned 500'); + expect(serialized).toContain('provider.api_error'); + expect(scripted?.callCount() ?? 0).toBe(1); + }, 30_000); }); describe('acp-server builtin slash commands (local execution, no LLM turn)', () => { diff --git a/packages/acp-server/test/events-map.test.ts b/packages/acp-server/test/events-map.test.ts new file mode 100644 index 0000000000..fb8afbb7bb --- /dev/null +++ b/packages/acp-server/test/events-map.test.ts @@ -0,0 +1,75 @@ +import { describe, expect, it } from 'vitest'; + +import { isAuthError, isProviderError, turnEndReasonToStopReason } from '../src/events-map'; + +describe('isAuthError', () => { + it('matches every code in the auth set', () => { + expect(isAuthError({ code: 'provider.auth_error' })).toBe(true); + expect(isAuthError({ code: 'auth.login_required' })).toBe(true); + expect(isAuthError({ code: 'auth.token_missing' })).toBe(true); + expect(isAuthError({ code: 'auth.token_unauthorized' })).toBe(true); + expect(isAuthError({ code: 'auth.provisioning_required' })).toBe(true); + expect(isAuthError({ code: 'auth.model_not_resolved' })).toBe(true); + }); + + it('does not match provider.* codes that are not auth', () => { + expect(isAuthError({ code: 'provider.api_error' })).toBe(false); + expect(isAuthError({ code: 'provider.rate_limit' })).toBe(false); + expect(isAuthError({ code: 'provider.overloaded' })).toBe(false); + }); + + it('does not match when the error is missing or has no code', () => { + expect(isAuthError()).toBe(false); + expect(isAuthError(undefined)).toBe(false); + }); +}); + +describe('isProviderError', () => { + it('matches every code in the provider set', () => { + expect(isProviderError({ code: 'provider.api_error' })).toBe(true); + expect(isProviderError({ code: 'provider.rate_limit' })).toBe(true); + expect(isProviderError({ code: 'provider.connection_error' })).toBe(true); + expect(isProviderError({ code: 'provider.overloaded' })).toBe(true); + expect(isProviderError({ code: 'provider.not_found' })).toBe(true); + expect(isProviderError({ code: 'context.overflow' })).toBe(true); + expect(isProviderError({ code: 'loop.max_steps_exceeded' })).toBe(true); + }); + + it('does not match auth codes (those are routed through isAuthError)', () => { + expect(isProviderError({ code: 'provider.auth_error' })).toBe(false); + expect(isProviderError({ code: 'auth.login_required' })).toBe(false); + }); + + it('does not match provider.filtered — content-filter failures keep the legacy refusal mapping', () => { + expect(isProviderError({ code: 'provider.filtered' })).toBe(false); + }); + + it('does not match unrelated engine codes', () => { + expect(isProviderError({ code: 'session.not_found' })).toBe(false); + expect(isProviderError({ code: 'agent.not_found' })).toBe(false); + expect(isProviderError({ code: 'turn.agent_busy' })).toBe(false); + }); + + it('does not match when the error is missing or has no code', () => { + expect(isProviderError()).toBe(false); + expect(isProviderError(undefined)).toBe(false); + }); +}); + +describe('turnEndReasonToStopReason', () => { + it('maps provider.filtered failures to refusal', () => { + expect(turnEndReasonToStopReason('failed', { code: 'provider.filtered' })).toBe('refusal'); + }); + + it('keeps other failures as end_turn (legacy: acp has no failed stop reason)', () => { + expect(turnEndReasonToStopReason('failed', { code: 'provider.api_error' })).toBe('end_turn'); + expect(turnEndReasonToStopReason('failed', { code: 'provider.overloaded' })).toBe('end_turn'); + expect(turnEndReasonToStopReason('failed', undefined)).toBe('end_turn'); + }); + + it('keeps the other reasons unchanged', () => { + expect(turnEndReasonToStopReason('completed')).toBe('end_turn'); + expect(turnEndReasonToStopReason('cancelled')).toBe('cancelled'); + expect(turnEndReasonToStopReason('blocked')).toBe('refusal'); + }); +}); \ No newline at end of file