test: stop three assertions assuming POSIX separators on native paths - #644
Merged
zzet merged 1 commit intoAug 21, 2026
Merged
Conversation
TestFeedbackDir, TestParseDiffGitPaths and TestParseDiffLinesNewSide all
compare against a '/'-spelled path while the value under test is a native
filesystem path. On POSIX the two coincide, so the linux/macos matrix has
never seen it; on windows/amd64 all three fail:
"\home\user\.cache\gortex\5887dcec1741_latest"
does not contain ".cache/gortex"
parseDiffGitPaths("diff --git a/pkg/foo.go b/pkg/foo.go")
= "pkg\foo.go", want "pkg/foo.go"
diffmap_test.go:63: expected new-side lines for pkg/foo.go
The production values are correct in each case and are left alone:
- FeedbackDir is filepath.Join(cacheDir, key) and feeds os.Open, so it
must carry native separators.
- cleanDiffPath is filepath.Clean, and JoinFileNodes looks a diff path
up as repoPrefix + "/" + path. Indexed paths are repoPrefix + '/' +
the rest in native separators (see internal/graphpath), so the
cleaned native form is exactly what the join needs. Making diff paths
slash-canonical would break that lookup on Windows.
So the fix is on the assertion side: build the expectation with
filepath.Join / filepath.Clean instead of a literal. On POSIX both are
identity for these inputs, so the assertions keep exactly the strength
they had on the matrix that already runs them.
Fold the three into the existing native-separator windows step rather
than adding one: internal/analysis, internal/mcp, internal/resolver and
internal/graph/store_sqlite are already in that step's package list, and
internal/persistence is already compiled by the sidecar step above it.
zzet
approved these changes
Aug 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Three tests compare against a
/-spelled path while the value under test is a native filesystem path. On POSIX the two spellings coincide, so the linux/macos matrix has never been able to see it. On windows/amd64, go1.26.6, all three fail atmain:The production values are correct — I checked before touching them
This looked at first like a separator bug in
cleanDiffPath. It is not, and the tests are what needs to change:FeedbackDirisfilepath.Join(cacheDir, RepoCacheKey(repoPath))and its result feedsos.OpenthroughLoadFeedback. It must carry native separators.cleanDiffPathisfilepath.Clean, andJoinFileNodeslooks a diff path up asrepoPrefix + "/" + path. Perinternal/graphpath's package doc, indexed paths arerepoPrefix + '/' + the repo-relative path in native separators— on Windowsrepo/dir\file. So the cleaned native form is exactly the shape the join needs. Making diff paths slash-canonical would producerepo/pkg/foo.goand break the lookup on Windows.So the fix is on the assertion side: build the expectation with
filepath.Join/filepath.Cleaninstead of a literal. On POSIX both are the identity for these inputs, so the assertions keep exactly the strength they have today on the matrix that already runs them.Guarding it
Folded into the existing
Test native-separator store path comparisonsstep rather than adding a new one —internal/analysis,internal/mcp,internal/resolverandinternal/graph/store_sqliteare already in that step's package list, andinternal/persistenceis already compiled by the sidecar step directly above it. The regex gains exactly the three tests and nothing else:Verification
okfor all five packages.ci.ymlkept), that same command reports--- FAILfor all three. The step provably binds.golangci-lint run ./internal/persistence/...— 0 issues.+6/+12/+4against numstat8-2 / 16-4 / 5-1), so no line-ending noise rides along.Declared
internal/analysis/grounding.go'scleanFilesays it "normalizes a path the same way DiffHunk.FilePath / parseDiffLines do" but only doesTrimPrefix(p, "./")— noClean. That is a real inconsistency, and on Windows it would miss theparseDiffLinesmap key. I did not touch it: its only production caller (internal/review/anchor.go:277) builds the map and looks it up with the same variable, so the mismatch is not reachable today. Happy to send it separately if you want the comment and the code reconciled.internal/analysis(TestMapGitDiffRepoPrefixJoin,TestMapGitDiffFileChangeKinds, bothTempDir RemoveAll cleanup: …\.git: The directory is not empty) are not included: I traced them to a non-stockgiton my machine that writes.git/ai/logson everygit init. Reproduced with a baregit initoutside this repo, so they are my environment, not gortex.Branched from
mainatb8b13ca7. Windows 11, go1.26.6.