From 61959f66a9ddf6e164e7fcc68035d60a179a7695 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=AD=A6=E7=94=B0=20=E6=86=B2=E5=A4=AA=E9=83=8E?= Date: Mon, 23 Feb 2026 15:17:09 +0900 Subject: [PATCH] =?UTF-8?q?feat:=20AskUserQuestion=20=E5=AF=BE=E5=BF=9C=20?= =?UTF-8?q?(#161)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/__tests__/StreamDisplay.test.ts | 87 ++++ .../ask-user-question-handler.test.ts | 492 ++++++++++++++++++ src/__tests__/it-notification-sound.test.ts | 10 +- src/__tests__/it-sigint-interrupt.test.ts | 10 +- src/__tests__/opencode-client-cleanup.test.ts | 62 +++ .../pieceExecution-ask-user-question.test.ts | 200 +++++++ .../pieceExecution-debug-prompts.test.ts | 10 +- .../pieceExecution-session-loading.test.ts | 10 +- src/core/piece/ask-user-question-error.ts | 37 ++ src/core/piece/index.ts | 3 + src/features/tasks/execute/pieceExecution.ts | 3 +- src/infra/claude/ask-user-question-handler.ts | 28 + src/infra/claude/ask-user-question-tty.ts | 172 ++++++ src/infra/claude/options-builder.ts | 13 +- src/infra/opencode/client.ts | 36 +- src/shared/ui/StreamDisplay.ts | 8 +- 16 files changed, 1151 insertions(+), 30 deletions(-) create mode 100644 src/__tests__/ask-user-question-handler.test.ts create mode 100644 src/__tests__/pieceExecution-ask-user-question.test.ts create mode 100644 src/core/piece/ask-user-question-error.ts create mode 100644 src/infra/claude/ask-user-question-handler.ts create mode 100644 src/infra/claude/ask-user-question-tty.ts diff --git a/src/__tests__/StreamDisplay.test.ts b/src/__tests__/StreamDisplay.test.ts index 488365a..a44e27f 100644 --- a/src/__tests__/StreamDisplay.test.ts +++ b/src/__tests__/StreamDisplay.test.ts @@ -249,6 +249,93 @@ describe('StreamDisplay', () => { }); }); + describe('showToolUse spinner suppression', () => { + it('should not start spinner for AskUserQuestion tool', () => { + vi.useFakeTimers(); + try { + const display = new StreamDisplay('test-agent', false); + display.showToolUse('AskUserQuestion', { questions: [] }); + + // Advance time past spinner interval (80ms) + vi.advanceTimersByTime(200); + + // Spinner writes to stdout via setInterval — should NOT have been called + expect(stdoutWriteSpy).not.toHaveBeenCalled(); + + display.flush(); + } finally { + vi.useRealTimers(); + } + }); + + it('should start spinner for non-AskUserQuestion tools', () => { + vi.useFakeTimers(); + try { + const display = new StreamDisplay('test-agent', false); + display.showToolUse('Bash', { command: 'ls' }); + + // Advance time past spinner interval (80ms) + vi.advanceTimersByTime(200); + + // Spinner should have written to stdout + expect(stdoutWriteSpy).toHaveBeenCalled(); + + display.flush(); + } finally { + vi.useRealTimers(); + } + }); + }); + + describe('showToolResult AskUserQuestion content suppression', () => { + it('should suppress content preview for AskUserQuestion non-error result', () => { + const display = new StreamDisplay('test-agent', false); + display.showToolUse('AskUserQuestion', { questions: [] }); + display.showToolResult('Error: Answer questions?', false); + + // Find the result line with ✓ + const resultLine = consoleLogSpy.mock.calls.find( + (call) => typeof call[0] === 'string' && (call[0] as string).includes('✓'), + ); + expect(resultLine).toBeDefined(); + + // Should show only "✓ AskUserQuestion" without content preview + const fullOutput = resultLine!.join(' '); + expect(fullOutput).toContain('AskUserQuestion'); + expect(fullOutput).not.toContain('Error:'); + expect(fullOutput).not.toContain('Answer questions'); + }); + + it('should still show error for AskUserQuestion when isError is true', () => { + const display = new StreamDisplay('test-agent', false); + display.showToolUse('AskUserQuestion', { questions: [] }); + display.showToolResult('Something went wrong', true); + + // Find the error line with ✗ + const errorLine = consoleLogSpy.mock.calls.find( + (call) => typeof call[0] === 'string' && (call[0] as string).includes('✗'), + ); + expect(errorLine).toBeDefined(); + const fullOutput = errorLine!.join(' '); + expect(fullOutput).toContain('AskUserQuestion'); + expect(fullOutput).toContain('Something went wrong'); + }); + + it('should still show content preview for non-AskUserQuestion tools', () => { + const display = new StreamDisplay('test-agent', false); + display.showToolUse('Read', { file_path: '/test.ts' }); + display.showToolResult('File content here', false); + + const resultLine = consoleLogSpy.mock.calls.find( + (call) => typeof call[0] === 'string' && (call[0] as string).includes('✓'), + ); + expect(resultLine).toBeDefined(); + const fullOutput = resultLine!.join(' '); + expect(fullOutput).toContain('Read'); + expect(fullOutput).toContain('File content here'); + }); + }); + describe('progress prefix format', () => { it('should format progress as (iteration/max) step index/total', () => { const progressInfo: ProgressInfo = { diff --git a/src/__tests__/ask-user-question-handler.test.ts b/src/__tests__/ask-user-question-handler.test.ts new file mode 100644 index 0000000..d44db59 --- /dev/null +++ b/src/__tests__/ask-user-question-handler.test.ts @@ -0,0 +1,492 @@ +/** + * Unit tests for ask-user-question-handler, TTY handler, + * and AskUserQuestionDeniedError handling in SdkOptionsBuilder. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { + AskUserQuestionDeniedError, + createDenyAskUserQuestionHandler, +} from '../core/piece/ask-user-question-error.js'; +import { createAskUserQuestionHandler } from '../infra/claude/ask-user-question-handler.js'; +import { SdkOptionsBuilder, buildSdkOptions } from '../infra/claude/options-builder.js'; +import type { AskUserQuestionInput, ClaudeSpawnOptions } from '../infra/claude/types.js'; + +vi.mock('../shared/prompt/select.js', () => ({ + selectOption: vi.fn(), +})); + +vi.mock('../shared/prompt/confirm.js', () => ({ + promptInput: vi.fn(), +})); + +import { selectOption } from '../shared/prompt/select.js'; +import { promptInput } from '../shared/prompt/confirm.js'; +import { createTtyAskUserQuestionHandler } from '../infra/claude/ask-user-question-tty.js'; + +const mockedSelectOption = vi.mocked(selectOption); +const mockedPromptInput = vi.mocked(promptInput); + +describe('AskUserQuestionDeniedError', () => { + it('should have the correct name', () => { + const error = new AskUserQuestionDeniedError(); + expect(error.name).toBe('AskUserQuestionDeniedError'); + }); + + it('should have a message instructing text-based output', () => { + const error = new AskUserQuestionDeniedError(); + expect(error.message).toContain('not available in non-interactive mode'); + }); + + it('should be an instance of Error', () => { + const error = new AskUserQuestionDeniedError(); + expect(error).toBeInstanceOf(Error); + }); +}); + +describe('createAskUserQuestionHandler', () => { + const originalIsTTY = process.stdin.isTTY; + const originalNoTty = process.env.TAKT_NO_TTY; + const originalTouchTty = process.env.TAKT_TEST_FLG_TOUCH_TTY; + + afterEach(() => { + Object.defineProperty(process.stdin, 'isTTY', { value: originalIsTTY, writable: true }); + if (originalNoTty === undefined) { + delete process.env.TAKT_NO_TTY; + } else { + process.env.TAKT_NO_TTY = originalNoTty; + } + if (originalTouchTty === undefined) { + delete process.env.TAKT_TEST_FLG_TOUCH_TTY; + } else { + process.env.TAKT_TEST_FLG_TOUCH_TTY = originalTouchTty; + } + }); + + it('should return a handler when TTY is available', () => { + Object.defineProperty(process.stdin, 'isTTY', { value: true, writable: true }); + delete process.env.TAKT_NO_TTY; + delete process.env.TAKT_TEST_FLG_TOUCH_TTY; + + const handler = createAskUserQuestionHandler(); + expect(handler).toBeDefined(); + expect(typeof handler).toBe('function'); + }); + + it('should return a deny handler when no TTY is available', () => { + Object.defineProperty(process.stdin, 'isTTY', { value: false, writable: true }); + delete process.env.TAKT_NO_TTY; + delete process.env.TAKT_TEST_FLG_TOUCH_TTY; + + const handler = createAskUserQuestionHandler(); + expect(handler).toBeDefined(); + expect(typeof handler).toBe('function'); + }); + + it('should return a deny handler when TAKT_NO_TTY=1', () => { + Object.defineProperty(process.stdin, 'isTTY', { value: true, writable: true }); + process.env.TAKT_NO_TTY = '1'; + delete process.env.TAKT_TEST_FLG_TOUCH_TTY; + + const handler = createAskUserQuestionHandler(); + expect(handler).toBeDefined(); + }); + + it('deny handler should throw AskUserQuestionDeniedError', () => { + Object.defineProperty(process.stdin, 'isTTY', { value: false, writable: true }); + delete process.env.TAKT_NO_TTY; + delete process.env.TAKT_TEST_FLG_TOUCH_TTY; + + const handler = createAskUserQuestionHandler(); + const dummyInput: AskUserQuestionInput = { + questions: [{ question: 'Which option?' }], + }; + + expect(() => handler(dummyInput)).toThrow(AskUserQuestionDeniedError); + }); +}); + +describe('createDenyAskUserQuestionHandler', () => { + it('should always throw AskUserQuestionDeniedError', () => { + const handler = createDenyAskUserQuestionHandler(); + const input: AskUserQuestionInput = { + questions: [{ question: 'Test?' }], + }; + + expect(() => handler(input)).toThrow(AskUserQuestionDeniedError); + }); + + it('should return a function', () => { + const handler = createDenyAskUserQuestionHandler(); + expect(typeof handler).toBe('function'); + }); +}); + +describe('createTtyAskUserQuestionHandler', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe('single-select questions', () => { + it('should return the selected option label', async () => { + mockedSelectOption.mockResolvedValue('Option A'); + + const handler = createTtyAskUserQuestionHandler(); + const input: AskUserQuestionInput = { + questions: [{ + question: 'Which library?', + header: 'Library', + options: [ + { label: 'Option A', description: 'First option' }, + { label: 'Option B', description: 'Second option' }, + ], + }], + }; + + const result = await handler(input); + + expect(result).toEqual({ 'Which library?': 'Option A' }); + expect(mockedSelectOption).toHaveBeenCalledWith( + '[Library] Which library?', + expect.arrayContaining([ + expect.objectContaining({ label: 'Option A', value: 'Option A' }), + expect.objectContaining({ label: 'Option B', value: 'Option B' }), + expect.objectContaining({ label: 'Other', value: '__other__' }), + ]), + ); + }); + + it('should prompt for text input when Other is selected', async () => { + mockedSelectOption.mockResolvedValue('__other__'); + mockedPromptInput.mockResolvedValue('Custom answer'); + + const handler = createTtyAskUserQuestionHandler(); + const input: AskUserQuestionInput = { + questions: [{ + question: 'Which option?', + options: [{ label: 'Option A' }], + }], + }; + + const result = await handler(input); + + expect(result).toEqual({ 'Which option?': 'Custom answer' }); + }); + + it('should throw AskUserQuestionDeniedError when cancelled', async () => { + mockedSelectOption.mockResolvedValue(null); + + const handler = createTtyAskUserQuestionHandler(); + const input: AskUserQuestionInput = { + questions: [{ + question: 'Which option?', + options: [{ label: 'Option A' }], + }], + }; + + await expect(handler(input)).rejects.toThrow(AskUserQuestionDeniedError); + }); + }); + + describe('multi-select questions', () => { + it('should return comma-separated selected labels', async () => { + vi.spyOn(console, 'log').mockImplementation(() => {}); + mockedPromptInput.mockResolvedValueOnce('1,3'); + + const handler = createTtyAskUserQuestionHandler(); + const input: AskUserQuestionInput = { + questions: [{ + question: 'Which features?', + multiSelect: true, + options: [ + { label: 'Feature A' }, + { label: 'Feature B' }, + { label: 'Feature C' }, + ], + }], + }; + + const result = await handler(input); + + expect(result).toEqual({ 'Which features?': 'Feature A, Feature C' }); + }); + + it('should handle Other selection with additional text input', async () => { + vi.spyOn(console, 'log').mockImplementation(() => {}); + mockedPromptInput + .mockResolvedValueOnce('1,4') + .mockResolvedValueOnce('My custom feature'); + + const handler = createTtyAskUserQuestionHandler(); + const input: AskUserQuestionInput = { + questions: [{ + question: 'Which features?', + multiSelect: true, + options: [ + { label: 'Feature A' }, + { label: 'Feature B' }, + { label: 'Feature C' }, + ], + }], + }; + + const result = await handler(input); + + expect(result).toEqual({ 'Which features?': 'Feature A, My custom feature' }); + }); + + it('should throw AskUserQuestionDeniedError when cancelled', async () => { + vi.spyOn(console, 'log').mockImplementation(() => {}); + mockedPromptInput.mockResolvedValue(null); + + const handler = createTtyAskUserQuestionHandler(); + const input: AskUserQuestionInput = { + questions: [{ + question: 'Which features?', + multiSelect: true, + options: [{ label: 'Feature A' }], + }], + }; + + await expect(handler(input)).rejects.toThrow(AskUserQuestionDeniedError); + }); + }); + + describe('free-text questions', () => { + it('should return the entered text', async () => { + mockedPromptInput.mockResolvedValue('My answer'); + + const handler = createTtyAskUserQuestionHandler(); + const input: AskUserQuestionInput = { + questions: [{ question: 'What is your name?' }], + }; + + const result = await handler(input); + + expect(result).toEqual({ 'What is your name?': 'My answer' }); + }); + + it('should throw AskUserQuestionDeniedError when cancelled', async () => { + mockedPromptInput.mockResolvedValue(null); + + const handler = createTtyAskUserQuestionHandler(); + const input: AskUserQuestionInput = { + questions: [{ question: 'What is your name?' }], + }; + + await expect(handler(input)).rejects.toThrow(AskUserQuestionDeniedError); + }); + }); + + describe('multiple questions', () => { + it('should process all questions and return aggregated answers', async () => { + mockedSelectOption.mockResolvedValue('Option B'); + mockedPromptInput.mockResolvedValue('Free text answer'); + + const handler = createTtyAskUserQuestionHandler(); + const input: AskUserQuestionInput = { + questions: [ + { + question: 'Pick one', + options: [{ label: 'Option A' }, { label: 'Option B' }], + }, + { + question: 'Enter text', + }, + ], + }; + + const result = await handler(input); + + expect(result).toEqual({ + 'Pick one': 'Option B', + 'Enter text': 'Free text answer', + }); + }); + }); + + describe('header display', () => { + it('should prefix question with header when present', async () => { + mockedPromptInput.mockResolvedValue('answer'); + + const handler = createTtyAskUserQuestionHandler(); + const input: AskUserQuestionInput = { + questions: [{ question: 'What?', header: 'Auth' }], + }; + + await handler(input); + + expect(mockedPromptInput).toHaveBeenCalledWith('[Auth] What?'); + }); + + it('should not prefix when header is absent', async () => { + mockedPromptInput.mockResolvedValue('answer'); + + const handler = createTtyAskUserQuestionHandler(); + const input: AskUserQuestionInput = { + questions: [{ question: 'What?' }], + }; + + await handler(input); + + expect(mockedPromptInput).toHaveBeenCalledWith('What?'); + }); + }); +}); + +describe('SdkOptionsBuilder.createAskUserQuestionHooks — AskUserQuestionDeniedError handling', () => { + it('should return decision: block when handler throws AskUserQuestionDeniedError', async () => { + const denyHandler = (): never => { + throw new AskUserQuestionDeniedError(); + }; + + const hooks = SdkOptionsBuilder.createAskUserQuestionHooks(denyHandler); + const preToolUseHooks = hooks['PreToolUse']; + expect(preToolUseHooks).toBeDefined(); + expect(preToolUseHooks).toHaveLength(1); + + const hookFn = preToolUseHooks![0]!.hooks[0]!; + const input = { + tool_name: 'AskUserQuestion', + tool_input: { questions: [{ question: 'Test?' }] }, + }; + const abortController = new AbortController(); + + const result = await hookFn(input, undefined, { signal: abortController.signal }); + + expect(result).toEqual({ + continue: true, + decision: 'block', + reason: expect.stringContaining('not available in non-interactive mode'), + }); + }); + + it('should block on unexpected handler errors (fail-safe)', async () => { + const failingHandler = (): never => { + throw new Error('unexpected failure'); + }; + + const hooks = SdkOptionsBuilder.createAskUserQuestionHooks(failingHandler); + const hookFn = hooks['PreToolUse']![0]!.hooks[0]!; + const input = { + tool_name: 'AskUserQuestion', + tool_input: { questions: [{ question: 'Test?' }] }, + }; + const abortController = new AbortController(); + + const result = await hookFn(input, undefined, { signal: abortController.signal }); + + expect(result).toEqual({ + continue: true, + decision: 'block', + reason: 'Internal error in AskUserQuestion handler', + }); + }); + + it('should pass through successful handler results', async () => { + const successHandler = vi.fn().mockResolvedValue({ 'Which option?': 'Option A' }); + + const hooks = SdkOptionsBuilder.createAskUserQuestionHooks(successHandler); + const hookFn = hooks['PreToolUse']![0]!.hooks[0]!; + const input = { + tool_name: 'AskUserQuestion', + tool_input: { questions: [{ question: 'Which option?' }] }, + }; + const abortController = new AbortController(); + + const result = await hookFn(input, undefined, { signal: abortController.signal }); + + expect(result).toEqual({ + continue: true, + hookSpecificOutput: { + hookEventName: 'PreToolUse', + additionalContext: JSON.stringify({ 'Which option?': 'Option A' }), + }, + }); + }); +}); + +describe('buildSdkOptions — AskUserQuestion hooks registration', () => { + const originalIsTTY = process.stdin.isTTY; + const originalNoTty = process.env.TAKT_NO_TTY; + const originalTouchTty = process.env.TAKT_TEST_FLG_TOUCH_TTY; + + afterEach(() => { + Object.defineProperty(process.stdin, 'isTTY', { value: originalIsTTY, writable: true }); + if (originalNoTty === undefined) { + delete process.env.TAKT_NO_TTY; + } else { + process.env.TAKT_NO_TTY = originalNoTty; + } + if (originalTouchTty === undefined) { + delete process.env.TAKT_TEST_FLG_TOUCH_TTY; + } else { + process.env.TAKT_TEST_FLG_TOUCH_TTY = originalTouchTty; + } + }); + + it('should auto-register PreToolUse hooks in non-TTY when no handler is provided', () => { + Object.defineProperty(process.stdin, 'isTTY', { value: false, writable: true }); + delete process.env.TAKT_NO_TTY; + delete process.env.TAKT_TEST_FLG_TOUCH_TTY; + + const options: ClaudeSpawnOptions = { cwd: '/tmp/test' }; + const sdkOptions = buildSdkOptions(options); + + expect(sdkOptions.hooks).toBeDefined(); + expect(sdkOptions.hooks!['PreToolUse']).toBeDefined(); + }); + + it('should register hooks in TTY when no handler is provided', () => { + Object.defineProperty(process.stdin, 'isTTY', { value: true, writable: true }); + delete process.env.TAKT_NO_TTY; + delete process.env.TAKT_TEST_FLG_TOUCH_TTY; + + const options: ClaudeSpawnOptions = { cwd: '/tmp/test' }; + const sdkOptions = buildSdkOptions(options); + + expect(sdkOptions.hooks).toBeDefined(); + expect(sdkOptions.hooks!['PreToolUse']).toBeDefined(); + }); + + it('non-TTY auto-deny hook should return decision: block for AskUserQuestion', async () => { + Object.defineProperty(process.stdin, 'isTTY', { value: false, writable: true }); + delete process.env.TAKT_NO_TTY; + delete process.env.TAKT_TEST_FLG_TOUCH_TTY; + + const options: ClaudeSpawnOptions = { cwd: '/tmp/test' }; + const sdkOptions = buildSdkOptions(options); + + const hookFn = sdkOptions.hooks!['PreToolUse']![0]!.hooks[0]!; + const input = { + tool_name: 'AskUserQuestion', + tool_input: { questions: [{ question: 'Choose?' }] }, + }; + const abortController = new AbortController(); + + const result = await hookFn(input, undefined, { signal: abortController.signal }); + + expect(result).toEqual({ + continue: true, + decision: 'block', + reason: expect.stringContaining('not available in non-interactive mode'), + }); + }); + + it('should use explicit handler when provided, even in non-TTY', () => { + Object.defineProperty(process.stdin, 'isTTY', { value: false, writable: true }); + delete process.env.TAKT_NO_TTY; + delete process.env.TAKT_TEST_FLG_TOUCH_TTY; + + const customHandler = vi.fn().mockResolvedValue({}); + const options: ClaudeSpawnOptions = { + cwd: '/tmp/test', + onAskUserQuestion: customHandler, + }; + const sdkOptions = buildSdkOptions(options); + + // Hooks should be registered (using the custom handler) + expect(sdkOptions.hooks).toBeDefined(); + expect(sdkOptions.hooks!['PreToolUse']).toBeDefined(); + }); +}); diff --git a/src/__tests__/it-notification-sound.test.ts b/src/__tests__/it-notification-sound.test.ts index 85f26c8..459773e 100644 --- a/src/__tests__/it-notification-sound.test.ts +++ b/src/__tests__/it-notification-sound.test.ts @@ -100,9 +100,13 @@ const { // --- Module mocks --- -vi.mock('../core/piece/index.js', () => ({ - PieceEngine: MockPieceEngine, -})); +vi.mock('../core/piece/index.js', async () => { + const errorModule = await import('../core/piece/ask-user-question-error.js'); + return { + PieceEngine: MockPieceEngine, + createDenyAskUserQuestionHandler: errorModule.createDenyAskUserQuestionHandler, + }; +}); vi.mock('../infra/claude/query-manager.js', () => ({ interruptAllQueries: mockInterruptAllQueries, diff --git a/src/__tests__/it-sigint-interrupt.test.ts b/src/__tests__/it-sigint-interrupt.test.ts index 398b46c..114b733 100644 --- a/src/__tests__/it-sigint-interrupt.test.ts +++ b/src/__tests__/it-sigint-interrupt.test.ts @@ -70,9 +70,13 @@ const { mockInterruptAllQueries, MockPieceEngine } = vi.hoisted(() => { // --- Module mocks --- -vi.mock('../core/piece/index.js', () => ({ - PieceEngine: MockPieceEngine, -})); +vi.mock('../core/piece/index.js', async () => { + const errorModule = await import('../core/piece/ask-user-question-error.js'); + return { + PieceEngine: MockPieceEngine, + createDenyAskUserQuestionHandler: errorModule.createDenyAskUserQuestionHandler, + }; +}); vi.mock('../infra/claude/query-manager.js', async (importOriginal) => ({ ...(await importOriginal>()), diff --git a/src/__tests__/opencode-client-cleanup.test.ts b/src/__tests__/opencode-client-cleanup.test.ts index 4adb3d4..549ec15 100644 --- a/src/__tests__/opencode-client-cleanup.test.ts +++ b/src/__tests__/opencode-client-cleanup.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { AskUserQuestionDeniedError } from '../core/piece/ask-user-question-error.js'; class MockEventStream implements AsyncGenerator { private index = 0; @@ -336,6 +337,67 @@ describe('OpenCodeClient stream cleanup', () => { ); }); + it('should reject question via API when handler throws AskUserQuestionDeniedError', async () => { + const { OpenCodeClient } = await import('../infra/opencode/client.js'); + const stream = new MockEventStream([ + { + type: 'question.asked', + properties: { + id: 'q-deny', + sessionID: 'session-deny', + questions: [ + { + question: 'Pick one', + header: 'Test', + options: [{ label: 'A', description: 'desc' }], + }, + ], + }, + }, + { + type: 'session.idle', + properties: { sessionID: 'session-deny' }, + }, + ]); + + const promptAsync = vi.fn().mockResolvedValue(undefined); + const sessionCreate = vi.fn().mockResolvedValue({ data: { id: 'session-deny' } }); + const disposeInstance = vi.fn().mockResolvedValue({ data: {} }); + const questionReject = vi.fn().mockResolvedValue({ data: true }); + + const subscribe = vi.fn().mockResolvedValue({ stream }); + createOpencodeMock.mockResolvedValue({ + client: { + instance: { dispose: disposeInstance }, + session: { create: sessionCreate, promptAsync }, + event: { subscribe }, + permission: { reply: vi.fn() }, + question: { reject: questionReject, reply: vi.fn() }, + }, + server: { close: vi.fn() }, + }); + + const denyHandler = (): never => { + throw new AskUserQuestionDeniedError(); + }; + + const client = new OpenCodeClient(); + const result = await client.call('interactive', 'hello', { + cwd: '/tmp', + model: 'opencode/big-pickle', + onAskUserQuestion: denyHandler, + }); + + expect(result.status).toBe('done'); + expect(questionReject).toHaveBeenCalledWith( + { + requestID: 'q-deny', + directory: '/tmp', + }, + expect.objectContaining({ signal: expect.any(AbortSignal) }), + ); + }); + it('should pass mapped tools to promptAsync when allowedTools is set', async () => { const { OpenCodeClient } = await import('../infra/opencode/client.js'); const stream = new MockEventStream([ diff --git a/src/__tests__/pieceExecution-ask-user-question.test.ts b/src/__tests__/pieceExecution-ask-user-question.test.ts new file mode 100644 index 0000000..50eae35 --- /dev/null +++ b/src/__tests__/pieceExecution-ask-user-question.test.ts @@ -0,0 +1,200 @@ +/** + * Tests: executePiece() wires a deny handler for AskUserQuestion + * to PieceEngine during piece execution. + * + * This ensures that the agent cannot prompt the user interactively + * during automated piece runs — AskUserQuestion is always blocked. + */ + +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import type { PieceConfig } from '../core/models/index.js'; +import { AskUserQuestionDeniedError } from '../core/piece/ask-user-question-error.js'; + +const { MockPieceEngine } = vi.hoisted(() => { + // eslint-disable-next-line @typescript-eslint/no-require-imports + const { EventEmitter: EE } = require('node:events') as typeof import('node:events'); + + class MockPieceEngine extends EE { + static lastInstance: MockPieceEngine; + readonly receivedOptions: Record; + private readonly config: PieceConfig; + + constructor(config: PieceConfig, _cwd: string, _task: string, options: Record) { + super(); + this.config = config; + this.receivedOptions = options; + MockPieceEngine.lastInstance = this; + } + + abort(): void {} + + async run(): Promise<{ status: string; iteration: number }> { + const firstStep = this.config.movements[0]; + if (firstStep) { + this.emit('movement:start', firstStep, 1, firstStep.instructionTemplate); + } + this.emit('piece:complete', { status: 'completed', iteration: 1 }); + return { status: 'completed', iteration: 1 }; + } + } + + return { MockPieceEngine }; +}); + +vi.mock('../core/piece/index.js', async () => { + const errorModule = await import('../core/piece/ask-user-question-error.js'); + return { + PieceEngine: MockPieceEngine, + createDenyAskUserQuestionHandler: errorModule.createDenyAskUserQuestionHandler, + }; +}); + +vi.mock('../infra/claude/query-manager.js', () => ({ + interruptAllQueries: vi.fn(), +})); + +vi.mock('../agents/ai-judge.js', () => ({ + callAiJudge: vi.fn(), +})); + +vi.mock('../infra/config/index.js', () => ({ + loadPersonaSessions: vi.fn().mockReturnValue({}), + updatePersonaSession: vi.fn(), + loadWorktreeSessions: vi.fn().mockReturnValue({}), + updateWorktreeSession: vi.fn(), + resolvePieceConfigValues: vi.fn().mockReturnValue({ + notificationSound: true, + notificationSoundEvents: {}, + provider: 'claude', + runtime: undefined, + preventSleep: false, + model: undefined, + observability: undefined, + }), + saveSessionState: vi.fn(), + ensureDir: vi.fn(), + writeFileAtomic: vi.fn(), +})); + +vi.mock('../shared/context.js', () => ({ + isQuietMode: vi.fn().mockReturnValue(true), +})); + +vi.mock('../shared/ui/index.js', () => ({ + header: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + success: vi.fn(), + status: vi.fn(), + blankLine: vi.fn(), + StreamDisplay: vi.fn().mockImplementation(() => ({ + createHandler: vi.fn().mockReturnValue(vi.fn()), + flush: vi.fn(), + })), +})); + +vi.mock('../infra/fs/index.js', () => ({ + generateSessionId: vi.fn().mockReturnValue('test-session-id'), + createSessionLog: vi.fn().mockReturnValue({ + startTime: new Date().toISOString(), + iterations: 0, + }), + finalizeSessionLog: vi.fn().mockImplementation((log, status) => ({ + ...log, + status, + endTime: new Date().toISOString(), + })), + initNdjsonLog: vi.fn().mockReturnValue('/tmp/test-log.jsonl'), + appendNdjsonLine: vi.fn(), +})); + +vi.mock('../shared/utils/index.js', () => ({ + createLogger: vi.fn().mockReturnValue({ + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + }), + notifySuccess: vi.fn(), + notifyError: vi.fn(), + preventSleep: vi.fn(), + isDebugEnabled: vi.fn().mockReturnValue(false), + writePromptLog: vi.fn(), + generateReportDir: vi.fn().mockReturnValue('test-report-dir'), + isValidReportDirName: vi.fn().mockReturnValue(true), + playWarningSound: vi.fn(), +})); + +vi.mock('../shared/prompt/index.js', () => ({ + selectOption: vi.fn(), + promptInput: vi.fn(), +})); + +vi.mock('../shared/i18n/index.js', () => ({ + getLabel: vi.fn().mockImplementation((key: string) => key), +})); + +vi.mock('../shared/exitCodes.js', () => ({ + EXIT_SIGINT: 130, +})); + +import { executePiece } from '../features/tasks/execute/pieceExecution.js'; + +function makeConfig(): PieceConfig { + return { + name: 'test-piece', + maxMovements: 5, + initialMovement: 'implement', + movements: [ + { + name: 'implement', + persona: '../agents/coder.md', + personaDisplayName: 'coder', + instructionTemplate: 'Implement task', + passPreviousResponse: true, + rules: [{ condition: 'done', next: 'COMPLETE' }], + }, + ], + }; +} + +describe('executePiece AskUserQuestion deny handler wiring', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('should pass onAskUserQuestion handler to PieceEngine', async () => { + // Given: normal piece execution + await executePiece(makeConfig(), 'task', '/tmp/project', { + projectCwd: '/tmp/project', + }); + + // Then: PieceEngine receives an onAskUserQuestion handler + const handler = MockPieceEngine.lastInstance.receivedOptions.onAskUserQuestion; + expect(typeof handler).toBe('function'); + }); + + it('should provide a handler that throws AskUserQuestionDeniedError', async () => { + // Given: piece execution completed + await executePiece(makeConfig(), 'task', '/tmp/project', { + projectCwd: '/tmp/project', + }); + + // When: the handler is invoked (as PieceEngine would when agent calls AskUserQuestion) + const handler = MockPieceEngine.lastInstance.receivedOptions.onAskUserQuestion as () => never; + + // Then: it throws AskUserQuestionDeniedError + expect(() => handler()).toThrow(AskUserQuestionDeniedError); + }); + + it('should complete successfully despite deny handler being present', async () => { + // Given/When: normal piece execution with deny handler wired + const result = await executePiece(makeConfig(), 'task', '/tmp/project', { + projectCwd: '/tmp/project', + }); + + // Then: piece completes successfully + expect(result.success).toBe(true); + }); +}); diff --git a/src/__tests__/pieceExecution-debug-prompts.test.ts b/src/__tests__/pieceExecution-debug-prompts.test.ts index 93b9119..a09c940 100644 --- a/src/__tests__/pieceExecution-debug-prompts.test.ts +++ b/src/__tests__/pieceExecution-debug-prompts.test.ts @@ -73,9 +73,13 @@ const { mockIsDebugEnabled, mockWritePromptLog, MockPieceEngine } = vi.hoisted(( return { mockIsDebugEnabled, mockWritePromptLog, MockPieceEngine }; }); -vi.mock('../core/piece/index.js', () => ({ - PieceEngine: MockPieceEngine, -})); +vi.mock('../core/piece/index.js', async () => { + const errorModule = await import('../core/piece/ask-user-question-error.js'); + return { + PieceEngine: MockPieceEngine, + createDenyAskUserQuestionHandler: errorModule.createDenyAskUserQuestionHandler, + }; +}); vi.mock('../infra/claude/query-manager.js', () => ({ interruptAllQueries: vi.fn(), diff --git a/src/__tests__/pieceExecution-session-loading.test.ts b/src/__tests__/pieceExecution-session-loading.test.ts index 4ce1699..be611ab 100644 --- a/src/__tests__/pieceExecution-session-loading.test.ts +++ b/src/__tests__/pieceExecution-session-loading.test.ts @@ -42,9 +42,13 @@ const { MockPieceEngine, mockLoadPersonaSessions, mockLoadWorktreeSessions } = v return { MockPieceEngine, mockLoadPersonaSessions, mockLoadWorktreeSessions }; }); -vi.mock('../core/piece/index.js', () => ({ - PieceEngine: MockPieceEngine, -})); +vi.mock('../core/piece/index.js', async () => { + const errorModule = await import('../core/piece/ask-user-question-error.js'); + return { + PieceEngine: MockPieceEngine, + createDenyAskUserQuestionHandler: errorModule.createDenyAskUserQuestionHandler, + }; +}); vi.mock('../infra/claude/query-manager.js', () => ({ interruptAllQueries: vi.fn(), diff --git a/src/core/piece/ask-user-question-error.ts b/src/core/piece/ask-user-question-error.ts new file mode 100644 index 0000000..7f50dc4 --- /dev/null +++ b/src/core/piece/ask-user-question-error.ts @@ -0,0 +1,37 @@ +/** + * Error and deny handler for AskUserQuestion blocking. + * + * Lives in core/piece/ because it is used by multiple provider adapters + * (claude, opencode) — keeping it provider-neutral avoids cross-infra + * runtime dependencies. + */ + +import type { AskUserQuestionHandler } from './types.js'; + +const DENY_MESSAGE = + 'AskUserQuestion is not available in non-interactive mode. Present your questions directly as text output and wait for the user to respond.'; + +/** + * Thrown by the deny handler to signal that AskUserQuestion should be + * blocked rather than retried. Caught by SdkOptionsBuilder to return + * `decision: 'block'`. + */ +export class AskUserQuestionDeniedError extends Error { + constructor() { + super(DENY_MESSAGE); + this.name = 'AskUserQuestionDeniedError'; + } +} + +/** + * Create a handler that always denies AskUserQuestion. + * + * Used during piece execution to prevent user interaction — + * the thrown error is caught by SdkOptionsBuilder and converted + * to `decision: 'block'`, prompting the AI to proceed on its own. + */ +export function createDenyAskUserQuestionHandler(): AskUserQuestionHandler { + return (): never => { + throw new AskUserQuestionDeniedError(); + }; +} diff --git a/src/core/piece/index.ts b/src/core/piece/index.ts index 3684ea4..d93e7d8 100644 --- a/src/core/piece/index.ts +++ b/src/core/piece/index.ts @@ -11,6 +11,9 @@ export { PieceEngine } from './engine/index.js'; // Constants export { COMPLETE_MOVEMENT, ABORT_MOVEMENT, ERROR_MESSAGES } from './constants.js'; +// Errors +export { AskUserQuestionDeniedError, createDenyAskUserQuestionHandler } from './ask-user-question-error.js'; + // Types export type { PieceEvents, diff --git a/src/features/tasks/execute/pieceExecution.ts b/src/features/tasks/execute/pieceExecution.ts index 6ae36d5..59331e9 100644 --- a/src/features/tasks/execute/pieceExecution.ts +++ b/src/features/tasks/execute/pieceExecution.ts @@ -4,7 +4,7 @@ import { readFileSync } from 'node:fs'; import { join } from 'node:path'; -import { PieceEngine, type IterationLimitRequest, type UserInputRequest } from '../../../core/piece/index.js'; +import { PieceEngine, createDenyAskUserQuestionHandler, type IterationLimitRequest, type UserInputRequest } from '../../../core/piece/index.js'; import type { PieceConfig } from '../../../core/models/index.js'; import type { PieceExecutionResult, PieceExecutionOptions } from './types.js'; import { detectRuleIndex } from '../../../shared/utils/ruleIndex.js'; @@ -468,6 +468,7 @@ export async function executePiece( initialSessions: savedSessions, onSessionUpdate: sessionUpdateHandler, onIterationLimit: iterationLimitHandler, + onAskUserQuestion: createDenyAskUserQuestionHandler(), projectCwd, language: options.language, provider: options.provider, diff --git a/src/infra/claude/ask-user-question-handler.ts b/src/infra/claude/ask-user-question-handler.ts new file mode 100644 index 0000000..864e353 --- /dev/null +++ b/src/infra/claude/ask-user-question-handler.ts @@ -0,0 +1,28 @@ +/** + * AskUserQuestion handler factory. + * + * Returns the appropriate handler based on TTY availability: + * - TTY → interactive terminal UI (select / text input) + * - No TTY → immediately denies so the AI falls back to plain text + */ + +import { resolveTtyPolicy } from '../../shared/prompt/tty.js'; +import type { AskUserQuestionHandler } from './types.js'; +import { createTtyAskUserQuestionHandler } from './ask-user-question-tty.js'; +import { createDenyAskUserQuestionHandler } from '../../core/piece/ask-user-question-error.js'; + +export { AskUserQuestionDeniedError } from '../../core/piece/ask-user-question-error.js'; + +/** + * Create an AskUserQuestion handler based on TTY availability. + * + * - TTY available → returns interactive terminal UI handler + * - No TTY → returns a deny handler (throws `AskUserQuestionDeniedError`) + */ +export function createAskUserQuestionHandler(): AskUserQuestionHandler { + const { useTty } = resolveTtyPolicy(); + if (useTty) { + return createTtyAskUserQuestionHandler(); + } + return createDenyAskUserQuestionHandler(); +} diff --git a/src/infra/claude/ask-user-question-tty.ts b/src/infra/claude/ask-user-question-tty.ts new file mode 100644 index 0000000..36933d3 --- /dev/null +++ b/src/infra/claude/ask-user-question-tty.ts @@ -0,0 +1,172 @@ +/** + * TTY interactive handler for AskUserQuestion. + * + * Displays selection UI in the terminal when Claude invokes the + * AskUserQuestion tool in interactive (TTY) mode. + */ + +import chalk from 'chalk'; +import { selectOption, type SelectOptionItem } from '../../shared/prompt/select.js'; +import { promptInput } from '../../shared/prompt/confirm.js'; +import type { AskUserQuestionInput, AskUserQuestionHandler } from './types.js'; +import { AskUserQuestionDeniedError } from '../../core/piece/ask-user-question-error.js'; + +const OTHER_VALUE = '__other__'; + +/** + * Build a display message from a question, prefixing with header if present. + */ +function buildDisplayMessage(question: string, header: string | undefined): string { + if (header) { + return `[${header}] ${question}`; + } + return question; +} + +/** + * Handle a single-select question using cursor-based menu navigation. + */ +async function handleSingleSelect( + displayMessage: string, + options: Array<{ label: string; description?: string }>, +): Promise { + const items: SelectOptionItem[] = options.map((opt) => ({ + label: opt.label, + value: opt.label, + description: opt.description, + })); + + items.push({ + label: 'Other', + value: OTHER_VALUE, + description: 'Enter custom text', + }); + + const selected = await selectOption(displayMessage, items); + + if (selected === null) { + throw new AskUserQuestionDeniedError(); + } + + if (selected === OTHER_VALUE) { + return handleFreeText(displayMessage); + } + + return selected; +} + +/** + * Handle a multi-select question using numbered list and text input. + */ +async function handleMultiSelect( + displayMessage: string, + options: Array<{ label: string; description?: string }>, +): Promise { + console.log(); + console.log(chalk.cyan(displayMessage)); + console.log(chalk.gray(' (Enter comma-separated numbers, e.g. 1,3)')); + console.log(); + + const otherIndex = options.length + 1; + for (let i = 0; i < options.length; i++) { + const opt = options[i]!; + const desc = opt.description ? chalk.gray(` — ${opt.description}`) : ''; + console.log(` ${chalk.yellow(`${i + 1}.`)} ${opt.label}${desc}`); + } + console.log(` ${chalk.yellow(`${otherIndex}.`)} Other ${chalk.gray('— Enter custom text')}`); + + const rawInput = await promptInput('Your selection'); + if (rawInput === null) { + throw new AskUserQuestionDeniedError(); + } + + const indices = parseNumberInput(rawInput, otherIndex); + const selectedLabels: string[] = []; + let includesOther = false; + + for (const idx of indices) { + if (idx === otherIndex) { + includesOther = true; + } else { + const opt = options[idx - 1]; + if (opt) { + selectedLabels.push(opt.label); + } + } + } + + if (includesOther) { + const customText = await promptInput('Enter custom text'); + if (customText === null) { + throw new AskUserQuestionDeniedError(); + } + selectedLabels.push(customText); + } + + if (selectedLabels.length === 0) { + throw new AskUserQuestionDeniedError(); + } + + return selectedLabels.join(', '); +} + +/** + * Parse comma-separated number input, filtering invalid values. + */ +function parseNumberInput(raw: string, maxValue: number): number[] { + const parts = raw.split(','); + const result: number[] = []; + + for (const part of parts) { + const trimmed = part.trim(); + if (!trimmed) continue; + + const num = parseInt(trimmed, 10); + if (!Number.isNaN(num) && num >= 1 && num <= maxValue) { + result.push(num); + } + } + + return result; +} + +/** + * Handle a free-text question using text input. + */ +async function handleFreeText(displayMessage: string): Promise { + const answer = await promptInput(displayMessage); + if (answer === null) { + throw new AskUserQuestionDeniedError(); + } + return answer; +} + +/** + * Create a TTY interactive handler for AskUserQuestion. + * + * Processes each question sequentially, dispatching to the appropriate + * UI handler based on question type (single-select, multi-select, free-text). + */ +export function createTtyAskUserQuestionHandler(): AskUserQuestionHandler { + return async (input: AskUserQuestionInput): Promise> => { + const answers: Record = {}; + + for (const question of input.questions) { + const displayMessage = buildDisplayMessage(question.question, question.header); + const hasOptions = question.options && question.options.length > 0; + + let answer: string; + if (hasOptions && question.multiSelect) { + answer = await handleMultiSelect(displayMessage, question.options!); + } else if (hasOptions) { + answer = await handleSingleSelect(displayMessage, question.options!); + } else { + answer = await handleFreeText(displayMessage); + } + + answers[question.question] = answer; + } + + return answers; + }; +} diff --git a/src/infra/claude/options-builder.ts b/src/infra/claude/options-builder.ts index 9cf2ce6..8b0cba7 100644 --- a/src/infra/claude/options-builder.ts +++ b/src/infra/claude/options-builder.ts @@ -24,6 +24,7 @@ import type { AskUserQuestionHandler, ClaudeSpawnOptions, } from './types.js'; +import { AskUserQuestionDeniedError, createAskUserQuestionHandler } from './ask-user-question-handler.js'; const log = createLogger('claude-sdk'); @@ -46,9 +47,8 @@ export class SdkOptionsBuilder { ? SdkOptionsBuilder.createCanUseToolCallback(this.options.onPermissionRequest) : undefined; - const hooks = this.options.onAskUserQuestion - ? SdkOptionsBuilder.createAskUserQuestionHooks(this.options.onAskUserQuestion) - : undefined; + const askHandler = this.options.onAskUserQuestion ?? createAskUserQuestionHandler(); + const hooks = SdkOptionsBuilder.createAskUserQuestionHooks(askHandler); const permissionMode = this.resolvePermissionMode(); @@ -72,7 +72,7 @@ export class SdkOptionsBuilder { }; } if (canUseTool) sdkOptions.canUseTool = canUseTool; - if (hooks) sdkOptions.hooks = hooks; + sdkOptions.hooks = hooks; if (this.options.anthropicApiKey) { sdkOptions.env = { @@ -176,8 +176,11 @@ export class SdkOptionsBuilder { }, }; } catch (err) { + if (err instanceof AskUserQuestionDeniedError) { + return { continue: true, decision: 'block', reason: err.message }; + } log.error('AskUserQuestion handler failed', { error: err }); - return { continue: true }; + return { continue: true, decision: 'block', reason: 'Internal error in AskUserQuestion handler' }; } } return { continue: true }; diff --git a/src/infra/opencode/client.ts b/src/infra/opencode/client.ts index 748912a..04a6713 100644 --- a/src/infra/opencode/client.ts +++ b/src/infra/opencode/client.ts @@ -8,6 +8,7 @@ import { createOpencode } from '@opencode-ai/sdk/v2'; import { createServer } from 'node:net'; import type { AgentResponse } from '../../core/models/index.js'; +import { AskUserQuestionDeniedError } from '../../core/piece/ask-user-question-error.js'; import { createLogger, getErrorMessage, createStreamDiagnostics, parseStructuredOutput, type StreamDiagnostics } from '../../shared/utils/index.js'; import { parseProviderModel } from '../../shared/utils/providerModel.js'; import { @@ -506,16 +507,19 @@ export class OpenCodeClient { if (sseEvent.type === 'question.asked') { const questionProps = sseEvent.properties as OpenCodeQuestionAskedProperties; if (questionProps.sessionID === sessionId) { + const rejectQuestion = (): Promise => + withTimeout( + (signal) => opencodeApiClient!.question.reject({ + requestID: questionProps.id, + directory: options.cwd, + }, { signal }), + interactionTimeoutMs, + 'OpenCode question reject timed out', + ); + if (!options.onAskUserQuestion) { try { - await withTimeout( - (signal) => opencodeApiClient!.question.reject({ - requestID: questionProps.id, - directory: options.cwd, - }, { signal }), - interactionTimeoutMs, - 'OpenCode question reject timed out', - ); + await rejectQuestion(); } catch (e) { success = false; failureMessage = getErrorMessage(e); @@ -536,9 +540,19 @@ export class OpenCodeClient { 'OpenCode question reply timed out', ); } catch (e) { - success = false; - failureMessage = getErrorMessage(e); - break; + if (e instanceof AskUserQuestionDeniedError) { + try { + await rejectQuestion(); + } catch (rejectErr) { + success = false; + failureMessage = getErrorMessage(rejectErr); + break; + } + } else { + success = false; + failureMessage = getErrorMessage(e); + break; + } } } continue; diff --git a/src/shared/ui/StreamDisplay.ts b/src/shared/ui/StreamDisplay.ts index 8be5765..702cefa 100644 --- a/src/shared/ui/StreamDisplay.ts +++ b/src/shared/ui/StreamDisplay.ts @@ -99,7 +99,10 @@ export class StreamDisplay { if (this.quiet) return; this.flushText(); const inputPreview = this.formatToolInput(tool, input); - this.startToolSpinner(tool, inputPreview); + // Starting a spinner would corrupt the interactive prompt output. + if (tool !== 'AskUserQuestion') { + this.startToolSpinner(tool, inputPreview); + } this.lastToolUse = tool; this.currentToolInputPreview = inputPreview; this.toolOutputBuffer = ''; @@ -153,6 +156,9 @@ export class StreamDisplay { if (isError) { const errorContent = sanitizedContent || 'Unknown error'; console.log(chalk.red(` ✗ ${toolName}:`), chalk.red(truncate(errorContent, 70))); + } else if (toolName === 'AskUserQuestion') { + // SDK content preview includes misleading "Error:" text for successful responses. + console.log(chalk.green(` ✓ ${toolName}`)); } else if (sanitizedContent && sanitizedContent.length > 0) { const preview = sanitizedContent.split('\n')[0] || sanitizedContent; console.log(chalk.green(` ✓ ${toolName}`), chalk.gray(truncate(preview, 60)));