GitNexus/gitnexus/src/core/logger.ts
Gergő Magyar 47477e5554
fix(mcp): tolerate adapter-materialized line:0 in impact callgraph mode (#2279) (#2283)
* fix(mcp): tolerate adapter-materialized line:0 in impact callgraph mode (#2279)

Some MCP client/agent adapters serialize an omitted optional numeric
field as `0` rather than dropping it, so callgraph `impact` calls arrive
carrying a spurious `line: 0`. `line` is a PDG-only statement anchor and
is meaningless on the callgraph path, so the backend rejected the call
("'line' is only supported with mode:'pdg'") and strict clients rejected
it client-side against the advertised `minimum: 1`.

Treat a literal `line: 0` as omitted in `_impactImpl` when mode !== 'pdg'
and let the normal symbol→symbol BFS run. The coercion is deliberately
narrow: only the literal 0, only on the callgraph path. A genuine
positive `line` on callgraph still errors (real mode mistake), negative/
fractional values still error, and pdg mode is untouched — `line: 0`
there is still rejected (there is no 1-based source line 0 to anchor on).

Regression tests pin the full matrix: callgraph + line:0 runs the BFS and
is byte-identical to omitting line; pdg + line:0 still errors; positive
line on callgraph still errors.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(mcp): log swallowed best-effort query degradations at warn, not error

`logQueryError` is the shared handler for query failures that every caller
catches and degrades past with a safe fallback (the operation still returns
a result). It logged all of them at `logger.error` (level 50) — the same
severity as fatal failures — so a gracefully-handled degradation raised a
false alarm and drowned genuine errors. This surfaced as an ERROR-level log
firing during a passing unit test that intentionally injects a slice-callees
query failure to verify the degrade path.

Make the severity match reality:
  - benign missing optional table/label/column (a repo analyzed without
    processes/communities, or a pre-v3 PDG index lacking the `calleeIds`
    column — a query that fails on every pdg-downstream impact for such an
    index) → debug, the normal-configuration case.
  - any other swallowed failure → warn (handled degradation, still observable).
  - error is reserved for failures that actually abort an operation, which
    log directly rather than through this helper.

Also fix the sibling bm25/FTS fallback, which logged its swallowed
"FTS indexes may not exist" degradation at error while its own import-failure
fallback already used warn.

The slice-callees degradation test now captures the log and asserts it lands
at warn (40), not error (50), pinning the severity against regression.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(mcp): relax impact `line` schema minimum to 0 for adapter compatibility (#2279)

Strict MCP clients/agents validate against the advertised input schema and
reject a request before sending it. With `line` declaring `minimum: 1`, a
client that materializes the omitted optional `line` as `0` rejects a
perfectly valid callgraph impact call client-side — so the backend tolerance
added in the previous commit never gets a chance to run.

Lower the advertised `line.minimum` to 0 and document that 0 (or omission)
means "no statement anchor" while mode:'pdg' still requires a positive line.
The advertised schema is advisory (the backend self-validates and is the real
gate), so this cannot loosen any enforced contract — it only stops strict
clients from pre-rejecting `line: 0`. Negative lines are still rejected at the
client boundary.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(review): apply autofix feedback

Code-review autofix pass on the #2279 branch:
- Replace a newly-introduced `mode as any` cast in the #2279 it.each with the
  narrow `mode as 'callgraph' | undefined` (strict-typing-no-any).
- Add a degradation test for the new logQueryError benign-missing-table → debug
  branch (asserts no warn/error record surfaces, i.e. it routed to debug).
- Pin the bm25/FTS error→warn severity change with a _captureLogger assertion
  in the existing #1489 test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(mcp): make swallowed-failure callers surface degradation; narrow benign-error match (#2283)

Tri-review (#2283) found the `error → {debug|warn}` rework reduced telemetry
for `logQueryError` callers that do NOT degrade safely, while the docstring
over-claimed "every caller degrades to a safe fallback". Address the substance
rather than only the log level:

- rename apply-edit: track failed writes and return status:'partial' with
  `failed_files` instead of reporting `status:'success'` when a write was
  swallowed. A partial rename is no longer indistinguishable from a clean one.
- detect_changes: a swallowed symbol/process query failure now sets
  `partial:true` (rendered by the existing eval-server partial path) so the
  pre-commit safety gate can't return a false-clean `risk_level:'low'` no-op.
- isBenignMissingTableError: scope the `not (defined|found)` arm to a schema
  object (table/label/rel/column/property), mirroring lbug-adapter's
  isMissingColumnError. An unscoped "not found" matched operation failures like
  `rg: not found` / `Symbol not found` and silently demoted them to debug.
- logQueryError docstring: state the contract honestly — level reflects
  telemetry severity, and mutating/safety-critical callers MUST also surface a
  result-level degradation signal; `warn` alone is not a substitute.
- pdg dispatch: pass the normalized `effectiveLine` (not raw params.line) so
  the validation gate and engine share one source of truth (identity today).

Tests:
- _captureLogger(level?) lets tests capture below info; the benign-missing-table
  test now asserts the record IS emitted at debug (20), not merely absent —
  no longer a vacuous pass if the call were deleted.
- new: a non-schema "not found" failure logs at warn (regex-narrowing guard);
  rename write-failure degrades to status:'partial'+failed_files; line:-1 on
  the callgraph path still errors (line:0 coercion is narrow); typed the
  it.each tuple to drop a `mode as` cast.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* docs(mcp): fix impact `line` description contradiction for whole-symbol pdg (#2283)

The new `line` schema description said "mode:'pdg' requires a positive line",
which contradicted the top-level impact description ("Without 'line', pdg
returns whole-symbol inter-procedural reach plus local whole-symbol PDG
diagnostics"). A pdg call without a line is a valid (degraded whole-symbol)
call, not an error — the old wording could push an agent to avoid valid no-line
pdg calls or synthesize line:0 (which then hard-errors).

Reword to: omit line for whole-symbol pdg; a positive line anchors a statement
slice; literal 0 is tolerated only as an omitted-line compatibility sentinel on
the callgraph path and is rejected for mode:'pdg'. Update the schema test to
pin the new, non-contradictory wording and assert "requires a positive line" is
gone.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-23 20:35:49 +01:00

387 lines
14 KiB
TypeScript

/**
* Centralized structured logger for GitNexus.
*
* Wraps `pino` so the rest of the codebase imports from one place. Pino's
* NDJSON output is structurally log-injection-resistant (CWE-117 / CodeQL
* `js/log-injection`): each record is a single JSON object on its own line,
* with all string field values JSON-escaped. This replaces hand-rolled
* sanitizers (see PR #1329 history) that had recurring edge-case gaps
* (undefined Error.message, U+2028/U+2029, ANSI/C0).
*
* Usage:
* import { logger, createLogger } from '../core/logger.js';
* logger.warn({ groupDir }, 'msg');
* const childLogger = createLogger('bridge-db', { debugEnvVar: 'GITNEXUS_DEBUG_BRIDGE' });
*
* Operator semantics:
* - Default level: 'info' (matches pino default; preserves visibility of
* existing `console.log` migrations)
* - When `opts.debugEnvVar` is set and that env var is truthy at
* createLogger time, that named child logs at level 'debug'
* - Output is NDJSON in production / CI / vitest. pino-pretty is used only
* when stdout is a TTY AND CI is unset AND VITEST is unset, so test
* and pipeline output stay parseable.
*
* Test capture:
* The exported `logger` singleton is a Proxy that forwards every call to a
* lazily-built pino instance. Tests use `_captureLogger()` to redirect that
* inner instance to a memory stream so they can assert on records the
* production code logged. See `gitnexus/test/unit/logger.test.ts` for the
* pattern.
*/
import pino, { type Logger, type LoggerOptions, type DestinationStream } from 'pino';
import { Writable } from 'node:stream';
import { createRequire } from 'node:module';
export interface CreateLoggerOptions {
/** When set, this env var (truthy at construction time) bumps level to 'debug'. */
debugEnvVar?: string;
/** Override destination stream — primarily for tests. */
destination?: DestinationStream;
/**
* Explicit level for the destination-override path — primarily for tests that
* need to capture below the default `info` (e.g. asserting a `debug` record).
* Ignored unless `destination` is set; `debugEnvVar` still wins when truthy.
*/
level?: string;
}
function isTruthyEnv(value: string | undefined): boolean {
if (!value) return false;
const v = value.toLowerCase();
return v !== '' && v !== '0' && v !== 'false' && v !== 'no' && v !== 'off';
}
function shouldUsePretty(): boolean {
// Logger writes to stderr (fd 2) so CLI data on stdout (fd 1) stays clean.
// Pretty-print only when stderr is a TTY and not in CI/test environments.
return (
process.stderr.isTTY === true &&
!isTruthyEnv(process.env.CI) &&
!isTruthyEnv(process.env.VITEST)
);
}
/**
* Default pino destination — writes to stderr (fd 2) so CLI commands can
* keep stdout (fd 1) clean for tool data output (#324). Pino defaults to
* stdout; we override here.
*
* `sync: false` (SonicBoom buffered writes) so logger calls don't issue a
* blocking `write(2)` syscall on every record. Hot paths (parse-impl,
* ingestion phases, per-query backend calls) pay the cost without it.
*
* The buffered-write trade-off is record loss on hard exit. We mitigate via:
* - A `process.on('beforeExit')` hook below that calls `flushSync()` on
* normal exits.
* - The exported `flushLoggerSync()` helper, which entry-point shutdown
* handlers (SIGINT/SIGTERM) MUST call before `process.exit(N)` so
* in-flight buffered records still reach stderr.
* - `pino.final(...)` integration in `uncaughtException` / `unhandledRejection`
* handlers (see `gitnexus/src/cli/serve.ts` and `gitnexus/src/server/api.ts`).
*
* Skipped under `VITEST` so vitest's between-test cleanup doesn't fight
* `_captureLogger()`'s lifecycle. Tests use an in-memory destination via
* `_captureLogger()` and never reach this branch.
*/
let _dest: ReturnType<typeof pino.destination> | undefined;
function defaultDestination(): DestinationStream {
if (_dest) return _dest;
_dest = pino.destination({ dest: 2, sync: false });
return _dest;
}
/**
* Flush any buffered records on the default destination. Entry-point
* shutdown handlers (`SIGINT` / `SIGTERM`) MUST call this before
* `process.exit(N)` — otherwise async-buffered records are lost on hard
* exit. No-op when the destination hasn't been constructed yet (logger
* module imported but never emitted) or when called from `_captureLogger`
* test mode (tests use an in-memory destination).
*/
export function flushLoggerSync(): void {
if (!_dest) return;
try {
_dest.flushSync();
} catch {
// Defend against a destination that has already been closed (e.g.,
// double-flush on rapid shutdown). Losing the flush attempt is the
// correct trade-off vs. throwing during shutdown.
}
}
/**
* Idempotent registration: `process.on('beforeExit')` flushes the buffered
* destination before normal exit. Skipped under VITEST to avoid interfering
* with `_captureLogger()`'s lifecycle and vitest's per-worker cleanup.
*/
let _flushHookInstalled = false;
function installFlushHook(): void {
if (_flushHookInstalled) return;
if (isTruthyEnv(process.env.VITEST)) return;
_flushHookInstalled = true;
process.on('beforeExit', () => {
flushLoggerSync();
});
}
/**
* Probe whether `pino-pretty` is resolvable from this module. Cached for
* the lifetime of the process — the resolve cost only happens once, and
* the one-time stderr warning on miss only fires once.
*
* Production installs ship pino-pretty as a runtime dependency (see
* gitnexus/package.json). The probe is the safety net for `--omit=optional`,
* `--no-package-lock` style installs and for any environment where the
* module turns out to be missing for reasons we can't predict — pino's
* own transport-resolution path resolves the target lazily at FIRST log
* write, so without this probe a missing module would throw deep inside
* the pino call site rather than at logger construction.
*/
let _prettyAvailable: boolean | null = null;
const _require = createRequire(import.meta.url);
function isPrettyAvailable(): boolean {
if (_prettyAvailable !== null) return _prettyAvailable;
try {
_require.resolve('pino-pretty');
_prettyAvailable = true;
} catch {
_prettyAvailable = false;
// One-time stderr warning so operators learn why TTY output is plain
// NDJSON instead of pretty-printed. Use realStderrWrite-style direct
// write — going through `logger` here would recurse.
process.stderr.write(
'[gitnexus:logger] pino-pretty unavailable; falling back to NDJSON on stderr\n',
);
}
return _prettyAvailable;
}
/**
* @internal Test-only reset for the pino-pretty availability cache. Lets
* unit tests exercise both resolve outcomes within the same vitest worker.
*/
export function _resetPrettyAvailableCache(): void {
_prettyAvailable = null;
}
/**
* Build the pino-pretty transport options. Internal — exported only so unit
* tests can exercise the probe path without going through `shouldUsePretty()`
* (which is structurally false under vitest).
*/
export function _tryBuildPrettyTransport(): LoggerOptions['transport'] | undefined {
if (!isPrettyAvailable()) return undefined;
return {
target: 'pino-pretty',
options: {
// Route to stderr (fd 2) so pretty output doesn't contaminate
// CLI tool data on stdout (fd 1). pino-pretty's default is fd 1,
// which would interleave with `gitnexus query | jq` output.
destination: 2,
colorize: true,
translateTime: 'SYS:HH:MM:ss.l',
ignore: 'pid,hostname',
},
};
}
/**
* Pino accepts `'fatal' | 'error' | 'warn' | 'info' | 'debug' | 'trace' | 'silent'`.
* Anything else is silently ignored at runtime; we narrow here so a typo in
* the env var produces the documented default rather than masking the issue.
*/
const PINO_LEVELS = new Set(['fatal', 'error', 'warn', 'info', 'debug', 'trace', 'silent']);
function resolveBaseLevel(): string {
const fromEnv = process.env.GITNEXUS_LOG_LEVEL;
if (fromEnv && PINO_LEVELS.has(fromEnv.toLowerCase())) {
return fromEnv.toLowerCase();
}
return 'info';
}
function buildBaseOptions(): LoggerOptions {
const opts: LoggerOptions = {
level: resolveBaseLevel(),
base: undefined,
};
if (shouldUsePretty()) {
const transport = _tryBuildPrettyTransport();
if (transport) opts.transport = transport;
}
return opts;
}
/**
* Create a named child logger. When `opts.destination` is provided it bypasses
* the default stdout sink (useful for test capture). When `opts.debugEnvVar` is
* set and truthy at call time, the child runs at 'debug' level.
*/
export function createLogger(name: string, opts?: CreateLoggerOptions): Logger {
const debugRequested = opts?.debugEnvVar ? isTruthyEnv(process.env[opts.debugEnvVar]) : false;
if (opts?.destination) {
return pino(
{ level: debugRequested ? 'debug' : (opts.level ?? 'info'), base: undefined, name },
opts.destination,
);
}
const base = buildBaseOptions();
// When using a transport (pino-pretty), pino manages the destination
// internally and we cannot pass one explicitly. When transport is absent,
// route to stderr so stdout stays clean for CLI data output.
let root: Logger;
if (base.transport) {
root = pino({ ...base, level: debugRequested ? 'debug' : base.level });
} else {
root = pino({ ...base, level: debugRequested ? 'debug' : base.level }, defaultDestination());
// The default destination is buffered (`sync: false`); register the
// graceful-exit flush hook now that we know the destination will be
// used. Idempotent — runs at most once per process. Skipped under
// VITEST so test cleanup doesn't fight `_captureLogger`.
installFlushHook();
}
return root.child({ name });
}
/* ------------------------------------------------------------------ */
/* Default singleton (Proxy-backed for test capture) */
/* ------------------------------------------------------------------ */
let _activeDestination: DestinationStream | undefined;
let _activeLevel: string | undefined;
let _cached: Logger | undefined;
function _getInner(): Logger {
if (_cached) return _cached;
// Always go through createLogger so future defaults (serializers, redaction,
// formatters) apply uniformly. The destination override is honored when set
// by `_captureLogger()` below.
_cached = createLogger(
'gitnexus',
_activeDestination ? { destination: _activeDestination, level: _activeLevel } : undefined,
);
return _cached;
}
/**
* Default singleton logger (`name: 'gitnexus'`). Backed by a Proxy so test
* capture (`_captureLogger()`) can redirect output without breaking modules
* that already imported the singleton at module-load time.
*/
export const logger = new Proxy({} as Logger, {
get(_target, prop) {
const inner = _getInner();
// Reflect.get keeps symbol-keyed lookups (e.g. Symbol.toPrimitive) intact;
// a `prop as string` cast would silently coerce them to the wrong key.
const value = Reflect.get(inner as object, prop, inner);
if (typeof value === 'function') {
return (value as (...a: unknown[]) => unknown).bind(inner);
}
return value;
},
}) as Logger;
/**
* Shape of a parsed pino record. `level`, `time`, and `msg` are always
* present; `name` is set when emitted from a named child logger; arbitrary
* additional fields appear when callers pass a structured first arg.
*
* Exported so test helpers and downstream skills can type-narrow capture
* results without inline `Record<string, unknown>` casts.
*/
export interface PinoLogRecord {
level: number;
time: number;
msg: string;
name?: string;
[key: string]: unknown;
}
/**
* In-memory Writable used by `_captureLogger()` and by tests that build
* their own pino destination. Exported so the shape lives in one place
* (previously duplicated between this module and `logger.test.ts`).
*
* `text()` and `records()` are convenience helpers test code calls. They
* don't appear in production hot paths — only test destinations capture
* here — so the surface is intentionally small.
*/
export class MemoryWritable extends Writable {
chunks: string[] = [];
_write(chunk: Buffer | string, _enc: BufferEncoding, cb: (err?: Error | null) => void): void {
this.chunks.push(typeof chunk === 'string' ? chunk : chunk.toString('utf-8'));
cb();
}
/** Concatenate every captured write back into a single string. */
text(): string {
return this.chunks.join('');
}
/** Parse captured writes as one NDJSON record per non-empty line. */
records(): PinoLogRecord[] {
return this.text()
.split('\n')
.filter((l) => l.length > 0)
.map((l) => JSON.parse(l) as PinoLogRecord);
}
}
export interface LoggerCapture {
records(): PinoLogRecord[];
text(): string;
restore(): void;
}
/**
* Test helper. Redirects the default `logger` singleton to an in-memory
* stream and returns a capture object plus a restore function.
*
* Pattern:
* let cap: LoggerCapture;
* beforeEach(() => { cap = _captureLogger(); });
* afterEach(() => { cap.restore(); });
* it('warns', () => {
* fnUnderTest();
* expect(cap.records().some(r => r.msg?.includes('clamping'))).toBe(true);
* });
*
* Pass `level` (e.g. 'debug') to capture below the default 'info' — needed to
* assert that a record was emitted at debug rather than merely absent.
*
* Not a public API; underscore-prefixed and called only from test code.
* Throws if a previous capture is still active — see the body for context.
*/
export function _captureLogger(level?: string): LoggerCapture {
// Guard against double-capture: forgetting `restore()` between two
// `_captureLogger()` calls silently abandoned the previous capture and
// corrupted logger state for the rest of the vitest worker. Throwing here
// surfaces the bug at the moment of misuse instead of as inscrutable
// missing-records assertions in unrelated tests.
if (_activeDestination !== undefined) {
throw new Error(
'_captureLogger: a previous capture is still active — call restore() before starting a new one.',
);
}
const w = new MemoryWritable();
_activeDestination = w;
_activeLevel = level;
_cached = undefined;
return {
records: () =>
w.chunks
.join('')
.split('\n')
.filter((l) => l.length > 0)
.map((l) => JSON.parse(l) as PinoLogRecord),
text: () => w.chunks.join(''),
restore: () => {
_activeDestination = undefined;
_activeLevel = undefined;
_cached = undefined;
},
};
}