Преглед изворни кода

fix(acp): exhaustive result-card switch + tighten display-path guard

Address the render-intent-union review:

- toolResultUpdate branched on `if (card === 'terminal')` with a generic
  fallthrough; ToolResultView is a closed union, so make it an exhaustive
  `switch (view.card)` ending in assertNever (matching the call-side
  renderer and the § Conventions closed-union rule). Adding a result card
  later now fails to compile at the switch. Regression test: a rogue result
  card throws.
- displayTitle's `rel.startsWith('..')` guard mis-rejected an in-workspace
  target whose relative form merely begins with the chars `..` (e.g.
  `..cache/x`, a real sibling name), leaving its title absolute. Test for a
  `..` SEGMENT (`..` alone or `..<sep>…`) so such paths relativize, matching
  claude-agent-acp's `cwd + sep` prefix check. Regression test added.
Tianyi Cui пре 2 месеци
родитељ
комит
af79ceea1c
2 измењених фајлова са 70 додато и 31 уклоњено
  1. 39 31
      packages/ui/acp/src/index.ts
  2. 31 0
      packages/ui/acp/tests/stream-update.spec.ts

+ 39 - 31
packages/ui/acp/src/index.ts

@@ -36,7 +36,7 @@
 import type { Context } from 'cordis'
 import { Readable, Writable } from 'node:stream'
 import { randomUUID } from 'node:crypto'
-import { isAbsolute, relative as relativePath, resolve as resolvePath } from 'node:path'
+import { isAbsolute, relative as relativePath, resolve as resolvePath, sep as pathSep } from 'node:path'
 import Schema from 'schemastery'
 import {
   AgentSideConnection,
@@ -1008,9 +1008,12 @@ type AcpToolCallContent =
 function displayTitle(title: string, rawPath: string | undefined, sessionCwd: string | undefined): string {
   if (rawPath === undefined || sessionCwd === undefined || !isAbsolute(rawPath) || !isAbsolute(sessionCwd)) return title
   const rel = relativePath(sessionCwd, rawPath)
-  // `relative` returns a `..`-prefixed path for a target outside the workspace;
-  // only relativize paths that stay inside it (and never to the empty string).
-  if (rel.length === 0 || rel.startsWith('..')) return title
+  // Only relativize a target that stays INSIDE the workspace. `relative` prefixes
+  // a `..` SEGMENT for a target above the cwd — test for the segment (`..` alone
+  // or `..<sep>…`), NOT a bare `..` char prefix, so a sibling like `..cache/x`
+  // (a real in-workspace name) still relativizes. Never relativize to the empty
+  // string (rawPath === cwd — a non-file target).
+  if (rel.length === 0 || rel === '..' || rel.startsWith(`..${pathSep}`)) return title
   return title.split(rawPath).join(rel)
 }
 
@@ -1127,39 +1130,44 @@ function terminalExitMeta(callId: string, view: TerminalResultView): TerminalExi
  */
 function toolResultUpdate(callId: CallId, view: ToolResultView, isError: boolean, terminal: TerminalRendering): ToolCallSessionUpdate {
   const status = isError ? 'failed' as const : 'completed' as const
-  if (view.card === 'terminal') {
-    const output = view.output ?? ''
-    if (terminal.enabled) {
+  switch (view.card) {
+    case 'terminal': {
+      const output = view.output ?? ''
+      if (terminal.enabled) {
+        return {
+          sessionUpdate: 'tool_call_update',
+          toolCallId: callId,
+          status,
+          ...view.title !== undefined ? { title: view.title } : {},
+          _meta: {
+            terminal_output: { terminal_id: callId, data: output },
+            ...terminalExitMeta(callId, view),
+          },
+        }
+      }
+      // No terminal capability: the bridge derives the fenced ```console fallback.
+      const fenced = `\`\`\`console\n${output.replace(/\n+$/, '')}\n\`\`\``
       return {
         sessionUpdate: 'tool_call_update',
         toolCallId: callId,
         status,
+        content: [{ type: 'content', content: { type: 'text', text: fenced } }],
         ...view.title !== undefined ? { title: view.title } : {},
-        _meta: {
-          terminal_output: { terminal_id: callId, data: output },
-          ...terminalExitMeta(callId, view),
-        },
       }
     }
-    // No terminal capability: the bridge derives the fenced ```console fallback.
-    const fenced = `\`\`\`console\n${output.replace(/\n+$/, '')}\n\`\`\``
-    return {
-      sessionUpdate: 'tool_call_update',
-      toolCallId: callId,
-      status,
-      content: [{ type: 'content', content: { type: 'text', text: fenced } }],
-      ...view.title !== undefined ? { title: view.title } : {},
-    }
-  }
-  // The presenter fills a generic result's content from the raw result, so
-  // `content` is always defined here; the guard keeps this total for a
-  // directly-constructed view.
-  return {
-    sessionUpdate: 'tool_call_update',
-    toolCallId: callId,
-    status,
-    /* v8 ignore next -- content always defined via the presenter (see above) */
-    ...view.content !== undefined ? { content: toolResultContent(view.content) } : {},
-    ...view.title !== undefined ? { title: view.title } : {},
+    case 'generic':
+      return {
+        sessionUpdate: 'tool_call_update',
+        toolCallId: callId,
+        status,
+        // The presenter fills a generic result's content from the raw result, so
+        // `content` is always defined here; the guard keeps this total for a
+        // directly-constructed view.
+        /* v8 ignore next -- content always defined via the presenter (see above) */
+        ...view.content !== undefined ? { content: toolResultContent(view.content) } : {},
+        ...view.title !== undefined ? { title: view.title } : {},
+      }
+    default:
+      return assertNever(view, 'ToolResultView.card')
   }
 }

+ 31 - 0
packages/ui/acp/tests/stream-update.spec.ts

@@ -356,6 +356,26 @@ describe('ToolPresenter (tool-owned presentation via the tool registry)', () =>
     }))).toThrow('unreachable variant')
   })
 
