Fix ask_user_question review findings

2026-06-29 10:49:07 +08:00
parent 08fc8467bc
commit 51700d4685
38 changed files with 611 additions and 54 deletions
@@ -4,7 +4,7 @@ import { tmpdir } from 'node:os'
 import { join } from 'node:path'
 import { PROTOCOL_VERSION } from '@agentclientprotocol/sdk'
 import { AgentId } from '@deepseek-ai/dsh-agent'
-import { makeBridgeHarness, textResponse, type BridgeHarness } from './harness.ts'
+import { makeBridgeHarness, textResponse, toolCallResponse, type BridgeHarness } from './harness.ts'

 /**
 * End-to-end bridge specs over an in-memory transport: a real
@@ -53,6 +53,183 @@ describe('acp bridge', () => {
    expect(text).toBe('hello there')
  })

+  it('routes ask_user_question through ACP form elicitation and continues with the selected option', async () => {
+    harness = await makeBridgeHarness({
+      storageDir,
+      withAskUser: true,
+      script: [
+        toolCallResponse('ask-1', 'ask_user_question', {
+          header: 'Project config',
+          question: 'Which language should I use?',
+          options: [
+            { label: 'TypeScript', value: 'ts', description: 'Good for UI apps' },
+            { label: 'Python', value: 'py', description: 'Good for scripts', recommended: true },
+          ],
+          allow_custom: false,
+        }),
+        textResponse('Python it is.'),
+      ],
+    })
+    harness.onElicitation = () => ({ action: 'accept', content: { choice: 'py' } })
+    await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} })
+    const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] })
+
+    const result = await harness.client.prompt({ sessionId, prompt: [{ type: 'text', text: 'ask me' }] })
+
+    expect(result.stopReason).toBe('end_turn')
+    expect(harness.elicitationRequests).toHaveLength(1)
+    expect(harness.elicitationRequests[0]).toMatchObject({
+      sessionId,
+      mode: 'form',
+      message: 'Which language should I use?',
+      requestedSchema: {
+        title: 'Project config',
+        properties: {
+          choice: {
+            default: 'py',
+            oneOf: [
+              { const: 'py', title: 'Python: Good for scripts' },
+              { const: 'ts', title: 'TypeScript: Good for UI apps' },
+            ],
+          },
+        },
+        required: ['choice'],
+      },
+    })
+    const toolResult = harness.ctx.agents.get(AgentId(sessionId))!.session.events.find(event => event.type === 'tool/result')
+    expect(JSON.stringify(toolResult)).toContain('py')
+  })
+
+  it('routes optionless ask_user_question through an ACP free-form answer field', async () => {
+    harness = await makeBridgeHarness({
+      storageDir,
+      withAskUser: true,
+      script: [
+        toolCallResponse('ask-1', 'ask_user_question', {
+          question: 'What should I name it?',
+          allow_custom: false,
+        }),
+        textResponse('Name recorded.'),
+      ],
+    })
+    harness.onElicitation = () => ({ action: 'accept', content: { answer: 'apollo' } })
+    await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} })
+    const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] })
+
+    await harness.client.prompt({ sessionId, prompt: [{ type: 'text', text: 'ask me' }] })
+
+    expect(harness.elicitationRequests[0]).toMatchObject({
+      requestedSchema: {
+        properties: { answer: { type: 'string', title: 'What should I name it?' } },
+        required: ['answer'],
+      },
+    })
+    const toolResult = harness.ctx.agents.get(AgentId(sessionId))!.session.events.find(event => event.type === 'tool/result')
+    expect(JSON.stringify(toolResult)).toContain('apollo')
+  })
+
+  it('supports ACP custom answers alongside choices', async () => {
+    harness = await makeBridgeHarness({ storageDir, withAskUser: true })
+    harness.onElicitation = () => ({ action: 'accept', content: { custom_answer: 'Use Zig' } })
+    await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} })
+    const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] })
+    const agent = harness.ctx.agents.get(AgentId(sessionId))!
+
+    const result = await harness.ctx.userInteraction.ask({
+      agent,
+      question: 'Which language?',
+      options: [{ label: 'TypeScript' }],
+    })
+
+    expect(result).toEqual({ answer: 'Use Zig' })
+    expect(harness.elicitationRequests[0]).toMatchObject({
+      requestedSchema: {
+        properties: {
+          choice: {
+            description: 'Choose one option, or fill a custom answer below.',
+            oneOf: [{ const: 'TypeScript', title: 'TypeScript' }],
+          },
+          custom_answer: { type: 'string' },
+        },
+        required: [],
+      },
+    })
+  })
+
+  it('returns raw ACP answers when they do not match a provided option', async () => {
+    harness = await makeBridgeHarness({ storageDir, withAskUser: true })
+    harness.onElicitation = () => ({ action: 'accept', content: { choice: 'something else' } })
+    await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} })
+    const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] })
+    const agent = harness.ctx.agents.get(AgentId(sessionId))!
+
+    await expect(harness.ctx.userInteraction.ask({
+      agent,
+      question: 'Pick',
+      options: [{ label: 'A', value: 'a' }],
+      allowCustom: false,
+    })).resolves.toEqual({ answer: 'something else' })
+  })
+
+  it('reports ACP ask-user routing and answer failures as structured errors', async () => {
+    harness = await makeBridgeHarness({ storageDir, withAskUser: true })
+    await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} })
+    const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] })
+    const agent = harness.ctx.agents.get(AgentId(sessionId))!
+
+    await expect(harness.ctx.userInteraction.ask({ question: 'No agent?' }))
+      .rejects.toMatchObject({ name: 'UserInteractionError', code: 'NO_AGENT' })
+    await expect(harness.ctx.userInteraction.ask({ agent: { id: 'other' } as typeof agent, question: 'No session?' }))
+      .rejects.toMatchObject({ code: 'NO_SESSION' })
+
+    harness.onElicitation = () => ({ action: 'cancel' })
+    await expect(harness.ctx.userInteraction.ask({ agent, question: 'Cancel?' }))
+      .rejects.toMatchObject({ code: 'ASK_CANCELLED' })
+
+    harness.onElicitation = () => ({ action: 'accept', content: {} })
+    await expect(harness.ctx.userInteraction.ask({ agent, question: 'Empty?' }))
+      .rejects.toMatchObject({ code: 'NO_ANSWER' })
+
+    harness.onElicitation = () => { throw new Error('client boom') }
+    await expect(harness.ctx.userInteraction.ask({ agent, question: 'Client fails?', signal: new AbortController().signal }))
+      .rejects.toMatchObject({ code: 'ASK_FAILED' })
+  })
+
+  it('aborts ACP ask-user requests before and while waiting for elicitation', async () => {
+    harness = await makeBridgeHarness({ storageDir, withAskUser: true })
+    await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} })
+    const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] })
+    const agent = harness.ctx.agents.get(AgentId(sessionId))!
+
+    const alreadyAborted = new AbortController()
+    alreadyAborted.abort()
+    await expect(harness.ctx.userInteraction.ask({ agent, question: 'Already?', signal: alreadyAborted.signal }))
+      .rejects.toMatchObject({ code: 'ASK_ABORTED' })
+
+    let abortedReads = 0
+    const racingAbort = {
+      get aborted() { return abortedReads++ > 0 },
+      addEventListener() {},
+      removeEventListener() {},
+      dispatchEvent() { return false },
+      onabort: null,
+      reason: undefined,
+      throwIfAborted() {},
+    } as AbortSignal
+    await expect(harness.ctx.userInteraction.ask({ agent, question: 'Raced?', signal: racingAbort }))
+      .rejects.toMatchObject({ code: 'ASK_ABORTED' })
+
+    let release: ((value: { action: 'accept'; content: { answer: string } }) => void) | undefined
+    harness.onElicitation = () => new Promise((resolve) => { release = resolve })
+    const pendingAbort = new AbortController()
+    const ask = harness.ctx.userInteraction.ask({ agent, question: 'Pending?', signal: pendingAbort.signal })
+    await new Promise(resolve => setImmediate(resolve))
+    pendingAbort.abort()
+
+    await expect(ask).rejects.toMatchObject({ code: 'ASK_ABORTED' })
+    release?.({ action: 'accept', content: { answer: 'too late' } })
+  })
+
  it('allows multiple concurrent sessions, each with a distinct id', async () => {
    harness = await makeBridgeHarness({ storageDir, script: [] })
    await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} })
@@ -25,11 +25,15 @@ import {
  ndJsonStream,
  type Agent as AcpAgent,
  type Client,
+  type CreateElicitationRequest,
+  type CreateElicitationResponse,
  type RequestPermissionRequest,
  type RequestPermissionResponse,
  type SessionNotification,
  type Stream,
 } from '@agentclientprotocol/sdk'
+import UserInteractionService from '@deepseek-ai/dsh-user-interaction'
+import * as ToolAskUser from '@deepseek-ai/dsh-tool-ask-user'
 import * as AcpPlugin from '../src/index.ts'
 import { type AcpConfig } from '../src/index.ts'

@@ -117,6 +121,10 @@ export interface BridgeHarness {
  permissionRequests: RequestPermissionRequest[]
  /** Decide each permission request's outcome (default: cancelled). */
  onPermission: (req: RequestPermissionRequest) => RequestPermissionResponse
