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

[tiendungdev]

Problem

reviewRulepackMatches builds the changed-file key correctly, then looks it up with filepath.Clean — which rewrites the one separator a graph path deliberately keeps as /: the repo prefix.

Probed on windows/amd64 with the existing fixture:

changed map key             "repo-a/pkg\widget.go"
target.GraphPath            "repo-a/pkg\widget.go"    <- identical
filepath.Clean(GraphPath)   "repo-a\pkg\widget.go"    <- prefix slash lost
hit                         false

The narrowing therefore produced an empty target set and review reported zero findings on code its own detector bundle flags under analyze --kind review. That is the same false clean reviewChangedGraphPaths was written to fix — its doc-comment says so — reintroduced one line later by the Clean.

Change

Two halves of one contract, both in tools_review.go:

1. The join. Compare in graphpath.Norm form on both sides. Norm is the canonical comparison spelling for a path shaped "<prefix>/" + native remainder, and it is filepath.ToSlash, so this is the identity on POSIX.

2. The output. reviewRepoRelPath feeds the .gortex.yaml rule globs, rankFileRisk's row keys, and the forge comment API — the function's own doc-comment lists them, and all three speak /. On Windows it returned pkg\widget.go, which matches no glob and anchors no comment. Normalize there too.

Effect

Whole internal/mcp package on windows/amd64, go1.26.6, -count=1:

	failures
main (c982cf52)	27
with this change	24

Diffing failing test names: exactly three flip, newly-broken set empty.

FIXED:  TestReviewRulepackMatches_JoinsRepoRelativeChangedFiles
        TestReviewRulepackMatches_AcceptsAlreadyPrefixedChangedFiles
        TestReview_PrefixedGraphReportsRulepackFinding
NEWLY BROKEN: (none)

Verification

Each half sabotage-verified separately, because a fix that only looks necessary is not:

• revert the join → all three fail;
• revert the output normalization → the two that assert m.File fail on Not equal.

golangci-lint run ./internal/mcp/... reports 6 staticcheck SA5011 findings; pre-existing, identical with this change stashed. git diff --check clean.

Why there is no cross-platform test

graphpath.Norm is filepath.ToSlash, deliberately a no-op on POSIX — the package doc is explicit that a backslash is an ordinary filename byte there and must survive. So a test feeding a backslash path and asserting a slash result would fail on linux/macos, and one built with filepath.FromSlash asserts nothing on either. The three names therefore join the existing windows native-separator step, whose package list already carries internal/mcp — no new step, no new compile.

Declared, not swept under

TestReviewPackNeverClaimsNoTestSymbolsWithTestTargets fails on windows too and I deliberately left it out: it fails on test_targets being empty, not on the path join, and testpath.IsTestFile already normalizes separators itself — so it is a different cause and belongs in its own change rather than riding along here.

Branched from main at c982cf52. Windows 11, go1.26.6.

[zzet]

Requesting changes after an additional path-provenance validation.

Blocking correctness issue at internal/mcp/tools_review.go:700:

With repoPrefix repo-a and a legitimate Git-relative changed path repo-a/pkg/widget.go, normalization produces the same string. The HasPrefix check then treats it as already graph-prefixed, although the correct graph key is repo-a/repo-a/pkg/widget.go. This misses the changed target and, when pkg/widget.go also exists, can scan that unchanged shadow target instead. The ambiguity already exists on POSIX and this change newly exposes the wrong-target behavior on Windows.

Please address the following:

1. Carry explicit path provenance for Git-relative versus graph-keyed paths instead of inferring it with HasPrefix.
2. Prefix production MapGitDiff paths unconditionally, or otherwise make the input domain explicit.
3. Add a regression fixture containing both pkg/widget.go and repo-a/pkg/widget.go, mark only the nested path changed, and assert that only the changed target is scanned.
4. Rerun the focused Windows native-separator validation and the existing CI suite.

Adding both possible graph keys is not a safe workaround because it can still select unchanged code.

[tiendungdev]

You were right, and the failure mode reproduces exactly as you described. Pushed fdcf19d2.

I verified the claim before changing anything: restoring the HasPrefix inference against the new fixture fails with

--- FAIL: TestReviewRulepackMatches_PrefixShadowedPathScansOnlyTheChangedTarget
    Messages: only the changed target may be scanned;
              "pkg/widget.go" is the unchanged shadow

— the changed repo-a/pkg/widget.go never scanned, the unchanged shadow scanned in its place.

1 + 2 — explicit provenance, unconditional prefixing

The domain now travels with the data instead of being recovered from it:

changedPathsRepoRelative  // git's spelling, relative to the working tree
changedPathsGraphKeyed    // the graph's "<prefix>/<rel>" node key

reviewRulepackMatches takes it and passes it through; reviewChangedGraphPaths prefixes unconditionally when the domain is repo-relative and never inspects the string.

On "prefix production MapGitDiff paths unconditionally" — that is now what happens, and the contract already supported it. All four production callers (tools_review.go ×2, tools_critique_review.go, tools_review_post.go) pass DiffResult.ChangedFiles, and joinHunksToSymbols documents that field as keeping "the diff-relative paths (callers re-join them with git pathspecs); only the node lookup is prefix-aware". So the repo-relative domain is the production domain by contract, not by assumption. changedPathsGraphKeyed is now reached only by the test that covers a caller holding node keys — I kept the capability rather than deleting it, but it is opt-in and named.

3 — regression fixture

TestReviewRulepackMatches_PrefixShadowedPathScansOnlyTheChangedTarget: the repo tree carries both pkg/widget.go and repo-a/pkg/widget.go, only the nested path is marked changed, and every match must report repo-a/pkg/widget.go.

Both files carry the detector fixture deliberately — otherwise a wrong-target scan would return empty and the test would pass for the wrong reason. With both seeded, the wrong target still produces matches and only the reported path separates the outcomes.

4 — validation

• Focused Windows native-separator selector: ok for analysis, store_sqlite, mcp, resolver, persistence.
• Whole internal/mcp on windows/amd64: 27 → 24 against main, failing test names diffed — same three fixed, newly-broken set empty, and the new test passes.
• golangci-lint ./internal/mcp/...: 6 staticcheck findings, all pre-existing (identical with the change stashed).

One thing I removed

The ci.yml selector hunk is gone from this branch. #652 deletes that job outright, so leaving it would only hand you a conflict — and your point on #647 about selector lists applies here too.

Also worth flagging since it is upstream of this: the ambiguity is in the data model, not only in this function. DiffResult.ChangedFiles is documented as diff-relative but is plain []string, so the next caller can make the same inference. If you want that closed properly the type belongs on the field rather than at this call site — happy to do it as its own change if you think it is worth the churn.