feat(core): adopt pino structured logger

[magyargergo]

Summary

Adopt pino as GitNexus's project-wide structured logger. Migrate all 134 console.* sites in gitnexus/src/ (excluding cli/server which legitimately own stdout/stderr) to a thin wrapper at gitnexus/src/core/logger.ts. ESLint no-console: error is the forcing function — future contributions can't regress.

This supersedes the hand-rolled sanitizeLogValue helper that PR #1329 added (kept untouched on its branch as fallback until CodeQL confirms js/log-injection #466 closes via pino).

Why

PR #1329's sanitizeLogValue got 4 distinct code-review findings (TypeError on undefined Error.message, U+2028/U+2029 escape gap, test pinning re-derives the helper instead of calling production code, ANSI/C0 not escaped). Patching project-owned sanitizers per-call-site has bad maintenance trajectory. Pino's NDJSON output is structurally injection-resistant: one record per newline, all string fields JSON-escaped, Error objects natively serialized.

What's in this PR (3 commits)

1. Foundation (471f77e6):

• pino@^10 (runtime), pino-pretty@^13 (devDep)
• gitnexus/src/core/logger.ts — createLogger(name, opts?) factory + default logger singleton (Proxy-backed for test capture)
• gitnexus/test/unit/logger.test.ts — 11 tests
• bridge-db.ts migrated (security-critical site)
• ESLint no-console: warn for gitnexus/src/**/*.ts, cli/server exempt

2. Baseline-suppress (3e8e7c2a):

• 50 files, 134 mechanical // eslint-disable-next-line no-console -- TODO(pino-migration) markers — kept lint clean while the bulk migration was prepared

3. Full migration sweep (d18df1a4):

• All 134 console.* sites → logger.* using pino's structured-arg convention (object first, message second)
• All TODO(pino-migration) markers removed
• ESLint flipped from warn → error
• 10 test files updated: vi.spyOn(console, 'X') → _captureLogger() + cap.records() assertions
• Logger default destination → stderr (fd 2) so CLI tool output (cypher command outputs JSON to stderr instead of stdout #324) stays clean on stdout
• Logger default level → info (preserves console.log visibility)

Operator behaviour preserved

• GITNEXUS_DEBUG_BRIDGE=1 continues to enable verbose bridge-db output
• CLI query/cypher/impact JSON output stays clean on stdout
• All operator-visible startup messages (e.g. "🧠 Loading embedding model…") still appear, now formatted as NDJSON or pretty-print depending on TTY
• pino-pretty only engages when stderr is a TTY AND CI/VITEST env vars unset

Test plan

• npx vitest run test/unit/logger.test.ts — 11/11 pass
• npx vitest run (full suite) — 7762/7762 pass excluding 29 pre-existing PR feat(go): implement scope resolution hooks for Go language support #1302 Go resolver failures
• npx eslint gitnexus/src/ — 0 errors, 426 pre-existing warnings unchanged
• npx tsc --noEmit — only the pre-existing PR feat(go): implement scope resolution hooks for Go language support #1302 TS error
• git grep -n "TODO(pino-migration)" — 0 matches
• git grep -n "console\." gitnexus/src/ | grep -v cli/ | grep -v server/ | grep -v logger.ts — 2 comment-only references (no production calls)
• CLI-e2e stdout cleanliness verified: gitnexus query JSON parses without error

Follow-up issues to file

• GITNEXUS_DEBUG_* env-var consolidation into GITNEXUS_LOG_LEVEL
• gitnexus-web/ adoption if/when needed (currently console-based; out of scope)
• Per-request child loggers with correlation IDs once HTTP/MCP flows need them

Risks

• CodeQL recognition of pino as a sanitizer for js/log-injection is the load-bearing assumption for closing alert fix(swift): correct assignment query for tree-sitter-swift@0.6.0 (#386, #406) #466. If recognition gaps surface on the next scan, fallback options are (a) add a custom CodeQL model, or (b) add a small string-field sanitizer pass inside the wrapper (single location, not per-call-site). PR fix(core): close insecure-tempfile + log-injection in core/group (U6) #1329's sanitizer remains as defense-in-depth on its branch.
• pino-pretty graceful fallback when missing in production install is verified by the tryBuildPrettyTransport() try/catch path.
• Logger writes to stderr by default to preserve CLI tool output cleanliness. Existing operator workflows that grep stdout for log lines will need to switch to stderr — documented in the logger module JSDoc.

Refs: #1318, #1329, CodeQL js/log-injection #466, #324

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
gitnexus	Ready	Preview, Comment	May 7, 2026 3:02pm

[github-actions]

CI Report

❌ Some checks failed

Pipeline Status

Stage	Status	Details
✅ Typecheck	success	tsc --noEmit
❌ Tests	failure	unit tests, 3 platforms
✅ E2E	success	gitnexus-web changes only

Test Results

Tests	Passed	Failed	Skipped	Duration
8034	8033	0	1	374s

✅ All 8033 tests passed

1 test(s) skipped — expand for details

• buildTypeEnv > known limitations (documented skip tests) > Ruby block parameter: users.each { |user| } — closure param inference, different feature

Code Coverage

Tests

Metric	Coverage	Covered	Base	Delta	Status
Statements	77.63%	24520/31583	77.02%	📈 +0.6	🟢 ███████████████░░░░░
Branches	66.2%	15539/23471	65.97%	📈 +0.2	🟢 █████████████░░░░░░░
Functions	82.8%	2432/2937	81.86%	📈 +0.9	🟢 ████████████████░░░░
Lines	80.66%	22140/27446	79.89%	📈 +0.8	🟢 ████████████████░░░░

---

_{📋 View full run · Generated by CI}

[github-actions]

Claude finished @magyargergo's task in 7m 54s —— View job

---

PR #1336 Review: feat(core): adopt pino structured logger

Tasks

• Post initial comment
• Read repo context files (DoD.md, AGENTS.md, GUARDRAILS.md)
• Inspect core files (eslint.config.mjs, package.json, logger.ts, logger.test.ts)
• Inspect representative migrated call sites
• Check CLI stdout/stderr contracts (tool.ts, mcp.ts, analyze.ts, serve.ts)
• Search for console., TODO(pino-migration), hidden Unicode
• Get CI/GHAS status
• Assess migration correctness, security, ESLint, deps, tests, performance, scope
• Post final review

---

Review bar for this PR

• core/logger.ts is small, typed, and readable; default destination is stderr; debug gating works.
• pino-pretty activates only on a TTY stderr, never in CI/VITEST, and never on stdout.
• CLI commands that output machine-readable JSON (query, cypher, impact, context) keep stdout clean — no log line appears on the pipe.
• Log levels match semantic intent — errors at error, timing/startup at appropriate lower levels.
• GITNEXUS_DEBUG_BRIDGE debug-level gating is preserved; default is info.
• ESLint no-console: error is the forcing function for core; CLI/server are precisely exempted.
• Zero TODO(pino-migration) markers; zero unintended production console.* calls.
• CodeQL/GHAS confirms js/log-injection fix(swift): correct assignment query for tree-sitter-swift@0.6.0 (#386, #406) #466 resolved or fallback is documented and credible.

---

Current PR state

Attribute	Value
Head SHA	bb02b53aa20e00d7e575a06a2ccee3346d3ddd12
Status	Open, 6 commits
Changed files	80 (matches PR body)
CI (8030 tests)	✅ passing (in-progress rerun triggered by this review)
CodeQL	✅ success on latest run
Gitleaks	✅ success
Dependency Review	✅ success
Typecheck	✅ success (per previous CI comment)
TODO(pino-migration) markers	0 — confirmed by grep
Hidden bidi Unicode	None found — em-dashes in comments, intentional U+2028/U+2029 in test strings only
pino-pretty in lockfile	"dev": true ✓
Vercel preview	Irrelevant to this review

---

Understanding of the change

GitNexus migrated 134 console.* call sites across gitnexus/src/ (excluding CLI/server stdout-contract holders) to a thin pino wrapper at core/logger.ts. Pino's NDJSON output is structurally log-injection-resistant (every string field JSON-escaped, each record one line), replacing a hand-rolled sanitizer approach (PR #1329) that had recurring edge-case gaps. Logger defaults to stderr so CLI tool data (query, cypher, impact) remains clean on stdout. ESLint no-console: error enforces the migration for future contributions. Tests validate debug gating, error-object handling, and CR/LF/U+2028/ANSI injection containment.

---

Findings

---

[CRITICAL] pino-pretty writes to stdout, not stderr, breaking piped CLI output

• Category: CLI stdout/stderr contract

• Files: gitnexus/src/core/logger.ts:67-80, gitnexus/src/mcp/local/local-backend.ts:185-191, 967

• Issue: tryBuildPrettyTransport() returns a transport config without setting destination: 2. When using pino's transport option with pino-pretty, pino-pretty writes to stdout (fd 1) by default. shouldUsePretty() gates on process.stderr.isTTY === true — but when a user runs gitnexus query "auth" | jq, stderr is still a TTY while stdout is the pipe. So shouldUsePretty() is true, pino-pretty activates, and every logger.* call goes to stdout. logQueryTiming() (called at line 967 on every query invocation) uses logger.error(), which means a pino-pretty-formatted timing line appears on stdout before or after the JSON result. jq receives malformed input and fails.

  The comment at logger.ts:110-112 compounds the problem: "When using a transport (pino-pretty), pino manages the destination internally and we cannot pass one explicitly." This is factually incorrect — destination: 2 can and must be set in the transport options to route pretty output to stderr.

  Affected scenarios:

  • gitnexus query "..." | jq — timing log on stdout, breaks pipe
  • gitnexus cypher "..." | jq — same path
  • gitnexus impact "..." | jq — same path
  • gitnexus analyze on a TTY — pino-pretty logs on stdout, conflicts with progress bar

• Why it matters: This is the primary correctness claim of the PR. The PR body says "CLI-e2e stdout cleanliness verified" but the piped-stdout scenario is precisely where this fails.

• Recommended fix: Add destination: 2 to the pino-pretty transport options. One-line change:

  // in tryBuildPrettyTransport()
  options: {
    colorize: true,
    translateTime: 'SYS:HH:MM:ss.l',
    ignore: 'pid,hostname',
    destination: 2,  // stderr, matching the non-pretty path's pino.destination({dest:2})
  },

  Also correct the misleading comment at line 110-112. Fix this →

• Blocks merge: YES

---

[HIGH] logQueryTiming() and MCP startup banner use logger.error() for non-error conditions

• Category: Migration correctness / log level semantics

• Files: gitnexus/src/mcp/local/local-backend.ts:185-191, gitnexus/src/cli/mcp.ts:38-41

• Issue: Two mismatched level uses:

  1. logQueryTiming() (local-backend.ts:185-191) emits query wall-clock timing at logger.error(). Timing telemetry is not an error. This was likely migrated from console.warn() or similar and the level was set to error to keep stderr output visible, but it causes log aggregators and alerting rules to fire on every successful query. Should be logger.debug() (suppressed by default; enabled when GITNEXUS_LOG_LEVEL=debug or similar).

  2. MCP startup banner (mcp.ts:38-41) — "GitNexus: MCP server starting with N repo(s)..." uses logger.error(). A healthy startup is not an error. Should be logger.info().

  The logger.error() for "No indexed repos yet" in mcp.ts:34-36 is reasonable (it's an actionable operator problem), so that one is fine.

• Why it matters: Error-level flooding degrades observability. Operators building on the structured log stream for alerting will get false positives on every query call. The logQueryTiming() call at line 967 is on the hot path of every gitnexus query invocation — this is not a cold path.

• Recommended fix: Change logQueryTiming() to logger.debug(...) with structured fields; change the startup banner to logger.info(...). Fix this →

• Blocks merge: YES (the logQueryTiming issue at error level also compounds finding Add support for Ollama as a local inference backend #1 — it's the specific call that pollutes stdout in the pipe scenario)

---

[MEDIUM] Several migrated sites use string interpolation instead of structured pino format

• Category: Migration hygiene / observability quality
• Files: gitnexus/src/core/ingestion/workers/worker-pool.ts:262-267, 274-279, gitnexus/src/mcp/local/local-backend.ts:168, 188-190
• Issue: Pino's canonical form is logger.warn({ key: value }, 'message'). Several migrated sites embed structured data in the message string instead:

  // worker-pool.ts — all context locked in string, no structured fields
  logger.warn(
    `Worker ${workerIndex} parse job idle timeout after ${job.timeoutMs / 1000}s ` +
      `(${job.items.length} items, ${job.estimatedBytes} bytes, ...`
  );
  // local-backend.ts — unstructured
  logger.error(`GitNexus [${context}]: ${msg}`);
  logger.error(`GitNexus [query:timing] query=${JSON.stringify(truncated)} totalMs=${totalMs}...`);

  These negate much of the benefit of adopting a structured logger — fields can't be queried independently in log aggregators.
• Why it matters: Reduces the operational value of the migration. Worker timeout diagnosis and query latency monitoring require parsing message strings instead of reading structured fields.
• Recommended fix: Refactor to structured form. Example for worker-pool:

  logger.warn(
    { workerIndex, timeoutMs: job.timeoutMs, items: job.items.length, bytes: job.estimatedBytes },
    'Worker parse job idle timeout — splitting'
  );

  Not a hard blocker but significantly undercuts the migration's observability value.
• Blocks merge: No (but worth fixing before merge for the hot-path worker-pool site)

---

Logger wrapper assessment

core/logger.ts is clean and well-commented. createLogger(name, opts?) is small and typed. The Proxy-backed singleton for test capture is clever and works correctly — _captureLogger() replaces _cached and _activeDestination atomically, and restore() clears both. The test API is underscore-prefixed and clearly marked "not a public API." isTruthyEnv() correctly handles the 0/false/no/off falsiness spectrum. The GITNEXUS_DEBUG_BRIDGE gating via debugEnvVar is preserved and correctly time-gates at createLogger call time.

One substantive defect (related to Finding #1): shouldUsePretty() checks process.stderr.isTTY but pino-pretty's default destination is stdout. The check should be process.stdout.isTTY OR the transport options should include destination: 2. Currently the guard is inconsistent with the actual output path.

The tryBuildPrettyTransport() try/catch is misleading — it can't catch a missing pino-pretty module since worker thread module resolution is async. However, since shouldUsePretty() returns false in non-TTY contexts (production, CI, Docker), and pino-pretty is a devDependency absent from production installs, this is effectively dead code in production. The guard works by environment gating rather than exception catching. Not a blocker, but the comment should be corrected.

Error handling: logger.* calls in catch blocks do not throw (pino doesn't throw on logging). Undefined/non-Error thrown values are handled in tests. The MemoryWritable in _captureLogger is correctly isolated.

---

CLI stdout/stderr contract assessment

query / cypher / impact / context (tool.ts): Output goes via writeSync(1, ...) (raw fd write, bypasses Node.js stream layer). This is correct and robust. Logger calls (logger.error(...) for error messages) go to stderr — except in pino-pretty mode (see Finding #1). Error-path messages in getBackend() correctly use logger.error() and go to stderr via pino.

MCP stdio mode (mcp.ts): In MCP client context, process.stderr.isTTY is typically false (MCP client doesn't provide TTY stderr), so shouldUsePretty() returns false, pino-pretty is not activated, and logger writes NDJSON to fd 2. MCP protocol on stdout is safe.

gitnexus analyze (analyze.ts): The progress bar is on stdout. Console.log/warn/error are rerouted through barLog (stdout) during the run. Pino logs go to stderr (or stdout via pino-pretty if TTY — same issue as Finding #1). The writeFatalToStderr() pattern using captured realStderrWrite is a good defense for fatal error paths that bypass the console routing.

ESLint exemptions: CLI files correctly allow console.log (stdout product contract). The explicit // eslint-disable-next-line no-console comments in analyze.ts for capturing console.warn/console.error references are appropriate — this is deliberate progress-bar UX routing, not a logging concern.

---

Migration correctness assessment

Sampled files show consistent migration:

• bridge-db.ts: uses createLogger('bridge-db', { debugEnvVar: 'GITNEXUS_DEBUG_BRIDGE' }) — level and debug gate preserved. Log calls use structured { err, groupDir } format.
• worker-pool.ts: migrated from console.warn to logger.warn — level preserved. Unstructured string messages (see Finding readme #3).
• ignore-service.ts, config/*.ts: diagnostic warnings correctly moved to logger.
• No secrets or tokens appear in migrated log calls.
• No migration changed console.log to logger.* in CLI contract files.
• No logger.info({ msg }) anti-pattern (message in object field instead of second arg) found.

Hot-path concern: ingestion pipeline files use logger.debug() or logger.warn() for error paths — these are gated at info default level, so no new verbosity in the ingestion hot path. The worker-pool.ts timeout logger.warn calls are not hot-path (they fire only on idle timeout events).

---

Security / log-injection assessment

Technically sound. Pino's NDJSON output JSON-encodes all string values. CR/LF embedded in a log field becomes \r\n in the JSON string — it cannot create a second newline-terminated NDJSON record. U+2028/U+2029 are encoded as \u2028/\u2029. Tests at logger.test.ts:126-159 validate these invariants directly using the production logger, not a reimplemented sanitizer.

bridge-db.ts security-critical path: now uses bridgeLogger.debug({ groupDir, err, attempts }, '...') instead of console.warn. Correct.

CodeQL GHAS: The latest CodeQL scan on this PR run completed with success — the js/log-injection alert is not fired on the new pino-based paths. The PR's technical claim (pino NDJSON is structurally injection-resistant) is validated by both the test suite and CodeQL.

pino-pretty in interactive mode: pino-pretty's ANSI colorization does not reintroduce injection risk — pretty output goes to a terminal for human reading, not to machine parsers.

---

ESLint / no-console assessment

The eslint.config.mjs config is clean:

• Core source (gitnexus/src/**/*.ts minus cli/server/logger.ts): no-console: 'error' — hard gate.
• CLI/server: no-console: ['error', { allow: ['log'] }] — allows stdout product output, disallows diagnostic warn/error/debug.
• No duplicate no-console keys (one rule per block, ignores array separates the scopes).
• Logger module itself (core/logger.ts) is excluded from both — necessary since it uses pino not console.*.
• git grep TODO(pino-migration) → 0 results confirmed.
• analyze.ts uses // eslint-disable-next-line no-console comments for intentional progress-bar console.warn/error capture — these are justified and commented.

The forcing function is structurally correct and effective.

---

Dependency / supply-chain assessment

• pino@10.3.1 is a runtime dependency — correct placement.
• pino-pretty@13.1.3 is a devDependency ("dev": true in lockfile) — correct.
• Package lock adds ~257 lines; lockfile matches the two new packages plus their transitive deps. No unexplained churn.
• pino's dependencies (safe-stable-stringify, sonic-boom, thread-stream, etc.) are well-known, reputable, pure-JS or minimal native. No unusual native addon risk.
• pino is MIT licensed. No license conflict.
• Production install (without devDependencies) will not include pino-pretty. Since shouldUsePretty() returns false in non-TTY contexts, there is no runtime crash if pino-pretty is absent from production.

---

Test assessment

logger.test.ts is solid:

• ✅ API surface (all level methods present)
• ✅ debug gating with and without env var
• ✅ falsy env var values (0, false, no, off)
• ✅ structured NDJSON record format and name field
• ✅ undefined Error.message does not throw
• ✅ CR/LF stays in one record (injection test)
• ✅ U+2028/U+2029 embedded in actual test string literals (characters confirmed present via hex dump)
• ✅ ANSI escape in string field stays in one record
• Tests assert on actual record content, not just "was called"
• Test capture isolation: _captureLogger() clears _cached and _activeDestination; restore() clears both. Records do not leak across tests as long as beforeEach/afterEach pattern is followed.

Updated call-site tests correctly use _captureLogger() instead of vi.spyOn(console, ...). Spot-checking ignore-service.test.ts and sync.test.ts shows assertions check cap.records().some(r => r.msg?.includes(...)) — these remain behaviorally meaningful.

Gap: No test directly verifies that the non-pretty path (non-TTY) writes to fd 2 rather than fd 1. The existing tests use the destination opt which bypasses the fd routing entirely. This is a minor gap — the non-pretty path is straightforward (pino.destination({dest: 2})), but an integration-style test capturing stderr would be more robust.

---

Performance / operational assessment

• Pino is one of the fastest Node.js loggers (sonic-boom, async write). Negligible overhead vs. console.*.
• Logger instances are created once per module at module load time — not in hot loops.
• info default level suppresses debug calls in ingestion hot paths. No new verbosity.
• pino-pretty runs in a worker thread — zero overhead in production (non-TTY), and in interactive mode the pretty overhead is irrelevant.
• Docker/CI: stderr is non-TTY, CI env is set — raw NDJSON, machine-parseable, correct.

---

Scope / hygiene assessment

• 80 changed TypeScript files. Changes are mechanically consistent (import → logger, console.* → logger.*). No unrelated functional changes detected in sampled files.
• Commit structure is logical: foundation → baseline-suppress → full migration → test cleanup → cli/server tighten.
• PR fix(core): close insecure-tempfile + log-injection in core/group (U6) #1329 sanitizer branch: this PR explicitly supersedes it. Since fix(core): close insecure-tempfile + log-injection in core/group (U6) #1329 is not merged, there is no conflict. The PR body documents this correctly.
• Hidden Unicode: no bidi/zero-width controls in any executable file. Em-dashes in comments are benign. The U+2028/U+2029 in logger.test.ts:145 are intentional test fixtures — GitHub's "hidden Unicode" warning is technically correct but semantically appropriate here.
• The gitnexus-web/ scope exclusion is explicitly stated in the PR and is correct.

---

Final verdict

❌ Not production-ready

Primary blocker: Finding #1 (pino-pretty stdout contamination) directly breaks the CLI stdout contract that is the central correctness claim of this PR. When a user runs gitnexus query "..." | jq — a canonical usage pattern — shouldUsePretty() returns true (stderr is a TTY), pino-pretty activates, and logQueryTiming() (called on every query at local-backend.ts:967) emits a formatted log line to stdout. jq receives a malformed stream. The fix is a single-line addition of destination: 2 to the pino-pretty transport options. This must be fixed before merge.

Secondary blocker: logQueryTiming() using logger.error() for query timing metrics (Finding #2) is both semantically wrong and compounds the primary blocker — it's the specific hot-path call that pollutes stdout in the pipe scenario. Additionally, the MCP startup banner at logger.error() will cause alerting noise for operators.

Once Findings #1 and #2 are resolved, this PR is otherwise well-structured: the migration is consistent, the logger wrapper is clean, the ESLint forcing function is correctly scoped, the dependency footprint is minimal and justified, the security argument is technically sound and CodeQL-confirmed, and the tests are meaningful. The unstructured string messages (Finding #3) are a follow-up quality item, not a blocker.
| Branch

[github-actions]

Claude finished @magyargergo's task in 5m 5s —— View job

---

PR #1336 Review: feat(core): adopt pino structured logger

Review bar for this PR

• core/logger.ts is small, typed, readable; destination: 2 routes all output to stderr by default.
• pino-pretty writes to stderr (fd 2) — destination: 2 present in transport options; shouldUsePretty() correctly gates on process.stderr.isTTY.
• CLI commands that output machine-readable JSON (query, cypher, impact, context) keep stdout clean — all data paths use writeSync(1, ...) raw fd writes; diagnostics go through pino on stderr.
• MCP stdio path (mcp.ts → server.ts) writes zero diagnostic output to stdout; pino logger never activates on stdout.
• ESLint no-console: error is the forcing function for core; no duplicate/conflicting rules; CLI/server exemptions are precise.
• No TODO(pino-migration) markers; no production console.* calls outside intended exemptions.
• CodeQL js/log-injection passes on latest run; pino NDJSON structural injection-resistance validated by test suite.

---

Current PR state

Attribute	Value
Head SHA	9227c26 (latest on feat/pino-logger)
Status	Open, 12 commits
Changed files	86
CI	✅ In-progress (previous full run: 8033/8033 passed)
CodeQL	✅ success
Gitleaks	✅ success
Dependency Review	✅ success
Docker Build	⏳ in-progress
TODO(pino-migration) markers	0 — confirmed by grep
Hidden bidi Unicode	None — em-dashes in comments only; U+2028/U+2029 in test fixtures at logger.test.ts:135 are intentional and clearly marked
pino-pretty in lockfile	In runtime dependencies (not devDeps) — intentional P1 fix
Vercel preview	Irrelevant to this review

---

Understanding of the change

GitNexus migrated 134 console.* call sites across gitnexus/src/ (excluding CLI/server stdout-contract owners) to a thin pino wrapper at core/logger.ts. Pino NDJSON is structurally log-injection-resistant — every string field is JSON-escaped in a single record per line — replacing PR #1329's hand-rolled sanitizeLogValue that had recurring edge cases. Logger defaults to stderr (fd 2) so CLI tool JSON data (query, cypher, impact) remains clean on stdout. ESLint no-console: error enforces the migration for future contributions. A new cli-message.ts helper (cliError/cliWarn/cliInfo) double-writes to both plain stderr AND pino for user-facing banners, addressing buffer-drain races on hard exit.

---

Findings

All three blocking issues identified in the previous CI review have been resolved in the current branch. No new blockers found.

Previous [CRITICAL] — pino-pretty stdout contamination ✅ RESOLVED

_tryBuildPrettyTransport() (logger.ts:171-183) now includes destination: 2:

options: {
  destination: 2,   // ← present in current branch
  colorize: true,
  translateTime: 'SYS:HH:MM:ss.l',
  ignore: 'pid,hostname',
},

shouldUsePretty() checks process.stderr.isTTY — semantically correct since pino-pretty now writes to fd 2. The destination: 2 transport option was added as the "P1 fix" (referenced in logger.test.ts:212). gitnexus query "..." | jq no longer risks a polluted stdout pipe from pino-pretty.

Previous [HIGH] — wrong log levels ✅ RESOLVED

• logQueryTiming() (local-backend.ts:185-189) now uses logger.debug({ query: truncated, totalMs, phases }, 'GitNexus query timing'). The comment explicitly documents why error level was wrong: "caused alerting rules to fire on every successful query."
• MCP startup banner (mcp.ts:77-80) now uses logger.info() / logger.warn() with structured { repoCount, repos } fields. No logger.error() on a healthy startup.

Previous [MEDIUM] — unstructured string messages ✅ RESOLVED

worker-pool.ts:301-313 now uses fully structured format:

logger.warn(
  { workerIndex, timeoutSec: job.timeoutMs / 1000, items: job.items.length,
    estimatedBytes: job.estimatedBytes, lastProgress, firstSplitItems, secondSplitItems, nextTimeoutSec },
  `Worker ${workerIndex} parse job idle timeout. Splitting into ...`
);

Log fields are independently queryable; aggregators can filter/alert on timeoutSec, items, estimatedBytes without string parsing.

---

Logger wrapper assessment

core/logger.ts (376 lines) is clean and well-structured:

• createLogger(name, opts?) is small, typed, and routes through a single code path whether or not a transport is configured.
• defaultDestination() uses pino.destination({ dest: 2, sync: false }) — stderr, buffered for performance on hot paths.
• shouldUsePretty() correctly gates on process.stderr.isTTY && !CI && !VITEST. Under piped stdout, stderr is still a TTY — but the pino-pretty destination: 2 option means any pretty output goes to stderr, never contaminating the pipe.
• resolveBaseLevel() reads GITNEXUS_LOG_LEVEL (the follow-up env var mentioned in the PR body is already implemented, not a follow-up).
• isPrettyAvailable() / _tryBuildPrettyTransport() probe and cache pino-pretty resolution; the one-time stderr warning is a direct process.stderr.write (no recursion risk).
• flushLoggerSync() and installFlushHook() cover buffer drain on graceful and ungraceful exits. All hard-exit paths (serve.ts, mcp/server.ts, tool.ts) call flushLoggerSync() before process.exit().
• _captureLogger() test API: double-capture guard throws on misuse; restore() is atomic (_activeDestination = undefined; _cached = undefined). No leak between tests as long as beforeEach/afterEach pattern is followed — the guard throws if misused.
• Proxy-backed singleton: Reflect.get preserves symbol-keyed lookups correctly (not a prop as string cast).
• pino.final is absent and deliberately not integrated — pino v10 removed the API because worker-thread transports handle their own flush on process exit. Comment in serve.ts explains this.
• One note, not a blocker: cli-message.ts's cliError/cliWarn/cliInfo double-writes (plain stderr + pino record). The module docstring correctly scopes these to user-facing banners only — internal diagnostics should use logger.* directly. This discipline is enforced by documentation, not ESLint; if over-used in hot paths, cliInfo() will emit twice per call. Not a blocker; the docstring is clear.

---

CLI/MCP stdout-stderr contract assessment

Fully sound. No stdout contamination paths found.

• query / cypher / impact / context (tool.ts): output via writeSync(1, ...) (raw fd 1 write, bypasses Node.js stream layer). Diagnostic errors use cliError() which writes to process.stderr directly and tees a pino record. pino-pretty with destination: 2 means even in TTY mode, pino output stays on stderr.
• gitnexus analyze (analyze.ts): progress bar on stdout; console.warn/console.error are captured and rerouted through barLog for the lifetime of the run (lines 360-373). These are behind // eslint-disable-next-line no-console -- intentional console-routing for progress bar UX. This is a deliberate UI routing pattern, not a logging concern. realStderrWrite is captured at line 37 for fatal error paths that must bypass the bar routing.
• MCP stdio (cli/mcp.ts → mcp/server.ts): installGlobalStdoutSentinel() is the first statement before any heavy import. Logger is dynamically imported (await import(...)) to preserve the leaf static-import closure invariant. server.ts crash handlers use process.stderr.write directly (not buffered pino) — correct for the crash path where buffer drain can't be trusted. shutdown() calls flushLoggerSync() before process.exit.
• Integration test (test/integration/cli/tool-no-index-stderr.test.ts): spawns the built CLI with CI=1 (forces NDJSON, no TTY), asserts stderr.contains('No indexed repositories found') and stdout.not.contains(...). This is the PR's concrete e2e stdout-cleanliness proof.

---

Migration correctness assessment

Sampled across representative files:

File	Pattern	Assessment
bridge-db.ts	createLogger('bridge-db', { debugEnvVar: 'GITNEXUS_DEBUG_BRIDGE' })	✅ debug gate preserved; structured { groupDir, err, attempts }
worker-pool.ts	logger.warn({ workerIndex, timeoutSec, items, ... }, 'msg')	✅ fully structured; all fields preserved
local-backend.ts:logQueryTiming	logger.debug({ query, totalMs, phases }, 'msg')	✅ debug level; structured fields
mcp.ts	logger.info({ repoCount, repos }, 'msg') / logger.warn(...)	✅ correct levels
serve.ts crash handlers	logger.error({ err }, 'msg'); flushLoggerSync(); process.exit(1)	✅ Error object preserved; buffer flushed
optional-grammars.ts	cliWarn(msg, { grammar, extensions, error })	✅ previously console.error; now structured + plain text
cli-message.ts	writeStderr(msg); logger.*(fields, msg)	✅ new pattern; correct dual-write for user-facing output

No logger.info({ msg }) anti-pattern (message in object field) found. No logger.error(err) losing message context. No secrets newly logged. No high-volume hot-loop at info level (ingestion loops use logger.debug or logger.warn on error paths only).

---

Security / log-injection assessment

Technically sound. CodeQL confirms.

• Pino NDJSON: every string value is JSON.stringify-escaped. CR/LF in a field becomes \r\n in the JSON payload — cannot forge a second NDJSON record. U+2028/U+2029 become \u2028/\u2029. ANSI sequences are inert inside a JSON string.
• logger.test.ts validates these invariants directly against the production logger:
  • CR/LF in a string field stays inside one NDJSON record — line 116
  • U+2028/U+2029 in a string field stays inside one NDJSON record — line 132 (literal characters confirmed in source)
  • ANSI escape sequence in a string field stays inside one NDJSON record — line 143
• bridge-db.ts (security-critical path): openBridgeDbReadOnly logs via bridgeLogger.debug({ groupDir, err, attempts }, ...) — structured, injection-safe.
• No sanitizeLogValue helper in this branch.
• CodeQL js/log-injection status: ✅ success on latest run (25503998453). The PR's technical claim is validated.

---

ESLint / no-console assessment

The eslint.config.mjs is clean. The rendered diff appeared to show duplicate no-console keys — the actual file has a single unambiguous rule per block:

Glob	Rule	Effect
gitnexus/src/cli/**, gitnexus/src/server/**	['error', { allow: ['log'] }]	Only console.log allowed (stdout contract)
gitnexus/src/** (excl. cli, server, logger.ts)	'error'	No console.* at all
MCP-reachable block	['error', { allow: ['error'] }]	console.error (→ stderr) allowed in deep MCP paths

ESLint flat-config rule ordering is correct: the MCP-reachable block is declared after the core-source block, so for gitnexus/src/mcp/**/*.ts the MCP rule (allow: ['error']) takes precedence via last-wins semantics. This is intentional — MCP code may need a direct stderr escape hatch. The no-restricted-syntax selectors block all process.stdout.write calls in MCP-reachable code.

git grep TODO(pino-migration) → 0 matches.

Remaining console.* in gitnexus/src/:

• cli/skill-gen.ts, cli/list.ts, cli/wiki.ts: all console.log — legitimate CLI stdout output, exempted.
• cli/analyze.ts: console.warn/console.error binding captures with explicit eslint-disable comments — intentional progress-bar UX routing, not diagnostic calls.
• core/lbug/lbug-adapter.ts:749: comment-only reference (// Don't console.error here).
• cli/optional-grammars.ts:72: comment-only reference.

Zero production diagnostic console.* calls outside intended exemptions.

---

Dependency / supply-chain assessment

• pino@10.3.1: runtime dependency ✅
• pino-pretty@13.1.3: runtime dependency (not devDep) ✅ — moved to dependencies in the P1 fix. This ensures isPrettyAvailable() always returns true in standard installs and the isPrettyAvailable() probe is a safety net for --omit=optional-style or stripped installs only. Trade-off: ~200KB added to production CLI bundle — acceptable for a CLI tool (not a library).
• Lockfile: +246 lines, matches pino + pino-pretty + transitive deps. No unrelated churn.
• All new transitive deps (sonic-boom, thread-stream, safe-stable-stringify, etc.) are well-known pure-JS packages, MIT/Apache licensed.
• No native addons introduced by pino.
• ESM import style: import pino from 'pino' — compatible with "type": "module" in package.json.

---

Test assessment

logger.test.ts (279 lines, 12 tests): covers API surface, debug gating, env-var falsiness spectrum (0/false/no/off), NDJSON structure and name field, Error with undefined message, CR/LF/U+2028/U+2029/ANSI single-record invariants, _captureLogger lifecycle (double-capture guard, re-capture after restore), _tryBuildPrettyTransport with destination: 2 assertion, flushLoggerSync safety, one-time stderr warning bound. Tests exercise the production logger, not a reimplementation.

Updated call-site tests correctly use _captureLogger() / cap.records() instead of vi.spyOn(console, ...). Assertions check .some(r => r.msg?.includes(...)) — behaviorally meaningful.

Integration test test/integration/cli/tool-no-index-stderr.test.ts: spawns real CLI binary, asserts:

1. stderr contains the no-index diagnostic
2. stdout does NOT contain the diagnostic
3. Exit code is 1

This is the concrete e2e proof of CLI stdout cleanliness that was the gap called out in the previous review.

Minor gap (not a blocker): No test directly verifies that the non-pretty NDJSON path writes to fd 2 in a live subprocess. The existing tests use the destination opt which bypasses fd routing. The integration test above covers the e2e contract sufficiently.

---

Performance / operational assessment

• Logger instances created once per module at load time — not in hot loops.
• info default level suppresses debug calls in ingestion hot paths. worker-pool.ts timeout warns (logger.warn) fire only on idle-timeout events, not per-item.
• Pino's SonicBoom (sync: false) means logger calls in the ingestion pipeline are async-buffered — negligible per-call overhead vs. former console.*.
• pino-pretty runs in a worker thread under TTY; zero overhead in non-TTY contexts (production, Docker, CI).
• Docker/non-TTY: VITEST / CI unset or stderr non-TTY → raw NDJSON to stderr → machine-parseable.

---

Scope / hygiene assessment

• 86 changed files; changes are logging-only except for:
  • package.json / package-lock.json — dep additions
  • eslint.config.mjs — no-console rules
  • gitnexus/src/cli/cli-message.ts — new helper (justified by buffer-drain race)
  • Test additions/updates — appropriate
• No unrelated functional changes detected in sampled files.
• PR fix(core): close insecure-tempfile + log-injection in core/group (U6) #1329 (sanitizeLogValue): not merged; this PR intentionally supersedes it. No merge-order conflict.
• gitnexus-web/ exclusion: correct and explicitly documented.
• Hidden Unicode: em-dashes (—) in comments throughout; the U+2028/U+2029 in logger.test.ts:135 are intentional test fixtures (const evil = 'before\u2028after\u2029more'). No bidi/zero-width controls anywhere in executable code.

---

Final verdict

✅ Production-ready

All three blocking issues from the previous adversarial review have been resolved: destination: 2 is present in the pino-pretty transport options, logQueryTiming() uses logger.debug() with structured fields, and the MCP startup banner uses logger.info(). The migration is complete, consistent, and correct — ESLint enforcement is sound, the dependency footprint is minimal and justified, CodeQL confirms the log-injection claim, and a new integration test (tool-no-index-stderr.test.ts) provides concrete e2e proof that CLI stdout stays clean. The cli-message.ts helper and flushLoggerSync() pattern address the buffer-drain race that would have silently dropped diagnostics on hard exits. There are no remaining blockers.

Minor follow-ups (not blocking):

• cli-message.ts double-write discipline should be enforced by convention: if cliInfo/cliWarn/cliError drift into internal diagnostics or ingestion paths, per-call overhead doubles. An @internal or doc-comment lint annotation could help — but the current module docstring is clear.
• No test directly verifies pino's non-pretty NDJSON writes to fd 2 in a subprocess (the integration test covers the e2e observable; the fd-level assertion is a minor gap).
• GITNEXUS_LOG_LEVEL is implemented (not just a follow-up anymore) — update the PR description to reflect this.

---

| Branch