From c864d6bbbdbbde3bf0ddc4e6cd22fc94ccc1ef21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9E=97=20=E9=A7=BF=E7=94=AB=20=28Shunsuke=20Hayashi=29?= Date: Wed, 18 Mar 2026 15:21:32 +0900 Subject: [PATCH] fix(cli): write tool output to stdout via fd 1 instead of stderr (#324) (#346) --- gitnexus/src/cli/eval-server.ts | 17 +- gitnexus/src/cli/tool.ts | 56 +++++-- gitnexus/src/mcp/local/local-backend.ts | 34 +++- gitnexus/test/integration/cli-e2e.test.ts | 174 ++++++++++++++++++++- gitnexus/test/unit/eval-formatters.test.ts | 29 +++- 5 files changed, 291 insertions(+), 19 deletions(-) diff --git a/gitnexus/src/cli/eval-server.ts b/gitnexus/src/cli/eval-server.ts index 35a35406c5..35827eb91d 100644 --- a/gitnexus/src/cli/eval-server.ts +++ b/gitnexus/src/cli/eval-server.ts @@ -25,6 +25,7 @@ */ import http from 'http'; +import { writeSync } from 'node:fs'; import { LocalBackend } from '../mcp/local/local-backend.js'; export interface EvalServerOptions { @@ -142,7 +143,10 @@ export function formatContextResult(result: any): string { } export function formatImpactResult(result: any): string { - if (result.error) return `Error: ${result.error}`; + if (result.error) { + const suggestion = result.suggestion ? `\nSuggestion: ${result.suggestion}` : ''; + return `Error: ${result.error}${suggestion}`; + } const target = result.target; const direction = result.direction; @@ -155,7 +159,11 @@ export function formatImpactResult(result: any): string { const lines: string[] = []; const dirLabel = direction === 'upstream' ? 'depends on this (will break if changed)' : 'this depends on'; - lines.push(`Blast radius for ${target?.kind || ''} ${target?.name} (${direction}): ${total} symbol(s) ${dirLabel}\n`); + lines.push(`Blast radius for ${target?.kind || ''} ${target?.name} (${direction}): ${total} symbol(s) ${dirLabel}`); + if (result.partial) { + lines.push('⚠️ Partial results — graph traversal was interrupted. Deeper impacts may exist.'); + } + lines.push(''); const depthLabels: Record = { 1: 'WILL BREAK (direct)', @@ -401,9 +409,10 @@ export async function evalServerCommand(options?: EvalServerOptions): Promise { return _backend; } +/** + * Write tool output to stdout using low-level fd write. + * + * LadybugDB's native module captures Node.js process.stdout during init, + * but the underlying OS file descriptor 1 (stdout) remains intact. + * By using fs.writeSync(1, ...) we bypass the Node.js stream layer + * and write directly to the real stdout fd (#324). + * + * Falls back to stderr if the fd write fails (e.g., broken pipe). + */ function output(data: any): void { const text = typeof data === 'string' ? data : JSON.stringify(data, null, 2); - // stderr because LadybugDB captures stdout at OS level - process.stderr.write(text + '\n'); + try { + writeSync(1, text + '\n'); + } catch (err: any) { + if (err?.code === 'EPIPE') { + // Consumer closed the pipe (e.g., `gitnexus cypher ... | head -1`) + // Exit cleanly per Unix convention + process.exit(0); + } + // Fallback: stderr (previous behavior, works on all platforms) + process.stderr.write(text + '\n'); + } } export async function queryCommand(queryText: string, options?: { @@ -93,14 +114,25 @@ export async function impactCommand(target: string, options?: { } const backend = await getBackend(); - const result = await backend.callTool('impact', { - target, - direction: options?.direction || 'upstream', - maxDepth: options?.depth ? parseInt(options.depth) : undefined, - includeTests: options?.includeTests ?? false, - repo: options?.repo, - }); - output(result); + try { + const result = await backend.callTool('impact', { + target, + direction: options?.direction || 'upstream', + maxDepth: options?.depth ? parseInt(options.depth) : undefined, + includeTests: options?.includeTests ?? false, + repo: options?.repo, + }); + output(result); + } catch (err: any) { + // Return structured error JSON instead of crashing (#321) + output({ + error: err?.message || 'Impact analysis failed unexpectedly', + target, + direction: options?.direction || 'upstream', + suggestion: 'Try reducing --depth or using gitnexus context as a fallback', + }); + process.exit(1); + } } export async function cypherCommand(query: string, options?: { diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index 1828c326af..47463cd7f5 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -1329,6 +1329,30 @@ export class LocalBackend { relationTypes?: string[]; includeTests?: boolean; minConfidence?: number; + }): Promise { + try { + return await this._impactImpl(repo, params); + } catch (err: any) { + // Return structured error instead of crashing (#321) + return { + error: err?.message || 'Impact analysis failed', + target: params.target, + direction: params.direction, + impactedCount: 0, + risk: 'UNKNOWN', + partial: true, + suggestion: 'The graph query failed — try gitnexus context as a fallback', + }; + } + } + + private async _impactImpl(repo: RepoHandle, params: { + target: string; + direction: 'upstream' | 'downstream'; + maxDepth?: number; + relationTypes?: string[]; + includeTests?: boolean; + minConfidence?: number; }): Promise { await this.ensureInitialized(repo.id); @@ -1358,6 +1382,7 @@ export class LocalBackend { const impacted: any[] = []; const visited = new Set([symId]); let frontier = [symId]; + let traversalComplete = true; for (let depth = 1; depth <= maxDepth && frontier.length > 0; depth++) { const nextFrontier: string[] = []; @@ -1391,7 +1416,13 @@ export class LocalBackend { }); } } - } catch (e) { logQueryError('impact:depth-traversal', e); } + } catch (e) { + logQueryError('impact:depth-traversal', e); + // Break out of depth loop on query failure but return partial results + // collected so far, rather than silently swallowing the error (#321) + traversalComplete = false; + break; + } frontier = nextFrontier; } @@ -1474,6 +1505,7 @@ export class LocalBackend { direction, impactedCount: impacted.length, risk, + ...(!traversalComplete && { partial: true }), summary: { direct: directCount, processes_affected: processCount, diff --git a/gitnexus/test/integration/cli-e2e.test.ts b/gitnexus/test/integration/cli-e2e.test.ts index 50a01fd6e9..c896846317 100644 --- a/gitnexus/test/integration/cli-e2e.test.ts +++ b/gitnexus/test/integration/cli-e2e.test.ts @@ -9,7 +9,7 @@ * Accepts status === null (timeout) as valid on slow CI runners. */ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; -import { spawnSync } from 'child_process'; +import { spawnSync, spawn } from 'child_process'; import path from 'path'; import fs from 'fs'; import os from 'os'; @@ -231,4 +231,176 @@ describe('CLI end-to-end', () => { }); }); + + // ─── stdout fd 1 tests (#324) ─────────────────────────────────────── + // These tests verify that tool output goes to stdout (fd 1), not stderr. + // Requires analyze to have run first (the analyze test above populates .gitnexus/). + + describe('tool output goes to stdout via fd 1 (#324)', () => { + it('cypher: JSON appears on stdout, not stderr', () => { + const result = runCliRaw(['cypher', 'MATCH (n) RETURN n.name LIMIT 3'], MINI_REPO); + if (result.status === null) return; // CI timeout tolerance + + expect(result.status).toBe(0); + + // stdout must contain valid JSON (array or object) + expect(() => JSON.parse(result.stdout.trim())).not.toThrow(); + + // stderr must NOT contain JSON — only human-readable diagnostics allowed + const stderrTrimmed = result.stderr.trim(); + if (stderrTrimmed.length > 0) { + expect(() => JSON.parse(stderrTrimmed)).toThrow(); + } + }); + + it('query: JSON appears on stdout, not stderr', () => { + // "handler" is a generic term likely to match something in mini-repo + const result = runCliRaw(['query', 'handler'], MINI_REPO); + if (result.status === null) return; + + expect(result.status).toBe(0); + expect(() => JSON.parse(result.stdout.trim())).not.toThrow(); + }); + + it('impact: JSON appears on stdout, not stderr', () => { + const result = runCliRaw( + ['impact', 'handleRequest', '--direction', 'upstream'], + MINI_REPO, + ); + if (result.status === null) return; + + expect(result.status).toBe(0); + // impact may return an error object (symbol not found) or a real result — + // either way it must be valid JSON on stdout + expect(() => JSON.parse(result.stdout.trim())).not.toThrow(); + }); + + it('stdout is pipeable: cypher output parses as valid JSON', () => { + const result = runCliRaw( + ['cypher', 'MATCH (n:Function) RETURN n.name LIMIT 5'], + MINI_REPO, + ); + if (result.status === null) return; + + expect(result.status).toBe(0); + + // Simulate what jq does: parse stdout as JSON + const parsed = JSON.parse(result.stdout.trim()); + expect(Array.isArray(parsed) || typeof parsed === 'object').toBe(true); + }); + }); + + // ─── EPIPE clean exit test (#324) ─────────────────────────────────── + + describe('EPIPE handling (#324)', () => { + it('cypher: EPIPE exits with code 0, not stderr dump', () => { + return new Promise((resolve, reject) => { + const child = spawn( + process.execPath, + ['--import', 'tsx', cliEntry, 'cypher', 'MATCH (n) RETURN n LIMIT 500'], + { + cwd: MINI_REPO, + stdio: ['ignore', 'pipe', 'pipe'], + env: { + ...process.env, + NODE_OPTIONS: `${process.env.NODE_OPTIONS || ''} --max-old-space-size=8192`.trim(), + }, + }, + ); + + let stderrOutput = ''; + child.stderr.on('data', (chunk: Buffer) => { stderrOutput += chunk.toString(); }); + + // Destroy stdout immediately — simulates `| head -0` (consumer closes early) + child.stdout.once('data', () => { + child.stdout.destroy(); // triggers EPIPE on next write + }); + + const timer = setTimeout(() => { + child.kill('SIGTERM'); + // Timeout is acceptable on CI — not a failure + resolve(); + }, 20000); + + child.on('close', (code) => { + clearTimeout(timer); + try { + // Clean EPIPE exit: code 0 + expect(code).toBe(0); + // No JSON payload should appear on stderr + const trimmed = stderrOutput.trim(); + if (trimmed.length > 0) { + expect(() => JSON.parse(trimmed)).toThrow(); + } + resolve(); + } catch (err) { + reject(err); + } + }); + }); + }, 25000); + }); + + // ─── eval-server READY signal test (#324) ─────────────────────────── + + describe('eval-server READY signal (#324)', () => { + it('READY signal appears on stdout, not stderr', () => { + return new Promise((resolve, reject) => { + const child = spawn( + process.execPath, + ['--import', 'tsx', cliEntry, 'eval-server', '--port', '0', '--idle-timeout', '3'], + { + cwd: MINI_REPO, + stdio: ['ignore', 'pipe', 'pipe'], + env: { + ...process.env, + NODE_OPTIONS: `${process.env.NODE_OPTIONS || ''} --max-old-space-size=8192`.trim(), + }, + }, + ); + + let stdoutBuffer = ''; + let foundOnStdout = false; + let foundOnStderr = false; + + child.stdout.on('data', (chunk: Buffer) => { + stdoutBuffer += chunk.toString(); + if (stdoutBuffer.includes('GITNEXUS_EVAL_SERVER_READY:')) { + foundOnStdout = true; + child.kill('SIGTERM'); + } + }); + + child.stderr.on('data', (chunk: Buffer) => { + const text = chunk.toString(); + if (text.includes('GITNEXUS_EVAL_SERVER_READY:')) { + foundOnStderr = true; + child.kill('SIGTERM'); + } + }); + + const timer = setTimeout(() => { + child.kill('SIGTERM'); + // Timeout is acceptable on CI — not a failure + resolve(); + }, 30000); + + child.on('close', () => { + clearTimeout(timer); + try { + if (foundOnStderr) { + reject(new Error('READY signal appeared on stderr instead of stdout')); + } else if (foundOnStdout) { + resolve(); + } else { + // eval-server may not start on all CI environments — don't fail + resolve(); + } + } catch (err) { + reject(err); + } + }); + }); + }, 35000); + }); }); diff --git a/gitnexus/test/unit/eval-formatters.test.ts b/gitnexus/test/unit/eval-formatters.test.ts index 81fb72d5fb..4d288fcd4b 100644 --- a/gitnexus/test/unit/eval-formatters.test.ts +++ b/gitnexus/test/unit/eval-formatters.test.ts @@ -147,7 +147,34 @@ describe('formatContextResult', () => { describe('formatImpactResult', () => { it('returns error message for error input', () => { - expect(formatImpactResult({ error: 'bad request' })).toBe('Error: bad request'); + expect(formatImpactResult({ error: 'bad request' })).toContain('Error: bad request'); + }); + + it('returns error with suggestion when provided', () => { + const result = formatImpactResult({ + error: 'Impact analysis failed', + suggestion: 'Try gitnexus context as a fallback', + }); + expect(result).toContain('Error: Impact analysis failed'); + expect(result).toContain('Suggestion: Try gitnexus context'); + }); + + it('shows partial warning when traversal was interrupted', () => { + const result = formatImpactResult({ + target: { kind: 'Function', name: 'foo' }, + direction: 'upstream', + impactedCount: 2, + partial: true, + byDepth: { + 1: [ + { type: 'Function', name: 'caller1', filePath: 'src/a.ts', relationType: 'CALLS', confidence: 1 }, + { type: 'Function', name: 'caller2', filePath: 'src/b.ts', relationType: 'CALLS', confidence: 1 }, + ], + }, + }); + expect(result).toContain('Partial results'); + expect(result).toContain('caller1'); + expect(result).toContain('caller2'); }); it('handles zero impact', () => {