Skip to content

Commit 61bcc71

Browse files
authored
fix(facts): cap traversal output, not the BFS frontier (#157)
traverseFrom had two exclusion paths. The node-kind filter queues the node it excludes so traversal continues through it; the maxNodes path did not, so the BFS frontier died roughly one ply past the cap. Every node reachable only through a capped-out node was silently missed, and NodesVisited / MaxDepthReached — computed over that truncated walk — were reported as properties of the graph. Queue the node when the cap is hit, matching the kind-filter contract. maxNodes now bounds the returned set only. Nodes already appended are unaffected, so the result stays the BFS-order prefix of an uncapped walk. This also repairs a fused statistic in the impact summary: the dependent count came from an uncapped reachableCount while the depth printed beside it came from the cut walk, so one sentence combined a complete count with an incomplete depth. The comment documenting the frontier cut compensated for the count only, and is corrected to match the new behaviour. Measured on a 1.9M-fact graph: worst-case traversal 6ms -> 22ms. Extraction output is unchanged — facts and insights are byte-identical across the change, and no explainer calls traverseFrom — so no cacheVersion bump and no golden regeneration. Tests pin that the walk is cap-independent and that the returned set stays the uncapped prefix. The branching fixture is load-bearing: a chain has one successor per node and cannot detect sibling reordering.
1 parent a13dbf6 commit 61bcc71

2 files changed

Lines changed: 140 additions & 4 deletions

File tree

internal/facts/graph.go

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -353,7 +353,16 @@ func (g *Graph) traverseFrom(starts []string, direction string, relKinds, nodeKi
353353
}
354354

355355
if len(result.Nodes) >= maxNodes {
356+
// Still traverse through this node but don't include it in
357+
// results — same contract as the node-kind filter above.
358+
// maxNodes bounds the returned set, not the walk: dropping the
359+
// node from the queue would hide everything reachable only
360+
// through it and make NodesVisited/MaxDepthReached describe the
361+
// truncated walk rather than the graph. Nodes already appended
362+
// are unaffected, so the returned set stays the BFS-order prefix
363+
// of an uncapped traversal.
356364
truncated = true
365+
queue = append(queue, queueItem{name: e.Target, depth: newDepth})
357366
continue
358367
}
359368

@@ -515,10 +524,11 @@ func (g *Graph) ImpactSet(target string, maxDepth, maxNodes int, includeForward
515524
seeds := g.impactSeeds(target)
516525
rev := g.traverseFrom(seeds, "reverse", nil, nil, maxDepth, maxNodes)
517526

518-
// The max_nodes cap stops the BFS frontier, so rev's node/visited counts do
519-
// not reflect the true dependent count. Compute it with a cheap count-only
520-
// pass (same seeds, same depth, no node cap) so the summary is accurate even
521-
// when the displayed set is truncated.
527+
// max_nodes caps rev.Nodes, so len(rev.Nodes) is not the dependent count.
528+
// (The cap bounds the returned set only — the BFS itself walks the full
529+
// reachable set, so rev.Stats.NodesVisited/MaxDepthReached do describe the
530+
// graph.) Count with a cheap count-only pass that excludes the seeds
531+
// themselves, so the summary is accurate even when the display is truncated.
522532
totalDependents := g.reachableCount(seeds, "reverse", maxDepth)
523533

524534
result := ImpactResult{

internal/facts/graph_test.go

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,132 @@ func TestTraverse_EdgesConsistentWhenTruncated(t *testing.T) {
166166
}
167167
}
168168

169+
// buildChainGraph creates a linear chain N00 -> N01 -> ... -> N20 (21 nodes,
170+
// 20 hops), which is exactly the traverseFrom maxDepth ceiling. A chain isolates
171+
// the BFS frontier: every node is reachable only through its predecessor, so any
172+
// node dropped from the queue silently hides the whole remaining tail.
173+
func buildChainGraph(n int) *Graph {
174+
s := NewStore()
175+
for i := 0; i < n; i++ {
176+
f := Fact{Kind: KindSymbol, Name: chainName(i), File: "chain.go", Line: i + 1}
177+
if i < n-1 {
178+
f.Relations = []Relation{{Kind: RelCalls, Target: chainName(i + 1)}}
179+
}
180+
s.Add(f)
181+
}
182+
s.BuildGraph()
183+
return s.Graph()
184+
}
185+
186+
func chainName(i int) string {
187+
return "N" + string(rune('0'+i/10)) + string(rune('0'+i%10))
188+
}
189+
190+
// TestTraverse_MaxNodesCapsOutputNotFrontier pins that maxNodes bounds what is
191+
// RETURNED, not how far the BFS walks. The node-kind filter path already queues
192+
// nodes it excludes from results ("Still traverse through this node but don't
193+
// include it in results"); the maxNodes path must do the same, or every node
194+
// reachable only through a capped-out node becomes invisible and the reported
195+
// stats describe a truncated walk rather than the graph.
196+
func TestTraverse_MaxNodesCapsOutputNotFrontier(t *testing.T) {
197+
g := buildChainGraph(21)
198+
199+
result := g.Traverse("N00", "forward", nil, nil, 20, 2)
200+
201+
if !result.Stats.Truncated {
202+
t.Fatal("expected Truncated with maxNodes=2")
203+
}
204+
if len(result.Nodes) != 2 {
205+
t.Errorf("len(Nodes) = %d, want 2 (maxNodes caps the returned set)", len(result.Nodes))
206+
}
207+
// The frontier must survive the cap: the whole 21-node chain is walked.
208+
if result.Stats.NodesVisited != 21 {
209+
t.Errorf("NodesVisited = %d, want 21 — the BFS frontier stopped at the cap instead of walking the chain", result.Stats.NodesVisited)
210+
}
211+
if result.Stats.MaxDepthReached != 20 {
212+
t.Errorf("MaxDepthReached = %d, want 20 — depth is reported from a truncated walk, understating the graph", result.Stats.MaxDepthReached)
213+
}
214+
}
215+
216+
// TestTraverse_MaxNodesReturnsSamePrefixAsUncapped pins the no-collateral-damage
217+
// property: raising the frontier past the cap must not change WHICH nodes are
218+
// returned. The capped result must stay the exact BFS-order prefix of the
219+
// uncapped one, so the cap remains a pure output bound.
220+
func TestTraverse_MaxNodesReturnsSamePrefixAsUncapped(t *testing.T) {
221+
g := buildChainGraph(21)
222+
223+
full := g.Traverse("N00", "forward", nil, nil, 20, 500)
224+
if len(full.Nodes) != 21 {
225+
t.Fatalf("uncapped traversal returned %d nodes, want 21", len(full.Nodes))
226+
}
227+
228+
for _, cap := range []int{1, 2, 5, 13} {
229+
capped := g.Traverse("N00", "forward", nil, nil, 20, cap)
230+
if len(capped.Nodes) != cap {
231+
t.Errorf("maxNodes=%d returned %d nodes, want %d", cap, len(capped.Nodes), cap)
232+
continue
233+
}
234+
want := nodeNames(full.Nodes[:cap])
235+
if got := nodeNames(capped.Nodes); !reflect.DeepEqual(got, want) {
236+
t.Errorf("maxNodes=%d returned %v, want the uncapped prefix %v", cap, got, want)
237+
}
238+
}
239+
}
240+
241+
// TestTraverse_MaxNodesPrefixWithBranching is the branching counterpart of
242+
// TestTraverse_MaxNodesReturnsSamePrefixAsUncapped. A chain cannot catch
243+
// sibling-ordering effects — every node has one successor — so it would pass
244+
// even if the cap changed WHICH of several equal-depth siblings is returned.
245+
// This topology gives depth 1 three siblings and depth 2 five, so any reordering
246+
// introduced by queueing capped-out nodes shows up as a prefix mismatch.
247+
func TestTraverse_MaxNodesPrefixWithBranching(t *testing.T) {
248+
s := NewStore()
249+
s.Add(
250+
Fact{Kind: KindSymbol, Name: "R", File: "r.go", Relations: []Relation{
251+
{Kind: RelCalls, Target: "A"},
252+
{Kind: RelCalls, Target: "B"},
253+
{Kind: RelCalls, Target: "C"},
254+
}},
255+
Fact{Kind: KindSymbol, Name: "A", File: "a.go", Relations: []Relation{
256+
{Kind: RelCalls, Target: "A1"},
257+
{Kind: RelCalls, Target: "A2"},
258+
}},
259+
Fact{Kind: KindSymbol, Name: "B", File: "b.go", Relations: []Relation{
260+
{Kind: RelCalls, Target: "B1"},
261+
}},
262+
Fact{Kind: KindSymbol, Name: "C", File: "c.go", Relations: []Relation{
263+
{Kind: RelCalls, Target: "C1"},
264+
{Kind: RelCalls, Target: "C2"},
265+
}},
266+
Fact{Kind: KindSymbol, Name: "A1", File: "a1.go"},
267+
Fact{Kind: KindSymbol, Name: "A2", File: "a2.go"},
268+
Fact{Kind: KindSymbol, Name: "B1", File: "b1.go"},
269+
Fact{Kind: KindSymbol, Name: "C1", File: "c1.go"},
270+
Fact{Kind: KindSymbol, Name: "C2", File: "c2.go"},
271+
)
272+
s.BuildGraph()
273+
g := s.Graph()
274+
275+
full := g.Traverse("R", "forward", nil, nil, 5, 500)
276+
if len(full.Nodes) != 9 {
277+
t.Fatalf("uncapped traversal returned %d nodes, want 9", len(full.Nodes))
278+
}
279+
280+
for cap := 1; cap <= 9; cap++ {
281+
capped := g.Traverse("R", "forward", nil, nil, 5, cap)
282+
if got, want := nodeNames(capped.Nodes), nodeNames(full.Nodes[:cap]); !reflect.DeepEqual(got, want) {
283+
t.Errorf("maxNodes=%d returned %v, want the uncapped prefix %v", cap, got, want)
284+
}
285+
// The walk itself must be cap-independent: every cap sees the whole graph.
286+
if capped.Stats.NodesVisited != 9 {
287+
t.Errorf("maxNodes=%d: NodesVisited = %d, want 9 (the frontier must survive the cap)", cap, capped.Stats.NodesVisited)
288+
}
289+
if capped.Stats.MaxDepthReached != full.Stats.MaxDepthReached {
290+
t.Errorf("maxNodes=%d: MaxDepthReached = %d, want %d", cap, capped.Stats.MaxDepthReached, full.Stats.MaxDepthReached)
291+
}
292+
}
293+
}
294+
169295
func TestTraverse_RelationKindFilter(t *testing.T) {
170296
g, _ := buildTestGraph()
171297

0 commit comments

Comments
 (0)