mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-10-19 12:43:39 +00:00
* fix(core): surface PreToolUse ask content on MCP tool confirmations (#13687) A PreToolUse 'ask' now merges into the tool's normal approval as one confirmation, but withPreToolUseAskReason only handled edit/exec/info confirmation details. MCP tools return 'mcp' details, which fell into the default passthrough, so the hook's question and the exact content it asked about were silently dropped: approving the plain MCP prompt admitted the call blind (or, under YOLO, the forced confirmation showed no content at all). Fall back to the literal-text info confirmation the pre-merge hook bounce used, so the content stays reviewable and expandable before approval. The Mem0 write E2E test asserted the pre-merge two-dialog sequence: MCP permission prompt, then a separate hook content prompt. Update the scenarios to the merged single-dialog contract — one confirmation whose heading is the hook's literal content, in every approval mode — and add a scheduler unit test pinning the MCP fallback. Fixes #13687. Unblocks the v0.25.1-preview.1 release run (#13696). * fix(core): judge plan mode from the tool's real confirmation shape A PreToolUse 'ask' on an MCP tool rewrites the confirmation to the literal-text `info` shape, and `isPlanModeBlocked` exempts `info` — so a hook-asked MCP call in plan mode flipped from a hard block into an approvable dialog. Read the pre-rewrite shape in the plan-mode gate and pin it with a regression test. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmuzv9c3wfd * test(core): pin the hook-ask guard and name the MCP destination The PreToolUse ask fallback for MCP calls now names the server and tool in the dialog body: the `info` shape renders no title and carries no server field, so the hook's reason alone need not identify the destination. The MCP fixtures use a distinct `toolDisplayName` so the title assertion discriminates which field production reads, and a new AUTO_EDIT test pins the `!preToolUseAsk` term that keeps a hook-asked MCP write from being silently auto-approved. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmuzzjnorfk --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
This commit is contained in:
易良
GitHub
parent
a044cc0971
commit
509c310fd4
3 changed files with 136 additions and 24 deletions
|
|
@ -61,7 +61,6 @@ type WriteScenario = {
|
|||
approvalMode: string;
|
||||
approveWrite: boolean;
|
||||
expectedRequests: number;
|
||||
expectsMcpConfirmation: boolean;
|
||||
content?: string;
|
||||
verifiesShortLiteral?: boolean;
|
||||
verifiesLongConfirmation?: boolean;
|
||||
|
|
@ -71,12 +70,11 @@ type ManagedWritePaths = { mcpConfigPath?: string };
|
|||
|
||||
const WRITE_SCENARIOS: WriteScenario[] = [
|
||||
{
|
||||
name: 'uses two confirmations in default mode',
|
||||
name: 'asks for content confirmation in default mode',
|
||||
provider: 'platform-v3',
|
||||
approvalMode: 'default',
|
||||
approveWrite: true,
|
||||
expectedRequests: 1,
|
||||
expectsMcpConfirmation: true,
|
||||
content:
|
||||
'LINK [visible label](https://hidden.example/secret-target) BOLD **bold-value** CODE `code-value` UNDER <u>under-value</u>',
|
||||
verifiesShortLiteral: true,
|
||||
|
|
@ -87,7 +85,6 @@ const WRITE_SCENARIOS: WriteScenario[] = [
|
|||
approvalMode: 'default',
|
||||
approveWrite: false,
|
||||
expectedRequests: 0,
|
||||
expectsMcpConfirmation: true,
|
||||
},
|
||||
{
|
||||
name: 'asks for content confirmation in auto-edit mode',
|
||||
|
|
@ -95,7 +92,6 @@ const WRITE_SCENARIOS: WriteScenario[] = [
|
|||
approvalMode: 'auto-edit',
|
||||
approveWrite: true,
|
||||
expectedRequests: 1,
|
||||
expectsMcpConfirmation: true,
|
||||
},
|
||||
{
|
||||
name: 'still asks for content confirmation in YOLO mode',
|
||||
|
|
@ -103,7 +99,6 @@ const WRITE_SCENARIOS: WriteScenario[] = [
|
|||
approvalMode: 'yolo',
|
||||
approveWrite: true,
|
||||
expectedRequests: 1,
|
||||
expectsMcpConfirmation: false,
|
||||
content:
|
||||
'LINK [visible label](https://hidden.example/secret-target) BOLD **bold-value** CODE `code-value` UNDER <u>under-value</u>',
|
||||
verifiesShortLiteral: true,
|
||||
|
|
@ -114,17 +109,15 @@ const WRITE_SCENARIOS: WriteScenario[] = [
|
|||
approvalMode: 'yolo',
|
||||
approveWrite: true,
|
||||
expectedRequests: 1,
|
||||
expectsMcpConfirmation: false,
|
||||
content: LONG_CONFIRMATION_CONTENT,
|
||||
verifiesLongConfirmation: true,
|
||||
},
|
||||
{
|
||||
name: 'uses two confirmations for the OSS REST provider with its credential from settings.env',
|
||||
name: 'asks for content confirmation for the OSS REST provider with its credential from settings.env',
|
||||
provider: 'oss-rest',
|
||||
approvalMode: 'default',
|
||||
approveWrite: true,
|
||||
expectedRequests: 1,
|
||||
expectsMcpConfirmation: true,
|
||||
},
|
||||
{
|
||||
name: 'does not write to the OSS REST provider when content confirmation is rejected',
|
||||
|
|
@ -132,7 +125,6 @@ const WRITE_SCENARIOS: WriteScenario[] = [
|
|||
approvalMode: 'default',
|
||||
approveWrite: false,
|
||||
expectedRequests: 0,
|
||||
expectsMcpConfirmation: true,
|
||||
},
|
||||
{
|
||||
name: 'still asks for content confirmation for the OSS REST provider in YOLO mode',
|
||||
|
|
@ -140,7 +132,6 @@ const WRITE_SCENARIOS: WriteScenario[] = [
|
|||
approvalMode: 'yolo',
|
||||
approveWrite: true,
|
||||
expectedRequests: 1,
|
||||
expectsMcpConfirmation: false,
|
||||
},
|
||||
];
|
||||
|
||||
|
|
@ -265,18 +256,12 @@ const WRITE_SCENARIOS: WriteScenario[] = [
|
|||
await type(ptyProcess, 'Remember the repository policy.');
|
||||
await type(ptyProcess, '\r');
|
||||
|
||||
if (scenario.expectsMcpConfirmation) {
|
||||
// Screen-based, not rig.waitForText: OpenTUI emits pty bytes by cell
|
||||
// diff and drops spaces over previously-blank cells, so multi-word
|
||||
// rows never appear verbatim in the raw stream on that leg.
|
||||
await waitForScreen(
|
||||
screen,
|
||||
(value) => value.includes('Allow execution of MCP tool'),
|
||||
'ordinary MCP confirmation did not appear',
|
||||
);
|
||||
await type(ptyProcess, '\r');
|
||||
}
|
||||
|
||||
// A PreToolUse 'ask' joins the tool's normal approval as one merged
|
||||
// confirmation: the hook's literal content heads the dialog regardless
|
||||
// of approval mode, and approving it also admits the MCP call. All
|
||||
// waits are screen-based, not rig.waitForText: OpenTUI emits pty bytes
|
||||
// by cell diff and drops spaces over previously-blank cells, so
|
||||
// multi-word rows never appear verbatim in the raw stream on that leg.
|
||||
if (scenario.verifiesLongConfirmation) {
|
||||
const constrainedScreen = await waitForScreen(
|
||||
screen,
|
||||
|
|
|
|||
|
|
@ -11034,6 +11034,115 @@ describe('CoreToolScheduler telemetry spans', () => {
|
|||
expect(getToolSpans()[0].ended).toBe(false);
|
||||
});
|
||||
|
||||
it('shows a PreToolUse ask on an MCP tool as a literal-text info confirmation', async () => {
|
||||
const mcpConfirmation = async (): Promise<ToolCallConfirmationDetails> => ({
|
||||
type: 'mcp',
|
||||
title: 'Confirm MCP Tool Execution',
|
||||
serverName: 'external-context',
|
||||
toolName: 'context_remember',
|
||||
// Distinct from `toolName`, as in production (#13687): the title
|
||||
// assertion below must discriminate which field is read.
|
||||
toolDisplayName: 'context_remember (external-context MCP Server)',
|
||||
onConfirm: async () => {},
|
||||
});
|
||||
const { waiting } = await askUntilApproval({
|
||||
messageBus: askMessageBus(
|
||||
'Save this exact content to the bound Mem0 repository memory?\n[visible](https://hidden.example/target)',
|
||||
),
|
||||
tools: [
|
||||
new MockTool({
|
||||
name: 'mcpTool',
|
||||
execute: vi.fn().mockResolvedValue(textResult('ok')),
|
||||
getConfirmationDetails: mcpConfirmation,
|
||||
}),
|
||||
],
|
||||
});
|
||||
|
||||
// MCP details have no body for the reason, so the ask falls back to the
|
||||
// literal-text info confirmation the pre-merge hook bounce used (#13687).
|
||||
expect(waiting.confirmationDetails.type).toBe('info');
|
||||
const details = waiting.confirmationDetails as {
|
||||
title: string;
|
||||
prompt: string;
|
||||
renderPromptAsPlainText?: boolean;
|
||||
hideAlwaysAllow?: boolean;
|
||||
};
|
||||
expect(details.title).toBe(
|
||||
'Hook requested confirmation to run context_remember',
|
||||
);
|
||||
// The reason stays literal and first; the destination follows it, because
|
||||
// the info dialog renders no title and carries no server field.
|
||||
expect(details.prompt).toBe(
|
||||
'Save this exact content to the bound Mem0 repository memory?\n' +
|
||||
'[visible](https://hidden.example/target)\n\n' +
|
||||
'MCP Server: external-context\nTool: context_remember',
|
||||
);
|
||||
expect(details.renderPromptAsPlainText).toBe(true);
|
||||
expect(details.hideAlwaysAllow).toBe(true);
|
||||
});
|
||||
|
||||
it('still blocks a hook-asked MCP tool call in plan mode', async () => {
|
||||
const mcpConfirmation = async (): Promise<ToolCallConfirmationDetails> => ({
|
||||
type: 'mcp',
|
||||
title: 'Confirm MCP Tool Execution',
|
||||
serverName: 'external-context',
|
||||
toolName: 'context_remember',
|
||||
toolDisplayName: 'context_remember (external-context MCP Server)',
|
||||
onConfirm: async () => {},
|
||||
});
|
||||
const { onAllToolCallsComplete } = await scheduleWithAsk({
|
||||
approvalMode: ApprovalMode.PLAN,
|
||||
configOverrides: { getSdkMode: () => false },
|
||||
messageBus: askMessageBus('Save this exact content?'),
|
||||
tools: [
|
||||
new MockTool({
|
||||
name: 'mcpTool',
|
||||
execute: vi.fn().mockResolvedValue(textResult('ok')),
|
||||
getConfirmationDetails: mcpConfirmation,
|
||||
}),
|
||||
],
|
||||
});
|
||||
|
||||
// The ask rewrite must not turn the blocked MCP call into an approvable
|
||||
// `info` dialog: plan mode blocks it before any prompt is shown.
|
||||
const [completed] = (await settledLastBatch(
|
||||
onAllToolCallsComplete,
|
||||
)) as CompletedToolCall[];
|
||||
expectStatus(completed, 'error');
|
||||
expect(JSON.stringify(completed.response.responseParts)).toContain(
|
||||
'Tool blocked by plan mode',
|
||||
);
|
||||
});
|
||||
|
||||
it('does not let AUTO_EDIT auto-approve a hook-asked MCP call', async () => {
|
||||
const mcpConfirmation = async (): Promise<ToolCallConfirmationDetails> => ({
|
||||
type: 'mcp',
|
||||
title: 'Confirm MCP Tool Execution',
|
||||
serverName: 'external-context',
|
||||
toolName: 'context_remember',
|
||||
toolDisplayName: 'context_remember (external-context MCP Server)',
|
||||
onConfirm: async () => {},
|
||||
});
|
||||
const execute = vi.fn().mockResolvedValue(textResult('ok'));
|
||||
const { waiting } = await askUntilApproval({
|
||||
approvalMode: ApprovalMode.AUTO_EDIT,
|
||||
messageBus: askMessageBus('Save this exact content?'),
|
||||
tools: [
|
||||
new MockTool({
|
||||
name: 'mcpTool',
|
||||
execute,
|
||||
getConfirmationDetails: mcpConfirmation,
|
||||
}),
|
||||
],
|
||||
});
|
||||
|
||||
// The ask rewrite makes the details an approvable `info` shape, so the
|
||||
// `!preToolUseAsk` term is the only thing between AUTO_EDIT and a silently
|
||||
// executed write the hook explicitly asked the user about.
|
||||
expect(waiting.confirmationDetails.type).toBe('info');
|
||||
expect(execute).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('executes the tool exactly once when the user approves an ask (no re-ask loop)', async () => {
|
||||
const execute = vi.fn().mockResolvedValue(textResult('ok'));
|
||||
const messageBus = askMessageBus();
|
||||
|
|
|
|||
|
|
@ -1608,6 +1608,20 @@ function withPreToolUseAskReason(
|
|||
prompt: `${reason}\n\n${details.prompt}`,
|
||||
renderPromptAsPlainText: true,
|
||||
};
|
||||
case 'mcp':
|
||||
// MCP details have no body that can carry the reason, so the ask falls
|
||||
// back to a literal-text info confirmation, as the pre-merge bounce did.
|
||||
// The `info` shape has no server field and its `title` is not rendered in
|
||||
// the TUI, so the destination is named in the body: the hook's reason
|
||||
// alone need not identify which server receives the write.
|
||||
return {
|
||||
type: 'info',
|
||||
title: `Hook requested confirmation to run ${details.toolName}`,
|
||||
prompt: `${reason}\n\nMCP Server: ${details.serverName}\nTool: ${details.toolName}`,
|
||||
renderPromptAsPlainText: true,
|
||||
hideAlwaysAllow: true,
|
||||
onConfirm: details.onConfirm,
|
||||
};
|
||||
default:
|
||||
return details;
|
||||
}
|
||||
|
|
@ -4155,6 +4169,10 @@ export class CoreToolScheduler {
|
|||
throw new Error('Unable to prepare shell confirmation.');
|
||||
}
|
||||
|
||||
// Plan mode judges the tool's own confirmation shape: the ask
|
||||
// rewrite must not turn a blocked MCP call into an approvable
|
||||
// `info` dialog.
|
||||
const planModeDetails = confirmationDetails;
|
||||
if (preToolUseAsk) {
|
||||
confirmationDetails = withPreToolUseAskReason(
|
||||
confirmationDetails,
|
||||
|
|
@ -4171,7 +4189,7 @@ export class CoreToolScheduler {
|
|||
isPlanMode,
|
||||
isExitPlanModeTool,
|
||||
isAskUserQuestionTool,
|
||||
confirmationDetails,
|
||||
planModeDetails,
|
||||
isEnterPlanModeTool,
|
||||
)
|
||||
) {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue