diff --git a/.changeset/surface-acp-turn-failures.md b/.changeset/surface-acp-turn-failures.md new file mode 100644 index 0000000000..af3f3dbea3 --- /dev/null +++ b/.changeset/surface-acp-turn-failures.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/kimi-code": patch +--- + +Report failed ACP turns through auth, refusal, or sanitized JSON-RPC error outcomes instead of successful empty completions. diff --git a/docs/en/reference/kimi-acp.md b/docs/en/reference/kimi-acp.md index 30f0787651..38e7ed18f1 100644 --- a/docs/en/reference/kimi-acp.md +++ b/docs/en/reference/kimi-acp.md @@ -49,6 +49,8 @@ The spec divides methods into a **stable** surface and an evolving **unstable** | `session/close` | No | | | `logout` | No | | +`session/prompt` does not report a failed turn as `end_turn`. Authentication failures return `authRequired (-32000)` without error data, provider filtering and prompt-hook blocks return `refusal`, and other failures return `internalError (-32603)`. For public Kimi error codes returned as `internalError`, `error.data` contains only `code`, the canonical `retryable` value, and an optional valid HTTP `statusCode`; private codes, raw provider messages, and other details are not sent over ACP. + ### Stable client-side reverse-RPC — agent → IDE (4 / 9) | Method | Implemented | Description | diff --git a/docs/zh/reference/kimi-acp.md b/docs/zh/reference/kimi-acp.md index d58c4460ab..d1108ad10c 100644 --- a/docs/zh/reference/kimi-acp.md +++ b/docs/zh/reference/kimi-acp.md @@ -49,6 +49,8 @@ kimi acp | `session/close` | 否 | | | `logout` | 否 | | +`session/prompt` 不会把失败的轮次报告为 `end_turn`。认证失败返回不含错误数据的 `authRequired (-32000)`，供应商过滤和提示词 Hook 拦截返回 `refusal`，其余失败返回 `internalError (-32603)`。对于以 `internalError` 返回的公开 Kimi 错误码，`error.data` 仅包含 `code`、规范的 `retryable` 值和可选的合法 HTTP `statusCode`；私有错误码、供应商原始消息与其他详细信息不会通过 ACP 发送。 + ### 稳定面 client-side reverse-RPC — agent → IDE（4 / 9） | 方法 | 状态 | 说明 | diff --git a/packages/acp-adapter/src/events-map.ts b/packages/acp-adapter/src/events-map.ts index 0448f2eb9c..091be558f0 100644 --- a/packages/acp-adapter/src/events-map.ts +++ b/packages/acp-adapter/src/events-map.ts @@ -49,33 +49,22 @@ export function assistantDeltaToSessionUpdate( * * `completed` → `end_turn`: the model finished a clean turn. * `cancelled` → `cancelled`: the client/agent cancelled mid-turn. - * `failed` → `end_turn` *with* an out-of-band log: the SDK reports a - * step-level error via `TurnEndedEvent.error`. ACP's `StopReason` does - * not have a dedicated `failed` variant in this protocol version, and - * the spec discourages signaling errors through `stopReason` (errors - * belong on the JSON-RPC error channel). Returning `end_turn` keeps the - * client unblocked; the caller is expected to log the `error` payload - * separately so the failure is observable in the agent logs. - * `failed` + `provider.filtered` → `refusal`: the provider's safety policy - * blocked the response. ACP's `refusal` stop reason is the native signal - * for a model/provider decline, so the client can render the block instead - * of mistaking it for a clean `end_turn`. * `blocked` → `refusal`: a prompt hook blocked the turn before the model * ran. ACP has no separate hook-blocked terminal state, so reuse the * refusal channel instead of reporting a clean `end_turn`. + * + * Failed turns are deliberately excluded from this function's input. The + * caller must settle those through the JSON-RPC error channel, except for + * provider filtering, which has the native ACP `refusal` stop reason. */ export function turnEndReasonToStopReason( - reason: TurnEndReason, - error?: { readonly code: string }, + reason: Exclude, ): AcpStopReason { switch (reason) { case 'completed': return 'end_turn'; case 'cancelled': return 'cancelled'; - case 'failed': - if (error?.code === 'provider.filtered') return 'refusal'; - return 'end_turn'; case 'blocked': return 'refusal'; } diff --git a/packages/acp-adapter/src/prompt-failure.ts b/packages/acp-adapter/src/prompt-failure.ts new file mode 100644 index 0000000000..966438fc47 --- /dev/null +++ b/packages/acp-adapter/src/prompt-failure.ts @@ -0,0 +1,55 @@ +import { RequestError, type PromptResponse } from '@agentclientprotocol/sdk'; +import { ErrorCodes, KIMI_ERROR_INFO } from '@moonshot-ai/kimi-code-sdk'; + +export type PromptFailureOutcome = + | { readonly kind: 'authRequired'; readonly error: RequestError } + | { readonly kind: 'refusal'; readonly response: PromptResponse } + | { readonly kind: 'internalError'; readonly error: RequestError }; + +export function mapPromptFailure(error: unknown): PromptFailureOutcome { + const code = kimiErrorCodeFromUnknown(error); + if (code === ErrorCodes.AUTH_LOGIN_REQUIRED || code === ErrorCodes.PROVIDER_AUTH_ERROR) { + return { kind: 'authRequired', error: RequestError.authRequired() }; + } + if (code === ErrorCodes.PROVIDER_FILTERED) { + return { kind: 'refusal', response: { stopReason: 'refusal' } }; + } + + const info = code === undefined ? undefined : KIMI_ERROR_INFO[code]; + const data = + info?.public === true + ? { + code, + retryable: info.retryable, + statusCode: validHttpStatusCode(detailsFromUnknown(error)?.['statusCode']), + } + : undefined; + return { + kind: 'internalError', + error: RequestError.internalError(data, 'session prompt failed'), + }; +} + +function kimiErrorCodeFromUnknown( + error: unknown, +): keyof typeof KIMI_ERROR_INFO | undefined { + if (error === null || typeof error !== 'object' || !('code' in error)) return undefined; + const code = error.code; + return typeof code === 'string' && Object.hasOwn(KIMI_ERROR_INFO, code) + ? (code as keyof typeof KIMI_ERROR_INFO) + : undefined; +} + +function detailsFromUnknown(error: unknown): Record | undefined { + if (error === null || typeof error !== 'object' || !('details' in error)) return undefined; + const details = error.details; + return details !== null && typeof details === 'object' && !Array.isArray(details) + ? (details as Record) + : undefined; +} + +function validHttpStatusCode(value: unknown): number | undefined { + return typeof value === 'number' && Number.isInteger(value) && value >= 100 && value <= 599 + ? value + : undefined; +} diff --git a/packages/acp-adapter/src/session.ts b/packages/acp-adapter/src/session.ts index 747b44ea9c..52a98310fa 100644 --- a/packages/acp-adapter/src/session.ts +++ b/packages/acp-adapter/src/session.ts @@ -56,6 +56,7 @@ import { turnEndReasonToStopReason, } from './events-map'; import { acpModeToToggles, DEFAULT_MODE_ID, isAcpModeId, type AcpModeId } from './modes'; +import { mapPromptFailure } from './prompt-failure'; import { outcomeToQuestionAnswer, questionItemToPermissionOptions } from './question'; import { detectSlashIntent } from './slash'; @@ -89,9 +90,9 @@ export class AcpSession { * * Updated inside the existing `onEvent` listener in {@link prompt} * (any event carrying a numeric `turnId` advances the value), and - * reset to `undefined` on `turn.ended`. Approval flows are gated by - * the SDK on the active turn so a stale value is effectively - * unreachable in practice; the `undefined` fallback in + * cleared when the owning prompt settles if it still points to that + * prompt. Approval flows are gated by the SDK on the active turn so a + * stale value is effectively unreachable in practice; the fallback in * `buildPermissionToolCallUpdate` exists for defence-in-depth. */ private currentTurnId: number | undefined = undefined; @@ -770,17 +771,18 @@ export class AcpSession { /** * Run an ACP `session/prompt` against the underlying SDK session. * - * Error mapping (Phase 11.1): + * Error mapping: * - Auth-coded errors (`AUTH_LOGIN_REQUIRED`, `PROVIDER_AUTH_ERROR`) * surface as `RequestError.authRequired()` so the ACP client can * drive its own re-auth UX rather than a generic internal error. - * - Everything else becomes `RequestError.internalError(...)` with - * the stack/message logged to the agent log file but NOT exposed - * to the client (the JSON-RPC layer would otherwise leak details). - * - Auth-coded failures may arrive on TWO paths: a `turn.ended` - * event with `reason: 'failed'` and an `event.error` payload, OR - * a synchronous `session.prompt(...)` rejection. Both are - * routed through {@link mapPromptError} for parity. + * - Provider filtering and prompt-hook blocks use ACP's native + * `refusal` stop reason. + * - Other failed turns become `RequestError.internalError(...)`. + * Public Kimi failures expose only code, canonical retryability, and + * a valid HTTP status; private codes and raw diagnostics stay local. + * - Failures may arrive on TWO paths: a `turn.ended` event with an + * error payload, or a `session.prompt(...)` rejection. Both use the + * same safe JSON-RPC data shape. * * Subscribes to the session event stream; for every `assistant.delta`, * pushes an `agent_message_chunk` `session/update` notification to the @@ -1019,29 +1021,63 @@ export class AcpSession { // each turn produces a distinct wire-level tool call that needs // its own CREATE. const startedToolCalls = new Set(); + // Lock this request to the first main-agent turn.started for a new turn; + // a previously active request's terminal event must not settle it. const initialActiveTurnId = this.currentTurnId; - let hasReceivedOwnTurnStarted = false; - const unsub = this.session.onEvent((event) => { + let ownTurnId: number | undefined; + let unsubscribe: (() => void) | undefined; + const cleanup = (): void => { + argsByToolCall.clear(); + startedToolCalls.clear(); + if (ownTurnId !== undefined && this.currentTurnId === ownTurnId) { + this.currentTurnId = undefined; + } + const stopListening = unsubscribe; + unsubscribe = undefined; + stopListening?.(); + }; + const settle = (action: () => void): void => { + if (settled) return; + settled = true; + cleanup(); + action(); + }; + const settleFailure = (failure: ReturnType): void => { + if (failure.kind === 'refusal') { + settle(() => { + resolve(failure.response); + }); + } else { + settle(() => { + reject(failure.error); + }); + } + }; + + unsubscribe = this.session.onEvent((event) => { if ( + ownTurnId === undefined && event.type === 'turn.started' && isFromMainAgent(event) && (initialActiveTurnId === undefined || event.turnId !== initialActiveTurnId) ) { - hasReceivedOwnTurnStarted = true; + ownTurnId = event.turnId; + } + if (event.type === 'turn.ended' && isFromMainAgent(event)) { + if (ownTurnId === undefined) { + if (initialActiveTurnId !== undefined) return; + // Compatibility fallback for Session implementations that omit + // turn.started. A terminal event can only adopt ownership when no + // turn was active before this prompt subscribed. + ownTurnId = event.turnId; + } else if (event.turnId !== ownTurnId) { + return; + } } - // Track the active turn so `handleApproval` (registered once at - // construction, called via `setApprovalHandler`) can compose the - // prefixed `${turnId}:${toolCallId}` wire id that matches the - // tool card the client already rendered. This branch is purely - // additive: it runs before the existing dispatch and never - // returns, so the if-chain below behaves exactly as in Phase 4. - // Subagent turn events carry their own `turnId`; filtering on - // `agentId` keeps `currentTurnId` aligned with the parent turn - // that the approval prompt actually belongs to. if ( + isFromMainAgent(event) && 'turnId' in event && - typeof event.turnId === 'number' && - isFromMainAgent(event) + typeof event.turnId === 'number' ) { this.currentTurnId = event.turnId; } @@ -1049,22 +1085,19 @@ export class AcpSession { if (settled) return; if (!isFromMainAgent(event)) return; if (event.code !== ErrorCodes.TURN_AGENT_BUSY) return; - if (hasReceivedOwnTurnStarted) return; - settled = true; - argsByToolCall.clear(); - startedToolCalls.clear(); - this.currentTurnId = undefined; - unsub(); + if (ownTurnId !== undefined) return; log.warn('acp: prompt rejected because another turn is active', { sessionId, details: event.details, }); - reject( - RequestError.invalidRequest( - { code: event.code, details: event.details }, - event.message, - ), - ); + settle(() => { + reject( + RequestError.invalidRequest( + { code: event.code, details: event.details }, + event.message, + ), + ); + }); return; } if (event.type === 'assistant.delta') { @@ -1228,56 +1261,51 @@ export class AcpSession { if (event.type === 'turn.ended') { if (settled) return; if (!isFromMainAgent(event)) return; - settled = true; if (event.reason === 'failed') { - // Failures bubble up via the SDK `error` payload. Phase 11.1 - // upgrades the prior "log + resolve end_turn" behaviour to - // route auth-coded failures through `RequestError.authRequired()` - // so the client can trigger its re-auth UX. Other failure - // codes still resolve with `end_turn` (the spec discourages - // signaling errors through `stopReason`; the failure is - // observable in the log). log.warn('acp: turn ended with failed reason', { sessionId, error: event.error, }); - argsByToolCall.clear(); - startedToolCalls.clear(); - this.currentTurnId = undefined; - unsub(); - const authErr = authRequiredFromPayload(event.error); - if (authErr) { - reject(authErr); - return; - } - } else { - if (event.reason === 'blocked') { - // Provider safety and prompt hooks both map to ACP `refusal` - // (see turnEndReasonToStopReason); log them here too so the - // block stays observable in the agent logs, mirroring the - // `failed` branch above. - log.warn('acp: turn ended with blocked reason', { - reason: event.reason, - sessionId, - }); - } - argsByToolCall.clear(); - startedToolCalls.clear(); - // Drop the turnId so a late-arriving approval (e.g. an SDK - // reverse-RPC racing the turn boundary) falls back to the raw - // SDK id rather than re-prefixing with a stale value. - this.currentTurnId = undefined; - unsub(); + } else if (event.reason === 'blocked') { + log.warn('acp: turn ended with blocked reason', { + reason: event.reason, + sessionId, + }); + } + if (event.reason === 'failed') { + settleFailure(mapPromptFailure(event.error)); + return; } - resolve({ stopReason: turnEndReasonToStopReason(event.reason, event.error) }); + const stopReason = turnEndReasonToStopReason(event.reason); + settle(() => { + resolve({ stopReason }); + }); } }); + if (settled) cleanup(); - kick().catch((err) => { + void kick().catch((error) => { if (settled) return; - settled = true; - unsub(); - reject(mapPromptError(err, sessionId)); + const failure = mapPromptFailure(error); + if (failure.kind === 'authRequired') { + log.warn('acp: prompt rejected with auth error; mapping to authRequired', { + sessionId, + error: error instanceof Error ? error.message : String(error), + }); + } else if (failure.kind === 'refusal') { + log.warn('acp: prompt rejected by provider filtering; mapping to refusal', { + sessionId, + }); + } else { + log.error('acp: prompt failed', { + sessionId, + error: + error instanceof Error + ? { message: error.message, stack: error.stack } + : String(error), + }); + } + settleFailure(failure); }); }); } @@ -1450,21 +1478,6 @@ export class AcpSession { } } -/** - * Map a Kimi SDK error (raw `Error`, `KimiError`, or `KimiErrorPayload`) - * into the ACP {@link RequestError} shape used by the JSON-RPC layer. - * - * Auth-coded inputs (`auth.login_required`, `provider.auth_error`) - * become `RequestError.authRequired()` so the client can drive its own - * re-auth UX. Everything else becomes `RequestError.internalError(...)` - * with the raw error logged to the agent log file but NOT exposed in - * the JSON-RPC response — the client only sees the canonical - * "session prompt failed" message, preventing accidental leakage of - * stack frames or PII through the wire. - * - * The kimi-cli Python reference performs the same mapping at - * `kimi-cli/src/kimi_cli/acp/session.py:218-247`; this is the TS port. - */ type CompactionCompletedResult = Extract['result']; type CompactionOutcome = @@ -1592,71 +1605,6 @@ function detectLeadingSlashIntent( return detectSlashIntent(first.text, skillCommandMap); } -function mapPromptError(err: unknown, sessionId: string): RequestError { - const authErr = authRequiredFromUnknown(err); - if (authErr) { - log.warn('acp: prompt rejected with auth error; mapping to authRequired', { - sessionId, - error: err instanceof Error ? err.message : String(err), - }); - return authErr; - } - log.error('acp: prompt failed', { - sessionId, - error: err instanceof Error ? { message: err.message, stack: err.stack } : String(err), - }); - return RequestError.internalError(undefined, 'session prompt failed'); -} - -/** - * Inspect a {@link KimiErrorPayload} (as carried on `turn.ended` - * failed events) and return a `RequestError.authRequired()` if its - * `code` is one of the auth-required codes; otherwise `undefined`. - * - * Kept separate from {@link authRequiredFromUnknown} because the - * `turn.ended` event hands us a serialized payload (no class identity - * to branch on) — we only need the `code` discriminator here. - */ -function authRequiredFromPayload( - payload: { readonly code: unknown } | undefined, -): RequestError | undefined { - if (!payload) return undefined; - if (isAuthErrorCode(payload.code)) { - return RequestError.authRequired(); - } - return undefined; -} - -/** - * Type-narrowing predicate for the codes the adapter treats as - * "the client must re-authenticate before retrying". Currently: - * - `auth.login_required` — Kimi Platform / OAuth login flow needed. - * - `provider.auth_error` — the downstream provider rejected the - * request with a 401 (the node SDK lifts these into `KimiError` - * at `kimi-code-model-provider.ts:99-103`). - */ -function isAuthErrorCode(code: unknown): boolean { - return code === ErrorCodes.AUTH_LOGIN_REQUIRED || code === ErrorCodes.PROVIDER_AUTH_ERROR; -} - -/** - * Best-effort detection of "auth required" for the `session.prompt(...)` - * rejection path. The thrown value MAY be: - * - A `KimiError` instance with a recognized `code` field. - * - A plain object that happens to expose a `code` (covers RPC-layer - * deserialized payloads that lost class identity). - * - Anything else — returns `undefined`. - */ -function authRequiredFromUnknown(err: unknown): RequestError | undefined { - if (err && typeof err === 'object' && 'code' in err) { - const code = (err as { code?: unknown }).code; - if (isAuthErrorCode(code)) { - return RequestError.authRequired(); - } - } - return undefined; -} - /** * Identifier the agent-core session emits for the main (user-facing) * agent. Subagents are issued generated ids by `Session.spawnAgent`; diff --git a/packages/acp-adapter/test/error-mapping.test.ts b/packages/acp-adapter/test/error-mapping.test.ts index f05bfef128..18660151b4 100644 --- a/packages/acp-adapter/test/error-mapping.test.ts +++ b/packages/acp-adapter/test/error-mapping.test.ts @@ -1,3 +1,9 @@ +/** + * Scenario: ACP session/prompt terminal failure mapping. + * Responsibilities: map terminal outcomes and expose only safe JSON-RPC error data. + * Wiring: real ACP connections over in-memory NDJSON; scripted SDK Session boundary. + * Run: pnpm exec vitest run packages/acp-adapter/test/error-mapping.test.ts + */ import { describe, expect, it } from 'vitest'; import { @@ -23,7 +29,6 @@ import { type Session, } from '@moonshot-ai/kimi-code-sdk'; -import { turnEndReasonToStopReason } from '../src/events-map'; import { AcpServer } from '../src/server'; import { AUTHED_STATUS } from './_helpers/harness-stubs'; @@ -59,11 +64,8 @@ interface ScriptedSession { } /** - * Build a fake `Session` whose `prompt()` either rejects with a - * caller-supplied error OR fans out a pre-recorded event sequence - * through any subscribed listener — covering the two distinct error - * paths that {@link AcpSession.prompt} routes through - * `mapPromptError` / `authRequiredFromPayload`. + * Build the SDK boundary used by the wire-level tests. Its prompt either + * rejects or emits a caller-supplied event sequence to active subscribers. */ function makeScriptedSession( sessionId: string, @@ -102,78 +104,149 @@ function makeHarnessWithSession(session: Session): KimiHarness { } as unknown as KimiHarness; } -describe('AcpServer error mapping', () => { - it('maps a turn.ended failed event with auth.login_required to authRequired (-32000)', async () => { - const sessionId = 'sess-auth-payload'; - const errorPayload: KimiErrorPayload = { - code: ErrorCodes.AUTH_LOGIN_REQUIRED, - message: 'Login required', +function connectToSession( + session: Session, +): readonly [AgentSideConnection, ClientSideConnection] { + const { agentStream, clientStream } = makeInMemoryStreamPair(); + const agent = new AgentSideConnection( + (connection) => new AcpServer(makeHarnessWithSession(session), connection), + agentStream, + ); + const client = new ClientSideConnection(() => new StubClient(), clientStream); + return [agent, client]; +} + +function makeFailedSession( + sessionId: string, + error?: KimiErrorPayload, +): ScriptedSession { + return makeScriptedSession(sessionId, { + script: [ + { + type: 'turn.ended', + sessionId, + agentId: 'main', + turnId: 1, + reason: 'failed', + error, + } as Event, + ], + }); +} + +async function capturePromptError( + client: ClientSideConnection, + sessionId: string, + text = 'hi', +): Promise { + try { + await client.prompt({ sessionId, prompt: [textBlock(text)] }); + } catch (error) { + return error; + } + throw new Error('Expected session/prompt to reject'); +} + +function exposedError(error: unknown): string | undefined { + return error instanceof Error + ? JSON.stringify({ + message: error.message, + data: (error as Error & { data?: unknown }).data, + }) + : JSON.stringify(error); +} + +describe('ACP prompt failure mapping (terminal outcomes and safe error data)', () => { + it.each([ + [ErrorCodes.AUTH_LOGIN_REQUIRED, 'Login required'], + [ErrorCodes.PROVIDER_AUTH_ERROR, 'Provider returned 401'], + ] as const)('rejects with authRequired when a failed event carries %s', async (code, message) => { + const sessionId = `sess-${code}`; + const { session } = makeFailedSession(sessionId, { + code, + message, retryable: false, - }; - const { session } = makeScriptedSession(sessionId, { - script: [ - { - type: 'turn.ended', - sessionId, - agentId: 'main', - turnId: 1, - reason: 'failed', - error: errorPayload, - } as Event, - ], }); - const { agentStream, clientStream } = makeInMemoryStreamPair(); - new AgentSideConnection((c) => new AcpServer(makeHarnessWithSession(session), c), agentStream); - const client = new ClientSideConnection(() => new StubClient(), clientStream); + const [, client] = connectToSession(session); await client.newSession({ cwd: '/tmp/x', mcpServers: [] }); await expect( client.prompt({ sessionId, prompt: [textBlock('hi')] }), - ).rejects.toMatchObject({ code: -32000 }); + ).rejects.toMatchObject({ code: -32000, data: undefined }); }); - it('maps a turn.ended failed event with provider.auth_error to authRequired (-32000)', async () => { - const sessionId = 'sess-provider-auth'; + it('rejects with safe machine-readable data when a turn has a known non-auth failure', async () => { + const sessionId = 'sess-context-overflow'; const errorPayload: KimiErrorPayload = { - code: ErrorCodes.PROVIDER_AUTH_ERROR, - message: 'Provider returned 401', - retryable: false, + code: ErrorCodes.CONTEXT_OVERFLOW, + message: 'Context window exceeded', + retryable: true, }; - const { session } = makeScriptedSession(sessionId, { - script: [ - { - type: 'turn.ended', - sessionId, - agentId: 'main', - turnId: 1, - reason: 'failed', - error: errorPayload, - } as Event, - ], + const { session } = makeFailedSession(sessionId, errorPayload); + + const [, client] = connectToSession(session); + + await client.newSession({ cwd: '/tmp/x', mcpServers: [] }); + const caught = await capturePromptError(client, sessionId); + + expect(caught).toMatchObject({ code: -32603 }); + expect((caught as { data?: unknown }).data).toEqual({ + code: ErrorCodes.CONTEXT_OVERFLOW, + retryable: true, + }); + }); + + it('settles a new prompt after the previous prompt ends with a failure', async () => { + const sessionId = 'sess-consecutive-failures'; + const { session } = makeFailedSession(sessionId, { + code: ErrorCodes.CONTEXT_OVERFLOW, + message: 'Context window exceeded', + retryable: true, }); - const { agentStream, clientStream } = makeInMemoryStreamPair(); - new AgentSideConnection((c) => new AcpServer(makeHarnessWithSession(session), c), agentStream); - const client = new ClientSideConnection(() => new StubClient(), clientStream); + const [, client] = connectToSession(session); await client.newSession({ cwd: '/tmp/x', mcpServers: [] }); - await expect( - client.prompt({ sessionId, prompt: [textBlock('hi')] }), - ).rejects.toMatchObject({ code: -32000 }); + await capturePromptError(client, sessionId); + await expect(capturePromptError(client, sessionId, 'try again')).resolves.toMatchObject({ + code: -32603, + }); }); - it('resolves with end_turn when turn.ended fails with a non-auth code (log-only path)', async () => { - // Non-auth failures stay on the existing log-and-resolve path so - // the client is unblocked. The error appears in the agent log; - // `stopReason` does not signal it (ACP spec discourages errors-via-stopReason). - const sessionId = 'sess-context-overflow'; - const errorPayload: KimiErrorPayload = { + it('unsubscribes from SDK events when a failed turn settles', async () => { + const sessionId = 'sess-failure-cleanup'; + const { session, unsubscribeCount } = makeFailedSession(sessionId, { code: ErrorCodes.CONTEXT_OVERFLOW, message: 'Context window exceeded', retryable: true, - }; - const { session, unsubscribeCount } = makeScriptedSession(sessionId, { + }); + + const [, client] = connectToSession(session); + + await client.newSession({ cwd: '/tmp/x', mcpServers: [] }); + await capturePromptError(client, sessionId); + + expect(unsubscribeCount()).toBe(1); + }); + + it('rejects a failed turn without inventing data when the SDK payload is missing', async () => { + const sessionId = 'sess-failed-without-payload'; + const { session } = makeFailedSession(sessionId); + + const [, client] = connectToSession(session); + + await client.newSession({ cwd: '/tmp/x', mcpServers: [] }); + await expect(capturePromptError(client, sessionId)).resolves.toMatchObject({ + code: -32603, + data: undefined, + }); + }); + + it('omits error data when a failed event carries an unrecognized code', async () => { + const sessionId = 'sess-unknown-error-code'; + const privateMessage = 'unknown-error-message-must-not-cross-the-wire'; + const { session } = makeScriptedSession(sessionId, { script: [ { type: 'turn.ended', @@ -181,69 +254,160 @@ describe('AcpServer error mapping', () => { agentId: 'main', turnId: 1, reason: 'failed', - error: errorPayload, + error: { + code: 'validation.failed', + message: privateMessage, + retryable: false, + }, } as Event, ], }); - const { agentStream, clientStream } = makeInMemoryStreamPair(); - new AgentSideConnection((c) => new AcpServer(makeHarnessWithSession(session), c), agentStream); - const client = new ClientSideConnection(() => new StubClient(), clientStream); + const [, client] = connectToSession(session); await client.newSession({ cwd: '/tmp/x', mcpServers: [] }); - const response = await client.prompt({ sessionId, prompt: [textBlock('hi')] }); - expect(response.stopReason).toBe('end_turn'); - expect(unsubscribeCount()).toBe(1); + const caught = await capturePromptError(client, sessionId); + + expect(caught).toMatchObject({ code: -32603, data: undefined }); + const exposed = exposedError(caught); + expect(exposed).not.toContain(privateMessage); + expect(exposed).not.toContain('validation.failed'); }); - it('maps a synchronous session.prompt rejection carrying an auth code to authRequired (-32000)', async () => { + it.each(['failed event', 'prompt rejection'] as const)( + 'omits private Kimi error codes and messages when delivered as a %s', + async (delivery) => { + const sessionId = `sess-private-kimi-error-${delivery.replace(' ', '-')}`; + const privateMessage = 'private-kimi-error-message-must-not-cross-the-wire'; + const { session } = + delivery === 'failed event' + ? makeFailedSession(sessionId, { + code: ErrorCodes.SESSION_INIT_FAILED, + message: privateMessage, + retryable: false, + }) + : makeScriptedSession(sessionId, { + rejectWith: new KimiError(ErrorCodes.SESSION_INIT_FAILED, privateMessage), + }); + + const [, client] = connectToSession(session); + + await client.newSession({ cwd: '/tmp/x', mcpServers: [] }); + const caught = await capturePromptError(client, sessionId); + + expect(caught).toMatchObject({ code: -32603, data: undefined }); + const exposed = exposedError(caught); + expect(exposed).not.toContain(ErrorCodes.SESSION_INIT_FAILED); + expect(exposed).not.toContain(privateMessage); + }, + ); + + it.each(['failed event', 'prompt rejection'] as const)( + 'exposes the same canonical safe data when a provider failure arrives as a %s', + async (delivery) => { + const sessionId = `sess-provider-api-error-${delivery.replace(' ', '-')}`; + const privateMessage = 'provider-response-body-must-not-cross-the-wire'; + const privateRequestId = 'request-id-must-not-cross-the-wire'; + const details = { + statusCode: 403, + requestId: privateRequestId, + providerBody: { account: 'private-account-data' }, + }; + const { session } = + delivery === 'failed event' + ? makeFailedSession(sessionId, { + code: ErrorCodes.PROVIDER_API_ERROR, + message: privateMessage, + name: 'PrivateProviderErrorName', + details, + retryable: true, + }) + : makeScriptedSession(sessionId, { + rejectWith: new KimiError(ErrorCodes.PROVIDER_API_ERROR, privateMessage, { details }), + }); + + const [, client] = connectToSession(session); + + await client.newSession({ cwd: '/tmp/x', mcpServers: [] }); + const caught = await capturePromptError(client, sessionId); + + expect(caught).toMatchObject({ code: -32603 }); + expect((caught as { data?: unknown }).data).toEqual({ + code: ErrorCodes.PROVIDER_API_ERROR, + retryable: false, + statusCode: 403, + }); + const exposed = exposedError(caught); + expect(exposed).not.toContain(privateMessage); + expect(exposed).not.toContain(privateRequestId); + expect(exposed).not.toContain('PrivateProviderErrorName'); + expect(exposed).not.toContain('private-account-data'); + }, + ); + + it.each([99, 600, 403.5, '403', Number.NaN])( + 'omits statusCode when provider status %j is not a valid HTTP status', + async (statusCode) => { + const sessionId = 'sess-invalid-provider-status'; + const { session } = makeFailedSession(sessionId, { + code: ErrorCodes.PROVIDER_API_ERROR, + message: 'Provider request failed', + details: { statusCode }, + retryable: false, + }); + + const [, client] = connectToSession(session); + + await client.newSession({ cwd: '/tmp/x', mcpServers: [] }); + const caught = await capturePromptError(client, sessionId); + + expect(caught).toMatchObject({ code: -32603 }); + expect((caught as { data?: unknown }).data).toEqual({ + code: ErrorCodes.PROVIDER_API_ERROR, + retryable: false, + }); + }, + ); + + it('rejects with authRequired when session.prompt rejects with an auth code', async () => { const sessionId = 'sess-prompt-rejects-auth'; const { session } = makeScriptedSession(sessionId, { rejectWith: new KimiError(ErrorCodes.PROVIDER_AUTH_ERROR, 'Provider 401'), }); - const { agentStream, clientStream } = makeInMemoryStreamPair(); - new AgentSideConnection((c) => new AcpServer(makeHarnessWithSession(session), c), agentStream); - const client = new ClientSideConnection(() => new StubClient(), clientStream); + const [, client] = connectToSession(session); await client.newSession({ cwd: '/tmp/x', mcpServers: [] }); await expect( client.prompt({ sessionId, prompt: [textBlock('hi')] }), - ).rejects.toMatchObject({ code: -32000 }); + ).rejects.toMatchObject({ code: -32000, data: undefined }); }); - it('maps a generic session.prompt rejection to internalError (-32603) without leaking the stack', async () => { + it('rejects without internal details when session.prompt throws a generic error', async () => { const sessionId = 'sess-generic-error'; const stackTip = 'super-secret-stack-frame-do-not-leak'; const generic = new Error('boom internal'); generic.stack = `Error: boom internal\n at ${stackTip} (secret.ts:1:1)`; const { session } = makeScriptedSession(sessionId, { rejectWith: generic }); - const { agentStream, clientStream } = makeInMemoryStreamPair(); - new AgentSideConnection((c) => new AcpServer(makeHarnessWithSession(session), c), agentStream); - const client = new ClientSideConnection(() => new StubClient(), clientStream); + const [, client] = connectToSession(session); await client.newSession({ cwd: '/tmp/x', mcpServers: [] }); - let captured: unknown; - try { - await client.prompt({ sessionId, prompt: [textBlock('hi')] }); - } catch (err) { - captured = err; - } - expect(captured).toMatchObject({ code: -32603 }); - // Privacy guarantee: the JSON-RPC error response carries only the - // `code` (and optionally a structured `data`); neither the - // original stack nor the raw message crosses the wire. We assert - // negatively rather than on the canonical message because the - // ACP SDK strips the message from the deserialized client-side - // error and only retains the code. - const serialized = JSON.stringify(captured); - expect(serialized).not.toContain(stackTip); - expect(serialized).not.toContain('boom internal'); + const captured = await capturePromptError(client, sessionId); + expect(captured).toMatchObject({ + code: -32603, + data: undefined, + message: 'Internal error: session prompt failed', + }); + // Error.message is non-enumerable, so JSON.stringify(error) alone would + // miss a leak. Reconstruct the observable wire fields explicitly. + const exposed = exposedError(captured); + expect(exposed).not.toContain(stackTip); + expect(exposed).not.toContain('boom internal'); }); - it('still maps reason: cancelled to stop_reason: cancelled (Phase 3/4 regression guard)', async () => { + it('returns cancelled when the turn ends after cancellation', async () => { const sessionId = 'sess-cancel-regression'; const { session } = makeScriptedSession(sessionId, { script: [ @@ -251,50 +415,57 @@ describe('AcpServer error mapping', () => { ], }); - const { agentStream, clientStream } = makeInMemoryStreamPair(); - new AgentSideConnection((c) => new AcpServer(makeHarnessWithSession(session), c), agentStream); - const client = new ClientSideConnection(() => new StubClient(), clientStream); + const [, client] = connectToSession(session); await client.newSession({ cwd: '/tmp/x', mcpServers: [] }); const response = await client.prompt({ sessionId, prompt: [textBlock('hi')] }); expect(response.stopReason).toBe('cancelled'); }); - it('maps blocked turn-end reasons to ACP stopReason refusal', () => { - // ACP has a native `refusal` stop reason that matches a provider safety - // block or prompt-hook block; mapping either to anything else (e.g. - // end_turn) would let the client mistake the block for a clean turn. - expect(turnEndReasonToStopReason('failed', { code: 'provider.filtered' })).toBe('refusal'); - expect(turnEndReasonToStopReason('blocked')).toBe('refusal'); - }); - - it('resolves with refusal when turn.ended fails with provider.filtered', async () => { - const sessionId = 'sess-filtered'; - const { session, unsubscribeCount } = makeScriptedSession(sessionId, { + it('returns refusal when a prompt hook blocks the turn', async () => { + const sessionId = 'sess-blocked'; + const { session } = makeScriptedSession(sessionId, { script: [ { type: 'turn.ended', sessionId, agentId: 'main', turnId: 1, - reason: 'failed', - error: { - code: 'provider.filtered', - message: 'Provider safety policy blocked the response.', - name: 'ProviderFilteredError', - retryable: false, - }, + reason: 'blocked', } as Event, ], }); - const { agentStream, clientStream } = makeInMemoryStreamPair(); - new AgentSideConnection((c) => new AcpServer(makeHarnessWithSession(session), c), agentStream); - const client = new ClientSideConnection(() => new StubClient(), clientStream); + const [, client] = connectToSession(session); await client.newSession({ cwd: '/tmp/x', mcpServers: [] }); const response = await client.prompt({ sessionId, prompt: [textBlock('hi')] }); + expect(response.stopReason).toBe('refusal'); - expect(unsubscribeCount()).toBe(1); }); + + it.each(['failed event', 'prompt rejection'] as const)( + 'returns refusal when provider filtering arrives as a %s', + async (delivery) => { + const sessionId = `sess-filtered-${delivery.replace(' ', '-')}`; + const message = 'Provider safety policy blocked the response.'; + const { session } = + delivery === 'failed event' + ? makeFailedSession(sessionId, { + code: ErrorCodes.PROVIDER_FILTERED, + message, + name: 'ProviderFilteredError', + retryable: false, + }) + : makeScriptedSession(sessionId, { + rejectWith: new KimiError(ErrorCodes.PROVIDER_FILTERED, message), + }); + + const [, client] = connectToSession(session); + + await client.newSession({ cwd: '/tmp/x', mcpServers: [] }); + const response = await client.prompt({ sessionId, prompt: [textBlock('hi')] }); + expect(response.stopReason).toBe('refusal'); + }, + ); }); diff --git a/packages/acp-adapter/test/session-prompt.test.ts b/packages/acp-adapter/test/session-prompt.test.ts index 048fd57f06..7962d63809 100644 --- a/packages/acp-adapter/test/session-prompt.test.ts +++ b/packages/acp-adapter/test/session-prompt.test.ts @@ -14,7 +14,13 @@ import { type WriteTextFileRequest, type WriteTextFileResponse, } from '@agentclientprotocol/sdk'; -import type { Event, KimiHarness, Session } from '@moonshot-ai/kimi-code-sdk'; +import type { + ApprovalHandler, + ApprovalRequest, + Event, + KimiHarness, + Session, +} from '@moonshot-ai/kimi-code-sdk'; import { AcpServer } from '../src/server'; import { AUTHED_STATUS } from './_helpers/harness-stubs'; @@ -50,6 +56,17 @@ class CollectingClient implements Client { } } +class PermissionCollectingClient extends CollectingClient { + readonly permissionRequests: RequestPermissionRequest[] = []; + + override async requestPermission( + request: RequestPermissionRequest, + ): Promise { + this.permissionRequests.push(request); + return { outcome: { outcome: 'cancelled' } }; + } +} + function makeInMemoryStreamPair(): { agentStream: ReturnType; clientStream: ReturnType; @@ -263,6 +280,7 @@ describe('AcpServer session/prompt', () => { let unsubCount = 0; let promptCall = 0; let firstError: unknown; + let approvalHandler: ApprovalHandler | undefined; let resolveFirstTurn: (() => void) | undefined; const firstTurn = new Promise((resolve) => { resolveFirstTurn = () => { @@ -312,6 +330,9 @@ describe('AcpServer session/prompt', () => { listeners.delete(fn); }; }, + setApprovalHandler: (handler: ApprovalHandler | undefined) => { + approvalHandler = handler; + }, } as unknown as Session; const harness = { auth: { status: async () => AUTHED_STATUS }, @@ -320,7 +341,8 @@ describe('AcpServer session/prompt', () => { const { agentStream, clientStream } = makeInMemoryStreamPair(); new AgentSideConnection((c) => new AcpServer(harness, c), agentStream); - const client = new ClientSideConnection(() => new CollectingClient(), clientStream); + const collecting = new PermissionCollectingClient(); + const client = new ClientSideConnection(() => collecting, clientStream); await client.newSession({ cwd: '/tmp/x', mcpServers: [] }); @@ -340,11 +362,128 @@ describe('AcpServer session/prompt', () => { ).rejects.toMatchObject({ code: -32600 }); expect(firstError).toBeUndefined(); + if (approvalHandler === undefined) { + throw new Error('approval handler was not registered'); + } + const approvalRequest: ApprovalRequest = { + toolCallId: 'active-tool', + toolName: 'Bash', + action: 'run command', + display: { kind: 'command', command: 'echo active' }, + }; + await approvalHandler(approvalRequest); + expect(collecting.permissionRequests[0]?.toolCall.toolCallId).toBe('1:active-tool'); + resolveFirstTurn?.(); await expect(firstPrompt).resolves.toMatchObject({ stopReason: 'end_turn' }); expect(unsubCount).toBe(2); }); + it('waits for its own turn when an earlier main-agent turn ends before the prompt launch arrives', async () => { + const sessionId = 'sess-terminal-ownership'; + const listeners = new Set<(event: Event) => void>(); + let promptCall = 0; + let approvalHandler: ApprovalHandler | undefined; + let releaseSecondLaunch: (() => void) | undefined; + let notifySecondLaunchWaiting: (() => void) | undefined; + const secondLaunchWaiting = new Promise((resolve) => { + notifySecondLaunchWaiting = resolve; + }); + const secondLaunchGate = new Promise((resolve) => { + releaseSecondLaunch = resolve; + }); + const emit = (event: Event): void => { + for (const listener of listeners) listener(event); + }; + const session = { + id: sessionId, + prompt: async (_input: unknown) => { + promptCall += 1; + if (promptCall === 1) { + emit({ + type: 'turn.started', + sessionId, + agentId: 'main', + turnId: 1, + origin: { kind: 'user' }, + } as Event); + return; + } + notifySecondLaunchWaiting?.(); + await secondLaunchGate; + emit({ + type: 'turn.started', + sessionId, + agentId: 'main', + turnId: 2, + origin: { kind: 'user' }, + } as Event); + emit({ + type: 'turn.ended', + sessionId, + agentId: 'main', + turnId: 2, + reason: 'completed', + } as Event); + }, + cancel: async () => undefined, + onEvent: (listener: (event: Event) => void) => { + listeners.add(listener); + return () => { + listeners.delete(listener); + }; + }, + setApprovalHandler: (handler: ApprovalHandler | undefined) => { + approvalHandler = handler; + }, + } as unknown as Session; + const harness = { + auth: { status: async () => AUTHED_STATUS }, + createSession: async () => session, + } as unknown as KimiHarness; + + const { agentStream, clientStream } = makeInMemoryStreamPair(); + new AgentSideConnection((connection) => new AcpServer(harness, connection), agentStream); + const collecting = new PermissionCollectingClient(); + const client = new ClientSideConnection(() => collecting, clientStream); + await client.newSession({ cwd: '/tmp/x', mcpServers: [] }); + + const firstPrompt = client.prompt({ sessionId, prompt: [textBlock('first')] }); + await Promise.resolve(); + let secondSettled = false; + const secondPrompt = client + .prompt({ sessionId, prompt: [textBlock('second')] }) + .then((response) => { + secondSettled = true; + return response; + }); + await secondLaunchWaiting; + + emit({ + type: 'turn.ended', + sessionId, + agentId: 'main', + turnId: 1, + reason: 'cancelled', + } as Event); + await expect(firstPrompt).resolves.toMatchObject({ stopReason: 'cancelled' }); + await Promise.resolve(); + expect(secondSettled).toBe(false); + if (approvalHandler === undefined) { + throw new Error('approval handler was not registered'); + } + await approvalHandler({ + toolCallId: 'late-tool', + toolName: 'Bash', + action: 'run command', + display: { kind: 'command', command: 'echo late' }, + }); + expect(collecting.permissionRequests[0]?.toolCall.toolCallId).toBe('late-tool'); + + releaseSecondLaunch?.(); + await expect(secondPrompt).resolves.toMatchObject({ stopReason: 'end_turn' }); + }); + it('ignores a subagent turn.ended and resolves on the main agent turn.ended', async () => { const sessionId = 'sess-subagent'; const { session, unsubscribeCount } = makeScriptedSession(sessionId, [