mirror of
https://github.com/colbymchenry/codegraph.git
synced 2026-10-18 13:53:01 +08:00
fix(graph): don't lose dependents within the depth limit to edge order (#1974)
getImpactRadius, getCallers and getCallees are depth-limited DFS walks with one visited set. A node first reached through a longer path at the depth limit was marked visited without being expanded, so when a shorter path reached it later it was skipped and its own dependents within the limit were lost. Record the shallowest depth each node was expanded at and expand it again when a nearer path reaches it. Results and edges are still reported once, and output order is unchanged where nothing was missing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -162,6 +162,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
|
||||
|
||||
#### MCP / indexing
|
||||
|
||||
- `codegraph impact --depth N`, multi-level callers and callees, and the viewer's blast radius no longer miss dependents within N hops when a longer path reaches one of them first. (#1974)
|
||||
- Daemon startup and cleanup now preserve live legacy PID-only locks while still reclaiming dead or identity-disproved records, preventing two writers from serving the same project.
|
||||
- Incremental sync now keeps edge rebinding crash-safe: replacing a resolved edge with its recovery reference commits atomically, so an interruption cannot permanently remove the relationship.
|
||||
- Status now detects committed but unindexed changes and restored edits without scanning every source file; thanks @inth3shadows. (#1829)
|
||||
|
||||
@@ -612,4 +612,42 @@ describe('Traversal edge-completeness & limits (#1086–#1090)', () => {
|
||||
// The regression: this direct dependency edge used to vanish.
|
||||
expect(sub.edges.some((e) => e.source === 'Q' && e.target === 'P' && e.kind === 'calls')).toBe(true);
|
||||
});
|
||||
|
||||
// The issue's graph: a→t, b→a, b→t, c→b. Edge order sends the walk to b
|
||||
// through a first, at the depth limit, before the direct b→t edge (#1974).
|
||||
const depthNodes = ['t', 'a', 'b', 'c'].map((n) => tNode(n));
|
||||
const depthEdges: Edge[] = [
|
||||
{ source: 'a', target: 't', kind: 'calls', line: 1 },
|
||||
{ source: 'b', target: 'a', kind: 'calls', line: 2 },
|
||||
{ source: 'b', target: 't', kind: 'calls', line: 3 },
|
||||
{ source: 'c', target: 'b', kind: 'calls', line: 4 },
|
||||
];
|
||||
|
||||
it('getImpactRadius finds a dependent within the depth even when a longer path reaches its parent first (#1974)', () => {
|
||||
const sub = tGraph(depthNodes, depthEdges).getImpactRadius('t', 2);
|
||||
// c → b → t is two hops. Pre-fix: b was first reached via a at depth 2 and
|
||||
// never expanded again, so c was missing.
|
||||
expect([...sub.nodes.keys()].sort()).toEqual(['a', 'b', 'c', 't']);
|
||||
expect(sub.edges.some((e) => e.source === 'c' && e.target === 'b')).toBe(true);
|
||||
// Re-expanding b does not record its incoming edges twice.
|
||||
const keys = sub.edges.map((e) => `${e.source}>${e.target}:${e.line}`);
|
||||
expect(new Set(keys).size).toBe(keys.length);
|
||||
});
|
||||
|
||||
it('getCallers at depth N finds every caller within N hops, each once (#1974)', () => {
|
||||
const callers = tGraph(depthNodes, depthEdges).getCallers('t', 2);
|
||||
expect(callers.map((c) => c.node.id).sort()).toEqual(['a', 'b', 'c']);
|
||||
});
|
||||
|
||||
it('getCallees at depth N finds every callee within N hops, each once (#1974)', () => {
|
||||
// Mirror image: t→a, a→b, t→b, b→c. From t, b is 1 hop and c is 2.
|
||||
const edges: Edge[] = [
|
||||
{ source: 't', target: 'a', kind: 'calls', line: 1 },
|
||||
{ source: 'a', target: 'b', kind: 'calls', line: 2 },
|
||||
{ source: 't', target: 'b', kind: 'calls', line: 3 },
|
||||
{ source: 'b', target: 'c', kind: 'calls', line: 4 },
|
||||
];
|
||||
const callees = tGraph(depthNodes, edges).getCallees('t', 2);
|
||||
expect(callees.map((c) => c.node.id).sort()).toEqual(['a', 'b', 'c']);
|
||||
});
|
||||
});
|
||||
|
||||
+55
-33
@@ -251,6 +251,24 @@ export class GraphTraverser {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Depth-limited walks record the SHALLOWEST depth each node was expanded at.
|
||||
* A node first reached through a longer path at the depth limit is expanded
|
||||
* again when a shorter path reaches it, so its own dependents within the
|
||||
* limit are not lost to edge order (#1974). Returns false when the node was
|
||||
* already expanded at this depth or nearer.
|
||||
*/
|
||||
private enterAtDepth(visited: Map<string, number>, nodeId: string, depth: number): boolean {
|
||||
if (!this.nearerThanBefore(visited, nodeId, depth)) return false;
|
||||
visited.set(nodeId, depth);
|
||||
return true;
|
||||
}
|
||||
|
||||
private nearerThanBefore(visited: Map<string, number>, nodeId: string, depth: number): boolean {
|
||||
const seen = visited.get(nodeId);
|
||||
return seen === undefined || depth < seen;
|
||||
}
|
||||
|
||||
/**
|
||||
* Find all callers of a function/method
|
||||
*
|
||||
@@ -260,9 +278,9 @@ export class GraphTraverser {
|
||||
*/
|
||||
getCallers(nodeId: string, maxDepth: number = 1): Array<{ node: Node; edge: Edge }> {
|
||||
const result: Array<{ node: Node; edge: Edge }> = [];
|
||||
const visited = new Set<string>();
|
||||
const visited = new Map<string, number>();
|
||||
|
||||
this.getCallersRecursive(nodeId, maxDepth, 0, result, visited);
|
||||
this.getCallersRecursive(nodeId, maxDepth, 0, result, visited, new Set([nodeId]));
|
||||
|
||||
return result;
|
||||
}
|
||||
@@ -272,17 +290,15 @@ export class GraphTraverser {
|
||||
maxDepth: number,
|
||||
currentDepth: number,
|
||||
result: Array<{ node: Node; edge: Edge }>,
|
||||
visited: Set<string>
|
||||
visited: Map<string, number>,
|
||||
reported: Set<string>
|
||||
): void {
|
||||
// Mark visited BEFORE the depth check, not after. Folding both into one
|
||||
// guard meant that when `currentDepth >= maxDepth` fired we returned without
|
||||
// marking the node — so a caller reachable from the same parent via two
|
||||
// edges (two call sites, or calls + references) was pushed once per edge,
|
||||
// duplicating it in `result` at the default `maxDepth=1` (#1086).
|
||||
if (visited.has(nodeId)) {
|
||||
return;
|
||||
}
|
||||
visited.add(nodeId);
|
||||
if (!this.enterAtDepth(visited, nodeId, currentDepth)) return;
|
||||
if (currentDepth >= maxDepth) {
|
||||
return;
|
||||
}
|
||||
@@ -302,10 +318,12 @@ export class GraphTraverser {
|
||||
|
||||
for (const edge of incomingEdges) {
|
||||
const callerNode = callerNodes.get(edge.source);
|
||||
if (callerNode && !visited.has(callerNode.id)) {
|
||||
if (!callerNode || !this.nearerThanBefore(visited, callerNode.id, currentDepth + 1)) continue;
|
||||
if (!reported.has(callerNode.id)) {
|
||||
reported.add(callerNode.id);
|
||||
result.push({ node: callerNode, edge });
|
||||
this.getCallersRecursive(callerNode.id, maxDepth, currentDepth + 1, result, visited);
|
||||
}
|
||||
this.getCallersRecursive(callerNode.id, maxDepth, currentDepth + 1, result, visited, reported);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -318,9 +336,9 @@ export class GraphTraverser {
|
||||
*/
|
||||
getCallees(nodeId: string, maxDepth: number = 1): Array<{ node: Node; edge: Edge }> {
|
||||
const result: Array<{ node: Node; edge: Edge }> = [];
|
||||
const visited = new Set<string>();
|
||||
const visited = new Map<string, number>();
|
||||
|
||||
this.getCalleesRecursive(nodeId, maxDepth, 0, result, visited);
|
||||
this.getCalleesRecursive(nodeId, maxDepth, 0, result, visited, new Set([nodeId]));
|
||||
|
||||
return result;
|
||||
}
|
||||
@@ -330,15 +348,13 @@ export class GraphTraverser {
|
||||
maxDepth: number,
|
||||
currentDepth: number,
|
||||
result: Array<{ node: Node; edge: Edge }>,
|
||||
visited: Set<string>
|
||||
visited: Map<string, number>,
|
||||
reported: Set<string>
|
||||
): void {
|
||||
// Mark visited before the depth check — see getCallersRecursive: the merged
|
||||
// guard dropped the `visited.add` at the depth boundary, duplicating a
|
||||
// callee reached from the same node via two edges at `maxDepth=1` (#1086).
|
||||
if (visited.has(nodeId)) {
|
||||
return;
|
||||
}
|
||||
visited.add(nodeId);
|
||||
if (!this.enterAtDepth(visited, nodeId, currentDepth)) return;
|
||||
if (currentDepth >= maxDepth) {
|
||||
return;
|
||||
}
|
||||
@@ -356,10 +372,12 @@ export class GraphTraverser {
|
||||
|
||||
for (const edge of outgoingEdges) {
|
||||
const calleeNode = calleeNodes.get(edge.target);
|
||||
if (calleeNode && !visited.has(calleeNode.id)) {
|
||||
if (!calleeNode || !this.nearerThanBefore(visited, calleeNode.id, currentDepth + 1)) continue;
|
||||
if (!reported.has(calleeNode.id)) {
|
||||
reported.add(calleeNode.id);
|
||||
result.push({ node: calleeNode, edge });
|
||||
this.getCalleesRecursive(calleeNode.id, maxDepth, currentDepth + 1, result, visited);
|
||||
}
|
||||
this.getCalleesRecursive(calleeNode.id, maxDepth, currentDepth + 1, result, visited, reported);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -525,13 +543,13 @@ export class GraphTraverser {
|
||||
|
||||
const nodes = new Map<string, Node>();
|
||||
const edges: Edge[] = [];
|
||||
const visited = new Set<string>();
|
||||
const visited = new Map<string, number>();
|
||||
|
||||
// Add focal node
|
||||
nodes.set(focalNode.id, focalNode);
|
||||
|
||||
// Traverse incoming edges to find all dependents
|
||||
this.getImpactRecursive(nodeId, maxDepth, 0, nodes, edges, visited);
|
||||
this.getImpactRecursive(nodeId, maxDepth, 0, nodes, edges, visited, new Set());
|
||||
|
||||
return {
|
||||
nodes,
|
||||
@@ -546,19 +564,21 @@ export class GraphTraverser {
|
||||
currentDepth: number,
|
||||
nodes: Map<string, Node>,
|
||||
edges: Edge[],
|
||||
visited: Set<string>
|
||||
visited: Map<string, number>,
|
||||
expanded: Set<string>
|
||||
): void {
|
||||
// Mark visited before the depth check so a node collected at the depth
|
||||
// boundary still lands in `visited`. Otherwise it could sit in `nodes` but
|
||||
// not `visited`, and the two loops below — which used different sets to
|
||||
// gate re-processing — would disagree about it (#1089).
|
||||
if (visited.has(nodeId)) {
|
||||
return;
|
||||
}
|
||||
visited.add(nodeId);
|
||||
if (!this.enterAtDepth(visited, nodeId, currentDepth)) return;
|
||||
if (currentDepth >= maxDepth) {
|
||||
return;
|
||||
}
|
||||
// A node re-expanded from a nearer depth re-reads the same edges; record
|
||||
// them on its first expansion only.
|
||||
const firstExpansion = !expanded.has(nodeId);
|
||||
expanded.add(nodeId);
|
||||
|
||||
// For container nodes (classes, interfaces, structs, etc.), also traverse
|
||||
// into their children so that callers of contained methods appear in impact
|
||||
@@ -571,11 +591,13 @@ export class GraphTraverser {
|
||||
const children = this.queries.getNodesByIds(containsEdges.map((e) => e.target));
|
||||
for (const edge of containsEdges) {
|
||||
const childNode = children.get(edge.target);
|
||||
if (childNode && !visited.has(childNode.id)) {
|
||||
nodes.set(childNode.id, childNode);
|
||||
edges.push(edge);
|
||||
if (childNode && this.nearerThanBefore(visited, childNode.id, currentDepth)) {
|
||||
if (!nodes.has(childNode.id)) {
|
||||
nodes.set(childNode.id, childNode);
|
||||
edges.push(edge);
|
||||
}
|
||||
// Recurse into children at the same depth (they're part of the same symbol)
|
||||
this.getImpactRecursive(childNode.id, maxDepth, currentDepth, nodes, edges, visited);
|
||||
this.getImpactRecursive(childNode.id, maxDepth, currentDepth, nodes, edges, visited, expanded);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -597,11 +619,11 @@ export class GraphTraverser {
|
||||
// edge collection (`!nodes.has(...)`), so a second incoming edge into a
|
||||
// node already collected via another path was silently dropped from
|
||||
// `edges` even though it's a real dependency (#1089). Each node's incoming
|
||||
// edges are fetched once (nodes are expanded once), so no edge repeats.
|
||||
edges.push(edge);
|
||||
if (!visited.has(sourceNode.id)) {
|
||||
// edges are recorded on its first expansion only, so no edge repeats.
|
||||
if (firstExpansion) edges.push(edge);
|
||||
if (this.nearerThanBefore(visited, sourceNode.id, currentDepth + 1)) {
|
||||
nodes.set(sourceNode.id, sourceNode);
|
||||
this.getImpactRecursive(sourceNode.id, maxDepth, currentDepth + 1, nodes, edges, visited);
|
||||
this.getImpactRecursive(sourceNode.id, maxDepth, currentDepth + 1, nodes, edges, visited, expanded);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user