refactor(acp): trim unreachable bridge surface (branding knobs, kind-sniffing fallback)

Two pieces of dsh-acp surface were unreachable from any shipped config: - AcpConfig.agentName/agentVersion: the app package hands the bridge only { model, systemPrompt }, so no leaf cordis.yml could set them; they were settable only by direct-mounting the bridge (a unit test). Hardcode agentInfo at the initialize site and delete the fields, their schema defaults, the ?? fallbacks, and the TODO(double-default) whose subject vanishes. The handshake wire value is unchanged (all snapshot initialize lines byte-identical). - The toolKindFor name heuristic special-cased bash*/read*/write/edit* names in the generic-fallback path, violating the bridge's own design rule ("the bridge never special-cases tool names"). Every first-party tool ships its kind via presentCall; the fallback now renders the neutral kind 'other'. The fallback is reachable when a presentCall throws OR when model args fail the tool schema (defineTool's presentCall wrapper returns undefined on violations) — the latter shows up in one committed golden (hook-codex-posttool-block: three bash calls missing the required description), whose kind cells flip execute->other. That 3-line golden refresh is the whole transcript delta. The empty-arguments branch of parseToolArguments lost its only exercise with the deleted heuristic test; it is live behavior (JSON.parse('') throws, so the guard is what renders a zero-arg call as rawInput {}), so it gets a dedicated pin instead of deletion. RFC moved to docs/rfc/implemented/simplification/ and amended to shipped reality: fallback reachability includes schema-invalid args, and the golden churn is exactly the three kind cells (the original zero-churn claim held only for the branding half).
2026-07-04 15:46:11 +08:00
parent 226a8b5e4c
commit 4036300353
10 changed files with 62 additions and 80 deletions
--- a/packages/ui/acp/README.md
+++ b/packages/ui/acp/README.md
@@ -16,8 +16,8 @@ It is a **client-driver / UI plugin**, the structured analogue of the readline `
 |---|---|---|
 | `model` | — | Model name for created agents (must have a registered adapter). |
 | `systemPrompt` | — | Per-agent system prompt. |
-| `agentName` | `deepseek-harness-acp` | Server name reported in `initialize`. |
-| `agentVersion` | `0.0.1` | Server version reported in `initialize`. |
+
+The `initialize` handshake reports a fixed server identity (`agentInfo: { name: 'deepseek-harness-acp', version: '0.0.1' }`) — branding is a literal at the `initialize` site, not config.

 ## ACP method mapping

@@ -48,7 +48,7 @@ How a tool call renders in the editor is owned by the TOOL, not the bridge — t
 - `{ card: 'terminal', title, description?, cwd? }` — a shell command → a terminal card (see Terminal card).
 - `{ card: 'diff', title, diffs, locations? }` — a file create/modify → an inline diff card; `diffs` is `FileDiff[]` (`{ path, oldText, newText }`, `oldText: null` ⇒ new file). The bridge emits each diff as an ACP `{ type: 'diff', path, oldText, newText }` `tool_call.content` block, which Zed renders as an inline diff / new-file preview.

-`presentResult` returns a `ToolResultView`, one of three cards: `{ card: 'generic', title?, content? }` (an optional replacement `title` and reformatted `content`), `{ card: 'terminal', title?, output?, exitCode?, signal? }` (the captured run output + exit — see Terminal card), or `{ card: 'diff', title?, diffs }` (a completed file mutation → typically the applied hunks with context lines computed from the before/after content, or a whole-file diff for a create; a successful mutation ALWAYS returns this so the model-facing result text can't clobber the diff — an ACP `tool_call_update.content` replaces the call's content). The bridge looks the definition up by name in `ctx.tools` and `switch (view.card)`es to build the ACP `tool_call`/`tool_call_update` wire shape per card; 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` returns a `terminal` card for a foreground `bash` (title = the exact `command` "ls -la src", `description` = the model description); the `dsh-tool-fs` `write`/`edit` tools return a `diff` call card and a `diff` result card, and `read` returns a `generic` card (`kind: 'read'`, the read window in its title — `Read foo.txt (5 - 8)` — and a `locations` entry for the file). For a file card the bridge **relativizes the title** against the session cwd (mirroring `claude-agent-acp`'s `toDisplayPath` — `Read src/foo.ts`, not the absolute path) while keeping `locations[]`/`diffs[].path` **raw** so the editor opens the real path.
+`presentResult` returns a `ToolResultView`, one of three cards: `{ card: 'generic', title?, content? }` (an optional replacement `title` and reformatted `content`), `{ card: 'terminal', title?, output?, exitCode?, signal? }` (the captured run output + exit — see Terminal card), or `{ card: 'diff', title?, diffs }` (a completed file mutation → typically the applied hunks with context lines computed from the before/after content, or a whole-file diff for a create; a successful mutation ALWAYS returns this so the model-facing result text can't clobber the diff — an ACP `tool_call_update.content` replaces the call's content). The bridge looks the definition up by name in `ctx.tools` and `switch (view.card)`es to build the ACP `tool_call`/`tool_call_update` wire shape per card; a tool that declares neither gets a generic fallback (title = tool name, raw parsed args as `rawInput`, kind `other` — the bridge never sniffs a kind from the tool name). For example `dsh-tool-bash` returns a `terminal` card for a foreground `bash` (title = the exact `command` "ls -la src", `description` = the model description); the `dsh-tool-fs` `write`/`edit` tools return a `diff` call card and a `diff` result card, and `read` returns a `generic` card (`kind: 'read'`, the read window in its title — `Read foo.txt (5 - 8)` — and a `locations` entry for the file). For a file card the bridge **relativizes the title** against the session cwd (mirroring `claude-agent-acp`'s `toDisplayPath` — `Read src/foo.ts`, not the absolute path) while keeping `locations[]`/`diffs[].path` **raw** so the editor opens the real path.

 The `tool/result` session event does not carry the tool name or args — so to call a tool's `presentResult` the bridge keeps a small per-session map from `callId` to the in-flight call's `(name, args)`, populated on `tool/call` and removed as each result is presented (it holds only currently-in-flight calls, never finished ones). This is bridge-local state — NOT a change to the event schema or a core service. The map lives on the `SessionRecord`, so two concurrent sessions never cross their in-flight tool state; a `session/load` replay uses a throwaway presenter that pairs each `tool/call` with its `tool/result` as the log replays in order, so replayed tool cards render identically to live ones.

--- a/packages/ui/acp/acp-feature-support.md
+++ b/packages/ui/acp/acp-feature-support.md
@@ -63,7 +63,7 @@ These are capabilities the bridge would *drive* on the editor. The harness runs
 | `sessionCapabilities.*` | S | ❌ | ✅ | ✅ | None advertised (list/delete/resume/close/additionalDirectories/fork all off). |
 | `auth.logout` | S | ❌ | ✅ | ✅ | Not advertised. |
 | `authMethods[]` | S | ⚠️ | ✅ | ✅ | Advertised as empty (no auth required to reach the model). |
-| `agentInfo` (name/version) | S | ✅ | ✅ | ✅ | From `agentName` / `agentVersion` config. |
+| `agentInfo` (name/version) | S | ✅ | ✅ | ✅ | Fixed literals: `deepseek-harness-acp` / `0.0.1` (not config). |
 | `_meta` custom caps | S | ❌ | ✅ | — | E.g. Claude's `claudeCode.promptQueueing`. The bridge advertises no custom `_meta`. |

 ### 3b. `clientCapabilities` (consumed by the bridge)
@@ -96,7 +96,7 @@ Tool-call presentation is **owned by each tool** (`presentCall` / `presentResult

 | Feature | Stable | Bridge | Claude | Codex | Notes |
 |---|---|---|---|---|---|
-| `ToolCallKind` mapping | S | ✅ | ✅ | ✅ | `execute`/`read`/`edit`/`other` inferred from the tool; richer mapping possible. |
+| `ToolCallKind` mapping | S | ✅ | ✅ | ✅ | `execute`/`read`/`edit` declared by each tool's `presentCall`; presenter-less tools render `other` (no name sniffing); richer mapping possible. |
 | `ToolCallStatus` | S | ✅ | ✅ | ✅ | `in_progress` → `completed`/`failed`. |
 | `content` blocks | S | ✅ | ✅ | ✅ | Text content; the description renders above the card. |
 | `diff` content | S | ✅ | ✅ | ✅ | The `write`/`edit` tools declare a `diff` render intent: `presentCall` → a call-time `{ card: 'diff' }` snippet, and `presentResult` → a result-time `{ card: 'diff' }`. For an edit or an overwrite it carries the applied hunk(s) with surrounding context (one per `replace_all` site), computed from the before/after text and persisted on the `tool/result` event as `meta`; for a create (no before-image) it is an args-derived whole-file diff. The bridge emits `{ type: 'diff', path, oldText, newText }` content blocks; a successful mutation ALWAYS returns the result diff (an ACP `tool_call_update.content` replaces the call's content, so the result diff — not the model-facing text — is what survives). |
--- a/packages/ui/acp/src/index.ts
+++ b/packages/ui/acp/src/index.ts
@@ -67,7 +67,7 @@ import type { Agent, AgentStatus } from '@deepseek-ai/dsh-agent'
 import { AgentId } from '@deepseek-ai/dsh-agent'
 import { SessionId } from '@deepseek-ai/dsh-session'
 import type { SessionEvent, TodoItem, TurnEndReason } from '@deepseek-ai/dsh-session'
-import type { ToolCallKind, ToolCallView, ToolRegistry, ToolResultView, TerminalResultView } from '@deepseek-ai/dsh-tools'
+import type { ToolCallView, ToolRegistry, ToolResultView, TerminalResultView } 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'
@@ -117,10 +117,6 @@ export interface AcpConfig {
  model?: string
  /** Per-agent system prompt. */
  systemPrompt?: string
-  /** Agent/server name reported to the client in `initialize`. */
-  agentName?: string
-  /** Agent/server version reported to the client in `initialize`. */
-  agentVersion?: string
  /**
   * Transport stream override. Production omits this (the plugin wires
   * `process.stdin`/`process.stdout` via `ndJsonStream`). Tests inject an
@@ -134,8 +130,6 @@ export interface AcpConfig {
 export const Config: Schema<AcpConfig> = Schema.object({
  model: Schema.string(),
  systemPrompt: Schema.string(),
-  agentName: Schema.string().default('deepseek-harness-acp'),
-  agentVersion: Schema.string().default('0.0.1'),
 })

 /**
@@ -209,13 +203,6 @@ interface SessionRecord {
 * (settle-exactly-once).
 */
 export function apply(ctx: Context, config: AcpConfig): void {
-  // TODO(double-default): these literals duplicate the Config schema defaults
-  // (`agentName`/`agentVersion` `.default(...)` above). The Loader applies the
-  // schema before apply() runs, so the `??` only fires for direct-apply unit
-  // tests. Pick one home for the default to avoid drift.
-  const agentName = config.agentName ?? 'deepseek-harness-acp'
-  const agentVersion = config.agentVersion ?? '0.0.1'
-
  // Capture the injected services NOW, during apply(), while we are inside this
  // plugin's fiber (where `inject` grants access). The ACP method handlers run
  // LATER, from the AgentSideConnection's JSON-RPC read loop — a context that is
@@ -430,7 +417,9 @@ export function apply(ctx: Context, config: AcpConfig): void {
        terminalOutputCap = params.clientCapabilities?._meta?.['terminal_output'] === true
        return Promise.resolve({
          protocolVersion,
-          agentInfo: { name: agentName, version: agentVersion },
+          // Fixed server identity: this bridge IS the harness ACP server, so the
+          // branding is a literal, not config (no shipped surface sets it).
+          agentInfo: { name: 'deepseek-harness-acp', version: '0.0.1' },
          agentCapabilities: {
            loadSession: true,
            // Baseline prompt blocks only: text plus resource_link rendered as
@@ -911,9 +900,11 @@ export class ToolPresenter {
      this.onError(`acp: tool "${name}" presentCall threw, using generic presentation: ${String(error)}`)
      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 generic card).
-    const view: ToolCallView = present ?? { card: 'generic', title: name, kind: toolKindFor(name), rawInput: args }
+    // No tool-owned presentation: fall back to the tool name as the title, the
+    // full parsed args as the raw input, and kind `other` (the generic card).
+    // The kind is never sniffed from the name — the bridge does not special-case
+    // tool names; a tool that wants a richer kind declares `presentCall`.
+    const view: ToolCallView = present ?? { card: 'generic', title: name, kind: 'other', rawInput: args }
    this.pending.set(callId, { name, args, card: view.card })
    return view
  }
@@ -951,18 +942,10 @@ export class ToolPresenter {
 * results pass their raw content through unchanged.
 */
 export const nullToolPresenter: Pick<ToolPresenter, 'call' | 'result'> = {
-  call: (_callId, name, argsJson) => ({ card: 'generic', title: name, kind: toolKindFor(name), rawInput: parseToolArguments(argsJson) }),
+  call: (_callId, name, argsJson) => ({ card: 'generic', title: name, kind: 'other', rawInput: parseToolArguments(argsJson) }),
  result: (_callId, content) => ({ card: 'generic', content }),
 }

-/** Map a harness tool name to an ACP ToolKind (best-effort; default `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'
-  return 'other'
-}
-
 /** Parse a tool-call arguments JSON string for `rawInput`; raw string on failure. */
 function parseToolArguments(args: string): unknown {
  try {
--- a/packages/ui/acp/tests/bridge.spec.ts
+++ b/packages/ui/acp/tests/bridge.spec.ts
@@ -33,7 +33,7 @@ describe('acp bridge', () => {
    expect(res.protocolVersion).toBe(PROTOCOL_VERSION)
    expect(res.agentCapabilities?.loadSession).toBe(true)
    expect(res.agentCapabilities?.promptCapabilities).toMatchObject({ image: false, audio: false })
-    expect(res.agentInfo?.name).toBe('deepseek-harness-acp')
+    expect(res.agentInfo).toEqual({ name: 'deepseek-harness-acp', version: '0.0.1' })
  })

  it('session/new creates a session and a full prompt turn streams text then settles end_turn', async () => {
@@ -148,14 +148,13 @@ describe('acp bridge', () => {
    await expect(harness.client.authenticate({ methodId: 'whatever' })).resolves.toBeDefined()
  })

-  it('honors agentName/agentVersion/systemPrompt config', async () => {
+  it('honors systemPrompt config', async () => {
    harness = await makeBridgeHarness({
      storageDir,
      script: [textResponse('ok')],
-      config: { agentName: 'custom-agent', agentVersion: '9.9.9', systemPrompt: 'be terse' },
+      config: { systemPrompt: 'be terse' },
    })
-    const res = await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} })
-    expect(res.agentInfo).toMatchObject({ name: 'custom-agent', version: '9.9.9' })
+    await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} })
    // Create + prompt so the systemPrompt config flows through agentOptions and
    // reaches the model request.
    const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] })
--- a/packages/ui/acp/tests/stream-update.spec.ts
+++ b/packages/ui/acp/tests/stream-update.spec.ts
@@ -50,32 +50,34 @@ describe('streamSessionEventUpdate', () => {
      .toEqual([])
  })

-  it('maps tool/call to an in_progress tool_call with inferred kind and parsed rawInput (generic fallback, no presenter)', () => {
+  it('maps tool/call to an in_progress tool_call with kind other 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',
      toolCallId: 'c1',
      title: 'bash',
-      kind: 'execute',
+      // The fallback never sniffs a kind from the tool name — even a name a
+      // first-party tool uses (`bash`) renders `other`; kinds are tool-owned
+      // via presentCall.
+      kind: 'other',
      status: 'in_progress',
      rawInput: { command: 'ls' },
    }])
  })

