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>
This commit is contained in:
Gergo Magyar 2026-06-23 17:23:34 +00:00
parent cc761ce7cd
commit 4279937679
2 changed files with 51 additions and 13 deletions

View file

@ -266,10 +266,31 @@ export const IMPACT_RELATION_CONFIDENCE: Readonly<Record<string, number>> = {
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 };
}

View file

@ -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 () => {