fix: tolerate non-JSON output in JSONL subprocess parser

libs/platform/src/subprocess.ts

@@ -1,10 +1,56 @@
 /**
  * Subprocess management utilities for CLI providers
+ *
+ * JSONL parsing is tolerant of non-JSON output (e.g., from bun/npm install).
+ * Lines that don't start with { or [ are logged but not treated as errors.
  */
 
 import { spawn, type ChildProcess } from 'child_process';
 import { StringDecoder } from 'string_decoder';
 
+/**
+ * Checks if a line looks like a JSON candidate (starts with { or [)
+ * This is used to tolerate non-JSON output from tools like bun/npm install
+ */
+function isJsonCandidate(line: string): boolean {
+  if (!line) return false;
+  const firstChar = line[0];
+  return firstChar === '{' || firstChar === '[';
+}
+
+/**
+ * Sanitizes a string for safe logging by:
+ * 1. Stripping ANSI escape sequences
+ * 2. Removing control characters
+ * 3. Truncating to max length with marker
+ */
+function sanitizeForLog(str: string, maxLength: number = 200): string {
+  // Strip ANSI escape sequences (colors, cursor movement, etc.)
+  // Matches ESC[ followed by parameters and command letter
+  let sanitized = str.replace(/\x1b\[[0-9;]*[a-zA-Z]/g, '');
+
+  // Remove other control characters (except newline/tab which are already stripped)
+  // Keep printable ASCII (32-126) and common Unicode
+  sanitized = sanitized.replace(/[\x00-\x1f\x7f]/g, '');
+
+  // Truncate if too long
+  if (sanitized.length > maxLength) {
+    return sanitized.substring(0, maxLength) + '…[truncated]';
+  }
+
+  return sanitized;
+}
+
+/**
+ * Logs a non-JSON line that was skipped during parsing
+ * Sanitizes output to prevent secret leaks and log forging
+ */
+function logNonJsonOutput(trimmed: string, isFinal: boolean = false): void {
+  const suffix = isFinal ? ' (final)' : '';
+  const safePreview = sanitizeForLog(trimmed);
+  console.log(`[SubprocessManager] Non-JSON output${suffix}: ${safePreview}`);
+}
+
 export interface SubprocessOptions {
   command: string;
   args: string[];
@@ -191,6 +237,14 @@ export async function* spawnJSONLProcess(options: SubprocessOptions): AsyncGener
         const trimmed = line.trim();
         if (!trimmed) continue;
 
+        // Only try to parse lines that look like JSON (start with { or [)
+        // This tolerates non-JSON output from tools like bun/npm install
+        if (!isJsonCandidate(trimmed)) {
+          // Non-JSON output (e.g., "bun install v1.3.10") - log but don't error
+          logNonJsonOutput(trimmed);
+          continue;
+        }
+
         try {
           eventQueue.push(JSON.parse(trimmed));
         } catch (parseError) {
@@ -215,19 +269,25 @@ export async function* spawnJSONLProcess(options: SubprocessOptions): AsyncGener
 
       // Process any remaining partial line
       if (lineBuffer.trim()) {
-        try {
-          eventQueue.push(JSON.parse(lineBuffer.trim()));
-        } catch (parseError) {
-          console.error(
-            `[SubprocessManager] Failed to parse final JSONL line: ${lineBuffer}`,
-            parseError
-          );
-          eventQueue.push({
-            type: 'error',
-            error: `Failed to parse output: ${lineBuffer}`,
-          });
+        const trimmed = lineBuffer.trim();
+
+        // Only try to parse lines that look like JSON
+        if (isJsonCandidate(trimmed)) {
+          try {
+            eventQueue.push(JSON.parse(trimmed));
+          } catch (parseError) {
+            console.error(
+              `[SubprocessManager] Failed to parse final JSONL line: ${trimmed}`,
+              parseError
+            );
+            eventQueue.push({
+              type: 'error',
+              error: `Failed to parse output: ${trimmed}`,
+            });
+          }
+        } else {
+          logNonJsonOutput(trimmed, true);
         }
-        lineBuffer = '';
       }
 
       streamEnded = true;

libs/platform/tests/subprocess.test.ts

@@ -147,6 +147,123 @@ describe('subprocess.ts', () => {
       expect(results[1]).toEqual({ type: 'second' });
     });
 
+    it('should skip non-JSON output lines (e.g., from bun/npm install)', async () => {
+      const mockProcess = createMockProcess({
+        stdoutLines: [
+          'bun install v1.3.10 (30e609e0)',
+          '{"type":"progress","value":50}',
+          'Checked 3 installs across 4 packages (no changes) [4.00ms]',
+          '{"type":"complete"}',
+        ],
+        exitCode: 0,
+      });
+
+      vi.mocked(cp.spawn).mockReturnValue(mockProcess);
+
+      const generator = spawnJSONLProcess(baseOptions);
+      const results = await collectAsyncGenerator(generator);
+
+      // Non-JSON lines should be skipped (logged but not yielded as errors)
+      expect(results).toHaveLength(2);
+      expect(results[0]).toEqual({ type: 'progress', value: 50 });
+      expect(results[1]).toEqual({ type: 'complete' });
+
+      // Non-JSON lines should be logged
+      expect(consoleSpy.log).toHaveBeenCalledWith(
+        expect.stringContaining('[SubprocessManager] Non-JSON output: bun install')
+      );
+    });
+
+    it('should log non-JSON output (final) for partial lines at end of stream', async () => {
+      // Create a custom mock process that ends with a partial non-JSON line (no trailing \n)
+      const mockProcess = new EventEmitter() as cp.ChildProcess & {
+        stdout: Readable;
+        stderr: Readable;
+        kill: ReturnType<typeof vi.fn>;
+      };
+      const stdout = new Readable({ read() {} });
+      const stderr = new Readable({ read() {} });
+
+      mockProcess.stdout = stdout;
+      mockProcess.stderr = stderr;
+      mockProcess.kill = vi.fn().mockReturnValue(true);
+
+      vi.mocked(cp.spawn).mockReturnValue(mockProcess);
+
+      process.nextTick(async () => {
+        // Push a valid JSON line (with \n)
+        stdout.push('{"type":"valid"}\n');
+        await new Promise((resolve) => setImmediate(resolve));
+
+        // Push a partial non-JSON line WITHOUT \n - this stays in the buffer
+        stdout.push('npm WARN something');
+        await new Promise((resolve) => setImmediate(resolve));
+
+        // End stdout - triggers the 'end' handler which flushes the buffer
+        stdout.push(null);
+
+        await new Promise((resolve) => setTimeout(resolve, 10));
+        mockProcess.emit('exit', 0);
+      });
+
+      const generator = spawnJSONLProcess(baseOptions);
+      const results = await collectAsyncGenerator(generator);
+
+      // Only the valid JSON should be yielded
+      expect(results).toHaveLength(1);
+      expect(results[0]).toEqual({ type: 'valid' });
+
+      // The final partial non-JSON line should be logged with "(final)" suffix
+      expect(consoleSpy.log).toHaveBeenCalledWith(
+        expect.stringContaining('[SubprocessManager] Non-JSON output (final): npm WARN something')
+      );
+    });
+
+    it('should sanitize non-JSON output before logging (strip ANSI and control chars)', async () => {
+      // ANSI escape sequence for red text: \x1b[31m
+      const ansiColoredLine = '\x1b[31m\x1b[1mbun install\x1b[0m done';
+      const mockProcess = createMockProcess({
+        stdoutLines: [ansiColoredLine, '{"type":"complete"}'],
+        exitCode: 0,
+      });
+
+      vi.mocked(cp.spawn).mockReturnValue(mockProcess);
+
+      const generator = spawnJSONLProcess(baseOptions);
+      await collectAsyncGenerator(generator);
+
+      // Should log sanitized version without ANSI codes
+      expect(consoleSpy.log).toHaveBeenCalledWith(
+        expect.stringContaining('[SubprocessManager] Non-JSON output: bun install done')
+      );
+      // Should NOT contain raw ANSI escape sequences
+      expect(consoleSpy.log).not.toHaveBeenCalledWith(expect.stringContaining('\x1b['));
+    });
+
+    it('should truncate very long non-JSON output before logging', async () => {
+      // Create a very long line (>200 chars)
+      const longLine = 'a'.repeat(300);
+      const mockProcess = createMockProcess({
+        stdoutLines: [longLine, '{"type":"complete"}'],
+        exitCode: 0,
+      });
+
+      vi.mocked(cp.spawn).mockReturnValue(mockProcess);
+
+      const generator = spawnJSONLProcess(baseOptions);
+      await collectAsyncGenerator(generator);
+
+      // Should log truncated version with marker
+      expect(consoleSpy.log).toHaveBeenCalledWith(expect.stringContaining('…[truncated]'));
+      // The logged message should be less than original length
+      const loggedCall = consoleSpy.log.mock.calls.find((call) =>
+        call[0].includes('[SubprocessManager] Non-JSON output')
+      );
+      expect(loggedCall).toBeDefined();
+      // The 'a' characters portion should be ~200 chars, not 300
+      expect(loggedCall![0].length).toBeLessThan(350);
+    });
+
     it('should yield error for malformed JSON and continue processing', async () => {
       const mockProcess = createMockProcess({
         stdoutLines: ['{"type":"valid"}', '{invalid json}', '{"type":"also_valid"}'],