+  it('an unknown render-intent RESULT card throws via the exhaustiveness guard (closed union)', () => {
+    // The result-side renderer is also an exhaustive switch + assertNever: a rogue
+    // result card (only reachable by a cast) must throw, so adding a real result
+    // variant later fails to compile at the switch.
+    const rogue: ToolDefinition = {
+      name: 'rogue',
+      description: 'r',
+      parameters: {},
+      execute: async () => [],
+      presentCall: () => ({ card: 'generic', title: 'r' }),
+      presentResult: () => ({ card: 'chart' }) as unknown as ReturnType<NonNullable<ToolDefinition['presentResult']>>,
+    }
+    const presenter = new ToolPresenter(registryOf(rogue))
+    expect(() => updatesWith(
+      presenter,
+      evt('tool/call', { turn: 1, step: 1, callId: CallId('c1'), name: 'rogue', arguments: '{}' }),
+      evt('tool/result', { turn: 1, step: 1, callId: CallId('c1'), content: [{ type: 'text', text: 'x' }], isError: false }),
+    )).toThrow('unreachable variant')
+  })
+
   it('forwards fs-tool render intents onto the wire (REAL read → generic locations, edit → diff content)', async () => {
     // Use the SHIPPING fs tools (not a stand-in), booted through their real
     // plugins, so the wire tool_call carries the actual presentCall output —
@@ -653,6 +673,17 @@ describe('relative-path display titles (bridge relativizes the title against the
     await ctx.fiber.dispose()
   })
 
+  it('an in-workspace file whose relative form starts with `..` chars (a sibling name) still relativizes', async () => {
+    // `/work/proj/..cache/x` is INSIDE the workspace — its relative form
+    // `..cache/x` begins with the chars `..` but is NOT a parent segment. The
+    // guard tests for a `..` SEGMENT, so this relativizes (matching the reference
+    // adapter, which accepts any target under `cwd + sep`).
+    const ctx = await fsCtx()
+    const update = callUpdate(ctx, '/work/proj', 'read', { file_path: '/work/proj/..cache/x.ts' })
+    expect((update as { title: string }).title).toBe('Read ..cache/x.ts')
+    await ctx.fiber.dispose()
+  })
+
   it('no session cwd → the absolute title is left unchanged', async () => {
     const ctx = await fsCtx()
     const update = callUpdate(ctx, undefined, 'read', { file_path: '/work/proj/src/a.ts' })