fix(review): join the changeset to native-separator graph paths

.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
 

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
 	}

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 "<prefix>/<rel>". 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 "<prefix>/" + 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, "<prefix>/<rel>",
+	// 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 "<prefix>/<rel>", 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 != "" {

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
 	}

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)
 }