diff --git a/libs/platform/src/subprocess.ts b/libs/platform/src/subprocess.ts index 62ef0b21d..7456e017a 100644 --- a/libs/platform/src/subprocess.ts +++ b/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; diff --git a/libs/platform/tests/subprocess.test.ts b/libs/platform/tests/subprocess.test.ts index c302df11b..21afc7734 100644 --- a/libs/platform/tests/subprocess.test.ts +++ b/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; + }; + 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"}'],