From 90d604d4c2ca23235a7e80f218c5bd7f4de4c913 Mon Sep 17 00:00:00 2001 From: qqqys Date: Wed, 26 Aug 2026 20:57:03 +0800 Subject: [PATCH 1/3] fix(core): keep the ask_user_question dialog behind allow rules and auto-approval MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The confirmation dialog is this tool: answers are collected through onConfirm. Approving it without the dialog does not allow the tool, it silently answers "declined" on the user's behalf, and execute() then reports "User declined to answer the questions." as a successful result. Any bare ask_user_question allow rule does exactly that — a skill's allowedTools grant (applied session-wide via applySkillAllowedTools), a permissions.allow entry, or an "always allow" answer — and once one has loaded, every later question in the session loses its dialog too. The invocation now declares requiresUserInteraction() whenever a host can show the dialog (interactive TUI, ACP / stream-json hosts), so the permission flow forces 'ask' regardless of L4 allow rules and the scheduler never auto-approves it beside a sibling. Headless runs are unchanged: nothing can prompt there, the flag stays false, and execute() keeps returning its existing non-interactive message. The three mode checks are folded into one canCollectAnswers() helper so the permission default, the flag, and execute() cannot drift apart. Tests pin the flag per mode and, at the permissionFlow level with the real tool, the real PermissionManager and the real allowedTools grant, that the grant overrides the default to allow at L4 yet the flow still yields 'ask' (and needsConfirmation is true even in YOLO), that headless still yields 'allow', and that an explicit deny rule is preserved. Follow-up to the #10002 review finding on the ask_user_question grant. Claude-Session: https://claude.ai/code/session_01FV7i3w7egJ2kMw4AhQC38Z --- packages/core/src/core/permissionFlow.test.ts | 105 ++++++++++++++++++ .../core/src/tools/askUserQuestion.test.ts | 41 +++++++ packages/core/src/tools/askUserQuestion.ts | 42 +++++-- 3 files changed, 176 insertions(+), 12 deletions(-) diff --git a/packages/core/src/core/permissionFlow.test.ts b/packages/core/src/core/permissionFlow.test.ts index 5710cfdbd63..66387da2ba5 100644 --- a/packages/core/src/core/permissionFlow.test.ts +++ b/packages/core/src/core/permissionFlow.test.ts @@ -18,6 +18,9 @@ import { isPlanModeBlocked, isAutoEditApproved, } from './permissionFlow.js'; +import { AskUserQuestionTool } from '../tools/askUserQuestion.js'; +import { PermissionManager } from '../permissions/permission-manager.js'; +import { applySkillAllowedTools } from '../tools/skill-utils.js'; // Mock types for testing const mockConfig = (overrides: Partial = {}): Config => @@ -217,6 +220,108 @@ describe('evaluatePermissionFlow', () => { }); }); +describe('evaluatePermissionFlow with ask_user_question', () => { + const questions = [ + { + question: 'Which check defines success?', + header: 'Check', + options: [ + { label: 'npm test', description: 'exit code 0' }, + { label: 'npm run lint', description: 'no warnings' }, + ], + multiSelect: false, + }, + ]; + + const askConfig = (interactive: boolean) => + ({ + isInteractive: vi.fn().mockReturnValue(interactive), + getApprovalMode: vi.fn().mockReturnValue(ApprovalMode.DEFAULT), + getTargetDir: vi.fn().mockReturnValue('/test'), + getExperimentalZedIntegration: vi.fn().mockReturnValue(false), + getInputFormat: vi.fn().mockReturnValue(undefined), + }) as unknown as Config; + + const pmWithSkillGrant = () => { + const pm = new PermissionManager({ + getPermissionsAllow: () => [], + getPermissionsAsk: () => [], + getPermissionsDeny: () => [], + getApprovalMode: () => ApprovalMode.DEFAULT, + }); + pm.initialize(); + // Exactly what loading a skill whose SKILL.md lists + // `allowedTools: [ask_user_question]` does to the session. + applySkillAllowedTools(pm, [ToolNames.ASK_USER_QUESTION]); + return pm; + }; + + it("keeps the dialog when a skill's allowedTools grant would otherwise allow the tool", async () => { + const config = askConfig(true); + const pm = pmWithSkillGrant(); + const invocation = new AskUserQuestionTool(config).build({ questions }); + + const result = await evaluatePermissionFlow( + { ...config, getPermissionManager: () => pm } as unknown as Config, + invocation, + ToolNames.ASK_USER_QUESTION, + { questions }, + ); + + // The grant did override the 'ask' default at L4 … + expect(result.defaultPermission).toBe('ask'); + expect(await pm.evaluate(result.pmCtx)).toBe('allow'); + // … but the invocation still reaches the user, in every approval mode. + expect(result.requiresUserInteraction).toBe(true); + expect(result.finalPermission).toBe('ask'); + expect( + needsConfirmation( + result.finalPermission, + ApprovalMode.YOLO, + ToolNames.ASK_USER_QUESTION, + result.requiresUserInteraction, + ), + ).toBe(true); + }); + + it('still lets headless runs skip the tool, where nothing can prompt', async () => { + const config = askConfig(false); + const pm = pmWithSkillGrant(); + const invocation = new AskUserQuestionTool(config).build({ questions }); + + const result = await evaluatePermissionFlow( + { ...config, getPermissionManager: () => pm } as unknown as Config, + invocation, + ToolNames.ASK_USER_QUESTION, + { questions }, + ); + + expect(result.requiresUserInteraction).toBe(false); + expect(result.finalPermission).toBe('allow'); + }); + + it('preserves an explicit deny rule for ask_user_question', async () => { + const config = askConfig(true); + const pm = new PermissionManager({ + getPermissionsAllow: () => [], + getPermissionsAsk: () => [], + getPermissionsDeny: () => [ToolNames.ASK_USER_QUESTION], + getApprovalMode: () => ApprovalMode.DEFAULT, + }); + pm.initialize(); + const invocation = new AskUserQuestionTool(config).build({ questions }); + + const result = await evaluatePermissionFlow( + { ...config, getPermissionManager: () => pm } as unknown as Config, + invocation, + ToolNames.ASK_USER_QUESTION, + { questions }, + ); + + expect(result.finalPermission).toBe('deny'); + }); +}); + describe('needsConfirmation', () => { it('should return false for YOLO mode non-ask_user_question tools', () => { expect(needsConfirmation('ask', ApprovalMode.YOLO, 'shell')).toBe(false); diff --git a/packages/core/src/tools/askUserQuestion.test.ts b/packages/core/src/tools/askUserQuestion.test.ts index f8d32ea8c1d..f6abe14c4d1 100644 --- a/packages/core/src/tools/askUserQuestion.test.ts +++ b/packages/core/src/tools/askUserQuestion.test.ts @@ -203,6 +203,47 @@ describe('AskUserQuestionTool', () => { }); }); + describe('requiresUserInteraction', () => { + const params = { + questions: [ + { + question: 'Pick a framework?', + header: 'Framework', + options: [ + { label: 'React', description: 'A JavaScript library' }, + { label: 'Vue', description: 'Progressive framework' }, + ], + multiSelect: false, + }, + ], + }; + + it('requires the dialog in interactive mode so allow rules cannot skip it', () => { + // A bare `ask_user_question` allow rule (a skill's `allowedTools` + // grant, permissions.allow, "always allow") overrides the 'ask' + // default at L4. Without this flag the scheduler would then run the + // tool with no dialog and execute() would report "declined". + const invocation = tool.build(params); + expect(invocation.requiresUserInteraction?.()).toBe(true); + }); + + it('requires the dialog for ACP hosts that run non-interactively', () => { + (mockConfig.isInteractive as Mock).mockReturnValue(false); + (mockConfig.getInputFormat as Mock).mockReturnValue('stream-json'); + expect(tool.build(params).requiresUserInteraction?.()).toBe(true); + + (mockConfig.getInputFormat as Mock).mockReturnValue(undefined); + (mockConfig.getExperimentalZedIntegration as Mock).mockReturnValue(true); + expect(tool.build(params).requiresUserInteraction?.()).toBe(true); + }); + + it('does not require a dialog in headless mode, where nothing can prompt', () => { + (mockConfig.isInteractive as Mock).mockReturnValue(false); + const invocation = tool.build(params); + expect(invocation.requiresUserInteraction?.()).toBe(false); + }); + }); + describe('execute', () => { it('should return error in non-interactive mode', async () => { (mockConfig.isInteractive as Mock).mockReturnValue(false); diff --git a/packages/core/src/tools/askUserQuestion.ts b/packages/core/src/tools/askUserQuestion.ts index 9a737f55b5d..46c44209eb3 100644 --- a/packages/core/src/tools/askUserQuestion.ts +++ b/packages/core/src/tools/askUserQuestion.ts @@ -172,22 +172,46 @@ class AskUserQuestionToolInvocation extends BaseToolInvocation< } /** - * ask_user_question always requires user confirmation so the user can - * provide answers. In non-interactive mode without ACP support, we skip - * confirmation (and subsequently skip execution). + * Whether a host is present that can put the questions in front of the + * user. ACP hosts (VSCode extension, Zed, stream-json clients) run in + * non-interactive mode but still collect answers through the + * confirmation channel. */ - override async getDefaultPermission(): Promise { + private canCollectAnswers(): boolean { const isAcpMode = this._config.getExperimentalZedIntegration() || this._config.getInputFormat() === InputFormat.STREAM_JSON; + return this._config.isInteractive() || isAcpMode; + } - if (!this._config.isInteractive() && !isAcpMode) { + /** + * ask_user_question always requires user confirmation so the user can + * provide answers. In non-interactive mode without ACP support, we skip + * confirmation (and subsequently skip execution). + */ + override async getDefaultPermission(): Promise { + if (!this.canCollectAnswers()) { // Non-interactive + no ACP: skip entirely return 'allow'; } return 'ask'; } + /** + * The confirmation dialog IS this tool: the answers are collected through + * `onConfirm`, so an approval that skips the dialog does not "allow" the + * tool, it silently answers "declined" on the user's behalf. Permission + * rules and automatic approval modes must therefore never satisfy it — + * a bare `ask_user_question` allow rule (a skill's `allowedTools` grant, + * `permissions.allow`, an "always allow" answer) would otherwise override + * the 'ask' default at L4 and the scheduler would run the tool with no + * dialog ever shown. Headless runs stay as they were: nothing can prompt + * there, and `execute()` reports that instead. + */ + override requiresUserInteraction(): boolean { + return this.canCollectAnswers(); + } + override async getConfirmationDetails( _abortSignal: AbortSignal, ): Promise { @@ -222,14 +246,8 @@ class AskUserQuestionToolInvocation extends BaseToolInvocation< async execute(_signal: AbortSignal): Promise { try { - // Check if we're in a mode that supports user interaction - // ACP mode (VSCode extension, etc.) uses non-interactive mode but can still collect user input - const isAcpMode = - this._config.getExperimentalZedIntegration() || - this._config.getInputFormat() === InputFormat.STREAM_JSON; - // In non-interactive mode without ACP support, we cannot collect user input - if (!this._config.isInteractive() && !isAcpMode) { + if (!this.canCollectAnswers()) { const errorMessage = 'Cannot ask user questions in non-interactive mode without ACP support. Please run in interactive mode or enable ACP mode to use this tool.'; return { From 62aac5b85d3f5d861b657ce911cde6525145ee83 Mon Sep 17 00:00:00 2001 From: qqqys <266654365+qqqys@users.noreply.github.com> Date: Wed, 26 Aug 2026 21:35:42 +0800 Subject: [PATCH 2/3] fix(cli): preserve interactive tool payloads --- .../control/controllers/permissionController.test.ts | 3 +++ .../control/controllers/permissionController.ts | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/nonInteractive/control/controllers/permissionController.test.ts b/packages/cli/src/nonInteractive/control/controllers/permissionController.test.ts index 349bcd55319..39b97295b8c 100644 --- a/packages/cli/src/nonInteractive/control/controllers/permissionController.test.ts +++ b/packages/cli/src/nonInteractive/control/controllers/permissionController.test.ts @@ -541,6 +541,9 @@ describe('PermissionController', () => { name: 'ask_user_question', args: { questions: [] } as Record, }, + invocation: { + requiresUserInteraction: () => true, + }, confirmationDetails: { type: 'ask_user_question', title: 'Please answer', diff --git a/packages/cli/src/nonInteractive/control/controllers/permissionController.ts b/packages/cli/src/nonInteractive/control/controllers/permissionController.ts index 3652b634c79..9a2cbdd83e0 100644 --- a/packages/cli/src/nonInteractive/control/controllers/permissionController.ts +++ b/packages/cli/src/nonInteractive/control/controllers/permissionController.ts @@ -574,7 +574,10 @@ export class PermissionController extends BaseController { const behavior = String(payload['behavior'] || '').toLowerCase(); if (behavior === 'allow') { - if (requiresUserInteraction) { + if ( + requiresUserInteraction && + toolCall.request.name === ToolNames.EXIT_PLAN_MODE + ) { await toolCall.confirmationDetails.onConfirm( ToolConfirmationOutcome.ProceedOnce, ); From 1190f0b0dfb335515417c2a197c9708760e67afa Mon Sep 17 00:00:00 2001 From: qqqys <266654365+qqqys@users.noreply.github.com> Date: Wed, 26 Aug 2026 16:15:35 +0000 Subject: [PATCH 3/3] refactor: delegate ask_user_question host check to resolveInteractionMode (#10160) canCollectAnswers() was a fifth copy of the host-capability predicate; delegating to resolveInteractionMode keeps one source of truth shared with the tool-registration gate. Also pin the stream-json payload contract in permissionController with a comment: only exit_plan_mode may approve through the payload-less branch. --- .../control/controllers/permissionController.ts | 5 +++++ packages/core/src/tools/askUserQuestion.ts | 7 ++----- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/nonInteractive/control/controllers/permissionController.ts b/packages/cli/src/nonInteractive/control/controllers/permissionController.ts index 9a2cbdd83e0..de61ca87a28 100644 --- a/packages/cli/src/nonInteractive/control/controllers/permissionController.ts +++ b/packages/cli/src/nonInteractive/control/controllers/permissionController.ts @@ -574,6 +574,11 @@ export class PermissionController extends BaseController { const behavior = String(payload['behavior'] || '').toLowerCase(); if (behavior === 'allow') { + // exit_plan_mode approves through the dialog alone: its onConfirm + // takes no payload, and the approved plan must not be replaced by + // the host's updatedInput. Any other requiresUserInteraction tool + // (e.g. ask_user_question) must take the updatedInput path below — + // that channel carries the user's answers. if ( requiresUserInteraction && toolCall.request.name === ToolNames.EXIT_PLAN_MODE diff --git a/packages/core/src/tools/askUserQuestion.ts b/packages/core/src/tools/askUserQuestion.ts index 46c44209eb3..be1033ce72c 100644 --- a/packages/core/src/tools/askUserQuestion.ts +++ b/packages/core/src/tools/askUserQuestion.ts @@ -20,7 +20,7 @@ import type { FunctionDeclaration } from '@google/genai'; import type { Config } from '../config/config.js'; import { ToolDisplayNames, ToolNames } from './tool-names.js'; import { createDebugLogger } from '../utils/debugLogger.js'; -import { InputFormat } from '../output/types.js'; +import { resolveInteractionMode } from '../core/prompts.js'; const debugLogger = createDebugLogger('ASK_USER_QUESTION'); @@ -178,10 +178,7 @@ class AskUserQuestionToolInvocation extends BaseToolInvocation< * confirmation channel. */ private canCollectAnswers(): boolean { - const isAcpMode = - this._config.getExperimentalZedIntegration() || - this._config.getInputFormat() === InputFormat.STREAM_JSON; - return this._config.isInteractive() || isAcpMode; + return resolveInteractionMode(this._config) !== 'headless'; } /**