diff --git a/cmd/gortex/audit.go b/cmd/gortex/audit.go index 4e588f5f..5ba1e48d 100644 --- a/cmd/gortex/audit.go +++ b/cmd/gortex/audit.go @@ -74,7 +74,11 @@ func runAudit(cmd *cobra.Command, _ []string) error { w := cmd.ErrOrStderr() emitAuditBanner(w, abs) - out, err := requireDaemonTool(auditPath, "audit_health", map[string]any{}) + // --path both selects the daemon connection and narrows the report. + // Passing it as the `repo` selector is what makes `gortex audit + // --path /tmp/myrepo` grade that tree rather than every repo the + // daemon happens to track. + out, err := requireDaemonTool(auditPath, "audit_health", map[string]any{"repo": abs}) if err != nil { return err } diff --git a/internal/mcp/analyze_alias_scope_test.go b/internal/mcp/analyze_alias_scope_test.go new file mode 100644 index 00000000..45875252 --- /dev/null +++ b/internal/mcp/analyze_alias_scope_test.go @@ -0,0 +1,182 @@ +package mcp + +// Scope regression tests for `analyze kind=health`, which the facade +// routes to the legacy audit_health tool instead of the analyze +// dispatcher. Because that alias shadows the dispatcher, the handler +// never ran handleAnalyze's resolveScope — it discarded its whole +// request and graded every node in the index, so a workspace-bound +// session was graded against sibling workspaces and an explicit repo +// selector was accepted and silently ignored. +// +// The substring probe ("repo-a" / "repo-b") and the fixtures match +// analyze_scope_test.go: multi-repo node IDs and file paths carry the +// repo name, so a result narrowed to repo-a must never contain "repo-b". + +import ( + "context" + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/zzet/gortex/internal/graph" +) + +// ─── helpers ──────────────────────────────────────────────────────────────── + +// runAuditHealth drives the audit_health handler and decodes its report. +func runAuditHealth(t *testing.T, srv *Server, ctx context.Context, args map[string]any) (AuditReport, string, string) { + t.Helper() + res, err := srv.handleAuditHealth(ctx, makeReq("audit_health", args)) + require.NoError(t, err) + require.NotNil(t, res) + require.False(t, res.IsError, "audit_health %v errored: %s", args, toolResultText(res)) + + text := toolResultText(res) + var report AuditReport + require.NoError(t, json.Unmarshal([]byte(text), &report), "audit_health must return a JSON AuditReport: %s", text) + + applied := "" + if res.Meta != nil { + applied, _ = res.Meta.AdditionalFields["scope_applied"].(string) + } + return report, applied, text +} + +// callableCountForRepo counts the callable symbols the audit grades in +// one repo, straight from the graph — so assertions compare against the +// fixture's real shape instead of a hard-coded number. +func callableCountForRepo(g graph.Store, repoPrefix string) int { + n := 0 + for _, node := range g.AllNodes() { + if node == nil || node.RepoPrefix != repoPrefix { + continue + } + if node.Kind == graph.KindFunction || node.Kind == graph.KindMethod { + n++ + } + } + return n +} + +// ─── audit_health (analyze kind=health) ───────────────────────────────────── + +// TestAuditHealth_WorkspaceIsolation is the security invariant: a +// session bound to workspace alpha must never be graded against beta's +// symbols. Before the fix handleAuditHealth discarded its context, so +// this leaked every repo in the index. +func TestAuditHealth_WorkspaceIsolation(t *testing.T) { + srv, paths := newAnalyzeServer(t, true, + analyzeRepoSpec{name: "repo-a", workspace: "alpha", body: structuralBody("a")}, + analyzeRepoSpec{name: "repo-b", workspace: "beta", body: structuralBody("b")}, + ) + ctxA := sessionCtx("s-alpha", paths["repo-a"]) + + report, _, text := runAuditHealth(t, srv, ctxA, nil) + + wantA := callableCountForRepo(srv.graph, "repo-a") + require.Positive(t, wantA, "fixture must produce callable symbols in repo-a") + assert.Equal(t, wantA, report.SymbolCount, + "alpha-bound session must be graded on repo-a's callables only") + assert.NotContains(t, text, "repo-b", + "alpha-bound session leaked beta symbols through audit_health — cross-workspace isolation breach") +} + +// TestAuditHealth_RepoNarrows is issue #349's repro 2: within a single +// workspace holding two repos, an explicit repo selector must narrow the +// grade instead of being accepted and ignored. +func TestAuditHealth_RepoNarrows(t *testing.T) { + srv, paths := newAnalyzeServer(t, true, + analyzeRepoSpec{name: "repo-a", workspace: "shared-ws", body: structuralBody("a")}, + analyzeRepoSpec{name: "repo-b", workspace: "shared-ws", body: structuralBody("b")}, + ) + ctx := sessionCtx("s-shared", paths["repo-a"]) + + countA := callableCountForRepo(srv.graph, "repo-a") + countB := callableCountForRepo(srv.graph, "repo-b") + require.Positive(t, countA) + require.Positive(t, countB) + + // No selector: the whole workspace, both repos. + broad, _, _ := runAuditHealth(t, srv, ctx, nil) + assert.Equal(t, countA+countB, broad.SymbolCount, + "an unnarrowed call must grade the whole bound workspace") + + // Narrowed by tracked repo name. + narrow, applied, text := runAuditHealth(t, srv, ctx, map[string]any{"repo": "repo-a"}) + assert.Equal(t, countA, narrow.SymbolCount, "repo selector must narrow the graded set") + assert.Less(t, narrow.SymbolCount, broad.SymbolCount, + "repo:repo-a must grade strictly fewer symbols than the whole workspace") + assert.Equal(t, "repo:repo-a", applied, "the response must disclose the applied scope") + assert.NotContains(t, text, "repo-b", "repo:repo-a must not report repo-b symbols") +} + +// TestAuditHealth_RepoPathFormNarrows covers the other selector form the +// issue exercised — target={"repo":""} — which reaches the +// handler as a filesystem path and must resolve to the same tracked +// prefix as the name form. +func TestAuditHealth_RepoPathFormNarrows(t *testing.T) { + srv, paths := newAnalyzeServer(t, true, + analyzeRepoSpec{name: "repo-a", workspace: "shared-ws", body: structuralBody("a")}, + analyzeRepoSpec{name: "repo-b", workspace: "shared-ws", body: structuralBody("b")}, + ) + ctx := sessionCtx("s-shared", paths["repo-a"]) + + byName, _, _ := runAuditHealth(t, srv, ctx, map[string]any{"repo": "repo-a"}) + byPath, _, text := runAuditHealth(t, srv, ctx, map[string]any{"repo": paths["repo-a"]}) + + assert.Equal(t, byName.SymbolCount, byPath.SymbolCount, + "the path form of the repo selector must narrow identically to the name form") + assert.NotContains(t, text, "repo-b", "the path form must not report repo-b symbols") +} + +// TestAuditHealth_UnknownRepoErrors pins the "no silent no-ops" contract: +// a selector that resolves to nothing inside the workspace must fail +// loudly rather than return a plausible-looking global report. +func TestAuditHealth_UnknownRepoErrors(t *testing.T) { + srv, paths := newAnalyzeServer(t, true, + analyzeRepoSpec{name: "repo-a", workspace: "alpha", body: structuralBody("a")}, + analyzeRepoSpec{name: "repo-b", workspace: "beta", body: structuralBody("b")}, + ) + ctxA := sessionCtx("s-alpha", paths["repo-a"]) + + res, err := srv.handleAuditHealth(ctxA, makeReq("audit_health", map[string]any{"repo": "repo-b"})) + require.NoError(t, err) + require.NotNil(t, res) + assert.True(t, res.IsError, + "naming an out-of-workspace repo must error, not silently return a global report") +} + +// TestAuditHealth_SingleRepoUnaffected guards the README-badge path. +// A single-repo daemon mints nodes with an empty RepoPrefix and has no +// multi-indexer to resolve a path selector against, so `gortex audit` +// now sends a `repo` argument that matches no registry prefix. Those +// nodes must still be graded — QueryOptions.ScopeAllows admits +// unprefixed nodes rather than letting a repo narrow match vacuously. +func TestAuditHealth_SingleRepoUnaffected(t *testing.T) { + srv, dir := setupTestServer(t) + + bare, _, _ := runAuditHealth(t, srv, context.Background(), nil) + require.Positive(t, bare.SymbolCount, "single-repo fixture must grade its callables") + + // The path form the CLI sends, which resolves to no tracked prefix. + withPath, _, _ := runAuditHealth(t, srv, context.Background(), map[string]any{"repo": dir}) + assert.Equal(t, bare.SymbolCount, withPath.SymbolCount, + "a path selector in single-repo mode must not narrow the report to nothing") +} + +// TestAuditHealth_UnboundSessionStaysGlobal guards the compatibility +// edge: a session with no cwd anchor has no workspace ceiling, so the +// badge/CLI path keeps grading the whole index. +func TestAuditHealth_UnboundSessionStaysGlobal(t *testing.T) { + srv, _ := newAnalyzeServer(t, true, + analyzeRepoSpec{name: "repo-a", workspace: "alpha", body: structuralBody("a")}, + analyzeRepoSpec{name: "repo-b", workspace: "beta", body: structuralBody("b")}, + ) + + report, _, _ := runAuditHealth(t, srv, context.Background(), nil) + want := callableCountForRepo(srv.graph, "repo-a") + callableCountForRepo(srv.graph, "repo-b") + assert.Equal(t, want, report.SymbolCount, + "an unbound session has no workspace ceiling and must still grade the whole index") +} diff --git a/internal/mcp/tools_audit.go b/internal/mcp/tools_audit.go index ee35c685..a80100d4 100644 --- a/internal/mcp/tools_audit.go +++ b/internal/mcp/tools_audit.go @@ -30,28 +30,55 @@ type AuditSymbolScore struct { Line int `json:"line"` } +// auditHealthKinds is the callable kind set the complexity axis scores. +// Declared once so the scoped-node pushdown and ComputeAuditReport's +// defensive re-check agree on what "callable" means. +var auditHealthKinds = []graph.NodeKind{graph.KindFunction, graph.KindMethod} + func (s *Server) registerAuditTool() { s.addTool( mcp.NewTool("audit_health", - mcp.WithDescription("Compute a repo-level complexity-axis health grade (A-F) from the graph: per-callable fan-in/fan-out health, the mean grade, the per-grade distribution, and the worst-scored symbols. Backs `gortex audit` and its README badge."), + mcp.WithDescription("Compute a repo-level complexity-axis health grade (A-F) from the graph: per-callable fan-in/fan-out health, the mean grade, the per-grade distribution, and the worst-scored symbols. Backs `gortex audit` and its README badge. Scored symbols are clamped to the session workspace and narrowed further by repo/project/scope."), + mcp.WithString("repo", mcp.Description("Narrow the audit to a single repository prefix (tracked name or path), clamped to the session workspace.")), + mcp.WithString("project", mcp.Description("Narrow the audit to the repositories in a project, clamped to the session workspace.")), + mcp.WithString("workspace", mcp.Description("Restrict the audit to the active workspace slug; daemon sessions may only name their own workspace.")), + mcp.WithString("scope", mcp.Description("Name of a saved scope (see save_scope) — its repositories narrow the audit, clamped to the session workspace.")), ), s.handleAuditHealth, ) } -func (s *Server) handleAuditHealth(_ context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) { +// handleAuditHealth grades the callable symbols the caller can actually +// see. It resolves the uniform repo/project/workspace/scope narrowing +// exactly like the analyze dispatcher does, then sources its symbols +// through the scoped-node accessor so a workspace-bound session can +// never be graded against another workspace's symbols — and so an +// explicit repo selector narrows instead of being silently dropped. +func (s *Server) handleAuditHealth(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { g := s.graph if g == nil { return mcp.NewToolResultError("audit: graph is not initialised"), nil } - data, err := json.Marshal(ComputeAuditReport(g)) + resolved, errResult := s.resolveScope(ctx, req, IntentAnalyze) + if errResult != nil { + return errResult, nil + } + ctx = withRepoAllow(ctx, resolved.RepoAllow) + data, err := json.Marshal(ComputeAuditReport(g, s.scopedNodesByKinds(ctx, auditHealthKinds))) if err != nil { return mcp.NewToolResultError(err.Error()), nil } - return mcp.NewToolResultText(string(data)), nil + return withScopeResult(mcp.NewToolResultText(string(data)), nil, resolved) } -// ComputeAuditReport walks the graph and produces the repo grade. +// ComputeAuditReport grades the supplied callable nodes and produces the +// repo grade. Callers pass the node set already narrowed to the scope +// they may observe (see Server.scopedNodesByKinds) — this function never +// widens beyond it. Fan-in / fan-out are still counted over the whole +// graph, so a cross-repo caller still contributes to a symbol's score; +// only which symbols get scored is scoped. That matches the health_score +// analyzer, which likewise scopes candidates and counts edges globally. +// // Complexity-axis-only math matching the health_score analyzer's // complexity component: // @@ -59,14 +86,14 @@ func (s *Server) handleAuditHealth(_ context.Context, _ mcp.CallToolRequest) (*m // complexity_health = 100 / (1 + raw/20) // mean = mean across callable symbols // grade = auditScoreGrade(mean) -func ComputeAuditReport(g graph.Store) AuditReport { +func ComputeAuditReport(g graph.Store, nodes []*graph.Node) AuditReport { type entry struct { id, file string line int score float64 } var entries []entry - for _, n := range g.AllNodes() { + for _, n := range nodes { if n == nil { continue } diff --git a/internal/mcp/tools_audit_test.go b/internal/mcp/tools_audit_test.go index 418e67dd..6555ef8c 100644 --- a/internal/mcp/tools_audit_test.go +++ b/internal/mcp/tools_audit_test.go @@ -23,7 +23,8 @@ func TestAuditScoreGrade_Boundaries(t *testing.T) { } func TestComputeAuditReport_EmptyGraphReturnsF(t *testing.T) { - r := ComputeAuditReport(graph.New()) + emptyG := graph.New() + r := ComputeAuditReport(emptyG, emptyG.AllNodes()) if r.SymbolCount != 0 { t.Errorf("empty graph SymbolCount = %d, want 0", r.SymbolCount) } @@ -49,7 +50,7 @@ func TestComputeAuditReport_ScoresAndGradesPopulated(t *testing.T) { g.AddEdge(&graph.Edge{From: "c.go::caller_" + strconv.Itoa(i), To: "f.go::med", Kind: graph.EdgeCalls}) } - r := ComputeAuditReport(g) + r := ComputeAuditReport(g, g.AllNodes()) if r.SymbolCount == 0 { t.Fatalf("expected callable symbols in report, got 0") } @@ -69,7 +70,7 @@ func TestComputeAuditReport_GradeCountsSumToSymbolCount(t *testing.T) { for i := range 20 { g.AddNode(&graph.Node{ID: "f.go::F" + strconv.Itoa(i), Kind: graph.KindFunction}) } - r := ComputeAuditReport(g) + r := ComputeAuditReport(g, g.AllNodes()) sum := 0 for _, n := range r.GradeCounts { sum += n @@ -84,7 +85,7 @@ func TestComputeAuditReport_NonCallableKindsSkipped(t *testing.T) { g.AddNode(&graph.Node{ID: "f.go::T", Name: "T", Kind: graph.KindType}) g.AddNode(&graph.Node{ID: "f.go::V", Name: "V", Kind: graph.KindVariable}) g.AddNode(&graph.Node{ID: "f.go::F", Name: "F", Kind: graph.KindFunction}) - r := ComputeAuditReport(g) + r := ComputeAuditReport(g, g.AllNodes()) if r.SymbolCount != 1 { t.Errorf("SymbolCount = %d, want 1 (only function counted)", r.SymbolCount) } diff --git a/internal/mcp/tools_clones.go b/internal/mcp/tools_clones.go index 54d5e443..73b84253 100644 --- a/internal/mcp/tools_clones.go +++ b/internal/mcp/tools_clones.go @@ -25,7 +25,10 @@ func (s *Server) registerCloneTools() { mcp.WithNumber("min_similarity", mcp.Description("Only report clone pairs at or above this estimated Jaccard similarity (0..1). Default 0 — every recorded EdgeSimilarTo edge.")), mcp.WithBoolean("dead_only", mcp.Description("Only return clusters that contain at least one dead-code symbol — the \"dead duplicates of live code\" view. Default false.")), mcp.WithString("path_prefix", mcp.Description("Restrict to symbols whose file path starts with this prefix.")), - mcp.WithString("repo", mcp.Description("Restrict to symbols in a specific repository (RepoPrefix exact match).")), + mcp.WithString("repo", mcp.Description("Narrow to a single repository prefix (tracked name or path), clamped to the session workspace.")), + mcp.WithString("project", mcp.Description("Narrow to the repositories in a project, clamped to the session workspace.")), + mcp.WithString("workspace", mcp.Description("Restrict to the active workspace slug; daemon sessions may only name their own workspace.")), + mcp.WithString("scope", mcp.Description("Name of a saved scope (see save_scope) — its repositories narrow the search, clamped to the session workspace.")), mcp.WithNumber("limit", mcp.Description("Maximum clusters to return (default: 50). Clusters are ranked largest-first.")), mcp.WithString("format", mcp.Description("Output format: json (default), gcx (GCX1 compact wire format), or toon")), mcp.WithNumber("max_bytes", mcp.Description("Cap the marshaled response at this many bytes. The longest list is trimmed; truncation metadata rides on the response. Omit for no cap.")), @@ -58,12 +61,24 @@ func (s *Server) handleFindClones(ctx context.Context, req mcp.CallToolRequest) minSim := req.GetFloat("min_similarity", 0) deadOnly := req.GetBool("dead_only", false) pathPrefix := strings.TrimSpace(stringArg(args, "path_prefix")) - repo := strings.TrimSpace(stringArg(args, "repo")) limit := req.GetInt("limit", 50) if limit <= 0 { limit = 50 } + // Resolve the uniform repo/project/workspace/scope narrowing the way + // the analyze dispatcher does, then let analyzeNodeVisible enforce + // both the session-workspace ceiling and the resolved allow-set. The + // `repo` arg used to be matched here as a bare RepoPrefix equality, + // which honoured an exact tracked name but silently ignored the path + // form and never clamped an unnarrowed call to the session's own + // workspace — so a bound session saw sibling workspaces' clones. + resolved, errResult := s.resolveScope(ctx, req, IntentAnalyze) + if errResult != nil { + return errResult, nil + } + ctx = withRepoAllow(ctx, resolved.RepoAllow) + inScope := func(n *graph.Node) bool { if n == nil { return false @@ -74,10 +89,7 @@ func (s *Server) handleFindClones(ctx context.Context, req mcp.CallToolRequest) if pathPrefix != "" && !strings.HasPrefix(n.FilePath, pathPrefix) { return false } - if repo != "" && n.RepoPrefix != repo { - return false - } - return true + return s.analyzeNodeVisible(ctx, n) } // Walk EdgeSimilarTo edges. The graph holds them symmetrically diff --git a/internal/mcp/tools_clones_scope_test.go b/internal/mcp/tools_clones_scope_test.go new file mode 100644 index 00000000..da03d852 --- /dev/null +++ b/internal/mcp/tools_clones_scope_test.go @@ -0,0 +1,118 @@ +package mcp + +// Scope regression tests for `analyze kind=clones`, which the facade +// routes to the legacy find_clones tool instead of the analyze +// dispatcher. Because that alias shadows the dispatcher, the handler +// never ran handleAnalyze's resolveScope: it matched `repo` as a bare +// RepoPrefix equality — honouring an exact tracked name but silently +// ignoring the path form — and never applied the session's workspace +// ceiling, so a bound session saw sibling workspaces' clone clusters. +// +// The substring probe ("repo-a" / "repo-b") and the fixtures match +// analyze_scope_test.go: multi-repo node IDs and file paths carry the +// repo name, so a result narrowed to repo-a must never contain "repo-b". + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/zzet/gortex/internal/graph" +) + +// firstTwoCallables returns two callable node IDs from one repo, used to +// synthesise a clone pair inside that repo. +func firstTwoCallables(t *testing.T, g graph.Store, repoPrefix string) (string, string) { + t.Helper() + var ids []string + for _, node := range g.AllNodes() { + if node == nil || node.RepoPrefix != repoPrefix { + continue + } + if node.Kind == graph.KindFunction || node.Kind == graph.KindMethod { + ids = append(ids, node.ID) + } + } + require.GreaterOrEqual(t, len(ids), 2, "fixture repo %q needs >=2 callables", repoPrefix) + // Sorted for determinism — AllNodes order is not guaranteed. + if ids[0] > ids[1] { + ids[0], ids[1] = ids[1], ids[0] + } + return ids[0], ids[1] +} + +// addClonePair records a symmetric EdgeSimilarTo pair, the shape the +// index-time MinHash/LSH pass materialises and find_clones reads. +func addClonePair(t *testing.T, g graph.Store, a, b string) { + t.Helper() + for _, e := range []*graph.Edge{ + {From: a, To: b, Kind: graph.EdgeSimilarTo, Meta: map[string]any{"similarity": 0.95}}, + {From: b, To: a, Kind: graph.EdgeSimilarTo, Meta: map[string]any{"similarity": 0.95}}, + } { + g.AddEdge(e) + } +} + +// TestFindClones_WorkspaceIsolation covers issue #349's repro 1 for +// clones: an unnarrowed call from a bound session returned clusters from +// sibling workspaces, because find_clones filtered only on an explicit +// `repo` string and never applied the workspace ceiling. +func TestFindClones_WorkspaceIsolation(t *testing.T) { + srv, paths := newAnalyzeServer(t, true, + analyzeRepoSpec{name: "repo-a", workspace: "alpha", body: structuralBody("a")}, + analyzeRepoSpec{name: "repo-b", workspace: "beta", body: structuralBody("b")}, + ) + + // One clone pair wholly inside each workspace. + a1, a2 := firstTwoCallables(t, srv.graph, "repo-a") + b1, b2 := firstTwoCallables(t, srv.graph, "repo-b") + addClonePair(t, srv.graph, a1, a2) + addClonePair(t, srv.graph, b1, b2) + + ctxA := sessionCtx("s-alpha", paths["repo-a"]) + res, err := srv.handleFindClones(ctxA, makeReq("find_clones", nil)) + require.NoError(t, err) + require.NotNil(t, res) + require.False(t, res.IsError, "find_clones errored: %s", toolResultText(res)) + + text := toolResultText(res) + assert.Contains(t, text, "repo-a", "alpha session must still see its own clone cluster") + assert.NotContains(t, text, "repo-b", + "alpha-bound session leaked a beta clone cluster — cross-workspace isolation breach") +} + +// TestFindClones_RepoPathFormNarrows pins the selector normalisation the +// handler gained with the workspace clamp: `repo` used to be an exact +// RepoPrefix match, so the path form silently matched nothing. +func TestFindClones_RepoPathFormNarrows(t *testing.T) { + srv, paths := newAnalyzeServer(t, true, + analyzeRepoSpec{name: "repo-a", workspace: "shared-ws", body: structuralBody("a")}, + analyzeRepoSpec{name: "repo-b", workspace: "shared-ws", body: structuralBody("b")}, + ) + + a1, a2 := firstTwoCallables(t, srv.graph, "repo-a") + b1, b2 := firstTwoCallables(t, srv.graph, "repo-b") + addClonePair(t, srv.graph, a1, a2) + addClonePair(t, srv.graph, b1, b2) + + ctx := sessionCtx("s-shared", paths["repo-a"]) + + // Unnarrowed: the whole workspace, so both clusters. + res, err := srv.handleFindClones(ctx, makeReq("find_clones", nil)) + require.NoError(t, err) + require.False(t, res.IsError, "find_clones errored: %s", toolResultText(res)) + broad := toolResultText(res) + assert.Contains(t, broad, "repo-a") + assert.Contains(t, broad, "repo-b", "an unnarrowed call must span the bound workspace") + + // Both selector forms must narrow identically. + for _, selector := range []string{"repo-a", paths["repo-a"]} { + res, err = srv.handleFindClones(ctx, makeReq("find_clones", map[string]any{"repo": selector})) + require.NoError(t, err) + require.False(t, res.IsError, "find_clones repo=%s errored: %s", selector, toolResultText(res)) + text := toolResultText(res) + assert.Contains(t, text, "repo-a", "repo=%s must keep repo-a's cluster", selector) + assert.NotContains(t, text, "repo-b", "repo=%s must drop repo-b's cluster", selector) + } +}