mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-10-19 12:43:39 +00:00
feat(cli): forward ask_user_question answers from SDK can_use_tool (#6655)
* feat(cli): forward ask_user_question answers from SDK can_use_tool SDK-hosted agents could receive ask_user_question calls through the can_use_tool callback and approve them, but the user's answers never reached the tool: the CLI called onConfirm(ProceedOnce) with no payload, so the tool read an empty answers map and the model never got the decisions. Route updatedInput.answers from the SDK's allow response into the tool confirmation payload so the collected answers reach the tool. Reuses the existing updatedInput channel — no new SDK API or types. Document the pattern in the TypeScript and Python SDK READMEs. * fix(cli): forward ask_user_question answers on teammate approval path Address review feedback on #6655: - handleTeammateApproval now mirrors the leader path and promotes the user's answers from updatedInput into the confirmation payload, so ask_user_question calls approved through a teammate no longer drop the user's choices (wenshao). - Extract a shared buildAllowConfirmationPayload helper used by both the leader and teammate paths, and only promote `answers` for ask_user_question so a same-named field on any other tool's input can't leak into the payload. - Add tests for the teammate path and the defensive guards (array updatedInput, array/null/empty answers, foreign answers field). * test(web-shell): stub Range client-rect methods to fix flaky CI CodeMirror's async measure pass (scheduled via requestAnimationFrame) calls getClientRects()/getBoundingClientRect() on a text Range. jsdom implements these on Element but not on Range, so the call throws "textRange(...).getClientRects is not a function" from a rAF callback after the test completed. Vitest surfaces it as an unhandled error and fails the whole run with exit code 1 even though every assertion passed (seen intermittently in useComposerCore.dom.test.tsx). Polyfill both methods on Range.prototype in the shared test setup, mirroring the existing ResizeObserver/scrollIntoView stubs. * refactor(cli): use ToolNames constant and broaden permission tests Address review suggestions on #6655: - buildAllowConfirmationPayload now gates answers-promotion on the ToolNames.ASK_USER_QUESTION constant instead of a bare string literal, so a future rename of the tool name is a compile-time break rather than a silent regression. - Add an it.each case for a non-object primitive updatedInput (string) to cover the `typeof updatedInput !== 'object'` guard branch. - Assert the leader path overrides toolCall.request.args with the host's sanitized updatedInput before confirming. - Add a teammate-path test for an allow response with no updatedInput, asserting respond is called with (ProceedOnce, undefined). --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>
This commit is contained in:
Tianyuan
GitHub
parent
417fef2130
commit
075c3f03e5
5 changed files with 437 additions and 15 deletions
|
|
@ -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<string, unknown>,
|
||||
},
|
||||
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(),
|
||||
|
|
|
|||
|
|
@ -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<string, unknown>;
|
||||
const answers =
|
||||
toolName === ToolNames.ASK_USER_QUESTION
|
||||
? updatedInputObj['answers']
|
||||
: undefined;
|
||||
return {
|
||||
updatedInput: updatedInputObj,
|
||||
...(answers && typeof answers === 'object' && !Array.isArray(answers)
|
||||
? { answers: answers as Record<string, string> }
|
||||
: {}),
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* 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<string, unknown> }
|
||||
: 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<string, unknown>;
|
||||
}
|
||||
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 =
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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<string, string> = {};
|
||||
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
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue