From 4279937679fee425f986853513fcee1d4849630d Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Tue, 23 Jun 2026 17:23:34 +0000 Subject: [PATCH] fix(mcp): log swallowed best-effort query degradations at warn, not error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- gitnexus/src/mcp/local/local-backend.ts | 34 +++++++++++++++++--- gitnexus/test/unit/calltool-dispatch.test.ts | 30 +++++++++++------ 2 files changed, 51 insertions(+), 13 deletions(-) diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index 8ea6880e7..7ea98463c 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -266,10 +266,31 @@ export const IMPACT_RELATION_CONFIDENCE: Readonly> = { const confidenceForRelType = (relType: string | undefined): number => IMPACT_RELATION_CONFIDENCE[relType ?? ''] ?? 0.5; -/** Structured error logging for query failures — replaces empty catch blocks */ +/** + * Structured logging for *swallowed* query failures — replaces empty catch + * blocks. Every caller of this helper catches the failure and degrades to a + * safe fallback (the operation still returns a result), so these are NOT + * operation-level errors and must not be logged at `error`: + * + * - A benign missing optional table/label/column — a repo analyzed without + * processes/communities, or a pre-v3 PDG index lacking the `calleeIds` + * column — is a normal configuration, not a failure. Logged at `debug` + * (suppressed at the default `info` level; surfaced only when troubleshooting). + * - Any other swallowed failure is an unexpected-but-handled degradation: + * logged at `warn` so it stays observable without raising a false `error` + * alarm that would drown genuine, operation-aborting failures. + * + * `error` is intentionally NOT used here — it is reserved for failures that + * actually abort an operation, which log directly rather than through this + * best-effort-degradation helper. + */ function logQueryError(context: string, err: unknown): void { const msg = err instanceof Error ? err.message : String(err); - logger.error({ context, err: msg }, 'GitNexus query failed'); + if (isBenignMissingTableError(err)) { + logger.debug({ context, err: msg }, 'GitNexus query skipped (missing optional data)'); + return; + } + logger.warn({ context, err: msg }, 'GitNexus query failed (degraded)'); } /** @@ -1955,9 +1976,14 @@ export class LocalBackend { try { ftsResponse = await searchFTSFromLbug(query, limit, repo.lbugPath); } catch (err: any) { - logger.error( + // Swallowed, gracefully-degraded failure: the search falls back to + // semantic-only (a valid result), and the most common cause is simply an + // un-indexed FTS extension — a normal configuration, not an operation + // error. Logged at warn (matching the sibling import-failure fallback + // above), never error, so it does not raise a false alarm. + logger.warn( { err: err.message }, - 'GitNexus: BM25/FTS search failed (FTS indexes may not exist) -', + 'GitNexus: BM25/FTS search failed (FTS indexes may not exist) — falling back to semantic-only', ); return { results: [], ftsUsed: false }; } diff --git a/gitnexus/test/unit/calltool-dispatch.test.ts b/gitnexus/test/unit/calltool-dispatch.test.ts index 9fc72086f..e8fad5928 100644 --- a/gitnexus/test/unit/calltool-dispatch.test.ts +++ b/gitnexus/test/unit/calltool-dispatch.test.ts @@ -1866,15 +1866,27 @@ describe('LocalBackend impact mode (KTD1/KTD5/KTD12)', () => { criterionLine: 8, }); const bfsSpy = vi.spyOn(backend as any, '_runImpactBFS'); - const result = await backend.callTool('impact', { - target: 'main', - direction: 'downstream', - mode: 'pdg', - line: 8, - }); - // The error was swallowed: no bridge passed to the BFS, and no error surfaced. - expect(result.error).toBeUndefined(); - expect(bfsSpy.mock.calls[0][4].pdgBridge).toBeUndefined(); + const cap = _captureLogger(); + try { + const result = await backend.callTool('impact', { + target: 'main', + direction: 'downstream', + mode: 'pdg', + line: 8, + }); + // The error was swallowed: no bridge passed to the BFS, and no error surfaced. + expect(result.error).toBeUndefined(); + expect(bfsSpy.mock.calls[0][4].pdgBridge).toBeUndefined(); + // The swallowed, gracefully-degraded query failure is logged at warn (40), + // never error (50): it degraded to a safe fallback and is not an operation + // failure. Pinning the severity guards against a regression to a false + // ERROR alarm that would drown genuine, operation-aborting failures. + const slice = cap.records().find((r) => r.context === 'impact:pdg-slice-callees'); + expect(slice).toBeDefined(); + expect(slice?.level).toBe(40); + } finally { + cap.restore(); + } }); it("mode:'pdg' + crossDepth → hard {error} (single-repo PDG impact)", async () => {