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