Skip to content

Commit aa97ec4

Browse files
committed
Improving find_path with vague names and traverse cross-repos dependants following
1 parent e21ba9f commit aa97ec4

5 files changed

Lines changed: 300 additions & 13 deletions

File tree

internal/facts/graph.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,23 @@ func (g *Graph) Traverse(start, direction string, relKinds, nodeKinds []string,
219219
return g.traverseFrom([]string{start}, direction, relKinds, nodeKinds, maxDepth, maxNodes)
220220
}
221221

222+
// TraverseFrom performs a BFS traversal seeded at every name in starts (see
223+
// Traverse for the parameters). Use with RollupSeeds so a reverse traversal of a
224+
// type also covers its methods and constructor — otherwise callers that reference
225+
// the type only through its methods are missed (the bug impact_analysis avoids).
226+
func (g *Graph) TraverseFrom(starts []string, direction string, relKinds, nodeKinds []string, maxDepth, maxNodes int) TraversalResult {
227+
return g.traverseFrom(starts, direction, relKinds, nodeKinds, maxDepth, maxNodes)
228+
}
229+
230+
// RollupSeeds returns name plus, when name is a type symbol
231+
// (struct/class/interface/type), its methods (via has_method edges) and its
232+
// constructor (New<Type> in the same package). For any other node it returns just
233+
// name. This is the seed set reverse traversal needs to find everything that
234+
// references the entity through any of its members.
235+
func (g *Graph) RollupSeeds(name string) []string {
236+
return g.impactSeeds(name)
237+
}
238+
222239
// traverseFrom is the multi-source BFS underlying Traverse. Every name in starts
223240
// is seeded at depth 0 (deduplicated), so a logical entity spread across several
224241
// fact nodes — e.g. a type plus its methods and constructor — can be traversed as

internal/facts/graph_test.go

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -910,6 +910,26 @@ func TestImpactSet_NonTypeUnchanged(t *testing.T) {
910910
}
911911
}
912912

