From 7803c38824f6617536a34b918e00fca0632c001e Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Thu, 18 Jun 2026 09:01:36 +0800 Subject: [PATCH 01/10] feat(acp): tool-owned tool-call UI presentation (title/command/output) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In Zed the tool-call card showed only "bash" — the bare tool name — instead of what the command does. Fix it by letting each TOOL own how its calls render, rather than the bridge special-casing names. dsh-tools: add an optional two-state presentation seam to ToolDefinition / defineTool — `presentCall(args)` (pending: title, kind, rawInput) and `presentResult(args, result)` (completed: title?, content?). Provider-neutral `ToolCallKind`/`ToolCallPresentation`/`ToolResultPresentation` vocabulary so tools never depend on ACP. defineTool soft-validates args (display runs on log replay, so a malformed/old shape returns undefined instead of throwing). dsh-tool-bash: bash declares presentCall (model `description` → title, exact `command` → rawInput, kind execute) and presentResult (wrap output in a fenced ```console block — a UI-only affordance kept out of the model-facing result); bash_output/bash_kill present task-scoped titles. dsh-acp: inject `tools`; a per-session `ToolPresenter` looks the tool up by name and maps its neutral presentation to the ACP tool_call/tool_call_update wire shape, with a generic fallback (title = name) for tools that declare nothing. Because the `tool/result` event carries only {callId, content, isError}, the presenter keeps a small bridge-local map of ONLY in-flight calls' (name, args), keyed by callId and removed as each result is presented — no event-schema or core change. Replay uses a throwaway presenter so loaded sessions render identically to live ones. Tests: dsh-tools defineTool presenters (typed args, soft-validate), tool-bash bash/bash_output/bash_kill presenters, acp ToolPresenter (tool-owned mapping, unknown-callId fallback, in-flight-only map), and an end-to-end turn through the bridge. The key-gated e2e now asserts a real bash call's title is the model description (not "bash") and rawInput is the command — verified against the real DeepSeek model. The test harness derives its inject from the bridge's exported `inject` so it can't drift again. --- docs/module-graph.md | 11 +- examples/acp-agent/tests/acp.e2e.ts | 17 ++- packages/acp/README.md | 8 +- packages/acp/package.json | 1 + packages/acp/src/index.ts | 127 +++++++++++++++++++-- packages/acp/tests/harness.ts | 6 +- packages/acp/tests/stream-update.spec.ts | 136 ++++++++++++++++++++++- packages/acp/tests/turns.spec.ts | 36 ++++++ packages/acp/tsconfig.json | 1 + packages/tool-bash/README.md | 4 + packages/tool-bash/src/index.ts | 44 ++++++++ packages/tool-bash/tests/tools.spec.ts | 55 +++++++++ packages/tools/README.md | 36 +++++- packages/tools/src/index.ts | 78 +++++++++++++ packages/tools/src/schema.ts | 41 ++++++- packages/tools/tests/tools.spec.ts | 51 +++++++++ 16 files changed, 626 insertions(+), 26 deletions(-) diff --git a/docs/module-graph.md b/docs/module-graph.md index a99e6cb071..5110178e0e 100644 --- a/docs/module-graph.md +++ b/docs/module-graph.md @@ -15,10 +15,6 @@ graph TD agent --> llm agent --> session session-persistence --> session - acp --> agent - acp --> llm - acp --> session - acp --> session-persistence invariants --> agent invariants --> llm invariants --> session @@ -29,6 +25,11 @@ graph TD tools --> agent tools --> llm tools --> system-prompt + acp --> agent + acp --> llm + acp --> session + acp --> session-persistence + acp --> tools agent-loop --> agent agent-loop --> llm agent-loop --> session @@ -52,10 +53,10 @@ graph TD | `system-prompt` | `llm` | | `agent` | `llm`, `session` | | `session-persistence` | `session` | -| `acp` | `agent`, `llm`, `session`, `session-persistence` | | `invariants` | `agent`, `llm`, `session` | | `session-persistence-jsonl` | `session`, `session-persistence` | | `session-persistence-sqlite` | `session`, `session-persistence` | | `tools` | `agent`, `llm`, `system-prompt` | +| `acp` | `agent`, `llm`, `session`, `session-persistence`, `tools` | | `agent-loop` | `agent`, `llm`, `session`, `session-persistence`, `system-prompt`, `tools` | | `tool-bash` | `agent`, `bash`, `llm`, `tools` | diff --git a/examples/acp-agent/tests/acp.e2e.ts b/examples/acp-agent/tests/acp.e2e.ts index 35bef79e14..caab2b0f4a 100644 --- a/examples/acp-agent/tests/acp.e2e.ts +++ b/examples/acp-agent/tests/acp.e2e.ts @@ -174,6 +174,21 @@ describe.skipIf(!process.env.DEEPSEEK_API_KEY)('acp-agent e2e: real prompt over expect(proof).toContain('ACP_OK') // And the client saw tool-call activity stream through. - expect(updates.some(u => u.sessionUpdate === 'tool_call')).toBe(true) + const toolCalls = updates.filter(u => u.sessionUpdate === 'tool_call') + expect(toolCalls.length).toBeGreaterThan(0) + + // Tool-call UI quality (the tool owns its presentation): the bash tool's + // `presentCall` sets the title to the model's human-readable `description` + // and the `rawInput` to the exact command — NOT the bare tool name "bash". + // A `bash` call must therefore carry an execute kind, a non-"bash" title, + // and a string rawInput (the command). `toolCalls` is already narrowed to + // the `tool_call` shape by the filter above, so these fields are reachable. + const bashCall = toolCalls.find(u => u.kind === 'execute') + expect(bashCall).toBeDefined() + if (bashCall === undefined) throw new Error('expected an execute tool_call') + expect(typeof bashCall.title).toBe('string') + expect(bashCall.title.length).toBeGreaterThan(0) + expect(bashCall.title).not.toBe('bash') // the old, unhelpful title + expect(typeof bashCall.rawInput).toBe('string') // the exact command }, 180_000) }) diff --git a/packages/acp/README.md b/packages/acp/README.md index e528acdb45..f515ae8293 100644 --- a/packages/acp/README.md +++ b/packages/acp/README.md @@ -28,7 +28,7 @@ It is a **client-driver / UI plugin**, the structured analogue of the readline ` | `session/load` | `ctx.agents.resume(...)` | replays the persisted event log to the client as `session/update` — the USER side (`user/message` → `user_message_chunk`), assistant text/reasoning (`assistant/chunk`), and tool calls/results (`tool/call` + `tool/result`). Re-loading an already-live id is rejected; the id's load slot is reserved (`loadingIds`) BEFORE the async resume so a pipelined load of the SAME id can't leak a second agent (distinct ids load concurrently). The resumed session keeps its PERSISTED header `cwd`, so its bash tools run in the original workspace; the requested `cwd` only needs to be absolute. After the async resume a `closed` re-check refuses to install a record if the bridge tore down mid-load | | `session/prompt` | `agent.send()` | text-only; rejects image/audio and empty prompts; one in-flight prompt PER session (independent); settles on the OWNING turn's end (a turn that ends in `error` rejects the RPC) | | `session/cancel` | `agent.abort()` | aborts a running step + settles the prompt `cancelled` for ONLY that session — a cancel never touches another session's stream or prompt (see limitation below) | -| `session/update` | `session/event` | `agent_message_chunk` (text-delta), `agent_thought_chunk` (reasoning-delta), `user_message_chunk` (load replay), `tool_call`/`tool_call_update` | +| `session/update` | `session/event` | `agent_message_chunk` (text-delta), `agent_thought_chunk` (reasoning-delta), `user_message_chunk` (load replay), `tool_call`/`tool_call_update` (title/kind/rawInput/content owned by the TOOL via `presentCall`/`presentResult` — see Tool-call presentation) | ## Multi-session (RFC 011) @@ -40,6 +40,12 @@ Background-task isolation rides on `dsh-tool-bash`: bash task ids are global and Each session runs in its own workspace, recorded as the session's `SessionHeader.cwd`. On `session/new` the (absolute) request `cwd` becomes that header cwd; on `session/load` the resumed session keeps its PERSISTED header cwd (the request `cwd` is only shape-checked — it does not override the stored one), and a load whose persisted session has no absolute cwd is REJECTED up front via a metadata-only `list()` check, BEFORE resume constructs an agent (else bash would silently fall back to the server's launch dir, and a post-resume reject would leak the registered agent). `dsh-tool-bash` then defaults the bash workdir to the calling agent's `session.header.cwd` (an explicit model `workdir` still wins; a relative one resolves against the session cwd; with no session cwd the executor falls back to its own config / `process.cwd()`). So the server no longer has to be launched in the workspace — an editor can open any project folder, and N sessions over one connection can each target a different directory. (`additionalDirectories` is still rejected: widening the tool/filesystem scope beyond the single cwd is a separate sandbox concern.) +## Tool-call presentation + +How a tool call renders in the editor is owned by the TOOL, not the bridge — the bridge never special-cases tool names. Each tool may declare `presentCall(args)` (pending state: a human-readable `title`, a `kind` for the icon, and the salient `rawInput` to show in a detail view) and `presentResult(args, result)` (completed state: an optional replacement `title` and reformatted `content`) on its `dsh-tools` definition. The bridge looks the definition up by name in `ctx.tools` and maps the neutral `ToolCallPresentation`/`ToolResultPresentation` to the ACP `tool_call`/`tool_call_update` wire shapes. A tool that declares neither gets a generic fallback (title = tool name, raw parsed args as `rawInput`, kind inferred from the name). For example `dsh-tool-bash` makes the model-written one-line `description` the title ("List files in the current directory"), the exact `command` the `rawInput`, `kind: 'execute'`, and wraps the completed output in a fenced ` ```console ` block. + +The `tool/result` session event carries only `{ callId, content, isError }` — not the tool name or args — so to call a tool's `presentResult` the bridge keeps a small per-session map from `callId` to the in-flight call's `(name, args)`, populated on `tool/call` and removed as each result is presented (it holds only currently-in-flight calls, never finished ones). This is bridge-local state — NOT a change to the event schema or a core service. The map lives on the `SessionRecord`, so two concurrent sessions never cross their in-flight tool state; a `session/load` replay uses a throwaway presenter that pairs each `tool/call` with its `tool/result` as the log replays in order, so replayed tool cards render identically to live ones. + ## Settle-exactly-once A `session/prompt` resolves (or rejects) exactly once, keyed off the canonical session log (the `session/event` stream), NOT the `agent/turn-start`/`agent/turn-end` events. One listener captures the prompt's owning turn from the log's `turn/start` and settles on the matching `turn/end` — the one signal that always fires (`closeTurn` appends it unconditionally, even when a boundary emit throws and the `agent/turn-end` EVENT is skipped). A prompt settles only on ITS OWN turn (`inflight.turn === turn/end.turn`), so a stale `turn/end` for a previously-cancelled turn whose end arrives late can never settle the wrong prompt. A turn that ends `error` REJECTS the RPC with an internal error carrying the failure message (ACP has no error stop reason); every other reason resolves via the codec. As a fallback, when the agent settles to `idle`/`disposed` with a prompt still pending — e.g. a `session/event` listener registered before the bridge threw and starved the bridge's listener — an `agent/status` handler reconciles the prompt from the log (the owning turn's `turn/end`, or `cancelled` if the turn was torn down without one). An empty/whitespace prompt is rejected up front — it would queue no work, so no turn would start and the RPC would hang. diff --git a/packages/acp/package.json b/packages/acp/package.json index 669c03043d..9cac888518 100644 --- a/packages/acp/package.json +++ b/packages/acp/package.json @@ -29,6 +29,7 @@ "@deepseek-ai/dsh-llm": "^0.0.1", "@deepseek-ai/dsh-session": "^0.0.1", "@deepseek-ai/dsh-session-persistence": "^0.0.1", + "@deepseek-ai/dsh-tools": "^0.0.1", "cordis": "^4.0.0-rc.6" }, "devDependencies": { diff --git a/packages/acp/src/index.ts b/packages/acp/src/index.ts index 18f3adb670..793685072b 100644 --- a/packages/acp/src/index.ts +++ b/packages/acp/src/index.ts @@ -60,6 +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' // 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' @@ -73,8 +74,10 @@ import { export const name = 'acp' // The bridge programs against the interface packages only (architecture rule: // plugins never depend on dsh-agent-loop). `sessionPersistence` is required -// because `initialize` advertises `loadSession: true`. -export const inject = ['agents', 'sessions', 'sessionPersistence'] +// because `initialize` advertises `loadSession: true`. `tools` lets a tool own +// how its calls render (`presentCall`/`presentResult`); the bridge looks up the +// definition by name and falls back to a generic presentation when absent. +export const inject = ['agents', 'sessions', 'sessionPersistence', 'tools'] /** * Build an ACP "invalid params" error whose human detail rides in the message. @@ -131,6 +134,13 @@ export const Config: Schema = Schema.object({ interface SessionRecord { sessionId: string agent: Agent + /** + * Resolves tool-owned presentation for THIS session's tool calls and remembers + * each in-flight call's `(name, args)` so the matching `tool/result` can find + * its tool. Per-session so two concurrent sessions never cross their in-flight + * tool state. + */ + presenter: ToolPresenter /** * The in-flight `session/prompt`, or `undefined` when none is pending. A * prompt resolves with a {@link StopReason} or rejects with an Error (a @@ -182,6 +192,7 @@ export function apply(ctx: Context, config: AcpConfig): void { const agents = ctx.agents const sessionPersistence = ctx.sessionPersistence const logger = ctx.logger + const tools = ctx.tools // 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). @@ -270,7 +281,7 @@ export function apply(ctx: Context, config: AcpConfig): void { ctx.on('session/event', (session, event: SessionEvent) => { const rec = sessions.get(session.header.id) if (rec === undefined) return - streamSessionEventUpdate(rec.sessionId, event, notify) + streamSessionEventUpdate(rec.sessionId, event, notify, rec.presenter) const inflight = rec.inflight if (inflight === undefined) return if (event.type === 'turn/start') { @@ -395,7 +406,7 @@ export function apply(ctx: Context, config: AcpConfig): void { agentOptions: agentOptions(config), }) bySession.set(agent, sessionId) - sessions.set(sessionId, { sessionId, agent, inflight: undefined }) + sessions.set(sessionId, { sessionId, agent, presenter: new ToolPresenter(tools), inflight: undefined }) return Promise.resolve({ sessionId }) }, @@ -448,14 +459,18 @@ export function apply(ctx: Context, config: AcpConfig): void { throw invalidParams('connection closed during session/load') } bySession.set(agent, params.sessionId) - sessions.set(params.sessionId, { sessionId: params.sessionId, agent, inflight: undefined }) + const record: SessionRecord = { sessionId: params.sessionId, agent, presenter: new ToolPresenter(tools), 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. + // 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. for (const event of agent.session.events) { - streamSessionEventUpdate(params.sessionId, event, notify) + streamSessionEventUpdate(params.sessionId, event, notify, record.presenter) } return {} } finally { @@ -659,6 +674,14 @@ function validateWorkspaceParams(params: { cwd: string; additionalDirectories?: * - `tool/call` → `tool_call` (pending) * - `tool/result` → `tool_call_update` (completed/failed) * + * Tool-call presentation (title/kind/rawInput, and the completed-state content) + * is owned by each TOOL via `presentCall`/`presentResult` — the bridge never + * special-cases tool names. `presenter` resolves those from the tool registry + * and remembers each call's `(name, args)` so the completed `tool/result` (which + * carries neither) can find its tool. A {@link nullToolPresenter} gives the + * generic fallback (title = tool name, raw args as input) when no registry is + * available (e.g. pure translator tests). + * * Other event types (turn/step boundaries, context/message, usage, …) produce * no client update. */ @@ -666,6 +689,7 @@ export function streamSessionEventUpdate( sessionId: string, event: SessionEvent, notify: (notification: SessionNotification) => void, + presenter: Pick = nullToolPresenter, ): void { switch (event.type) { case 'assistant/chunk': { @@ -690,27 +714,30 @@ export function streamSessionEventUpdate( return } case 'tool/call': { + const present = presenter.call(event.data.callId, event.data.name, event.data.arguments) notify({ sessionId, update: { sessionUpdate: 'tool_call', toolCallId: event.data.callId, - title: event.data.name, - kind: toolKindFor(event.data.name), + title: present.title, + kind: present.kind, status: 'in_progress', - rawInput: parseToolArguments(event.data.arguments), + ...present.rawInput !== undefined ? { rawInput: present.rawInput } : {}, }, }) return } case 'tool/result': { + const present = presenter.result(event.data.callId, event.data.content, event.data.isError) notify({ sessionId, update: { sessionUpdate: 'tool_call_update', toolCallId: event.data.callId, status: event.data.isError ? 'failed' : 'completed', - content: toolResultContent(event.data.content), + content: toolResultContent(present.content), + ...present.title !== undefined ? { title: present.title } : {}, }, }) return @@ -722,8 +749,84 @@ export function streamSessionEventUpdate( } } +/** + * Resolved pending-state presentation the bridge feeds into a `tool_call` + * update: a title is always present (tool name when the tool gives none), `kind` + * and `rawInput` are optional. + */ +interface ResolvedCallPresentation { + title: string + kind: ToolCallKind + rawInput?: unknown +} + +/** Resolved completed-state presentation fed into a `tool_call_update`. */ +interface ResolvedResultPresentation { + /** UI content for the result (harness blocks; the tool may reformat, else the raw result). */ + content: ContentBlock[] + /** Optional replacement title for the completed call. */ + title?: string +} + +/** + * Resolves tool-owned presentation for a session's tool-call events. A tool + * declares `presentCall`/`presentResult` (see `dsh-tools`); this looks them up + * by name in the registry and applies the generic fallback when a tool defines + * neither. + * + * The `tool/result` session event carries only `{ callId, content, isError }` — + * NOT the tool name or args — so to call a tool's `presentResult` (which needs + * both), the presenter remembers each `tool/call`'s `{ name, args }` keyed by + * callId and looks it up on the matching result. The map is bridge-LOCAL (not a + * change to the event schema or a core service): one presenter per live session + * (and a throwaway per `session/load` replay), entries removed as each result + * arrives, so it holds only the currently-in-flight calls. + */ +export class ToolPresenter { + private readonly pending = new Map() + + constructor(private readonly tools: Pick) {} + + /** 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) + 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). + return { title: name, kind: toolKindFor(name), rawInput: args } + } + return { title: present.title, kind: present.kind ?? 'other', rawInput: present.rawInput } + } + + /** Completed-state presentation for a `tool/result`; consumes the remembered `(name, args)`. */ + 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 + if (present === undefined) return { content } + return { + content: present.content ?? content, + ...present.title !== undefined ? { title: present.title } : {}, + } + } +} + +/** + * The no-op presenter used when no tool registry is available (e.g. the pure + * translator tests): every tool gets the generic fallback presentation, and + * results pass their raw content through unchanged. + */ +export const nullToolPresenter: Pick = { + call: (_callId, name, argsJson) => ({ title: name, kind: toolKindFor(name), rawInput: parseToolArguments(argsJson) }), + result: (_callId, content) => ({ content }), +} + /** Map a harness tool name to an ACP ToolKind (best-effort; default `other`). */ -function toolKindFor(name: string): 'read' | 'edit' | 'delete' | 'move' | 'search' | 'execute' | 'fetch' | 'other' { +function toolKindFor(name: string): ToolCallKind { if (name === 'bash' || name === 'bash_output' || name === 'bash_kill') return 'execute' if (name === 'read' || name.startsWith('read')) return 'read' if (name === 'write' || name === 'edit' || name.startsWith('edit')) return 'edit' diff --git a/packages/acp/tests/harness.ts b/packages/acp/tests/harness.ts index bee16c46eb..27335cfcff 100644 --- a/packages/acp/tests/harness.ts +++ b/packages/acp/tests/harness.ts @@ -234,7 +234,11 @@ export async function makeBridgeHarness(options: { // tears down JUST the bridge (its listeners + effect) for the HMR test. harness.acpFiber = await ctx.plugin({ name: 'acp-test', - inject: ['agents', 'sessions', 'sessionPersistence'], + // Use the bridge's REAL exported `inject` so this never drifts from the + // plugin's actual dependency list (adding a service to the bridge must not + // require editing the harness — a hardcoded list silently broke when `tools` + // was added). The bridge programs against the interface packages only. + inject: [...AcpPlugin.inject], apply: (inner: Context) => { AcpPlugin.apply(inner, cfg) }, }) harness.client = new ClientSideConnection(makeClient, clientStream) diff --git a/packages/acp/tests/stream-update.spec.ts b/packages/acp/tests/stream-update.spec.ts index c37de8b344..4328b5fb6b 100644 --- a/packages/acp/tests/stream-update.spec.ts +++ b/packages/acp/tests/stream-update.spec.ts @@ -2,15 +2,22 @@ import { describe, expect, it } from 'vitest' import { CallId } from '@deepseek-ai/dsh-llm' import type { SessionEvent } from '@deepseek-ai/dsh-session' import type { SessionNotification } from '@agentclientprotocol/sdk' -import { streamSessionEventUpdate, agentOptions } from '../src/index.ts' +import type { ToolDefinition, ToolRegistry } from '@deepseek-ai/dsh-tools' +import { streamSessionEventUpdate, agentOptions, ToolPresenter } from '../src/index.ts' -/** Collect the updates a single event produces. */ +/** Collect the updates a single event produces (no presenter → generic fallback). */ function updatesFor(event: SessionEvent): SessionNotification['update'][] { const out: SessionNotification['update'][] = [] streamSessionEventUpdate('s1', event, n => out.push(n.update)) return out } +/** A tiny tool registry stub exposing just `get` for {@link ToolPresenter}. */ +function registryOf(...tools: ToolDefinition[]): Pick { + const map = new Map(tools.map(t => [t.name, t])) + return { get: name => map.get(name) } +} + function evt(type: T, data: Extract['data']): SessionEvent { return { type, seq: 0, time: 0, data } as SessionEvent } @@ -31,7 +38,7 @@ describe('streamSessionEventUpdate', () => { .toEqual([]) }) - it('maps tool/call to an in_progress tool_call with inferred kind and parsed rawInput', () => { + it('maps tool/call to an in_progress tool_call with inferred kind and parsed rawInput (generic fallback, no presenter)', () => { const updates = updatesFor(evt('tool/call', { turn: 1, step: 1, callId: CallId('c1'), name: 'bash', arguments: '{"command":"ls"}' })) expect(updates).toEqual([{ sessionUpdate: 'tool_call', @@ -99,6 +106,129 @@ describe('streamSessionEventUpdate', () => { }) }) +describe('ToolPresenter (tool-owned presentation via the tool registry)', () => { + /** A tool whose presentCall/presentResult mirror what tool-bash declares. */ + const bashLike: ToolDefinition = { + name: 'bash', + description: 'run a command', + parameters: {}, + execute: async () => [], + presentCall: (args: unknown) => { + const a = args as { command: string; description: string } + return { title: a.description, kind: 'execute', rawInput: a.command } + }, + presentResult: (_args: unknown, result: { content: { type: string }[] }) => ({ + content: [{ type: 'text', text: `wrapped:${result.content.length}` }], + }), + } + + function updatesWith(presenter: ToolPresenter, ...events: SessionEvent[]): SessionNotification['update'][] { + const out: SessionNotification['update'][] = [] + for (const event of events) streamSessionEventUpdate('s1', event, n => out.push(n.update), presenter) + return out + } + + it('tool/call uses the tool: description→title, command→rawInput, tool kind', () => { + const presenter = new ToolPresenter(registryOf(bashLike)) + const [update] = updatesWith(presenter, evt('tool/call', { + turn: 1, step: 1, callId: CallId('c1'), name: 'bash', + arguments: JSON.stringify({ command: 'ls -la', description: 'List files' }), + })) + expect(update).toEqual({ + sessionUpdate: 'tool_call', + toolCallId: 'c1', + title: 'List files', + kind: 'execute', + status: 'in_progress', + rawInput: 'ls -la', + }) + }) + + it('tool/result uses the tool to reformat content (resolved by the remembered tool/call)', () => { + const presenter = new ToolPresenter(registryOf(bashLike)) + const updates = updatesWith( + presenter, + evt('tool/call', { turn: 1, step: 1, callId: CallId('c1'), name: 'bash', arguments: JSON.stringify({ command: 'x', description: 'd' }) }), + evt('tool/result', { turn: 1, step: 1, callId: CallId('c1'), content: [{ type: 'text', text: 'out' }], isError: false }), + ) + expect(updates[1]).toEqual({ + sessionUpdate: 'tool_call_update', + toolCallId: 'c1', + status: 'completed', + content: [{ type: 'content', content: { type: 'text', text: 'wrapped:1' } }], + }) + }) + + it('a result with NO preceding call (unknown callId) falls back to the raw content', () => { + const presenter = new ToolPresenter(registryOf(bashLike)) + // No tool/call for c9 → presenter has nothing remembered → generic fallback. + const [update] = updatesWith(presenter, evt('tool/result', { + turn: 1, step: 1, callId: CallId('c9'), content: [{ type: 'text', text: 'raw' }], isError: false, + })) + expect(update).toEqual({ + sessionUpdate: 'tool_call_update', + toolCallId: 'c9', + status: 'completed', + content: [{ type: 'content', content: { type: 'text', text: 'raw' } }], + }) + }) + + it('a tool with no presentCall/presentResult gets the generic fallback (title = name)', () => { + const plain: ToolDefinition = { name: 'plain', description: 'p', parameters: {}, execute: async () => [] } + const presenter = new ToolPresenter(registryOf(plain)) + const [update] = updatesWith(presenter, evt('tool/call', { + turn: 1, step: 1, callId: CallId('c1'), name: 'plain', arguments: '{"a":1}', + })) + expect(update).toMatchObject({ title: 'plain', kind: 'other', rawInput: { a: 1 } }) + }) + + it('a presentation that omits kind/content/rawInput uses the defaults (kind other, raw result content kept)', () => { + // A minimal tool-owned presentation: presentCall returns only a title (no + // kind → defaults to `other`, no rawInput → omitted); presentResult returns + // only a title (no content → the raw result content is kept). + const minimal: ToolDefinition = { + name: 'mini', + description: 'm', + parameters: {}, + execute: async () => [], + presentCall: () => ({ title: 'Doing a thing' }), + presentResult: () => ({ title: 'Did the thing' }), + } + const presenter = new ToolPresenter(registryOf(minimal)) + const updates = updatesWith( + presenter, + evt('tool/call', { turn: 1, step: 1, callId: CallId('c1'), name: 'mini', arguments: '{}' }), + evt('tool/result', { turn: 1, step: 1, callId: CallId('c1'), content: [{ type: 'text', text: 'kept' }], isError: false }), + ) + // No kind → 'other'; no rawInput key at all. + expect(updates[0]).toEqual({ sessionUpdate: 'tool_call', toolCallId: 'c1', title: 'Doing a thing', kind: 'other', status: 'in_progress' }) + // Title replaced; content falls back to the raw result content. + expect(updates[1]).toEqual({ + sessionUpdate: 'tool_call_update', + toolCallId: 'c1', + status: 'completed', + content: [{ type: 'content', content: { type: 'text', text: 'kept' } }], + title: 'Did the thing', + }) + }) + + it('holds ONLY in-flight calls: the callId entry is removed once its result is presented', () => { + const presenter = new ToolPresenter(registryOf(bashLike)) + updatesWith( + presenter, + evt('tool/call', { turn: 1, step: 1, callId: CallId('c1'), name: 'bash', arguments: JSON.stringify({ command: 'x', description: 'd' }) }), + evt('tool/result', { turn: 1, step: 1, callId: CallId('c1'), content: [{ type: 'text', text: 'o' }], isError: false }), + ) + // A SECOND result for the same callId now finds nothing remembered, so it + // falls back to raw content (proving the first result consumed the entry — + // the map does not retain finished calls). + const [late] = updatesWith(presenter, evt('tool/result', { + turn: 1, step: 1, callId: CallId('c1'), content: [{ type: 'text', text: 'late' }], isError: false, + })) + expect(late).toMatchObject({ content: [{ type: 'content', content: { type: 'text', text: 'late' } }] }) + }) +}) + describe('agentOptions', () => { it('includes only the fields present in config', () => { expect(agentOptions({})).toEqual({}) diff --git a/packages/acp/tests/turns.spec.ts b/packages/acp/tests/turns.spec.ts index eef0f392aa..7258c590b4 100644 --- a/packages/acp/tests/turns.spec.ts +++ b/packages/acp/tests/turns.spec.ts @@ -75,6 +75,42 @@ describe('acp bridge — turn outcomes', () => { expect(callIdx).toBeLessThan(updIdx) }) + it('a tool-owned presentation flows end-to-end: presentCall sets title/rawInput, presentResult reformats output', async () => { + harness = await makeBridgeHarness({ + storageDir, + script: [toolCallResponse('c1', 'bash', { command: 'ls -la', description: 'List files' }), textResponse('done')], + }) + // A tool that declares its OWN presentation (like the real tool-bash). The + // bridge must use it — NOT the generic title=name fallback — proving the + // tool-owns-its-rendering seam works through the real session-event path. + harness.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\`\`\`` }] } + }, + })) + const sessionId = await newSession(harness) + await harness.client.prompt({ sessionId, prompt: [{ type: 'text', text: 'list' }] }) + + const call = harness.updates.find(u => u.sessionUpdate === 'tool_call') + expect(call).toMatchObject({ toolCallId: 'c1', title: 'List files', kind: 'execute', rawInput: 'ls -la', status: 'in_progress' }) + const update = harness.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 failing tool yields a failed tool_call_update', async () => { harness = await makeBridgeHarness({ storageDir, diff --git a/packages/acp/tsconfig.json b/packages/acp/tsconfig.json index 2efad36448..83330256e3 100644 --- a/packages/acp/tsconfig.json +++ b/packages/acp/tsconfig.json @@ -12,6 +12,7 @@ { "path": "../llm" }, { "path": "../session" }, { "path": "../agent" }, + { "path": "../tools" }, { "path": "../session-persistence" } ] } diff --git a/packages/tool-bash/README.md b/packages/tool-bash/README.md index 83f84e967d..473f5a4099 100644 --- a/packages/tool-bash/README.md +++ b/packages/tool-bash/README.md @@ -32,6 +32,10 @@ Result text: stdout, then a `[stderr]` section, then status markers — `[timed The owning agent is recorded per task id at spawn and kept for the lifetime of the loaded plugin instance (it is **not** cleared on completion). `bash_output`/`bash_kill` reject a task owned by a *different* agent with `task belongs to another session` (a task started with no agent — a non-loop caller — has no owner and is open to anyone; a call with no `exec.agent` cannot access an owned task). Task ids are global and predictable, so under multi-session ACP this ownership check is the fence that stops one session's agent from reading or killing another session's background task. (`TODO(tool-bash-owner-hmr)`: an independent HMR reload of this plugin starts a fresh map, so a task spawned before the reload becomes un-owned — acceptable as HMR is dev-only and the session boundary is one user's cooperative editor; a durable fix attaches ownership to the executor/task lifetime.) +## UI presentation + +These tools own how their calls render in a UI (an editor's tool-call card) via the `dsh-tools` `presentCall`/`presentResult` seam — a UI never special-cases tool names. For `bash`: the model-written `description` is the always-visible **title** (e.g. "List files in the current directory"), the exact `command` is the **rawInput** (the verbatim command stays visible in a detail view without crowding the title), `kind` is `execute` (terminal/run treatment), and the completed output is wrapped in a fenced ` ```console ` block — a UI-only affordance, so the model-facing result text stays unfenced. `bash_output`/`bash_kill` present a task-scoped title ("Read output from background task bash-3" / "Kill background task bash-3") with the task id as rawInput. These methods are pure/display-only (they also run on `session/load` replay), and a malformed/older logged arg shape falls back to a generic presentation rather than throwing. See `packages/tools` ("Tool-owned UI presentation") and `packages/acp` ("Tool-call presentation"). + ## Background completion notices When a background task finishes, a short notice is injected into the owning agent's session (`agent.inject()`, source `{kind: 'plugin', plugin: 'tool-bash'}`). Injection is **durable context for the next model request, not a wake-up** — an idle agent stays idle until something sends a message. That's why the tool descriptions tell the model to poll with `bash_output`. diff --git a/packages/tool-bash/src/index.ts b/packages/tool-bash/src/index.ts index 9a15c6ad7a..9b4aa0819b 100644 --- a/packages/tool-bash/src/index.ts +++ b/packages/tool-bash/src/index.ts @@ -39,6 +39,8 @@ import type { Context } from 'cordis' import { isAbsolute, resolve as resolvePath } from 'node:path' import { defineTool } from '@deepseek-ai/dsh-tools' +import type { ToolCallPresentation, ToolResult, ToolResultPresentation } from '@deepseek-ai/dsh-tools' +import type { ContentBlock } from '@deepseek-ai/dsh-llm' import type { Agent } from '@deepseek-ai/dsh-agent' import type { BashRunResult, BashTask, CollectedOutput } from '@deepseek-ai/dsh-bash' @@ -124,6 +126,44 @@ export function renderResult(result: BashRunResult): string { return body + markers.join('\n') } +// --------------------------------------------------------------------------- +// UI presentation (tool-owned). These shape how a UI (e.g. the ACP bridge) +// renders a bash call's pending and completed states. They are display-only and +// pure — a UI may call them during live streaming AND a session-log replay. +// --------------------------------------------------------------------------- + +/** + * Pending-state presentation for a `bash` call: the model-written `description` + * is the always-visible title (the schema requires it precisely so a UI has a + * readable summary — "List files in the current directory"), `kind: 'execute'` + * (a terminal/run treatment), and the exact `command` is the `rawInput` so the + * verbatim command stays visible in a UI's detail view without crowding the + * title. Mirrors how Zed / the reference ACP adapters render execute tools. + */ +function presentBashCall(args: { command: string; description: string }): ToolCallPresentation { + return { title: args.description, kind: 'execute', rawInput: args.command } +} + +/** + * Completed-state presentation for a `bash` call: wrap the model-facing result + * text in a fenced ```console block so a UI renders the output monospaced as a + * terminal transcript. The model-facing `content` (what `execute` returned) is + * intentionally NOT fenced — the fences are a UI-only affordance, so they live + * here, not in `renderResult`. A non-text result (unexpected for bash) is left + * untouched by falling back to `undefined`. + */ +function presentBashResult(_args: unknown, result: ToolResult): ToolResultPresentation | undefined { + const block = result.content.length === 1 ? result.content[0] : undefined + if (block === undefined || block.type !== 'text') return undefined + const fenced: ContentBlock = { type: 'text', text: `\`\`\`console\n${block.text.replace(/\n+$/, '')}\n\`\`\`` } + return { content: [fenced] } +} + +/** Pending-state presentation for `bash_output`/`bash_kill` (background-task tools). */ +function presentTaskCall(verb: string, args: { task_id: string }): ToolCallPresentation { + return { title: `${verb} background task ${args.task_id}`, kind: 'execute', rawInput: args.task_id } +} + /** * Resolve the working directory for a bash call. Precedence: an explicit model * `workdir` wins; otherwise default to the calling agent's session cwd @@ -242,6 +282,8 @@ export function apply(ctx: Context): void { if (result.aborted) throw new Error('command aborted') return [{ type: 'text', text: renderResult(result) }] }, + presentCall: presentBashCall, + presentResult: presentBashResult, })) ctx.tools.register(defineTool({ @@ -266,6 +308,7 @@ export function apply(ctx: Context): void { text += `\n${statusLine(read.task)}` return Promise.resolve([{ type: 'text', text }]) }, + presentCall: args => presentTaskCall('Read output from', args), })) ctx.tools.register(defineTool({ @@ -283,5 +326,6 @@ export function apply(ctx: Context): void { text: killed ? `killed background task ${id}` : `task ${id} had already finished`, }]) }, + presentCall: args => presentTaskCall('Kill', args), })) } diff --git a/packages/tool-bash/tests/tools.spec.ts b/packages/tool-bash/tests/tools.spec.ts index cc25d9d9d4..8b68de213b 100644 --- a/packages/tool-bash/tests/tools.spec.ts +++ b/packages/tool-bash/tests/tools.spec.ts @@ -562,3 +562,58 @@ describe('status lines', () => { expect(text(read)).toContain('[status: completed, exit code: 0]') }) }) + +describe('tool-owned UI presentation (presentCall / presentResult)', () => { + it('bash presentCall: the model description is the title, the command is the rawInput, kind execute', async () => { + const ctx = await setup() + const present = ctx.tools.get('bash')!.presentCall!({ command: 'ls -la src', description: 'List files in src' }) + expect(present).toEqual({ title: 'List files in src', kind: 'execute', rawInput: 'ls -la src' }) + }) + + it('bash presentResult: wraps the model-facing text in a fenced console block', async () => { + const ctx = await setup() + const present = ctx.tools.get('bash')!.presentResult!( + { command: 'echo hi', description: 'echo' }, + { content: [{ type: 'text', text: 'hi\n[exit code: 0]\n\n' }], isError: false }, + ) + // Trailing blank lines are trimmed; the body is fenced as ```console. + expect(present).toEqual({ content: [{ type: 'text', text: '```console\nhi\n[exit code: 0]\n```' }] }) + }) + + it('bash presentResult: leaves a non-text (unexpected) result untouched → undefined (UI keeps raw content)', async () => { + const ctx = await setup() + const present = ctx.tools.get('bash')!.presentResult!( + { command: 'x', description: 'x' }, + { content: [{ type: 'image', url: 'https://x/y.png' }], isError: false }, + ) + expect(present).toBeUndefined() + }) + + it('bash presentResult: a result that is not exactly one block → undefined (no single text to fence)', async () => { + const ctx = await setup() + const args = { command: 'x', description: 'x' } + // Empty content (no block) and multi-block content both fall through. + expect(ctx.tools.get('bash')!.presentResult!(args, { content: [], isError: false })).toBeUndefined() + expect(ctx.tools.get('bash')!.presentResult!(args, { + content: [{ type: 'text', text: 'a' }, { type: 'text', text: 'b' }], + isError: false, + })).toBeUndefined() + }) + + it('bash_output / bash_kill presentCall: a readable task-scoped title, task id as rawInput', async () => { + const ctx = await setup() + expect(ctx.tools.get('bash_output')!.presentCall!({ task_id: 'bash-3' })) + .toEqual({ title: 'Read output from background task bash-3', kind: 'execute', rawInput: 'bash-3' }) + expect(ctx.tools.get('bash_kill')!.presentCall!({ task_id: 'bash-3' })) + .toEqual({ title: 'Kill background task bash-3', kind: 'execute', rawInput: 'bash-3' }) + }) + + it('presentCall validates softly: malformed args (missing required description) return undefined, never throw', async () => { + const ctx = await setup() + // defineTool wraps presentCall to soft-validate against the schema and fall + // back to undefined (a generic UI presentation) rather than throwing on the + // display path — it may run on replay of arbitrary logged args. The + // ToolDefinition.presentCall takes `unknown`, so a malformed shape needs no cast. + expect(ctx.tools.get('bash')?.presentCall?.({ command: 'ls' })).toBeUndefined() + }) +}) diff --git a/packages/tools/README.md b/packages/tools/README.md index 560195387d..d33e28f1e9 100644 --- a/packages/tools/README.md +++ b/packages/tools/README.md @@ -24,9 +24,10 @@ Tool registry and execution waterfall. Tool plugins register their schemas and e ### Key types -- `ToolDefinition` — `ToolSchema` + `execute(args, exec): Promise`. +- `ToolDefinition` — `ToolSchema` + `execute(args, exec): Promise`, plus optional `presentCall(args)` / `presentResult(args, result)` for tool-owned UI presentation (see below). - `ToolExecution` — one pending tool call: `{ callId, name, arguments, agent?, signal? }`. - `ToolExecutionResult` — outcome: `{ callId, content, isError, error? }`. On failure with a `HarnessError`, `error: { name, code }` carries the structured failure class alongside the model-facing text (the loop forwards it onto the `tool/result` session event for retry/sandbox plugins and replay). +- `ToolCallPresentation` / `ToolResultPresentation` — provider-neutral shapes a tool returns from `presentCall` / `presentResult` to own how a UI renders ITS calls (see "Tool-owned UI presentation"). ### Extension points @@ -67,6 +68,39 @@ A `defineTool` tool also **validates the model-generated arguments against its ` See `defineTool`, `validateArgs`, `ToolArgsError`, `SchemaSpec`, `InferArgs`, and `schemaSpecToJsonSchema` in the public API for details. +### Tool-owned UI presentation + +A tool owns how ITS calls render in a UI (an editor's tool-call card, a CLI log line) — a UI plugin must NOT special-case tool names. A `ToolDefinition` may declare two optional, pure, display-only methods: + +- `presentCall(args): ToolCallPresentation | undefined` — the PENDING state: a human-readable `title` (always-visible label), an optional `kind` (`read`/`edit`/`execute`/… for icon/treatment, default `other`), and an optional `rawInput` (the salient input to show in a detail view — e.g. a shell command as a string, NOT the whole args object). +- `presentResult(args, result): ToolResultPresentation | undefined` — the COMPLETED state, given the same `args` and the `{ content, isError }` result: an optional replacement `title` and reformatted `content` (e.g. wrap command output in a fenced ` ```console ` block — a UI-only affordance that must NOT appear in the model-facing `execute` result). + +Returning `undefined` (or omitting a method) tells a UI to fall back to a generic presentation (title = tool name, raw args as input, raw result content). Both methods must be **pure and side-effect-free**: a UI may call them during live streaming AND during a session-log replay, so they depend only on their arguments. With `defineTool`, `args` is the typed `InferArgs` shape; the helper soft-validates before calling (a malformed/older logged arg shape yields `undefined` rather than throwing, since display must never crash a replay). The shapes are provider-neutral — the ACP bridge (`dsh-acp`) maps them to ACP `tool_call`/`tool_call_update` wire fields, and `dsh-tool-bash` is the reference implementation. + +```ts +import { defineTool } from '@deepseek-ai/dsh-tools' + +const bash = defineTool({ + name: 'bash', + description: 'Run a shell command.', + parameters: { + command: { type: 'string', required: true, description: 'The command to run.' }, + description: { type: 'string', required: true, description: 'One-line summary shown in the UI.' }, + }, + async execute(args) { + return [{ type: 'text', text: `ran: ${args.command}` }] + }, + // The model-written description is the readable title; the command is the detail. + presentCall: args => ({ title: args.description, kind: 'execute', rawInput: args.command }), + // Wrap the output as a console block for the UI (not in the model-facing result). + 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 + '\n```' }] } + }, +}) +``` + ### What is NOT here (TODO) - **Tool shapes review** — when real tools land (e.g. a concurrency-safety hint for parallel execution); phase 1 executes tool calls sequentially. diff --git a/packages/tools/src/index.ts b/packages/tools/src/index.ts index 8e8c106147..9e1e7b74a9 100644 --- a/packages/tools/src/index.ts +++ b/packages/tools/src/index.ts @@ -50,9 +50,87 @@ declare module 'cordis' { // parallel execution — Claude Code partitions read-only tools; phase 1 // executes sequentially). +/** + * Category of a tool call, used by a UI to pick an icon / treatment. A neutral + * vocabulary owned here (NOT an ACP type) so tools describe themselves without + * depending on any client protocol; a UI bridge maps it to its own enum. The + * member set mirrors the common ACP `ToolKind` values; `other` is the default. + */ +export type ToolCallKind = 'read' | 'edit' | 'delete' | 'move' | 'search' | 'execute' | 'fetch' | 'other' + +/** + * How a tool wants ONE of its calls shown in a UI (an editor's tool-call card, + * a CLI log line) BEFORE the result is known — the *pending* state. Provider- + * neutral: a tool returns this from {@link ToolDefinition.presentCall} and a UI + * plugin (e.g. the ACP bridge) maps it to its own wire shape. The tool owns its + * own presentation — the UI must not special-case tool names. + */ +export interface ToolCallPresentation { + /** + * Human-readable, always-visible label describing what THIS call does (e.g. + * the model-written one-line summary of a bash command). Keep it short — a UI + * shows it as a card header / log line. Required: a presentation must have a + * title (a UI falls back to the tool name only when `presentCall` is absent). + */ + title: string + /** Category for icon/treatment; defaults to `other` when omitted. */ + kind?: ToolCallKind + /** + * The salient input to surface in a detail/expanded view — e.g. the bash + * COMMAND itself (as a string), so the title can stay a readable summary + * while the exact command is still visible. Omit to show nothing; a string is + * rendered as-is, an object as pretty JSON. NOT the full raw args object + * unless that is genuinely what a reader wants. + */ + rawInput?: unknown +} + +/** + * How a tool wants the COMPLETED call shown — the *result* state, after + * `execute` returns. Lets the tool reformat its result for a UI distinctly from + * the model-facing text it returned from `execute` (e.g. wrap command output in + * a fenced ```console block for monospace rendering, which the model-facing + * result must NOT carry). All fields optional: a UI keeps the pending-state + * title and renders the raw result content for anything left unset. + */ +export interface ToolResultPresentation { + /** Replacement title for the completed call (e.g. append an exit status). Omit to keep the pending-state title. */ + title?: string + /** + * UI-facing result content (harness {@link ContentBlock}s), reformatted from + * the model-facing result. Omit to let the UI render the raw result content. + * Stays in harness vocabulary; the UI maps these to its own content blocks. + */ + content?: ContentBlock[] +} + /** A registered tool: its schema plus the execution function. */ export interface ToolDefinition extends ToolSchema { execute(args: unknown, exec: ToolExecution): Promise + /** + * Optional: how to present the PENDING state of one call in a UI, derived + * from the call's `args` (parsed arguments, `unknown` — the tool validates/ + * narrows its own input). Returning `undefined` (or omitting the method) tells + * a UI to fall back to a generic presentation (title = tool name, raw args as + * input). Pure and side-effect-free: a UI may call it during live streaming + * AND a session-log replay, so it must depend only on `args`. + */ + presentCall?(args: unknown): ToolCallPresentation | undefined + /** + * Optional: how to present the COMPLETED state, given the same `args` and the + * `result` (`execute`'s content + whether it errored). Returning `undefined` + * (or omitting the method) tells a UI to keep the pending title and render the + * raw result content. Pure and side-effect-free for the same replay reason. + */ + presentResult?(args: unknown, result: ToolResult): ToolResultPresentation | undefined +} + +/** The completed outcome handed to {@link ToolDefinition.presentResult}. */ +export interface ToolResult { + /** The model-facing content `execute` returned (or the error text on failure). */ + content: ContentBlock[] + /** Whether the call failed. */ + isError: boolean } /** One pending tool call, as it flows through the execution waterfall. */ diff --git a/packages/tools/src/schema.ts b/packages/tools/src/schema.ts index 5572e43ff3..5e8887f11b 100644 --- a/packages/tools/src/schema.ts +++ b/packages/tools/src/schema.ts @@ -21,7 +21,7 @@ import type { ContentBlock } from '@deepseek-ai/dsh-llm' import { assertNever, HarnessError } from '@deepseek-ai/dsh-llm' -import type { ToolDefinition, ToolExecution } from './index.ts' +import type { ToolCallPresentation, ToolDefinition, ToolExecution, ToolResult, ToolResultPresentation } from './index.ts' // --------------------------------------------------------------------------- // SchemaSpec — the author-facing per-property type @@ -287,6 +287,22 @@ export interface DefineToolOptions { * casts needed. */ execute(args: InferArgs, exec: ToolExecution): Promise + /** + * Optional: how to present the PENDING state of one call in a UI (an editor + * tool-call card, a CLI log line). `args` is the typed, schema-validated + * argument shape — zero casts. Pure and side-effect-free: a UI may call it + * during live streaming AND a session-log replay, so depend only on `args`. + * The tool owns its presentation so a UI never special-cases tool names. See + * {@link ToolCallPresentation}. + */ + presentCall?(args: InferArgs): ToolCallPresentation | undefined + /** + * Optional: how to present the COMPLETED state, given the typed `args` and the + * `result`. Use it to reformat result content for a UI distinctly from the + * model-facing text (e.g. a fenced ```console block). Pure and side-effect- + * free for the same replay reason. See {@link ToolResultPresentation}. + */ + presentResult?(args: InferArgs, result: ToolResult): ToolResultPresentation | undefined /** Whether the tool requires structured output (default false). */ strict?: boolean } @@ -322,7 +338,11 @@ export function defineTool(options: DefineToolOptions): // Object-literal execute methods don't use `this`; the reference is safe. // eslint-disable-next-line @typescript-eslint/unbound-method const userExecute = options.execute - return { + // eslint-disable-next-line @typescript-eslint/unbound-method + const userPresentCall = options.presentCall + // eslint-disable-next-line @typescript-eslint/unbound-method + const userPresentResult = options.presentResult + const tool: ToolDefinition = { name: options.name, description: options.description, parameters: schemaSpecToJsonSchema(options.parameters) as unknown as Record, @@ -337,4 +357,21 @@ export function defineTool(options: DefineToolOptions): return userExecute(args as InferArgs, exec) }, } + // Presentation is display-only and may run on REPLAY of arbitrary logged args + // (possibly from an older schema), so it must never throw: validate softly and + // fall back to `undefined` (a generic UI presentation) on any mismatch, rather + // than the hard `ToolArgsError` the execute path raises. + if (userPresentCall) { + tool.presentCall = (args: unknown): ToolCallPresentation | undefined => { + if (validateArgs(options.parameters, args).length > 0) return undefined + return userPresentCall(args as InferArgs) + } + } + if (userPresentResult) { + tool.presentResult = (args: unknown, result: ToolResult): ToolResultPresentation | undefined => { + if (validateArgs(options.parameters, args).length > 0) return undefined + return userPresentResult(args as InferArgs, result) + } + } + return tool } diff --git a/packages/tools/tests/tools.spec.ts b/packages/tools/tests/tools.spec.ts index fb42df4492..e42e7b4387 100644 --- a/packages/tools/tests/tools.spec.ts +++ b/packages/tools/tests/tools.spec.ts @@ -814,3 +814,54 @@ describe('defineTool validation (the runtime-validation RFC, part 1)', () => { expect(result.isError).toBe(false) }) }) + +describe('defineTool presentation (presentCall / presentResult)', () => { + it('threads presentCall/presentResult onto the ToolDefinition with typed args', () => { + const tool = defineTool({ + name: 'demo', + description: 'demo', + parameters: { path: { type: 'string', required: true }, n: { type: 'number' } }, + async execute() { return [{ type: 'text', text: 'ok' }] }, + presentCall(args) { + // args is typed { path: string; n?: number } — zero casts. + expectTypeOf(args).toEqualTypeOf<{ path: string; n?: number }>() + return { title: `Open ${args.path}`, kind: 'read', rawInput: args.path } + }, + presentResult(args, result) { + return { title: `Opened ${args.path}`, content: result.content } + }, + }) + expect(tool.presentCall!({ path: '/a', n: 2 })).toEqual({ title: 'Open /a', kind: 'read', rawInput: '/a' }) + expect(tool.presentResult!({ path: '/a' }, { content: [{ type: 'text', text: 'x' }], isError: false })) + .toEqual({ title: 'Opened /a', content: [{ type: 'text', text: 'x' }] }) + }) + + it('a tool without presentCall/presentResult leaves them undefined (UI falls back generically)', () => { + const tool = defineTool({ + name: 'plain', + description: 'plain', + parameters: { x: { type: 'string', required: true } }, + async execute() { return [] }, + }) + expect(typeof tool.presentCall).toBe('undefined') + expect(typeof tool.presentResult).toBe('undefined') + }) + + it('presentCall/presentResult validate softly: malformed args return undefined, never throw (display runs on replay)', () => { + const tool = defineTool({ + name: 'demo', + description: 'demo', + parameters: { path: { type: 'string', required: true } }, + async execute() { return [] }, + presentCall: args => ({ title: args.path }), + presentResult: (args, result) => ({ title: args.path, content: result.content }), + }) + // Unlike execute (which throws ToolArgsError on a mismatch), the display + // methods soft-validate and fall back to undefined so a UI never crashes + // replaying an old/foreign log entry. The ToolDefinition methods take + // `unknown`, so malformed shapes pass without a cast. + expect(tool.presentCall?.({})).toBeUndefined() + expect(tool.presentResult?.({ wrong: 1 }, { content: [], isError: false })).toBeUndefined() + }) +}) + 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 02/10] 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() }) }) - From 8acafe918fed6bed8afd94287e0ba7dbeb89453c Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Thu, 18 Jun 2026 11:23:12 +0800 Subject: [PATCH 03/10] feat(acp): show the command in execute titles; test via the real bash tool; RFC for terminal rendering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - bash presentCall title is now "description — command" (e.g. "List files in src — ls -la src"). An execute-kind ACP card HIDES rawInput (Zed renders it only for non-terminal tools), so the command must ride in the always-visible title to be seen — matching how claude-agent-acp/codex-acp title execute tools. The command stays in rawInput too for non-execute UIs that show it. - Rework the acp tool-call presentation tests (turns + load replay) to drive the REAL dsh-tool-bash + dsh-bash-local via a new makeBridgeHarness({ withBash }) option, running an actual `echo` — instead of an inline fake bash tool. The mock MODEL still scripts the call (deterministic, no key), but the tool and executor are real, so the test verifies the shipping presentCall/presentResult. - AGENTS.md: add the principle "prefer the REAL implementation over a mock/ stand-in in tests" (mock only the expensive/non-deterministic boundary). - RFC (proposed): the ACP terminal sub-protocol + command classification — the capability-gated rich rendering (live cwd-header terminal card, classify a `cat` as a read / `grep` as a search) that the reference adapters do; the fenced ```console text block stays the no-capability baseline. Studied codex-acp, claude-agent-acp, and Zed's renderer to ground it. --- AGENTS.md | 1 + docs/rfc/README.md | 1 + ...6-06-18-acp-terminal-and-tool-rendering.md | 49 ++++++++++++++ packages/acp/README.md | 4 +- packages/acp/package.json | 2 + packages/acp/src/index.ts | 11 +++- packages/acp/tests/harness.ts | 14 ++++ packages/acp/tests/load.spec.ts | 64 ++++++------------- packages/acp/tests/turns.spec.ts | 53 +++++++-------- packages/tool-bash/README.md | 2 +- packages/tool-bash/src/index.ts | 17 +++-- packages/tool-bash/tests/tools.spec.ts | 6 +- pnpm-lock.yaml | 6 ++ 13 files changed, 144 insertions(+), 86 deletions(-) create mode 100644 docs/rfc/proposed/2026-06-18-acp-terminal-and-tool-rendering.md diff --git a/AGENTS.md b/AGENTS.md index c1f183ee04..d4b8474865 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -121,6 +121,7 @@ Dev/test/demo run **unbuilt** via tsx + the `paths` map in the root `tsconfig.js - **Merging PRs**: always merge with a **merge commit** (`gh pr merge --merge`), never squash or rebase. The per-PR commit history is intentional — review-fix commits, regression-test commits, and the reasoning in each message are part of the record — and squashing flattens it away. - **TODO markers**: use `FIXME`/`TODO`/`XXX` to flag known issues by urgency — see [docs/development.md](docs/development.md) for the semantics of each. - **Tests**: vitest, colocated under `packages//tests/*.spec.ts`. Every registry needs an HMR-safety test (dispose the contributing fiber, assert cleanup). **Excessive tests are welcome** — when in doubt, write the test; err on the side of covering edge cases, error paths, event ordering, and concurrency races even if they seem unlikely. Review findings get regression tests (see `packages/agent-loop/tests/review-fixes.spec.ts`). The same generosity applies to **real-API (with-key) e2e tests — inference is cheap here (we are DeepSeek), so do not ration them**: cover the agent's real flows (a real prompt that writes a file, multi-turn, tool use, cancellation) and run them frequently while developing, especially cheap **smoke tests** that boot the real example and check the world. A green mock/no-key suite proves the plumbing, not the product — the with-key smoke test is what catches "green units, broken product". See § Secrets / .env for the with-key policy and why self-skip is a CI accommodation, not a verdict that real-API tests are expensive. +- **Prefer the REAL implementation over a mock/stand-in in tests.** When the genuine collaborator is available in the repo, wire it up instead of hand-rolling a fake — a test that registers an inline `defineTool({ name: 'bash', … })` to stand in for `dsh-tool-bash` proves the *bridge* moves bytes but not that the *shipping tool* renders the way the test asserts; the two drift and the test passes while the product is wrong. Mock only the genuinely expensive/non-deterministic boundary (the LLM adapter, the network, the clock) and keep everything downstream real: a bridge tool-call test runs the scripted mock MODEL but the REAL tool + REAL executor (e.g. `makeBridgeHarness({ withBash: true })` plugs `dsh-bash-local` + `dsh-tool-bash` and runs an actual `echo`), so it verifies the actual `presentCall`/`presentResult` an editor sees. This is the unit-test echo of "verify the world, not a synthetic stand-in" (see § Defensive patterns) — a fake you wrote will agree with whatever you assumed; the real thing won't. ## Defensive patterns (hard-won) diff --git a/docs/rfc/README.md b/docs/rfc/README.md index 6a4ddb6f05..90fd5fe6d4 100644 --- a/docs/rfc/README.md +++ b/docs/rfc/README.md @@ -31,6 +31,7 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r | [Multiplex concurrent ACP sessions over one connection](proposed/2026-06-14-acp-multi-session.md) | 2026-06-14 | | [Optional Code Mode — model writes TypeScript against an SDK of all tools](proposed/2026-06-15-optional-code-mode.md) | 2026-06-15 | | [Runtime schemas for the event vocabulary (Zod vs the merge-extensible-map pattern)](proposed/2026-06-16-typed-event-schemas.md) | 2026-06-16 | +| [Rich ACP bash rendering — the terminal sub-protocol and command classification](proposed/2026-06-18-acp-terminal-and-tool-rendering.md) | 2026-06-18 | ## Implemented diff --git a/docs/rfc/proposed/2026-06-18-acp-terminal-and-tool-rendering.md b/docs/rfc/proposed/2026-06-18-acp-terminal-and-tool-rendering.md new file mode 100644 index 0000000000..e138609db0 --- /dev/null +++ b/docs/rfc/proposed/2026-06-18-acp-terminal-and-tool-rendering.md @@ -0,0 +1,49 @@ +# RFC: Rich ACP bash rendering — the terminal sub-protocol and command classification + +Status: proposed + +## Problem + +The ACP bridge now lets each tool own its call rendering via `presentCall`/`presentResult` (see [tool-call UI presentation](2026-06-14-acp-agent-client-protocol.md) and `packages/tools`). For `bash` we surface the model's `description` plus the command as the `tool_call` title, `kind: 'execute'`, and the completed output wrapped in a fenced ` ```console ` text block. + +That is a correct, capability-free MVP, but it is not how the reference editors render a *terminal* tool at its best. Two gaps: + +1. **No live terminal card.** An editor like Zed has a dedicated terminal tool-call card — a header showing the working directory, the command, a copy button, and **streaming** output with an exit-status pill — but it only uses that card when the `tool_call`'s `content` is an ACP `terminal` block (`{ type: 'terminal', terminalId }`), not a text block. With a text block the command output appears only as static markdown *after the turn completes*; there is no live stream and no cwd header. (Zed also HIDES `rawInput` for `kind: 'execute'`, which is why our command currently has to ride inside the title.) + +2. **No command classification.** A bash invocation is opaque — `bash -lc "sed -n 1,40p foo.ts"` is really a file read, `rg foo` is a search. The reference adapters classify common commands and present them with a *semantic* kind/title/locations (a `read` card titled "Read file 'foo.ts'" with a follow-along file location, a `search` card), falling back to a terminal card only for an unrecognized command. This is what makes one bash call render with a search icon and "List …" while the next renders as a raw terminal. + +## What the reference adapters do (studied 2026-06-18) + +- **`codex-acp`** (`CodexToolCallMapper.ts`): classifies each command into `commandActions`. A recognized action maps to a semantic update — `read` → `{kind:'read', title:"Read file '…'", locations:[{path}]}`, `search` → `{kind:'search', title:"Search for '…' in …"}`, `listFiles` → `{kind:'read', title:"List files in '…'"}`. An `unknown` action becomes a terminal card: `{kind:'execute', title: stripShellPrefix(command), content:[{type:'terminal', terminalId}], _meta:{terminal_info:{cwd, terminal_id}}}`. The `_meta.terminal_info.cwd` is what renders the working directory as the card header. +- **`claude-agent-acp`** (`tools.ts`): gates on `clientCapabilities._meta.terminal_output`. WITH it: a terminal content block plus `_meta.terminal_{info,output,exit}` (output + exit code). WITHOUT it: the same fenced ` ```console ` text-block fallback this bridge ships today. Title is the command; the model's `description` (when present) is shown as content. +- **Zed** (`crates/agent_ui/.../thread_view.rs`, `crates/acp_thread/.../acp_thread.rs`): `render_terminal_tool_call` reads the terminal's `working_dir` as the header and `tool_call.label` (the title) as the command; a non-terminal text `content` block renders via `render_markdown_output`. `should_show_raw_input = !is_terminal_tool` confirms `rawInput` is suppressed for execute-kind cards. + +The full terminal experience is an ACP **sub-protocol**, not just a content shape: the client advertises a terminal capability, and the agent drives `terminal/create` → streams via `terminal/output` → `terminal/release`, attaching the `terminalId` to the `tool_call` content. That is a cross-seam feature (bridge ⇄ `dsh-bash` executor ⇄ client), which is why it is deferred to this RFC rather than folded into the presentation-seam PR. + +## Proposal + +Two independent, separately shippable pieces. Both build on the existing tool-owned presentation seam — neither reintroduces tool-name special-casing in the bridge. + +### A. Terminal content type + cwd metadata (capability-gated) + +1. In `initialize`, read the client's terminal capability (`clientCapabilities.terminal` / the `_meta.terminal_output` convention the references use) and remember it per connection. +2. Extend the `dsh-tools` presentation vocabulary so a tool can ask for a terminal rendering — e.g. a `ToolResultPresentation`/`ToolCallPresentation` variant carrying `{ kind: 'terminal', cwd, terminalId? }` (provider-neutral; the bridge maps it to the ACP `terminal` content block + `_meta.terminal_info`). `dsh-tool-bash` returns it for `bash` when a cwd is known. +3. When the client supports it, the bridge maps that to `content:[{type:'terminal', terminalId}]` + `_meta.terminal_info.{cwd,terminal_id}`; otherwise it keeps the current ` ```console ` text fallback. The fenced-text path stays the guaranteed baseline. +4. *(Stretch)* drive live streaming through the real `terminal/*` methods so output appears as it is produced, with an exit-status pill — this needs a streaming seam on `dsh-bash` (the executor already has the process; it would push incremental output to the bridge). Scope this as a follow-up sub-step; steps 1–3 already give the cwd-header card with output attached at completion. + +### B. Command classification (capability-free) + +A small, pure classifier (in `dsh-tool-bash`, since it owns the bash schema) maps a command string to an optional semantic presentation: detect common read/search/list shapes (`cat`/`sed -n`/`head`/`tail` → `read` + a `path` location; `grep`/`rg` → `search`; `ls` → list) and return the richer `ToolCallPresentation` (`kind`, a human title, `locations`). Anything unrecognized falls through to the current execute/terminal presentation. This needs a `locations?: ToolCallLocation[]`-style field on `ToolCallPresentation` (neutral `{ path, line? }`), which the bridge maps to ACP `tool_call.locations` to drive editor "follow-along". + +Classification is best-effort and explicitly fallible: a misparse must degrade to the plain terminal card, never mislabel destructively (e.g. never title a `rm` as a "read"). Keep the matcher conservative and unit-test each recognized shape plus the fallthrough. + +## Risks / trade-offs + +- **Terminal sub-protocol is cross-seam and stateful.** Live streaming couples the bridge, the `dsh-bash` executor, and the client's terminal lifecycle; getting disposal/cancel right (release the terminal on turn end, abort, and disconnect) is the hard part — it must honor the same quiescence rules as the rest of the bridge. Steps A1–A3 (static cwd header + output at completion) are low-risk; A4 (live streaming) is where the lifecycle complexity lives. +- **Capability detection must stay honest.** Advertise/emit terminal content only when the client opted in; the text fallback is the contract for everyone else, so it must never regress. +- **Classification can mislead.** A wrong guess is worse than no guess. Bias to the terminal fallback; treat the classifier as additive polish, not a correctness path. (Security note: classification is display-only — it must never change what actually executes.) +- **Provider-neutral vocabulary creep.** Adding `terminal`/`locations` to `ToolCallPresentation` widens the `dsh-tools` surface. Keep the additions neutral (no ACP types leak into `dsh-tools`) and only as rich as a second consumer would also want. + +## Out of scope / non-goals + +The MVP shipped in the tool-call-UI PR (description+command title, `kind:'execute'`, ` ```console ` output fallback) stays the baseline and the no-capability default. This RFC is purely additive polish on top of it. diff --git a/packages/acp/README.md b/packages/acp/README.md index 9818c7b13a..3cc54dff3b 100644 --- a/packages/acp/README.md +++ b/packages/acp/README.md @@ -42,10 +42,12 @@ Each session runs in its own workspace, recorded as the session's `SessionHeader ## Tool-call presentation -How a tool call renders in the editor is owned by the TOOL, not the bridge — the bridge never special-cases tool names. Each tool may declare `presentCall(args)` (pending state: a human-readable `title`, a `kind` for the icon, and the salient `rawInput` to show in a detail view) and `presentResult(args, result)` (completed state: an optional replacement `title` and reformatted `content`) on its `dsh-tools` definition. The bridge looks the definition up by name in `ctx.tools` and maps the neutral `ToolCallPresentation`/`ToolResultPresentation` to the ACP `tool_call`/`tool_call_update` wire shapes. A tool that declares neither gets a generic fallback (title = tool name, raw parsed args as `rawInput`, kind inferred from the name). For example `dsh-tool-bash` makes the model-written one-line `description` the title ("List files in the current directory"), the exact `command` the `rawInput`, `kind: 'execute'`, and wraps the completed output in a fenced ` ```console ` block. +How a tool call renders in the editor is owned by the TOOL, not the bridge — the bridge never special-cases tool names. Each tool may declare `presentCall(args)` (pending state: a human-readable `title`, a `kind` for the icon, and the salient `rawInput` to show in a detail view) and `presentResult(args, result)` (completed state: an optional replacement `title` and reformatted `content`) on its `dsh-tools` definition. The bridge looks the definition up by name in `ctx.tools` and maps the neutral `ToolCallPresentation`/`ToolResultPresentation` to the ACP `tool_call`/`tool_call_update` wire shapes. A tool that declares neither gets a generic fallback (title = tool name, raw parsed args as `rawInput`, kind inferred from the name). For example `dsh-tool-bash` sets the title to the model `description` + the exact `command` ("List files in src — ls -la src"), `kind: 'execute'`, the `command` as `rawInput`, and wraps the completed output in a fenced ` ```console ` block. (The command goes in the title because an editor hides `rawInput` for execute-kind cards — Zed renders it only for non-terminal tools.) The `tool/result` session event carries only `{ callId, content, isError }` — not the tool name or args — so to call a tool's `presentResult` the bridge keeps a small per-session map from `callId` to the in-flight call's `(name, args)`, populated on `tool/call` and removed as each result is presented (it holds only currently-in-flight calls, never finished ones). This is bridge-local state — NOT a change to the event schema or a core service. The map lives on the `SessionRecord`, so two concurrent sessions never cross their in-flight tool state; a `session/load` replay uses a throwaway presenter that pairs each `tool/call` with its `tool/result` as the log replays in order, so replayed tool cards render identically to live ones. +A richer rendering — the ACP **terminal** content type (a live cwd-header terminal card with streaming output) and command classification (a `cat` shown as a `read`, a `grep` as a `search`) — is a capability-gated follow-up; the ` ```console ` text block here is the guaranteed baseline for clients without the terminal capability. See [the terminal-rendering RFC](../../docs/rfc/proposed/2026-06-18-acp-terminal-and-tool-rendering.md). + ## Settle-exactly-once A `session/prompt` resolves (or rejects) exactly once, keyed off the canonical session log (the `session/event` stream), NOT the `agent/turn-start`/`agent/turn-end` events. One listener captures the prompt's owning turn from the log's `turn/start` and settles on the matching `turn/end` — the one signal that always fires (`closeTurn` appends it unconditionally, even when a boundary emit throws and the `agent/turn-end` EVENT is skipped). A prompt settles only on ITS OWN turn (`inflight.turn === turn/end.turn`), so a stale `turn/end` for a previously-cancelled turn whose end arrives late can never settle the wrong prompt. A turn that ends `error` REJECTS the RPC with an internal error carrying the failure message (ACP has no error stop reason); every other reason resolves via the codec. As a fallback, when the agent settles to `idle`/`disposed` with a prompt still pending — e.g. a `session/event` listener registered before the bridge threw and starved the bridge's listener — an `agent/status` handler reconciles the prompt from the log (the owning turn's `turn/end`, or `cancelled` if the turn was torn down without one). An empty/whitespace prompt is rejected up front — it would queue no work, so no turn would start and the RPC would hang. diff --git a/packages/acp/package.json b/packages/acp/package.json index 9cac888518..0a4a890658 100644 --- a/packages/acp/package.json +++ b/packages/acp/package.json @@ -35,11 +35,13 @@ "devDependencies": { "@deepseek-ai/dsh-agent": "workspace:^", "@deepseek-ai/dsh-agent-loop": "workspace:^", + "@deepseek-ai/dsh-bash-local": "workspace:^", "@deepseek-ai/dsh-llm": "workspace:^", "@deepseek-ai/dsh-session": "workspace:^", "@deepseek-ai/dsh-session-persistence": "workspace:^", "@deepseek-ai/dsh-session-persistence-jsonl": "workspace:^", "@deepseek-ai/dsh-system-prompt": "workspace:^", + "@deepseek-ai/dsh-tool-bash": "workspace:^", "@deepseek-ai/dsh-tools": "workspace:^", "cordis": "^4.0.0-rc.6" } diff --git a/packages/acp/src/index.ts b/packages/acp/src/index.ts index 283b8fc151..b418fad9f5 100644 --- a/packages/acp/src/index.ts +++ b/packages/acp/src/index.ts @@ -788,8 +788,15 @@ interface ResolvedResultPresentation { * both), the presenter remembers each `tool/call`'s `{ name, args }` keyed by * callId and looks it up on the matching result. The map is bridge-LOCAL (not a * change to the event schema or a core service): one presenter per live session - * (and a throwaway per `session/load` replay), entries removed as each result - * arrives, so it holds only the currently-in-flight calls. + * (and a throwaway per `session/load` replay), and each entry is removed when + * its result arrives. In the normal loop a `tool/call` is always followed by a + * `tool/result` (the registry turns even a thrown tool into an isError result), + * so the map holds only currently-in-flight calls. The one exception is a step + * torn down mid-tool (an abort between `tool/call` and `tool/result`), which can + * leave a single stale entry per such call; this is bounded by the session + * lifetime (the whole presenter is dropped on teardown) and never affects + * correctness — a later result for a different callId is unaffected, and the + * stale entry's only cost is one map slot until the session ends. */ export class ToolPresenter { private readonly pending = new Map() diff --git a/packages/acp/tests/harness.ts b/packages/acp/tests/harness.ts index 27335cfcff..4f6b5ac17a 100644 --- a/packages/acp/tests/harness.ts +++ b/packages/acp/tests/harness.ts @@ -18,6 +18,8 @@ import ToolRegistry from '@deepseek-ai/dsh-tools' import AgentRegistry from '@deepseek-ai/dsh-agent' import AgentLoop from '@deepseek-ai/dsh-agent-loop' import SessionPersistenceJsonl from '@deepseek-ai/dsh-session-persistence-jsonl' +import { LocalBashExecutor } from '@deepseek-ai/dsh-bash-local' +import * as ToolBash from '@deepseek-ai/dsh-tool-bash' import { ClientSideConnection, ndJsonStream, @@ -148,6 +150,14 @@ export async function makeBridgeHarness(options: { script?: (StreamChunk[] | 'hang')[] config?: Partial storageDir: string + /** + * Plug the REAL `dsh-bash-local` executor + `dsh-tool-bash` tools (instead of + * a test's own inline tool). Lets a test drive the actual `bash` tool — its + * real `presentCall`/`presentResult` — through the bridge, so tool-call UI + * tests verify the SHIPPING tool, not a stand-in (AGENTS.md "prefer the real + * implementation over a mock in tests"). + */ + withBash?: boolean } = { storageDir: '' }): Promise { const adapter = new MockAdapter(options.script ?? []) @@ -159,6 +169,10 @@ export async function makeBridgeHarness(options: { await ctx.plugin(AgentRegistry) await ctx.plugin(AgentLoop, { agents: [] }) await ctx.plugin(SessionPersistenceJsonl, { root: options.storageDir }) + if (options.withBash) { + await ctx.plugin(LocalBashExecutor, { timeoutMs: 10_000 }) + await ctx.plugin(ToolBash) + } ctx.llm.registerAdapter(['mock'], adapter) // Two identity byte pipes cross-wired into the two ndJsonStreams: bytes the diff --git a/packages/acp/tests/load.spec.ts b/packages/acp/tests/load.spec.ts index 3ffef1a87e..89bd8253f7 100644 --- a/packages/acp/tests/load.spec.ts +++ b/packages/acp/tests/load.spec.ts @@ -4,7 +4,6 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { PROTOCOL_VERSION } from '@agentclientprotocol/sdk' import { SessionId } from '@deepseek-ai/dsh-session' -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. */ @@ -58,64 +57,37 @@ describe('acp bridge — session/load replay', () => { }) 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. + // A turn with a REAL bash tool call is persisted, then loaded by a fresh + // bridge. The replayed tool_call/tool_call_update must carry the tool's OWN + // presentation — identical to how it streamed live — via a throwaway + // presenter that pairs call→result as the log replays in order. Uses the + // shipping tool (withBash), not a stand-in (AGENTS.md "prefer the real + // implementation over a mock in tests"). live = await makeBridgeHarness({ storageDir, - script: [toolCallResponse('c1', 'bash', { command: 'ls -la', description: 'List files' }), textResponse('done')], + withBash: true, + script: [toolCallResponse('c1', 'bash', { command: 'echo hello', description: 'Print a greeting' }), 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.client.prompt({ sessionId, prompt: [{ type: 'text', text: 'greet' }] }) 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\`\`\`` }] } - }, - })) + // A fresh bridge — also with the real bash tool, since the presentation is + // resolved from the live registry at replay time — loads the session. + loader = await makeBridgeHarness({ storageDir, withBash: true, script: [] }) 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' }) + expect(call).toMatchObject({ toolCallId: 'c1', title: 'Print a greeting — echo hello', kind: 'execute', rawInput: 'echo hello' }) 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```' } }], - }) + expect(update?.sessionUpdate).toBe('tool_call_update') + if (update?.sessionUpdate !== 'tool_call_update') throw new Error('expected a tool_call_update') + expect(update).toMatchObject({ toolCallId: 'c1', status: 'completed' }) + const content = update.content as { content: { text: string } }[] + expect(content[0]?.content.text).toBe('```console\nhello\n```') }) it('a load whose resume finishes after a client disconnect leaks no live session', async () => { diff --git a/packages/acp/tests/turns.spec.ts b/packages/acp/tests/turns.spec.ts index 101f6849fe..7b37233585 100644 --- a/packages/acp/tests/turns.spec.ts +++ b/packages/acp/tests/turns.spec.ts @@ -75,40 +75,41 @@ describe('acp bridge — turn outcomes', () => { expect(callIdx).toBeLessThan(updIdx) }) - it('a tool-owned presentation flows end-to-end: presentCall sets title/rawInput, presentResult reformats output', async () => { + it('the REAL bash tool drives the tool-call UI end-to-end: description—command title + console output', async () => { + // Use the SHIPPING tool (dsh-tool-bash + dsh-bash-local), not an inline + // stand-in, so this verifies the actual presentCall/presentResult the editor + // sees (AGENTS.md "prefer the real implementation over a mock in tests"). + // The mock MODEL still scripts the tool call (no real LLM needed), but the + // tool and executor are real: a real `echo` runs and its real output flows + // back through the bridge. harness = await makeBridgeHarness({ storageDir, - script: [toolCallResponse('c1', 'bash', { command: 'ls -la', description: 'List files' }), textResponse('done')], + withBash: true, + script: [ + toolCallResponse('c1', 'bash', { command: 'echo hello', description: 'Print a greeting' }), + textResponse('done'), + ], }) - // A tool that declares its OWN presentation (like the real tool-bash). The - // bridge must use it — NOT the generic title=name fallback — proving the - // tool-owns-its-rendering seam works through the real session-event path. - harness.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\`\`\`` }] } - }, - })) const sessionId = await newSession(harness) - await harness.client.prompt({ sessionId, prompt: [{ type: 'text', text: 'list' }] }) + await harness.client.prompt({ sessionId, prompt: [{ type: 'text', text: 'greet' }] }) + // presentCall: execute kind, title is "description — command" (an execute + // card hides rawInput, so the command rides in the title), command in rawInput. const call = harness.updates.find(u => u.sessionUpdate === 'tool_call') - expect(call).toMatchObject({ toolCallId: 'c1', title: 'List files', kind: 'execute', rawInput: 'ls -la', status: 'in_progress' }) - const update = harness.updates.find(u => u.sessionUpdate === 'tool_call_update') - expect(update).toMatchObject({ + expect(call).toMatchObject({ toolCallId: 'c1', - status: 'completed', - content: [{ type: 'content', content: { type: 'text', text: '```console\na.txt\nb.txt\n```' } }], + title: 'Print a greeting — echo hello', + kind: 'execute', + rawInput: 'echo hello', + status: 'in_progress', }) + // presentResult: the REAL command output, wrapped in a fenced console block. + const update = harness.updates.find(u => u.sessionUpdate === 'tool_call_update') + expect(update?.sessionUpdate).toBe('tool_call_update') + if (update?.sessionUpdate !== 'tool_call_update') throw new Error('expected a tool_call_update') + expect(update).toMatchObject({ toolCallId: 'c1', status: 'completed' }) + const content = update.content as { content: { type: string; text: string } }[] + expect(content[0]?.content.text).toBe('```console\nhello\n```') }) it('a throwing tool presenter does not break the turn: the bridge falls back generically', async () => { diff --git a/packages/tool-bash/README.md b/packages/tool-bash/README.md index 473f5a4099..194c94c1ca 100644 --- a/packages/tool-bash/README.md +++ b/packages/tool-bash/README.md @@ -34,7 +34,7 @@ The owning agent is recorded per task id at spawn and kept for the lifetime of t ## UI presentation -These tools own how their calls render in a UI (an editor's tool-call card) via the `dsh-tools` `presentCall`/`presentResult` seam — a UI never special-cases tool names. For `bash`: the model-written `description` is the always-visible **title** (e.g. "List files in the current directory"), the exact `command` is the **rawInput** (the verbatim command stays visible in a detail view without crowding the title), `kind` is `execute` (terminal/run treatment), and the completed output is wrapped in a fenced ` ```console ` block — a UI-only affordance, so the model-facing result text stays unfenced. `bash_output`/`bash_kill` present a task-scoped title ("Read output from background task bash-3" / "Kill background task bash-3") with the task id as rawInput. These methods are pure/display-only (they also run on `session/load` replay), and a malformed/older logged arg shape falls back to a generic presentation rather than throwing. See `packages/tools` ("Tool-owned UI presentation") and `packages/acp` ("Tool-call presentation"). +These tools own how their calls render in a UI (an editor's tool-call card) via the `dsh-tools` `presentCall`/`presentResult` seam — a UI never special-cases tool names. For `bash`: the **title** is the model-written `description` followed by the exact `command` ("List files in src — ls -la src"), `kind` is `execute` (terminal/run treatment), and the `command` is ALSO the **rawInput**. Why both in the title: an execute-kind card hides `rawInput` (Zed renders it only for non-terminal tools), so the command must ride in the always-visible title to be seen — the reference ACP adapters (claude-agent-acp, codex-acp) likewise put the command in an execute tool's title. The completed output is wrapped in a fenced ` ```console ` block — a UI-only affordance, so the model-facing result text stays unfenced. `bash_output`/`bash_kill` present a task-scoped title ("Read output from background task bash-3" / "Kill background task bash-3") with the task id as rawInput. These methods are pure/display-only (they also run on `session/load` replay), and a malformed/older logged arg shape falls back to a generic presentation rather than throwing. See `packages/tools` ("Tool-owned UI presentation") and `packages/acp` ("Tool-call presentation"). ## Background completion notices diff --git a/packages/tool-bash/src/index.ts b/packages/tool-bash/src/index.ts index 9b4aa0819b..660c4ecfea 100644 --- a/packages/tool-bash/src/index.ts +++ b/packages/tool-bash/src/index.ts @@ -133,15 +133,18 @@ export function renderResult(result: BashRunResult): string { // --------------------------------------------------------------------------- /** - * Pending-state presentation for a `bash` call: the model-written `description` - * is the always-visible title (the schema requires it precisely so a UI has a - * readable summary — "List files in the current directory"), `kind: 'execute'` - * (a terminal/run treatment), and the exact `command` is the `rawInput` so the - * verbatim command stays visible in a UI's detail view without crowding the - * title. Mirrors how Zed / the reference ACP adapters render execute tools. + * Pending-state presentation for a `bash` call. The title is the model-written + * `description` followed by the exact `command` ("List files — ls -la src"): + * `kind: 'execute'` gets a terminal/run treatment in a UI, but an execute-kind + * card HIDES `rawInput` (Zed: `should_show_raw_input = !is_terminal_tool`), so + * the command MUST ride in the always-visible title to be seen — the reference + * ACP adapters (claude-agent-acp, codex-acp) likewise put the command in the + * title for execute tools. The description leads (a readable summary the schema + * requires); the command follows so the verbatim text is still there. `rawInput` + * still carries the bare command for non-execute UIs that DO render it. */ function presentBashCall(args: { command: string; description: string }): ToolCallPresentation { - return { title: args.description, kind: 'execute', rawInput: args.command } + return { title: `${args.description} — ${args.command}`, kind: 'execute', rawInput: args.command } } /** diff --git a/packages/tool-bash/tests/tools.spec.ts b/packages/tool-bash/tests/tools.spec.ts index 8b68de213b..c1b1e18808 100644 --- a/packages/tool-bash/tests/tools.spec.ts +++ b/packages/tool-bash/tests/tools.spec.ts @@ -564,10 +564,10 @@ describe('status lines', () => { }) describe('tool-owned UI presentation (presentCall / presentResult)', () => { - it('bash presentCall: the model description is the title, the command is the rawInput, kind execute', async () => { + it('bash presentCall: title is "description — command" (execute cards hide rawInput), command also in rawInput', async () => { const ctx = await setup() - const present = ctx.tools.get('bash')!.presentCall!({ command: 'ls -la src', description: 'List files in src' }) - expect(present).toEqual({ title: 'List files in src', kind: 'execute', rawInput: 'ls -la src' }) + const present = ctx.tools.get('bash')?.presentCall?.({ command: 'ls -la src', description: 'List files in src' }) + expect(present).toEqual({ title: 'List files in src — ls -la src', kind: 'execute', rawInput: 'ls -la src' }) }) it('bash presentResult: wraps the model-facing text in a fenced console block', async () => { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 9c80db6c8f..9c534153ab 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -84,6 +84,9 @@ importers: '@deepseek-ai/dsh-agent-loop': specifier: workspace:^ version: link:../agent-loop + '@deepseek-ai/dsh-bash-local': + specifier: workspace:^ + version: link:../bash-local '@deepseek-ai/dsh-llm': specifier: workspace:^ version: link:../llm @@ -99,6 +102,9 @@ importers: '@deepseek-ai/dsh-system-prompt': specifier: workspace:^ version: link:../system-prompt + '@deepseek-ai/dsh-tool-bash': + specifier: workspace:^ + version: link:../tool-bash '@deepseek-ai/dsh-tools': specifier: workspace:^ version: link:../tools From 386ee14af356bcfe51cedd0b2f9d9f50eded9339 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Thu, 18 Jun 2026 14:36:02 +0800 Subject: [PATCH 04/10] docs(acp): RFC for the terminal-card rendering (implemented design) Records the verified design before implementing: keep dsh-bash agent-side execution and render Zed's terminal tool-call card via the `_meta` convention (terminal_info/terminal_output/terminal_exit), capability-gated on clientCapabilities._meta.terminal_output, with the ```console text block as the no-capability fallback. Rejects the spec's client-side terminal/create path (it would bypass dsh-bash's sandbox/env-scrub/ownership/cwd). Studied claude-agent-acp, codex-acp, and Zed's renderer to ground the wire contract. Live streaming and command classification are noted as separate follow-ups. --- docs/rfc/README.md | 2 +- ...6-06-18-acp-terminal-and-tool-rendering.md | 40 +++++++++++++++ ...6-06-18-acp-terminal-and-tool-rendering.md | 49 ------------------- packages/acp/README.md | 2 +- 4 files changed, 42 insertions(+), 51 deletions(-) create mode 100644 docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md delete mode 100644 docs/rfc/proposed/2026-06-18-acp-terminal-and-tool-rendering.md diff --git a/docs/rfc/README.md b/docs/rfc/README.md index 90fd5fe6d4..bc56275477 100644 --- a/docs/rfc/README.md +++ b/docs/rfc/README.md @@ -31,7 +31,6 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r | [Multiplex concurrent ACP sessions over one connection](proposed/2026-06-14-acp-multi-session.md) | 2026-06-14 | | [Optional Code Mode — model writes TypeScript against an SDK of all tools](proposed/2026-06-15-optional-code-mode.md) | 2026-06-15 | | [Runtime schemas for the event vocabulary (Zod vs the merge-extensible-map pattern)](proposed/2026-06-16-typed-event-schemas.md) | 2026-06-16 | -| [Rich ACP bash rendering — the terminal sub-protocol and command classification](proposed/2026-06-18-acp-terminal-and-tool-rendering.md) | 2026-06-18 | ## Implemented @@ -56,6 +55,7 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r | [Session persistence as an abstract service over `SessionEvent`](implemented/2026-06-14-session-persistence.md) | 2026-06-14 | | [Every session event is enclosed in a turn](implemented/2026-06-15-turn-enclosure-invariant.md) | 2026-06-15 | | [pnpm as the package manager instead of Yarn 4](implemented/2026-06-16-pnpm-over-yarn.md) | 2026-06-16 | +| [Rich ACP bash rendering — the terminal card (`_meta`) and command classification](implemented/2026-06-18-acp-terminal-and-tool-rendering.md) | 2026-06-18 | ## Rejected diff --git a/docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md b/docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md new file mode 100644 index 0000000000..b1588461f0 --- /dev/null +++ b/docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md @@ -0,0 +1,40 @@ +# RFC: Rich ACP bash rendering — the terminal card via the `_meta` convention + +Status: implemented + +## Problem + +The ACP bridge lets each tool own its call rendering via `presentCall`/`presentResult` (see [tool-call UI presentation](../proposed/2026-06-14-acp-agent-client-protocol.md) and `packages/tools`). For `bash` we surface the model's `description` plus the command as the `tool_call` title, `kind: 'execute'`, and the completed output wrapped in a fenced ` ```console ` text block. + +That is a correct, capability-free baseline, but not how the reference editors render a *terminal* tool at its best. An editor like Zed has a dedicated terminal tool-call card — a header showing the working directory, the command as the label, and the command output rendered as a terminal — but it only builds that card when the `tool_call` carries terminal metadata (below). With a plain text block the output appears as static markdown and there is no cwd header. (Zed also HIDES `rawInput` for `kind: 'execute'`, which is why our command rides inside the title.) + +## Key finding: agent-executed terminals use a `_meta` convention, NOT `terminal/create` + +The ACP spec has a *client-side* terminal sub-protocol — the agent calls the client's `terminal/create` with `{ command, args, cwd, env }` and the **editor** executes the process, then the agent reads `terminal/output` / `wait_for_exit`. That model is wrong for us: our harness executes bash itself through `dsh-bash` (sandboxed env-scrub, background-task ownership, per-session cwd). Routing execution to the editor would bypass all of that and fork execution into two backends. + +Studying the two reference agents (2026-06-18) shows neither uses `terminal/create` for their own shell tool — **both keep agent-side execution and emit a `_meta` convention** that Zed special-cases: + +- **`claude-agent-acp`** (`tools.ts`, `acp-agent.ts`): gated on `clientCapabilities._meta.terminal_output`. The `tool_call` carries `content: [{ type: 'terminal', terminalId }]` and `_meta.terminal_info.{ terminal_id, cwd }`; output/exit arrive on the `tool_call_update`'s `_meta.terminal_output.{ terminal_id, data }` and `_meta.terminal_exit.{ terminal_id, exit_code, signal }`. +- **`codex-acp`** (`CodexToolCallMapper.ts`, `TerminalOutputMode.ts`): same `terminal_info` on the call; output via `_meta.terminal_output` (full) or `_meta.terminal_output_delta` (incremental), selected from the same `_meta.terminal_output` capability. + +Zed's side (`crates/agent_servers/src/acp.rs`, verified): on a `ToolCall` whose `_meta.terminal_info.terminal_id` is set, it registers a **display-only** terminal (header = `terminal_info.cwd`, label = `tool_call.title`); on a `ToolCallUpdate`, `_meta.terminal_output.data` writes to that terminal and `_meta.terminal_exit.{exit_code,signal}` sets the status. It advertises the capability as `clientCapabilities._meta.terminal_output = true`. This is an off-spec `_meta` extension, but it is the de-facto contract for the Zed integration and the only way to get the terminal card while keeping execution agent-side. + +## Decision + +Keep `dsh-bash` agent-side execution; render the terminal card via the `_meta` convention, capability-gated, with the ` ```console ` text block as the guaranteed fallback. + +1. **Capability.** `initialize` reads `clientCapabilities._meta.terminal_output` and the bridge remembers it per connection. +2. **Neutral presentation vocabulary.** `dsh-tools` gains a terminal-shaped presentation a tool can return — provider-neutral (`cwd`, the output `data`, an `exitCode`/`signal`), NO ACP types. `dsh-tool-bash` returns it for `bash` (cwd from the resolved workdir; output + exit parsed from the run result). +3. **Bridge mapping.** When the client advertised the capability, the bridge maps that presentation to: on `tool_call`, `content:[{type:'terminal', terminalId}]` + `_meta.terminal_info.{terminal_id,cwd}`; on `tool_call_update`, `_meta.terminal_output.{terminal_id,data}` + `_meta.terminal_exit.{terminal_id,exit_code,signal}`. `terminalId` is derived from the harness `callId` (stable, unique per call). When the capability is absent, the bridge uses the existing ` ```console ` text content — unchanged. +4. **No new execution path, no live streaming.** Output is attached at completion (from the agent's own `tool/result`), not streamed token-by-token. Disposal is unaffected: nothing new to tear down, since the bridge never creates a client-side terminal. + +## Risks / trade-offs + +- **Off-spec `_meta`.** The terminal card rides on a Zed-specific `_meta` extension, not the ACP terminal sub-protocol. A client that doesn't recognize it still gets the text fallback (the capability gate ensures we only emit it when the client opted in via `_meta.terminal_output`), so a non-Zed client is never worse off. If ACP later standardizes agent-executed terminals, migrate to that and drop the `_meta`. +- **Capability honesty.** Emit terminal metadata ONLY when the client advertised `_meta.terminal_output`; the text fallback is the contract for everyone else and must never regress. Covered by a no-capability test asserting the ` ```console ` path. +- **terminalId collisions.** Deriving it from the per-call `callId` keeps it unique within a session and stable across the call/result pair; never reuse one across calls. +- **Provider-neutral vocabulary creep.** The terminal presentation widens the `dsh-tools` surface; keep it neutral (no ACP types leak into `dsh-tools`) and only as rich as a second UI consumer would also want. + +## Out of scope / non-goals + +The text-block baseline stays the no-capability default. Client-side `terminal/create` execution is explicitly rejected (it bypasses `dsh-bash`). Two follow-ups are deliberately NOT built here and would each warrant their own RFC when someone takes them on: **live incremental streaming** (`_meta.terminal_output_delta` as chunks arrive, which needs an incremental-output seam on `dsh-bash`), and **command classification** (parsing a `cat`/`sed` as a `read` card with a file location, a `grep` as a `search`, etc., falling back to the terminal card — display-only, must never change what executes). diff --git a/docs/rfc/proposed/2026-06-18-acp-terminal-and-tool-rendering.md b/docs/rfc/proposed/2026-06-18-acp-terminal-and-tool-rendering.md deleted file mode 100644 index e138609db0..0000000000 --- a/docs/rfc/proposed/2026-06-18-acp-terminal-and-tool-rendering.md +++ /dev/null @@ -1,49 +0,0 @@ -# RFC: Rich ACP bash rendering — the terminal sub-protocol and command classification - -Status: proposed - -## Problem - -The ACP bridge now lets each tool own its call rendering via `presentCall`/`presentResult` (see [tool-call UI presentation](2026-06-14-acp-agent-client-protocol.md) and `packages/tools`). For `bash` we surface the model's `description` plus the command as the `tool_call` title, `kind: 'execute'`, and the completed output wrapped in a fenced ` ```console ` text block. - -That is a correct, capability-free MVP, but it is not how the reference editors render a *terminal* tool at its best. Two gaps: - -1. **No live terminal card.** An editor like Zed has a dedicated terminal tool-call card — a header showing the working directory, the command, a copy button, and **streaming** output with an exit-status pill — but it only uses that card when the `tool_call`'s `content` is an ACP `terminal` block (`{ type: 'terminal', terminalId }`), not a text block. With a text block the command output appears only as static markdown *after the turn completes*; there is no live stream and no cwd header. (Zed also HIDES `rawInput` for `kind: 'execute'`, which is why our command currently has to ride inside the title.) - -2. **No command classification.** A bash invocation is opaque — `bash -lc "sed -n 1,40p foo.ts"` is really a file read, `rg foo` is a search. The reference adapters classify common commands and present them with a *semantic* kind/title/locations (a `read` card titled "Read file 'foo.ts'" with a follow-along file location, a `search` card), falling back to a terminal card only for an unrecognized command. This is what makes one bash call render with a search icon and "List …" while the next renders as a raw terminal. - -## What the reference adapters do (studied 2026-06-18) - -- **`codex-acp`** (`CodexToolCallMapper.ts`): classifies each command into `commandActions`. A recognized action maps to a semantic update — `read` → `{kind:'read', title:"Read file '…'", locations:[{path}]}`, `search` → `{kind:'search', title:"Search for '…' in …"}`, `listFiles` → `{kind:'read', title:"List files in '…'"}`. An `unknown` action becomes a terminal card: `{kind:'execute', title: stripShellPrefix(command), content:[{type:'terminal', terminalId}], _meta:{terminal_info:{cwd, terminal_id}}}`. The `_meta.terminal_info.cwd` is what renders the working directory as the card header. -- **`claude-agent-acp`** (`tools.ts`): gates on `clientCapabilities._meta.terminal_output`. WITH it: a terminal content block plus `_meta.terminal_{info,output,exit}` (output + exit code). WITHOUT it: the same fenced ` ```console ` text-block fallback this bridge ships today. Title is the command; the model's `description` (when present) is shown as content. -- **Zed** (`crates/agent_ui/.../thread_view.rs`, `crates/acp_thread/.../acp_thread.rs`): `render_terminal_tool_call` reads the terminal's `working_dir` as the header and `tool_call.label` (the title) as the command; a non-terminal text `content` block renders via `render_markdown_output`. `should_show_raw_input = !is_terminal_tool` confirms `rawInput` is suppressed for execute-kind cards. - -The full terminal experience is an ACP **sub-protocol**, not just a content shape: the client advertises a terminal capability, and the agent drives `terminal/create` → streams via `terminal/output` → `terminal/release`, attaching the `terminalId` to the `tool_call` content. That is a cross-seam feature (bridge ⇄ `dsh-bash` executor ⇄ client), which is why it is deferred to this RFC rather than folded into the presentation-seam PR. - -## Proposal - -Two independent, separately shippable pieces. Both build on the existing tool-owned presentation seam — neither reintroduces tool-name special-casing in the bridge. - -### A. Terminal content type + cwd metadata (capability-gated) - -1. In `initialize`, read the client's terminal capability (`clientCapabilities.terminal` / the `_meta.terminal_output` convention the references use) and remember it per connection. -2. Extend the `dsh-tools` presentation vocabulary so a tool can ask for a terminal rendering — e.g. a `ToolResultPresentation`/`ToolCallPresentation` variant carrying `{ kind: 'terminal', cwd, terminalId? }` (provider-neutral; the bridge maps it to the ACP `terminal` content block + `_meta.terminal_info`). `dsh-tool-bash` returns it for `bash` when a cwd is known. -3. When the client supports it, the bridge maps that to `content:[{type:'terminal', terminalId}]` + `_meta.terminal_info.{cwd,terminal_id}`; otherwise it keeps the current ` ```console ` text fallback. The fenced-text path stays the guaranteed baseline. -4. *(Stretch)* drive live streaming through the real `terminal/*` methods so output appears as it is produced, with an exit-status pill — this needs a streaming seam on `dsh-bash` (the executor already has the process; it would push incremental output to the bridge). Scope this as a follow-up sub-step; steps 1–3 already give the cwd-header card with output attached at completion. - -### B. Command classification (capability-free) - -A small, pure classifier (in `dsh-tool-bash`, since it owns the bash schema) maps a command string to an optional semantic presentation: detect common read/search/list shapes (`cat`/`sed -n`/`head`/`tail` → `read` + a `path` location; `grep`/`rg` → `search`; `ls` → list) and return the richer `ToolCallPresentation` (`kind`, a human title, `locations`). Anything unrecognized falls through to the current execute/terminal presentation. This needs a `locations?: ToolCallLocation[]`-style field on `ToolCallPresentation` (neutral `{ path, line? }`), which the bridge maps to ACP `tool_call.locations` to drive editor "follow-along". - -Classification is best-effort and explicitly fallible: a misparse must degrade to the plain terminal card, never mislabel destructively (e.g. never title a `rm` as a "read"). Keep the matcher conservative and unit-test each recognized shape plus the fallthrough. - -## Risks / trade-offs - -- **Terminal sub-protocol is cross-seam and stateful.** Live streaming couples the bridge, the `dsh-bash` executor, and the client's terminal lifecycle; getting disposal/cancel right (release the terminal on turn end, abort, and disconnect) is the hard part — it must honor the same quiescence rules as the rest of the bridge. Steps A1–A3 (static cwd header + output at completion) are low-risk; A4 (live streaming) is where the lifecycle complexity lives. -- **Capability detection must stay honest.** Advertise/emit terminal content only when the client opted in; the text fallback is the contract for everyone else, so it must never regress. -- **Classification can mislead.** A wrong guess is worse than no guess. Bias to the terminal fallback; treat the classifier as additive polish, not a correctness path. (Security note: classification is display-only — it must never change what actually executes.) -- **Provider-neutral vocabulary creep.** Adding `terminal`/`locations` to `ToolCallPresentation` widens the `dsh-tools` surface. Keep the additions neutral (no ACP types leak into `dsh-tools`) and only as rich as a second consumer would also want. - -## Out of scope / non-goals - -The MVP shipped in the tool-call-UI PR (description+command title, `kind:'execute'`, ` ```console ` output fallback) stays the baseline and the no-capability default. This RFC is purely additive polish on top of it. diff --git a/packages/acp/README.md b/packages/acp/README.md index 3cc54dff3b..c86be26994 100644 --- a/packages/acp/README.md +++ b/packages/acp/README.md @@ -46,7 +46,7 @@ How a tool call renders in the editor is owned by the TOOL, not the bridge — t The `tool/result` session event carries only `{ callId, content, isError }` — not the tool name or args — so to call a tool's `presentResult` the bridge keeps a small per-session map from `callId` to the in-flight call's `(name, args)`, populated on `tool/call` and removed as each result is presented (it holds only currently-in-flight calls, never finished ones). This is bridge-local state — NOT a change to the event schema or a core service. The map lives on the `SessionRecord`, so two concurrent sessions never cross their in-flight tool state; a `session/load` replay uses a throwaway presenter that pairs each `tool/call` with its `tool/result` as the log replays in order, so replayed tool cards render identically to live ones. -A richer rendering — the ACP **terminal** content type (a live cwd-header terminal card with streaming output) and command classification (a `cat` shown as a `read`, a `grep` as a `search`) — is a capability-gated follow-up; the ` ```console ` text block here is the guaranteed baseline for clients without the terminal capability. See [the terminal-rendering RFC](../../docs/rfc/proposed/2026-06-18-acp-terminal-and-tool-rendering.md). +A richer rendering — the ACP **terminal** content type (a live cwd-header terminal card with streaming output) and command classification (a `cat` shown as a `read`, a `grep` as a `search`) — is a capability-gated follow-up; the ` ```console ` text block here is the guaranteed baseline for clients without the terminal capability. See [the terminal-rendering RFC](../../docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md). ## Settle-exactly-once From 149ab1bba4c3eeee8d9970531c697c3ce9cae5b9 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Thu, 18 Jun 2026 17:25:09 +0800 Subject: [PATCH 05/10] feat(acp): render bash as a terminal card via the _meta convention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the client advertises clientCapabilities._meta.terminal_output (Zed), a bash tool call now renders as a real TERMINAL card — a cwd header + the command + its output — instead of the plain ```console text block. Keeps agent-side dsh-bash execution; rejects the spec's client-side terminal/create (which would bypass sandbox/env-scrub/ownership/cwd). Matches what claude-agent-acp and codex-acp do; wire contract verified against Zed's source. - dsh-tools: a provider-neutral ToolTerminal shape ({ cwd?, output? }) on ToolCallPresentation/ToolResultPresentation — a tool asks "render me as a terminal"; no ACP types leak in. - dsh-tool-bash: bash presentCall marks terminal (cwd from an explicit absolute workdir, else left for the bridge to fill from the session cwd); presentResult carries the output alongside the ```console fallback. - dsh-acp: initialize reads/remembers the _meta.terminal_output capability; streamSessionEventUpdate maps a terminal presentation to content:[{type:'terminal',terminalId}] + _meta.terminal_info on the call and _meta.terminal_output on the update WHEN capable — else the unchanged text path. terminalId is the callId; cwd defaults to the session header. The pure translator gained a TerminalRendering {enabled,cwd} param (off by default). Tests via the REAL tool-bash + bash-local: capability ON -> terminal content + _meta; OFF -> no _meta (text path). The with-key e2e adds a real-model terminal card case (echo over ACP with the capability on). 773 tests, 100% coverage. The exit-status pill (_meta.terminal_exit), live streaming (_meta.terminal_output_delta), and command classification are RFC follow-ups. --- ...6-06-18-acp-terminal-and-tool-rendering.md | 6 +- examples/acp-agent/tests/acp.e2e.ts | 31 ++++++++ packages/acp/README.md | 9 ++- packages/acp/src/index.ts | 74 ++++++++++++++++++- packages/acp/tests/turns.spec.ts | 37 +++++++++- packages/tool-bash/README.md | 2 +- packages/tool-bash/src/index.ts | 37 +++++++--- packages/tool-bash/tests/tools.spec.ts | 24 ++++-- packages/tools/README.md | 4 +- packages/tools/src/index.ts | 30 ++++++++ 10 files changed, 224 insertions(+), 30 deletions(-) diff --git a/docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md b/docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md index b1588461f0..7817b1e441 100644 --- a/docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md +++ b/docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md @@ -25,8 +25,8 @@ Keep `dsh-bash` agent-side execution; render the terminal card via the `_meta` c 1. **Capability.** `initialize` reads `clientCapabilities._meta.terminal_output` and the bridge remembers it per connection. 2. **Neutral presentation vocabulary.** `dsh-tools` gains a terminal-shaped presentation a tool can return — provider-neutral (`cwd`, the output `data`, an `exitCode`/`signal`), NO ACP types. `dsh-tool-bash` returns it for `bash` (cwd from the resolved workdir; output + exit parsed from the run result). -3. **Bridge mapping.** When the client advertised the capability, the bridge maps that presentation to: on `tool_call`, `content:[{type:'terminal', terminalId}]` + `_meta.terminal_info.{terminal_id,cwd}`; on `tool_call_update`, `_meta.terminal_output.{terminal_id,data}` + `_meta.terminal_exit.{terminal_id,exit_code,signal}`. `terminalId` is derived from the harness `callId` (stable, unique per call). When the capability is absent, the bridge uses the existing ` ```console ` text content — unchanged. -4. **No new execution path, no live streaming.** Output is attached at completion (from the agent's own `tool/result`), not streamed token-by-token. Disposal is unaffected: nothing new to tear down, since the bridge never creates a client-side terminal. +3. **Bridge mapping.** When the client advertised the capability, the bridge maps that presentation to: on `tool_call`, `content:[{type:'terminal', terminalId}]` + `_meta.terminal_info.{terminal_id,cwd}`; on `tool_call_update`, `_meta.terminal_output.{terminal_id,data}` (the captured output). `terminalId` is derived from the harness `callId` (stable, unique per call). When the capability is absent, the bridge uses the existing ` ```console ` text content — unchanged. +4. **No new execution path, no live streaming, no exit pill yet.** Output is attached at completion (from the agent's own `tool/result`), not streamed token-by-token. The exit-status pill (`_meta.terminal_exit.{exit_code,signal}`) is NOT emitted: it needs a structured exit code the pure `presentResult(args, result)` seam doesn't get (the result is content blocks), and the exit is already visible in the output text's `[exit code: N]` / `[killed by signal: …]` marker. Disposal is unaffected: nothing new to tear down, since the bridge never creates a client-side terminal. ## Risks / trade-offs @@ -37,4 +37,4 @@ Keep `dsh-bash` agent-side execution; render the terminal card via the `_meta` c ## Out of scope / non-goals -The text-block baseline stays the no-capability default. Client-side `terminal/create` execution is explicitly rejected (it bypasses `dsh-bash`). Two follow-ups are deliberately NOT built here and would each warrant their own RFC when someone takes them on: **live incremental streaming** (`_meta.terminal_output_delta` as chunks arrive, which needs an incremental-output seam on `dsh-bash`), and **command classification** (parsing a `cat`/`sed` as a `read` card with a file location, a `grep` as a `search`, etc., falling back to the terminal card — display-only, must never change what executes). +The text-block baseline stays the no-capability default. Client-side `terminal/create` execution is explicitly rejected (it bypasses `dsh-bash`). Three follow-ups are deliberately NOT built here and would each warrant their own RFC when someone takes them on: the **exit-status pill** (`_meta.terminal_exit.{exit_code,signal}`, which needs the structured exit surfaced from the run rather than parsed out of the rendered output text), **live incremental streaming** (`_meta.terminal_output_delta` as chunks arrive, which needs an incremental-output seam on `dsh-bash`), and **command classification** (parsing a `cat`/`sed` as a `read` card with a file location, a `grep` as a `search`, etc., falling back to the terminal card — display-only, must never change what executes). diff --git a/examples/acp-agent/tests/acp.e2e.ts b/examples/acp-agent/tests/acp.e2e.ts index caab2b0f4a..4a43429eb3 100644 --- a/examples/acp-agent/tests/acp.e2e.ts +++ b/examples/acp-agent/tests/acp.e2e.ts @@ -190,5 +190,36 @@ describe.skipIf(!process.env.DEEPSEEK_API_KEY)('acp-agent e2e: real prompt over expect(bashCall.title.length).toBeGreaterThan(0) expect(bashCall.title).not.toBe('bash') // the old, unhelpful title expect(typeof bashCall.rawInput).toBe('string') // the exact command + // Capability OFF: no terminal _meta — the ```console text path renders. + expect((bashCall as { _meta?: unknown })._meta).toBeUndefined() + }, 180_000) + + it('with the terminal_output capability, a real bash call renders as a terminal card (content + _meta)', async () => { + workdir = await mkdtemp(join(tmpdir(), 'acp-e2e-')) + spawned = spawnAcpAgent(workdir) + const { client, updates } = spawned + + // Advertise the Zed `_meta.terminal_output` capability so the bridge emits + // the terminal card for the real bash tool. + await client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: { _meta: { terminal_output: true } } }) + const { sessionId } = await client.newSession({ cwd: workdir, mcpServers: [] }) + const res = await client.prompt({ + sessionId, + prompt: [{ type: 'text', text: 'Use the bash tool to run: echo ACP_TERMINAL_OK. Then stop.' }], + }) + expect(['end_turn', 'max_tokens']).toContain(res.stopReason) + + // A bash tool_call now carries a terminal content block + _meta.terminal_info + // with the session cwd as the header; the matching update streams the output + // on _meta.terminal_output. + const bashCall = updates.find(u => u.sessionUpdate === 'tool_call' && u.kind === 'execute') + if (bashCall?.sessionUpdate !== 'tool_call') throw new Error('expected an execute tool_call') + const block = bashCall.content?.[0] as { type: string; terminalId?: string } | undefined + expect(block?.type).toBe('terminal') + expect(typeof block?.terminalId).toBe('string') + const info = (bashCall._meta as { terminal_info?: { terminal_id: string; cwd?: string } }).terminal_info + expect(info?.cwd).toBe(workdir) + const updatesForTerminal = updates.filter(u => u.sessionUpdate === 'tool_call_update' && (u._meta as { terminal_output?: unknown } | undefined)?.terminal_output !== undefined) + expect(updatesForTerminal.length).toBeGreaterThan(0) }, 180_000) }) diff --git a/packages/acp/README.md b/packages/acp/README.md index c86be26994..3cb311a43e 100644 --- a/packages/acp/README.md +++ b/packages/acp/README.md @@ -46,7 +46,14 @@ How a tool call renders in the editor is owned by the TOOL, not the bridge — t The `tool/result` session event carries only `{ callId, content, isError }` — not the tool name or args — so to call a tool's `presentResult` the bridge keeps a small per-session map from `callId` to the in-flight call's `(name, args)`, populated on `tool/call` and removed as each result is presented (it holds only currently-in-flight calls, never finished ones). This is bridge-local state — NOT a change to the event schema or a core service. The map lives on the `SessionRecord`, so two concurrent sessions never cross their in-flight tool state; a `session/load` replay uses a throwaway presenter that pairs each `tool/call` with its `tool/result` as the log replays in order, so replayed tool cards render identically to live ones. -A richer rendering — the ACP **terminal** content type (a live cwd-header terminal card with streaming output) and command classification (a `cat` shown as a `read`, a `grep` as a `search`) — is a capability-gated follow-up; the ` ```console ` text block here is the guaranteed baseline for clients without the terminal capability. See [the terminal-rendering RFC](../../docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md). +## Terminal card (capability-gated) + +A tool whose call IS a shell command (`bash`) can render as a real **terminal card** — a working-directory header with the command's output — rather than a plain text block. The tool asks for this with the neutral `terminal` field on its presentation (`dsh-tools`: a `{ cwd?, output? }` shape on `ToolCallPresentation`/`ToolResultPresentation`); the bridge maps it to the Zed `_meta` convention, gated on the client advertising `clientCapabilities._meta.terminal_output` in `initialize`: + +- `tool_call`: `content:[{type:'terminal', terminalId}]` + `_meta.terminal_info.{terminal_id, cwd}` — the terminal id is the harness `callId`; the cwd is the tool's explicit `terminal.cwd` if it has one, else the session's workspace cwd (the bridge fills that, since the pure tool presenter can't see it). +- `tool_call_update`: `_meta.terminal_output.{terminal_id, data}` — the captured output, attached at completion. + +When the client does NOT advertise the capability, none of the `_meta`/terminal content is emitted and the ` ```console ` text block (above) is the rendering — so a non-Zed client is never worse off. This is an off-spec Zed `_meta` extension, not the ACP `terminal/create` sub-protocol: that would make the editor execute the command, bypassing `dsh-bash`'s sandbox/env-scrub/ownership/cwd. The exit-status pill, live streaming, and command classification are follow-ups. See [the terminal-rendering RFC](../../docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md). ## Settle-exactly-once diff --git a/packages/acp/src/index.ts b/packages/acp/src/index.ts index b418fad9f5..7d0624bc92 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, ToolCallPresentation, ToolRegistry, ToolResultPresentation } from '@deepseek-ai/dsh-tools' +import type { ToolCallKind, ToolCallPresentation, ToolRegistry, ToolResultPresentation, ToolTerminal } 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' @@ -212,6 +212,11 @@ export function apply(ctx: Context, config: AcpConfig): void { // await and NOT install a record (which would resurrect a live agent/listeners // after the bridge closed). Checked after every load await. let closed = false + // Whether the client advertised the Zed `_meta.terminal_output` capability in + // `initialize`. When true, a tool's terminal presentation is rendered as a + // terminal card (content + `_meta.terminal_*`); when false, the bridge uses + // the tool's text fallback. Set once in `initialize`, read on every tool event. + let terminalOutputCap = false // Assigned at the bottom, before any agent event can fire (a session only // exists after `newSession`, which the client calls after construction), so @@ -284,7 +289,10 @@ export function apply(ctx: Context, config: AcpConfig): void { ctx.on('session/event', (session, event: SessionEvent) => { const rec = sessions.get(session.header.id) if (rec === undefined) return - streamSessionEventUpdate(rec.sessionId, event, notify, rec.presenter) + streamSessionEventUpdate(rec.sessionId, event, notify, rec.presenter, { + enabled: terminalOutputCap, + cwd: session.header.cwd, + }) const inflight = rec.inflight if (inflight === undefined) return if (event.type === 'turn/start') { @@ -380,6 +388,11 @@ export function apply(ctx: Context, config: AcpConfig): void { // exactly PROTOCOL_VERSION; any other requested version negotiates // down to ours (the client disconnects if it can't speak it). const protocolVersion = params.protocolVersion === PROTOCOL_VERSION ? params.protocolVersion : PROTOCOL_VERSION + // Remember the Zed terminal-output `_meta` capability: when set, bash and + // other shell tools render as a terminal card (see streamSessionEventUpdate + // + the terminal-rendering RFC). `_meta` is `{[k]: unknown} | null`, so + // narrow defensively to a strict boolean true. + terminalOutputCap = params.clientCapabilities?._meta?.['terminal_output'] === true return Promise.resolve({ protocolVersion, agentInfo: { name: agentName, version: agentVersion }, @@ -478,8 +491,12 @@ export function apply(ctx: Context, config: AcpConfig): void { // 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() + const replayTerminal: TerminalRendering = { + enabled: terminalOutputCap, + cwd: agent.session.header.cwd, + } for (const event of agent.session.events) { - streamSessionEventUpdate(params.sessionId, event, notify, replayPresenter) + streamSessionEventUpdate(params.sessionId, event, notify, replayPresenter, replayTerminal) } return {} } finally { @@ -699,6 +716,7 @@ export function streamSessionEventUpdate( event: SessionEvent, notify: (notification: SessionNotification) => void, presenter: Pick = nullToolPresenter, + terminal: TerminalRendering = noTerminalRendering, ): void { switch (event.type) { case 'assistant/chunk': { @@ -724,6 +742,11 @@ export function streamSessionEventUpdate( } case 'tool/call': { const present = presenter.call(event.data.callId, event.data.name, event.data.arguments) + // A terminal-rendered call (a shell command) gets a terminal CARD when the + // client supports it: a `terminal` content block plus `_meta.terminal_info` + // (the cwd header). Otherwise it is an ordinary tool_call and the output + // arrives as text on the result. See the terminal-rendering RFC. + const asTerminal = present.terminal !== undefined && terminal.enabled notify({ sessionId, update: { @@ -733,12 +756,27 @@ export function streamSessionEventUpdate( kind: present.kind, status: 'in_progress', ...present.rawInput !== undefined ? { rawInput: present.rawInput } : {}, + ...asTerminal + ? { + content: [{ type: 'terminal', terminalId: event.data.callId }], + _meta: { terminal_info: { terminal_id: event.data.callId, cwd: present.terminal?.cwd ?? terminal.cwd } }, + } + : {}, }, }) return } case 'tool/result': { const present = presenter.result(event.data.callId, event.data.content, event.data.isError) + const term = present.terminal + // When the call rendered as a terminal AND the client is capable, stream + // the output on the update's `_meta.terminal_output` (the terminal card + // consumes it). The text `content` is still sent as the record/fallback; + // a capable UI shows the terminal card, an incapable one shows the text. + // (The exit-status pill via `_meta.terminal_exit` needs a structured exit + // code the tool doesn't surface yet — see the RFC follow-up; the exit is + // already visible in the output text's `[exit code: N]` marker.) + const asTerminal = term?.output !== undefined && terminal.enabled notify({ sessionId, update: { @@ -747,6 +785,7 @@ export function streamSessionEventUpdate( status: event.data.isError ? 'failed' : 'completed', content: toolResultContent(present.content), ...present.title !== undefined ? { title: present.title } : {}, + ...asTerminal ? { _meta: { terminal_output: { terminal_id: event.data.callId, data: term.output } } } : {}, }, }) return @@ -758,6 +797,23 @@ export function streamSessionEventUpdate( } } +/** + * Per-connection terminal-rendering context threaded into + * {@link streamSessionEventUpdate}: whether the client advertised the + * `_meta.terminal_output` capability, and the session's workspace cwd (the + * default terminal-card header when a tool doesn't supply its own). Kept out of + * the pure translator's required params so the no-capability / no-presenter + * tests stay terse. + */ +export interface TerminalRendering { + enabled: boolean + /** The session workspace cwd (terminal-card header default); `undefined` when the session has none. */ + cwd: string | undefined +} + +/** Default: terminal rendering off (the ` ```console ` text fallback path). */ +const noTerminalRendering: TerminalRendering = { enabled: false, cwd: undefined } + /** * Resolved pending-state presentation the bridge feeds into a `tool_call` * update: a title is always present (tool name when the tool gives none), `kind` @@ -767,6 +823,8 @@ interface ResolvedCallPresentation { title: string kind: ToolCallKind rawInput?: unknown + /** Tool's request to render as a terminal (the pending side carries the cwd). */ + terminal?: ToolTerminal } /** Resolved completed-state presentation fed into a `tool_call_update`. */ @@ -775,6 +833,8 @@ interface ResolvedResultPresentation { content: ContentBlock[] /** Optional replacement title for the completed call. */ title?: string + /** Tool's terminal output/exit for a terminal-rendered call (the result side). */ + terminal?: ToolTerminal } /** @@ -831,7 +891,12 @@ export class ToolPresenter { // the full parsed args as the raw input (the pre-seam behavior). return { title: name, kind: toolKindFor(name), rawInput: args } } - return { title: present.title, kind: present.kind ?? 'other', rawInput: present.rawInput } + return { + title: present.title, + kind: present.kind ?? 'other', + rawInput: present.rawInput, + ...present.terminal !== undefined ? { terminal: present.terminal } : {}, + } } /** Completed-state presentation for a `tool/result`; consumes the remembered `(name, args)`. */ @@ -852,6 +917,7 @@ export class ToolPresenter { return { content: present.content ?? content, ...present.title !== undefined ? { title: present.title } : {}, + ...present.terminal !== undefined ? { terminal: present.terminal } : {}, } } } diff --git a/packages/acp/tests/turns.spec.ts b/packages/acp/tests/turns.spec.ts index 7b37233585..23f4c2e706 100644 --- a/packages/acp/tests/turns.spec.ts +++ b/packages/acp/tests/turns.spec.ts @@ -14,8 +14,8 @@ import { } from './harness.ts' /** Boilerplate: initialize + create one session, returning its id. */ -async function newSession(h: BridgeHarness): Promise { - await h.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) +async function newSession(h: BridgeHarness, clientCapabilities: Record = {}): Promise { + await h.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities }) const { sessionId } = await h.client.newSession({ cwd: process.cwd(), mcpServers: [] }) return sessionId } @@ -110,6 +110,39 @@ describe('acp bridge — turn outcomes', () => { expect(update).toMatchObject({ toolCallId: 'c1', status: 'completed' }) const content = update.content as { content: { type: string; text: string } }[] expect(content[0]?.content.text).toBe('```console\nhello\n```') + // Capability OFF (the default newSession): NO terminal _meta on either update. + expect((call as { _meta?: unknown })._meta).toBeUndefined() + expect((update as { _meta?: unknown })._meta).toBeUndefined() + }) + + it('with the terminal_output capability ON, a real bash call renders as a TERMINAL card (content + _meta)', async () => { + // Drive the REAL bash tool, and advertise the Zed `_meta.terminal_output` + // capability in initialize. The bridge must then emit the terminal CARD: a + // terminal content block + `_meta.terminal_info` (cwd header) on the call, + // and `_meta.terminal_output`/`terminal_exit` on the result. + harness = await makeBridgeHarness({ + storageDir, + withBash: true, + script: [toolCallResponse('c1', 'bash', { command: 'echo hi', description: 'Greet' }), textResponse('done')], + }) + // Capability lives under clientCapabilities._meta.terminal_output. + const sessionId = await newSession(harness, { _meta: { terminal_output: true } }) + await harness.client.prompt({ sessionId, prompt: [{ type: 'text', text: 'greet' }] }) + + const call = harness.updates.find(u => u.sessionUpdate === 'tool_call') + if (call?.sessionUpdate !== 'tool_call') throw new Error('expected a tool_call') + // A terminal content block keyed by the callId, and terminal_info with the + // session cwd (the bridge fills it from the session header). + expect(call.content).toEqual([{ type: 'terminal', terminalId: 'c1' }]) + expect((call._meta as { terminal_info?: unknown }).terminal_info).toEqual({ terminal_id: 'c1', cwd: process.cwd() }) + + const update = harness.updates.find(u => u.sessionUpdate === 'tool_call_update') + if (update?.sessionUpdate !== 'tool_call_update') throw new Error('expected a tool_call_update') + // Output rides on _meta.terminal_output; the text content is still present + // as the fallback for a UI that ignores the _meta. + const meta = update._meta as { terminal_output?: { terminal_id: string; data: string } } + expect(meta.terminal_output?.terminal_id).toBe('c1') + expect(meta.terminal_output?.data).toBe('hi') }) it('a throwing tool presenter does not break the turn: the bridge falls back generically', async () => { diff --git a/packages/tool-bash/README.md b/packages/tool-bash/README.md index 194c94c1ca..3068bc34dd 100644 --- a/packages/tool-bash/README.md +++ b/packages/tool-bash/README.md @@ -34,7 +34,7 @@ The owning agent is recorded per task id at spawn and kept for the lifetime of t ## UI presentation -These tools own how their calls render in a UI (an editor's tool-call card) via the `dsh-tools` `presentCall`/`presentResult` seam — a UI never special-cases tool names. For `bash`: the **title** is the model-written `description` followed by the exact `command` ("List files in src — ls -la src"), `kind` is `execute` (terminal/run treatment), and the `command` is ALSO the **rawInput**. Why both in the title: an execute-kind card hides `rawInput` (Zed renders it only for non-terminal tools), so the command must ride in the always-visible title to be seen — the reference ACP adapters (claude-agent-acp, codex-acp) likewise put the command in an execute tool's title. The completed output is wrapped in a fenced ` ```console ` block — a UI-only affordance, so the model-facing result text stays unfenced. `bash_output`/`bash_kill` present a task-scoped title ("Read output from background task bash-3" / "Kill background task bash-3") with the task id as rawInput. These methods are pure/display-only (they also run on `session/load` replay), and a malformed/older logged arg shape falls back to a generic presentation rather than throwing. See `packages/tools` ("Tool-owned UI presentation") and `packages/acp` ("Tool-call presentation"). +These tools own how their calls render in a UI (an editor's tool-call card) via the `dsh-tools` `presentCall`/`presentResult` seam — a UI never special-cases tool names. For `bash`: the **title** is the model-written `description` followed by the exact `command` ("List files in src — ls -la src"), `kind` is `execute` (terminal/run treatment), and the `command` is ALSO the **rawInput**. Why both in the title: an execute-kind card hides `rawInput` (Zed renders it only for non-terminal tools), so the command must ride in the always-visible title to be seen — the reference ACP adapters (claude-agent-acp, codex-acp) likewise put the command in an execute tool's title. The completed output is wrapped in a fenced ` ```console ` block — a UI-only affordance, so the model-facing result text stays unfenced. `bash` also flags itself as a **terminal** (the neutral `terminal` field on its presentation: `presentCall` sets a `cwd` from an explicit absolute `workdir`, else leaves it for the UI bridge to fill from the session cwd; `presentResult` carries the output) so a capable client (Zed) renders a terminal card instead of the text block — see `packages/acp` ("Terminal card"). `bash_output`/`bash_kill` present a task-scoped title ("Read output from background task bash-3" / "Kill background task bash-3") with the task id as rawInput. These methods are pure/display-only (they also run on `session/load` replay), and a malformed/older logged arg shape falls back to a generic presentation rather than throwing. See `packages/tools` ("Tool-owned UI presentation") and `packages/acp` ("Terminal card" / "Tool-call presentation"). ## Background completion notices diff --git a/packages/tool-bash/src/index.ts b/packages/tool-bash/src/index.ts index 660c4ecfea..908fdf274d 100644 --- a/packages/tool-bash/src/index.ts +++ b/packages/tool-bash/src/index.ts @@ -40,7 +40,6 @@ import type { Context } from 'cordis' import { isAbsolute, resolve as resolvePath } from 'node:path' import { defineTool } from '@deepseek-ai/dsh-tools' import type { ToolCallPresentation, ToolResult, ToolResultPresentation } from '@deepseek-ai/dsh-tools' -import type { ContentBlock } from '@deepseek-ai/dsh-llm' import type { Agent } from '@deepseek-ai/dsh-agent' import type { BashRunResult, BashTask, CollectedOutput } from '@deepseek-ai/dsh-bash' @@ -142,24 +141,40 @@ export function renderResult(result: BashRunResult): string { * title for execute tools. The description leads (a readable summary the schema * requires); the command follows so the verbatim text is still there. `rawInput` * still carries the bare command for non-execute UIs that DO render it. + * + * `terminal` marks the call so a capable UI renders a TERMINAL card. The cwd + * header comes from an explicit absolute model `workdir` when given; otherwise + * the call ran in the session workspace, which this PURE presenter (args only, + * no `exec`) can't see — the UI bridge fills that default from the session's own + * cwd. An empty `terminal: {}` still flags "this is a terminal". */ -function presentBashCall(args: { command: string; description: string }): ToolCallPresentation { - return { title: `${args.description} — ${args.command}`, kind: 'execute', rawInput: args.command } +function presentBashCall(args: { command: string; description: string; workdir?: string }): ToolCallPresentation { + const cwd = args.workdir !== undefined && isAbsolute(args.workdir) ? args.workdir : undefined + return { + title: `${args.description} — ${args.command}`, + kind: 'execute', + rawInput: args.command, + terminal: cwd !== undefined ? { cwd } : {}, + } } /** - * Completed-state presentation for a `bash` call: wrap the model-facing result - * text in a fenced ```console block so a UI renders the output monospaced as a - * terminal transcript. The model-facing `content` (what `execute` returned) is - * intentionally NOT fenced — the fences are a UI-only affordance, so they live - * here, not in `renderResult`. A non-text result (unexpected for bash) is left - * untouched by falling back to `undefined`. + * Completed-state presentation for a `bash` call. Two parallel renderings of the + * same output: `terminal.output` for a UI that shows a terminal card (the run's + * stdout/stderr + status markers, exactly as the model sees them — it already + * carries the `[exit code: N]` marker), and a fenced ```console `content` block + * as the fallback for a UI without terminal support (the fences are a UI-only + * affordance, so they live here, not in `renderResult`). A non-text result + * (unexpected for bash) falls through to `undefined` (UI keeps the raw result). */ function presentBashResult(_args: unknown, result: ToolResult): ToolResultPresentation | undefined { const block = result.content.length === 1 ? result.content[0] : undefined if (block === undefined || block.type !== 'text') return undefined - const fenced: ContentBlock = { type: 'text', text: `\`\`\`console\n${block.text.replace(/\n+$/, '')}\n\`\`\`` } - return { content: [fenced] } + const text = block.text.replace(/\n+$/, '') + return { + content: [{ type: 'text', text: `\`\`\`console\n${text}\n\`\`\`` }], + terminal: { output: text }, + } } /** Pending-state presentation for `bash_output`/`bash_kill` (background-task tools). */ diff --git a/packages/tool-bash/tests/tools.spec.ts b/packages/tool-bash/tests/tools.spec.ts index c1b1e18808..a5a5cdb58f 100644 --- a/packages/tool-bash/tests/tools.spec.ts +++ b/packages/tool-bash/tests/tools.spec.ts @@ -564,20 +564,32 @@ describe('status lines', () => { }) describe('tool-owned UI presentation (presentCall / presentResult)', () => { - it('bash presentCall: title is "description — command" (execute cards hide rawInput), command also in rawInput', async () => { + it('bash presentCall: title is "description — command", marks a terminal; explicit absolute workdir → cwd header', async () => { const ctx = await setup() - const present = ctx.tools.get('bash')?.presentCall?.({ command: 'ls -la src', description: 'List files in src' }) - expect(present).toEqual({ title: 'List files in src — ls -la src', kind: 'execute', rawInput: 'ls -la src' }) + // No explicit workdir → the call still flags a terminal, but with no cwd (the + // UI bridge fills the session cwd it owns; the pure presenter can't see it). + expect(ctx.tools.get('bash')?.presentCall?.({ command: 'ls -la src', description: 'List files in src' })) + .toEqual({ title: 'List files in src — ls -la src', kind: 'execute', rawInput: 'ls -la src', terminal: {} }) + // An explicit ABSOLUTE workdir is surfaced as the terminal cwd header. + expect(ctx.tools.get('bash')?.presentCall?.({ command: 'pwd', description: 'Print dir', workdir: '/tmp/x' })) + .toEqual({ title: 'Print dir — pwd', kind: 'execute', rawInput: 'pwd', terminal: { cwd: '/tmp/x' } }) + // A RELATIVE workdir is not an absolute cwd → omitted (terminal still flagged). + expect(ctx.tools.get('bash')?.presentCall?.({ command: 'pwd', description: 'Print dir', workdir: 'sub' })) + .toEqual({ title: 'Print dir — pwd', kind: 'execute', rawInput: 'pwd', terminal: {} }) }) - it('bash presentResult: wraps the model-facing text in a fenced console block', async () => { + it('bash presentResult: console-block content AND terminal.output (both renderings of the run)', async () => { const ctx = await setup() const present = ctx.tools.get('bash')!.presentResult!( { command: 'echo hi', description: 'echo' }, { content: [{ type: 'text', text: 'hi\n[exit code: 0]\n\n' }], isError: false }, ) - // Trailing blank lines are trimmed; the body is fenced as ```console. - expect(present).toEqual({ content: [{ type: 'text', text: '```console\nhi\n[exit code: 0]\n```' }] }) + // Trailing blank lines trimmed; content is the fenced ```console fallback, + // terminal.output is the same text for a capable terminal card. + expect(present).toEqual({ + content: [{ type: 'text', text: '```console\nhi\n[exit code: 0]\n```' }], + terminal: { output: 'hi\n[exit code: 0]' }, + }) }) it('bash presentResult: leaves a non-text (unexpected) result untouched → undefined (UI keeps raw content)', async () => { diff --git a/packages/tools/README.md b/packages/tools/README.md index d33e28f1e9..d15b1d1687 100644 --- a/packages/tools/README.md +++ b/packages/tools/README.md @@ -72,8 +72,8 @@ See `defineTool`, `validateArgs`, `ToolArgsError`, `SchemaSpec`, `InferArgs`, an A tool owns how ITS calls render in a UI (an editor's tool-call card, a CLI log line) — a UI plugin must NOT special-case tool names. A `ToolDefinition` may declare two optional, pure, display-only methods: -- `presentCall(args): ToolCallPresentation | undefined` — the PENDING state: a human-readable `title` (always-visible label), an optional `kind` (`read`/`edit`/`execute`/… for icon/treatment, default `other`), and an optional `rawInput` (the salient input to show in a detail view — e.g. a shell command as a string, NOT the whole args object). -- `presentResult(args, result): ToolResultPresentation | undefined` — the COMPLETED state, given the same `args` and the `{ content, isError }` result: an optional replacement `title` and reformatted `content` (e.g. wrap command output in a fenced ` ```console ` block — a UI-only affordance that must NOT appear in the model-facing `execute` result). +- `presentCall(args): ToolCallPresentation | undefined` — the PENDING state: a human-readable `title` (always-visible label), an optional `kind` (`read`/`edit`/`execute`/… for icon/treatment, default `other`), an optional `rawInput` (the salient input to show in a detail view — e.g. a shell command as a string, NOT the whole args object), and an optional `terminal` (a neutral `{ cwd? }` asking a capable UI to render this call as a TERMINAL, e.g. for `bash`). +- `presentResult(args, result): ToolResultPresentation | undefined` — the COMPLETED state, given the same `args` and the `{ content, isError }` result: an optional replacement `title`, reformatted `content` (e.g. wrap command output in a fenced ` ```console ` block — a UI-only affordance that must NOT appear in the model-facing `execute` result), and an optional `terminal` (the `{ output? }` for a terminal-rendered call). The `ToolTerminal` shape is provider-neutral; a UI bridge (the ACP bridge) maps it to a terminal card and a UI that can't ignores it and uses `content`. Returning `undefined` (or omitting a method) tells a UI to fall back to a generic presentation (title = tool name, raw args as input, raw result content). Both methods must be **pure and side-effect-free**: a UI may call them during live streaming AND during a session-log replay, so they depend only on their arguments. With `defineTool`, `args` is the typed `InferArgs` shape; the helper soft-validates before calling (a malformed/older logged arg shape yields `undefined` rather than throwing, since display must never crash a replay). The shapes are provider-neutral — the ACP bridge (`dsh-acp`) maps them to ACP `tool_call`/`tool_call_update` wire fields, and `dsh-tool-bash` is the reference implementation. diff --git a/packages/tools/src/index.ts b/packages/tools/src/index.ts index c967f0fba2..4de993b5c9 100644 --- a/packages/tools/src/index.ts +++ b/packages/tools/src/index.ts @@ -83,6 +83,29 @@ export interface ToolCallPresentation { * unless that is genuinely what a reader wants. */ rawInput?: unknown + /** + * Ask a capable UI to render this call as a TERMINAL (a command running in a + * working directory), not a generic tool card — set by a tool whose call IS a + * shell command (e.g. `bash`). Provider-neutral; a UI bridge maps it to its + * own terminal affordance and a UI that can't falls back to the normal card. + * Pair with {@link ToolResultPresentation.terminal} for the output/exit. + */ + terminal?: ToolTerminal +} + +/** + * A request to render a tool call as a terminal. The pending presentation + * supplies the working directory; the result presentation (see + * {@link ToolResultPresentation.terminal}) supplies the captured output. + * Provider-neutral — no client-protocol types. A UI that supports terminals + * shows a cwd-headed terminal card with the command and its output; a UI that + * does not ignores this and renders the ordinary card/content. + */ +export interface ToolTerminal { + /** Absolute working directory the command ran in, shown as the terminal header. Omit if unknown. */ + cwd?: string + /** Captured command output (stdout+stderr as the tool chooses to combine them). Result-state only. */ + output?: string } /** @@ -102,6 +125,13 @@ export interface ToolResultPresentation { * Stays in harness vocabulary; the UI maps these to its own content blocks. */ content?: ContentBlock[] + /** + * Terminal output/exit for a call the pending presentation marked as a + * terminal (see {@link ToolCallPresentation.terminal}). A capable UI renders + * `output` in the terminal card and shows the exit status; an incapable UI + * uses `content` (the tool should supply a text fallback there too). + */ + terminal?: ToolTerminal } /** A registered tool: its schema plus the execution function. */ From c8dbe6567afecc85524e9a71253783e09c99ca27 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Thu, 18 Jun 2026 17:34:50 +0800 Subject: [PATCH 06/10] docs(acp): clarify _meta is a spec extensibility point; the terminal keys are the Zed convention --- .../implemented/2026-06-18-acp-terminal-and-tool-rendering.md | 4 ++-- packages/acp/README.md | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md b/docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md index 7817b1e441..db0f744c1f 100644 --- a/docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md +++ b/docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md @@ -17,7 +17,7 @@ Studying the two reference agents (2026-06-18) shows neither uses `terminal/crea - **`claude-agent-acp`** (`tools.ts`, `acp-agent.ts`): gated on `clientCapabilities._meta.terminal_output`. The `tool_call` carries `content: [{ type: 'terminal', terminalId }]` and `_meta.terminal_info.{ terminal_id, cwd }`; output/exit arrive on the `tool_call_update`'s `_meta.terminal_output.{ terminal_id, data }` and `_meta.terminal_exit.{ terminal_id, exit_code, signal }`. - **`codex-acp`** (`CodexToolCallMapper.ts`, `TerminalOutputMode.ts`): same `terminal_info` on the call; output via `_meta.terminal_output` (full) or `_meta.terminal_output_delta` (incremental), selected from the same `_meta.terminal_output` capability. -Zed's side (`crates/agent_servers/src/acp.rs`, verified): on a `ToolCall` whose `_meta.terminal_info.terminal_id` is set, it registers a **display-only** terminal (header = `terminal_info.cwd`, label = `tool_call.title`); on a `ToolCallUpdate`, `_meta.terminal_output.data` writes to that terminal and `_meta.terminal_exit.{exit_code,signal}` sets the status. It advertises the capability as `clientCapabilities._meta.terminal_output = true`. This is an off-spec `_meta` extension, but it is the de-facto contract for the Zed integration and the only way to get the terminal card while keeping execution agent-side. +Zed's side (`crates/agent_servers/src/acp.rs`, verified): on a `ToolCall` whose `_meta.terminal_info.terminal_id` is set, it registers a **display-only** terminal (header = `terminal_info.cwd`, label = `tool_call.title`); on a `ToolCallUpdate`, `_meta.terminal_output.data` writes to that terminal and `_meta.terminal_exit.{exit_code,signal}` sets the status. It advertises the capability as `clientCapabilities._meta.terminal_output = true`. `_meta` itself is a spec-blessed ACP extensibility point (typed `{[k]: unknown} | null` on `ToolCall`/`ToolCallUpdate`); the *specific keys* here (`terminal_info`/`terminal_output`/`terminal_exit`) are a Zed convention, not part of the ACP spec — but they are the de-facto contract for the Zed integration and the only way to get the terminal card while keeping execution agent-side. ## Decision @@ -30,7 +30,7 @@ Keep `dsh-bash` agent-side execution; render the terminal card via the `_meta` c ## Risks / trade-offs -- **Off-spec `_meta`.** The terminal card rides on a Zed-specific `_meta` extension, not the ACP terminal sub-protocol. A client that doesn't recognize it still gets the text fallback (the capability gate ensures we only emit it when the client opted in via `_meta.terminal_output`), so a non-Zed client is never worse off. If ACP later standardizes agent-executed terminals, migrate to that and drop the `_meta`. +- **Zed-convention `_meta` keys.** The terminal card rides on Zed-specific keys (`terminal_info`/`terminal_output`) inside ACP's spec-blessed `_meta` extensibility point, NOT on the ACP terminal sub-protocol. A client that doesn't recognize the keys still gets the text fallback (the capability gate ensures we only emit them when the client opted in via `_meta.terminal_output`), so a non-Zed client is never worse off. If ACP later standardizes agent-executed terminals, migrate to that and drop the convention keys. - **Capability honesty.** Emit terminal metadata ONLY when the client advertised `_meta.terminal_output`; the text fallback is the contract for everyone else and must never regress. Covered by a no-capability test asserting the ` ```console ` path. - **terminalId collisions.** Deriving it from the per-call `callId` keeps it unique within a session and stable across the call/result pair; never reuse one across calls. - **Provider-neutral vocabulary creep.** The terminal presentation widens the `dsh-tools` surface; keep it neutral (no ACP types leak into `dsh-tools`) and only as rich as a second UI consumer would also want. diff --git a/packages/acp/README.md b/packages/acp/README.md index 3cb311a43e..4b6d81dae5 100644 --- a/packages/acp/README.md +++ b/packages/acp/README.md @@ -53,7 +53,7 @@ A tool whose call IS a shell command (`bash`) can render as a real **terminal ca - `tool_call`: `content:[{type:'terminal', terminalId}]` + `_meta.terminal_info.{terminal_id, cwd}` — the terminal id is the harness `callId`; the cwd is the tool's explicit `terminal.cwd` if it has one, else the session's workspace cwd (the bridge fills that, since the pure tool presenter can't see it). - `tool_call_update`: `_meta.terminal_output.{terminal_id, data}` — the captured output, attached at completion. -When the client does NOT advertise the capability, none of the `_meta`/terminal content is emitted and the ` ```console ` text block (above) is the rendering — so a non-Zed client is never worse off. This is an off-spec Zed `_meta` extension, not the ACP `terminal/create` sub-protocol: that would make the editor execute the command, bypassing `dsh-bash`'s sandbox/env-scrub/ownership/cwd. The exit-status pill, live streaming, and command classification are follow-ups. See [the terminal-rendering RFC](../../docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md). +When the client does NOT advertise the capability, none of the `_meta`/terminal content is emitted and the ` ```console ` text block (above) is the rendering — so a non-Zed client is never worse off. The `_meta` object is ACP's spec-blessed extensibility point; the specific `terminal_info`/`terminal_output` keys are a Zed convention, not the ACP `terminal/create` sub-protocol (which would make the editor execute the command, bypassing `dsh-bash`'s sandbox/env-scrub/ownership/cwd). The exit-status pill, live streaming, and command classification are follow-ups. See [the terminal-rendering RFC](../../docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md). ## Settle-exactly-once From e51dabbb8b002001fe1d12e1bfa94b88480a1106 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Thu, 18 Jun 2026 18:54:32 +0800 Subject: [PATCH 07/10] feat(acp): align bash terminal card with reference adapters (command title, description block, exit pill) Match claude-agent-acp / codex-acp: the bash tool_call title IS the command (an execute card hides rawInput), the model description rides as a content text block above the card, and the completed card carries an exit-status pill via _meta.terminal_exit. Bridge fixes found in review of the prior terminal-card commit: - tool_call_update.content is OMITTED in terminal mode (an ACP update.content REPLACES the call's content collection in Zed, so the fenced ```console block would clobber the terminal content block). - terminal.output preserves RAW newlines (terminal renderers rely on exact bytes); only the fenced fallback trims trailing blank lines. - a relative workdir is resolved against the session cwd for the card header, matching where the command actually ran. - result-side terminal output is gated on the pending call having registered a terminal (no orphan _meta.terminal_output for a terminal Zed never made). The exit pill is recovered by parsing renderResult's status markers (the pure presentResult seam sees only content blocks); a round-trip test pins the parse to the marker emission. Neutral ToolTerminal gains exitCode/signal; widened ToolCallPresentation with a content block. Docs (RFC + 3 READMEs) updated; with-key e2e verifies the card + exit pill against the real model. --- ...6-06-18-acp-terminal-and-tool-rendering.md | 13 ++- examples/acp-agent/tests/acp.e2e.ts | 19 ++- packages/acp/README.md | 10 +- packages/acp/src/index.ts | 94 ++++++++++++--- packages/acp/tests/load.spec.ts | 41 ++++++- packages/acp/tests/stream-update.spec.ts | 110 ++++++++++++++++++ packages/acp/tests/turns.spec.ts | 44 ++++--- packages/tool-bash/README.md | 2 +- packages/tool-bash/src/index.ts | 75 ++++++++---- packages/tool-bash/tests/tools.spec.ts | 60 ++++++++-- packages/tools/README.md | 8 +- packages/tools/src/index.ts | 37 +++++- 12 files changed, 417 insertions(+), 96 deletions(-) diff --git a/docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md b/docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md index db0f744c1f..d7b0ee12f1 100644 --- a/docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md +++ b/docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md @@ -4,9 +4,9 @@ Status: implemented ## Problem -The ACP bridge lets each tool own its call rendering via `presentCall`/`presentResult` (see [tool-call UI presentation](../proposed/2026-06-14-acp-agent-client-protocol.md) and `packages/tools`). For `bash` we surface the model's `description` plus the command as the `tool_call` title, `kind: 'execute'`, and the completed output wrapped in a fenced ` ```console ` text block. +The ACP bridge lets each tool own its call rendering via `presentCall`/`presentResult` (see [tool-call UI presentation](../proposed/2026-06-14-acp-agent-client-protocol.md) and `packages/tools`). For `bash` we surface the exact command as the `tool_call` title, the model's `description` as a content text block, `kind: 'execute'`, and the completed output wrapped in a fenced ` ```console ` text block. -That is a correct, capability-free baseline, but not how the reference editors render a *terminal* tool at its best. An editor like Zed has a dedicated terminal tool-call card — a header showing the working directory, the command as the label, and the command output rendered as a terminal — but it only builds that card when the `tool_call` carries terminal metadata (below). With a plain text block the output appears as static markdown and there is no cwd header. (Zed also HIDES `rawInput` for `kind: 'execute'`, which is why our command rides inside the title.) +That is a correct, capability-free baseline, but not how the reference editors render a *terminal* tool at its best. An editor like Zed has a dedicated terminal tool-call card — a header showing the working directory, the command as the label, the command output rendered as a terminal, and an exit-status pill — but it only builds that card when the `tool_call` carries terminal metadata (below). With a plain text block the output appears as static markdown and there is no cwd header. (Zed also HIDES `rawInput` for `kind: 'execute'`, which is why the command IS the title — both reference adapters do the same — and the human-readable description rides as a separate content block, since a terminal card has no description slot.) ## Key finding: agent-executed terminals use a `_meta` convention, NOT `terminal/create` @@ -25,16 +25,17 @@ Keep `dsh-bash` agent-side execution; render the terminal card via the `_meta` c 1. **Capability.** `initialize` reads `clientCapabilities._meta.terminal_output` and the bridge remembers it per connection. 2. **Neutral presentation vocabulary.** `dsh-tools` gains a terminal-shaped presentation a tool can return — provider-neutral (`cwd`, the output `data`, an `exitCode`/`signal`), NO ACP types. `dsh-tool-bash` returns it for `bash` (cwd from the resolved workdir; output + exit parsed from the run result). -3. **Bridge mapping.** When the client advertised the capability, the bridge maps that presentation to: on `tool_call`, `content:[{type:'terminal', terminalId}]` + `_meta.terminal_info.{terminal_id,cwd}`; on `tool_call_update`, `_meta.terminal_output.{terminal_id,data}` (the captured output). `terminalId` is derived from the harness `callId` (stable, unique per call). When the capability is absent, the bridge uses the existing ` ```console ` text content — unchanged. -4. **No new execution path, no live streaming, no exit pill yet.** Output is attached at completion (from the agent's own `tool/result`), not streamed token-by-token. The exit-status pill (`_meta.terminal_exit.{exit_code,signal}`) is NOT emitted: it needs a structured exit code the pure `presentResult(args, result)` seam doesn't get (the result is content blocks), and the exit is already visible in the output text's `[exit code: N]` / `[killed by signal: …]` marker. Disposal is unaffected: nothing new to tear down, since the bridge never creates a client-side terminal. +3. **Bridge mapping.** When the client advertised the capability, the bridge maps that presentation to: on `tool_call`, `content:[…, {type:'terminal', terminalId}]` (any tool `content`, e.g. the description, rendered BEFORE the terminal block) + `_meta.terminal_info.{terminal_id,cwd}`; on `tool_call_update`, `_meta.terminal_output.{terminal_id,data}` (the captured output) + `_meta.terminal_exit.{terminal_id, exit_code|signal}` (the parsed exit), with the update's text `content` OMITTED (an ACP `tool_call_update.content` REPLACES the call's content collection, so re-sending the fenced block would clobber the terminal content block). `terminalId` is derived from the harness `callId` (stable, unique per call). When the capability is absent, the bridge sends the description content block on the call and the existing ` ```console ` text content on the update — unchanged. +4. **The exit pill is parsed from the rendered output; no new execution path, no live streaming.** Output is attached at completion (from the agent's own `tool/result`), not streamed token-by-token. The exit-status pill (`_meta.terminal_exit.{exit_code,signal}`) IS emitted: the pure `presentResult(args, result)` seam sees only content blocks, so `dsh-tool-bash` recovers the structured exit by parsing the status markers (`[exit code: N]` / `[killed by signal: …]`) that `renderResult` appended — the parse is the exact inverse of the marker emission, the two co-evolve in one file, and a round-trip test guards the pair. Disposal is unaffected: nothing new to tear down, since the bridge never creates a client-side terminal. ## Risks / trade-offs -- **Zed-convention `_meta` keys.** The terminal card rides on Zed-specific keys (`terminal_info`/`terminal_output`) inside ACP's spec-blessed `_meta` extensibility point, NOT on the ACP terminal sub-protocol. A client that doesn't recognize the keys still gets the text fallback (the capability gate ensures we only emit them when the client opted in via `_meta.terminal_output`), so a non-Zed client is never worse off. If ACP later standardizes agent-executed terminals, migrate to that and drop the convention keys. +- **Zed-convention `_meta` keys.** The terminal card rides on Zed-specific keys (`terminal_info`/`terminal_output`/`terminal_exit`) inside ACP's spec-blessed `_meta` extensibility point, NOT on the ACP terminal sub-protocol. A client that doesn't recognize the keys still gets the text fallback (the capability gate ensures we only emit them when the client opted in via `_meta.terminal_output`), so a non-Zed client is never worse off. If ACP later standardizes agent-executed terminals, migrate to that and drop the convention keys. - **Capability honesty.** Emit terminal metadata ONLY when the client advertised `_meta.terminal_output`; the text fallback is the contract for everyone else and must never regress. Covered by a no-capability test asserting the ` ```console ` path. - **terminalId collisions.** Deriving it from the per-call `callId` keeps it unique within a session and stable across the call/result pair; never reuse one across calls. +- **Exit parsed from rendered text.** The exit pill recovers `exit_code`/`signal` by parsing `renderResult`'s status markers rather than threading a structured exit through the event schema (which the pure `presentResult` seam never sees). The parse is the exact inverse of the marker emission and lives in the same file; a round-trip test pins the pair so a marker-format change that breaks the parse fails the suite. If the markers ever need to diverge from what the pill wants, surface a structured exit on the result event instead. - **Provider-neutral vocabulary creep.** The terminal presentation widens the `dsh-tools` surface; keep it neutral (no ACP types leak into `dsh-tools`) and only as rich as a second UI consumer would also want. ## Out of scope / non-goals -The text-block baseline stays the no-capability default. Client-side `terminal/create` execution is explicitly rejected (it bypasses `dsh-bash`). Three follow-ups are deliberately NOT built here and would each warrant their own RFC when someone takes them on: the **exit-status pill** (`_meta.terminal_exit.{exit_code,signal}`, which needs the structured exit surfaced from the run rather than parsed out of the rendered output text), **live incremental streaming** (`_meta.terminal_output_delta` as chunks arrive, which needs an incremental-output seam on `dsh-bash`), and **command classification** (parsing a `cat`/`sed` as a `read` card with a file location, a `grep` as a `search`, etc., falling back to the terminal card — display-only, must never change what executes). +The text-block baseline stays the no-capability default. Client-side `terminal/create` execution is explicitly rejected (it bypasses `dsh-bash`). Two follow-ups are deliberately NOT built here and would each warrant their own RFC when someone takes them on: **live incremental streaming** (`_meta.terminal_output_delta` as chunks arrive, which needs an incremental-output seam on `dsh-bash`), and **command classification** (parsing a `cat`/`sed` as a `read` card with a file location, a `grep` as a `search`, etc., falling back to the terminal card — display-only, must never change what executes). diff --git a/examples/acp-agent/tests/acp.e2e.ts b/examples/acp-agent/tests/acp.e2e.ts index 4a43429eb3..8dd8af6d01 100644 --- a/examples/acp-agent/tests/acp.e2e.ts +++ b/examples/acp-agent/tests/acp.e2e.ts @@ -178,8 +178,8 @@ describe.skipIf(!process.env.DEEPSEEK_API_KEY)('acp-agent e2e: real prompt over expect(toolCalls.length).toBeGreaterThan(0) // Tool-call UI quality (the tool owns its presentation): the bash tool's - // `presentCall` sets the title to the model's human-readable `description` - // and the `rawInput` to the exact command — NOT the bare tool name "bash". + // `presentCall` sets the title to the exact command (an execute card hides + // rawInput, so the command IS the title) — NOT the bare tool name "bash". // A `bash` call must therefore carry an execute kind, a non-"bash" title, // and a string rawInput (the command). `toolCalls` is already narrowed to // the `tool_call` shape by the filter above, so these fields are reachable. @@ -194,7 +194,7 @@ describe.skipIf(!process.env.DEEPSEEK_API_KEY)('acp-agent e2e: real prompt over expect((bashCall as { _meta?: unknown })._meta).toBeUndefined() }, 180_000) - it('with the terminal_output capability, a real bash call renders as a terminal card (content + _meta)', async () => { + it('with the terminal_output capability, a real bash call renders as a terminal card (content + _meta + exit)', async () => { workdir = await mkdtemp(join(tmpdir(), 'acp-e2e-')) spawned = spawnAcpAgent(workdir) const { client, updates } = spawned @@ -214,12 +214,19 @@ describe.skipIf(!process.env.DEEPSEEK_API_KEY)('acp-agent e2e: real prompt over // on _meta.terminal_output. const bashCall = updates.find(u => u.sessionUpdate === 'tool_call' && u.kind === 'execute') if (bashCall?.sessionUpdate !== 'tool_call') throw new Error('expected an execute tool_call') - const block = bashCall.content?.[0] as { type: string; terminalId?: string } | undefined - expect(block?.type).toBe('terminal') - expect(typeof block?.terminalId).toBe('string') + // The content carries the description text block AND a terminal block (the + // description renders above the card) — find the terminal block by type, not + // by position. + const blocks = (bashCall.content ?? []) as { type: string; terminalId?: string }[] + const terminalBlock = blocks.find(b => b.type === 'terminal') + expect(terminalBlock).toBeDefined() + expect(typeof terminalBlock?.terminalId).toBe('string') const info = (bashCall._meta as { terminal_info?: { terminal_id: string; cwd?: string } }).terminal_info expect(info?.cwd).toBe(workdir) const updatesForTerminal = updates.filter(u => u.sessionUpdate === 'tool_call_update' && (u._meta as { terminal_output?: unknown } | undefined)?.terminal_output !== undefined) expect(updatesForTerminal.length).toBeGreaterThan(0) + // The completed update also carries the parsed exit on _meta.terminal_exit. + const exitUpdate = updates.find(u => u.sessionUpdate === 'tool_call_update' && (u._meta as { terminal_exit?: unknown } | undefined)?.terminal_exit !== undefined) + expect(exitUpdate).toBeDefined() }, 180_000) }) diff --git a/packages/acp/README.md b/packages/acp/README.md index 4b6d81dae5..a47a7327c2 100644 --- a/packages/acp/README.md +++ b/packages/acp/README.md @@ -42,18 +42,18 @@ Each session runs in its own workspace, recorded as the session's `SessionHeader ## Tool-call presentation -How a tool call renders in the editor is owned by the TOOL, not the bridge — the bridge never special-cases tool names. Each tool may declare `presentCall(args)` (pending state: a human-readable `title`, a `kind` for the icon, and the salient `rawInput` to show in a detail view) and `presentResult(args, result)` (completed state: an optional replacement `title` and reformatted `content`) on its `dsh-tools` definition. The bridge looks the definition up by name in `ctx.tools` and maps the neutral `ToolCallPresentation`/`ToolResultPresentation` to the ACP `tool_call`/`tool_call_update` wire shapes. A tool that declares neither gets a generic fallback (title = tool name, raw parsed args as `rawInput`, kind inferred from the name). For example `dsh-tool-bash` sets the title to the model `description` + the exact `command` ("List files in src — ls -la src"), `kind: 'execute'`, the `command` as `rawInput`, and wraps the completed output in a fenced ` ```console ` block. (The command goes in the title because an editor hides `rawInput` for execute-kind cards — Zed renders it only for non-terminal tools.) +How a tool call renders in the editor is owned by the TOOL, not the bridge — the bridge never special-cases tool names. Each tool may declare `presentCall(args)` (pending state: a human-readable `title`, a `kind` for the icon, the salient `rawInput` to show in a detail view, and optional `content` blocks shown alongside) and `presentResult(args, result)` (completed state: an optional replacement `title` and reformatted `content`) on its `dsh-tools` definition. The bridge looks the definition up by name in `ctx.tools` and maps the neutral `ToolCallPresentation`/`ToolResultPresentation` to the ACP `tool_call`/`tool_call_update` wire shapes. A tool that declares neither gets a generic fallback (title = tool name, raw parsed args as `rawInput`, kind inferred from the name). For example `dsh-tool-bash` sets the title to the exact `command` ("ls -la src"), `kind: 'execute'`, the `command` as `rawInput`, the model `description` as a `content` text block, and wraps the completed output in a fenced ` ```console ` block. (The command is the title because an editor hides `rawInput` for execute-kind cards — Zed renders it only for non-terminal tools — and the reference adapters likewise use the command as an execute tool's title.) The `tool/result` session event carries only `{ callId, content, isError }` — not the tool name or args — so to call a tool's `presentResult` the bridge keeps a small per-session map from `callId` to the in-flight call's `(name, args)`, populated on `tool/call` and removed as each result is presented (it holds only currently-in-flight calls, never finished ones). This is bridge-local state — NOT a change to the event schema or a core service. The map lives on the `SessionRecord`, so two concurrent sessions never cross their in-flight tool state; a `session/load` replay uses a throwaway presenter that pairs each `tool/call` with its `tool/result` as the log replays in order, so replayed tool cards render identically to live ones. ## Terminal card (capability-gated) -A tool whose call IS a shell command (`bash`) can render as a real **terminal card** — a working-directory header with the command's output — rather than a plain text block. The tool asks for this with the neutral `terminal` field on its presentation (`dsh-tools`: a `{ cwd?, output? }` shape on `ToolCallPresentation`/`ToolResultPresentation`); the bridge maps it to the Zed `_meta` convention, gated on the client advertising `clientCapabilities._meta.terminal_output` in `initialize`: +A tool whose call IS a shell command (`bash`) can render as a real **terminal card** — a working-directory header with the command's output and an exit-status pill — rather than a plain text block. The tool asks for this with the neutral `terminal` field on its presentation (`dsh-tools`: a `{ cwd?, output?, exitCode?, signal? }` shape on `ToolCallPresentation`/`ToolResultPresentation`); the bridge maps it to the Zed `_meta` convention, gated on the client advertising `clientCapabilities._meta.terminal_output` in `initialize`: -- `tool_call`: `content:[{type:'terminal', terminalId}]` + `_meta.terminal_info.{terminal_id, cwd}` — the terminal id is the harness `callId`; the cwd is the tool's explicit `terminal.cwd` if it has one, else the session's workspace cwd (the bridge fills that, since the pure tool presenter can't see it). -- `tool_call_update`: `_meta.terminal_output.{terminal_id, data}` — the captured output, attached at completion. +- `tool_call`: `content:[…, {type:'terminal', terminalId}]` + `_meta.terminal_info.{terminal_id, cwd}` — the terminal id is the harness `callId`; the cwd is the tool's explicit absolute `terminal.cwd`, else a relative `terminal.cwd` resolved against the session cwd, else the session's workspace cwd (the bridge fills the default, since the pure tool presenter can't see it). Any pending `content` the tool supplied (e.g. bash's `description`) renders BEFORE the terminal block, so the description sits above the card. +- `tool_call_update`: `_meta.terminal_output.{terminal_id, data}` (the captured output) plus `_meta.terminal_exit.{terminal_id, exit_code | signal}` when the tool reported a structured exit. In terminal mode the update's `content` is OMITTED — an ACP `tool_call_update.content` REPLACES the call's content, so sending the fenced text block would clobber the terminal content block from the call. -When the client does NOT advertise the capability, none of the `_meta`/terminal content is emitted and the ` ```console ` text block (above) is the rendering — so a non-Zed client is never worse off. The `_meta` object is ACP's spec-blessed extensibility point; the specific `terminal_info`/`terminal_output` keys are a Zed convention, not the ACP `terminal/create` sub-protocol (which would make the editor execute the command, bypassing `dsh-bash`'s sandbox/env-scrub/ownership/cwd). The exit-status pill, live streaming, and command classification are follow-ups. See [the terminal-rendering RFC](../../docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md). +When the client does NOT advertise the capability, none of the `_meta`/terminal content is emitted: the `tool_call` shows the `description` content block and the `tool_call_update` carries the ` ```console ` text block (above) as the rendering — so a non-Zed client is never worse off. The `_meta` object is ACP's spec-blessed extensibility point; the specific `terminal_info`/`terminal_output`/`terminal_exit` keys are a Zed convention, not the ACP `terminal/create` sub-protocol (which would make the editor execute the command, bypassing `dsh-bash`'s sandbox/env-scrub/ownership/cwd). Live incremental streaming and command classification are follow-ups. See [the terminal-rendering RFC](../../docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md). ## Settle-exactly-once diff --git a/packages/acp/src/index.ts b/packages/acp/src/index.ts index 7d0624bc92..20d77f9a20 100644 --- a/packages/acp/src/index.ts +++ b/packages/acp/src/index.ts @@ -34,7 +34,7 @@ import type { Context } from 'cordis' import { Readable, Writable } from 'node:stream' import { randomUUID } from 'node:crypto' -import { isAbsolute } from 'node:path' +import { isAbsolute, resolve as resolvePath } from 'node:path' import Schema from 'schemastery' import { AgentSideConnection, @@ -747,6 +747,14 @@ export function streamSessionEventUpdate( // (the cwd header). Otherwise it is an ordinary tool_call and the output // arrives as text on the result. See the terminal-rendering RFC. const asTerminal = present.terminal !== undefined && terminal.enabled + // The tool's pending content (e.g. bash's `description`) renders ABOVE the + // card; when the card is shown, append the terminal block AFTER it so the + // description sits over the command (Zed renders content blocks in order). + // Without the capability the description still renders as the card's body. + const callContent: ({ type: 'content'; content: AcpContentBlock } | { type: 'terminal'; terminalId: string })[] = [ + ...present.content !== undefined ? toolResultContent(present.content) : [], + ...asTerminal ? [{ type: 'terminal' as const, terminalId: event.data.callId }] : [], + ] notify({ sessionId, update: { @@ -756,11 +764,9 @@ export function streamSessionEventUpdate( kind: present.kind, status: 'in_progress', ...present.rawInput !== undefined ? { rawInput: present.rawInput } : {}, + ...callContent.length > 0 ? { content: callContent } : {}, ...asTerminal - ? { - content: [{ type: 'terminal', terminalId: event.data.callId }], - _meta: { terminal_info: { terminal_id: event.data.callId, cwd: present.terminal?.cwd ?? terminal.cwd } }, - } + ? { _meta: { terminal_info: { terminal_id: event.data.callId, cwd: terminalCwd(present.terminal, terminal.cwd) } } } : {}, }, }) @@ -769,23 +775,30 @@ export function streamSessionEventUpdate( case 'tool/result': { const present = presenter.result(event.data.callId, event.data.content, event.data.isError) const term = present.terminal - // When the call rendered as a terminal AND the client is capable, stream - // the output on the update's `_meta.terminal_output` (the terminal card - // consumes it). The text `content` is still sent as the record/fallback; - // a capable UI shows the terminal card, an incapable one shows the text. - // (The exit-status pill via `_meta.terminal_exit` needs a structured exit - // code the tool doesn't surface yet — see the RFC follow-up; the exit is - // already visible in the output text's `[exit code: N]` marker.) + // When the call rendered as a terminal AND the client is capable, the output + // and exit status ride on `_meta` (the terminal card consumes them) and the + // text `content` is OMITTED: a `tool_call_update.content` REPLACES the call's + // content collection in Zed, so sending the fenced ```console block here + // would clobber the terminal content block the call installed. The incapable + // path keeps sending `content` (the fenced fallback is the only rendering). const asTerminal = term?.output !== undefined && terminal.enabled + const terminalResultMeta = asTerminal + ? { + _meta: { + terminal_output: { terminal_id: event.data.callId, data: term.output }, + ...terminalExitMeta(event.data.callId, term), + }, + } + : {} notify({ sessionId, update: { sessionUpdate: 'tool_call_update', toolCallId: event.data.callId, status: event.data.isError ? 'failed' : 'completed', - content: toolResultContent(present.content), + ...asTerminal ? {} : { content: toolResultContent(present.content) }, ...present.title !== undefined ? { title: present.title } : {}, - ...asTerminal ? { _meta: { terminal_output: { terminal_id: event.data.callId, data: term.output } } } : {}, + ...terminalResultMeta, }, }) return @@ -823,6 +836,8 @@ interface ResolvedCallPresentation { title: string kind: ToolCallKind rawInput?: unknown + /** UI content shown on the pending call (e.g. a bash description text block above the card). */ + content?: ContentBlock[] /** Tool's request to render as a terminal (the pending side carries the cwd). */ terminal?: ToolTerminal } @@ -859,7 +874,7 @@ interface ResolvedResultPresentation { * stale entry's only cost is one map slot until the session ends. */ export class ToolPresenter { - private readonly pending = new Map() + private readonly pending = new Map() /** * @param tools the registry to resolve tool definitions by name. @@ -877,7 +892,6 @@ export class ToolPresenter { /** 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 }) let present: ToolCallPresentation | undefined try { present = this.tools.get(name)?.presentCall?.(args) @@ -888,13 +902,21 @@ export class ToolPresenter { } 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). + // the full parsed args as the raw input (the pre-seam behavior). A generic + // call is never a terminal, so a later result can't emit terminal output. + this.pending.set(callId, { name, args, isTerminal: false }) return { title: name, kind: toolKindFor(name), rawInput: args } } + // Remember whether THIS call rendered as a terminal, so `result()` only emits + // terminal output/exit for a call that actually registered a terminal — a + // `presentResult().terminal` without a matching `presentCall().terminal` + // would otherwise orphan `_meta.terminal_output` to a terminal Zed never made. + this.pending.set(callId, { name, args, isTerminal: present.terminal !== undefined }) return { title: present.title, kind: present.kind ?? 'other', rawInput: present.rawInput, + ...present.content !== undefined ? { content: present.content } : {}, ...present.terminal !== undefined ? { terminal: present.terminal } : {}, } } @@ -917,7 +939,10 @@ export class ToolPresenter { return { content: present.content ?? content, ...present.title !== undefined ? { title: present.title } : {}, - ...present.terminal !== undefined ? { terminal: present.terminal } : {}, + // Only propagate terminal output/exit when the PENDING call registered a + // terminal (finding: orphan terminal output otherwise). A result-only + // terminal with no matching call-side terminal is dropped. + ...present.terminal !== undefined && call.isTerminal ? { terminal: present.terminal } : {}, } } } @@ -961,3 +986,36 @@ function toolResultContent(blocks: ContentBlock[]): { type: 'content'; content: } return out } + +/** + * Resolve the terminal card's header cwd. The tool's `terminal.cwd` (a model + * `workdir`) wins when ABSOLUTE; a RELATIVE one resolves against the session + * cwd (matching how `dsh-tool-bash` resolves a relative workdir for execution, + * so the header matches where the command actually ran); when the tool gives no + * cwd, the session workspace cwd is the default. Returns `undefined` only when + * neither the tool nor the session supplies one (Zed then shows "current + * directory"). + */ +function terminalCwd(term: ToolTerminal | undefined, sessionCwd: string | undefined): string | undefined { + const toolCwd = term?.cwd + if (toolCwd === undefined) return sessionCwd + if (isAbsolute(toolCwd)) return toolCwd + return sessionCwd !== undefined ? resolvePath(sessionCwd, toolCwd) : toolCwd +} + +/** The `terminal_exit` `_meta` entry for a completed terminal call. */ +interface TerminalExitMeta { + terminal_exit?: { terminal_id: string; exit_code?: number; signal?: string } +} + +/** + * Build the optional `terminal_exit` portion of a `tool_call_update`'s `_meta` + * from the tool's terminal result: a `signal` death yields `{signal}`, an + * `exitCode` yields `{exit_code}`, and neither yields nothing (the card simply + * shows no exit pill). Spread into the `_meta` object alongside `terminal_output`. + */ +function terminalExitMeta(callId: string, term: ToolTerminal): TerminalExitMeta { + if (term.signal !== undefined) return { terminal_exit: { terminal_id: callId, signal: term.signal } } + if (term.exitCode !== undefined) return { terminal_exit: { terminal_id: callId, exit_code: term.exitCode } } + return {} +} diff --git a/packages/acp/tests/load.spec.ts b/packages/acp/tests/load.spec.ts index 89bd8253f7..a10aca4bd4 100644 --- a/packages/acp/tests/load.spec.ts +++ b/packages/acp/tests/load.spec.ts @@ -81,7 +81,10 @@ describe('acp bridge — session/load replay', () => { 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: 'Print a greeting — echo hello', kind: 'execute', rawInput: 'echo hello' }) + expect(call).toMatchObject({ toolCallId: 'c1', title: 'echo hello', kind: 'execute', rawInput: 'echo hello' }) + if (call?.sessionUpdate !== 'tool_call') throw new Error('expected a tool_call') + // Capability OFF on this loader: the description renders as a content block, no terminal block. + expect(call.content).toEqual([{ type: 'content', content: { type: 'text', text: 'Print a greeting' } }]) const update = loader.updates.find(u => u.sessionUpdate === 'tool_call_update') expect(update?.sessionUpdate).toBe('tool_call_update') if (update?.sessionUpdate !== 'tool_call_update') throw new Error('expected a tool_call_update') @@ -90,6 +93,42 @@ describe('acp bridge — session/load replay', () => { expect(content[0]?.content.text).toBe('```console\nhello\n```') }) + it('replays a persisted bash call as a TERMINAL card when the loader advertises the capability', async () => { + // The presentation is resolved at replay time, so a loader that advertised + // _meta.terminal_output must reconstruct the terminal card (content + _meta) + // from the persisted log — identical to how it would have streamed live. + live = await makeBridgeHarness({ + storageDir, + withBash: true, + script: [toolCallResponse('c1', 'bash', { command: 'echo hi', description: 'Greet' }), textResponse('done')], + }) + 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: 'greet' }] }) + await live.dispose() + live = undefined + + loader = await makeBridgeHarness({ storageDir, withBash: true, script: [] }) + await loader.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: { _meta: { terminal_output: true } } }) + await loader.client.loadSession({ sessionId, cwd: process.cwd(), mcpServers: [] }) + + const call = loader.updates.find(u => u.sessionUpdate === 'tool_call') + if (call?.sessionUpdate !== 'tool_call') throw new Error('expected a tool_call') + // Replay reconstructs the terminal card: description block, then terminal block. + expect(call.content).toEqual([ + { type: 'content', content: { type: 'text', text: 'Greet' } }, + { type: 'terminal', terminalId: 'c1' }, + ]) + expect((call._meta as { terminal_info?: unknown }).terminal_info).toEqual({ terminal_id: 'c1', cwd: process.cwd() }) + const update = loader.updates.find(u => u.sessionUpdate === 'tool_call_update') + if (update?.sessionUpdate !== 'tool_call_update') throw new Error('expected a tool_call_update') + // Terminal mode: content omitted, output + exit on _meta — matching live. + expect(update.content).toBeUndefined() + const meta = update._meta as { terminal_output?: { data: string }; terminal_exit?: { exit_code?: number } } + expect(meta.terminal_output?.data).toBe('hi\n') + expect(meta.terminal_exit?.exit_code).toBe(0) + }) + 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 8a22bc71f1..bf73fe0629 100644 --- a/packages/acp/tests/stream-update.spec.ts +++ b/packages/acp/tests/stream-update.spec.ts @@ -281,6 +281,116 @@ describe('ToolPresenter (tool-owned presentation via the tool registry)', () => }) }) +describe('terminal-card mapping (capability-gated)', () => { + // A tool that asks to render as a terminal — a stand-in for tool-bash's shape, + // letting us drive the bridge's terminal mapping without the real executor. + type CallTerm = { cwd?: string } | undefined + type ResultTerm = { output?: string; exitCode?: number; signal?: string } | undefined + const termTool = (callTerminal: CallTerm, resultTerminal: ResultTerm): ToolDefinition => ({ + name: 'bash', + description: 'run a command', + parameters: {}, + execute: async () => [], + presentCall: (args: unknown) => ({ + title: (args as { command: string }).command, + kind: 'execute', + rawInput: (args as { command: string }).command, + content: [{ type: 'text', text: (args as { description: string }).description }], + ...callTerminal !== undefined ? { terminal: callTerminal } : {}, + }), + presentResult: () => ({ + content: [{ type: 'text', text: 'fallback' }], + ...resultTerminal !== undefined ? { terminal: resultTerminal } : {}, + }), + }) + + const callEvent = evt('tool/call', { turn: 1, step: 1, callId: CallId('c1'), name: 'bash', arguments: JSON.stringify({ command: 'echo hi', description: 'Greet' }) }) + const resultEvent = evt('tool/result', { turn: 1, step: 1, callId: CallId('c1'), content: [{ type: 'text', text: 'hi\n' }], isError: false }) + + function termUpdates(tool: ToolDefinition, enabled: boolean, cwd: string | undefined, ...events: SessionEvent[]): SessionNotification['update'][] { + const presenter = new ToolPresenter(registryOf(tool)) + const out: SessionNotification['update'][] = [] + for (const event of events) streamSessionEventUpdate('s1', event, n => out.push(n.update), presenter, { enabled, cwd }) + return out + } + + it('capability ON: description content THEN terminal block; cwd from the session header when the tool gives none', () => { + const [call, update] = termUpdates(termTool({}, { output: 'hi\n', exitCode: 0 }), true, '/work/proj', callEvent, resultEvent) + expect(call).toMatchObject({ + sessionUpdate: 'tool_call', + content: [ + { type: 'content', content: { type: 'text', text: 'Greet' } }, + { type: 'terminal', terminalId: 'c1' }, + ], + _meta: { terminal_info: { terminal_id: 'c1', cwd: '/work/proj' } }, + }) + // The update OMITS content (it would clobber the terminal block) and carries output + exit. + expect(update).toEqual({ + sessionUpdate: 'tool_call_update', + toolCallId: 'c1', + status: 'completed', + _meta: { terminal_output: { terminal_id: 'c1', data: 'hi\n' }, terminal_exit: { terminal_id: 'c1', exit_code: 0 } }, + }) + }) + + it('capability ON: an ABSOLUTE tool cwd wins; a RELATIVE one resolves against the session cwd', () => { + const [absCall] = termUpdates(termTool({ cwd: '/explicit/abs' }, { output: 'x' }), true, '/work/proj', callEvent) + expect((absCall as unknown as { _meta: { terminal_info: { cwd: string } } })._meta.terminal_info.cwd).toBe('/explicit/abs') + const [relCall] = termUpdates(termTool({ cwd: 'sub/dir' }, { output: 'x' }), true, '/work/proj', callEvent) + // Relative workdir resolved against the session cwd — the card header matches + // where execution actually ran (tool-bash resolves the same way). + expect((relCall as unknown as { _meta: { terminal_info: { cwd: string } } })._meta.terminal_info.cwd).toBe('/work/proj/sub/dir') + // No session cwd to resolve against → the relative tool cwd is passed through as-is. + const [noSessionCwd] = termUpdates(termTool({ cwd: 'rel/only' }, { output: 'x' }), true, undefined, callEvent) + expect((noSessionCwd as unknown as { _meta: { terminal_info: { cwd: string } } })._meta.terminal_info.cwd).toBe('rel/only') + }) + + it('capability ON: a signal kill maps to terminal_exit.signal', () => { + const [, update] = termUpdates(termTool({}, { output: 'gone', signal: 'SIGKILL' }), true, '/w', callEvent, resultEvent) + expect((update as unknown as { _meta: { terminal_exit: unknown } })._meta.terminal_exit).toEqual({ terminal_id: 'c1', signal: 'SIGKILL' }) + }) + + it('capability ON: a terminal result with output but NO exit/signal emits terminal_output and NO exit pill', () => { + // A terminal-rendering tool that reports no structured exit (neither exitCode + // nor signal) — the card shows output but no exit pill. + const [, update] = termUpdates(termTool({}, { output: 'partial' }), true, '/w', callEvent, resultEvent) + const meta = (update as unknown as { _meta: { terminal_output?: unknown; terminal_exit?: unknown } })._meta + expect(meta.terminal_output).toEqual({ terminal_id: 'c1', data: 'partial' }) + expect(meta.terminal_exit).toBeUndefined() + }) + + it('capability OFF: no terminal block or _meta; the description content and fenced result still render', () => { + const [call, update] = termUpdates(termTool({}, { output: 'hi\n' }), false, '/work/proj', callEvent, resultEvent) + expect(call).toEqual({ + sessionUpdate: 'tool_call', + toolCallId: 'c1', + title: 'echo hi', + kind: 'execute', + status: 'in_progress', + rawInput: 'echo hi', + content: [{ type: 'content', content: { type: 'text', text: 'Greet' } }], + }) + expect(update).toEqual({ + sessionUpdate: 'tool_call_update', + toolCallId: 'c1', + status: 'completed', + content: [{ type: 'content', content: { type: 'text', text: 'fallback' } }], + }) + }) + + it('orphan guard: a result-side terminal with NO call-side terminal is dropped (no orphan terminal_output)', () => { + // presentCall declares NO terminal, but presentResult returns one — the + // bridge must not emit _meta.terminal_output for a terminal Zed never made. + const [call, update] = termUpdates(termTool(undefined, { output: 'hi\n', exitCode: 0 }), true, '/w', callEvent, resultEvent) + // The call had no terminal → ordinary tool_call (description content, no _meta). + expect((call as { _meta?: unknown })._meta).toBeUndefined() + expect((call as { content: unknown }).content).toEqual([{ type: 'content', content: { type: 'text', text: 'Greet' } }]) + // The result falls back to text content; NO terminal _meta. + expect((update as { _meta?: unknown })._meta).toBeUndefined() + expect((update as { content: unknown }).content).toEqual([{ type: 'content', content: { type: 'text', text: 'fallback' } }]) + }) +}) + describe('agentOptions', () => { it('includes only the fields present in config', () => { expect(agentOptions({})).toEqual({}) diff --git a/packages/acp/tests/turns.spec.ts b/packages/acp/tests/turns.spec.ts index 23f4c2e706..3cf12a67e8 100644 --- a/packages/acp/tests/turns.spec.ts +++ b/packages/acp/tests/turns.spec.ts @@ -75,7 +75,7 @@ describe('acp bridge — turn outcomes', () => { expect(callIdx).toBeLessThan(updIdx) }) - it('the REAL bash tool drives the tool-call UI end-to-end: description—command title + console output', async () => { + it('the REAL bash tool drives the tool-call UI end-to-end: command title + description block + console output', async () => { // Use the SHIPPING tool (dsh-tool-bash + dsh-bash-local), not an inline // stand-in, so this verifies the actual presentCall/presentResult the editor // sees (AGENTS.md "prefer the real implementation over a mock in tests"). @@ -93,16 +93,20 @@ describe('acp bridge — turn outcomes', () => { const sessionId = await newSession(harness) await harness.client.prompt({ sessionId, prompt: [{ type: 'text', text: 'greet' }] }) - // presentCall: execute kind, title is "description — command" (an execute - // card hides rawInput, so the command rides in the title), command in rawInput. + // presentCall: execute kind, title IS the command (an execute card hides + // rawInput, so the command is the title), the description rides as a content + // text block, the command is also rawInput for non-terminal UIs. const call = harness.updates.find(u => u.sessionUpdate === 'tool_call') expect(call).toMatchObject({ toolCallId: 'c1', - title: 'Print a greeting — echo hello', + title: 'echo hello', kind: 'execute', rawInput: 'echo hello', status: 'in_progress', }) + if (call?.sessionUpdate !== 'tool_call') throw new Error('expected a tool_call') + // Capability OFF: the description renders as the only content block (no terminal block). + expect(call.content).toEqual([{ type: 'content', content: { type: 'text', text: 'Print a greeting' } }]) // presentResult: the REAL command output, wrapped in a fenced console block. const update = harness.updates.find(u => u.sessionUpdate === 'tool_call_update') expect(update?.sessionUpdate).toBe('tool_call_update') @@ -115,11 +119,12 @@ describe('acp bridge — turn outcomes', () => { expect((update as { _meta?: unknown })._meta).toBeUndefined() }) - it('with the terminal_output capability ON, a real bash call renders as a TERMINAL card (content + _meta)', async () => { + it('with the terminal_output capability ON, a real bash call renders as a TERMINAL card (content + _meta + exit)', async () => { // Drive the REAL bash tool, and advertise the Zed `_meta.terminal_output` - // capability in initialize. The bridge must then emit the terminal CARD: a - // terminal content block + `_meta.terminal_info` (cwd header) on the call, - // and `_meta.terminal_output`/`terminal_exit` on the result. + // capability in initialize. The bridge must then emit the terminal CARD: the + // description content block THEN a terminal content block + `_meta.terminal_info` + // (cwd header) on the call, and `_meta.terminal_output`/`terminal_exit` on the + // result — and OMIT the update's text content (it would clobber the card). harness = await makeBridgeHarness({ storageDir, withBash: true, @@ -131,18 +136,27 @@ describe('acp bridge — turn outcomes', () => { const call = harness.updates.find(u => u.sessionUpdate === 'tool_call') if (call?.sessionUpdate !== 'tool_call') throw new Error('expected a tool_call') - // A terminal content block keyed by the callId, and terminal_info with the + // The description content block FIRST (renders above the card), then a + // terminal content block keyed by the callId; terminal_info carries the // session cwd (the bridge fills it from the session header). - expect(call.content).toEqual([{ type: 'terminal', terminalId: 'c1' }]) + expect(call.content).toEqual([ + { type: 'content', content: { type: 'text', text: 'Greet' } }, + { type: 'terminal', terminalId: 'c1' }, + ]) expect((call._meta as { terminal_info?: unknown }).terminal_info).toEqual({ terminal_id: 'c1', cwd: process.cwd() }) const update = harness.updates.find(u => u.sessionUpdate === 'tool_call_update') if (update?.sessionUpdate !== 'tool_call_update') throw new Error('expected a tool_call_update') - // Output rides on _meta.terminal_output; the text content is still present - // as the fallback for a UI that ignores the _meta. - const meta = update._meta as { terminal_output?: { terminal_id: string; data: string } } - expect(meta.terminal_output?.terminal_id).toBe('c1') - expect(meta.terminal_output?.data).toBe('hi') + // In terminal mode the text content is OMITTED (a tool_call_update.content + // REPLACES the call's content — it would clobber the terminal block). + expect(update.content).toBeUndefined() + // Output rides on _meta.terminal_output; the parsed exit on _meta.terminal_exit. + const meta = update._meta as { + terminal_output?: { terminal_id: string; data: string } + terminal_exit?: { terminal_id: string; exit_code?: number; signal?: string } + } + expect(meta.terminal_output).toEqual({ terminal_id: 'c1', data: 'hi\n' }) + expect(meta.terminal_exit).toEqual({ terminal_id: 'c1', exit_code: 0 }) }) it('a throwing tool presenter does not break the turn: the bridge falls back generically', async () => { diff --git a/packages/tool-bash/README.md b/packages/tool-bash/README.md index 3068bc34dd..39e56a6126 100644 --- a/packages/tool-bash/README.md +++ b/packages/tool-bash/README.md @@ -34,7 +34,7 @@ The owning agent is recorded per task id at spawn and kept for the lifetime of t ## UI presentation -These tools own how their calls render in a UI (an editor's tool-call card) via the `dsh-tools` `presentCall`/`presentResult` seam — a UI never special-cases tool names. For `bash`: the **title** is the model-written `description` followed by the exact `command` ("List files in src — ls -la src"), `kind` is `execute` (terminal/run treatment), and the `command` is ALSO the **rawInput**. Why both in the title: an execute-kind card hides `rawInput` (Zed renders it only for non-terminal tools), so the command must ride in the always-visible title to be seen — the reference ACP adapters (claude-agent-acp, codex-acp) likewise put the command in an execute tool's title. The completed output is wrapped in a fenced ` ```console ` block — a UI-only affordance, so the model-facing result text stays unfenced. `bash` also flags itself as a **terminal** (the neutral `terminal` field on its presentation: `presentCall` sets a `cwd` from an explicit absolute `workdir`, else leaves it for the UI bridge to fill from the session cwd; `presentResult` carries the output) so a capable client (Zed) renders a terminal card instead of the text block — see `packages/acp` ("Terminal card"). `bash_output`/`bash_kill` present a task-scoped title ("Read output from background task bash-3" / "Kill background task bash-3") with the task id as rawInput. These methods are pure/display-only (they also run on `session/load` replay), and a malformed/older logged arg shape falls back to a generic presentation rather than throwing. See `packages/tools` ("Tool-owned UI presentation") and `packages/acp` ("Terminal card" / "Tool-call presentation"). +These tools own how their calls render in a UI (an editor's tool-call card) via the `dsh-tools` `presentCall`/`presentResult` seam — a UI never special-cases tool names. For `bash`: the **title** is the exact `command` ("ls -la src") and `kind` is `execute` (terminal/run treatment), matching the reference ACP adapters (claude-agent-acp, codex-acp), which both use the bare command as an execute tool's title. The command is ALSO the **rawInput** for non-terminal UIs that render it (an execute-kind card hides rawInput — Zed shows it only for non-terminal tools — so the command must BE the title to be seen). The model-written `description` rides as a **content** text block shown ABOVE the card (a terminal card has no description slot, so it sits over the command; claude-agent-acp likewise surfaces the description as a separate content block). The completed output is wrapped in a fenced ` ```console ` block as the no-terminal-capability fallback — a UI-only affordance, so the model-facing result text stays unfenced. `bash` also flags itself as a **terminal** (the neutral `terminal` field: `presentCall` sets a `cwd` from the model `workdir` when given — absolute as-is, relative for the UI bridge to resolve against the session cwd — else leaves it for the bridge to fill from the session cwd; `presentResult` carries the raw output plus the parsed `exitCode`/`signal`) so a capable client (Zed) renders a terminal card with an exit-status pill instead of the text block — see `packages/acp` ("Terminal card"). `bash_output`/`bash_kill` present a task-scoped title ("Read output from background task bash-3" / "Kill background task bash-3") with the task id as rawInput. These methods are pure/display-only (they also run on `session/load` replay), and a malformed/older logged arg shape falls back to a generic presentation rather than throwing. See `packages/tools` ("Tool-owned UI presentation") and `packages/acp` ("Terminal card" / "Tool-call presentation"). ## Background completion notices diff --git a/packages/tool-bash/src/index.ts b/packages/tool-bash/src/index.ts index 908fdf274d..f48b1f7a08 100644 --- a/packages/tool-bash/src/index.ts +++ b/packages/tool-bash/src/index.ts @@ -132,51 +132,76 @@ export function renderResult(result: BashRunResult): string { // --------------------------------------------------------------------------- /** - * Pending-state presentation for a `bash` call. The title is the model-written - * `description` followed by the exact `command` ("List files — ls -la src"): - * `kind: 'execute'` gets a terminal/run treatment in a UI, but an execute-kind - * card HIDES `rawInput` (Zed: `should_show_raw_input = !is_terminal_tool`), so - * the command MUST ride in the always-visible title to be seen — the reference - * ACP adapters (claude-agent-acp, codex-acp) likewise put the command in the - * title for execute tools. The description leads (a readable summary the schema - * requires); the command follows so the verbatim text is still there. `rawInput` - * still carries the bare command for non-execute UIs that DO render it. + * Pending-state presentation for a `bash` call. The TITLE is the exact `command` + * — a `kind: 'execute'` card is rendered as a terminal whose header label IS the + * title, and an execute-kind card HIDES `rawInput` (Zed: `should_show_raw_input + * = !is_terminal_tool`), so the command must BE the title to be seen. This + * mirrors the reference ACP adapters (claude-agent-acp, codex-acp), which both + * use the bare command as an execute tool's title. The model-written + * `description` (a readable summary) rides as a `content` text block shown ABOVE + * the card, since a terminal card has no description slot — claude-agent-acp + * likewise surfaces its description as a separate content block. `rawInput` still + * carries the bare command for non-execute UIs that DO render it. * - * `terminal` marks the call so a capable UI renders a TERMINAL card. The cwd - * header comes from an explicit absolute model `workdir` when given; otherwise - * the call ran in the session workspace, which this PURE presenter (args only, - * no `exec`) can't see — the UI bridge fills that default from the session's own - * cwd. An empty `terminal: {}` still flags "this is a terminal". + * `terminal` marks the call so a capable UI renders a TERMINAL card. Its `cwd` + * (header) is the model `workdir` when given — ABSOLUTE as-is, RELATIVE for the + * UI bridge to resolve against the session cwd; when omitted entirely the bridge + * fills the session workspace cwd (this PURE presenter, args only, can't see it). */ function presentBashCall(args: { command: string; description: string; workdir?: string }): ToolCallPresentation { - const cwd = args.workdir !== undefined && isAbsolute(args.workdir) ? args.workdir : undefined return { - title: `${args.description} — ${args.command}`, + title: args.command, kind: 'execute', rawInput: args.command, - terminal: cwd !== undefined ? { cwd } : {}, + content: [{ type: 'text', text: args.description }], + terminal: args.workdir !== undefined ? { cwd: args.workdir } : {}, } } /** * Completed-state presentation for a `bash` call. Two parallel renderings of the * same output: `terminal.output` for a UI that shows a terminal card (the run's - * stdout/stderr + status markers, exactly as the model sees them — it already - * carries the `[exit code: N]` marker), and a fenced ```console `content` block - * as the fallback for a UI without terminal support (the fences are a UI-only - * affordance, so they live here, not in `renderResult`). A non-text result - * (unexpected for bash) falls through to `undefined` (UI keeps the raw result). + * stdout/stderr + status markers, exactly as the model sees them — the RAW text, + * newlines preserved, since a terminal renderer relies on exact bytes), and a + * fenced ```console `content` block as the fallback for a UI without terminal + * support (the fences are a UI-only affordance, so they live here, not in the + * model-facing result; the fenced body is trimmed of trailing blank lines for a + * tidy block). A capable UI also gets an exit-status pill from `terminal.exitCode` + * / `terminal.signal`, parsed from the status markers `renderResult` appended + * (this parse is the exact inverse of those markers — they co-evolve in this + * file and a round-trip test guards the pair). A non-text result (unexpected for + * bash) falls through to `undefined` (UI keeps the raw result). */ function presentBashResult(_args: unknown, result: ToolResult): ToolResultPresentation | undefined { const block = result.content.length === 1 ? result.content[0] : undefined if (block === undefined || block.type !== 'text') return undefined - const text = block.text.replace(/\n+$/, '') + const raw = block.text + const fenced = raw.replace(/\n+$/, '') return { - content: [{ type: 'text', text: `\`\`\`console\n${text}\n\`\`\`` }], - terminal: { output: text }, + content: [{ type: 'text', text: `\`\`\`console\n${fenced}\n\`\`\`` }], + terminal: { output: raw, ...parseExitStatus(raw) }, } } +/** + * Recover the structured exit status from a rendered `renderResult` string — the + * inverse of the status markers it appends. A `[killed by signal: SIG]` marker + * yields `{signal}`; otherwise an `[exit code: N]` marker yields `{exitCode:N}`; + * a clean run appends neither, so absent both we report `{exitCode:0}`. (A + * trapped-timeout run that exits 0 has no signal/exit marker either and reads as + * exitCode 0, which is accurate — it did exit 0.) `renderResult` always appends + * the exit/signal marker LAST (after any timeout marker) onto a non-empty body, + * so the marker is anchored at end-of-string here — output that merely CONTAINS + * such text earlier is not mistaken for it. + */ +function parseExitStatus(text: string): { exitCode: number } | { signal: string } { + const signal = /\[killed by signal: ([^\]\n]+)\]$/.exec(text) + if (signal?.[1] !== undefined) return { signal: signal[1] } + const exit = /\[exit code: (\d+)\]$/.exec(text) + if (exit?.[1] !== undefined) return { exitCode: Number(exit[1]) } + return { exitCode: 0 } +} + /** Pending-state presentation for `bash_output`/`bash_kill` (background-task tools). */ function presentTaskCall(verb: string, args: { task_id: string }): ToolCallPresentation { return { title: `${verb} background task ${args.task_id}`, kind: 'execute', rawInput: args.task_id } diff --git a/packages/tool-bash/tests/tools.spec.ts b/packages/tool-bash/tests/tools.spec.ts index a5a5cdb58f..06eb16ac07 100644 --- a/packages/tool-bash/tests/tools.spec.ts +++ b/packages/tool-bash/tests/tools.spec.ts @@ -564,34 +564,74 @@ describe('status lines', () => { }) describe('tool-owned UI presentation (presentCall / presentResult)', () => { - it('bash presentCall: title is "description — command", marks a terminal; explicit absolute workdir → cwd header', async () => { + it('bash presentCall: title is the command, description as a content block, marks a terminal; workdir → cwd (absolute or relative, bridge resolves)', async () => { const ctx = await setup() // No explicit workdir → the call still flags a terminal, but with no cwd (the // UI bridge fills the session cwd it owns; the pure presenter can't see it). + // The command is the title (an execute card hides rawInput); the description + // rides as a content text block (shown above the terminal card). expect(ctx.tools.get('bash')?.presentCall?.({ command: 'ls -la src', description: 'List files in src' })) - .toEqual({ title: 'List files in src — ls -la src', kind: 'execute', rawInput: 'ls -la src', terminal: {} }) - // An explicit ABSOLUTE workdir is surfaced as the terminal cwd header. + .toEqual({ title: 'ls -la src', kind: 'execute', rawInput: 'ls -la src', content: [{ type: 'text', text: 'List files in src' }], terminal: {} }) + // An ABSOLUTE workdir is surfaced verbatim as the terminal cwd header. expect(ctx.tools.get('bash')?.presentCall?.({ command: 'pwd', description: 'Print dir', workdir: '/tmp/x' })) - .toEqual({ title: 'Print dir — pwd', kind: 'execute', rawInput: 'pwd', terminal: { cwd: '/tmp/x' } }) - // A RELATIVE workdir is not an absolute cwd → omitted (terminal still flagged). + .toEqual({ title: 'pwd', kind: 'execute', rawInput: 'pwd', content: [{ type: 'text', text: 'Print dir' }], terminal: { cwd: '/tmp/x' } }) + // A RELATIVE workdir is passed through AS-IS (the bridge resolves it against + // the session cwd, matching where execution runs) — not dropped. expect(ctx.tools.get('bash')?.presentCall?.({ command: 'pwd', description: 'Print dir', workdir: 'sub' })) - .toEqual({ title: 'Print dir — pwd', kind: 'execute', rawInput: 'pwd', terminal: {} }) + .toEqual({ title: 'pwd', kind: 'execute', rawInput: 'pwd', content: [{ type: 'text', text: 'Print dir' }], terminal: { cwd: 'sub' } }) }) - it('bash presentResult: console-block content AND terminal.output (both renderings of the run)', async () => { + it('bash presentResult: console-block content AND terminal.output (RAW newlines) + parsed exit code', async () => { const ctx = await setup() const present = ctx.tools.get('bash')!.presentResult!( { command: 'echo hi', description: 'echo' }, { content: [{ type: 'text', text: 'hi\n[exit code: 0]\n\n' }], isError: false }, ) - // Trailing blank lines trimmed; content is the fenced ```console fallback, - // terminal.output is the same text for a capable terminal card. + // The fenced ```console content trims trailing blank lines for a tidy block; + // terminal.output keeps the RAW bytes (newlines intact) a terminal renderer + // needs; exitCode is parsed back from the [exit code: N] marker. expect(present).toEqual({ content: [{ type: 'text', text: '```console\nhi\n[exit code: 0]\n```' }], - terminal: { output: 'hi\n[exit code: 0]' }, + terminal: { output: 'hi\n[exit code: 0]\n\n', exitCode: 0 }, }) }) + it('bash presentResult: a non-zero exit and a signal kill parse into exitCode / signal', async () => { + const ctx = await setup() + const args = { command: 'x', description: 'x' } + const nonzero = ctx.tools.get('bash')!.presentResult!(args, { content: [{ type: 'text', text: 'oops\n[exit code: 3]' }], isError: false }) + expect(nonzero?.terminal).toEqual({ output: 'oops\n[exit code: 3]', exitCode: 3 }) + const killed = ctx.tools.get('bash')!.presentResult!(args, { content: [{ type: 'text', text: 'gone\n[killed by signal: SIGKILL]' }], isError: false }) + expect(killed?.terminal).toEqual({ output: 'gone\n[killed by signal: SIGKILL]', signal: 'SIGKILL' }) + }) + + it('bash presentResult exit parse is the inverse of renderResult markers (round-trip)', async () => { + const ctx = await setup() + const present = ctx.tools.get('bash')! + // For each renderResult outcome, the rendered text fed back through + // presentResult recovers the matching structured exit — the parse and the + // marker emission co-evolve in one file, so this pins the pair. + const base = { + aborted: false, + timeoutMs: 1000, + stdout: { text: 'out', truncated: false }, + stderr: { text: '', truncated: false }, + } + const cases = [ + { result: { ...base, exitCode: 0, signal: null, timedOut: false }, expect: { exitCode: 0 } }, + { result: { ...base, exitCode: 7, signal: null, timedOut: false }, expect: { exitCode: 7 } }, + { result: { ...base, exitCode: null, signal: 'SIGTERM' as const, timedOut: false }, expect: { signal: 'SIGTERM' } }, + // A trapped-timeout run that exits 0 has no signal/exit marker → reads as exit 0 (it did exit 0). + { result: { ...base, exitCode: 0, signal: null, timedOut: true }, expect: { exitCode: 0 } }, + ] + for (const c of cases) { + const rendered = renderResult(c.result) + const out = present.presentResult!({ command: 'x', description: 'x' }, { content: [{ type: 'text', text: rendered }], isError: false }) + const { output: _o, ...exit } = out?.terminal ?? {} + expect(exit).toEqual(c.expect) + } + }) + it('bash presentResult: leaves a non-text (unexpected) result untouched → undefined (UI keeps raw content)', async () => { const ctx = await setup() const present = ctx.tools.get('bash')!.presentResult!( diff --git a/packages/tools/README.md b/packages/tools/README.md index d15b1d1687..6b1634cd70 100644 --- a/packages/tools/README.md +++ b/packages/tools/README.md @@ -72,8 +72,8 @@ See `defineTool`, `validateArgs`, `ToolArgsError`, `SchemaSpec`, `InferArgs`, an A tool owns how ITS calls render in a UI (an editor's tool-call card, a CLI log line) — a UI plugin must NOT special-case tool names. A `ToolDefinition` may declare two optional, pure, display-only methods: -- `presentCall(args): ToolCallPresentation | undefined` — the PENDING state: a human-readable `title` (always-visible label), an optional `kind` (`read`/`edit`/`execute`/… for icon/treatment, default `other`), an optional `rawInput` (the salient input to show in a detail view — e.g. a shell command as a string, NOT the whole args object), and an optional `terminal` (a neutral `{ cwd? }` asking a capable UI to render this call as a TERMINAL, e.g. for `bash`). -- `presentResult(args, result): ToolResultPresentation | undefined` — the COMPLETED state, given the same `args` and the `{ content, isError }` result: an optional replacement `title`, reformatted `content` (e.g. wrap command output in a fenced ` ```console ` block — a UI-only affordance that must NOT appear in the model-facing `execute` result), and an optional `terminal` (the `{ output? }` for a terminal-rendered call). The `ToolTerminal` shape is provider-neutral; a UI bridge (the ACP bridge) maps it to a terminal card and a UI that can't ignores it and uses `content`. +- `presentCall(args): ToolCallPresentation | undefined` — the PENDING state: a human-readable `title` (always-visible label), an optional `kind` (`read`/`edit`/`execute`/… for icon/treatment, default `other`), an optional `rawInput` (the salient input to show in a detail view — e.g. a shell command as a string, NOT the whole args object), an optional `content` (UI content shown alongside the title/card — e.g. a bash `description` as a text block above the terminal card), and an optional `terminal` (a neutral `{ cwd? }` asking a capable UI to render this call as a TERMINAL, e.g. for `bash`). +- `presentResult(args, result): ToolResultPresentation | undefined` — the COMPLETED state, given the same `args` and the `{ content, isError }` result: an optional replacement `title`, reformatted `content` (e.g. wrap command output in a fenced ` ```console ` block — a UI-only affordance that must NOT appear in the model-facing `execute` result), and an optional `terminal` (the `{ output?, exitCode?, signal? }` for a terminal-rendered call). The `ToolTerminal` shape is provider-neutral; a UI bridge (the ACP bridge) maps it to a terminal card (with an exit-status pill) and a UI that can't ignores it and uses `content`. Returning `undefined` (or omitting a method) tells a UI to fall back to a generic presentation (title = tool name, raw args as input, raw result content). Both methods must be **pure and side-effect-free**: a UI may call them during live streaming AND during a session-log replay, so they depend only on their arguments. With `defineTool`, `args` is the typed `InferArgs` shape; the helper soft-validates before calling (a malformed/older logged arg shape yields `undefined` rather than throwing, since display must never crash a replay). The shapes are provider-neutral — the ACP bridge (`dsh-acp`) maps them to ACP `tool_call`/`tool_call_update` wire fields, and `dsh-tool-bash` is the reference implementation. @@ -90,8 +90,8 @@ const bash = defineTool({ async execute(args) { return [{ type: 'text', text: `ran: ${args.command}` }] }, - // The model-written description is the readable title; the command is the detail. - presentCall: args => ({ title: args.description, kind: 'execute', rawInput: args.command }), + // The command is the readable title; the description rides as a content block. + presentCall: args => ({ title: args.command, kind: 'execute', rawInput: args.command, content: [{ type: 'text', text: args.description }] }), // Wrap the output as a console block for the UI (not in the model-facing result). presentResult: (_args, result) => { const block = result.content.length === 1 ? result.content[0] : undefined diff --git a/packages/tools/src/index.ts b/packages/tools/src/index.ts index 4de993b5c9..fad513fa52 100644 --- a/packages/tools/src/index.ts +++ b/packages/tools/src/index.ts @@ -83,6 +83,16 @@ export interface ToolCallPresentation { * unless that is genuinely what a reader wants. */ rawInput?: unknown + /** + * UI-facing content to show on the PENDING call alongside the title/card — + * harness {@link ContentBlock}s, in render order. A terminal tool uses this to + * surface its human-readable `description` as a text block ABOVE the terminal + * card (the card itself is requested via {@link terminal} and labelled by the + * command in `title`), since the card has no description slot. Omit to show no + * extra content. A UI maps these to its own content blocks and renders a + * {@link terminal} block (if any) as a terminal card. + */ + content?: ContentBlock[] /** * Ask a capable UI to render this call as a TERMINAL (a command running in a * working directory), not a generic tool card — set by a tool whose call IS a @@ -96,16 +106,33 @@ export interface ToolCallPresentation { /** * A request to render a tool call as a terminal. The pending presentation * supplies the working directory; the result presentation (see - * {@link ToolResultPresentation.terminal}) supplies the captured output. - * Provider-neutral — no client-protocol types. A UI that supports terminals - * shows a cwd-headed terminal card with the command and its output; a UI that - * does not ignores this and renders the ordinary card/content. + * {@link ToolResultPresentation.terminal}) supplies the captured output and exit + * status. Provider-neutral — no client-protocol types. A UI that supports + * terminals shows a cwd-headed terminal card with the command, its output, and + * an exit-status pill; a UI that does not ignores this and renders the ordinary + * card/content. */ export interface ToolTerminal { - /** Absolute working directory the command ran in, shown as the terminal header. Omit if unknown. */ + /** + * Working directory the command ran in, shown as the terminal header. An + * ABSOLUTE path is used as-is; a RELATIVE path is resolved by the UI bridge + * against the session workspace (the pure tool presenter can't see the + * session cwd). Omit entirely to let the bridge use the session workspace. + */ cwd?: string /** Captured command output (stdout+stderr as the tool chooses to combine them). Result-state only. */ output?: string + /** + * Process exit code, when the run ended by exiting (not a signal). Result-state + * only; lets a capable UI show an exit-status pill on the terminal card. Omit + * when the command was killed by a signal or the exit code is unknown. + */ + exitCode?: number + /** + * Signal name that killed the process (e.g. `SIGTERM`), when it died by signal + * rather than exiting. Result-state only; mutually exclusive with `exitCode`. + */ + signal?: string } /** From 9f6b96c55506648bf4e9d7233fefe72db2010c6c Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Thu, 18 Jun 2026 19:35:15 +0800 Subject: [PATCH 08/10] fix(acp): address review of the terminal-card alignment (exit parse, background/error, capability snapshot) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex + an independent review pass found three real defects in the prior commit: 1. parseExitStatus could misreport a SUCCESSFUL command as a failure: a clean exit 0 appends no marker, so output ending in "[exit code: 5]" (no trailing newline) was read as the marker. Anchor the parse to a LEADING newline — renderResult always inserts one before a real marker, so a body that merely ends in marker-like text no longer matches. A narrow residual (a clean exit 0 whose final line is exactly the marker) is inherent to the replay-only-sees- text design and documented; the complete fix (a structured exit on the event) is the RFC's named escape hatch. 2. A run_in_background start and an isError result were rendered as exited terminal cards with a false exit-0 pill. A background start returns a task-id ack (not a streamed terminal) and is no longer marked terminal; an isError result (spawn failure / abort) carries no exit pill. 3. The terminal capability was re-read live on the result path, so a second initialize between a call and its result could desync them (orphan terminal_output or clobbered card). Snapshot the capability per session at creation (SessionRecord.terminalEnabled) so call and result always agree. Also reword the reference-parity claim: keeping the description as a content block in terminal mode is a DELIBERATE divergence (claude-agent-acp drops it). Tests added for each; with-key e2e still green. --- ...6-06-18-acp-terminal-and-tool-rendering.md | 2 +- packages/acp/src/index.ts | 25 +++++- packages/acp/tests/turns.spec.ts | 28 ++++++ packages/tool-bash/README.md | 2 +- packages/tool-bash/src/index.ts | 88 +++++++++++++------ packages/tool-bash/tests/tools.spec.ts | 42 +++++++++ 6 files changed, 152 insertions(+), 35 deletions(-) diff --git a/docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md b/docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md index d7b0ee12f1..3ff014f55f 100644 --- a/docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md +++ b/docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md @@ -6,7 +6,7 @@ Status: implemented The ACP bridge lets each tool own its call rendering via `presentCall`/`presentResult` (see [tool-call UI presentation](../proposed/2026-06-14-acp-agent-client-protocol.md) and `packages/tools`). For `bash` we surface the exact command as the `tool_call` title, the model's `description` as a content text block, `kind: 'execute'`, and the completed output wrapped in a fenced ` ```console ` text block. -That is a correct, capability-free baseline, but not how the reference editors render a *terminal* tool at its best. An editor like Zed has a dedicated terminal tool-call card — a header showing the working directory, the command as the label, the command output rendered as a terminal, and an exit-status pill — but it only builds that card when the `tool_call` carries terminal metadata (below). With a plain text block the output appears as static markdown and there is no cwd header. (Zed also HIDES `rawInput` for `kind: 'execute'`, which is why the command IS the title — both reference adapters do the same — and the human-readable description rides as a separate content block, since a terminal card has no description slot.) +That is a correct, capability-free baseline, but not how the reference editors render a *terminal* tool at its best. An editor like Zed has a dedicated terminal tool-call card — a header showing the working directory, the command as the label, the command output rendered as a terminal, and an exit-status pill — but it only builds that card when the `tool_call` carries terminal metadata (below). With a plain text block the output appears as static markdown and there is no cwd header. (Zed also HIDES `rawInput` for `kind: 'execute'`, which is why the command IS the title — both reference adapters do the same. The human-readable description rides as a separate content block above the card; note this is a DELIBERATE divergence — claude-agent-acp DROPS the description in terminal mode and renders only the card — we keep the summary visible alongside.) ## Key finding: agent-executed terminals use a `_meta` convention, NOT `terminal/create` diff --git a/packages/acp/src/index.ts b/packages/acp/src/index.ts index 20d77f9a20..84c1b585ed 100644 --- a/packages/acp/src/index.ts +++ b/packages/acp/src/index.ts @@ -141,6 +141,17 @@ interface SessionRecord { * tool state. */ presenter: ToolPresenter + /** + * Whether THIS session renders shell tools as terminal cards — snapshotted + * from the client's `_meta.terminal_output` capability at session creation + * (`session/new`/`session/load`), NOT re-read live. A capability snapshot per + * session means the `tool_call` (which registers the terminal) and the matching + * `tool_call_update` (which streams its output) ALWAYS agree, even if a later + * `initialize` mutates the connection-level capability between them — otherwise + * a re-`initialize` mid-call could orphan a `terminal_output` (call non-terminal, + * result terminal) or clobber the card (call terminal, result non-terminal). + */ + terminalEnabled: boolean /** * The in-flight `session/prompt`, or `undefined` when none is pending. A * prompt resolves with a {@link StopReason} or rejects with an Error (a @@ -290,7 +301,7 @@ export function apply(ctx: Context, config: AcpConfig): void { const rec = sessions.get(session.header.id) if (rec === undefined) return streamSessionEventUpdate(rec.sessionId, event, notify, rec.presenter, { - enabled: terminalOutputCap, + enabled: rec.terminalEnabled, cwd: session.header.cwd, }) const inflight = rec.inflight @@ -422,7 +433,7 @@ export function apply(ctx: Context, config: AcpConfig): void { agentOptions: agentOptions(config), }) bySession.set(agent, sessionId) - sessions.set(sessionId, { sessionId, agent, presenter: makePresenter(), inflight: undefined }) + sessions.set(sessionId, { sessionId, agent, presenter: makePresenter(), terminalEnabled: terminalOutputCap, inflight: undefined }) return Promise.resolve({ sessionId }) }, @@ -475,7 +486,13 @@ 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: makePresenter(), inflight: undefined } + // Snapshot the terminal capability ONCE for this session (used by both + // the replay below and the post-load live stream) so a later + // `initialize` can't desync the call/result of a tool card. + const terminalEnabled = terminalOutputCap + const record: SessionRecord = { + sessionId: params.sessionId, agent, presenter: makePresenter(), terminalEnabled, 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 @@ -492,7 +509,7 @@ export function apply(ctx: Context, config: AcpConfig): void { // so the record's presenter starts clean for the post-load live stream. const replayPresenter = makePresenter() const replayTerminal: TerminalRendering = { - enabled: terminalOutputCap, + enabled: terminalEnabled, cwd: agent.session.header.cwd, } for (const event of agent.session.events) { diff --git a/packages/acp/tests/turns.spec.ts b/packages/acp/tests/turns.spec.ts index 3cf12a67e8..19c3be2556 100644 --- a/packages/acp/tests/turns.spec.ts +++ b/packages/acp/tests/turns.spec.ts @@ -159,6 +159,34 @@ describe('acp bridge — turn outcomes', () => { expect(meta.terminal_exit).toEqual({ terminal_id: 'c1', exit_code: 0 }) }) + it('the terminal capability is snapshotted per-session: a later initialize cannot desync a call/result', async () => { + // The session is created with the capability ON. A SECOND initialize then + // turns it OFF at the connection level — but this session keeps its snapshot, + // so its bash call STILL renders as a terminal card (call + result agree). + // Without the snapshot, the result path would re-read the now-OFF capability + // and either clobber the card (content sent) or be inconsistent with the call. + harness = await makeBridgeHarness({ + storageDir, + withBash: true, + script: [toolCallResponse('c1', 'bash', { command: 'echo hi', description: 'Greet' }), textResponse('done')], + }) + await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: { _meta: { terminal_output: true } } }) + const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] }) + // A re-initialize that DROPS the capability after the session exists. + await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) + await harness.client.prompt({ sessionId, prompt: [{ type: 'text', text: 'greet' }] }) + + const call = harness.updates.find(u => u.sessionUpdate === 'tool_call') + if (call?.sessionUpdate !== 'tool_call') throw new Error('expected a tool_call') + // Still a terminal card (the session's snapshot, not the mutated connection cap). + expect((call._meta as { terminal_info?: unknown }).terminal_info).toBeDefined() + const update = harness.updates.find(u => u.sessionUpdate === 'tool_call_update') + if (update?.sessionUpdate !== 'tool_call_update') throw new Error('expected a tool_call_update') + // The result AGREES with the call: terminal output present, content omitted. + expect(update.content).toBeUndefined() + expect((update._meta as { terminal_output?: unknown }).terminal_output).toBeDefined() + }) + 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 diff --git a/packages/tool-bash/README.md b/packages/tool-bash/README.md index 39e56a6126..f5e0e1dfda 100644 --- a/packages/tool-bash/README.md +++ b/packages/tool-bash/README.md @@ -34,7 +34,7 @@ The owning agent is recorded per task id at spawn and kept for the lifetime of t ## UI presentation -These tools own how their calls render in a UI (an editor's tool-call card) via the `dsh-tools` `presentCall`/`presentResult` seam — a UI never special-cases tool names. For `bash`: the **title** is the exact `command` ("ls -la src") and `kind` is `execute` (terminal/run treatment), matching the reference ACP adapters (claude-agent-acp, codex-acp), which both use the bare command as an execute tool's title. The command is ALSO the **rawInput** for non-terminal UIs that render it (an execute-kind card hides rawInput — Zed shows it only for non-terminal tools — so the command must BE the title to be seen). The model-written `description` rides as a **content** text block shown ABOVE the card (a terminal card has no description slot, so it sits over the command; claude-agent-acp likewise surfaces the description as a separate content block). The completed output is wrapped in a fenced ` ```console ` block as the no-terminal-capability fallback — a UI-only affordance, so the model-facing result text stays unfenced. `bash` also flags itself as a **terminal** (the neutral `terminal` field: `presentCall` sets a `cwd` from the model `workdir` when given — absolute as-is, relative for the UI bridge to resolve against the session cwd — else leaves it for the bridge to fill from the session cwd; `presentResult` carries the raw output plus the parsed `exitCode`/`signal`) so a capable client (Zed) renders a terminal card with an exit-status pill instead of the text block — see `packages/acp` ("Terminal card"). `bash_output`/`bash_kill` present a task-scoped title ("Read output from background task bash-3" / "Kill background task bash-3") with the task id as rawInput. These methods are pure/display-only (they also run on `session/load` replay), and a malformed/older logged arg shape falls back to a generic presentation rather than throwing. See `packages/tools` ("Tool-owned UI presentation") and `packages/acp` ("Terminal card" / "Tool-call presentation"). +These tools own how their calls render in a UI (an editor's tool-call card) via the `dsh-tools` `presentCall`/`presentResult` seam — a UI never special-cases tool names. For `bash`: the **title** is the exact `command` ("ls -la src") and `kind` is `execute` (terminal/run treatment), matching the reference ACP adapters (claude-agent-acp, codex-acp), which both use the bare command as an execute tool's title. The command is ALSO the **rawInput** for non-terminal UIs that render it (an execute-kind card hides rawInput — Zed shows it only for non-terminal tools — so the command must BE the title to be seen). The model-written `description` rides as a **content** text block shown ABOVE the card. (claude-agent-acp DROPS the description in terminal mode and shows only the card; surfacing it as a content block is a deliberate divergence — we keep the human summary visible alongside the card.) The completed output is wrapped in a fenced ` ```console ` block as the no-terminal-capability fallback — a UI-only affordance, so the model-facing result text stays unfenced. A FOREGROUND `bash` run also flags itself as a **terminal** (the neutral `terminal` field: `presentCall` sets a `cwd` from the model `workdir` when given — absolute as-is, relative for the UI bridge to resolve against the session cwd — else leaves it for the bridge to fill from the session cwd; `presentResult` carries the raw output plus the parsed `exitCode`/`signal`) so a capable client (Zed) renders a terminal card with an exit-status pill instead of the text block — see `packages/acp` ("Terminal card"). A `run_in_background` call is NOT a terminal (it returns a task id immediately and never streams a terminal — poll with `bash_output`), and an `isError` result (spawn failure / abort) carries no exit pill (there is no real process exit); both render as the ordinary execute card / fenced text. `bash_output`/`bash_kill` present a task-scoped title ("Read output from background task bash-3" / "Kill background task bash-3") with the task id as rawInput. These methods are pure/display-only (they also run on `session/load` replay), and a malformed/older logged arg shape falls back to a generic presentation rather than throwing. See `packages/tools` ("Tool-owned UI presentation") and `packages/acp` ("Terminal card" / "Tool-call presentation"). ## Background completion notices diff --git a/packages/tool-bash/src/index.ts b/packages/tool-bash/src/index.ts index f48b1f7a08..5ea6a93a8f 100644 --- a/packages/tool-bash/src/index.ts +++ b/packages/tool-bash/src/index.ts @@ -139,23 +139,32 @@ export function renderResult(result: BashRunResult): string { * mirrors the reference ACP adapters (claude-agent-acp, codex-acp), which both * use the bare command as an execute tool's title. The model-written * `description` (a readable summary) rides as a `content` text block shown ABOVE - * the card, since a terminal card has no description slot — claude-agent-acp - * likewise surfaces its description as a separate content block. `rawInput` still - * carries the bare command for non-execute UIs that DO render it. + * the card. (Note: claude-agent-acp DROPS the description in terminal mode and + * shows only the card; surfacing it as a content block is a deliberate + * divergence here — we keep the human summary visible alongside the card.) + * `rawInput` still carries the bare command for non-execute UIs that DO render it. * - * `terminal` marks the call so a capable UI renders a TERMINAL card. Its `cwd` - * (header) is the model `workdir` when given — ABSOLUTE as-is, RELATIVE for the - * UI bridge to resolve against the session cwd; when omitted entirely the bridge - * fills the session workspace cwd (this PURE presenter, args only, can't see it). + * `terminal` marks the call so a capable UI renders a TERMINAL card — but ONLY a + * FOREGROUND run is a terminal: a `run_in_background` call returns a task id + * immediately (it never streams a terminal; its output is polled via + * `bash_output`), so it is NOT marked terminal and renders as an ordinary + * execute card. For a foreground run the `terminal.cwd` (header) is the model + * `workdir` when given — ABSOLUTE as-is, RELATIVE for the UI bridge to resolve + * against the session cwd; when omitted the bridge fills the session workspace + * cwd (this PURE presenter, args only, can't see it). */ -function presentBashCall(args: { command: string; description: string; workdir?: string }): ToolCallPresentation { - return { +type BashCallArgs = { command: string; description: string; workdir?: string; run_in_background?: boolean } + +function presentBashCall(args: BashCallArgs): ToolCallPresentation { + const base = { title: args.command, - kind: 'execute', + kind: 'execute' as const, rawInput: args.command, - content: [{ type: 'text', text: args.description }], - terminal: args.workdir !== undefined ? { cwd: args.workdir } : {}, + content: [{ type: 'text' as const, text: args.description }], } + // A background start is not an interactive terminal — no terminal card. + if (args.run_in_background === true) return base + return { ...base, terminal: args.workdir !== undefined ? { cwd: args.workdir } : {} } } /** @@ -167,37 +176,58 @@ function presentBashCall(args: { command: string; description: string; workdir?: * support (the fences are a UI-only affordance, so they live here, not in the * model-facing result; the fenced body is trimmed of trailing blank lines for a * tidy block). A capable UI also gets an exit-status pill from `terminal.exitCode` - * / `terminal.signal`, parsed from the status markers `renderResult` appended - * (this parse is the exact inverse of those markers — they co-evolve in this - * file and a round-trip test guards the pair). A non-text result (unexpected for - * bash) falls through to `undefined` (UI keeps the raw result). + * / `terminal.signal`, parsed from the status markers `renderResult` appended. + * + * Terminal output/exit is suppressed for results that are NOT a finished + * foreground run: a `run_in_background` start (`isBackground` — the text is a + * task-id ack, not a streamed run) and an `isError` result (a spawn failure or + * abort — there is no real process exit to pill, and the body is an error + * message, not `renderResult` output, so parsing it would be meaningless). Those + * fall back to the fenced `content` block with no terminal metadata. The bridge's + * orphan guard also drops a result terminal when the call wasn't terminal, so a + * background call (not marked terminal in `presentBashCall`) is doubly safe. + * A non-text result (unexpected for bash) falls through to `undefined`. */ -function presentBashResult(_args: unknown, result: ToolResult): ToolResultPresentation | undefined { +function presentBashResult(args: unknown, result: ToolResult): ToolResultPresentation | undefined { const block = result.content.length === 1 ? result.content[0] : undefined if (block === undefined || block.type !== 'text') return undefined const raw = block.text const fenced = raw.replace(/\n+$/, '') - return { - content: [{ type: 'text', text: `\`\`\`console\n${fenced}\n\`\`\`` }], - terminal: { output: raw, ...parseExitStatus(raw) }, - } + const content = [{ type: 'text' as const, text: `\`\`\`console\n${fenced}\n\`\`\`` }] + const isBackground = typeof args === 'object' && args !== null && (args as { run_in_background?: unknown }).run_in_background === true + // No exit pill / terminal output for a background ack or an errored run. + if (isBackground || result.isError) return { content } + return { content, terminal: { output: raw, ...parseExitStatus(raw) } } } /** * Recover the structured exit status from a rendered `renderResult` string — the * inverse of the status markers it appends. A `[killed by signal: SIG]` marker * yields `{signal}`; otherwise an `[exit code: N]` marker yields `{exitCode:N}`; - * a clean run appends neither, so absent both we report `{exitCode:0}`. (A - * trapped-timeout run that exits 0 has no signal/exit marker either and reads as - * exitCode 0, which is accurate — it did exit 0.) `renderResult` always appends - * the exit/signal marker LAST (after any timeout marker) onto a non-empty body, - * so the marker is anchored at end-of-string here — output that merely CONTAINS - * such text earlier is not mistaken for it. + * absent both we report `{exitCode:0}` (a clean run appends no marker — and a + * trapped-timeout run that exits 0 also has none and is accurately exit 0). + * + * Why parse rendered text at all: `presentResult` is replay-safe and on a + * `session/load` the ONLY thing persisted is this content text — the structured + * `BashRunResult` is long gone — so unless the exit were added to the persisted + * event schema (deliberately NOT done; see the terminal-rendering RFC), parsing + * is the only channel. The match is anchored to a LEADING newline + end-of-string + * because `renderResult` always inserts a `\n` before the marker (line ~124) onto + * a non-empty body: a real marker is therefore always its own final line. That + * defeats the common spoof (program output that simply ENDS in `[exit code: 5]` + * with no trailing newline — a clean exit 0 — no longer reads as a failure). + * + * KNOWN RESIDUAL (inherent to the replay-only-sees-text design): a clean exit 0 + * whose body's FINAL line is itself exactly `\n[exit code: N]` (the program + * printed that line and nothing after) is still indistinguishable from a real + * marker and would show a wrong pill. This is display-only (execution and the + * model-facing text are unaffected) and narrow; the complete fix is to persist a + * structured exit on the result event, which the RFC names as the escape hatch. */ function parseExitStatus(text: string): { exitCode: number } | { signal: string } { - const signal = /\[killed by signal: ([^\]\n]+)\]$/.exec(text) + const signal = /\n\[killed by signal: ([^\]\n]+)\]$/.exec(text) if (signal?.[1] !== undefined) return { signal: signal[1] } - const exit = /\[exit code: (\d+)\]$/.exec(text) + const exit = /\n\[exit code: (\d+)\]$/.exec(text) if (exit?.[1] !== undefined) return { exitCode: Number(exit[1]) } return { exitCode: 0 } } diff --git a/packages/tool-bash/tests/tools.spec.ts b/packages/tool-bash/tests/tools.spec.ts index 06eb16ac07..cb54a8a60f 100644 --- a/packages/tool-bash/tests/tools.spec.ts +++ b/packages/tool-bash/tests/tools.spec.ts @@ -632,6 +632,48 @@ describe('tool-owned UI presentation (presentCall / presentResult)', () => { } }) + it('bash presentResult: a clean exit-0 whose output ENDS in marker-like text is NOT read as a failure', async () => { + const ctx = await setup() + const args = { command: 'printf "[exit code: 5]"', description: 'print' } + // A successful command can print text that looks like a marker. renderResult + // for a clean exit 0 appends NOTHING (and no trailing newline), so the body's + // own tail is `[exit code: 5]`. The parse requires a LEADING newline before + // the marker (renderResult always inserts one before a REAL marker), so this + // no-trailing-newline body is NOT mistaken for a failure → exitCode 0. + const out = ctx.tools.get('bash')!.presentResult!(args, { content: [{ type: 'text', text: '[exit code: 5]' }], isError: false }) + expect(out?.terminal).toEqual({ output: '[exit code: 5]', exitCode: 0 }) + // Same for a fake signal marker with no leading newline. + const sig = ctx.tools.get('bash')!.presentResult!(args, { content: [{ type: 'text', text: '[killed by signal: SIGKILL]' }], isError: false }) + expect(sig?.terminal).toEqual({ output: '[killed by signal: SIGKILL]', exitCode: 0 }) + }) + + it('bash presentCall/presentResult: a run_in_background call is NOT a terminal and its ack carries no exit pill', async () => { + const ctx = await setup() + // The background start returns a task-id ack, not a streamed run — no terminal. + const call = ctx.tools.get('bash')!.presentCall!({ command: 'sleep 100', description: 'wait', run_in_background: true }) + expect(call).toEqual({ title: 'sleep 100', kind: 'execute', rawInput: 'sleep 100', content: [{ type: 'text', text: 'wait' }] }) + expect((call as { terminal?: unknown }).terminal).toBeUndefined() + // The ack result is fenced text only — no terminal output / exit pill. + const result = ctx.tools.get('bash')!.presentResult!( + { command: 'sleep 100', description: 'wait', run_in_background: true }, + { content: [{ type: 'text', text: 'started background task bash-1' }], isError: false }, + ) + expect(result?.terminal).toBeUndefined() + expect(result?.content).toEqual([{ type: 'text', text: '```console\nstarted background task bash-1\n```' }]) + }) + + it('bash presentResult: an isError result carries no exit pill (no real process exit to report)', async () => { + const ctx = await setup() + // A spawn failure / abort has no process exit — the body is an error message, + // not renderResult output, so no terminal output/exit is emitted. + const out = ctx.tools.get('bash')!.presentResult!( + { command: 'x', description: 'x' }, + { content: [{ type: 'text', text: 'command aborted' }], isError: true }, + ) + expect(out?.terminal).toBeUndefined() + expect(out?.content).toEqual([{ type: 'text', text: '```console\ncommand aborted\n```' }]) + }) + it('bash presentResult: leaves a non-text (unexpected) result untouched → undefined (UI keeps raw content)', async () => { const ctx = await setup() const present = ctx.tools.get('bash')!.presentResult!( From 21855d42b74fb1765c400c614a97f8eacd9d4bd8 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Thu, 18 Jun 2026 19:45:05 +0800 Subject: [PATCH 09/10] docs(tool-bash): note the parseExitStatus residual applies to the signal marker too Review nit: the KNOWN RESIDUAL comment named only the [exit code: N] case, but the same end-of-string spoof applies to [killed by signal: SIG]. Reword to cover both markers. --- packages/tool-bash/src/index.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/tool-bash/src/index.ts b/packages/tool-bash/src/index.ts index 5ea6a93a8f..7c52705a44 100644 --- a/packages/tool-bash/src/index.ts +++ b/packages/tool-bash/src/index.ts @@ -218,11 +218,12 @@ function presentBashResult(args: unknown, result: ToolResult): ToolResultPresent * with no trailing newline — a clean exit 0 — no longer reads as a failure). * * KNOWN RESIDUAL (inherent to the replay-only-sees-text design): a clean exit 0 - * whose body's FINAL line is itself exactly `\n[exit code: N]` (the program - * printed that line and nothing after) is still indistinguishable from a real - * marker and would show a wrong pill. This is display-only (execution and the - * model-facing text are unaffected) and narrow; the complete fix is to persist a - * structured exit on the result event, which the RFC names as the escape hatch. + * whose body's FINAL line is itself exactly the marker text — `[exit code: N]` + * or `[killed by signal: SIG]`, printed by the program with nothing after — is + * still indistinguishable from a real marker and would show a wrong pill. This is + * display-only (execution and the model-facing text are unaffected) and narrow; + * the complete fix is to persist a structured exit on the result event, which the + * RFC names as the escape hatch. */ function parseExitStatus(text: string): { exitCode: number } | { signal: string } { const signal = /\n\[killed by signal: ([^\]\n]+)\]$/.exec(text) From a009b1e995d38a2149d84067bbf461f8be0e3456 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Thu, 18 Jun 2026 23:01:16 +0800 Subject: [PATCH 10/10] docs(tools): FIXME to rethink the ToolPresentation type shapes The ToolCallPresentation / ToolResultPresentation / ToolTerminal shapes grew incrementally and the responsibility split is now muddy (overlapping call/result terminal fields, the bridge stitching content + terminal + rawInput per call). Flag it as a release-blocking FIXME to redesign around a tool's render INTENT (a tagged union over card kinds) and pin it in an RFC before more tools/UIs depend on the current bag-of-optionals. --- packages/tools/src/index.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/packages/tools/src/index.ts b/packages/tools/src/index.ts index fad513fa52..5730b778c7 100644 --- a/packages/tools/src/index.ts +++ b/packages/tools/src/index.ts @@ -58,6 +58,18 @@ declare module 'cordis' { */ export type ToolCallKind = 'read' | 'edit' | 'delete' | 'move' | 'search' | 'execute' | 'fetch' | 'other' +// FIXME(tool-presentation): the ToolCallPresentation / ToolResultPresentation / +// ToolTerminal shapes need a rethink. They grew incrementally (title/kind/ +// rawInput, then a `content` block, then a `terminal` sub-shape carrying cwd/ +// output/exit) and the split of responsibility is now muddy: the call vs result +// terminal fields overlap, the bridge has to reconcile a `content` block AND a +// `terminal` block AND `rawInput` per call, and the "pending vs completed" +// boundary doesn't cleanly map to how editors actually render (terminal card, +// diff, generic card). Before more tools/UIs depend on this, redesign the type +// so a tool declares its render INTENT once (e.g. a tagged union over card +// kinds) rather than a bag of optional fields the bridge stitches together. +// Pin the design in an RFC and migrate dsh-tool-bash + the ACP bridge together. + /** * How a tool wants ONE of its calls shown in a UI (an editor's tool-call card, * a CLI log line) BEFORE the result is known — the *pending* state. Provider-