diff --git a/cmd/gortex/review.go b/cmd/gortex/review.go index 97d04f93..4143546a 100644 --- a/cmd/gortex/review.go +++ b/cmd/gortex/review.go @@ -5,7 +5,6 @@ import ( "fmt" "io" "os" - "sort" "strings" "github.com/spf13/cobra" @@ -180,9 +179,12 @@ type reviewPayloadCLI struct { Source string `json:"source"` } `json:"comments"` FileRisk []struct { - File string `json:"file"` - Risk string `json:"risk"` - Findings int `json:"findings"` + File string `json:"file"` + Risk string `json:"risk"` + Findings int `json:"findings"` + Affected int `json:"affected"` + Symbols int `json:"symbols"` + Uncovered int `json:"uncovered"` } `json:"file_risk"` Depth string `json:"depth"` Cost *struct { @@ -225,9 +227,12 @@ func reviewReportFromPayload(p reviewPayloadCLI) *review.ReviewReport { } for _, fr := range p.FileRisk { report.FileRisk = append(report.FileRisk, review.FileRisk{ - File: fr.File, - Risk: fr.Risk, - Findings: fr.Findings, + File: fr.File, + Risk: fr.Risk, + Findings: fr.Findings, + Affected: fr.Affected, + Symbols: fr.Symbols, + Uncovered: fr.Uncovered, }) } if p.Cost != nil { @@ -258,67 +263,15 @@ func printReview(cmd *cobra.Command, raw json.RawMessage) error { return nil } - // The agent audience renders the terse, machine-first summary from the - // canonical review renderer so the CLI and any sub-agent shelling the verb - // see byte-identical output. The human audience keeps the readable per-file - // packet below. + // Both audiences render through the canonical review renderer so the CLI, + // the MCP text rendering, and any sub-agent shelling the verb see the + // same packet — the human path used to keep a hand-rolled duplicate here, + // which silently dropped every field the renderer learned after it forked + // (coverage evidence, the rulepack-passed line). if strings.EqualFold(reviewAudience, "agent") { _, _ = io.WriteString(out, review.RenderSummary(reviewReportFromPayload(p), review.AudienceAgent)) return nil } - - verdict := p.Verdict - if verdict == "" { - verdict = "APPROVE" - } - _, _ = fmt.Fprintf(out, "Verdict: %s\n", verdict) - if p.Summary != "" { - _, _ = fmt.Fprintf(out, "%s\n", p.Summary) - } - - if len(p.FileRisk) > 0 { - _, _ = fmt.Fprintln(out, "\nFile risk:") - for _, fr := range p.FileRisk { - _, _ = fmt.Fprintf(out, " %-8s %s (%d finding(s))\n", fr.Risk, fr.File, fr.Findings) - } - } - - if len(p.Comments) == 0 { - _, _ = fmt.Fprintln(out, "\nNo inline findings.") - return nil - } - - // Group the comments by file, ordered within a file by line, so the output - // reads like a per-file review pass. - byFile := map[string][]int{} - for i, c := range p.Comments { - byFile[c.File] = append(byFile[c.File], i) - } - files := make([]string, 0, len(byFile)) - for f := range byFile { - files = append(files, f) - } - sort.Strings(files) - - _, _ = fmt.Fprintf(out, "\nFindings (%d):\n", len(p.Comments)) - for _, f := range files { - idxs := byFile[f] - sort.Slice(idxs, func(a, b int) bool { - return p.Comments[idxs[a]].Line < p.Comments[idxs[b]].Line - }) - _, _ = fmt.Fprintf(out, "\n%s\n", f) - for _, i := range idxs { - c := p.Comments[i] - rule := c.Rule - if c.Category != "" { - rule = strings.TrimSpace(c.Category + "/" + c.Rule) - } - _, _ = fmt.Fprintf(out, " L%-5d %-8s %s", c.Line, c.Severity, c.Message) - if rule != "" { - _, _ = fmt.Fprintf(out, " [%s]", rule) - } - _, _ = fmt.Fprintln(out) - } - } + _, _ = io.WriteString(out, review.RenderSummary(reviewReportFromPayload(p), review.AudienceHuman)) return nil } diff --git a/internal/analysis/diffmap.go b/internal/analysis/diffmap.go index ed5f3ae8..56f9bd4c 100644 --- a/internal/analysis/diffmap.go +++ b/internal/analysis/diffmap.go @@ -40,7 +40,10 @@ type DiffResult struct { // scope: "unstaged", "staged", "all", "compare" // baseRef: used when scope is "compare" (e.g., "main") // repoRoot: absolute path to the repository root -func MapGitDiff(g graph.Store, repoRoot, scope, baseRef string) (*DiffResult, error) { +// repoPrefix: the graph repo prefix anchoring repoRoot's indexed nodes. +// Multi-repo daemons key file paths as "/" while git emits +// repo-relative paths; empty in single-repo / unprefixed mode. +func MapGitDiff(g graph.Store, repoRoot, repoPrefix, scope, baseRef string) (*DiffResult, error) { args := buildDiffArgs(scope, baseRef) ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() @@ -56,10 +59,48 @@ func MapGitDiff(g graph.Store, repoRoot, scope, baseRef string) (*DiffResult, er } hunks := parseDiffHunks(string(output)) + return joinHunksToSymbols(g, repoPrefix, hunks), nil +} - result := &DiffResult{ - Hunks: hunks, +// JoinFileNodes maps one repo-relative changed-file path to the graph nodes +// its file defines. Indexed file paths carry the repo prefix in multi-repo +// mode ("/") while git and forge APIs emit repo-relative paths, +// so the raw lookup is tried first (single-repo / already-prefixed input) +// and the prefixed form second. +func JoinFileNodes(g graph.Store, repoPrefix, path string) []*graph.Node { + if nodes := g.GetFileNodes(path); len(nodes) > 0 { + return nodes + } + if repoPrefix == "" || strings.HasPrefix(path, repoPrefix+"/") { + return nil + } + return g.GetFileNodes(repoPrefix + "/" + path) +} + +// JoinFilePath returns the graph-keyed variant of a repo-relative file path: +// the raw path when it resolves (or no prefix applies), otherwise the prefixed +// form when that resolves. Falls back to the raw path when neither does, so +// the caller's downstream lookup misses exactly as it would have anyway. +func JoinFilePath(g graph.Store, repoPrefix, path string) string { + if repoPrefix == "" || strings.HasPrefix(path, repoPrefix+"/") { + return path + } + if len(g.GetFileNodes(path)) > 0 { + return path } + if prefixed := repoPrefix + "/" + path; len(g.GetFileNodes(prefixed)) > 0 { + return prefixed + } + return path +} + +// joinHunksToSymbols builds the hunk→symbol/file join shared by MapGitDiff +// and MapGitDiffWithLines: every symbol whose line range overlaps a changed +// hunk in its file, deduped, plus the changed-file set. ChangedFiles keeps +// the diff-relative paths (callers re-join them with git pathspecs); only +// the node lookup is prefix-aware. +func joinHunksToSymbols(g graph.Store, repoPrefix string, hunks []DiffHunk) *DiffResult { + result := &DiffResult{Hunks: hunks} fileSet := make(map[string]bool) symbolSeen := make(map[string]bool) @@ -68,8 +109,7 @@ func MapGitDiff(g graph.Store, repoRoot, scope, baseRef string) (*DiffResult, er fileSet[hunk.FilePath] = true // Find symbols whose line range overlaps the hunk - nodes := g.GetFileNodes(hunk.FilePath) - for _, n := range nodes { + for _, n := range JoinFileNodes(g, repoPrefix, hunk.FilePath) { if n.Kind == graph.KindFile { continue } @@ -93,41 +133,45 @@ func MapGitDiff(g graph.Store, repoRoot, scope, baseRef string) (*DiffResult, er result.ChangedFiles = append(result.ChangedFiles, f) } - return result, nil + return result } -func buildDiffArgs(scope, baseRef string) []string { +// GitDiffArgs builds the `git diff` argv for a scope ("unstaged", "staged", +// "all", "compare") with the given context width. The -c overrides pin the +// diff header prefixes to git's standard a/ b/ form: parseDiffHunks and +// parseDiffLines anchor on "+++ b/", which a developer's diff.mnemonicPrefix +// (c/ w/ headers on worktree-side diffs) or diff.noprefix config would +// otherwise silently zero out — every hunk drops, every diff-driven tool +// reports an empty changeset. +func GitDiffArgs(scope, baseRef string, unified int) []string { + args := []string{ + "-c", "diff.mnemonicPrefix=false", + "-c", "diff.noprefix=false", + "diff", + } switch scope { case "staged": - return []string{"diff", "--cached", "--unified=0"} + args = append(args, "--cached") case "all": - return []string{"diff", "HEAD", "--unified=0"} + args = append(args, "HEAD") case "compare": if baseRef == "" { baseRef = "main" } - return []string{"diff", baseRef + "...HEAD", "--unified=0"} - default: // unstaged - return []string{"diff", "--unified=0"} + args = append(args, baseRef+"...HEAD") + default: // unstaged — bare `git diff` } + return append(args, fmt.Sprintf("--unified=%d", unified)) +} + +func buildDiffArgs(scope, baseRef string) []string { + return GitDiffArgs(scope, baseRef, 0) } // buildDiffArgsWithContext mirrors buildDiffArgs but emits a context window so // the new-side line text survives into the hunk body for snippet grounding. func buildDiffArgsWithContext(scope, baseRef string) []string { - switch scope { - case "staged": - return []string{"diff", "--cached", "--unified=3"} - case "all": - return []string{"diff", "HEAD", "--unified=3"} - case "compare": - if baseRef == "" { - baseRef = "main" - } - return []string{"diff", baseRef + "...HEAD", "--unified=3"} - default: // unstaged - return []string{"diff", "--unified=3"} - } + return GitDiffArgs(scope, baseRef, 3) } // HunkLine is a single new-side line carried out of a unified diff: added lines @@ -253,7 +297,8 @@ func parseNewStart(line string) (int, bool) { // their post-change line numbers — the substrate snippet grounding anchors on. // The returned *DiffResult is computed with the same logic as MapGitDiff (only // the diff's context width differs), so symbol overlap is unaffected. -func MapGitDiffWithLines(g graph.Store, repoRoot, scope, baseRef string) (*DiffResult, map[string][]HunkLine, error) { +// repoPrefix anchors the node join exactly as in MapGitDiff. +func MapGitDiffWithLines(g graph.Store, repoRoot, repoPrefix, scope, baseRef string) (*DiffResult, map[string][]HunkLine, error) { args := buildDiffArgsWithContext(scope, baseRef) ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() @@ -271,39 +316,7 @@ func MapGitDiffWithLines(g graph.Store, repoRoot, scope, baseRef string) (*DiffR hunks := parseDiffHunks(text) lines := parseDiffLines(text) - result := &DiffResult{Hunks: hunks} - - fileSet := make(map[string]bool) - symbolSeen := make(map[string]bool) - - for _, hunk := range hunks { - fileSet[hunk.FilePath] = true - - nodes := g.GetFileNodes(hunk.FilePath) - for _, n := range nodes { - if n.Kind == graph.KindFile { - continue - } - if n.StartLine <= hunk.EndLine && n.EndLine >= hunk.StartLine { - if !symbolSeen[n.ID] { - symbolSeen[n.ID] = true - result.ChangedSymbols = append(result.ChangedSymbols, ChangedSymbol{ - ID: n.ID, - Name: n.Name, - Kind: string(n.Kind), - FilePath: n.FilePath, - Line: n.StartLine, - }) - } - } - } - } - - for f := range fileSet { - result.ChangedFiles = append(result.ChangedFiles, f) - } - - return result, lines, nil + return joinHunksToSymbols(g, repoPrefix, hunks), lines, nil } func parseDiffHunks(output string) []DiffHunk { diff --git a/internal/analysis/diffmap_test.go b/internal/analysis/diffmap_test.go index e3559d38..07b3a2b2 100644 --- a/internal/analysis/diffmap_test.go +++ b/internal/analysis/diffmap_test.go @@ -153,7 +153,7 @@ func TestMapGitDiffWithLinesReturnsNewSideLines(t *testing.T) { Language: "go", }) - res, lines, err := MapGitDiffWithLines(g, dir, "all", "") + res, lines, err := MapGitDiffWithLines(g, dir, "", "all", "") if err != nil { t.Fatalf("MapGitDiffWithLines: %v", err) } @@ -189,6 +189,149 @@ func TestMapGitDiffWithLinesReturnsNewSideLines(t *testing.T) { } } +// TestMapGitDiffRepoPrefixJoin covers the multi-repo daemon shape: indexed +// file paths carry the repo prefix ("myrepo/foo.go") while git emits +// repo-relative hunk paths ("foo.go"). The prefix-aware join must find the +// symbol; ChangedFiles must stay diff-relative so git pathspec re-joins keep +// working. +func TestMapGitDiffRepoPrefixJoin(t *testing.T) { + dir := newTestRepo(t) + + g := graph.New() + g.AddNode(&graph.Node{ + ID: "myrepo/foo.go::Foo", + Kind: graph.KindFunction, + Name: "Foo", + FilePath: "myrepo/foo.go", + StartLine: 3, + EndLine: 6, + Language: "go", + }) + + res, err := MapGitDiff(g, dir, "myrepo", "all", "") + if err != nil { + t.Fatalf("MapGitDiff: %v", err) + } + var sawFoo bool + for _, cs := range res.ChangedSymbols { + if cs.ID == "myrepo/foo.go::Foo" { + sawFoo = true + } + } + if !sawFoo { + t.Fatalf("expected prefixed Foo among changed symbols: %#v", res.ChangedSymbols) + } + if len(res.ChangedFiles) != 1 || res.ChangedFiles[0] != "foo.go" { + t.Fatalf("ChangedFiles must keep diff-relative paths, got %#v", res.ChangedFiles) + } + + // Without the prefix the join misses — the pre-fix behavior, kept for + // single-repo graphs whose paths are unprefixed. + res, err = MapGitDiff(g, dir, "", "all", "") + if err != nil { + t.Fatalf("MapGitDiff (no prefix): %v", err) + } + if len(res.ChangedSymbols) != 0 { + t.Fatalf("unprefixed join against a prefixed graph should miss, got %#v", res.ChangedSymbols) + } +} + +// TestMapGitDiffMnemonicPrefixConfig pins the diff header prefixes against +// hostile git config: with diff.mnemonicPrefix=true a worktree diff emits +// "+++ w/..." headers, which the "+++ b/" parser anchor would zero out — +// every diff-driven tool would silently report an empty changeset. The -c +// overrides in GitDiffArgs must win over repo and global config. +func TestMapGitDiffMnemonicPrefixConfig(t *testing.T) { + dir := newTestRepo(t) + run := func(args ...string) { + cmd := exec.Command("git", args...) + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + // Hostile repo-local config (newTestRepo sets both to false; flip them). + run("config", "diff.mnemonicPrefix", "true") + + g := graph.New() + g.AddNode(&graph.Node{ + ID: "foo.go::Foo", + Kind: graph.KindFunction, + Name: "Foo", + FilePath: "foo.go", + StartLine: 3, + EndLine: 6, + Language: "go", + }) + + res, err := MapGitDiff(g, dir, "", "all", "") + if err != nil { + t.Fatalf("MapGitDiff: %v", err) + } + if len(res.Hunks) == 0 { + t.Fatalf("expected hunks despite diff.mnemonicPrefix=true, got none") + } + var sawFoo bool + for _, cs := range res.ChangedSymbols { + if cs.ID == "foo.go::Foo" { + sawFoo = true + } + } + if !sawFoo { + t.Fatalf("expected Foo among changed symbols: %#v", res.ChangedSymbols) + } + + run("config", "diff.noprefix", "true") + res, err = MapGitDiff(g, dir, "", "all", "") + if err != nil { + t.Fatalf("MapGitDiff (noprefix): %v", err) + } + if len(res.Hunks) == 0 { + t.Fatalf("expected hunks despite diff.noprefix=true, got none") + } +} + +func TestJoinFileNodes(t *testing.T) { + g := graph.New() + g.AddNode(&graph.Node{ID: "myrepo/a.go::A", Kind: graph.KindFunction, Name: "A", FilePath: "myrepo/a.go"}) + g.AddNode(&graph.Node{ID: "b.go::B", Kind: graph.KindFunction, Name: "B", FilePath: "b.go"}) + + // Raw hit wins (single-repo / unprefixed graph). + if nodes := JoinFileNodes(g, "myrepo", "b.go"); len(nodes) != 1 || nodes[0].ID != "b.go::B" { + t.Fatalf("raw lookup should win: %#v", nodes) + } + // Relative path retries with the prefix. + if nodes := JoinFileNodes(g, "myrepo", "a.go"); len(nodes) != 1 || nodes[0].ID != "myrepo/a.go::A" { + t.Fatalf("prefixed retry should hit: %#v", nodes) + } + // Already-prefixed input does not double-prefix. + if nodes := JoinFileNodes(g, "myrepo", "myrepo/a.go"); len(nodes) != 1 || nodes[0].ID != "myrepo/a.go::A" { + t.Fatalf("already-prefixed input should hit raw: %#v", nodes) + } + // No prefix → raw only. + if nodes := JoinFileNodes(g, "", "a.go"); len(nodes) != 0 { + t.Fatalf("no-prefix miss should stay a miss: %#v", nodes) + } +} + +func TestJoinFilePath(t *testing.T) { + g := graph.New() + g.AddNode(&graph.Node{ID: "myrepo/a.go::A", Kind: graph.KindFunction, Name: "A", FilePath: "myrepo/a.go"}) + + if got := JoinFilePath(g, "myrepo", "a.go"); got != "myrepo/a.go" { + t.Fatalf("expected prefixed path, got %q", got) + } + if got := JoinFilePath(g, "myrepo", "myrepo/a.go"); got != "myrepo/a.go" { + t.Fatalf("already-prefixed path should pass through, got %q", got) + } + if got := JoinFilePath(g, "myrepo", "missing.go"); got != "missing.go" { + t.Fatalf("unresolvable path should pass through raw, got %q", got) + } + if got := JoinFilePath(g, "", "a.go"); got != "a.go" { + t.Fatalf("no prefix should pass through, got %q", got) + } +} + // TestMapGitDiffUnchanged asserts the existing --unified=0 path still yields the // same DiffResult shape (hunks + changed symbols + changed files) it always did, // independent of the new sibling. @@ -206,7 +349,7 @@ func TestMapGitDiffUnchanged(t *testing.T) { Language: "go", }) - res, err := MapGitDiff(g, dir, "all", "") + res, err := MapGitDiff(g, dir, "", "all", "") if err != nil { t.Fatalf("MapGitDiff: %v", err) } diff --git a/internal/analysis/impact.go b/internal/analysis/impact.go index ad8719ff..51a3b694 100644 --- a/internal/analysis/impact.go +++ b/internal/analysis/impact.go @@ -380,6 +380,11 @@ func assessRisk(directDeps, transitiveDeps int) RiskLevel { return RiskLow } +// IsTestFile reports whether path looks like a test source file — the same +// suffix set the impact traversal uses to collect covering tests, exported +// for callers that need to probe whether a graph indexes tests at all. +func IsTestFile(path string) bool { return isTestFile(path) } + func isTestFile(path string) bool { return containsAny(path, "_test.go", ".test.ts", ".test.js", ".spec.ts", ".spec.js", diff --git a/internal/mcp/diff_repo_scope_test.go b/internal/mcp/diff_repo_scope_test.go new file mode 100644 index 00000000..734f1316 --- /dev/null +++ b/internal/mcp/diff_repo_scope_test.go @@ -0,0 +1,80 @@ +package mcp + +import ( + "context" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/zzet/gortex/internal/config" + "github.com/zzet/gortex/internal/graph" + "github.com/zzet/gortex/internal/indexer" + "github.com/zzet/gortex/internal/parser" + "github.com/zzet/gortex/internal/parser/languages" + "github.com/zzet/gortex/internal/query" + "github.com/zzet/gortex/internal/search" +) + +// TestDiffRepoScope covers the shared repo-scope resolution the diff-driven +// handlers use: an explicit selector (prefix or path) is honoured +// exclusively, then the lone tracked repo, then the session's cwd-bound +// repo. An unknown selector resolves nothing so callers error instead of +// silently diffing another repo. +func TestDiffRepoScope(t *testing.T) { + repoA := setupMiniRepo(t, "repo-a") + repoB := setupMiniRepo(t, "repo-b") + + tmpCfg := filepath.Join(t.TempDir(), "config.yaml") + gc := &config.GlobalConfig{ + Repos: []config.RepoEntry{ + {Path: repoA, Name: "repo-a"}, + {Path: repoB, Name: "repo-b"}, + }, + } + gc.SetConfigPath(tmpCfg) + require.NoError(t, gc.Save()) + cm, err := config.NewConfigManager(tmpCfg) + require.NoError(t, err) + + reg := parser.NewRegistry() + reg.Register(languages.NewGoExtractor()) + g := graph.New() + mi := indexer.NewMultiIndexer(g, reg, search.NewBM25(), cm, zap.NewNop()) + _, err = mi.IndexAll() + require.NoError(t, err) + + srv := NewServer(query.NewEngine(g), g, nil, nil, zap.NewNop(), nil, MultiRepoOptions{ + ConfigManager: cm, + MultiIndexer: mi, + }) + + ctx := context.Background() + + // Explicit prefix selector. + root, prefix := srv.diffRepoScope(ctx, "repo-a") + require.Equal(t, repoA, root) + require.Equal(t, "repo-a", prefix) + + // Explicit path selector normalizes to the prefix. + root, prefix = srv.diffRepoScope(ctx, repoA) + require.Equal(t, repoA, root) + require.Equal(t, "repo-a", prefix) + + // An unknown selector resolves nothing — the caller errors instead of + // falling back to an unrelated repo. + root, prefix = srv.diffRepoScope(ctx, "nope") + require.Empty(t, root) + require.Empty(t, prefix) + + // No selector + multiple tracked repos + no session binding: ambiguous. + root, prefix = srv.diffRepoScope(ctx, "") + require.Empty(t, root) + require.Empty(t, prefix) + + // No selector + session cwd inside repo-b resolves repo-b. + root, prefix = srv.diffRepoScope(WithSessionCWD(ctx, repoB), "") + require.Equal(t, repoB, root) + require.Equal(t, "repo-b", prefix) +} diff --git a/internal/mcp/gcx.go b/internal/mcp/gcx.go index 037bde76..5e7de6cb 100644 --- a/internal/mcp/gcx.go +++ b/internal/mcp/gcx.go @@ -2292,13 +2292,16 @@ func encodeReview(result map[string]any) ([]byte, error) { return nil, err } - riskEnc := newGCX(&buf, "review.file_risk", []string{"file", "risk", "findings"}) + riskEnc := newGCX(&buf, "review.file_risk", []string{"file", "risk", "findings", "affected", "symbols", "uncovered"}) if risks, ok := result["file_risk"].([]map[string]any); ok { for _, r := range risks { file, _ := r["file"].(string) risk, _ := r["risk"].(string) findings, _ := r["findings"].(int) - if err := riskEnc.WriteRow(file, risk, findings); err != nil { + affected, _ := r["affected"].(int) + symbols, _ := r["symbols"].(int) + uncovered, _ := r["uncovered"].(int) + if err := riskEnc.WriteRow(file, risk, findings, affected, symbols, uncovered); err != nil { return nil, err } } @@ -2459,13 +2462,16 @@ func encodeReviewPack(result map[string]any) ([]byte, error) { return nil, err } - riskEnc := newGCX(&buf, "review_pack.file_risk", []string{"file", "risk", "findings"}) + riskEnc := newGCX(&buf, "review_pack.file_risk", []string{"file", "risk", "findings", "affected", "symbols", "uncovered"}) if risks, ok := result["file_risk"].([]map[string]any); ok { for _, r := range risks { file, _ := r["file"].(string) risk, _ := r["risk"].(string) findings, _ := r["findings"].(int) - if err := riskEnc.WriteRow(file, risk, findings); err != nil { + affected, _ := r["affected"].(int) + symbols, _ := r["symbols"].(int) + uncovered, _ := r["uncovered"].(int) + if err := riskEnc.WriteRow(file, risk, findings, affected, symbols, uncovered); err != nil { return nil, err } } diff --git a/internal/mcp/prompts.go b/internal/mcp/prompts.go index 3c91b377..0ebe9ca1 100644 --- a/internal/mcp/prompts.go +++ b/internal/mcp/prompts.go @@ -18,6 +18,9 @@ func (s *Server) registerPrompts() { mcp.WithArgument("scope", mcp.ArgumentDescription("Git diff scope: unstaged (default), staged, all, or compare"), ), + mcp.WithArgument("repo", + mcp.ArgumentDescription("Repository prefix or path (multi-repo mode); defaults to the lone tracked repo or the session's cwd-bound repo"), + ), ), s.handlePromptPreCommit, ) @@ -70,14 +73,15 @@ func (s *Server) handlePromptPreCommit(ctx context.Context, req mcp.GetPromptReq scope = "all" } - repoRoot := "." - if s.indexer != nil { - if root := s.indexer.RootPath(); root != "" { - repoRoot = root - } + // Resolve the working tree: explicit repo argument, lone tracked repo, + // or the session's cwd-bound repo. The "." fallback keeps the standalone + // (indexer-less) server working from its own cwd. + repoRoot, repoPrefix := s.diffRepoScope(ctx, promptArg(req, "repo")) + if repoRoot == "" { + repoRoot = "." } - diff, err := analysis.MapGitDiff(s.graph, repoRoot, scope, "main") + diff, err := analysis.MapGitDiff(s.graph, repoRoot, repoPrefix, scope, "main") if err != nil { return promptError("Could not analyze git changes: " + err.Error()), nil } diff --git a/internal/mcp/server.go b/internal/mcp/server.go index 4c21f674..71bc5da2 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -91,6 +91,11 @@ type Server struct { multiIndexer *indexer.MultiIndexer configManager *config.ConfigManager activeProject string + // testIndexProbe caches, per repo prefix, which test-path fragments the + // graph carries symbols for (see testFragmentsIndexed). The answer only + // changes with a reindex under a different exclude set, so + // daemon-lifetime caching is safe. + testIndexProbe sync.Map // scopeWorkspace / scopeProject default-scope every query at this // server instance to a single (workspace, project) tuple. Set by // `gortex server --workspace [--scope-project ]`. diff --git a/internal/mcp/tools_analysis.go b/internal/mcp/tools_analysis.go index a62e73d3..5ba39b63 100644 --- a/internal/mcp/tools_analysis.go +++ b/internal/mcp/tools_analysis.go @@ -35,6 +35,7 @@ func (s *Server) registerAnalysisTools() { mcp.WithDescription("Maps uncommitted git changes to symbols in the graph and runs blast radius analysis. The key pre-commit review tool."), mcp.WithString("scope", mcp.Description("unstaged (default), staged, all, or compare")), mcp.WithString("base_ref", mcp.Description("Branch/commit for compare scope (default: main)")), + mcp.WithString("repo", mcp.Description("Repository prefix or path (multi-repo mode); defaults to the lone tracked repo or the session's cwd-bound repo")), ), s.handleDetectChanges, ) @@ -234,15 +235,15 @@ func (s *Server) handleDetectChanges(ctx context.Context, req mcp.CallToolReques scope := req.GetString("scope", "unstaged") baseRef := req.GetString("base_ref", "main") - // Determine repo root from the indexer's last indexed path - repoRoot := "." - if s.indexer != nil { - if root := s.indexer.RootPath(); root != "" { - repoRoot = root - } + // Resolve the working tree: explicit repo selector, lone tracked repo, + // or the session's cwd-bound repo. The "." fallback keeps the standalone + // (indexer-less) server working from its own cwd. + repoRoot, repoPrefix := s.diffRepoScope(ctx, strings.TrimSpace(req.GetString("repo", ""))) + if repoRoot == "" { + repoRoot = "." } - diff, err := analysis.MapGitDiff(s.graph, repoRoot, scope, baseRef) + diff, err := analysis.MapGitDiff(s.graph, repoRoot, repoPrefix, scope, baseRef) if err != nil { return mcp.NewToolResultError(err.Error()), nil } diff --git a/internal/mcp/tools_conflicts.go b/internal/mcp/tools_conflicts.go index 0a1709db..d7a83a52 100644 --- a/internal/mcp/tools_conflicts.go +++ b/internal/mcp/tools_conflicts.go @@ -77,8 +77,7 @@ func (s *Server) handleConflictsPRs(ctx context.Context, req mcp.CallToolRequest if !forge.Available(ctx) && len(filesByNumber) == 0 { return s.respondJSONOrTOON(ctx, req, forgeUnavailablePayload()) } - roots := s.collectRepoRoots(repo) - repoRoot := pickRepoRoot(roots, repo) + repoRoot, _ := s.diffRepoScope(ctx, repo) fetched, ferr := forgeList(ctx, repoRoot, forge.ListOpts{State: "open", Limit: limit, WithCI: true}) if ferr != nil { if errors.Is(ferr, forge.ErrRateLimited) { @@ -111,6 +110,7 @@ func (s *Server) handleConflictsPRs(ctx context.Context, req mcp.CallToolRequest // Per-PR community fan-out and a per-PR risk score for the suggested // merge order. Both derive from the PR's changed-file set. + joinPrefix := s.prJoinPrefix(ctx, repo) prCommunities := map[int][]string{} prRisk := map[int]float64{} for _, pr := range prs { @@ -122,7 +122,7 @@ func (s *Server) handleConflictsPRs(ctx context.Context, req mcp.CallToolRequest return mcp.NewToolResultError(ferr.Error()), nil } - changedFiles, changedSymbolNodes := s.changedSymbolsForFiles(files) + changedFiles, changedSymbolNodes := s.changedSymbolsForFiles(joinPrefix, files) symbolIDs := make([]string, 0, len(changedSymbolNodes)) for _, n := range changedSymbolNodes { symbolIDs = append(symbolIDs, n.ID) diff --git a/internal/mcp/tools_core.go b/internal/mcp/tools_core.go index c3f04f46..9541e076 100644 --- a/internal/mcp/tools_core.go +++ b/internal/mcp/tools_core.go @@ -345,19 +345,36 @@ func subGraphToTOON(sg *query.SubGraph) (*mcp.CallToolResult, error) { // narrow. With no explicit narrowing the allow-set is every repo in // the session's workspace — not "all repos". // +// gitDiffScopes are the diff-selection values the review-family tools +// accept in their `scope` argument; they share the argument name with the +// saved-scope filter and are reserved (never saved-scope names). +var gitDiffScopes = map[string]bool{"unstaged": true, "staged": true, "all": true, "compare": true} + // For an unbound session (embedded stdio / `gortex server // --workspace` / legacy) it falls back to resolveRepoFilterArgs with // the active-project default applied. A nil result there still means // "no filter — all repos". func (s *Server) resolveRepoFilter(ctx context.Context, req mcp.CallToolRequest) (map[string]bool, error) { repo := req.GetString("repo", "") + // The selector may be a filesystem path (the CLI defaults to the + // caller's working directory) — normalize to the tracked prefix so the + // filter matches what the workspace knows the repo as. + if p := s.resolveRepoPrefix(repo); p != "" { + repo = p + } project := req.GetString("project", "") ref := req.GetString("ref", "") workspaceArg := req.GetString("workspace", "") // A named saved-scope supplies the repo allow-set when no explicit - // repo/project/ref narrows the call (see scopes.go). + // repo/project/ref narrows the call (see scopes.go). The review-family + // tools overload `scope` for git diff selection — those reserved values + // are never saved-scope names, otherwise every repo-less review call + // would fail with "unknown scope". scopeArg := req.GetString("scope", "") + if gitDiffScopes[scopeArg] { + scopeArg = "" + } var scopeRepos map[string]bool if scopeArg != "" && repo == "" && project == "" && ref == "" { sc, ok := s.lookupScope(scopeArg) diff --git a/internal/mcp/tools_critique_review.go b/internal/mcp/tools_critique_review.go index c02626cf..b5ac4d30 100644 --- a/internal/mcp/tools_critique_review.go +++ b/internal/mcp/tools_critique_review.go @@ -86,7 +86,7 @@ func (s *Server) handleCritiqueReview(ctx context.Context, req mcp.CallToolReque }) } - repoRoot := s.prReviewRepoRoot(req) + repoRoot := s.prReviewRepoRoot(ctx, req) diffText := strings.TrimSpace(req.GetString("diff", "")) findings, err := s.critiqueFindingsFor(ctx, req, repoRoot) @@ -181,8 +181,10 @@ func (s *Server) critiqueFindingsFor(ctx context.Context, req mcp.CallToolReques rulepack []astquery.Match impact map[string]*analysis.ImpactResult ) + repoPrefix := s.diffJoinPrefix(repoRoot) + var changedFiles []string if diffText == "" { - diff, err := analysis.MapGitDiff(s.graph, repoRoot, scope, baseRef) + diff, err := analysis.MapGitDiff(s.graph, repoRoot, repoPrefix, scope, baseRef) if err != nil { return nil, err } @@ -192,11 +194,14 @@ func (s *Server) critiqueFindingsFor(ctx context.Context, req mcp.CallToolReques } rulepack = s.reviewRulepackMatches(ctx, diff.ChangedFiles, allowedRepos) impact = s.reviewImpact(diff.ChangedSymbols) + changedFiles = diff.ChangedFiles } suppStore, suppRepoKey := s.reviewSuppressions() report, err := review.Run(ctx, s.graph, nil, review.Options{ RepoRoot: repoRoot, + RepoPrefix: repoPrefix, + CoverageKnown: s.coverageKnownForDiff(repoPrefix, changedFiles), Scope: scope, BaseRef: baseRef, Diff: diffText, diff --git a/internal/mcp/tools_enhancements.go b/internal/mcp/tools_enhancements.go index 386220fb..1b8be072 100644 --- a/internal/mcp/tools_enhancements.go +++ b/internal/mcp/tools_enhancements.go @@ -231,6 +231,7 @@ func (s *Server) registerEnhancementTools() { mcp.WithString("scope", mcp.Description("unstaged (default), staged, all, or compare")), mcp.WithString("base_ref", mcp.Description("Branch/commit for compare scope (default: main)")), mcp.WithBoolean("compact", mcp.Description("One-line-per-symbol condensed output")), + mcp.WithString("repo", mcp.Description("Repository prefix or path (multi-repo mode); defaults to the lone tracked repo or the session's cwd-bound repo")), ), s.handleDiffContext, ) @@ -2485,14 +2486,15 @@ func (s *Server) handleDiffContext(ctx context.Context, req mcp.CallToolRequest) scope := req.GetString("scope", "unstaged") baseRef := req.GetString("base_ref", "main") - repoRoot := "." - if s.indexer != nil { - if root := s.indexer.RootPath(); root != "" { - repoRoot = root - } + // Resolve the working tree: explicit repo selector, lone tracked repo, + // or the session's cwd-bound repo. The "." fallback keeps the standalone + // (indexer-less) server working from its own cwd. + repoRoot, repoPrefix := s.diffRepoScope(ctx, strings.TrimSpace(req.GetString("repo", ""))) + if repoRoot == "" { + repoRoot = "." } - diff, err := analysis.MapGitDiff(s.graph, repoRoot, scope, baseRef) + diff, err := analysis.MapGitDiff(s.graph, repoRoot, repoPrefix, scope, baseRef) if err != nil { return mcp.NewToolResultError(err.Error()), nil } diff --git a/internal/mcp/tools_multi.go b/internal/mcp/tools_multi.go index 1a95b71e..d5d9395b 100644 --- a/internal/mcp/tools_multi.go +++ b/internal/mcp/tools_multi.go @@ -277,7 +277,7 @@ func (s *Server) buildActiveProjectPayload(ctx context.Context) map[string]any { // resolveRepoPrefixOrReconcile when drift between persisted config and // in-memory state could produce a false miss. func (s *Server) resolveRepoPrefix(pathOrPrefix string) string { - if s.multiIndexer == nil { + if s.multiIndexer == nil || pathOrPrefix == "" { return "" } @@ -298,6 +298,56 @@ func (s *Server) resolveRepoPrefix(pathOrPrefix string) string { return "" } +// diffJoinPrefix resolves the graph repo prefix used to join repo-relative +// diff / forge file paths to indexed nodes: multi-repo daemons key file +// paths as "/" while git and forge APIs emit repo-relative +// paths. repoRoot is the already-resolved working-tree root. Returns "" in +// single-repo / unprefixed mode, where the raw lookup already matches. +func (s *Server) diffJoinPrefix(repoRoot string) string { + if repoRoot == "" { + return "" + } + if p := s.resolveRepoPrefix(repoRoot); p != "" { + return p + } + if s.indexer != nil && s.indexer.RootPath() == repoRoot { + return s.indexer.RepoPrefix() + } + return "" +} + +// diffRepoScope resolves the working-tree root and graph repo prefix a +// diff-driven handler operates on. An explicit selector (a repo prefix or a +// filesystem path — the CLI defaults to the caller's working directory) is +// normalized and honoured exclusively: when it names nothing tracked the +// result is empty so the caller errors instead of silently diffing another +// repo. With no selector the lone tracked repo wins, then the session's +// cwd-bound repo (clients dial the daemon with their working directory). +// Both empty means no resolvable working tree. +func (s *Server) diffRepoScope(ctx context.Context, repo string) (repoRoot, repoPrefix string) { + if repo != "" { + if p := s.resolveRepoPrefix(repo); p != "" { + repo = p + } + root := pickRepoRoot(s.collectRepoRoots(repo), repo) + if root == "" { + return "", "" + } + return root, s.diffJoinPrefix(root) + } + if root := pickRepoRoot(s.collectRepoRoots(""), ""); root != "" { + return root, s.diffJoinPrefix(root) + } + if cwd := SessionCWDFromContext(ctx); cwd != "" && s.multiIndexer != nil { + if _, _, prefix, ok := s.multiIndexer.ScopeForCWD(cwd); ok && prefix != "" { + if root, ok := s.multiIndexer.RepoRoot(prefix); ok && root != "" { + return root, prefix + } + } + } + return "", "" +} + // resolveRepoPrefixOrReconcile resolves a path-or-prefix to a repo prefix // and reconciles persisted-config state into the in-memory MultiIndexer on // miss. Warmup can silently drop a repo (transient index failure, daemon diff --git a/internal/mcp/tools_pr_review_context.go b/internal/mcp/tools_pr_review_context.go index cbc71edc..937743e5 100644 --- a/internal/mcp/tools_pr_review_context.go +++ b/internal/mcp/tools_pr_review_context.go @@ -169,7 +169,7 @@ func (s *Server) handlePRReviewContext(ctx context.Context, req mcp.CallToolRequ wantDiffCtx := wantSection("diff_context") wantSimulate := wantSection("simulate") - repoRoot := s.prReviewRepoRoot(req) + repoRoot := s.prReviewRepoRoot(ctx, req) scope, baseRef := siblingDiffScope(req) @@ -188,7 +188,7 @@ func (s *Server) handlePRReviewContext(ctx context.Context, req mcp.CallToolRequ if repoRoot == "" { return mcp.NewToolResultError("could not resolve a repository root for the changeset diff"), nil } - d, err := analysis.MapGitDiff(s.graph, repoRoot, scope, baseRef) + d, err := analysis.MapGitDiff(s.graph, repoRoot, s.diffJoinPrefix(repoRoot), scope, baseRef) if err != nil { return mcp.NewToolResultError(err.Error()), nil } @@ -300,17 +300,10 @@ func (s *Server) handlePRReviewContext(ctx context.Context, req mcp.CallToolRequ } // prReviewRepoRoot resolves the working-tree root for the changeset diff, -// mirroring the review tools' resolution order: an explicit repo prefix's -// root, then the single indexer's root. -func (s *Server) prReviewRepoRoot(req mcp.CallToolRequest) string { - repo := strings.TrimSpace(req.GetString("repo", "")) - roots := s.collectRepoRoots(repo) - repoRoot := pickRepoRoot(roots, repo) - if repoRoot == "" && s.indexer != nil { - if root := s.indexer.RootPath(); root != "" { - repoRoot = root - } - } +// mirroring the review tools' resolution order: an explicit repo selector +// (prefix or path), the lone tracked repo, or the session's cwd-bound repo. +func (s *Server) prReviewRepoRoot(ctx context.Context, req mcp.CallToolRequest) string { + repoRoot, _ := s.diffRepoScope(ctx, strings.TrimSpace(req.GetString("repo", ""))) return repoRoot } diff --git a/internal/mcp/tools_pr_risk.go b/internal/mcp/tools_pr_risk.go index 4e7b6485..84fb1d53 100644 --- a/internal/mcp/tools_pr_risk.go +++ b/internal/mcp/tools_pr_risk.go @@ -60,12 +60,11 @@ func (s *Server) handlePRRisk(ctx context.Context, req mcp.CallToolRequest) (*mc } } case base != "": - roots := s.collectRepoRoots(repo) - root := pickRepoRoot(roots, repo) + root, prefix := s.diffRepoScope(ctx, repo) if root == "" { return mcp.NewToolResultError("could not resolve a repository root for the base diff"), nil } - diff, err := analysis.MapGitDiff(s.graph, root, "compare", base) + diff, err := analysis.MapGitDiff(s.graph, root, prefix, "compare", base) if err != nil { return mcp.NewToolResultError(fmt.Sprintf("git diff against %q failed: %v", base, err)), nil } diff --git a/internal/mcp/tools_prs.go b/internal/mcp/tools_prs.go index 5700050d..2b354d1f 100644 --- a/internal/mcp/tools_prs.go +++ b/internal/mcp/tools_prs.go @@ -251,8 +251,7 @@ func (s *Server) handleListPRs(ctx context.Context, req mcp.CallToolRequest) (*m if !forge.Available(ctx) { return s.respondJSONOrTOON(ctx, req, forgeUnavailablePayload()) } - roots := s.collectRepoRoots(repo) - repoRoot := pickRepoRoot(roots, repo) + repoRoot, _ := s.diffRepoScope(ctx, repo) fetched, ferr := forgeList(ctx, repoRoot, forge.ListOpts{State: state, Limit: limit, WithDecision: true, WithCI: true}) if ferr != nil { if errors.Is(ferr, forge.ErrRateLimited) { @@ -341,7 +340,7 @@ func (s *Server) handleGetPRImpact(ctx context.Context, req mcp.CallToolRequest) files = fetched } - payload := s.prImpactForNumber(ctx, number, files, receipt, scrub) + payload := s.prImpactForNumber(ctx, number, s.prJoinPrefix(ctx, repo), files, receipt, scrub) if s.isGCX(ctx, req) { return s.gcxResponseWithBudget(req)(encodePRImpact(payload)) @@ -356,8 +355,10 @@ func (s *Server) handleGetPRImpact(ctx context.Context, req mcp.CallToolRequest) // changed file set: file→symbol join, PR-risk score, and community / blast // grouping. receipt adds a privacy-safe review receipt projected from the // same risk result. It performs no network I/O — the files are supplied. -func (s *Server) prImpactForNumber(ctx context.Context, number int, files []string, receipt, scrub bool) map[string]any { - changedFiles, changedSymbolNodes := s.changedSymbolsForFiles(files) +// repoPrefix anchors the file→symbol join in multi-repo mode (see +// prJoinPrefix). +func (s *Server) prImpactForNumber(ctx context.Context, number int, repoPrefix string, files []string, receipt, scrub bool) map[string]any { + changedFiles, changedSymbolNodes := s.changedSymbolsForFiles(repoPrefix, files) symbolIDs := make([]string, 0, len(changedSymbolNodes)) for _, n := range changedSymbolNodes { symbolIDs = append(symbolIDs, n.ID) @@ -431,11 +432,13 @@ func (s *Server) prImpactForNumber(ctx context.Context, number int, files []stri } // changedSymbolsForFiles maps a set of changed file paths to the code -// symbols those files define, via GetFileNodes (whole-file granularity — -// the coarse mapping a not-checked-out PR allows). Returns the deduped -// non-empty file list and the deduped symbol nodes (file nodes excluded), -// both deterministically ordered. -func (s *Server) changedSymbolsForFiles(files []string) ([]string, []*graph.Node) { +// symbols those files define, via the prefix-aware file→node join +// (whole-file granularity — the coarse mapping a not-checked-out PR +// allows). Forge APIs return repo-relative paths while multi-repo daemons +// key file paths as "/"; repoPrefix bridges the two. Returns +// the deduped non-empty file list and the deduped symbol nodes (file nodes +// excluded), both deterministically ordered. +func (s *Server) changedSymbolsForFiles(repoPrefix string, files []string) ([]string, []*graph.Node) { fileSeen := map[string]bool{} var changedFiles []string nodeSeen := map[string]bool{} @@ -447,7 +450,7 @@ func (s *Server) changedSymbolsForFiles(files []string) ([]string, []*graph.Node } fileSeen[f] = true changedFiles = append(changedFiles, f) - for _, n := range s.graph.GetFileNodes(f) { + for _, n := range analysis.JoinFileNodes(s.graph, repoPrefix, f) { if n == nil || n.Kind == graph.KindFile { continue } @@ -462,6 +465,17 @@ func (s *Server) changedSymbolsForFiles(files []string) ([]string, []*graph.Node return changedFiles, nodes } +// prJoinPrefix resolves the graph repo prefix for the forge-file → symbol +// join via the shared diff-handler scope resolution: the caller's repo +// selector (prefix or path), the lone tracked repo, or the session's +// cwd-bound repo (the CLI dials the daemon with its working directory, so +// `gortex prs ` run inside a tracked repo joins against that repo +// without an explicit selector). Empty in single-repo / unprefixed mode. +func (s *Server) prJoinPrefix(ctx context.Context, repo string) string { + _, prefix := s.diffRepoScope(ctx, repo) + return prefix +} + // handleTriagePRs ranks a repository's open PRs by get_pr_impact score // (highest first, deterministic). The PR list and per-PR files come from // supplied maps or a self-served forge fan-out. @@ -489,8 +503,7 @@ func (s *Server) handleTriagePRs(ctx context.Context, req mcp.CallToolRequest) ( if !forge.Available(ctx) && len(filesByNumber) == 0 { return s.respondJSONOrTOON(ctx, req, forgeUnavailablePayload()) } - roots := s.collectRepoRoots(repo) - repoRoot := pickRepoRoot(roots, repo) + repoRoot, _ := s.diffRepoScope(ctx, repo) fetched, ferr := forgeList(ctx, repoRoot, forge.ListOpts{State: "open", Limit: limit, WithCI: true}) if ferr != nil { if errors.Is(ferr, forge.ErrRateLimited) { @@ -511,6 +524,7 @@ func (s *Server) handleTriagePRs(ctx context.Context, req mcp.CallToolRequest) ( prs = prs[:limit] } + joinPrefix := s.prJoinPrefix(ctx, repo) ranked := make([]map[string]any, 0, len(prs)) for _, pr := range prs { files, degraded, ferr := s.resolvePRFiles(ctx, repo, pr, filesByNumber) @@ -520,7 +534,7 @@ func (s *Server) handleTriagePRs(ctx context.Context, req mcp.CallToolRequest) ( if ferr != nil { return mcp.NewToolResultError(ferr.Error()), nil } - impact := s.prImpactForNumber(ctx, pr.Number, files, false, false) + impact := s.prImpactForNumber(ctx, pr.Number, joinPrefix, files, false, false) ranked = append(ranked, map[string]any{ "number": pr.Number, "title": pr.Title, @@ -800,8 +814,7 @@ func (s *Server) fetchPRFiles(ctx context.Context, repo string, number int) (fil if !forge.Available(ctx) { return nil, forgeUnavailablePayload(), nil } - roots := s.collectRepoRoots(repo) - repoRoot := pickRepoRoot(roots, repo) + repoRoot, _ := s.diffRepoScope(ctx, repo) fetched, ferr := forgeFiles(ctx, repoRoot, number) if ferr != nil { if errors.Is(ferr, forge.ErrRateLimited) { diff --git a/internal/mcp/tools_prs_test.go b/internal/mcp/tools_prs_test.go index 111a2d5c..eb713886 100644 --- a/internal/mcp/tools_prs_test.go +++ b/internal/mcp/tools_prs_test.go @@ -171,6 +171,35 @@ func TestGetPRImpact_RequiresNumber(t *testing.T) { require.True(t, res.IsError, "expected an error when number is missing") } +// --- multi-repo prefix join -------------------------------------------------- + +// TestChangedSymbolsForFiles_RepoPrefixJoin covers the multi-repo daemon +// shape: indexed file paths carry the repo prefix ("myrepo/") while +// forge file lists are repo-relative. The join must retry with the prefix, +// stay raw-first for unprefixed graphs, and never double-prefix. +func TestChangedSymbolsForFiles_RepoPrefixJoin(t *testing.T) { + g := graph.New() + prefixedID := "myrepo/internal/auth/login.go::ValidateToken" + g.AddNode(&graph.Node{ID: prefixedID, Kind: graph.KindFunction, Name: "ValidateToken", FilePath: "myrepo/internal/auth/login.go", StartLine: 1, EndLine: 10}) + srv := NewServer(query.NewEngine(g), g, nil, nil, zap.NewNop(), nil) + + files, nodes := srv.changedSymbolsForFiles("myrepo", []string{"internal/auth/login.go"}) + require.Equal(t, []string{"internal/auth/login.go"}, files, + "the reported changed files keep the forge-relative paths") + require.Len(t, nodes, 1, "the prefixed retry must find the symbol") + require.Equal(t, prefixedID, nodes[0].ID) + + // Without a prefix the relative path misses — the unprefixed single-repo + // graph shape keeps its exact-match behavior. + _, nodes = srv.changedSymbolsForFiles("", []string{"internal/auth/login.go"}) + require.Empty(t, nodes) + + // Already-prefixed input hits raw and is not double-prefixed. + _, nodes = srv.changedSymbolsForFiles("myrepo", []string{"myrepo/internal/auth/login.go"}) + require.Len(t, nodes, 1) + require.Equal(t, prefixedID, nodes[0].ID) +} + // --- list_prs: classification ---------------------------------------------- func TestListPRs_ClassifiesSupplied(t *testing.T) { diff --git a/internal/mcp/tools_review.go b/internal/mcp/tools_review.go index f4f15d44..ccca668b 100644 --- a/internal/mcp/tools_review.go +++ b/internal/mcp/tools_review.go @@ -15,6 +15,7 @@ import ( "github.com/zzet/gortex/internal/astquery" "github.com/zzet/gortex/internal/config" "github.com/zzet/gortex/internal/gitcmd" + "github.com/zzet/gortex/internal/graph" "github.com/zzet/gortex/internal/llm" "github.com/zzet/gortex/internal/query" "github.com/zzet/gortex/internal/review" @@ -246,21 +247,13 @@ func (s *Server) handleSiblingDiffContext(ctx context.Context, req mcp.CallToolR scope, baseRef := siblingDiffScope(req) repo := strings.TrimSpace(req.GetString("repo", "")) - roots := s.collectRepoRoots(repo) - repoRoot := pickRepoRoot(roots, repo) - if repoRoot == "" { - if s.indexer != nil { - if root := s.indexer.RootPath(); root != "" { - repoRoot = root - } - } - } + repoRoot, repoPrefix := s.diffRepoScope(ctx, repo) if repoRoot == "" { return mcp.NewToolResultError("could not resolve a repository root for the changeset diff"), nil } // Enumerate the whole changeset. - diff, err := analysis.MapGitDiff(s.graph, repoRoot, scope, baseRef) + diff, err := analysis.MapGitDiff(s.graph, repoRoot, repoPrefix, scope, baseRef) if err != nil { return mcp.NewToolResultError(err.Error()), nil } @@ -502,19 +495,9 @@ func (s *Server) rawFileDiff(ctx context.Context, repoRoot, scope, baseRef, file // siblingDiffArgs mirrors the analysis diff-arg selection but emits a context // window (unified=3) so the raw sibling diff carries readable surrounding lines. func siblingDiffArgs(scope, baseRef string) []string { - switch scope { - case "staged": - return []string{"diff", "--cached", "--unified=3"} - case "all": - return []string{"diff", "HEAD", "--unified=3"} - case "compare": - if baseRef == "" { - baseRef = "main" - } - return []string{"diff", baseRef + "...HEAD", "--unified=3"} - default: // unstaged - return []string{"diff", "--unified=3"} - } + // analysis.GitDiffArgs pins the a/ b/ header prefixes the diff parsers + // anchor on (diff.mnemonicPrefix / diff.noprefix would zero them out). + return analysis.GitDiffArgs(scope, baseRef, 3) } // siblingDiffPayload projects the ranked siblings onto the wire shape. @@ -554,13 +537,7 @@ func (s *Server) handleReview(ctx context.Context, req mcp.CallToolRequest) (*mc scope, baseRef := siblingDiffScope(req) repo := strings.TrimSpace(req.GetString("repo", "")) - roots := s.collectRepoRoots(repo) - repoRoot := pickRepoRoot(roots, repo) - if repoRoot == "" && s.indexer != nil { - if root := s.indexer.RootPath(); root != "" { - repoRoot = root - } - } + repoRoot, repoPrefix := s.diffRepoScope(ctx, repo) // An on-disk review needs a working tree; a pasted-diff review does not. if repoRoot == "" && diffText == "" { return mcp.NewToolResultError("could not resolve a repository root for the changeset diff"), nil @@ -571,11 +548,12 @@ func (s *Server) handleReview(ctx context.Context, req mcp.CallToolRequest) (*mc // diff there is no git changeset to scan, so both stay empty and review.Run // degrades to the diff-window substrate. var ( - rulepack []astquery.Match - impact map[string]*analysis.ImpactResult + rulepack []astquery.Match + impact map[string]*analysis.ImpactResult + changedFiles []string ) if diffText == "" { - diff, err := analysis.MapGitDiff(s.graph, repoRoot, scope, baseRef) + diff, err := analysis.MapGitDiff(s.graph, repoRoot, repoPrefix, scope, baseRef) if err != nil { return mcp.NewToolResultError(err.Error()), nil } @@ -585,6 +563,7 @@ func (s *Server) handleReview(ctx context.Context, req mcp.CallToolRequest) (*mc } rulepack = s.reviewRulepackMatches(ctx, diff.ChangedFiles, allowedRepos) impact = s.reviewImpact(diff.ChangedSymbols) + changedFiles = diff.ChangedFiles } // LLM seam: a closure over the optional LLM service's Generate, gated on the @@ -596,6 +575,8 @@ func (s *Server) handleReview(ctx context.Context, req mcp.CallToolRequest) (*mc suppStore, suppRepoKey := s.reviewSuppressions() report, err := review.RunWithUsage(ctx, s.graph, gen, s.reviewPricing(), review.Options{ RepoRoot: repoRoot, + RepoPrefix: repoPrefix, + CoverageKnown: s.coverageKnownForDiff(repoPrefix, changedFiles), Scope: scope, BaseRef: baseRef, Diff: diffText, @@ -693,6 +674,87 @@ func (s *Server) reviewRulepackMatches(ctx context.Context, changedFiles []strin // reviewImpact builds the per-changed-symbol blast-radius map review.Run uses to // rank per-file risk. A symbol whose impact analysis is empty is omitted. +// testPatternsByExt maps a source-file extension to the path fragments its +// covering tests conventionally carry. Only languages with a recognizable +// convention are probed; a diff touching none of them reads as +// coverage-unknown. +var testPatternsByExt = map[string][]string{ + ".go": {"_test.go"}, + ".ts": {".test.ts", ".spec.ts", "__tests__/"}, + ".tsx": {".test.ts", ".spec.ts", "__tests__/"}, + ".js": {".test.js", ".spec.js", "__tests__/"}, + ".jsx": {".test.js", ".spec.js", "__tests__/"}, + ".py": {"test_"}, +} + +// testFragmentsIndexed reports, per test-path fragment, whether the repo's +// graph carries any non-file symbol whose path contains it. One scan, +// cached for the daemon's lifetime — the index's exclude set only changes +// with a reindex. +func (s *Server) testFragmentsIndexed(repoPrefix string) map[string]bool { + if v, ok := s.testIndexProbe.Load(repoPrefix); ok { + return v.(map[string]bool) + } + fragments := map[string]bool{} + for _, pats := range testPatternsByExt { + for _, p := range pats { + fragments[p] = false + } + } + var nodes []*graph.Node + if repoPrefix != "" { + nodes = s.graph.GetRepoNodes(repoPrefix) + } else { + nodes = s.graph.AllNodes() + } + remaining := len(fragments) + for _, n := range nodes { + if n == nil || n.Kind == graph.KindFile || !analysis.IsTestFile(n.FilePath) { + continue + } + for p, seen := range fragments { + if !seen && strings.Contains(n.FilePath, p) { + fragments[p] = true + remaining-- + } + } + if remaining == 0 { + break + } + } + s.testIndexProbe.Store(repoPrefix, fragments) + return fragments +} + +// coverageKnownForDiff reports whether the graph can attest test coverage +// for this changeset: every changed file whose language has a test +// convention must have that convention present in the index. An index +// config that excludes a language's test files (e.g. "**/*_test.go") makes +// "no covering test" blindness, not a finding — the review then says +// "coverage unknown" instead of "untested". +func (s *Server) coverageKnownForDiff(repoPrefix string, changedFiles []string) bool { + fragments := s.testFragmentsIndexed(repoPrefix) + sawCode := false + for _, f := range changedFiles { + pats := testPatternsByExt[strings.ToLower(filepath.Ext(f))] + if len(pats) == 0 { + continue + } + sawCode = true + ok := false + for _, p := range pats { + if fragments[p] { + ok = true + break + } + } + if !ok { + return false + } + } + return sawCode +} + func (s *Server) reviewImpact(changed []analysis.ChangedSymbol) map[string]*analysis.ImpactResult { if len(changed) == 0 { return nil @@ -834,9 +896,12 @@ func reviewPayload(report *review.ReviewReport) map[string]any { } for _, fr := range report.FileRisk { fileRisk = append(fileRisk, map[string]any{ - "file": fr.File, - "risk": fr.Risk, - "findings": fr.Findings, + "file": fr.File, + "risk": fr.Risk, + "findings": fr.Findings, + "affected": fr.Affected, + "symbols": fr.Symbols, + "uncovered": fr.Uncovered, }) } } @@ -964,13 +1029,7 @@ func (s *Server) handleReviewPack(ctx context.Context, req mcp.CallToolRequest) scope, baseRef := siblingDiffScope(req) repo := strings.TrimSpace(req.GetString("repo", "")) - roots := s.collectRepoRoots(repo) - repoRoot := pickRepoRoot(roots, repo) - if repoRoot == "" && s.indexer != nil { - if root := s.indexer.RootPath(); root != "" { - repoRoot = root - } - } + repoRoot, repoPrefix := s.diffRepoScope(ctx, repo) if repoRoot == "" && diffText == "" { return mcp.NewToolResultError("could not resolve a repository root for the changeset diff"), nil } @@ -984,7 +1043,7 @@ func (s *Server) handleReviewPack(ctx context.Context, req mcp.CallToolRequest) ids []string ) if diffText == "" { - d, err := analysis.MapGitDiff(s.graph, repoRoot, scope, baseRef) + d, err := analysis.MapGitDiff(s.graph, repoRoot, repoPrefix, scope, baseRef) if err != nil { return mcp.NewToolResultError(err.Error()), nil } @@ -1008,6 +1067,8 @@ func (s *Server) handleReviewPack(ctx context.Context, req mcp.CallToolRequest) suppStore, suppRepoKey := s.reviewSuppressions() report, err := review.RunWithUsage(ctx, s.graph, gen, s.reviewPricing(), review.Options{ RepoRoot: repoRoot, + RepoPrefix: repoPrefix, + CoverageKnown: diff != nil && s.coverageKnownForDiff(repoPrefix, diff.ChangedFiles), Scope: scope, BaseRef: baseRef, Diff: diffText, @@ -1122,7 +1183,7 @@ func (s *Server) reviewChangeView(diff *analysis.DiffResult) (*review.ChangeView if s.indexer != nil { repoRoot = s.indexer.RootPath() } - view, err := review.BuildChangeView(s.graph, repoRoot, "", "") + view, err := review.BuildChangeView(s.graph, repoRoot, s.diffJoinPrefix(repoRoot), "", "") if err != nil { return nil, diff } @@ -1357,9 +1418,12 @@ func reviewPackPayload(env reviewEnvelope) map[string]any { fileRisk := make([]map[string]any, 0, len(env.FileRisk)) for _, fr := range env.FileRisk { fileRisk = append(fileRisk, map[string]any{ - "file": fr.File, - "risk": fr.Risk, - "findings": fr.Findings, + "file": fr.File, + "risk": fr.Risk, + "findings": fr.Findings, + "affected": fr.Affected, + "symbols": fr.Symbols, + "uncovered": fr.Uncovered, }) } findings := make([]map[string]any, 0, len(env.Findings)) diff --git a/internal/mcp/tools_review_post.go b/internal/mcp/tools_review_post.go index 19705322..9c4e7872 100644 --- a/internal/mcp/tools_review_post.go +++ b/internal/mcp/tools_review_post.go @@ -43,13 +43,7 @@ func (s *Server) handlePostReview(ctx context.Context, req mcp.CallToolRequest) } repo := strings.TrimSpace(req.GetString("repo", "")) - roots := s.collectRepoRoots(repo) - repoRoot := pickRepoRoot(roots, repo) - if repoRoot == "" && s.indexer != nil { - if root := s.indexer.RootPath(); root != "" { - repoRoot = root - } - } + repoRoot, _ := s.diffRepoScope(ctx, repo) findings, err := s.postReviewFindingsFor(ctx, req, repoRoot) if err != nil { @@ -140,8 +134,10 @@ func (s *Server) postReviewFindingsFor(ctx context.Context, req mcp.CallToolRequ rulepack []astquery.Match impact map[string]*analysis.ImpactResult ) + repoPrefix := s.diffJoinPrefix(repoRoot) + var changedFiles []string if diffText == "" { - diff, err := analysis.MapGitDiff(s.graph, repoRoot, scope, baseRef) + diff, err := analysis.MapGitDiff(s.graph, repoRoot, repoPrefix, scope, baseRef) if err != nil { return nil, err } @@ -151,11 +147,14 @@ func (s *Server) postReviewFindingsFor(ctx context.Context, req mcp.CallToolRequ } rulepack = s.reviewRulepackMatches(ctx, diff.ChangedFiles, allowedRepos) impact = s.reviewImpact(diff.ChangedSymbols) + changedFiles = diff.ChangedFiles } suppStore, suppRepoKey := s.reviewSuppressions() report, err := review.Run(ctx, s.graph, nil, review.Options{ RepoRoot: repoRoot, + RepoPrefix: repoPrefix, + CoverageKnown: s.coverageKnownForDiff(repoPrefix, changedFiles), Scope: scope, BaseRef: baseRef, Diff: diffText, diff --git a/internal/mcp/tools_review_questions.go b/internal/mcp/tools_review_questions.go index 590191b7..ce2c4e5a 100644 --- a/internal/mcp/tools_review_questions.go +++ b/internal/mcp/tools_review_questions.go @@ -222,11 +222,11 @@ func (s *Server) resolveReviewQuestionTargets(ctx context.Context, req mcp.CallT case base != "": repo := strings.TrimSpace(req.GetString("repo", "")) - repoRoot := pickRepoRoot(s.collectRepoRoots(repo), repo) + repoRoot, repoPrefix := s.diffRepoScope(ctx, repo) if repoRoot == "" { return nil, errReviewQuestionsNoRoot } - diff, derr := analysis.MapGitDiff(s.graph, repoRoot, "compare", base) + diff, derr := analysis.MapGitDiff(s.graph, repoRoot, repoPrefix, "compare", base) if derr != nil { return nil, derr } diff --git a/internal/mcp/tools_suggest_reviewers.go b/internal/mcp/tools_suggest_reviewers.go index e5dcb17c..7e4c064b 100644 --- a/internal/mcp/tools_suggest_reviewers.go +++ b/internal/mcp/tools_suggest_reviewers.go @@ -71,8 +71,7 @@ func (s *Server) handleSuggestReviewers(ctx context.Context, req mcp.CallToolReq } repo := strings.TrimSpace(req.GetString("repo", "")) - roots := s.collectRepoRoots(repo) - repoRoot := pickRepoRoot(roots, repo) + repoRoot, repoPrefix := s.diffRepoScope(ctx, repo) changedFiles, _, err := s.resolveReviewerChangeset(ctx, req, repoRoot) if err != nil { @@ -100,10 +99,12 @@ func (s *Server) handleSuggestReviewers(ctx context.Context, req mcp.CallToolReq } // Ownership signal — recent authors of the changed symbols / files. + // The changed-file paths are repo-relative (git / forge), so the node + // join is prefix-aware in multi-repo mode. blame := blameRowsByID(s.graph) authorCounts := map[string]int{} for _, f := range changedFiles { - for _, n := range s.graph.GetFileNodes(f) { + for _, n := range analysis.JoinFileNodes(s.graph, repoPrefix, f) { if la, ok := lastAuthoredFrom(blame, n); ok && la.Email != "" { authorCounts[normalizeReviewer(la.Email)]++ } @@ -115,7 +116,7 @@ func (s *Server) handleSuggestReviewers(ctx context.Context, req mcp.CallToolReq // candidate experts; the count is the number of co-change links. coChangeCounts := map[string]int{} for _, f := range changedFiles { - for partner := range s.coChangeScores(f) { + for partner := range s.coChangeScores(analysis.JoinFilePath(s.graph, repoPrefix, f)) { for _, n := range s.graph.GetFileNodes(partner) { if la, ok := lastAuthoredFrom(blame, n); ok && la.Email != "" { coChangeCounts[normalizeReviewer(la.Email)]++ @@ -168,7 +169,7 @@ func (s *Server) resolveReviewerChangeset(ctx context.Context, req mcp.CallToolR if repoRoot == "" { return nil, nil, fmt.Errorf("could not resolve a repository root for the base diff") } - diff, derr := analysis.MapGitDiff(s.graph, repoRoot, "compare", base) + diff, derr := analysis.MapGitDiff(s.graph, repoRoot, s.diffJoinPrefix(repoRoot), "compare", base) if derr != nil { return nil, nil, fmt.Errorf("git diff against %q failed: %v", base, derr) } diff --git a/internal/review/anchor.go b/internal/review/anchor.go index 917b33a9..fcf11987 100644 --- a/internal/review/anchor.go +++ b/internal/review/anchor.go @@ -71,9 +71,10 @@ type Anchor struct { // new-side substrate (analysis.MapGitDiffWithLines) and parsing the removed // old-side lines from the same diff. The graph supplies the changed-symbol // spans inside MapGitDiffWithLines; it may be nil (then only the line text is -// available, which is all the resolver needs). -func BuildChangeView(g graph.Store, repoRoot, scope, baseRef string) (*ChangeView, error) { - _, newLines, err := analysis.MapGitDiffWithLines(g, repoRoot, scope, baseRef) +// available, which is all the resolver needs). repoPrefix anchors the node +// join in multi-repo mode (see analysis.MapGitDiff). +func BuildChangeView(g graph.Store, repoRoot, repoPrefix, scope, baseRef string) (*ChangeView, error) { + _, newLines, err := analysis.MapGitDiffWithLines(g, repoRoot, repoPrefix, scope, baseRef) if err != nil { return nil, err } @@ -515,24 +516,9 @@ func parseHunkStart(line, sidePrefix string) (int, bool) { // rawDiff runs the same context-bearing diff MapGitDiffWithLines uses so the // old-side lines can be recovered. Routed through gitcmd (no ad-hoc exec). +// analysis.GitDiffArgs pins the a/ b/ header prefixes the parsers anchor on. func rawDiff(repoRoot, scope, baseRef string) (string, error) { - return gitcmd.Output(context.Background(), repoRoot, diffArgs(scope, baseRef)...) -} - -func diffArgs(scope, baseRef string) []string { - switch scope { - case "staged": - return []string{"diff", "--cached", "--unified=3"} - case "all": - return []string{"diff", "HEAD", "--unified=3"} - case "compare": - if baseRef == "" { - baseRef = "main" - } - return []string{"diff", baseRef + "...HEAD", "--unified=3"} - default: - return []string{"diff", "--unified=3"} - } + return gitcmd.Output(context.Background(), repoRoot, analysis.GitDiffArgs(scope, baseRef, 3)...) } // readFile returns the post-change file content from disk under RepoRoot. diff --git a/internal/review/flow.go b/internal/review/flow.go index 3158905b..da989e45 100644 --- a/internal/review/flow.go +++ b/internal/review/flow.go @@ -24,6 +24,16 @@ type Options struct { // RepoRoot is the repository root the change is read from. May be empty for // the pasted-diff (off-disk) path, in which case Diff must be set. RepoRoot string + // RepoPrefix is the graph repo prefix anchoring RepoRoot's indexed nodes — + // multi-repo daemons key file paths as "/" while git emits + // repo-relative paths. Empty in single-repo / unprefixed mode. + RepoPrefix string + // CoverageKnown reports whether the graph indexes test symbols for this + // repo at all. When false the per-file coverage evidence is unknowable — + // an index config that excludes "**/*_test.go" would otherwise make + // every change read as untested — so the rows carry no untested counts + // and the headline says "coverage unknown" instead. + CoverageKnown bool // Scope selects the git diff range: "staged", "all", "compare", or // "unstaged" (default). Ignored when Diff is set. Scope string @@ -211,11 +221,11 @@ func buildSubstrate(g graph.Store, opts Options) (*ChangeView, *analysis.DiffRes return view, diffFromView(view), nil } - view, err := BuildChangeView(g, opts.RepoRoot, opts.Scope, opts.BaseRef) + view, err := BuildChangeView(g, opts.RepoRoot, opts.RepoPrefix, opts.Scope, opts.BaseRef) if err != nil { return nil, nil, err } - diff, err := analysis.MapGitDiff(g, opts.RepoRoot, opts.Scope, opts.BaseRef) + diff, err := analysis.MapGitDiff(g, opts.RepoRoot, opts.RepoPrefix, opts.Scope, opts.BaseRef) if err != nil { return nil, nil, err } @@ -379,7 +389,7 @@ func compress(opts Options, plan *reviewPlan, llmFindings []Finding, dropped int WithSuppression(opts.Suppressions, opts.RepoKey). Apply(merged) - fileRisk := rankFileRisk(plan.diff, opts.Impact, merged) + fileRisk := rankFileRisk(plan.diff, opts.Impact, merged, opts.RepoPrefix, opts.CoverageKnown) verdict := computeVerdict(merged, fileRisk) bySeverity := map[string]int{} @@ -439,9 +449,40 @@ func sortFindings(findings []Finding) { }) } -// summarize produces the one-line report headline. +// summarize produces the one-line report headline. With no findings a +// non-APPROVE verdict is risk-driven (computeVerdict escalates on per-file +// blast radius, tempered by test coverage), so the headline says which tier +// earned it — and whether the risk is untested — instead of the +// self-contradictory "BLOCK: no findings". func summarize(verdict Verdict, findings []Finding, fileRisk []FileRisk) string { if len(findings) == 0 { + if verdict != VerdictApprove && len(fileRisk) > 0 { + worst := fileRisk[0].Risk // sorted worst-first + n, untested, known := 0, 0, false + for _, fr := range fileRisk { + if fr.Symbols > 0 { + known = true + } + if fr.Risk != worst { + continue + } + n++ + if fr.Symbols > 0 && fr.Uncovered > 0 { + untested++ + } + } + switch { + case untested > 0: + return fmt.Sprintf("%s: no rule findings, but %d of %d changed file(s) carry %s blast-radius risk (%d without covering tests)", + verdict, n, len(fileRisk), worst, untested) + case known: + return fmt.Sprintf("%s: no rule findings; %d of %d changed file(s) carry %s blast-radius risk, all test-covered", + verdict, n, len(fileRisk), worst) + default: + return fmt.Sprintf("%s: no rule findings, but %d of %d changed file(s) carry %s blast-radius risk (test coverage unknown — the index carries no test symbols)", + verdict, n, len(fileRisk), worst) + } + } return fmt.Sprintf("%s: no findings across %d changed file(s)", verdict, len(fileRisk)) } return fmt.Sprintf("%s: %d finding(s) across %d changed file(s)", verdict, len(findings), len(fileRisk)) diff --git a/internal/review/flow_test.go b/internal/review/flow_test.go index 7223796f..4601b077 100644 --- a/internal/review/flow_test.go +++ b/internal/review/flow_test.go @@ -235,9 +235,105 @@ func TestRankFileRiskUsesImpact(t *testing.T) { "app/a.go::A": {Risk: analysis.RiskCritical}, "app/b.go::B": {Risk: analysis.RiskLow}, } - rows := rankFileRisk(diff, impact, nil) + rows := rankFileRisk(diff, impact, nil, "", true) require.Len(t, rows, 2) require.Equal(t, "app/a.go", rows[0].File, "the critical-risk file ranks first") require.Equal(t, string(analysis.RiskCritical), rows[0].Risk) require.Equal(t, string(analysis.RiskLow), rows[1].Risk) } + +// TestRankFileRiskNormalizesRepoPrefix pins the multi-repo shape: changed +// symbols carry graph-prefixed file paths while the diff's changed files are +// repo-relative. The rollup must merge both onto one row per file (keyed +// relative), carrying the symbol's impact tier — not emit a prefixed +// impact-tier row plus a LOW diff-only duplicate. +func TestRankFileRiskNormalizesRepoPrefix(t *testing.T) { + diff := &analysis.DiffResult{ + ChangedSymbols: []analysis.ChangedSymbol{ + {ID: "myrepo/app/a.go::A", FilePath: "myrepo/app/a.go", Line: 1}, + }, + ChangedFiles: []string{"app/a.go", "app/b.go"}, + } + impact := map[string]*analysis.ImpactResult{ + "myrepo/app/a.go::A": {Risk: analysis.RiskCritical}, + } + rows := rankFileRisk(diff, impact, nil, "myrepo", true) + require.Len(t, rows, 2, "one row per file, prefixed and relative forms merged") + require.Equal(t, "app/a.go", rows[0].File) + require.Equal(t, string(analysis.RiskCritical), rows[0].Risk) + require.Equal(t, "app/b.go", rows[1].File) +} + +// TestRankFileRiskCoverageEvidence pins the per-file coverage rollup: the +// widest blast radius among the file's changed symbols, the changed-symbol +// count, and how many of them lack a covering test. +func TestRankFileRiskCoverageEvidence(t *testing.T) { + diff := &analysis.DiffResult{ + ChangedSymbols: []analysis.ChangedSymbol{ + {ID: "app/a.go::A", FilePath: "app/a.go", Line: 1}, + {ID: "app/a.go::B", FilePath: "app/a.go", Line: 20}, + }, + ChangedFiles: []string{"app/a.go"}, + } + impact := map[string]*analysis.ImpactResult{ + "app/a.go::A": {Risk: analysis.RiskCritical, TotalAffected: 42, TestFiles: []string{"app/a_test.go"}}, + "app/a.go::B": {Risk: analysis.RiskLow, TotalAffected: 3}, + } + rows := rankFileRisk(diff, impact, nil, "", true) + require.Len(t, rows, 1) + require.Equal(t, 42, rows[0].Affected, "the widest symbol's blast radius wins") + require.Equal(t, 2, rows[0].Symbols) + require.Equal(t, 1, rows[0].Uncovered, "B has no covering test") +} + +// TestRankFileRiskCoverageUnknown pins the epistemic guard: when the graph +// indexes no test symbols (coverageKnown false), no row may claim untested +// symbols — blindness is not a finding. The risk tier itself stays. +func TestRankFileRiskCoverageUnknown(t *testing.T) { + diff := &analysis.DiffResult{ + ChangedSymbols: []analysis.ChangedSymbol{ + {ID: "app/a.go::A", FilePath: "app/a.go", Line: 1}, + }, + ChangedFiles: []string{"app/a.go"}, + } + impact := map[string]*analysis.ImpactResult{ + "app/a.go::A": {Risk: analysis.RiskCritical, TotalAffected: 42}, + } + rows := rankFileRisk(diff, impact, nil, "", false) + require.Len(t, rows, 1) + require.Equal(t, string(analysis.RiskCritical), rows[0].Risk, "the blast-radius tier stays") + require.Equal(t, 42, rows[0].Affected, "blast radius is a graph fact, not a coverage claim") + require.Zero(t, rows[0].Symbols, "no coverage claims when the index carries no tests") + require.Zero(t, rows[0].Uncovered) +} + +// TestSummarizeCoveragePhrasing pins the three risk-driven headlines: untested +// risk, fully covered risk, and unknown coverage. +func TestSummarizeCoveragePhrasing(t *testing.T) { + critical := string(analysis.RiskCritical) + + untested := summarize(VerdictBlock, nil, []FileRisk{{File: "a.go", Risk: critical, Symbols: 2, Uncovered: 1}}) + require.Contains(t, untested, "1 without covering tests") + + covered := summarize(VerdictReview, nil, []FileRisk{{File: "a.go", Risk: critical, Symbols: 2}}) + require.Contains(t, covered, "all test-covered") + + unknown := summarize(VerdictBlock, nil, []FileRisk{{File: "a.go", Risk: critical}}) + require.Contains(t, unknown, "test coverage unknown") +} + +// TestComputeVerdictCoverageCap pins the coverage temper: a critical-risk +// file whose changed symbols are all test-covered contributes at most +// REVIEW; the same file with an untested changed symbol blocks. +func TestComputeVerdictCoverageCap(t *testing.T) { + covered := []FileRisk{{File: "a.go", Risk: string(analysis.RiskCritical), Symbols: 2, Uncovered: 0}} + require.Equal(t, VerdictReview, computeVerdict(nil, covered), + "blast radius alone must not block a fully test-covered change") + + untested := []FileRisk{{File: "a.go", Risk: string(analysis.RiskCritical), Symbols: 2, Uncovered: 1}} + require.Equal(t, VerdictBlock, computeVerdict(nil, untested)) + + // No coverage evidence (no impact data) keeps the conservative ladder. + unknown := []FileRisk{{File: "a.go", Risk: string(analysis.RiskCritical)}} + require.Equal(t, VerdictBlock, computeVerdict(nil, unknown)) +} diff --git a/internal/review/report.go b/internal/review/report.go index 01b903ab..a16fdc75 100644 --- a/internal/review/report.go +++ b/internal/review/report.go @@ -2,6 +2,7 @@ package review import ( "sort" + "strings" "github.com/zzet/gortex/internal/analysis" ) @@ -26,11 +27,23 @@ type ReviewReport struct { } // FileRisk ranks one changed file by its impact-derived risk tier and the number -// of findings anchored to it. +// of findings anchored to it. The tier is a blast-radius fact; Affected, +// Symbols, and Uncovered carry the evidence behind it so "CRITICAL" is +// explainable: how many symbols the change can reach, and how much of the +// change has covering tests. A fully covered file (Symbols > 0, Uncovered 0) +// contributes at most REVIEW to the verdict — blast radius alone does not +// block a well-tested change. type FileRisk struct { File string `json:"file"` Risk string `json:"risk"` Findings int `json:"findings"` + // Affected is the widest blast radius among the file's changed symbols + // (total transitively affected symbols from the impact analysis). + Affected int `json:"affected,omitempty"` + // Symbols counts the changed symbols in this file with impact data; + // Uncovered counts how many of them have no covering test. + Symbols int `json:"symbols,omitempty"` + Uncovered int `json:"uncovered,omitempty"` } // ReviewStats records how the report was assembled: how many findings each @@ -53,13 +66,22 @@ type ReviewStats struct { // warning finding (or a high/medium-risk file) → REVIEW; otherwise APPROVE. It // reuses the FromSeverity / FromRisk adapters so the ladder is never // re-implemented here. +// +// Coverage tempers the risk side: a file whose changed symbols are all +// test-covered contributes at most REVIEW — blast radius says how far a +// regression could reach, covering tests say how likely it is to land +// unnoticed, and only the combination blocks. func computeVerdict(findings []Finding, fileRisk []FileRisk) Verdict { worst := VerdictApprove for _, f := range findings { worst = worseVerdict(worst, FromSeverity(f.Severity)) } for _, fr := range fileRisk { - worst = worseVerdict(worst, FromRisk(analysis.RiskLevel(fr.Risk))) + v := FromRisk(analysis.RiskLevel(fr.Risk)) + if fr.Symbols > 0 && fr.Uncovered == 0 && verdictRank(v) > verdictRank(VerdictReview) { + v = VerdictReview + } + worst = worseVerdict(worst, v) } return worst } @@ -89,32 +111,77 @@ func worseVerdict(a, b Verdict) Verdict { // of the worst changed symbol it contains; with no impact data it falls back to a // finding-count heuristic via the same assessRisk ladder. The result is sorted // worst-first, then by file for determinism. -func rankFileRisk(diff *analysis.DiffResult, impact map[string]*analysis.ImpactResult, findings []Finding) []FileRisk { +// +// repoPrefix normalizes the two path vocabularies onto one key: changed-symbol +// (and finding) paths come from graph nodes, which multi-repo daemons key as +// "/", while the diff's changed files are repo-relative. Without +// stripping the prefix every file would surface twice — once with its real +// impact tier and once as a LOW diff-only row. +// +// coverageKnown gates the coverage evidence: when the graph indexes no test +// symbols at all, "no covering test" is blindness, not a finding — the rows +// then carry no untested counts and the verdict keeps the conservative +// ladder. +func rankFileRisk(diff *analysis.DiffResult, impact map[string]*analysis.ImpactResult, findings []Finding, repoPrefix string, coverageKnown bool) []FileRisk { + norm := func(file string) string { + file = cleanPath(file) + if repoPrefix != "" { + file = strings.TrimPrefix(file, repoPrefix+"/") + } + return file + } + byFile := map[string]string{} findingCount := map[string]int{} + type coverage struct { + affected int + symbols int + uncovered int + } + coverByFile := map[string]*coverage{} for _, f := range findings { if f.File != "" { - findingCount[f.File]++ + findingCount[norm(f.File)]++ } } if diff != nil { for _, cs := range diff.ChangedSymbols { - file := cleanPath(cs.FilePath) + file := norm(cs.FilePath) if file == "" { continue } risk := string(analysis.RiskLow) if impact != nil { - if ir := impact[cs.ID]; ir != nil && ir.Risk != "" { + ir := impact[cs.ID] + if ir != nil && ir.Risk != "" { risk = string(ir.Risk) } + // Blast-radius evidence (how far the worst changed symbol + // reaches) is a graph fact and always recorded. Coverage + // evidence is recorded only when the graph indexes tests + // at all — an index that excludes test files must not + // read as "untested". + cov := coverByFile[file] + if cov == nil { + cov = &coverage{} + coverByFile[file] = cov + } + if ir != nil && ir.TotalAffected > cov.affected { + cov.affected = ir.TotalAffected + } + if coverageKnown { + cov.symbols++ + if ir == nil || len(ir.TestFiles) == 0 { + cov.uncovered++ + } + } } byFile[file] = worseRisk(byFile[file], risk) } for _, file := range diff.ChangedFiles { - file = cleanPath(file) + file = norm(file) if file == "" { continue } @@ -136,7 +203,13 @@ func rankFileRisk(diff *analysis.DiffResult, impact map[string]*analysis.ImpactR if risk == "" { risk = string(riskFromFindingCount(findingCount[file])) } - rows = append(rows, FileRisk{File: file, Risk: risk, Findings: findingCount[file]}) + row := FileRisk{File: file, Risk: risk, Findings: findingCount[file]} + if cov := coverByFile[file]; cov != nil { + row.Affected = cov.affected + row.Symbols = cov.symbols + row.Uncovered = cov.uncovered + } + rows = append(rows, row) } sort.SliceStable(rows, func(i, j int) bool { ri, rj := riskRank(rows[i].Risk), riskRank(rows[j].Risk) diff --git a/internal/review/summary.go b/internal/review/summary.go index db3c3a85..98278083 100644 --- a/internal/review/summary.go +++ b/internal/review/summary.go @@ -124,12 +124,26 @@ func renderHumanSummary(report *ReviewReport) string { if len(report.FileRisk) > 0 { b.WriteString("\nFile risk:\n") for _, fr := range report.FileRisk { - fmt.Fprintf(&b, " %-8s %s (%d finding(s))\n", fr.Risk, fr.File, fr.Findings) + fmt.Fprintf(&b, " %-8s %s (%d finding(s))", fr.Risk, fr.File, fr.Findings) + // Evidence, when the impact analysis supplied it: the blast + // radius behind the tier, and — when the graph indexes tests + // at all — whether tests stand behind the change. + if fr.Affected > 0 || fr.Symbols > 0 { + fmt.Fprintf(&b, " — %d affected", fr.Affected) + if fr.Symbols > 0 { + if fr.Uncovered > 0 { + fmt.Fprintf(&b, ", %d/%d changed symbols untested", fr.Uncovered, fr.Symbols) + } else { + fmt.Fprintf(&b, ", all %d changed symbols test-covered ✓", fr.Symbols) + } + } + } + b.WriteByte('\n') } } if len(report.Findings) == 0 { - b.WriteString("\nNo inline findings.\n") + b.WriteString("\n✓ No inline findings — the deterministic rulepack passed.\n") } else { fmt.Fprintf(&b, "\nFindings (%d):\n", len(report.Findings)) for _, file := range findingFilesSorted(report.Findings) { diff --git a/internal/review/summary_test.go b/internal/review/summary_test.go index bee0da63..76b9d9f2 100644 --- a/internal/review/summary_test.go +++ b/internal/review/summary_test.go @@ -157,7 +157,7 @@ func TestRenderSummary_NilReport(t *testing.T) { t.Errorf("nil report agent rendering must carry a zero cost line, got %q", agent) } human := RenderSummary(nil, AudienceHuman) - if !strings.Contains(human, "Verdict: APPROVE") || !strings.Contains(human, "No inline findings.") { + if !strings.Contains(human, "Verdict: APPROVE") || !strings.Contains(human, "No inline findings") { t.Errorf("nil report human rendering must be an empty APPROVE packet, got %q", human) } }