913+
// TestTraverseFrom_ReverseTypeRollup documents the #6 fix: reverse traversal of a
914+
// bare type finds nothing (its reverse adjacency is empty — callers reference its
915+
// methods/constructor, not the type), but seeding with RollupSeeds surfaces the
916+
// cross-repo caller, matching impact_analysis.
917+
func TestTraverseFrom_ReverseTypeRollup(t *testing.T) {
918+
g, _ := buildCrossRepoTypeStore()
919+
920+
// Bare reverse traversal of the type: no callers reference the type directly.
921+
bare := g.Traverse("adapters.AuthHandler", "reverse", nil, nil, 3, 100)
922+
if got := nodeNames(bare.Nodes); contains(got, "pkg/auth.Setup") {
923+
t.Errorf("bare reverse unexpectedly found the caller; got %v", got)
924+
}
925+
926+
// Seeded with the type's methods + constructor: the cross-repo caller appears.
927+
seeded := g.TraverseFrom(g.RollupSeeds("adapters.AuthHandler"), "reverse", nil, nil, 3, 100)
928+
if got := nodeNames(seeded.Nodes); !contains(got, "pkg/auth.Setup") {
929+
t.Errorf("seeded reverse did not surface cross-repo caller pkg/auth.Setup; got %v", got)
930+
}
931+
}
932+
913933
// impactNodes flattens an ImpactResult's depth buckets into a node slice.
914934
func impactNodes(res ImpactResult) []TraversalNode {
915935
var out []TraversalNode

internal/server/scopedquery.go

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ func matchTier(name, term string) int {
136136
if n == term {
137137
return 2
138138
}
139-
if hasShortName(n, term) {
139+
if hasShortName(n, term) || isQualifiedSuffix(n, term) {
140140
return 1
141141
}
142142
return 0
@@ -153,6 +153,19 @@ func hasShortName(lowerName, term string) bool {
153153
return false
154154
}
155155

156+
// isQualifiedSuffix reports whether term is a suffix of lowerName that begins on a
157+
// package/path boundary, so a package-qualified term like "ticket.Repository"
158+
// matches ".../ticket.Repository" but not ".../contracts.Repository". This lets a
159+
// caller disambiguate an otherwise-common short name (e.g. "Repository") by
160+
// prefixing its package. lowerName must already be lowercased.
161+
func isQualifiedSuffix(lowerName, term string) bool {
162+
if len(term) >= len(lowerName) || !strings.HasSuffix(lowerName, term) {
163+
return false
164+
}
165+
b := lowerName[len(lowerName)-len(term)-1]
166+
return b == '.' || b == '/'
167+
}
168+
156169
// codeExtensions are source-file extensions stripped from a path basename when
157170
// deriving its short name, so a file fact resolves by its base name (e.g.
158171
// "golf/internal/http/auth_routes.go" → "auth_routes"). Restricted to a known set

internal/server/server.go

Lines changed: 153 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -577,9 +577,10 @@ func (s *Server) registerTools() {
577577
Name: "traverse",
578578
Description: "Walk the dependency/call graph from a starting node. " +
579579
"direction='forward' answers \"what does X depend on?\"; direction='reverse' answers \"what depends on X?\". " +
580-
"start= accepts substring match plus scoped prefixes (repo:, kind:, file:) to disambiguate; returns ranked candidates with confidence when ambiguous. " +
580+
"start= accepts substring match plus scoped prefixes (repo:, kind:, file:) and package-qualified names (e.g. 'domain/cart.CartService') to disambiguate; returns ranked candidates with confidence when ambiguous. " +
581581
"relation_kinds filter: imports, calls, declares, implements, depends_on, has_method. " +
582582
"Forward traversal from a struct/interface follows has_method edges to its methods (and then their calls). " +
583+
"Reverse traversal from a struct/interface automatically includes its methods and constructor as origins, so it surfaces callers (including cross-repo) that reference the type only through a method — matching impact_analysis. " +
583584
"Note: interface method calls cannot be statically bound to a concrete implementation, so such call edges may be absent or appear as unresolved nodes. " +
584585
"node_kinds filters output (not traversal itself): module, symbol, dependency, route, storage. " +
585586
"TOKEN COST — output_mode ladder: 'summary' (DEFAULT) aggregates counts by node/relation kind, internal/external split, and hottest modules (small, no node list); 'compact' lists nodes grouped by depth; 'full' returns the raw JSON node/edge graph and can be VERY large. " +
@@ -635,7 +636,16 @@ func (s *Server) registerTools() {
635636
return errorResult("direction must be 'forward' or 'reverse'"), nil, nil
636637
}
637638

638-
result := graph.Traverse(startName, direction, args.RelationKinds, args.NodeKinds, args.MaxDepth, args.MaxNodes)
639+
// Reverse traversal of a type must seed its methods + constructor (callers
640+
// reference those, not the bare type), matching impact_analysis — otherwise
641+
// cross-repo and same-repo dependents are missed. Forward already follows
642+
// has_method edges from the type, so it needs no rollup.
643+
var result facts.TraversalResult
644+
if direction == "reverse" {
645+
result = graph.TraverseFrom(graph.RollupSeeds(startName), direction, args.RelationKinds, args.NodeKinds, args.MaxDepth, args.MaxNodes)
646+
} else {
647+
result = graph.Traverse(startName, direction, args.RelationKinds, args.NodeKinds, args.MaxDepth, args.MaxNodes)
648+
}
639649

640650
resp := traverseResponse{Resolution: res, TraversalResult: result}
641651
if wantsFullOutput(mode) {
@@ -653,10 +663,13 @@ func (s *Server) registerTools() {
653663
Description: "Find the shortest path (BFS, by hop count) between two nodes in the architectural graph. " +
654664
"Answers \"how does X reach Y?\" or \"what is the call chain from A to B?\". " +
655665
"from= and to= use substring match with smart disambiguation, and accept scoped prefixes " +
656-
"(repo:, kind:, file:) to pin down an ambiguous name, e.g. from=\"repo:go-auth Login\". " +
657-
"When a name is ambiguous the response carries a resolution object with ranked candidates and a " +
658-
"confidence score; one dominant candidate (>80% confidence) is auto-resolved so the path is still returned. " +
659-
"Returns an ordered list of nodes and edges, or reports no path found within max_depth hops.",
666+
"(repo:, kind:, file:) plus PACKAGE-QUALIFIED names to pin down a common short name — e.g. " +
667+
"to=\"ticket.Repository\" or to=\"repo:golf domain/cart.CartService\" resolves where bare " +
668+
"\"Repository\"/\"CartService\" would be ambiguous. " +
669+
"When an endpoint is ambiguous, find_path TRIES the top candidates (and, for a type, its methods/constructor) " +
670+
"and returns the first path it finds; the response carries resolution objects with the ranked candidates. " +
671+
"If no path connects any candidate pair, found=false and a 'note' explains whether the endpoints were " +
672+
"ambiguous (with the candidates tried) or resolved uniquely but unreachable within max_depth hops.",
660673
}, func(ctx context.Context, req *mcp.CallToolRequest, args findPathArgs) (*mcp.CallToolResult, any, error) {
661674
if s.toolCallback != nil {
662675
s.toolCallback("find_path")
@@ -682,23 +695,46 @@ func (s *Server) registerTools() {
682695
if err != nil {
683696
return errorResult(fmt.Sprintf("to: %v", err)), nil, nil
684697
}
685-
// If either endpoint was too ambiguous to resolve, refuse to guess and
686-
// return the resolution(s) with an empty path.
687-
if (fromRes != nil && fromRes.Matched == "") || (toRes != nil && toRes.Matched == "") {
698+
699+
// Build ranked candidate lists for each endpoint (most-likely first) and try
700+
// a path across the combinations rather than silently giving up when a name
701+
// is ambiguous. This delivers the "give me vague names and I'll find the
702+
// connection" behavior.
703+
fromCands := s.pathCandidates(store, args.From, fromName, fromRes)
704+
toCands := s.pathCandidates(store, args.To, toName, toRes)
705+
if len(fromCands) == 0 || len(toCands) == 0 {
688706
return jsonResult(findPathResponse{
689707
FromResolution: fromRes,
690708
ToResolution: toRes,
691709
PathResult: facts.PathResult{From: fromName, To: toName, Found: false},
710+
Note: "could not resolve both endpoints to a graph node",
711+
FromTried: fromCands,
712+
ToTried: toCands,
692713
})
693714
}
694715

695-
result := graph.FindPath(fromName, toName, args.RelationKinds, args.MaxDepth)
716+
result := s.bestPath(graph, fromCands, toCands, args.RelationKinds, args.MaxDepth)
696717

697-
return jsonResult(findPathResponse{
718+
resp := findPathResponse{
698719
FromResolution: fromRes,
699720
ToResolution: toRes,
700721
PathResult: result,
701-
})
722+
FromTried: fromCands,
723+
ToTried: toCands,
724+
}
725+
if !result.Found {
726+
ambiguous := len(fromCands) > 1 || len(toCands) > 1
727+
if ambiguous {
728+
resp.Note = fmt.Sprintf("no path within %d hops between any candidate pair "+
729+
"(from: %d candidate(s), to: %d candidate(s)). Narrow with a package-qualified "+
730+
"name (e.g. \"repo:<label> pkg.Type\") — see from_tried/to_tried.",
731+
effectiveMaxDepth(args.MaxDepth), len(fromCands), len(toCands))
732+
} else {
733+
resp.Note = fmt.Sprintf("both endpoints resolved uniquely, but no path connects them within %d hops",
734+
effectiveMaxDepth(args.MaxDepth))
735+
}
736+
}
737+
return jsonResult(resp)
702738
})
703739

704740
// Tool: impact_analysis
@@ -785,6 +821,14 @@ const maxCandidates = 3
785821
// automatically to its top-scoring candidate instead of being refused.
786822
const autoPickConfidence = 0.80
787823

824+
// maxPathCandidates caps how many ranked candidates find_path considers per
825+
// endpoint when a name is ambiguous.
826+
const maxPathCandidates = 8
827+
828+
// maxPathAttempts caps the total FindPath probes find_path runs across the
829+
// candidate/seed combinations, so a doubly-ambiguous query stays bounded.
830+
const maxPathAttempts = 40
831+
788832
// nameResolution reports how a user-provided name was resolved to a concrete
789833
// fact name. It is surfaced in tool responses ONLY when the input matched more
790834
// than one fact (i.e. Ambiguous is true), so callers can detect and correct a
@@ -1140,6 +1184,96 @@ func fileBaseMatches(path, lowerTerm string) bool {
11401184
return false
11411185
}
11421186

1187+
// rankedCandidatesFor returns the facts matching an input (after repo-prefix
1188+
// auto-detection and scope parsing), ranked most-relevant first. Used by find_path
1189+
// to consider more candidates than the capped set carried in a nameResolution.
1190+
func (s *Server) rankedCandidatesFor(store *facts.Store, input string) []scoredCandidate {
1191+
input = s.maybePrefixRepoLabel(input)
1192+
sq := parseScopedQuery(input)
1193+
term := s.normalizeToRelative(sq.Term)
1194+
return rankCandidates(s.gatherCandidates(store, sq, term), sq)
1195+
}
1196+
1197+
// pathCandidates returns the ranked node names find_path should try for one
1198+
// endpoint, most-likely first and capped at maxPathCandidates. It leads with the
1199+
// resolved name (preserving resolveNodeName's exact/service/file-basename
1200+
// fallbacks), then any candidates from the resolution, then broadens with a fresh
1201+
// ranking so a heavily-ambiguous name (10+ matches) is not limited to the few
1202+
// candidates echoed in the resolution object.
1203+
func (s *Server) pathCandidates(store *facts.Store, input, resolved string, res *nameResolution) []string {
1204+
seen := make(map[string]struct{})
1205+
out := make([]string, 0, maxPathCandidates)
1206+
add := func(n string) {
1207+
if n == "" || len(out) >= maxPathCandidates {
1208+
return
1209+
}
1210+
if _, dup := seen[n]; dup {
1211+
return
1212+
}
1213+
seen[n] = struct{}{}
1214+
out = append(out, n)
1215+
}
1216+
add(resolved)
1217+
if res != nil {
1218+
for _, c := range res.Candidates {
1219+
add(c.Name)
1220+
}
1221+
}
1222+
for _, c := range s.rankedCandidatesFor(store, input) {
1223+
add(c.Name)
1224+
}
1225+
return out
1226+
}
1227+
1228+
// bestPath tries to connect any from-candidate to any to-candidate, expanding each
1229+
// to-candidate with RollupSeeds (a path to a type usually ends at one of its
1230+
// methods/constructor). It scans in ranked order, keeps the shortest path found,
1231+
// and is bounded by maxPathAttempts (returning early on a ≤2-hop hit). When no
1232+
// path exists it returns a not-found PathResult naming the first from/to tried.
1233+
func (s *Server) bestPath(graph *facts.Graph, fromCands, toCands []string, relKinds []string, maxDepth int) facts.PathResult {
1234+
var best facts.PathResult
1235+
attempts := 0
1236+
for _, from := range fromCands {
1237+
for _, to := range toCands {
1238+
for _, target := range graph.RollupSeeds(to) {
1239+
if attempts >= maxPathAttempts {
1240+
if best.Found {
1241+
return best
1242+
}
1243+
return facts.PathResult{From: fromCands[0], To: toCands[0], Found: false}
1244+
}
1245+
attempts++
1246+
r := graph.FindPath(from, target, relKinds, maxDepth)
1247+
if !r.Found {
1248+
continue
1249+
}
1250+
if !best.Found || len(r.Path) < len(best.Path) {
1251+
best = r
1252+
}
1253+
if len(best.Path) <= 2 { // direct or one-hop: good enough
1254+
return best
1255+
}
1256+
}
1257+
}
1258+
}
1259+
if best.Found {
1260+
return best
1261+
}
1262+
return facts.PathResult{From: fromCands[0], To: toCands[0], Found: false}
1263+
}
1264+
1265+
// effectiveMaxDepth mirrors graph.FindPath's clamping (0 → 10, cap 20) so the
1266+
// not-found note reports the depth actually searched.
1267+
func effectiveMaxDepth(maxDepth int) int {
1268+
if maxDepth <= 0 {
1269+
return 10
1270+
}
1271+
if maxDepth > 20 {
1272+
return 20
1273+
}
1274+
return maxDepth
1275+
}
1276+
11431277
// longestAlnumRun returns the longest maximal run of [A-Za-z0-9_] in s. Used to
11441278
// derive a robust substring probe from a dotted/spaced term.
11451279
func longestAlnumRun(s string) string {
@@ -2510,5 +2644,12 @@ type impactResponse struct {
25102644
type findPathResponse struct {
25112645
FromResolution *nameResolution `json:"from_resolution,omitempty"`
25122646
ToResolution *nameResolution `json:"to_resolution,omitempty"`
2647+
// Note explains a found:false result — whether the endpoints were ambiguous
2648+
// (with the candidates tried) or resolved uniquely but unreachable.
2649+
Note string `json:"note,omitempty"`
2650+
// FromTried/ToTried are the ranked endpoint candidates find_path attempted,
2651+
// most-likely first, so the caller can see what was searched.
2652+
FromTried []string `json:"from_tried,omitempty"`
2653+
ToTried []string `json:"to_tried,omitempty"`
25132654
facts.PathResult
25142655
}

0 commit comments

Comments
 (0)