From 463509741ae6cf917cda7c5e7d0a2b0a4555a654 Mon Sep 17 00:00:00 2001 From: Peter Chanthamynavong Date: Mon, 26 Oct 2026 06:42:10 -0700 Subject: [PATCH] fix(mcp): skip invisible repo source reads in hotspots A scoped analyze hotspots call loaded every hotspot in the store with GetNode and only then dropped the entries outside the session workspace or the repo allow-set. Each extra tracked repo added one point read per hotspot, so a call scoped to one repo got slower as other repos were added (269 reads per call with one repo, 516 with six, on a store with 1.9 times the nodes). In a multi-repo store, a source node's ID starts with its repo prefix and a slash. The handler now builds the set of tracked repos that cannot be visible once per call, and skips an entry without a read only when its ID has that source form and its exact prefix is in the set. Single-repo IDs, stub IDs such as ::stdlib::..., unknown prefixes and the unbound path keep the existing read, and analyzeNodeVisible still decides every entry that is not skipped. Ranking, the analysis cache, totals and truncation do not change. TestHotspotsScopeSkipsInvisibleRepoReads runs memory and SQLite readers with three repos, including gortex2 and a worktree prefix, for repo, workspace, empty-workspace, threshold and novelty requests. It checks that the full response equals the base filter's and counts the reads: 71 to 25 for one repo of three, 71 to 48 for the shared workspace. --- ...tools_analyze_hotspots_scope_reads_test.go | 199 ++++++++++++++++++ internal/mcp/tools_enhancements.go | 17 ++ 2 files changed, 216 insertions(+) create mode 100644 internal/mcp/tools_analyze_hotspots_scope_reads_test.go diff --git a/internal/mcp/tools_analyze_hotspots_scope_reads_test.go b/internal/mcp/tools_analyze_hotspots_scope_reads_test.go new file mode 100644 index 000000000..e5f47f1ae --- /dev/null +++ b/internal/mcp/tools_analyze_hotspots_scope_reads_test.go @@ -0,0 +1,199 @@ +package mcp + +import ( + "context" + "fmt" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/zzet/gortex/internal/analysis" + "github.com/zzet/gortex/internal/graph" + "github.com/zzet/gortex/internal/graph/store_sqlite" +) + +type hotspotScopeReader struct { + graph.Reader + lookedUp []string +} + +func (r *hotspotScopeReader) GetNode(id string) *graph.Node { + r.lookedUp = append(r.lookedUp, id) + return r.Reader.GetNode(id) +} + +// Real indexing supplies the repo registry and verifies the source-ID invariant +// for both callable kinds. Extra ranked rows exercise exact totals past the cap. +func hotspotScopeFixture(t *testing.T, sqlite bool) (*Server, []analysis.HotspotEntry) { + t.Helper() + body := "package main\ntype T struct{}\nfunc F() {}\nfunc (T) M() {}\n" + s, _ := newAnalyzeServer(t, true, + analyzeRepoSpec{name: "gortex", workspace: "shared", body: body}, + analyzeRepoSpec{name: "gortex2", workspace: "shared", body: body}, + analyzeRepoSpec{name: "gortex@feature", workspace: "feature", body: body}, + ) + kinds := map[graph.NodeKind]bool{} + for _, n := range s.graph.AllNodes() { + if n.Kind == graph.KindFunction || n.Kind == graph.KindMethod { + require.Equal(t, n.RepoPrefix, graph.RepoPrefixOfID(n.ID), n.ID) + kinds[n.Kind] = true + } + } + require.True(t, kinds[graph.KindFunction]) + require.True(t, kinds[graph.KindMethod]) + if sqlite { + store, err := store_sqlite.Open(filepath.Join(t.TempDir(), "hotspots.sqlite")) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, store.Close()) }) + store.AddBatch(s.graph.AllNodes(), s.graph.AllEdges()) + s.graph = store + } + var entries []analysis.HotspotEntry + for i := range 23 { + for _, repo := range []string{"gortex2", "gortex@feature", "gortex"} { + ws := "shared" + if repo == "gortex@feature" { + ws = "feature" + } + kind := graph.KindFunction + if i%2 != 0 { + kind = graph.KindMethod + } + n := &graph.Node{ID: fmt.Sprintf("%s/pkg/hot.go::F%02d", repo, i), Name: fmt.Sprintf("F%02d", i), Kind: kind, + FilePath: repo + "/pkg/hot.go", RepoPrefix: repo, WorkspaceID: ws} + s.graph.AddNode(n) + entries = append(entries, analysis.HotspotEntry{ID: n.ID, Name: n.Name, Kind: string(kind), FilePath: n.FilePath, ComplexityScore: float64(100 - len(entries))}) + } + } + // Stub and unknown-prefix rows cannot safely use a slash-derived repo key. + for _, n := range []*graph.Node{ + {ID: "gortex@feature::stdlib::net/http::Get", Kind: graph.KindFunction, RepoPrefix: "gortex@feature", WorkspaceID: "feature", FilePath: "external::net/http"}, + {ID: "unknown/pkg/legacy.go::F", Kind: graph.KindFunction, RepoPrefix: "gortex", WorkspaceID: "shared", FilePath: "unknown/pkg/legacy.go"}, + } { + s.graph.AddNode(n) + entries = append(entries, analysis.HotspotEntry{ID: n.ID, Kind: string(n.Kind), FilePath: n.FilePath}) + } + for repo, callers := range map[string]int{"gortex": 22, "gortex2": 20, "gortex@feature": 18} { + for i := 1; i <= callers; i++ { + s.graph.AddEdge(&graph.Edge{From: fmt.Sprintf("%s/pkg/hot.go::F%02d", repo, i), To: repo + "/pkg/hot.go::F00", Kind: graph.EdgeCalls}) + } + } + installCommunitiesForTest(s, &analysis.CommunityResult{}) + s.hotspots = entries + s.hotspotsReady = true + return s, entries +} + +func TestHotspotsScopeSkipsInvisibleRepoReads(t *testing.T) { + t.Setenv("GORTEX_DAEMON_MEMRELEASE", "0") + for _, backend := range []string{"memory", "sqlite"} { + t.Run(backend, func(t *testing.T) { + s, cached := hotspotScopeFixture(t, backend == "sqlite") + for _, tc := range []struct { + name, workspace, repo, mode string + bound bool + threshold float64 + reads, total int + }{ + {name: "repo", repo: "gortex", reads: 25, total: 24}, + {name: "workspace", workspace: "shared", bound: true, reads: 48, total: 47}, + {name: "workspace_repo", workspace: "shared", repo: "gortex2", bound: true, reads: 25, total: 23}, + {name: "worktree", repo: "gortex@feature", reads: 25, total: 24}, + {name: "unbound", reads: 0, total: 71}, + {name: "deny_all", workspace: "missing", bound: true, reads: 2, total: 0}, + {name: "novelty", repo: "gortex", mode: "novelty", reads: 25, total: 24}, + {name: "threshold", repo: "gortex", threshold: 50, reads: 1, total: 1}, + } { + t.Run(tc.name, func(t *testing.T) { + ctx := WithSessionID(context.Background(), backend+"-"+tc.name) + if tc.bound { + ss := s.sessionFor(ctx) + ss.scopeResolved, ss.scopeBound, ss.scopeWorkspaceID = true, true, tc.workspace + } + if tc.repo != "" { + ctx = withRepoAllow(ctx, map[string]bool{tc.repo: true}) + } + args := map[string]any{"kind": "hotspots", "format": "json", "max_bytes": float64(0), "mode": tc.mode, "threshold": tc.threshold} + req := makeReq("analyze", args) + entries := cached + if tc.threshold != 0 { + entries = analysis.FindHotspots(s.graph, s.getCommunities(), tc.threshold) + require.NotEmpty(t, entries) + } + if tc.mode != "" { + entries = rerankHotspots(entries, s.graph, tc.mode, "adds", 30) + } + // The base implementation hydrates every entry before filtering. + kept := entries + if s.scopeFiltersActive(ctx) { + kept = make([]analysis.HotspotEntry, 0, len(entries)) + for _, e := range entries { + if s.analyzeNodeVisible(ctx, s.graph.GetNode(e.ID)) { + kept = append(kept, e) + } + } + } + total := len(kept) + require.Equal(t, tc.total, total) + if total > 20 { + kept = kept[:20] + } + want, err := s.respondJSONOrTOON(ctx, req, map[string]any{"hotspots": kept, "total": total, "truncated": total > 20}) + require.NoError(t, err) + reader := &hotspotScopeReader{Reader: s.graph} + callCtx := withRequestView(ctx, &requestView{reader: reader}) + got, err := s.handleFindHotspots(callCtx, req) + require.NoError(t, err) + require.False(t, got.IsError, toolResultText(got)) + require.Equal(t, toolResultText(want), toolResultText(got), "complete response must match the base filter") + require.Len(t, reader.lookedUp, tc.reads) + for _, id := range reader.lookedUp { + if id == "gortex@feature::stdlib::net/http::Get" || id == "unknown/pkg/legacy.go::F" { + continue + } + require.True(t, s.analyzeNodeVisible(ctx, s.graph.GetNode(id)), "invisible source entry was hydrated: %s", id) + } + if tc.threshold == 0 && tc.name != "unbound" { + require.Contains(t, reader.lookedUp, "gortex@feature::stdlib::net/http::Get") + require.Contains(t, reader.lookedUp, "unknown/pkg/legacy.go::F") + } + }) + } + }) + } +} + +func TestHotspotsScopeSingleRepoKeepsUnprefixedReads(t *testing.T) { + t.Setenv("GORTEX_DAEMON_MEMRELEASE", "0") + for _, backend := range []string{"memory", "sqlite"} { + t.Run(backend, func(t *testing.T) { + s, _ := newAnalyzeServer(t, true, analyzeRepoSpec{name: "solo", workspace: "solo", body: structuralBody("solo")}) + require.False(t, s.multiIndexer.IsMultiRepo()) + if backend == "sqlite" { + store, err := store_sqlite.Open(filepath.Join(t.TempDir(), "solo.sqlite")) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, store.Close()) }) + store.AddBatch(s.graph.AllNodes(), s.graph.AllEdges()) + s.graph = store + } + n := &graph.Node{ID: "internal/pkg.go::F", Kind: graph.KindFunction, WorkspaceID: "solo"} + s.graph.AddNode(n) + require.NotEqual(t, n.RepoPrefix, graph.RepoPrefixOfID(n.ID), "single-repo IDs do not encode repo ownership") + installCommunitiesForTest(s, &analysis.CommunityResult{}) + s.hotspots = []analysis.HotspotEntry{{ID: n.ID}} + s.hotspotsReady = true + ctx := outlineBoundContext(s, "solo", "solo", nil) + req := makeReq("analyze", map[string]any{"format": "json"}) + want, err := s.respondJSONOrTOON(ctx, req, map[string]any{"hotspots": s.hotspots, "total": 1, "truncated": false}) + require.NoError(t, err) + reader := &hotspotScopeReader{Reader: s.graph} + ctx = withRequestView(ctx, &requestView{reader: reader}) + result, err := s.handleFindHotspots(ctx, req) + require.NoError(t, err) + require.False(t, result.IsError) + require.Equal(t, toolResultText(want), toolResultText(result)) + require.Equal(t, []string{n.ID}, reader.lookedUp) + }) + } +} diff --git a/internal/mcp/tools_enhancements.go b/internal/mcp/tools_enhancements.go index 65077ef5f..bfaf2486e 100644 --- a/internal/mcp/tools_enhancements.go +++ b/internal/mcp/tools_enhancements.go @@ -2734,8 +2734,25 @@ func (s *Server) handleFindHotspots(ctx context.Context, req mcp.CallToolRequest // Strict no-op for an unbound session with no RepoAllow. if s.scopeFiltersActive(ctx) { reader := s.readerFor(ctx) + var excludedRepos map[string]bool + if s.multiIndexer != nil && s.multiIndexer.IsMultiRepo() { + workspaceRepos, bound := s.sessionWorkspaceRepoSet(ctx) + allow := repoAllowFromContext(ctx) + excludedRepos = make(map[string]bool) + for _, prefix := range s.multiIndexer.RepoPrefixes() { + if (bound && !workspaceRepos[prefix]) || (len(allow) > 0 && !allow[prefix]) { + excludedRepos[prefix] = true + } + } + } kept := make([]analysis.HotspotEntry, 0, len(entries)) for _, e := range entries { + // Only multi-repo source IDs encode ownership as prefix/path::symbol. + // Unprefixed, stub and unknown IDs still need the node visibility check. + path, symbol, sourceID := strings.Cut(e.ID, "::") + if sourceID && symbol != "" && excludedRepos[graph.RepoPrefixOfID(path)] { + continue + } if s.analyzeNodeVisible(ctx, reader.GetNode(e.ID)) { kept = append(kept, e) }