mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-08-28 05:25:25 +00:00
* 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>
This commit is contained in:
Gergő Magyar
GitHub
parent
698f5efc82
commit
47477e5554
5 changed files with 351 additions and 31 deletions
|
|
@ -38,6 +38,12 @@ export interface CreateLoggerOptions {
|
|||
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 {
|
||||
|
|
@ -219,7 +225,7 @@ export function createLogger(name: string, opts?: CreateLoggerOptions): Logger {
|
|||
|
||||
if (opts?.destination) {
|
||||
return pino(
|
||||
{ level: debugRequested ? 'debug' : 'info', base: undefined, name },
|
||||
{ level: debugRequested ? 'debug' : (opts.level ?? 'info'), base: undefined, name },
|
||||
opts.destination,
|
||||
);
|
||||
}
|
||||
|
|
@ -247,6 +253,7 @@ export function createLogger(name: string, opts?: CreateLoggerOptions): Logger {
|
|||
/* ------------------------------------------------------------------ */
|
||||
|
||||
let _activeDestination: DestinationStream | undefined;
|
||||
let _activeLevel: string | undefined;
|
||||
let _cached: Logger | undefined;
|
||||
|
||||
function _getInner(): Logger {
|
||||
|
|
@ -256,7 +263,7 @@ function _getInner(): Logger {
|
|||
// by `_captureLogger()` below.
|
||||
_cached = createLogger(
|
||||
'gitnexus',
|
||||
_activeDestination ? { destination: _activeDestination } : undefined,
|
||||
_activeDestination ? { destination: _activeDestination, level: _activeLevel } : undefined,
|
||||
);
|
||||
return _cached;
|
||||
}
|
||||
|
|
@ -342,10 +349,13 @@ export interface LoggerCapture {
|
|||
* 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(): LoggerCapture {
|
||||
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
|
||||
|
|
@ -358,6 +368,7 @@ export function _captureLogger(): LoggerCapture {
|
|||
}
|
||||
const w = new MemoryWritable();
|
||||
_activeDestination = w;
|
||||
_activeLevel = level;
|
||||
_cached = undefined;
|
||||
return {
|
||||
records: () =>
|
||||
|
|
@ -369,6 +380,7 @@ export function _captureLogger(): LoggerCapture {
|
|||
text: () => w.chunks.join(''),
|
||||
restore: () => {
|
||||
_activeDestination = undefined;
|
||||
_activeLevel = undefined;
|
||||
_cached = undefined;
|
||||
},
|
||||
};
|
||||
|
|
|
|||
|
|
@ -266,10 +266,39 @@ 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. The level reflects telemetry severity, NOT a promise about the
|
||||
* caller: most callers catch the failure and degrade to a genuinely safe
|
||||
* fallback (a usable result, usually with a caller-visible `partial`/`ftsUsed`
|
||||
* flag), so these are not operation-level errors and must not log 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.
|
||||
*
|
||||
* Contract for callers (#2283 review): only route a failure here when the
|
||||
* caller ALSO surfaces the degradation in its result (a `partial` flag,
|
||||
* `failed_files`, `traversalComplete:false`, …). A mutating or safety-critical
|
||||
* path that would otherwise report success/clean (e.g. `rename` apply, the
|
||||
* `detect_changes` safety gate) MUST set that result-level signal — `warn`
|
||||
* alone is not a substitute for an honest result.
|
||||
*/
|
||||
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)');
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -282,7 +311,12 @@ function logQueryError(context: string, err: unknown): void {
|
|||
*/
|
||||
function isBenignMissingTableError(err: unknown): boolean {
|
||||
const msg = err instanceof Error ? err.message : String(err ?? '');
|
||||
return /does not exist|no such (table|label|rel)|unknown (table|label)|not (defined|found)/i.test(
|
||||
// The `not (defined|found)` arm is scoped to a schema object (table/label/
|
||||
// rel/column/property), mirroring lbug-adapter's isMissingColumnError
|
||||
// (`/(table|column|property).*not found/i`): an unscoped "not found" matched
|
||||
// operation failures like `rg: not found` (ripgrep absent) or `Symbol not
|
||||
// found`, which this helper would then silently demote to `debug` (#2283).
|
||||
return /does not exist|no such (table|label|rel)|unknown (table|label)|(table|label|rel|column|property)[^\n]*\bnot (defined|found)\b/i.test(
|
||||
msg,
|
||||
);
|
||||
}
|
||||
|
|
@ -1955,9 +1989,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 };
|
||||
}
|
||||
|
|
@ -3825,6 +3864,9 @@ export class LocalBackend {
|
|||
|
||||
// Map diff hunks to indexed symbols via range overlap
|
||||
const changedSymbols: any[] = [];
|
||||
// Set if a swallowed graph query fails below — surfaces `partial:true` so a
|
||||
// degraded run cannot report a false-clean `risk_level:'low'` (#2283).
|
||||
let queryDegraded = false;
|
||||
for (const fileDiff of fileDiffs) {
|
||||
if (fileDiff.hunks.length === 0) continue;
|
||||
|
||||
|
|
@ -3872,6 +3914,12 @@ export class LocalBackend {
|
|||
}
|
||||
} catch (e) {
|
||||
logQueryError('detect-changes:file-symbols', e);
|
||||
// The symbol query failed: changedSymbols stays empty and the result
|
||||
// would otherwise look like a clean no-op (`changed_count:0`,
|
||||
// `risk_level:'low'`). detect_changes is the pre-commit safety gate, so
|
||||
// flag the result `partial` rather than let a swallowed failure
|
||||
// masquerade as "nothing changed" (#2283).
|
||||
queryDegraded = true;
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -3910,6 +3958,7 @@ export class LocalBackend {
|
|||
}
|
||||
} catch (e) {
|
||||
logQueryError('detect-changes:process-lookup', e);
|
||||
queryDegraded = true;
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -3932,6 +3981,9 @@ export class LocalBackend {
|
|||
},
|
||||
changed_symbols: changedSymbols,
|
||||
affected_processes: Array.from(affectedProcesses.values()),
|
||||
// A swallowed query failure makes the counts/risk above incomplete — tell
|
||||
// the caller so the safety gate isn't trusted as a clean result (#2283).
|
||||
...(queryDegraded && { partial: true }),
|
||||
};
|
||||
}
|
||||
|
||||
|
|
@ -4132,6 +4184,7 @@ export class LocalBackend {
|
|||
const allChanges = Array.from(changes.values());
|
||||
const totalEdits = allChanges.reduce((sum, c) => sum + c.edits.length, 0);
|
||||
|
||||
const failedFiles: string[] = [];
|
||||
if (!dry_run) {
|
||||
// Apply edits to files
|
||||
for (const change of allChanges) {
|
||||
|
|
@ -4142,13 +4195,17 @@ export class LocalBackend {
|
|||
content = content.replace(regex, new_name);
|
||||
await fs.writeFile(fullPath, content, 'utf-8');
|
||||
} catch (e) {
|
||||
// A swallowed write failure must not be reported as a full success
|
||||
// (#2283): record the file so the result can degrade to 'partial'
|
||||
// with the unwritten files listed, rather than masquerading as done.
|
||||
logQueryError('rename:apply-edit', e);
|
||||
failedFiles.push(change.file_path);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return {
|
||||
status: 'success',
|
||||
status: failedFiles.length > 0 ? 'partial' : 'success',
|
||||
old_name: oldName,
|
||||
new_name,
|
||||
files_affected: allChanges.length,
|
||||
|
|
@ -4157,6 +4214,7 @@ export class LocalBackend {
|
|||
text_search_edits: astSearchEdits,
|
||||
changes: allChanges,
|
||||
applied: !dry_run,
|
||||
...(failedFiles.length > 0 && { failed_files: failedFiles }),
|
||||
};
|
||||
}
|
||||
|
||||
|
|
@ -4490,9 +4548,21 @@ export class LocalBackend {
|
|||
}
|
||||
const mode = modeResult.mode;
|
||||
|
||||
// #2279: some MCP client/agent adapters serialize an *omitted* optional
|
||||
// numeric field as `0` rather than dropping it, so callgraph calls arrive
|
||||
// carrying a spurious `line: 0`. `line` is meaningless on the callgraph path
|
||||
// (the symbol→symbol BFS has no statement notion), so treat a literal `0`
|
||||
// there as omitted and let the normal traversal run. The coercion is
|
||||
// deliberately narrow — only the literal `0`, only when mode !== 'pdg':
|
||||
// a genuine positive `line` on callgraph still errors (real mode mistake),
|
||||
// negative/fractional values still error, and pdg mode is untouched (the
|
||||
// normalization is an identity there, so `line: 0` is still rejected below —
|
||||
// there is no 1-based source line `0` to anchor on).
|
||||
const effectiveLine = mode !== 'pdg' && params.line === 0 ? undefined : params.line;
|
||||
|
||||
// `line` is a PDG-only statement anchor. Reject it on the callgraph path
|
||||
// rather than silently ignore (the symbol→symbol BFS has no statement notion).
|
||||
if (params.line !== undefined && mode !== 'pdg') {
|
||||
if (effectiveLine !== undefined && mode !== 'pdg') {
|
||||
return {
|
||||
error: `Parameter 'line' is only supported with mode:'pdg' (it anchors the dependence slice on a statement). Remove it or set mode:'pdg'.`,
|
||||
target: { name: params.target },
|
||||
|
|
@ -4503,8 +4573,8 @@ export class LocalBackend {
|
|||
}
|
||||
// A provided `line` must be a positive integer.
|
||||
if (
|
||||
params.line !== undefined &&
|
||||
(!Number.isInteger(params.line) || (params.line as number) < 1)
|
||||
effectiveLine !== undefined &&
|
||||
(!Number.isInteger(effectiveLine) || (effectiveLine as number) < 1)
|
||||
) {
|
||||
// Line param fails validation before target resolution → partial-but-typed
|
||||
// target on the pdg path (typed PdgImpactTarget, not an inline literal).
|
||||
|
|
@ -4840,7 +4910,11 @@ export class LocalBackend {
|
|||
symType,
|
||||
direction,
|
||||
maxDepth,
|
||||
line: params.line,
|
||||
// Use the normalized line, not raw params.line, so the gate and the
|
||||
// engine share one source of truth (#2283). Identity in pdg mode today
|
||||
// — effectiveLine === params.line when mode === 'pdg' — but this stays
|
||||
// correct if the normalization ever stops being an identity here.
|
||||
line: effectiveLine,
|
||||
limit: Number.isFinite(params.limit) ? params.limit : 100,
|
||||
// KTD2 extraction-seam discipline: hand the engine its DB dependency
|
||||
// explicitly rather than `this.`-binding it. LocalBackend owns repo
|
||||
|
|
|
|||
|
|
@ -469,9 +469,14 @@ SERVICE: optional monorepo path prefix (case-sensitive path segments). When "rep
|
|||
},
|
||||
line: {
|
||||
type: 'integer',
|
||||
minimum: 1,
|
||||
// `minimum: 0` (not 1) so strict client/agent adapters that materialize
|
||||
// an omitted optional numeric field as `0` do not reject the request
|
||||
// before sending (#2279). A positive line is still required for a real
|
||||
// pdg anchor — the backend enforces that — but `0`/omitted means "no
|
||||
// statement anchor" and is tolerated on the callgraph path.
|
||||
minimum: 0,
|
||||
description:
|
||||
"1-based source line — PDG statement anchor (mode:'pdg'). Seeds affectedStatements on the statement at this line; inter-procedural symbols are still returned in interproceduralByDepth/pdgInterprocedural and the compatibility byDepth bucket.",
|
||||
"1-based source line — PDG statement anchor (mode:'pdg'). Seeds affectedStatements on the statement at this line; inter-procedural symbols are still returned in interproceduralByDepth/pdgInterprocedural and the compatibility byDepth bucket. Omit line for whole-symbol pdg (whole-symbol reach + diagnostics); 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'.",
|
||||
},
|
||||
file_path: {
|
||||
type: 'string',
|
||||
|
|
|
|||
|
|
@ -9,6 +9,7 @@
|
|||
*/
|
||||
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
||||
import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'fs';
|
||||
import fsPromises from 'fs/promises';
|
||||
import os from 'os';
|
||||
import path from 'path';
|
||||
|
||||
|
|
@ -395,12 +396,23 @@ describe('LocalBackend.callTool', () => {
|
|||
vi.mocked(searchFTSFromLbug).mockRejectedValueOnce(new Error('bm25Results is not iterable'));
|
||||
(executeParameterized as any).mockResolvedValue([]);
|
||||
|
||||
const result = await backend.callTool('query', { query: 'auth' });
|
||||
const cap = _captureLogger();
|
||||
try {
|
||||
const result = await backend.callTool('query', { query: 'auth' });
|
||||
|
||||
// Should still return a valid result shape (semantic-only fallback)
|
||||
expect(result).toHaveProperty('processes');
|
||||
expect(result).toHaveProperty('definitions');
|
||||
expect(result).not.toHaveProperty('error');
|
||||
// Should still return a valid result shape (semantic-only fallback)
|
||||
expect(result).toHaveProperty('processes');
|
||||
expect(result).toHaveProperty('definitions');
|
||||
expect(result).not.toHaveProperty('error');
|
||||
// The FTS fallback is a gracefully-degraded result, not an operation failure:
|
||||
// it must log at warn (40), never error (50), matching its sibling
|
||||
// import-failure fallback. Pins the severity against regression.
|
||||
const fts = cap.records().find((r) => /BM25\/FTS search failed/.test(String(r.msg ?? '')));
|
||||
expect(fts).toBeDefined();
|
||||
expect(fts?.level).toBe(40);
|
||||
} finally {
|
||||
cap.restore();
|
||||
}
|
||||
});
|
||||
|
||||
it('skips vector index query when VECTOR is unsupported by the platform', async () => {
|
||||
|
|
@ -1288,6 +1300,46 @@ describe('LocalBackend.callTool', () => {
|
|||
expect(result.error).toContain('Either symbol_name or symbol_uid');
|
||||
});
|
||||
|
||||
it('rename: a swallowed apply-edit write failure degrades to status:partial + failed_files (#2283)', async () => {
|
||||
// Resolve the definition, no graph refs. readFile succeeds (so a def edit is
|
||||
// recorded), but writeFile fails on apply — the failure is swallowed via
|
||||
// logQueryError. The result must NOT report a clean success: it degrades to
|
||||
// 'partial' and lists the unwritten file, instead of status:'success'.
|
||||
(executeParameterized as any)
|
||||
.mockResolvedValueOnce([
|
||||
{
|
||||
id: 'func:oldName',
|
||||
name: 'oldName',
|
||||
type: 'Function',
|
||||
filePath: 'src/target.ts',
|
||||
startLine: 1,
|
||||
endLine: 5,
|
||||
},
|
||||
])
|
||||
.mockResolvedValue([]);
|
||||
const readSpy = vi
|
||||
.spyOn(fsPromises, 'readFile')
|
||||
.mockResolvedValue('function oldName() {}\n' as unknown as Buffer);
|
||||
const writeSpy = vi
|
||||
.spyOn(fsPromises, 'writeFile')
|
||||
.mockRejectedValue(new Error('EACCES: permission denied'));
|
||||
try {
|
||||
const result = await backend.callTool('rename', {
|
||||
symbol_name: 'oldName',
|
||||
new_name: 'newName',
|
||||
dry_run: false,
|
||||
});
|
||||
expect(result.status).toBe('partial');
|
||||
expect(result.failed_files).toContain('src/target.ts');
|
||||
// It DID attempt to apply (not a dry run) — `applied` stays true; the
|
||||
// honest signal is the 'partial' status + failed_files, not `applied`.
|
||||
expect(result.applied).toBe(true);
|
||||
} finally {
|
||||
readSpy.mockRestore();
|
||||
writeSpy.mockRestore();
|
||||
}
|
||||
});
|
||||
|
||||
// api_impact tool
|
||||
it('dispatches api_impact tool with route param', async () => {
|
||||
(executeParameterized as any).mockResolvedValue([
|
||||
|
|
@ -1642,6 +1694,67 @@ describe('LocalBackend impact mode (KTD1/KTD5/KTD12)', () => {
|
|||
},
|
||||
);
|
||||
|
||||
// #2279: some MCP client/agent adapters serialize an *omitted* optional
|
||||
// numeric field as `0`. On the callgraph path `line` is meaningless, so a
|
||||
// literal `line: 0` must be tolerated as omitted (NOT the PDG-only error) and
|
||||
// route to the normal BFS — distinct from a genuine positive `line` (above),
|
||||
// which stays a hard error.
|
||||
it.each<['callgraph' | undefined]>([['callgraph'], [undefined]])(
|
||||
'mode:%j + adapter-materialized line:0 is treated as omitted and runs the BFS (#2279)',
|
||||
async (mode) => {
|
||||
resolveSingleTarget();
|
||||
const bfsSpy = vi.spyOn(backend as any, '_runImpactBFS');
|
||||
const result = await backend.callTool('impact', {
|
||||
target: 'main',
|
||||
direction: 'upstream',
|
||||
mode,
|
||||
line: 0,
|
||||
});
|
||||
// No PDG-only error, no positive-integer error — line:0 is swallowed.
|
||||
expect(result.error ?? '').not.toMatch(/'line' is only supported with mode:'pdg'/);
|
||||
expect(result.error ?? '').not.toMatch(/'line' must be a positive integer/);
|
||||
expect(result.target).toBeDefined();
|
||||
expect(bfsSpy).toHaveBeenCalledTimes(1);
|
||||
},
|
||||
);
|
||||
|
||||
it.each<['callgraph' | undefined]>([['callgraph'], [undefined]])(
|
||||
'mode:%j + line:-1 still errors — the line:0 coercion is narrow, only literal 0 (#2279)',
|
||||
async (mode) => {
|
||||
resolveSingleTarget();
|
||||
const result = await backend.callTool('impact', {
|
||||
target: 'main',
|
||||
direction: 'upstream',
|
||||
mode,
|
||||
line: -1,
|
||||
});
|
||||
// A negative line is a real mistake, not an adapter-materialized "omitted":
|
||||
// it must NOT be swallowed like line:0, and stays the PDG-only hard error.
|
||||
expect(result.error).toMatch(/'line' is only supported with mode:'pdg'/);
|
||||
},
|
||||
);
|
||||
|
||||
it("mode:'callgraph'/undefined + line:0 is byte-identical to omitting line (#2279)", async () => {
|
||||
resolveSingleTarget();
|
||||
const omitted = await backend.callTool('impact', { target: 'main', direction: 'upstream' });
|
||||
const callgraphZero = await backend.callTool('impact', {
|
||||
target: 'main',
|
||||
direction: 'upstream',
|
||||
mode: 'callgraph',
|
||||
line: 0,
|
||||
});
|
||||
const undefZero = await backend.callTool('impact', {
|
||||
target: 'main',
|
||||
direction: 'upstream',
|
||||
mode: undefined,
|
||||
line: 0,
|
||||
});
|
||||
// The normalization must leave the callgraph result indistinguishable from a
|
||||
// call that never carried `line` — the spurious 0 must not leak into output.
|
||||
expect(callgraphZero).toEqual(omitted);
|
||||
expect(undefZero).toEqual(omitted);
|
||||
});
|
||||
|
||||
it.each([[0], [-1], [1.5]])(
|
||||
"mode:'pdg' + non-positive-integer line %j → structured {error}, never routed to traversal",
|
||||
async (badLine) => {
|
||||
|
|
@ -1821,15 +1934,121 @@ 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,
|
||||
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' slice-callees failing with a benign missing-table error logs at debug, not warn", async () => {
|
||||
// A repo analyzed without the optional column/table (e.g. a pre-v3 PDG index
|
||||
// missing `calleeIds`, or a BasicBlock table that simply isn't there) makes the
|
||||
// slice-callees query fail with a benign "missing optional data" error. That is a
|
||||
// normal configuration, not a degradation, so logQueryError routes it to debug —
|
||||
// suppressed at the default info level. We capture AT debug so the record is
|
||||
// visible: the assertion is that it was emitted AND at debug (level 10), which
|
||||
// distinguishes "logged at debug" from "not logged at all" — an info-level
|
||||
// absence check could not tell those apart and would pass vacuously if the
|
||||
// logQueryError call were deleted.
|
||||
resolveSingleTarget();
|
||||
vi.mocked(executeParameterized).mockImplementation(async (_repo, query) => {
|
||||
if (query.includes('RETURN b.callees')) throw new Error('Table BasicBlock does not exist');
|
||||
return [{ id: 'func:main', name: 'main', type: 'Function', filePath: 'src/index.ts' }];
|
||||
});
|
||||
// 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();
|
||||
vi.spyOn(backend as any, '_runImpactPDG').mockResolvedValueOnce({
|
||||
mode: 'pdg',
|
||||
target: { id: 'func:main', name: 'main', type: 'Function', filePath: 'src/index.ts' },
|
||||
direction: 'downstream',
|
||||
risk: 'UNKNOWN',
|
||||
impactedCount: 0,
|
||||
epistemic: 'pdg-intra-procedural',
|
||||
reachableBlocks: ['BasicBlock:src/index.ts:8:0:1'],
|
||||
intraReachableBlocks: ['BasicBlock:src/index.ts:8:0:1'],
|
||||
seedBlocks: ['BasicBlock:src/index.ts:8:0:0'],
|
||||
blockCount: 1,
|
||||
affectedStatements: [{ line: 8, filePath: 'src/index.ts', text: 'callee()' }],
|
||||
affectedStatementCount: 1,
|
||||
criterionLine: 8,
|
||||
});
|
||||
const bfsSpy = vi.spyOn(backend as any, '_runImpactBFS');
|
||||
const cap = _captureLogger('debug');
|
||||
try {
|
||||
const result = await backend.callTool('impact', {
|
||||
target: 'main',
|
||||
direction: 'downstream',
|
||||
mode: 'pdg',
|
||||
line: 8,
|
||||
});
|
||||
// Still degrades cleanly to no bridge / no surfaced error.
|
||||
expect(result.error).toBeUndefined();
|
||||
expect(bfsSpy.mock.calls[0][4].pdgBridge).toBeUndefined();
|
||||
// The benign failure was emitted at debug (20) — NOT warn (40)/error (50).
|
||||
// Capturing at debug proves the call fired and chose the suppressed level.
|
||||
const slice = cap.records().find((r) => r.context === 'impact:pdg-slice-callees');
|
||||
expect(slice).toBeDefined();
|
||||
expect(slice?.level).toBe(20);
|
||||
} finally {
|
||||
cap.restore();
|
||||
}
|
||||
});
|
||||
|
||||
it("mode:'pdg' slice-callees failing with a non-schema 'not found' error logs at warn, not debug (#2283)", async () => {
|
||||
// "Symbol not found" is an operation failure, not a benign missing optional
|
||||
// table — isBenignMissingTableError must NOT match an unscoped "not found"
|
||||
// (only "<table|column|property|…> … not found"), so it stays visible at warn
|
||||
// rather than being demoted to the suppressed debug level.
|
||||
resolveSingleTarget();
|
||||
vi.mocked(executeParameterized).mockImplementation(async (_repo, query) => {
|
||||
if (query.includes('RETURN b.callees')) throw new Error('Symbol not found');
|
||||
return [{ id: 'func:main', name: 'main', type: 'Function', filePath: 'src/index.ts' }];
|
||||
});
|
||||
vi.spyOn(backend as any, '_runImpactPDG').mockResolvedValueOnce({
|
||||
mode: 'pdg',
|
||||
target: { id: 'func:main', name: 'main', type: 'Function', filePath: 'src/index.ts' },
|
||||
direction: 'downstream',
|
||||
risk: 'UNKNOWN',
|
||||
impactedCount: 0,
|
||||
epistemic: 'pdg-intra-procedural',
|
||||
reachableBlocks: ['BasicBlock:src/index.ts:8:0:1'],
|
||||
intraReachableBlocks: ['BasicBlock:src/index.ts:8:0:1'],
|
||||
seedBlocks: ['BasicBlock:src/index.ts:8:0:0'],
|
||||
blockCount: 1,
|
||||
affectedStatements: [{ line: 8, filePath: 'src/index.ts', text: 'callee()' }],
|
||||
affectedStatementCount: 1,
|
||||
criterionLine: 8,
|
||||
});
|
||||
vi.spyOn(backend as any, '_runImpactBFS');
|
||||
const cap = _captureLogger();
|
||||
try {
|
||||
await backend.callTool('impact', {
|
||||
target: 'main',
|
||||
direction: 'downstream',
|
||||
mode: 'pdg',
|
||||
line: 8,
|
||||
});
|
||||
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 () => {
|
||||
|
|
|
|||
|
|
@ -134,17 +134,27 @@ describe('GITNEXUS_TOOLS', () => {
|
|||
expect(impactTool.inputSchema.required).toContain('direction');
|
||||
});
|
||||
|
||||
it('impact tool advertises the PDG-only `line` statement anchor (integer, min 1, not required)', () => {
|
||||
it('impact tool advertises the PDG-only `line` statement anchor (integer, min 0, not required)', () => {
|
||||
const impactTool = GITNEXUS_TOOLS.find((t) => t.name === 'impact')!;
|
||||
const line = (impactTool.inputSchema.properties as Record<string, any>).line;
|
||||
expect(line).toBeDefined();
|
||||
expect(line.type).toBe('integer');
|
||||
expect(line.minimum).toBe(1);
|
||||
// minimum is 0 (not 1) so strict adapters that materialize an omitted
|
||||
// optional numeric field as `0` are not rejected client-side (#2279); a
|
||||
// positive line is enforced backend-side for a real pdg anchor.
|
||||
expect(line.minimum).toBe(0);
|
||||
// Statement-anchored slice is optional — never required.
|
||||
expect(impactTool.inputSchema.required).not.toContain('line');
|
||||
// The description names the mode:'pdg' statement-anchor semantics.
|
||||
// The description names the mode:'pdg' statement-anchor semantics and the
|
||||
// literal-0 compatibility convention — without contradicting the top-level
|
||||
// "omit line for whole-symbol pdg" contract (#2283).
|
||||
expect(line.description).toMatch(/statement anchor/i);
|
||||
expect(line.description).toMatch(/pdg/i);
|
||||
expect(line.description).toMatch(/literal 0 is tolerated only .* on the callgraph path/i);
|
||||
expect(line.description).toMatch(/omit line for whole-symbol pdg/i);
|
||||
// Must NOT claim pdg "requires a positive line" — that contradicts the valid
|
||||
// no-line whole-symbol pdg call documented in the top-level description.
|
||||
expect(line.description).not.toMatch(/requires a positive line/i);
|
||||
// The top-level description mentions the statement-anchored slice and result shape.
|
||||
expect(impactTool.description).toMatch(/statement-anchored|STATEMENT-ANCHORED/);
|
||||
expect(impactTool.description).toContain('affectedStatements');
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue