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/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/docs/rfc/README.md b/docs/rfc/README.md index 6a4ddb6f05..bc56275477 100644 --- a/docs/rfc/README.md +++ b/docs/rfc/README.md @@ -55,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..3ff014f55f --- /dev/null +++ b/docs/rfc/implemented/2026-06-18-acp-terminal-and-tool-rendering.md @@ -0,0 +1,41 @@ +# 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 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. 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` + +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`. `_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 + +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}]` (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`/`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`). 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 35bef79e14..8dd8af6d01 100644 --- a/examples/acp-agent/tests/acp.e2e.ts +++ b/examples/acp-agent/tests/acp.e2e.ts @@ -174,6 +174,59 @@ 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 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. + 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 + // 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 + exit)', 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') + // 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 e528acdb45..a47a7327c2 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 @@ -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,21 @@ 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, 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 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 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: 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 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..0a4a890658 100644 --- a/packages/acp/package.json +++ b/packages/acp/package.json @@ -29,16 +29,19 @@ "@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": { "@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 18f3adb670..84c1b585ed 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, @@ -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, 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' @@ -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,24 @@ 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 + /** + * 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 @@ -182,6 +203,10 @@ export function apply(ctx: Context, config: AcpConfig): void { const agents = ctx.agents 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). @@ -198,6 +223,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 @@ -270,7 +300,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) + streamSessionEventUpdate(rec.sessionId, event, notify, rec.presenter, { + enabled: rec.terminalEnabled, + cwd: session.header.cwd, + }) const inflight = rec.inflight if (inflight === undefined) return if (event.type === 'turn/start') { @@ -366,6 +399,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 }, @@ -395,7 +433,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: makePresenter(), terminalEnabled: terminalOutputCap, inflight: undefined }) return Promise.resolve({ sessionId }) }, @@ -448,14 +486,34 @@ 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 }) + // 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 // 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. + // + // 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() + const replayTerminal: TerminalRendering = { + enabled: terminalEnabled, + cwd: agent.session.header.cwd, + } for (const event of agent.session.events) { - streamSessionEventUpdate(params.sessionId, event, notify) + streamSessionEventUpdate(params.sessionId, event, notify, replayPresenter, replayTerminal) } return {} } finally { @@ -659,6 +717,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 +732,8 @@ export function streamSessionEventUpdate( sessionId: string, event: SessionEvent, notify: (notification: SessionNotification) => void, + presenter: Pick = nullToolPresenter, + terminal: TerminalRendering = noTerminalRendering, ): void { switch (event.type) { case 'assistant/chunk': { @@ -690,27 +758,64 @@ export function streamSessionEventUpdate( return } 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 + // 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: { 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 } : {}, + ...callContent.length > 0 ? { content: callContent } : {}, + ...asTerminal + ? { _meta: { terminal_info: { terminal_id: event.data.callId, cwd: terminalCwd(present.terminal, 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, 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(event.data.content), + ...asTerminal ? {} : { content: toolResultContent(present.content) }, + ...present.title !== undefined ? { title: present.title } : {}, + ...terminalResultMeta, }, }) return @@ -722,8 +827,155 @@ 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` + * and `rawInput` are optional. + */ +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 +} + +/** 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 + /** Tool's terminal output/exit for a terminal-rendered call (the result side). */ + terminal?: ToolTerminal +} + +/** + * 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), 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() + + /** + * @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) + 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). 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 } : {}, + } + } + + /** 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) + // 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, + ...present.title !== undefined ? { title: present.title } : {}, + // 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 } : {}, + } + } +} + +/** + * 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' @@ -751,3 +1003,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/harness.ts b/packages/acp/tests/harness.ts index bee16c46eb..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 @@ -234,7 +248,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/load.spec.ts b/packages/acp/tests/load.spec.ts index 3bed1f836d..a10aca4bd4 100644 --- a/packages/acp/tests/load.spec.ts +++ b/packages/acp/tests/load.spec.ts @@ -4,7 +4,7 @@ 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 { 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 +56,79 @@ 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 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, + withBash: true, + script: [toolCallResponse('c1', 'bash', { command: 'echo hello', description: 'Print a greeting' }), 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 + + // 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: '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') + 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('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 c37de8b344..bf73fe0629 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,291 @@ 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' } }] }) + }) + + 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('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 eef0f392aa..19c3be2556 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 } @@ -75,6 +75,145 @@ describe('acp bridge — turn outcomes', () => { expect(callIdx).toBeLessThan(updIdx) }) + 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"). + // 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, + withBash: true, + script: [ + toolCallResponse('c1', 'bash', { command: 'echo hello', description: 'Print a greeting' }), + textResponse('done'), + ], + }) + const sessionId = await newSession(harness) + await harness.client.prompt({ sessionId, prompt: [{ type: 'text', text: 'greet' }] }) + + // 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: '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') + 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```') + // 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 + 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: 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, + 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') + // 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: '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') + // 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('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 + // 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/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 cf2a40f701..5a53bce17f 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. (`XXX(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 **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 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 4bc6e02650..1b01880e5a 100644 --- a/packages/tool-bash/src/index.ts +++ b/packages/tool-bash/src/index.ts @@ -39,6 +39,7 @@ 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 { Agent } from '@deepseek-ai/dsh-agent' import type { BashRunResult, BashTask, CollectedOutput } from '@deepseek-ai/dsh-bash' @@ -124,6 +125,119 @@ 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 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. (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 — 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). + */ +type BashCallArgs = { command: string; description: string; workdir?: string; run_in_background?: boolean } + +function presentBashCall(args: BashCallArgs): ToolCallPresentation { + const base = { + title: args.command, + kind: 'execute' as const, + rawInput: args.command, + 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 } : {} } +} + +/** + * 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 — 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. + * + * 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 { + 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+$/, '') + 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}`; + * 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 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) + if (signal?.[1] !== undefined) return { signal: signal[1] } + const exit = /\n\[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 } +} + /** * 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 +356,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 +382,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 +400,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 e519bd8850..46194f33d2 100644 --- a/packages/tool-bash/tests/tools.spec.ts +++ b/packages/tool-bash/tests/tools.spec.ts @@ -562,3 +562,152 @@ describe('status lines', () => { expect(text(read)).toContain('[status: completed, exit code: 0]') }) }) + +describe('tool-owned UI presentation (presentCall / presentResult)', () => { + 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: '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: '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: 'pwd', kind: 'execute', rawInput: 'pwd', content: [{ type: 'text', text: 'Print dir' }], terminal: { cwd: 'sub' } }) + }) + + 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 }, + ) + // 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]\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: 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!( + { 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..6b1634cd70 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`), 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. + +```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 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 + 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..5730b778c7 100644 --- a/packages/tools/src/index.ts +++ b/packages/tools/src/index.ts @@ -50,9 +50,156 @@ 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' + +// 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- + * 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 + /** + * 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 + * 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 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 { + /** + * 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 +} + +/** + * 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[] + /** + * 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. */ 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. */ @@ -166,14 +313,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/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..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) @@ -814,3 +847,53 @@ 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() + }) +}) 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