diff --git a/packages/cli/src/nonInteractive/control/controllers/permissionController.test.ts b/packages/cli/src/nonInteractive/control/controllers/permissionController.test.ts index ec03ad2847..63de2c3cea 100644 --- a/packages/cli/src/nonInteractive/control/controllers/permissionController.test.ts +++ b/packages/cli/src/nonInteractive/control/controllers/permissionController.test.ts @@ -97,6 +97,200 @@ describe('PermissionController', () => { }); }); + it('routes ask_user_question answers from updatedInput into the confirmation payload', async () => { + const context = createContext(120_000); + const controller = new PermissionController( + context, + createRegistry(), + 'PermissionController', + ); + const answers = { '0': 'PostgreSQL', '1': 'REST' }; + vi.spyOn(controller, 'sendControlRequest').mockResolvedValue({ + subtype: 'success', + request_id: 'request-answers', + response: { + behavior: 'allow', + updatedInput: { questions: [], answers }, + }, + }); + const onConfirm = vi.fn(); + const toolCall = { + status: 'awaiting_approval', + request: { + callId: 'tool-call-answers', + name: 'ask_user_question', + args: { questions: [] } as Record, + }, + confirmationDetails: { + type: 'ask_user_question', + title: 'Please answer', + onConfirm, + }, + }; + + controller.getToolCallUpdateCallback()([toolCall]); + + await vi.waitFor(() => { + expect(onConfirm).toHaveBeenCalledWith( + ToolConfirmationOutcome.ProceedOnce, + expect.objectContaining({ answers }), + ); + }); + + // The leader path overrides the tool's in-process args with the + // host's sanitized updatedInput before confirming. + expect(toolCall.request.args).toEqual({ questions: [], answers }); + }); + + it('omits answers from the payload when updatedInput has none', async () => { + const context = createContext(120_000); + const controller = new PermissionController( + context, + createRegistry(), + 'PermissionController', + ); + vi.spyOn(controller, 'sendControlRequest').mockResolvedValue({ + subtype: 'success', + request_id: 'request-no-answers', + response: { + behavior: 'allow', + updatedInput: { command: 'ls -a' }, + }, + }); + const onConfirm = vi.fn(); + + controller.getToolCallUpdateCallback()([ + { + status: 'awaiting_approval', + request: { + callId: 'tool-call-no-answers', + name: 'run_shell_command', + args: { command: 'ls' }, + }, + confirmationDetails: { + type: 'exec', + title: 'Run command', + onConfirm, + }, + }, + ]); + + await vi.waitFor(() => { + expect(onConfirm).toHaveBeenCalledWith( + ToolConfirmationOutcome.ProceedOnce, + { updatedInput: { command: 'ls -a' } }, + ); + }); + }); + + it('does not promote a same-named answers field for non-ask_user_question tools', async () => { + const context = createContext(120_000); + const controller = new PermissionController( + context, + createRegistry(), + 'PermissionController', + ); + vi.spyOn(controller, 'sendControlRequest').mockResolvedValue({ + subtype: 'success', + request_id: 'request-foreign-answers', + response: { + behavior: 'allow', + // A non-ask_user_question tool happens to carry an `answers` field; + // it must not leak into the confirmation payload. + updatedInput: { command: 'ls', answers: { '0': 'leak' } }, + }, + }); + const onConfirm = vi.fn(); + + controller.getToolCallUpdateCallback()([ + { + status: 'awaiting_approval', + request: { + callId: 'tool-call-foreign-answers', + name: 'run_shell_command', + args: { command: 'ls' }, + }, + confirmationDetails: { + type: 'exec', + title: 'Run command', + onConfirm, + }, + }, + ]); + + await vi.waitFor(() => { + expect(onConfirm).toHaveBeenCalledWith( + ToolConfirmationOutcome.ProceedOnce, + { updatedInput: { command: 'ls', answers: { '0': 'leak' } } }, + ); + }); + expect(onConfirm).not.toHaveBeenCalledWith( + ToolConfirmationOutcome.ProceedOnce, + expect.objectContaining({ answers: expect.anything() }), + ); + }); + + it.each([ + ['updatedInput is an array', ['ls'], undefined], + ['updatedInput is a string', 'ls', undefined], + ['answers is an array', { questions: [], answers: ['x'] }, undefined], + ['answers is null', { questions: [], answers: null }, undefined], + ['answers is an empty object', { questions: [], answers: {} }, {}], + ])( + 'omits answers from the payload when %s', + async (_desc, updatedInput, expectedAnswers) => { + const context = createContext(120_000); + const controller = new PermissionController( + context, + createRegistry(), + 'PermissionController', + ); + vi.spyOn(controller, 'sendControlRequest').mockResolvedValue({ + subtype: 'success', + request_id: 'request-guard', + response: { behavior: 'allow', updatedInput }, + }); + const onConfirm = vi.fn(); + + controller.getToolCallUpdateCallback()([ + { + status: 'awaiting_approval', + request: { + callId: 'tool-call-guard', + name: 'ask_user_question', + args: { questions: [] }, + }, + confirmationDetails: { + type: 'ask_user_question', + title: 'Please answer', + onConfirm, + }, + }, + ]); + + await vi.waitFor(() => { + expect(onConfirm).toHaveBeenCalled(); + }); + + const [outcome, payload] = onConfirm.mock.calls[0]; + expect(outcome).toBe(ToolConfirmationOutcome.ProceedOnce); + const isPlainObject = + updatedInput !== null && + typeof updatedInput === 'object' && + !Array.isArray(updatedInput); + if (!isPlainObject) { + // A non-object updatedInput (array or primitive) is rejected + // wholesale — plain confirm, no payload. + expect(payload).toBeUndefined(); + } else if (expectedAnswers === undefined) { + expect(payload).toEqual({ updatedInput }); + expect(payload).not.toHaveProperty('answers'); + } else { + expect(payload).toEqual({ updatedInput, answers: expectedAnswers }); + } + }, + ); + it('uses default timeout when SDK canUseTool timeout is undefined', async () => { const context = createContext(); // undefined timeout const controller = new PermissionController( @@ -184,6 +378,100 @@ describe('PermissionController', () => { }); }); + it('forwards ask_user_question answers to a teammate approval', async () => { + const context = createContext(120_000); + const controller = new PermissionController( + context, + createRegistry(), + 'PermissionController', + ); + const answers = { '0': 'PostgreSQL', '1': 'REST' }; + vi.spyOn(controller, 'sendControlRequest').mockResolvedValue({ + subtype: 'success', + request_id: 'teammate-request', + response: { + behavior: 'allow', + updatedInput: { questions: [], answers }, + }, + }); + const respond = vi.fn().mockResolvedValue(undefined); + + await controller.handleTeammateApproval({ + teammateName: 'worker', + toolName: 'ask_user_question', + toolInput: { questions: [] }, + respond, + timestamp: 123, + }); + + expect(respond).toHaveBeenCalledWith( + ToolConfirmationOutcome.ProceedOnce, + expect.objectContaining({ answers }), + ); + }); + + it('does not promote a same-named answers field for a non-ask_user_question teammate approval', async () => { + const context = createContext(120_000); + const controller = new PermissionController( + context, + createRegistry(), + 'PermissionController', + ); + vi.spyOn(controller, 'sendControlRequest').mockResolvedValue({ + subtype: 'success', + request_id: 'teammate-request-foreign', + response: { + behavior: 'allow', + updatedInput: { command: 'ls', answers: { '0': 'leak' } }, + }, + }); + const respond = vi.fn().mockResolvedValue(undefined); + + await controller.handleTeammateApproval({ + teammateName: 'worker', + toolName: 'run_shell_command', + toolInput: { command: 'ls' }, + respond, + timestamp: 456, + }); + + expect(respond).toHaveBeenCalledWith(ToolConfirmationOutcome.ProceedOnce, { + updatedInput: { command: 'ls', answers: { '0': 'leak' } }, + }); + expect(respond).not.toHaveBeenCalledWith( + ToolConfirmationOutcome.ProceedOnce, + expect.objectContaining({ answers: expect.anything() }), + ); + }); + + it('confirms a teammate approval with no payload when updatedInput is absent', async () => { + const context = createContext(120_000); + const controller = new PermissionController( + context, + createRegistry(), + 'PermissionController', + ); + vi.spyOn(controller, 'sendControlRequest').mockResolvedValue({ + subtype: 'success', + request_id: 'teammate-request-no-input', + response: { behavior: 'allow' }, + }); + const respond = vi.fn().mockResolvedValue(undefined); + + await controller.handleTeammateApproval({ + teammateName: 'worker', + toolName: 'run_shell_command', + toolInput: { command: 'ls' }, + respond, + timestamp: 789, + }); + + expect(respond).toHaveBeenCalledWith( + ToolConfirmationOutcome.ProceedOnce, + undefined, + ); + }); + it('omits modify suggestions when edit confirmation hides modify actions', () => { const controller = new PermissionController( createContext(), diff --git a/packages/cli/src/nonInteractive/control/controllers/permissionController.ts b/packages/cli/src/nonInteractive/control/controllers/permissionController.ts index 4474554eed..a3179f2c8c 100644 --- a/packages/cli/src/nonInteractive/control/controllers/permissionController.ts +++ b/packages/cli/src/nonInteractive/control/controllers/permissionController.ts @@ -25,6 +25,7 @@ import type { import { InputFormat, ToolConfirmationOutcome, + ToolNames, } from '@qwen-code/qwen-code-core'; import type { CLIControlPermissionRequest, @@ -388,6 +389,44 @@ export class PermissionController extends BaseController { }; } + /** + * Build the confirmation payload for an approved (`allow`) tool call. + * + * `updatedInput` carries the (possibly sanitised) tool args the host + * wants executed. For `ask_user_question` the host also delivers the + * user's answers on this channel as `updatedInput.answers`; those + * answers must reach the tool via `payload.answers` (the tool reads + * them from there, not from its args). Answers are promoted only for + * `ask_user_question`, so a same-named `answers` field on any other + * tool's input can never leak into the confirmation payload. + * + * Returns `undefined` when the host sent no usable `updatedInput`, so + * callers fall back to a plain single-argument confirmation. + */ + private buildAllowConfirmationPayload( + toolName: string, + updatedInput: unknown, + ): ToolConfirmationPayload | undefined { + if ( + !updatedInput || + typeof updatedInput !== 'object' || + Array.isArray(updatedInput) + ) { + return undefined; + } + const updatedInputObj = updatedInput as Record; + const answers = + toolName === ToolNames.ASK_USER_QUESTION + ? updatedInputObj['answers'] + : undefined; + return { + updatedInput: updatedInputObj, + ...(answers && typeof answers === 'object' && !Array.isArray(answers) + ? { answers: answers as Record } + : {}), + }; + } + /** * Handle a teammate tool approval request routed via the * TEAMMATE_APPROVAL_REQUEST team event. Stream-json only — @@ -447,14 +486,13 @@ export class PermissionController extends BaseController { // args and the host's policy is silently bypassed. The // leader's same-process path mutates `request.args` // directly; teammates can't reach across process so the - // payload carries the override instead. - const updatedInput = payload['updatedInput']; - const respondPayload: ToolConfirmationPayload | undefined = - updatedInput && - typeof updatedInput === 'object' && - !Array.isArray(updatedInput) - ? { updatedInput: updatedInput as Record } - : undefined; + // payload carries the override instead. For + // `ask_user_question` this same payload also carries the + // user's answers, mirroring the leader path. + const respondPayload = this.buildAllowConfirmationPayload( + event.toolName, + payload['updatedInput'], + ); await event.respond( ToolConfirmationOutcome.ProceedOnce, respondPayload, @@ -555,14 +593,32 @@ export class PermissionController extends BaseController { const behavior = String(payload['behavior'] || '').toLowerCase(); if (behavior === 'allow') { - // Handle updated input if provided - const updatedInput = payload['updatedInput']; - if (updatedInput && typeof updatedInput === 'object') { - toolCall.request.args = updatedInput as Record; - } - await toolCall.confirmationDetails.onConfirm( - ToolConfirmationOutcome.ProceedOnce, + // Handle updated input if provided. The SDK's `can_use_tool` + // callback returns `updatedInput` — the (possibly sanitised) + // tool args the host wants executed. For most tools this simply + // overrides `request.args`. For `ask_user_question` the host also + // uses this channel to deliver the user's answers: it returns + // `{ ...originalInput, answers }`, and those answers must reach the + // tool via the confirmation payload (`payload.answers`) — the tool + // reads answers from there, not from `request.args`. + const confirmationPayload = this.buildAllowConfirmationPayload( + toolCall.request.name, + payload['updatedInput'], ); + + if (confirmationPayload) { + // Override the tool's args in-process with the host's + // sanitised input before confirming. + toolCall.request.args = confirmationPayload.updatedInput ?? {}; + await toolCall.confirmationDetails.onConfirm( + ToolConfirmationOutcome.ProceedOnce, + confirmationPayload, + ); + } else { + await toolCall.confirmationDetails.onConfirm( + ToolConfirmationOutcome.ProceedOnce, + ); + } } else { // Extract cancel message from response if available const cancelMessage = diff --git a/packages/sdk-python/README.md b/packages/sdk-python/README.md index 3f324a4212..6deca2011f 100644 --- a/packages/sdk-python/README.md +++ b/packages/sdk-python/README.md @@ -286,6 +286,39 @@ The `context` argument includes `cancel_event`, `suggestions`, and `can_use_tool` must be an `async def` callback accepting `(tool_name, tool_input, context)`. `stderr` must accept a single `str`. +### Handling `ask_user_question` + +When the model needs a decision from the user it calls the built-in +`ask_user_question` tool. This flows through the same `can_use_tool` +callback: `tool_input` carries a `questions` list, and you return the +collected answers via `updatedInput["answers"]`. `answers` is a dict keyed +by the question's index (as a string), where each value is the label of the +chosen option (or free-form text when the user picks "Other"). + +```python +async def can_use_tool(tool_name, tool_input, context): + if tool_name == "ask_user_question": + questions = tool_input["questions"] + + # Present the questions to the user however your app sees fit, + # then build an index-keyed map of their answers. + answers = {} + for index, question in enumerate(questions): + answers[str(index)] = await prompt_user_to_choose(question) + + # Return the answers through updatedInput["answers"] — the CLI + # forwards them to the tool so the model receives the decisions. + return { + "behavior": "allow", + "updatedInput": {**tool_input, "answers": answers}, + } + + return {"behavior": "allow", "updatedInput": tool_input} +``` + +If you return `allow` without any `answers`, the tool reports that no answer +was provided; return `deny` to signal the user declined. + ## Runtime Controls Control methods can be called while a session is active: diff --git a/packages/sdk-typescript/README.md b/packages/sdk-typescript/README.md index 027e697e71..43896be759 100644 --- a/packages/sdk-typescript/README.md +++ b/packages/sdk-typescript/README.md @@ -314,6 +314,45 @@ const result = query({ }); ``` +### Handling `ask_user_question` + +When the model needs a decision from the user it calls the built-in +`ask_user_question` tool. The SDK surfaces this through the same +`canUseTool` callback: the tool input contains a `questions` array, and you +return the collected answers via `updatedInput.answers`. `answers` is an +object keyed by the question's index (as a string), where each value is the +label of the chosen option (or free-form text when the user picks "Other"). + +```typescript +import { query, type CanUseTool } from '@qwen-code/sdk'; + +const canUseTool: CanUseTool = async (toolName, input, { signal }) => { + if (toolName === 'ask_user_question') { + const questions = input.questions as Array<{ + question: string; + header: string; + options: Array<{ label: string; description: string }>; + }>; + + // Present the questions to the user however your app sees fit, then + // build an index-keyed map of their answers. + const answers: Record = {}; + for (let i = 0; i < questions.length; i++) { + answers[String(i)] = await promptUserToChoose(questions[i]); + } + + // Return the answers through `updatedInput.answers` — the CLI forwards + // them to the tool so the model receives the user's decisions. + return { behavior: 'allow', updatedInput: { ...input, answers } }; + } + + return { behavior: 'allow', updatedInput: input }; +}; +``` + +> If you return `allow` without any `answers`, the tool reports that no +> answer was provided; return `deny` to signal the user declined. + ### With External MCP Servers ```typescript diff --git a/packages/web-shell/client/test/setup.ts b/packages/web-shell/client/test/setup.ts index ace65e79f5..32d6513d99 100644 --- a/packages/web-shell/client/test/setup.ts +++ b/packages/web-shell/client/test/setup.ts @@ -50,6 +50,12 @@ if ( globalWithDom.Element.prototype.scrollIntoView = () => {}; } +// jsdom implements getClientRects()/getBoundingClientRect() on Element but not +// on Range. CodeMirror's async measure pass (scheduled via requestAnimationFrame) +// calls them on a text Range, so without this stub it throws +// "textRange(...).getClientRects is not a function" from a rAF callback after a +// test has completed — an unhandled error that flakes the whole run even though +// every assertion passed. if (typeof globalWithDom.Range !== 'undefined') { const rangePrototype = globalWithDom.Range.prototype as Range & { getBoundingClientRect?: () => DOMRect;