From 509c310fd4dd971d4c2824e60f06c268e0ca54c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=98=93=E8=89=AF?= <1204183885@qq.com> Date: Fri, 9 Oct 2026 02:28:32 +0000 Subject: [PATCH] fix(core): surface PreToolUse ask content on MCP tool confirmations (#13687) (#13697) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 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 Patrol-Run: qwen-pr-closeout/jmuzzjnorfk --------- Co-authored-by: Qwen-Coder --- .../external-context-mem0-write.test.ts | 31 ++--- .../core/src/core/coreToolScheduler.test.ts | 109 ++++++++++++++++++ packages/core/src/core/coreToolScheduler.ts | 20 +++- 3 files changed, 136 insertions(+), 24 deletions(-) diff --git a/integration-tests/interactive/external-context-mem0-write.test.ts b/integration-tests/interactive/external-context-mem0-write.test.ts index 02e9f8a04f..26cdaea3ac 100644 --- a/integration-tests/interactive/external-context-mem0-write.test.ts +++ b/integration-tests/interactive/external-context-mem0-write.test.ts @@ -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 under-value', 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 under-value', 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, diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index acaafc6d0d..499241054b 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -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 => ({ + 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 => ({ + 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 => ({ + 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(); diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 9ec925ac6d..1ccd55af83 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -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, ) ) {