+  /** Elicitation requests the bridge issued for ask_user_question. */
+  elicitationRequests: CreateElicitationRequest[]
+  /** Decide each elicitation response (default: cancel). */
+  onElicitation: (req: CreateElicitationRequest) => CreateElicitationResponse | Promise<CreateElicitationResponse>
  /** If set, the client's sessionUpdate throws this (tests notify error path). */
  onSessionUpdateError: (() => void) | undefined
  /**
@@ -158,6 +166,8 @@ export async function makeBridgeHarness(options: {
   * implementation over a mock in tests").
   */
  withBash?: boolean
+  /** Plug the REAL `ask_user_question` tool and ACP user-interaction provider. */
+  withAskUser?: boolean
 } = { storageDir: '' }): Promise<BridgeHarness> {
  const adapter = new MockAdapter(options.script ?? [])

@@ -169,6 +179,10 @@ export async function makeBridgeHarness(options: {
  await ctx.plugin(AgentRegistry)
  await ctx.plugin(AgentLoop, { agents: [] })
  await ctx.plugin(SessionPersistenceJsonl, { root: options.storageDir })
+  await ctx.plugin(UserInteractionService)
+  if (options.withAskUser) {
+    await ctx.plugin(ToolAskUser)
+  }
  if (options.withBash) {
    await ctx.plugin(LocalBashExecutor, { timeoutMs: 10_000 })
    await ctx.plugin(ToolBash)
@@ -197,6 +211,7 @@ export async function makeBridgeHarness(options: {
  const updates: CapturedUpdate[] = []
  const sessionUpdates: { sessionId: string; update: CapturedUpdate }[] = []
  const permissionRequests: RequestPermissionRequest[] = []
+  const elicitationRequests: CreateElicitationRequest[] = []
  const harness: BridgeHarness = {
    ctx,
    adapter,
@@ -204,6 +219,8 @@ export async function makeBridgeHarness(options: {
    sessionUpdates,
    permissionRequests,
    onPermission: () => ({ outcome: { outcome: 'cancelled' } }),
+    elicitationRequests,
+    onElicitation: () => ({ action: 'cancel' }),
    onSessionUpdateError: undefined,
    client: undefined as unknown as ClientSideConnection,
    acpFiber: undefined as unknown as BridgeHarness['acpFiber'],
@@ -229,6 +246,10 @@ export async function makeBridgeHarness(options: {
      permissionRequests.push(params)
      return Promise.resolve(harness.onPermission(params))
    },
+    unstable_createElicitation(params: CreateElicitationRequest): Promise<CreateElicitationResponse> {
+      elicitationRequests.push(params)
+      return Promise.resolve(harness.onElicitation(params))
+    },
  })

  // Wire the bridge (agent side) and the client (test side). The test config