-  it('infers tool kinds: read*/write*/edit*/other', () => {
-    const kind = (name: string): unknown =>
-      updatesFor(evt('tool/call', { turn: 1, step: 1, callId: CallId('c'), name, arguments: '' }))[0]
-    expect((kind('read_file') as { kind: string }).kind).toBe('read')
-    expect((kind('write') as { kind: string }).kind).toBe('edit')
-    expect((kind('edit_file') as { kind: string }).kind).toBe('edit')
-    expect((kind('frobnicate') as { kind: string }).kind).toBe('other')
-  })
-
  it('falls back to the raw argument string when tool arguments are not JSON', () => {
    const update = updatesFor(evt('tool/call', { turn: 1, step: 1, callId: CallId('c1'), name: 'bash', arguments: 'not json' }))[0]
    expect((update as { rawInput: unknown }).rawInput).toBe('not json')
  })

+  it('parses EMPTY tool arguments to an empty-object rawInput (a zero-arg call, not the raw-string fallback)', () => {
+    // `JSON.parse('')` throws, so without the empty-string guard a zero-arg
+    // call would render `rawInput: ''` via the non-JSON fallback; the guard
+    // normalizes it to `{}`.
+    const update = updatesFor(evt('tool/call', { turn: 1, step: 1, callId: CallId('c1'), name: 'noop', arguments: '' }))[0]
+    expect((update as { rawInput: unknown }).rawInput).toEqual({})
+  })
+
  it('maps tool/result to completed/failed tool_call_update with text content', () => {
    const ok = updatesFor(evt('tool/result', { turn: 1, step: 1, callId: CallId('c1'), content: [{ type: 'text', text: 'out' }], isError: false }))
    expect(ok).toEqual([{
--- a/packages/ui/acp/tests/turns.spec.ts
+++ b/packages/ui/acp/tests/turns.spec.ts
@@ -66,7 +66,10 @@ describe('acp bridge — turn outcomes', () => {
    const toolCalls = harness.updates.filter(u => u.sessionUpdate === 'tool_call')
    const toolUpdates = harness.updates.filter(u => u.sessionUpdate === 'tool_call_update')
    expect(toolCalls).toHaveLength(1)
-    expect(toolCalls[0]).toMatchObject({ toolCallId: 'c1', title: 'bash', kind: 'execute', status: 'in_progress' })
+    // The inline stand-in declares no presentCall, so the generic fallback
+    // renders kind `other` (kinds are tool-owned; the bridge never sniffs the
+    // name — the REAL dsh-tool-bash test below covers the execute card).
+    expect(toolCalls[0]).toMatchObject({ toolCallId: 'c1', title: 'bash', kind: 'other', status: 'in_progress' })
    expect(toolUpdates).toHaveLength(1)
    expect(toolUpdates[0]).toMatchObject({ toolCallId: 'c1', status: 'completed' })