mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-08 22:22:52 +00:00
* fix(impact): resolve repo-relative file paths via filePath (fixes #3074) - resolve repo-relative paths like supabase/functions/_shared/crypto.ts via n.filePath exact + anchored ENDS WITH suffix, not just n.id/n.name - return impactedCount:null on not_found so miss cannot be read as 0/UNKNOWN safe - relax parenthesised OR-clause test to allow extra filePath terms * fix(impact): scope filePath match to File nodes (review #3084 P1) * fix(impact): make file path resolution parseable and safe * docs(pdg): align result contract fixtures with v3 * test(impact): add exact path precedence and not_found contract assertions
This commit is contained in:
Chareonwit Kunna
GitHub
parent
38a0837e4b
commit
7c723ce794
7 changed files with 113 additions and 19 deletions
|
|
@ -3818,7 +3818,22 @@ export class LocalBackend {
|
|||
} else if (isQualified) {
|
||||
// Parenthesised because the kind filter below is appended with AND, which
|
||||
// binds tighter than OR.
|
||||
whereClause = `WHERE (n.id = $symName OR n.name = $symName)`;
|
||||
// #3074: a repo-relative file path (e.g. "supabase/functions/_shared/crypto.ts")
|
||||
// is the most natural way to name a File and is exactly what `target.filePath`
|
||||
// reports, but the old clause only matched `n.id` (= "File:<path>") or basename
|
||||
// `n.name`, so the same path the graph stores never resolved. Also match the
|
||||
// repo-relative `n.filePath` exactly and via an anchored suffix (segment-boundary
|
||||
// "ENDS WITH $suffix" where suffix is "/"+path) so "a.ts" does not spuriously
|
||||
// match "mylib/a.ts" — same anchoring used in detect_changes (#2915).
|
||||
const suffix = pathSuffixOf(name);
|
||||
// File-path terms must be scoped to File nodes — n.filePath is shared by
|
||||
// every symbol in the file, so an unlabeled predicate would turn
|
||||
// "src/actions.ts" into every symbol in that file (bot review #3084 P1).
|
||||
// LadybugDB does not allow label tests in WHERE (n:File), so scope via
|
||||
// id prefix — File nodes are `File:<path>`.
|
||||
whereClause = `WHERE (n.id = $symName OR n.name = $symName OR (n.id STARTS WITH $filePrefix AND (n.filePath = $symName OR n.filePath ENDS WITH $suffix)))`;
|
||||
queryParams.suffix = suffix;
|
||||
queryParams.filePrefix = 'File:';
|
||||
} else {
|
||||
whereClause = `WHERE n.name = $symName`;
|
||||
}
|
||||
|
|
@ -3909,7 +3924,7 @@ export class LocalBackend {
|
|||
if (rows.length === 0) return { kind: 'not_found' };
|
||||
|
||||
// Normalise row shape across object / tuple returns from LadybugDB.
|
||||
const normalized = rows.map((r: any) => ({
|
||||
let normalized = rows.map((r: any) => ({
|
||||
id: (r.id ?? r[0]) as string,
|
||||
name: (r.name ?? r[1]) as string,
|
||||
type: (r.type ?? r[2] ?? '') as string,
|
||||
|
|
@ -3919,6 +3934,16 @@ export class LocalBackend {
|
|||
...(include_content ? { content: (r.content ?? r[6]) as string | undefined } : {}),
|
||||
}));
|
||||
|
||||
// An exact File path wins over anchored suffix candidates. Without this,
|
||||
// `lib/a.ts` and `src/lib/a.ts` both score as File candidates and turn an
|
||||
// otherwise unambiguous exact target into `ambiguous` (#3084 review P2).
|
||||
if (isQualified) {
|
||||
const exactFiles = normalized.filter(
|
||||
(candidate) => candidate.id.startsWith('File:') && candidate.filePath === name,
|
||||
);
|
||||
if (exactFiles.length > 0) normalized = exactFiles;
|
||||
}
|
||||
|
||||
// The COUNT can never legitimately be below the page it accompanies, so a
|
||||
// value under `normalized.length` means the count leg failed or returned an
|
||||
// unreadable shape. Keep the window size as the floor — reporting zero would
|
||||
|
|
@ -6127,6 +6152,7 @@ export class LocalBackend {
|
|||
direction: params.direction,
|
||||
suggestion,
|
||||
recoverySuggestion,
|
||||
undetermined: true,
|
||||
});
|
||||
return pdgErr;
|
||||
}
|
||||
|
|
@ -6134,7 +6160,7 @@ export class LocalBackend {
|
|||
error: message,
|
||||
target: { name: params.target },
|
||||
direction: params.direction,
|
||||
impactedCount: 0,
|
||||
impactedCount: null,
|
||||
risk: 'UNKNOWN',
|
||||
suggestion,
|
||||
...(recoverySuggestion ? { recoverySuggestion } : {}),
|
||||
|
|
@ -6229,6 +6255,7 @@ export class LocalBackend {
|
|||
`(single-repo PDG impact). Remove them or use mode:'callgraph' for cross-repo fan-out.`,
|
||||
target: crossDepthTarget,
|
||||
direction,
|
||||
undetermined: true,
|
||||
});
|
||||
return pdgErr;
|
||||
}
|
||||
|
|
@ -6295,12 +6322,17 @@ export class LocalBackend {
|
|||
error: `Target '${missing}' not found`,
|
||||
target: notFoundTarget,
|
||||
direction,
|
||||
undetermined: true,
|
||||
})
|
||||
: {
|
||||
error: `Target '${missing}' not found`,
|
||||
target: { name: target },
|
||||
direction,
|
||||
impactedCount: 0,
|
||||
// #3074 follow-up: do not ship a normal-shaped 0/UNKNOWN blast radius
|
||||
// alongside the error — it reads as a real "nothing depends on this"
|
||||
// answer. Null marks UNDETERMINED (same as the ambiguous path) so a
|
||||
// consumer testing `impactedCount === 0` cannot misread a miss as safe.
|
||||
impactedCount: null,
|
||||
risk: 'UNKNOWN',
|
||||
};
|
||||
}
|
||||
|
|
|
|||
|
|
@ -118,8 +118,10 @@ export function splitCalleeIds(raw: unknown): string[] {
|
|||
* Bump on any breaking change to the PDG result fields.
|
||||
* v2: `startLine` in the result is now 1-based display (#2380), matching the
|
||||
* context/query/impact tools (was 0-based).
|
||||
* v3: error envelopes use `impactedCount: null` when a target cannot be resolved
|
||||
* (#3074), so consumers cannot read a miss as a measured zero.
|
||||
*/
|
||||
export const PDG_RESULT_VERSION = 2 as const;
|
||||
export const PDG_RESULT_VERSION = 3 as const;
|
||||
|
||||
/** A reachable dependence block resolved to its source statement. */
|
||||
export interface PdgStatement {
|
||||
|
|
@ -742,7 +744,7 @@ export interface PdgInterproceduralImpact {
|
|||
export interface PdgImpactBaseResult extends PdgImpactParityFields {
|
||||
mode: 'pdg';
|
||||
/** Contract version of the mode:'pdg' impact result shape; bump on any breaking change to the PDG result fields. */
|
||||
pdgResultVersion: 2;
|
||||
pdgResultVersion: 3;
|
||||
target: PdgImpactTarget;
|
||||
direction: 'upstream' | 'downstream';
|
||||
impactedCount: number;
|
||||
|
|
@ -815,11 +817,11 @@ export interface PdgImpactDegradedResult extends PdgImpactBaseResult {
|
|||
export interface PdgImpactErrorResult {
|
||||
mode?: 'pdg';
|
||||
/** Contract version of the mode:'pdg' impact result shape; bump on any breaking change to the PDG result fields. */
|
||||
pdgResultVersion: 2;
|
||||
pdgResultVersion: 3;
|
||||
error: string;
|
||||
target: PdgImpactTarget;
|
||||
direction: 'upstream' | 'downstream';
|
||||
impactedCount: 0;
|
||||
impactedCount: number | null;
|
||||
risk: 'UNKNOWN';
|
||||
suggestion?: string;
|
||||
recoverySuggestion?: string;
|
||||
|
|
@ -838,6 +840,8 @@ export function makePdgImpactErrorResult(input: {
|
|||
mode?: 'pdg';
|
||||
suggestion?: string;
|
||||
recoverySuggestion?: string;
|
||||
/** True when analysis did not obtain a measured impact count. */
|
||||
undetermined?: boolean;
|
||||
}): PdgImpactErrorResult {
|
||||
return {
|
||||
...(input.mode ? { mode: input.mode } : {}),
|
||||
|
|
@ -845,7 +849,10 @@ export function makePdgImpactErrorResult(input: {
|
|||
error: input.error,
|
||||
target: input.target,
|
||||
direction: input.direction,
|
||||
impactedCount: 0,
|
||||
// #3074 follow-up + P2 review: an unmeasured PDG result must not ship a
|
||||
// confident-looking 0 blast radius. null marks UNDETERMINED, so a consumer
|
||||
// testing === 0 cannot misread a miss or failed query as safe.
|
||||
impactedCount: input.undetermined ? null : 0,
|
||||
risk: 'UNKNOWN',
|
||||
...(input.suggestion ? { suggestion: input.suggestion } : {}),
|
||||
...(input.recoverySuggestion ? { recoverySuggestion: input.recoverySuggestion } : {}),
|
||||
|
|
|
|||
|
|
@ -469,7 +469,7 @@ MODE (opt-in): "callgraph" (default) walks symbol→symbol edges (CALLS/IMPORTS/
|
|||
|
||||
STATEMENT-ANCHORED PDG SLICE: with mode:'pdg', pass "line" (1-based source line within the target symbol) to seed the dependence slice on the statement at that line and return what depends on it in affectedStatements (line + text). Inter-procedural symbols are still reported through interproceduralByDepth/pdgInterprocedural and the compatibility byDepth bucket. Without "line", pdg returns whole-symbol inter-procedural reach plus local whole-symbol PDG diagnostics.
|
||||
|
||||
PDG OUTPUT CONTRACT: every mode:'pdg' result (success, empty, degraded, or error) carries pdgResultVersion:2 — a stable discriminator for external consumers that bumps on any breaking change to the PDG result shape (distinct from the DB schema version). Successful PDG results include mode:'pdg', a full target envelope (id/name/type/filePath), affectedStatements, affectedStatementCount, interproceduralByDepth/pdgInterprocedural for cross-function reach, compatibility byDepth/byDepthCounts, risk:'UNKNOWN', and a note describing the unified contract. Degraded PDG results (no-layer, sub-layer-missing, unknown) keep mode:'pdg', pdgResultVersion:2, target metadata when the target resolves, risk:'UNKNOWN', note/remediation, and empty byDepth parity fields — never a false-safe zero. If depth and limit both bound the slice, truncatedByReasons reports both causes while truncatedBy remains scalar. Return-value-ascent coverage is published structurally at pdgEvidence.ascent — present iff the inter-procedural descent ran, including on an empty slice — with referencesScanned (DISTINCT callees scanned for a CALL_SUMMARY: a distinct-id tally, not a call-site count — two call sites to the same callee count once), returnFlowFound (whether the ascent fired anywhere in the slice), undecodableSummaryCount, examinedComplete (whether that scan covered every callee the index recorded a resolved id for on the visited blocks), incompleteReasons ('traversal-truncated' | 'callee-list-capped' | 'callee-ids-unrecorded'), and callSummaryLayerPresent. Read callSummaryLayerPresent FIRST: false ⇒ a pre-CALL_SUMMARY index, so {referencesScanned:N>0, returnFlowFound:false} is self-consistent and says nothing about the callees — the scan ran, but no layer existed in which a return-flow could be recorded (remedy: re-run gitnexus analyze --pdg). Branch on those fields; the note narrates the same facts in prose for humans and is not a stable contract.
|
||||
PDG OUTPUT CONTRACT: every mode:'pdg' result (success, empty, degraded, or error) carries pdgResultVersion:3 — a stable discriminator for external consumers that bumps on any breaking change to the PDG result shape (distinct from the DB schema version). Successful PDG results include mode:'pdg', a full target envelope (id/name/type/filePath), affectedStatements, affectedStatementCount, interproceduralByDepth/pdgInterprocedural for cross-function reach, compatibility byDepth/byDepthCounts, risk:'UNKNOWN', and a note describing the unified contract. Degraded PDG results (no-layer, sub-layer-missing, unknown) keep mode:'pdg', pdgResultVersion:3, target metadata when the target resolves, risk:'UNKNOWN', note/remediation, and empty byDepth parity fields — never a false-safe zero. If depth and limit both bound the slice, truncatedByReasons reports both causes while truncatedBy remains scalar. Return-value-ascent coverage is published structurally at pdgEvidence.ascent — present iff the inter-procedural descent ran, including on an empty slice — with referencesScanned (DISTINCT callees scanned for a CALL_SUMMARY: a distinct-id tally, not a call-site count — two call sites to the same callee count once), returnFlowFound (whether the ascent fired anywhere in the slice), undecodableSummaryCount, examinedComplete (whether that scan covered every callee the index recorded a resolved id for on the visited blocks), incompleteReasons ('traversal-truncated' | 'callee-list-capped' | 'callee-ids-unrecorded'), and callSummaryLayerPresent. Read callSummaryLayerPresent FIRST: false ⇒ a pre-CALL_SUMMARY index, so {referencesScanned:N>0, returnFlowFound:false} is self-consistent and says nothing about the callees — the scan ran, but no layer existed in which a return-flow could be recorded (remedy: re-run gitnexus analyze --pdg). Branch on those fields; the note narrates the same facts in prose for humans and is not a stable contract.
|
||||
|
||||
WHEN TO USE: Before making code changes — especially refactoring, renaming, or modifying shared code. Shows what would break.
|
||||
AFTER THIS: Review d=1 items (WILL BREAK). Use context() on high-risk symbols.
|
||||
|
|
|
|||
|
|
@ -17,7 +17,7 @@
|
|||
* "complete" result.
|
||||
*
|
||||
* This golden asserts the EXACT degraded envelope (not just non-crash):
|
||||
* - the result is still mode:'pdg' with pdgResultVersion:2 (the contract
|
||||
* - the result is still mode:'pdg' with pdgResultVersion:3 (the contract
|
||||
* discriminator);
|
||||
* - the intra slice is PRESENT (CALL_SUMMARY is NOT a required sub-layer — the
|
||||
* index is `ready`, pdgLayer is undefined, risk is UNKNOWN, epistemic is the
|
||||
|
|
@ -75,14 +75,14 @@ withTestLbugDB(
|
|||
});
|
||||
|
||||
describe('CALL_SUMMARY-absent (v3 / pre-FU-C index): the ascent is silent but the user is TOLD', () => {
|
||||
it('returns the EXACT degraded envelope — mode:pdg, pdgResultVersion:2, intra slice present, risk UNKNOWN', async () => {
|
||||
it('returns the EXACT degraded envelope — mode:pdg, pdgResultVersion:3, intra slice present, risk UNKNOWN', async () => {
|
||||
const result = await slice();
|
||||
// Golden envelope: the index is `ready` (CALL_SUMMARY is NOT a required
|
||||
// sub-layer), so this is a real traversal result — NOT a pdgLayer
|
||||
// degradation early-return. The intra slice ran and risk stays UNKNOWN.
|
||||
expect(result).toMatchObject({
|
||||
mode: 'pdg',
|
||||
pdgResultVersion: 2,
|
||||
pdgResultVersion: 3,
|
||||
risk: 'UNKNOWN',
|
||||
epistemic: 'pdg-intra-procedural',
|
||||
target: { id: 'func:fnA', name: 'fnA' },
|
||||
|
|
|
|||
|
|
@ -1344,12 +1344,67 @@ describe('LocalBackend.callTool', () => {
|
|||
await backend.callTool('context', { name: 'src/a.ts:collide', kind: 'Function' });
|
||||
|
||||
const parenthesised =
|
||||
/WHERE \(n\.id = \$symName OR n\.name = \$symName\) AND n\.id STARTS WITH \$kindPrefix/;
|
||||
/WHERE \(n\.id = \$symName OR n\.name = \$symName OR \(n\.id STARTS WITH \$filePrefix AND \(n\.filePath = \$symName OR n\.filePath ENDS WITH \$suffix\)\)\) AND n\.id STARTS WITH \$kindPrefix/;
|
||||
const calls = resolverCalls();
|
||||
expect(calls).toHaveLength(2);
|
||||
expect(calls.filter((c) => parenthesised.test(c.query))).toHaveLength(2);
|
||||
});
|
||||
|
||||
it('exact File path wins over suffixed matches during qualified resolution (#3084 review P2)', async () => {
|
||||
(executeParameterized as any).mockImplementation(async (_repo: string, query: string) => {
|
||||
if (query.startsWith('MATCH (n)')) {
|
||||
return [
|
||||
{
|
||||
id: 'File:src/lib/a.ts',
|
||||
name: 'a.ts',
|
||||
filePath: 'src/lib/a.ts',
|
||||
kind: 'File',
|
||||
total_hits: 1,
|
||||
},
|
||||
{
|
||||
id: 'File:lib/a.ts',
|
||||
name: 'a.ts',
|
||||
filePath: 'lib/a.ts',
|
||||
kind: 'File',
|
||||
total_hits: 1,
|
||||
},
|
||||
];
|
||||
}
|
||||
return [{ total: 2 }];
|
||||
});
|
||||
|
||||
const result = await backend.callTool('context', { name: 'lib/a.ts' });
|
||||
expect(result).toMatchObject({
|
||||
status: 'found',
|
||||
symbol: {
|
||||
filePath: 'lib/a.ts',
|
||||
uid: 'File:lib/a.ts',
|
||||
},
|
||||
});
|
||||
});
|
||||
|
||||
it('not_found impact queries return impactedCount null and risk UNKNOWN across modes (#3074 / #3084 review)', async () => {
|
||||
(executeParameterized as any).mockImplementation(async () => []);
|
||||
|
||||
const cgResult = await backend.callTool('impact', { target: 'nonexistent_target_xyz' });
|
||||
expect(cgResult).toMatchObject({
|
||||
error: "Target 'nonexistent_target_xyz' not found",
|
||||
impactedCount: null,
|
||||
risk: 'UNKNOWN',
|
||||
});
|
||||
|
||||
const pdgResult = await backend.callTool('impact', {
|
||||
target: 'nonexistent_target_xyz',
|
||||
mode: 'pdg',
|
||||
});
|
||||
expect(pdgResult).toMatchObject({
|
||||
error: "Target 'nonexistent_target_xyz' not found",
|
||||
impactedCount: null,
|
||||
risk: 'UNKNOWN',
|
||||
pdgResultVersion: 3,
|
||||
});
|
||||
});
|
||||
|
||||
it('retries UNFILTERED when the kind hint matches no label prefix (#2787 review F5)', async () => {
|
||||
// `kind` is a free-form string on the tool schema. A miscased or
|
||||
// repo-absent kind must not turn a real name into `not_found` — the
|
||||
|
|
@ -3241,7 +3296,7 @@ describe('LocalBackend impact mode (KTD1/KTD5/KTD12)', () => {
|
|||
expect(result.mode).toBe('pdg');
|
||||
expect(result.target).toEqual({ name: 'missingSymbol' });
|
||||
expect(result.direction).toBe('upstream');
|
||||
expect(result.impactedCount).toBe(0);
|
||||
expect(result.impactedCount).toBeNull();
|
||||
expect(result.risk).toBe('UNKNOWN');
|
||||
});
|
||||
|
||||
|
|
@ -3257,7 +3312,7 @@ describe('LocalBackend impact mode (KTD1/KTD5/KTD12)', () => {
|
|||
expect(result.mode).toBe('pdg');
|
||||
expect(result.target).toEqual({ name: 'main' });
|
||||
expect(result.direction).toBe('downstream');
|
||||
expect(result.impactedCount).toBe(0);
|
||||
expect(result.impactedCount).toBeNull();
|
||||
expect(result.risk).toBe('UNKNOWN');
|
||||
expect(result.suggestion).toMatch(/context/);
|
||||
implSpy.mockRestore();
|
||||
|
|
|
|||
|
|
@ -38,7 +38,7 @@ function pdgFindings(overrides: Record<string, unknown> = {}): Record<string, un
|
|||
];
|
||||
return {
|
||||
mode: 'pdg',
|
||||
pdgResultVersion: 2,
|
||||
pdgResultVersion: 3,
|
||||
target: {
|
||||
id: 'Function:src/svc.ts:computeTotal',
|
||||
name: 'computeTotal',
|
||||
|
|
@ -136,7 +136,7 @@ describe('formatImpactResult — PDG (mode:pdg) rendering', () => {
|
|||
// The PDG result family advertises a contract version (FIX #2) so external
|
||||
// MCP/agent consumers can version against future shape evolution. It is a
|
||||
// mode:'pdg'-only field — never on the default callgraph result.
|
||||
expect(pdgFindings()).toMatchObject({ mode: 'pdg', pdgResultVersion: 2 });
|
||||
expect(pdgFindings()).toMatchObject({ mode: 'pdg', pdgResultVersion: 3 });
|
||||
});
|
||||
|
||||
it('surfaces ambiguous-projection and unresolved block counts honestly', () => {
|
||||
|
|
|
|||
|
|
@ -36,7 +36,7 @@ const local = (
|
|||
impactedCount: number,
|
||||
): PdgImpactSuccessResult => ({
|
||||
mode: 'pdg',
|
||||
pdgResultVersion: 2,
|
||||
pdgResultVersion: 3,
|
||||
target: { id: 'T', name: 'criterion', type: 'Function', filePath: 'src/a.ts' },
|
||||
direction: 'downstream',
|
||||
risk: 'UNKNOWN',
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue