From d5dfae6bbf7e94bd2a1d912489f470d188e0e346 Mon Sep 17 00:00:00 2001 From: Adib234 <30782825+Adib234@users.noreply.github.com> Date: Fri, 13 Feb 2026 10:03:52 -0500 Subject: [PATCH] fix(plan): make question type required in AskUser tool (#18959) --- .../src/ui/components/AskUserDialog.test.tsx | 31 ++++++++++ .../ui/components/BubblingRegression.test.tsx | 3 +- packages/core/src/confirmation-bus/types.ts | 6 +- packages/core/src/tools/ask-user.test.ts | 56 ++++++++++++------- packages/core/src/tools/ask-user.ts | 10 ++-- 5 files changed, 77 insertions(+), 29 deletions(-) diff --git a/packages/cli/src/ui/components/AskUserDialog.test.tsx b/packages/cli/src/ui/components/AskUserDialog.test.tsx index b93db2a2af..b03f375f71 100644 --- a/packages/cli/src/ui/components/AskUserDialog.test.tsx +++ b/packages/cli/src/ui/components/AskUserDialog.test.tsx @@ -37,6 +37,7 @@ describe('AskUserDialog', () => { { question: 'Which authentication method should we use?', header: 'Auth', + type: QuestionType.CHOICE, options: [ { label: 'OAuth 2.0', description: 'Industry standard, supports SSO' }, { label: 'JWT tokens', description: 'Stateless, good for APIs' }, @@ -74,6 +75,7 @@ describe('AskUserDialog', () => { { question: 'Which features?', header: 'Features', + type: QuestionType.CHOICE, options: [ { label: 'TypeScript', description: '' }, { label: 'ESLint', description: '' }, @@ -171,6 +173,7 @@ describe('AskUserDialog', () => { { question: 'Which authentication method?', header: 'Auth', + type: QuestionType.CHOICE, options: [{ label: 'OAuth 2.0', description: '' }], multiSelect: false, }, @@ -228,6 +231,7 @@ describe('AskUserDialog', () => { { question: 'Choose an option', header: 'Scroll Test', + type: QuestionType.CHOICE, options: Array.from({ length: 15 }, (_, i) => ({ label: `Option ${i + 1}`, description: `Description ${i + 1}`, @@ -296,6 +300,7 @@ describe('AskUserDialog', () => { { question: 'Which database should we use?', header: 'Database', + type: QuestionType.CHOICE, options: [ { label: 'PostgreSQL', description: 'Relational database' }, { label: 'MongoDB', description: 'Document database' }, @@ -305,6 +310,7 @@ describe('AskUserDialog', () => { { question: 'Which ORM do you prefer?', header: 'ORM', + type: QuestionType.CHOICE, options: [ { label: 'Prisma', description: 'Type-safe ORM' }, { label: 'Drizzle', description: 'Lightweight ORM' }, @@ -359,12 +365,14 @@ describe('AskUserDialog', () => { { question: 'Which testing framework?', header: 'Testing', + type: QuestionType.CHOICE, options: [{ label: 'Vitest', description: 'Fast unit testing' }], multiSelect: false, }, { question: 'Which CI provider?', header: 'CI', + type: QuestionType.CHOICE, options: [ { label: 'GitHub Actions', description: 'Built into GitHub' }, ], @@ -402,12 +410,14 @@ describe('AskUserDialog', () => { { question: 'Which package manager?', header: 'Package', + type: QuestionType.CHOICE, options: [{ label: 'pnpm', description: 'Fast, disk efficient' }], multiSelect: false, }, { question: 'Which bundler?', header: 'Bundler', + type: QuestionType.CHOICE, options: [{ label: 'Vite', description: 'Next generation bundler' }], multiSelect: false, }, @@ -465,6 +475,7 @@ describe('AskUserDialog', () => { { question: 'Which framework?', header: 'Framework', + type: QuestionType.CHOICE, options: [ { label: 'React', description: 'Component library' }, { label: 'Vue', description: 'Progressive framework' }, @@ -474,6 +485,7 @@ describe('AskUserDialog', () => { { question: 'Which styling?', header: 'Styling', + type: QuestionType.CHOICE, options: [ { label: 'Tailwind', description: 'Utility-first CSS' }, { label: 'CSS Modules', description: 'Scoped styles' }, @@ -500,12 +512,14 @@ describe('AskUserDialog', () => { { question: 'Create tests?', header: 'Tests', + type: QuestionType.CHOICE, options: [{ label: 'Yes', description: 'Generate test files' }], multiSelect: false, }, { question: 'Add documentation?', header: 'Docs', + type: QuestionType.CHOICE, options: [{ label: 'Yes', description: 'Generate JSDoc comments' }], multiSelect: false, }, @@ -545,12 +559,14 @@ describe('AskUserDialog', () => { { question: 'Which license?', header: 'License', + type: QuestionType.CHOICE, options: [{ label: 'MIT', description: 'Permissive license' }], multiSelect: false, }, { question: 'Include README?', header: 'README', + type: QuestionType.CHOICE, options: [{ label: 'Yes', description: 'Generate README.md' }], multiSelect: false, }, @@ -580,12 +596,14 @@ describe('AskUserDialog', () => { { question: 'Target Node version?', header: 'Node', + type: QuestionType.CHOICE, options: [{ label: 'Node 20', description: 'LTS version' }], multiSelect: false, }, { question: 'Enable strict mode?', header: 'Strict', + type: QuestionType.CHOICE, options: [{ label: 'Yes', description: 'Strict TypeScript' }], multiSelect: false, }, @@ -727,6 +745,7 @@ describe('AskUserDialog', () => { { question: 'Should it be async?', header: 'Async', + type: QuestionType.CHOICE, options: [ { label: 'Yes', description: 'Use async/await' }, { label: 'No', description: 'Synchronous hook' }, @@ -773,6 +792,7 @@ describe('AskUserDialog', () => { { question: 'Which styling approach?', header: 'Style', + type: QuestionType.CHOICE, options: [ { label: 'CSS Modules', description: 'Scoped CSS' }, { label: 'Tailwind', description: 'Utility classes' }, @@ -895,6 +915,7 @@ describe('AskUserDialog', () => { { question: 'Choice Q?', header: 'Choice', + type: QuestionType.CHOICE, options: [{ label: 'Option 1', description: '' }], multiSelect: false, }, @@ -952,12 +973,14 @@ describe('AskUserDialog', () => { { question: 'Question 1?', header: 'Q1', + type: QuestionType.CHOICE, options: [{ label: 'A1', description: '' }], multiSelect: false, }, { question: 'Question 2?', header: 'Q2', + type: QuestionType.CHOICE, options: [{ label: 'A2', description: '' }], multiSelect: false, }, @@ -1008,6 +1031,7 @@ describe('AskUserDialog', () => { { question: 'Which option do you prefer?', header: 'Test', + type: QuestionType.CHOICE, options: [{ label: 'Yes', description: '' }], multiSelect: false, }, @@ -1036,6 +1060,7 @@ describe('AskUserDialog', () => { { question: 'Is **this** working?', header: 'Test', + type: QuestionType.CHOICE, options: [{ label: 'Yes', description: '' }], multiSelect: false, }, @@ -1067,6 +1092,7 @@ describe('AskUserDialog', () => { { question: 'Is **this** working?', header: 'Test', + type: QuestionType.CHOICE, options: [{ label: 'Yes', description: '' }], multiSelect: false, }, @@ -1096,6 +1122,7 @@ describe('AskUserDialog', () => { { question: 'Run `npm start`?', header: 'Test', + type: QuestionType.CHOICE, options: [{ label: 'Yes', description: '' }], multiSelect: false, }, @@ -1126,6 +1153,7 @@ describe('AskUserDialog', () => { { question: 'Choose an option', header: 'Context Test', + type: QuestionType.CHOICE, options: Array.from({ length: 10 }, (_, i) => ({ label: `Option ${i + 1}`, description: `Description ${i + 1}`, @@ -1162,6 +1190,7 @@ describe('AskUserDialog', () => { { question: longQuestion, header: 'Alternate Buffer Test', + type: QuestionType.CHOICE, options: [{ label: 'Option 1', description: 'Desc 1' }], multiSelect: false, }, @@ -1195,6 +1224,7 @@ describe('AskUserDialog', () => { { question: 'Select your preferred language:', header: 'Language', + type: QuestionType.CHOICE, options: [ { label: 'TypeScript', description: '' }, { label: 'JavaScript', description: '' }, @@ -1228,6 +1258,7 @@ describe('AskUserDialog', () => { { question: 'Select your preferred language:', header: 'Language', + type: QuestionType.CHOICE, options: [ { label: 'TypeScript', description: '' }, { label: 'JavaScript', description: '' }, diff --git a/packages/cli/src/ui/components/BubblingRegression.test.tsx b/packages/cli/src/ui/components/BubblingRegression.test.tsx index f91f6fe2dc..b91943b019 100644 --- a/packages/cli/src/ui/components/BubblingRegression.test.tsx +++ b/packages/cli/src/ui/components/BubblingRegression.test.tsx @@ -9,7 +9,7 @@ import { act } from 'react'; import { renderWithProviders } from '../../test-utils/render.js'; import { waitFor } from '../../test-utils/async.js'; import { AskUserDialog } from './AskUserDialog.js'; -import type { Question } from '@google/gemini-cli-core'; +import { QuestionType, type Question } from '@google/gemini-cli-core'; describe('Key Bubbling Regression', () => { afterEach(() => { @@ -20,6 +20,7 @@ describe('Key Bubbling Regression', () => { { question: 'Choice Q?', header: 'Choice', + type: QuestionType.CHOICE, options: [ { label: 'Option 1', description: '' }, { label: 'Option 2', description: '' }, diff --git a/packages/core/src/confirmation-bus/types.ts b/packages/core/src/confirmation-bus/types.ts index 8aa21f8ca1..69aa98832e 100644 --- a/packages/core/src/confirmation-bus/types.ts +++ b/packages/core/src/confirmation-bus/types.ts @@ -147,9 +147,9 @@ export enum QuestionType { export interface Question { question: string; header: string; - /** Question type: 'choice' renders selectable options, 'text' renders free-form input, 'yesno' renders a binary Yes/No choice. Defaults to 'choice'. */ - type?: QuestionType; - /** Selectable choices. REQUIRED when type='choice' or omitted. IGNORED for 'text' and 'yesno'. */ + /** Question type: 'choice' renders selectable options, 'text' renders free-form input, 'yesno' renders a binary Yes/No choice. */ + type: QuestionType; + /** Selectable choices. REQUIRED when type='choice'. IGNORED for 'text' and 'yesno'. */ options?: QuestionOption[]; /** Allow multiple selections. Only applies when type='choice'. */ multiSelect?: boolean; diff --git a/packages/core/src/tools/ask-user.test.ts b/packages/core/src/tools/ask-user.test.ts index 19c98fbc6b..0273e2fc0d 100644 --- a/packages/core/src/tools/ask-user.test.ts +++ b/packages/core/src/tools/ask-user.test.ts @@ -131,6 +131,7 @@ describe('AskUserTool', () => { const questions = Array(5).fill({ question: 'Test?', header: 'Test', + type: QuestionType.CHOICE, options: [ { label: 'A', description: 'A' }, { label: 'B', description: 'B' }, @@ -156,7 +157,13 @@ describe('AskUserTool', () => { it('should return error if header exceeds max length', () => { const result = tool.validateToolParams({ - questions: [{ question: 'Test?', header: 'This is way too long' }], + questions: [ + { + question: 'Test?', + header: 'This is way too long', + type: QuestionType.CHOICE, + }, + ], }); expect(result).toContain('must NOT have more than 16 characters'); }); @@ -167,6 +174,7 @@ describe('AskUserTool', () => { { question: 'Test?', header: 'Test', + type: QuestionType.CHOICE, options: [{ label: 'A', description: 'A' }], }, ], @@ -182,6 +190,7 @@ describe('AskUserTool', () => { { question: 'Test?', header: 'Test', + type: QuestionType.CHOICE, options: [ { label: 'A', description: 'A' }, { label: 'B', description: 'B' }, @@ -201,6 +210,7 @@ describe('AskUserTool', () => { { question: 'Which approach?', header: 'Approach', + type: QuestionType.CHOICE, options: [ { label: 'A', description: 'Option A' }, { label: 'B', description: 'Option B' }, @@ -224,18 +234,16 @@ describe('AskUserTool', () => { expect(result).toContain("type='choice' requires 'options'"); }); - it('should return error if type is omitted and options missing (defaults to choice)', () => { + it('should return error if type is missing', () => { const result = tool.validateToolParams({ questions: [ { question: 'Pick one?', header: 'Choice', - // type omitted, defaults to 'choice' - // options missing - }, + } as unknown as Question, ], }); - expect(result).toContain("type='choice' requires 'options'"); + expect(result).toContain("must have required property 'type'"); }); it('should accept text type without options', () => { @@ -288,6 +296,7 @@ describe('AskUserTool', () => { { question: 'Pick one?', header: 'Choice', + type: QuestionType.CHOICE, options: [ { label: '', description: 'Empty label' }, { label: 'B', description: 'Option B' }, @@ -304,6 +313,7 @@ describe('AskUserTool', () => { { question: 'Pick one?', header: 'Choice', + type: QuestionType.CHOICE, options: [ { label: 'A' } as { label: string; description: string }, { label: 'B', description: 'Option B' }, @@ -318,7 +328,13 @@ describe('AskUserTool', () => { describe('validateBuildAndExecute', () => { it('should hide validation errors from returnDisplay', async () => { const params = { - questions: [{ question: 'Test?', header: 'This is way too long' }], + questions: [ + { + question: 'Test?', + header: 'This is way too long', + type: QuestionType.TEXT, + }, + ], }; const result = await tool.validateBuildAndExecute( @@ -337,7 +353,9 @@ describe('AskUserTool', () => { .mockReturnValue(null); const params = { - questions: [{ question: 'Valid?', header: 'Valid' }], + questions: [ + { question: 'Valid?', header: 'Valid', type: QuestionType.TEXT }, + ], }; const mockInvocation = { @@ -366,10 +384,11 @@ describe('AskUserTool', () => { describe('shouldConfirmExecute', () => { it('should return confirmation details with normalized questions', async () => { - const questions = [ + const questions: Question[] = [ { question: 'How should we proceed with this task?', header: 'Approach', + type: QuestionType.CHOICE, options: [ { label: 'Quick fix (Recommended)', @@ -394,12 +413,7 @@ describe('AskUserTool', () => { expect(details).not.toBe(false); if (details && details.type === 'ask_user') { expect(details.title).toBe('Ask User'); - expect(details.questions).toEqual( - questions.map((q) => ({ - ...q, - type: QuestionType.CHOICE, - })), - ); + expect(details.questions).toEqual(questions); expect(typeof details.onConfirm).toBe('function'); } else { // Type guard for TypeScript @@ -407,11 +421,12 @@ describe('AskUserTool', () => { } }); - it('should normalize question type to CHOICE when omitted', async () => { - const questions = [ + it('should use provided question type', async () => { + const questions: Question[] = [ { question: 'Which approach?', header: 'Approach', + type: QuestionType.CHOICE, options: [ { label: 'Option A', description: 'First option' }, { label: 'Option B', description: 'Second option' }, @@ -432,10 +447,11 @@ describe('AskUserTool', () => { describe('execute', () => { it('should return user answers after confirmation', async () => { - const questions = [ + const questions: Question[] = [ { question: 'How should we proceed with this task?', header: 'Approach', + type: QuestionType.CHOICE, options: [ { label: 'Quick fix (Recommended)', @@ -484,10 +500,11 @@ describe('AskUserTool', () => { }); it('should display message when user submits without answering', async () => { - const questions = [ + const questions: Question[] = [ { question: 'Which approach?', header: 'Approach', + type: QuestionType.CHOICE, options: [ { label: 'Option A', description: 'First option' }, { label: 'Option B', description: 'Second option' }, @@ -528,6 +545,7 @@ describe('AskUserTool', () => { { question: 'Which sections of the documentation should be updated?', header: 'Docs', + type: QuestionType.CHOICE, options: [ { label: 'User Guide', diff --git a/packages/core/src/tools/ask-user.ts b/packages/core/src/tools/ask-user.ts index 951094d9ad..f941af5d4c 100644 --- a/packages/core/src/tools/ask-user.ts +++ b/packages/core/src/tools/ask-user.ts @@ -42,7 +42,7 @@ export class AskUserTool extends BaseDeclarativeTool< maxItems: 4, items: { type: 'object', - required: ['question', 'header'], + required: ['question', 'header', 'type'], properties: { question: { type: 'string', @@ -111,7 +111,7 @@ export class AskUserTool extends BaseDeclarativeTool< for (let i = 0; i < params.questions.length; i++) { const q = params.questions[i]; - const questionType = q.type ?? QuestionType.CHOICE; + const questionType = q.type; // Validate that 'choice' type has options if (questionType === QuestionType.CHOICE) { @@ -186,7 +186,7 @@ export class AskUserInvocation extends BaseToolInvocation< ): Promise { const normalizedQuestions = this.params.questions.map((q) => ({ ...q, - type: q.type ?? QuestionType.CHOICE, + type: q.type, })); return { @@ -210,9 +210,7 @@ export class AskUserInvocation extends BaseToolInvocation< } async execute(_signal: AbortSignal): Promise { - const questionTypes = this.params.questions.map( - (q) => q.type ?? QuestionType.CHOICE, - ); + const questionTypes = this.params.questions.map((q) => q.type); if (this.confirmationOutcome === ToolConfirmationOutcome.Cancel) { return {