From 8a92338d2f25b2615e9632d4de8af0416e674a58 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Thu, 18 Jun 2026 10:36:53 +0800 Subject: [PATCH] fix(acp): address Codex review of the tool-call UI seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - schemas() builds the model-facing ToolSchema by EXPLICIT allowlist ({name, description, parameters, strict?}) instead of stripping `execute` — presentCall/presentResult are functions that must never leak into a model request, and an allowlist can't drift when a new ToolDefinition member lands. - session/load replay uses a THROWAWAY ToolPresenter, not record.presenter, so a historical interrupted-mid-tool turn (tool/call with no tool/result) can't leave stale in-flight state on the live presenter that serves later events. - ToolPresenter.call/result contain a throwing presentCall/presentResult: log via an onError sink and fall back to the generic presentation, so a buggy display callback can never fail a live turn or a load replay. - acp README inject list now includes `tools`. - remove a stray blank line at EOF (git diff --check gate). Regressions added: schemas() drops presenter callbacks (+ keeps `strict`); session/load replays a tool call with the tool-owned presentation; a throwing presenter is contained (direct + through the real bridge) with and without an onError sink. --- packages/acp/README.md | 2 +- packages/acp/src/index.ts | 60 +++++++++++++++++----- packages/acp/tests/load.spec.ts | 64 +++++++++++++++++++++++- packages/acp/tests/stream-update.spec.ts | 52 +++++++++++++++++++ packages/acp/tests/turns.spec.ts | 27 ++++++++++ packages/tools/src/index.ts | 20 +++++--- packages/tools/tests/tools.spec.ts | 34 ++++++++++++- 7 files changed, 237 insertions(+), 22 deletions(-) diff --git a/packages/acp/README.md b/packages/acp/README.md index f515ae8293..9818c7b13a 100644 --- a/packages/acp/README.md +++ b/packages/acp/README.md @@ -8,7 +8,7 @@ It is a **client-driver / UI plugin**, the structured analogue of the readline ` `apply(ctx, config)` — wires an `AgentSideConnection` (from `@agentclientprotocol/sdk`) to `process.stdin`/`process.stdout` and implements the ACP `Agent` method surface. -`inject: ['agents', 'sessions', 'sessionPersistence']` — programs against the interface packages only (never `dsh-agent-loop`). `sessionPersistence` is required because `initialize` advertises `loadSession: true`. +`inject: ['agents', 'sessions', 'sessionPersistence', 'tools']` — programs against the interface packages only (never `dsh-agent-loop`). `sessionPersistence` is required because `initialize` advertises `loadSession: true`; `tools` lets a tool own how its calls render (`presentCall`/`presentResult`) — the bridge looks the definition up by name and falls back to a generic presentation when a tool declares none (see Tool-call presentation). ### Config diff --git a/packages/acp/src/index.ts b/packages/acp/src/index.ts index 793685072b..283b8fc151 100644 --- a/packages/acp/src/index.ts +++ b/packages/acp/src/index.ts @@ -60,7 +60,7 @@ import { import type { ContentBlock } from '@deepseek-ai/dsh-llm' import type { Agent, AgentStatus } from '@deepseek-ai/dsh-agent' import type { SessionEvent } from '@deepseek-ai/dsh-session' -import type { ToolCallKind, ToolRegistry } from '@deepseek-ai/dsh-tools' +import type { ToolCallKind, ToolCallPresentation, ToolRegistry, ToolResultPresentation } from '@deepseek-ai/dsh-tools' // Side-effect type import: declaration-merges `ctx.sessionPersistence` onto // Context (the bridge injects it and reads `list()` for load cwd validation). import type {} from '@deepseek-ai/dsh-session-persistence' @@ -193,6 +193,9 @@ export function apply(ctx: Context, config: AcpConfig): void { const sessionPersistence = ctx.sessionPersistence const logger = ctx.logger const tools = ctx.tools + // A new ToolPresenter per session (and a throwaway per load replay), each given + // this warn sink so a throwing tool presenter is logged, not propagated. + const makePresenter = (): ToolPresenter => new ToolPresenter(tools, (message) => { logger.warn(message) }) // Live sessions keyed by id (RFC 011 multi-session), plus an agent→sessionId // reverse map so `agent/*` events (which carry only the Agent) demux in O(1). @@ -406,7 +409,7 @@ export function apply(ctx: Context, config: AcpConfig): void { agentOptions: agentOptions(config), }) bySession.set(agent, sessionId) - sessions.set(sessionId, { sessionId, agent, presenter: new ToolPresenter(tools), inflight: undefined }) + sessions.set(sessionId, { sessionId, agent, presenter: makePresenter(), inflight: undefined }) return Promise.resolve({ sessionId }) }, @@ -459,18 +462,24 @@ export function apply(ctx: Context, config: AcpConfig): void { throw invalidParams('connection closed during session/load') } bySession.set(agent, params.sessionId) - const record: SessionRecord = { sessionId: params.sessionId, agent, presenter: new ToolPresenter(tools), inflight: undefined } + const record: SessionRecord = { sessionId: params.sessionId, agent, presenter: makePresenter(), inflight: undefined } sessions.set(params.sessionId, record) // Replay the persisted event log to the client as session/update. Use // the raw event log (NOT deriveMessages, which drops assistant/chunk // and trace events): RFC 010's load contract reconstructs the streamed // turns — user prompts (user/message → user_message_chunk), assistant - // text and reasoning (assistant/chunk), and tool calls/results. The - // record's presenter pairs each tool/call with its tool/result as the - // log replays in order, so the replayed tool cards render identically - // to the live ones. + // text and reasoning (assistant/chunk), and tool calls/results. + // + // Replay through a THROWAWAY presenter, NOT `record.presenter`: a + // historical turn that was interrupted mid-tool (a `tool/call` with no + // matching `tool/result` in the persisted log) would otherwise leave a + // stale in-flight entry on the live presenter, which then serves all + // future live events for this session. The throwaway pairs call→result + // as the log replays in order (same as live) and is discarded after, + // so the record's presenter starts clean for the post-load live stream. + const replayPresenter = makePresenter() for (const event of agent.session.events) { - streamSessionEventUpdate(params.sessionId, event, notify, record.presenter) + streamSessionEventUpdate(params.sessionId, event, notify, replayPresenter) } return {} } finally { @@ -785,13 +794,31 @@ interface ResolvedResultPresentation { export class ToolPresenter { private readonly pending = new Map() - constructor(private readonly tools: Pick) {} + /** + * @param tools the registry to resolve tool definitions by name. + * @param onError invoked when a tool's `presentCall`/`presentResult` THROWS; + * the presenter swallows the error and falls back to the generic + * presentation so a buggy display callback can never fail a live turn or a + * `session/load` replay (AGENTS.md "contain callback exceptions at the + * boundary"). Defaults to a no-op for callers that don't supply a logger. + */ + constructor( + private readonly tools: Pick, + private readonly onError: (message: string) => void = () => {}, + ) {} /** Pending-state presentation for a `tool/call`; remembers `(name, args)` for the matching result. */ call(callId: string, name: string, argsJson: string): ResolvedCallPresentation { const args = parseToolArguments(argsJson) this.pending.set(callId, { name, args }) - const present = this.tools.get(name)?.presentCall?.(args) + let present: ToolCallPresentation | undefined + try { + present = this.tools.get(name)?.presentCall?.(args) + } catch (error: unknown) { + // A throwing presentCall must not break streaming: log and fall back. + this.onError(`acp: tool "${name}" presentCall threw, using generic presentation: ${String(error)}`) + present = undefined + } if (present === undefined) { // No tool-owned presentation: fall back to the tool name as the title and // the full parsed args as the raw input (the pre-seam behavior). @@ -804,9 +831,16 @@ export class ToolPresenter { result(callId: string, content: ContentBlock[], isError: boolean): ResolvedResultPresentation { const call = this.pending.get(callId) this.pending.delete(callId) - const present = call !== undefined - ? this.tools.get(call.name)?.presentResult?.(call.args, { content, isError }) - : undefined + // No remembered call (unknown/late callId) → nothing to present from; raw content. + if (call === undefined) return { content } + let present: ToolResultPresentation | undefined + try { + present = this.tools.get(call.name)?.presentResult?.(call.args, { content, isError }) + } catch (error: unknown) { + // A throwing presentResult must not break streaming/replay: log + fall back. + this.onError(`acp: tool "${call.name}" presentResult threw, using raw result: ${String(error)}`) + present = undefined + } if (present === undefined) return { content } return { content: present.content ?? content, diff --git a/packages/acp/tests/load.spec.ts b/packages/acp/tests/load.spec.ts index 3bed1f836d..3ffef1a87e 100644 --- a/packages/acp/tests/load.spec.ts +++ b/packages/acp/tests/load.spec.ts @@ -4,7 +4,8 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { PROTOCOL_VERSION } from '@agentclientprotocol/sdk' import { SessionId } from '@deepseek-ai/dsh-session' -import { makeBridgeHarness, textResponse, type BridgeHarness, type CapturedUpdate } from './harness.ts' +import { defineTool } from '@deepseek-ai/dsh-tools' +import { makeBridgeHarness, textResponse, toolCallResponse, type BridgeHarness, type CapturedUpdate } from './harness.ts' /** Concatenate the text of all agent_message_chunk updates. */ function messageText(updates: CapturedUpdate[]): string { @@ -56,6 +57,67 @@ describe('acp bridge — session/load replay', () => { expect(userText).toBe('remember this') }) + it('replays a persisted tool call with the TOOL-OWNED presentation (title/rawInput/console output)', async () => { + // A turn with a tool call is persisted, then loaded by a fresh bridge. The + // replayed tool_call/tool_call_update must carry the tool's OWN presentation + // (presentCall/presentResult) — identical to how they streamed live — using + // a throwaway presenter that pairs call→result as the log replays in order. + live = await makeBridgeHarness({ + storageDir, + script: [toolCallResponse('c1', 'bash', { command: 'ls -la', description: 'List files' }), textResponse('done')], + }) + live.ctx.tools.register(defineTool({ + name: 'bash', + description: 'run a command', + parameters: { + command: { type: 'string', required: true }, + description: { type: 'string', required: true }, + }, + async execute() { return [{ type: 'text', text: 'a.txt\nb.txt\n' }] }, + presentCall: args => ({ title: args.description, kind: 'execute', rawInput: args.command }), + presentResult: (_args, result) => { + const block = result.content.length === 1 ? result.content[0] : undefined + if (block === undefined || block.type !== 'text') return undefined + return { content: [{ type: 'text', text: `\`\`\`console\n${block.text.trimEnd()}\n\`\`\`` }] } + }, + })) + await live.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) + const { sessionId } = await live.client.newSession({ cwd: process.cwd(), mcpServers: [] }) + await live.client.prompt({ sessionId, prompt: [{ type: 'text', text: 'list' }] }) + await live.dispose() + live = undefined + + // A fresh bridge — which must ALSO have the tool registered, since the + // presentation is resolved from the live registry at replay time — loads it. + loader = await makeBridgeHarness({ storageDir, script: [] }) + loader.ctx.tools.register(defineTool({ + name: 'bash', + description: 'run a command', + parameters: { + command: { type: 'string', required: true }, + description: { type: 'string', required: true }, + }, + async execute() { return [] }, + presentCall: args => ({ title: args.description, kind: 'execute', rawInput: args.command }), + presentResult: (_args, result) => { + const block = result.content.length === 1 ? result.content[0] : undefined + if (block === undefined || block.type !== 'text') return undefined + return { content: [{ type: 'text', text: `\`\`\`console\n${block.text.trimEnd()}\n\`\`\`` }] } + }, + })) + await loader.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) + await loader.client.loadSession({ sessionId, cwd: process.cwd(), mcpServers: [] }) + + const call = loader.updates.find(u => u.sessionUpdate === 'tool_call') + expect(call).toMatchObject({ toolCallId: 'c1', title: 'List files', kind: 'execute', rawInput: 'ls -la' }) + const update = loader.updates.find(u => u.sessionUpdate === 'tool_call_update') + expect(update).toMatchObject({ + toolCallId: 'c1', + status: 'completed', + content: [{ type: 'content', content: { type: 'text', text: '```console\na.txt\nb.txt\n```' } }], + }) + }) + it('a load whose resume finishes after a client disconnect leaks no live session', async () => { // A session/load is mid-resume() when the client transport closes. The load // must NOT end up with a live registered agent for the connection that is diff --git a/packages/acp/tests/stream-update.spec.ts b/packages/acp/tests/stream-update.spec.ts index 4328b5fb6b..8a22bc71f1 100644 --- a/packages/acp/tests/stream-update.spec.ts +++ b/packages/acp/tests/stream-update.spec.ts @@ -227,6 +227,58 @@ describe('ToolPresenter (tool-owned presentation via the tool registry)', () => })) expect(late).toMatchObject({ content: [{ type: 'content', content: { type: 'text', text: 'late' } }] }) }) + + it('a THROWING presentCall/presentResult is contained: generic fallback + onError, never propagates', () => { + // A buggy tool whose display callbacks throw must NOT fail a live turn or a + // session/load replay (AGENTS.md "contain callback exceptions at the + // boundary"). The presenter swallows the throw, reports via onError, and + // falls back to the generic presentation. + const boom: ToolDefinition = { + name: 'boom', + description: 'b', + parameters: {}, + execute: async () => [], + presentCall: () => { throw new Error('call boom') }, + presentResult: () => { throw new Error('result boom') }, + } + const errors: string[] = [] + const presenter = new ToolPresenter(registryOf(boom), msg => errors.push(msg)) + const updates = updatesWith( + presenter, + evt('tool/call', { turn: 1, step: 1, callId: CallId('c1'), name: 'boom', arguments: '{"a":1}' }), + evt('tool/result', { turn: 1, step: 1, callId: CallId('c1'), content: [{ type: 'text', text: 'raw' }], isError: false }), + ) + // tool/call fell back to title=name, raw args as rawInput. + expect(updates[0]).toMatchObject({ sessionUpdate: 'tool_call', title: 'boom', kind: 'other', rawInput: { a: 1 } }) + // tool/result fell back to the raw content. + expect(updates[1]).toMatchObject({ sessionUpdate: 'tool_call_update', content: [{ type: 'content', content: { type: 'text', text: 'raw' } }] }) + // Both throws were reported, not propagated. + expect(errors).toHaveLength(2) + expect(errors[0]).toContain('presentCall threw') + expect(errors[1]).toContain('presentResult threw') + }) + + it('contains a throwing presenter even with the DEFAULT (no-op) onError sink', () => { + // Constructed without an onError sink (the default `() => {}`): a throwing + // presenter is still swallowed and falls back generically — the absence of a + // logger must not turn a display bug into a propagated exception. + const boom: ToolDefinition = { + name: 'boom', + description: 'b', + parameters: {}, + execute: async () => [], + presentCall: () => { throw new Error('call boom') }, + presentResult: () => { throw new Error('result boom') }, + } + const presenter = new ToolPresenter(registryOf(boom)) + const updates = updatesWith( + presenter, + evt('tool/call', { turn: 1, step: 1, callId: CallId('c1'), name: 'boom', arguments: '{}' }), + evt('tool/result', { turn: 1, step: 1, callId: CallId('c1'), content: [{ type: 'text', text: 'raw' }], isError: false }), + ) + expect(updates[0]).toMatchObject({ sessionUpdate: 'tool_call', title: 'boom' }) + expect(updates[1]).toMatchObject({ sessionUpdate: 'tool_call_update', content: [{ type: 'content', content: { type: 'text', text: 'raw' } }] }) + }) }) describe('agentOptions', () => { diff --git a/packages/acp/tests/turns.spec.ts b/packages/acp/tests/turns.spec.ts index 7258c590b4..101f6849fe 100644 --- a/packages/acp/tests/turns.spec.ts +++ b/packages/acp/tests/turns.spec.ts @@ -111,6 +111,33 @@ describe('acp bridge — turn outcomes', () => { }) }) + it('a throwing tool presenter does not break the turn: the bridge falls back generically', async () => { + // A buggy tool whose presentCall throws must not fail the live turn — the + // bridge's presenter contains the throw (logging via its onError sink) and + // falls back to the generic title=name presentation. Exercises the real + // bridge wiring of the per-session presenter's error sink. + harness = await makeBridgeHarness({ + storageDir, + script: [toolCallResponse('c1', 'kaboom', { x: 1 }), textResponse('done')], + }) + harness.ctx.tools.register(defineTool({ + name: 'kaboom', + description: 'explodes when presented', + parameters: { x: { type: 'number' } }, + async execute() { return [{ type: 'text', text: 'ok' }] }, + presentCall: () => { throw new Error('present boom') }, + })) + const sessionId = await newSession(harness) + const res = await harness.client.prompt({ sessionId, prompt: [{ type: 'text', text: 'go' }] }) + expect(res.stopReason).toBe('end_turn') // the turn completed despite the throw + + const call = harness.updates.find(u => u.sessionUpdate === 'tool_call') + // Generic fallback: title is the tool name, raw args as rawInput. + expect(call).toMatchObject({ toolCallId: 'c1', title: 'kaboom', kind: 'other', rawInput: { x: 1 } }) + const update = harness.updates.find(u => u.sessionUpdate === 'tool_call_update') + expect(update).toMatchObject({ toolCallId: 'c1', status: 'completed' }) + }) + it('a failing tool yields a failed tool_call_update', async () => { harness = await makeBridgeHarness({ storageDir, diff --git a/packages/tools/src/index.ts b/packages/tools/src/index.ts index 9e1e7b74a9..c967f0fba2 100644 --- a/packages/tools/src/index.ts +++ b/packages/tools/src/index.ts @@ -244,14 +244,22 @@ export class ToolRegistry extends Service { } /** - * Return all registered tool schemas, stripped of their `execute` functions. - * These are exactly what gets sent to the model via the system-prompt - * assembly. + * Return all registered tool schemas — exactly the model-facing fields + * (`name`, `description`, `parameters`, and `strict` when set), as sent to the + * model via the system-prompt assembly. Constructed EXPLICITLY rather than by + * stripping known non-schema members: a `ToolDefinition` also carries + * `execute` and the optional `presentCall`/`presentResult` UI callbacks, and + * those (especially the functions) must never leak into a model request. An + * allowlist can't drift when a new non-schema member is added to the + * definition; a denylist (rest-destructure) would silently leak it. */ schemas(): ToolSchema[] { - // Rest-destructure to drop `execute`; the unused binding is the idiom. - // eslint-disable-next-line @typescript-eslint/unbound-method, @typescript-eslint/no-unused-vars - return [...this.store.values()].map(({ execute, ...schema }) => schema) + return [...this.store.values()].map(({ name, description, parameters, strict }): ToolSchema => ({ + name, + description, + parameters, + ...strict !== undefined ? { strict } : {}, + })) } /** diff --git a/packages/tools/tests/tools.spec.ts b/packages/tools/tests/tools.spec.ts index e42e7b4387..9706790d55 100644 --- a/packages/tools/tests/tools.spec.ts +++ b/packages/tools/tests/tools.spec.ts @@ -41,6 +41,39 @@ describe('ToolRegistry', () => { expect(assembly.tools.map(t => t.name)).toEqual(['echo']) }) + it('schemas() drops the UI presentation callbacks — they must never reach the model', async () => { + const ctx = await setup() + // A tool that declares presentCall/presentResult (functions). schemas() feeds + // the system-prompt assembly → the model request, so those callbacks (and + // `execute`) must be stripped: a function in the JSON tool schema would + // corrupt the request. schemas() is an explicit allowlist, so it can't leak. + ctx.tools.register(defineTool({ + name: 'present', + description: 'has presenters', + parameters: { x: { type: 'string', required: true } }, + async execute() { return [] }, + presentCall: args => ({ title: args.x }), + presentResult: (args, result) => ({ title: args.x, content: result.content }), + })) + const schema = ctx.tools.schemas()[0] as unknown as Record + expect(Object.keys(schema).sort()).toEqual(['description', 'name', 'parameters']) + expect(schema.presentCall).toBeUndefined() + expect(schema.presentResult).toBeUndefined() + expect(schema.execute).toBeUndefined() + }) + + it('schemas() preserves `strict` when set (allowlist keeps the model-facing fields)', async () => { + const ctx = await setup() + ctx.tools.register(defineTool({ + name: 'strict-tool', + description: 'd', + parameters: { x: { type: 'string', required: true } }, + strict: true, + async execute() { return [] }, + })) + expect(ctx.tools.schemas()[0]).toMatchObject({ name: 'strict-tool', strict: true }) + }) + it('executes a tool and returns its content', async () => { const ctx = await setup() ctx.tools.register(echoTool) @@ -864,4 +897,3 @@ describe('defineTool presentation (presentCall / presentResult)', () => { expect(tool.presentResult?.({ wrong: 1 }, { content: [], isError: false })).toBeUndefined() }) }) -