takt: fix-copilot-review-findings (#434)

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
+    const promptIndex = args.indexOf('-p');
-    expect(args[1]).toBe('You are a strict reviewer.\n\nreview this code');
+    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');
       });

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<Copil
       reject(error);
     };
 
-    const appendChunk = (target: 'stdout' | 'stderr', chunk: Buffer | string): void => {
+    const toText = (chunk: Buffer | string): string =>
-      const text = typeof chunk === 'string' ? chunk : chunk.toString('utf-8');
+      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<Copil
     };
 
     child.stdout?.on('data', (chunk: Buffer | string) => {
-      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<AgentResponse> {
-    // 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,