From 29f8ca4bdc5b15959adf4a3ad594f8651589205a Mon Sep 17 00:00:00 2001 From: nrs <38722970+nrslib@users.noreply.github.com> Date: Mon, 2 Mar 2026 23:04:24 +0900 Subject: [PATCH] takt: fix-copilot-review-findings (#434) --- src/__tests__/copilot-client.test.ts | 34 +++++++++++++++++++++++----- src/infra/copilot/client.ts | 29 +++++++++++++++--------- 2 files changed, 46 insertions(+), 17 deletions(-) diff --git a/src/__tests__/copilot-client.test.ts b/src/__tests__/copilot-client.test.ts index 7fd93f6..4750946 100644 --- a/src/__tests__/copilot-client.test.ts +++ b/src/__tests__/copilot-client.test.ts @@ -75,7 +75,6 @@ describe('callCopilot', () => { beforeEach(() => { vi.clearAllMocks(); delete process.env.COPILOT_GITHUB_TOKEN; - // Default: mkdtemp creates a temp dir, readFile returns a session transcript, rm succeeds mockMkdtemp.mockResolvedValue('/tmp/takt-copilot-XXXXXX'); mockReadFile.mockResolvedValue( '# 🤖 Copilot CLI Session\n\n> **Session ID:** `aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee`\n', @@ -99,14 +98,12 @@ describe('callCopilot', () => { expect(result.status).toBe('done'); expect(result.content).toBe('Implementation complete. All tests pass.'); - // Session ID extracted from --share file expect(result.sessionId).toBe('aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee'); expect(mockSpawn).toHaveBeenCalledTimes(1); const [command, args, options] = mockSpawn.mock.calls[0] as [string, string[], { env?: NodeJS.ProcessEnv; stdio?: unknown }]; expect(command).toBe('copilot'); - // --yolo is used for full permission; --share is included for session extraction expect(args).toContain('-p'); expect(args).toContain('--silent'); expect(args).toContain('--no-color'); @@ -214,8 +211,9 @@ describe('callCopilot', () => { }); const [, args] = mockSpawn.mock.calls[0] as [string, string[]]; - // -p is at index 0, prompt is at index 1 - expect(args[1]).toBe('You are a strict reviewer.\n\nreview this code'); + const promptIndex = args.indexOf('-p'); + expect(promptIndex).toBeGreaterThan(-1); + expect(args[promptIndex + 1]).toBe('You are a strict reviewer.\n\nreview this code'); }); it('should return structured error when copilot binary is not found', async () => { @@ -268,6 +266,31 @@ describe('callCopilot', () => { expect(result.content).toContain('copilot returned empty output'); }); + it('should emit a failed result onStream event when stdout is empty', async () => { + mockSpawnWithScenario({ + stdout: '', + code: 0, + }); + + const onStream = vi.fn(); + const result = await callCopilot('coder', 'implement feature', { + cwd: '/repo', + onStream, + }); + + expect(result.status).toBe('error'); + expect(result.content).toContain('copilot returned empty output'); + expect(onStream).toHaveBeenCalledTimes(1); + expect(onStream).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'result', + data: expect.objectContaining({ + success: false, + }), + }), + ); + }); + it('should return plain text content (no JSON parsing needed)', async () => { const output = 'Here is the implementation:\n\n```typescript\nconsole.log("hello");\n```'; mockSpawnWithScenario({ @@ -334,7 +357,6 @@ describe('callCopilot', () => { const child = createMockChildProcess(); queueMicrotask(() => { - // Simulate abort controller.abort(); child.emit('close', null, 'SIGTERM'); }); diff --git a/src/infra/copilot/client.ts b/src/infra/copilot/client.ts index 2dc35bc..46720e4 100644 --- a/src/infra/copilot/client.ts +++ b/src/infra/copilot/client.ts @@ -73,7 +73,6 @@ function buildArgs(prompt: string, options: CopilotCallOptions & { shareFilePath args.push('--resume', options.sessionId); } - // Permission mode mapping // Note: -p mode is already non-interactive. --autopilot and // --max-autopilot-continues are not used because they conflict with // permission flags in Copilot CLI v0.0.418+ and -p already implies @@ -83,7 +82,6 @@ function buildArgs(prompt: string, options: CopilotCallOptions & { shareFilePath } else if (options.permissionMode === 'edit') { args.push('--allow-all-tools', '--no-ask-user'); } - // 'readonly' / undefined: no permission flags (copilot runs without tool access) // --share exports session transcript to a markdown file, which we parse // to extract the session ID for later resumption. @@ -176,8 +174,10 @@ function execCopilot(args: string[], options: CopilotCallOptions): Promise { - const text = typeof chunk === 'string' ? chunk : chunk.toString('utf-8'); + const toText = (chunk: Buffer | string): string => + typeof chunk === 'string' ? chunk : chunk.toString('utf-8'); + + const appendChunk = (target: 'stdout' | 'stderr', text: string): void => { const byteLength = Buffer.byteLength(text); if (target === 'stdout') { @@ -209,15 +209,15 @@ function execCopilot(args: string[], options: CopilotCallOptions): Promise { - appendChunk('stdout', chunk); + const text = toText(chunk); + appendChunk('stdout', text); if (options.onStream) { - const text = typeof chunk === 'string' ? chunk : chunk.toString('utf-8'); if (text) { options.onStream({ type: 'text', data: { text } }); } } }); - child.stderr?.on('data', (chunk: Buffer | string) => appendChunk('stderr', chunk)); + child.stderr?.on('data', (chunk: Buffer | string) => appendChunk('stderr', toText(chunk))); child.on('error', (error: NodeJS.ErrnoException) => { rejectOnce(createExecError(error.message, { @@ -353,9 +353,6 @@ export function extractSessionIdFromShareFile(content: string): string | undefin return match?.[1]; } -/** - * Read and extract session ID from a --share transcript file, then clean up. - */ function cleanupTmpDir(dir?: string): void { if (dir) { rm(dir, { recursive: true, force: true }).catch((err) => { @@ -396,7 +393,6 @@ function parseCopilotOutput(stdout: string): { content: string } | { error: stri */ export class CopilotClient { async call(agentType: string, prompt: string, options: CopilotCallOptions): Promise { - // Create a temp directory for --share session transcript let shareTmpDir: string | undefined; let shareFilePath: string | undefined; try { @@ -412,6 +408,17 @@ export class CopilotClient { const { stdout } = await execCopilot(args, options); const parsed = parseCopilotOutput(stdout); if ('error' in parsed) { + if (options.onStream) { + options.onStream({ + type: 'result', + data: { + result: '', + success: false, + error: parsed.error, + sessionId: options.sessionId ?? '', + }, + }); + } cleanupTmpDir(shareTmpDir); return { persona: agentType,