diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0870d8df..572499e8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -111,7 +111,7 @@ jobs: - name: Test native-separator store path comparisons run: > go test -timeout=10m -count=1 - -run 'NativeSeparator|MixedSeparator|ImportAdjacency|ParseDiffGitPaths|ParseDiffLinesNewSide|FeedbackDir' + -run 'NativeSeparator|MixedSeparator|ImportAdjacency|ParseDiffGitPaths|ParseDiffLinesNewSide|FeedbackDir|ReviewRulepack|PrefixedGraphReportsRulepack' ./internal/analysis ./internal/graph/store_sqlite ./internal/mcp ./internal/resolver ./internal/persistence diff --git a/internal/mcp/tools_critique_review.go b/internal/mcp/tools_critique_review.go index e9c740fe..28d1afdf 100644 --- a/internal/mcp/tools_critique_review.go +++ b/internal/mcp/tools_critique_review.go @@ -192,7 +192,7 @@ func (s *Server) critiqueFindingsFor(ctx context.Context, req mcp.CallToolReques if err != nil { return nil, err } - rulepack = s.reviewRulepackMatches(ctx, diff.ChangedFiles, repoPrefix, allowedRepos) + rulepack = s.reviewRulepackMatches(ctx, diff.ChangedFiles, changedPathsRepoRelative, repoPrefix, allowedRepos) impact = s.reviewImpact(diff.ChangedSymbols) changedFiles = diff.ChangedFiles } diff --git a/internal/mcp/tools_review.go b/internal/mcp/tools_review.go index 5b5196f5..740e7a26 100644 --- a/internal/mcp/tools_review.go +++ b/internal/mcp/tools_review.go @@ -16,6 +16,7 @@ import ( "github.com/zzet/gortex/internal/config" "github.com/zzet/gortex/internal/gitcmd" "github.com/zzet/gortex/internal/graph" + "github.com/zzet/gortex/internal/graphpath" "github.com/zzet/gortex/internal/llm" "github.com/zzet/gortex/internal/query" "github.com/zzet/gortex/internal/review" @@ -562,7 +563,7 @@ func (s *Server) handleReview(ctx context.Context, req mcp.CallToolRequest) (*mc if err != nil { return mcp.NewToolResultError(err.Error()), nil } - rulepack = s.reviewRulepackMatches(ctx, diff.ChangedFiles, repoPrefix, allowedRepos) + rulepack = s.reviewRulepackMatches(ctx, diff.ChangedFiles, changedPathsRepoRelative, repoPrefix, allowedRepos) impact = s.reviewImpact(diff.ChangedSymbols) changedFiles = diff.ChangedFiles } @@ -614,7 +615,10 @@ func (s *Server) handleReview(ctx context.Context, req mcp.CallToolRequest) (*mc // changed files relative to the working tree while the graph keys every file // node "/". Matches come back repo-relative — the spelling that // rule resolution, the per-file risk ranking, and the forge comment API speak. -func (s *Server) reviewRulepackMatches(ctx context.Context, changedFiles []string, repoPrefix string, allowedRepos map[string]bool) []astquery.Match { +// +// domain declares which vocabulary changedFiles is spelled in, because the two +// overlap and cannot be recovered by inspection — see reviewChangedGraphPaths. +func (s *Server) reviewRulepackMatches(ctx context.Context, changedFiles []string, domain changedPathDomain, repoPrefix string, allowedRepos map[string]bool) []astquery.Match { bundle := astquery.DetectorsByCategory("review") if len(bundle) == 0 { return nil @@ -627,13 +631,18 @@ func (s *Server) reviewRulepackMatches(ctx context.Context, changedFiles []strin // Narrow to the changed-file set so the rulepack only scans the changeset, // not the whole repository. - changed := reviewChangedGraphPaths(changedFiles, repoPrefix) + changed := reviewChangedGraphPaths(changedFiles, domain, repoPrefix) if len(changed) == 0 { return nil } targets := make([]astquery.Target, 0, len(allTargets)) for _, t := range allTargets { - if changed[filepath.Clean(t.GraphPath)] { + // Normalize, do not Clean. A graph path is "/" + the rest in + // native separators, so filepath.Clean rewrites the prefix slash on + // Windows ("repo-a/pkg\widget.go" -> "repo-a\pkg\widget.go") and the + // key the changeset built no longer matches. graphpath.Norm is the + // canonical comparison form and is the identity on POSIX. + if changed[graphpath.Norm(t.GraphPath)] { targets = append(targets, t) } } @@ -669,6 +678,24 @@ func (s *Server) reviewRulepackMatches(ctx context.Context, changedFiles []strin return kept } +// changedPathDomain names the vocabulary a changed-file list is spelled in. +// The review narrowing spans two, and they overlap, so the domain travels with +// the data instead of being recovered from it. +type changedPathDomain int + +const ( + // changedPathsRepoRelative is git's spelling, relative to the working + // tree. This is what analysis.DiffResult.ChangedFiles carries: MapGitDiff + // documents it as keeping "the diff-relative paths (callers re-join them + // with git pathspecs)", and every production caller of + // reviewRulepackMatches passes exactly that field. + changedPathsRepoRelative changedPathDomain = iota + // changedPathsGraphKeyed is the graph's own spelling, "/", + // for a caller that already holds node keys (e.g. a ChangedSymbol's + // FilePath) and must not have the prefix applied twice. + changedPathsGraphKeyed +) + // reviewChangedGraphPaths maps a changeset's file paths onto the vocabulary the // graph keys file nodes in. `git diff` names files relative to the working tree // while a multi-repo daemon keys every node "/", so intersecting @@ -677,16 +704,28 @@ func (s *Server) reviewRulepackMatches(ctx context.Context, changedFiles []strin // // Only the prefixed spelling is admitted once a prefix applies — a bare // relative path would also match a same-named file in a sibling tracked repo. -// Paths that already carry the prefix pass through unchanged, so a caller -// holding graph-keyed paths (a changed symbol's FilePath) joins too. -func reviewChangedGraphPaths(changedFiles []string, repoPrefix string) map[string]bool { +// +// The caller states which vocabulary it holds; this function never guesses. +// The two domains are not distinguishable by inspection: in a repo whose tree +// carries a top-level directory named like the repo prefix, +// `repo-a/pkg/widget.go` is a valid path in *both*. Inferring "already +// prefixed" from that spelling skips the real key +// `repo-a/repo-a/pkg/widget.go`, so the changed target is missed — and when a +// same-named `pkg/widget.go` also exists, that unchanged shadow is scanned in +// its place. Admitting both candidate keys is not a fix either: it still lets +// unchanged code be selected. +func reviewChangedGraphPaths(changedFiles []string, domain changedPathDomain, repoPrefix string) map[string]bool { changed := make(map[string]bool, len(changedFiles)) for _, f := range changedFiles { - f = filepath.Clean(strings.TrimSpace(f)) + // Clean collapses "./" and "..", then Norm puts both vocabularies in + // the one comparison spelling — git already speaks '/', and a caller + // handing in a graph-keyed path carries native separators after the + // prefix. Identity on POSIX. + f = graphpath.Norm(filepath.Clean(strings.TrimSpace(f))) if f == "" || f == "." { continue } - if repoPrefix != "" && !strings.HasPrefix(f, repoPrefix+"/") { + if domain == changedPathsRepoRelative && repoPrefix != "" { f = repoPrefix + "/" + f } changed[f] = true @@ -698,7 +737,12 @@ func reviewChangedGraphPaths(changedFiles []string, repoPrefix string) map[strin // repo-relative spelling the rest of the review pipeline speaks: the rule // resolver matches `.gortex.yaml` globs against it, rankFileRisk keys its rows // on it, and post_review hands it to the forge's comment API. +// Every one of those consumers speaks '/', so the result is normalized: a +// graph path carries native separators after the prefix, and handing +// `pkg\widget.go` to a `.gortex.yaml` glob or the forge comment API would +// match nothing and anchor a comment nowhere. Identity on POSIX. func reviewRepoRelPath(path, repoPrefix string) string { + path = graphpath.Norm(path) if repoPrefix == "" { return path } @@ -1101,7 +1145,7 @@ func (s *Server) handleReviewPack(ctx context.Context, req mcp.CallToolRequest) if err != nil { return mcp.NewToolResultError(err.Error()), nil } - rulepack = s.reviewRulepackMatches(ctx, diff.ChangedFiles, repoPrefix, allowedRepos) + rulepack = s.reviewRulepackMatches(ctx, diff.ChangedFiles, changedPathsRepoRelative, repoPrefix, allowedRepos) impact = s.reviewImpact(diff.ChangedSymbols) for _, cs := range diff.ChangedSymbols { if cs.ID != "" { diff --git a/internal/mcp/tools_review_post.go b/internal/mcp/tools_review_post.go index c23f5cc2..88160b5d 100644 --- a/internal/mcp/tools_review_post.go +++ b/internal/mcp/tools_review_post.go @@ -145,7 +145,7 @@ func (s *Server) postReviewFindingsFor(ctx context.Context, req mcp.CallToolRequ if err != nil { return nil, err } - rulepack = s.reviewRulepackMatches(ctx, diff.ChangedFiles, repoPrefix, allowedRepos) + rulepack = s.reviewRulepackMatches(ctx, diff.ChangedFiles, changedPathsRepoRelative, repoPrefix, allowedRepos) impact = s.reviewImpact(diff.ChangedSymbols) changedFiles = diff.ChangedFiles } diff --git a/internal/mcp/tools_review_rulepack_test.go b/internal/mcp/tools_review_rulepack_test.go index 121b57f6..be734464 100644 --- a/internal/mcp/tools_review_rulepack_test.go +++ b/internal/mcp/tools_review_rulepack_test.go @@ -92,7 +92,7 @@ func TestReviewRulepackMatches_JoinsRepoRelativeChangedFiles(t *testing.T) { "pkg/widget.go": reviewRulepackFixture, }) - matches := srv.reviewRulepackMatches(context.Background(), []string{"pkg/widget.go"}, prefix, nil) + matches := srv.reviewRulepackMatches(context.Background(), []string{"pkg/widget.go"}, changedPathsRepoRelative, prefix, nil) require.NotEmpty(t, matches, "repo-relative changed file must join the prefixed graph path %q/pkg/widget.go", prefix) @@ -114,10 +114,38 @@ func TestReviewRulepackMatches_IgnoresUnchangedFiles(t *testing.T) { "pkg/other.go": "package pkg\n\nfunc Other() {}\n", }) - matches := srv.reviewRulepackMatches(context.Background(), []string{"pkg/other.go"}, prefix, nil) + matches := srv.reviewRulepackMatches(context.Background(), []string{"pkg/other.go"}, changedPathsRepoRelative, prefix, nil) require.Empty(t, matches, "a file outside the changeset must not be scanned") } +// TestReviewRulepackMatches_PrefixShadowedPathScansOnlyTheChangedTarget pins +// the case that makes inferring the path domain unsafe. The repo's own tree +// carries a top-level directory named like the repo prefix, so the changed +// git-relative path `repo-a/pkg/widget.go` is *also* a well-formed graph key +// for the different, unchanged file `pkg/widget.go`. +// +// Guessing "this already looks prefixed" skips the real key +// `repo-a/repo-a/pkg/widget.go` — the changed file is never scanned, and the +// unchanged shadow is scanned in its place. Both files carry the detector +// fixture, so a wrong-target scan still returns matches and only the reported +// path distinguishes the two outcomes. +func TestReviewRulepackMatches_PrefixShadowedPathScansOnlyTheChangedTarget(t *testing.T) { + srv, prefix := setupPrefixedReviewServer(t, map[string]string{ + "pkg/widget.go": reviewRulepackFixture, + "repo-a/pkg/widget.go": reviewRulepackFixture, + }) + require.Equal(t, "repo-a", prefix, "the fixture's shadow directory must equal the repo prefix") + + matches := srv.reviewRulepackMatches(context.Background(), + []string{"repo-a/pkg/widget.go"}, changedPathsRepoRelative, prefix, nil) + + require.NotEmpty(t, matches, "the changed nested file must be scanned") + for _, m := range matches { + require.Equal(t, "repo-a/pkg/widget.go", m.File, + "only the changed target may be scanned; %q is the unchanged shadow", m.File) + } +} + // TestReviewRulepackMatches_AcceptsAlreadyPrefixedChangedFiles covers the // callers that hand in graph-keyed paths (a changed symbol's FilePath) rather // than git's repo-relative spelling. @@ -127,7 +155,7 @@ func TestReviewRulepackMatches_AcceptsAlreadyPrefixedChangedFiles(t *testing.T) }) matches := srv.reviewRulepackMatches(context.Background(), - []string{prefix + "/pkg/widget.go"}, prefix, nil) + []string{prefix + "/pkg/widget.go"}, changedPathsGraphKeyed, prefix, nil) require.NotEmpty(t, matches, "an already-prefixed changed file must still join") require.Equal(t, "pkg/widget.go", matches[0].File) }