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..de61ca87a28 100644 --- a/packages/cli/src/nonInteractive/control/controllers/permissionController.ts +++ b/packages/cli/src/nonInteractive/control/controllers/permissionController.ts @@ -574,7 +574,15 @@ export class PermissionController extends BaseController { const behavior = String(payload['behavior'] || '').toLowerCase(); if (behavior === 'allow') { - if (requiresUserInteraction) { + // 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 + ) { await toolCall.confirmationDetails.onConfirm( ToolConfirmationOutcome.ProceedOnce, ); 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..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'); @@ -171,23 +171,44 @@ class AskUserQuestionToolInvocation extends BaseToolInvocation< return `Ask user ${questionCount} question${questionCount > 1 ? 's' : ''}`; } + /** + * 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. + */ + private canCollectAnswers(): boolean { + return resolveInteractionMode(this._config) !== 'headless'; + } + /** * 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 { - const isAcpMode = - this._config.getExperimentalZedIntegration() || - this._config.getInputFormat() === InputFormat.STREAM_JSON; - - if (!this._config.isInteractive() && !isAcpMode) { + 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 +243,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 {