fix(acp): address Codex review of the tool-call UI seam
- 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.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user