normalize mixed-separator store paths in resolver, store scoping, and rule matching - #532
Conversation
zzet
left a comment
There was a problem hiding this comment.
@pbednarcik two gaps found during the change review
-
The new tests guard nothing on any platform CI runs. I reverted all six production changes and re-ran the new tests: all pass. On Linux/macOS they're documentation, not regression guards. The fix is cheap and the precedent is already in the repo: .github/workflows/ci.yml:36 has a build-windows job whose entire documented pattern is narrow go test steps for platform-divergent semantics — "Verifying it only on the linux/macos matrix would leave the platform whose semantics differ most as the untested one." ./internal/graphpath is a new self-contained package that would pass wholesale; the rest fit as -run 'NativeSeparator'. The PR body treats Windows CI as future work tied to #529, but the job exists today.
-
The audit missed ~28 siblings of fix #2. strings.HasPrefix(n.FilePath, pathPrefix) against the user-facing MCP path_prefix argument is still raw in ~28 handlers that walk the graph directly - tools_analyze_{bottlenecks,health_score,impact,role,tests}, tools_{architecture,ast,churn,clones,coupling,extract_candidates,knowledge_gaps,review_questions,untested,winnow}, tools_enhancements (ownership / coverage_gaps / coverage_summary), plus analysis/cycles.go, analysis/guards.go, contracts/endpoint_resolve.go, docs/docs.go. Same Windows failure mode: 0 rows for any prefix deeper than one segment. The PR fixed only the 3 in graph.go — defensible as "the store-interface reference impls the conformance suite pins," but "the remaining members my audit found" overstates it.
One out-of-scope finding worth a follow-up issue
fileDirColumnDDL (store_sqlite/schema.go:207) computes the file_dir generated column by splitting on / in SQL. On a Windows-indexed store every row collapses to the repo prefix, so the nodes_go_receiver_type index and the c.file_dir = e.member_receiver_dir join in method_receiver_rebind*.go degrade from "seek the package" to "match anything in the repo" — an over-match, not just a slowdown. Needs a generated-column migration, so correctly left out of this PR.
While putting together #529 I audited the rest of the tree for the same pattern — separator-sensitive
logic meeting the store's mixed-separator paths — and found four more places where behavior silently
degrades on Windows. This PR fixes them behind one small helper package.
The invariant
Indexed paths are
repoPrefix + '/' + repo-relative-path-in-native-separators. On Windows that meansone forward slash and then backslashes (
repo/dir\file.cs) — on my production C# repo's store,~117k of ~118k node paths carry backslashes, while synthetic nodes (
external-call::…, contracts)are slash-joined. So slash-only string logic sees the whole repo as a single directory, and anything
matching stored paths must accept both spellings.
internal/graphpathpins that invariant in one place:Norm(ToSlash — deliberately per-OS, abackslash is an ordinary filename byte on POSIX and must survive),
Dir, andPrefixForms(thespellings a stored path may use for a caller's
/-form prefix).The four fixes
resolver/dir_preferred.go— directory-convention resolution.dirOfsplit on'/'only, soon Windows every file in a repo reported the same "directory" and the same-dir uniqueness tier
could essentially never fire (unique-in-repo is a much stricter bar than unique-in-directory), and
the preferred-dir substring probe (
/middleware/) could not match native separators either. Thesame code produced fewer convention edges on Windows than on macOS. Both sites now compare
normalized paths.
Path-prefix scoping in the store aggregators — silently empty results.
NodeDegreeByKinds,ExtractCandidates, andThrowerErrorSurfacematchedfile_path LIKE '<prefix>%'(sqlite) /strings.HasPrefix(in-memory reference) against the rawcolumn. A
/-form prefix deeper than one segment below the repo cannot match a native-separatorpath. Live repro on my store:
LIKE 'repo/internal/resolver%'→ 0 rows; the native spelling →11,223. Shallow prefixes (
repo/internal%) happen to work, which is why this hid so well.The sqlite side now ORs one LIKE per
PrefixFormsspelling (the column itself can't be normalizedwithout defeating its index); the in-memory side compares normalized. Both backends are pinned by
a new conformance-suite case with
filepath.FromSlash-built fixtures.analysis/architecture.go— rule globs and layer segments.globMatchandpathHasSegmentsplit the path argument on
'/'only. Every consumer — architecture layer/pattern rules, guardExcept-lists, event-boundary Forbid/Producer/Consumer globs — fed raw store paths in, so a rule
like
internal/billing/**could never match on Windows: exceptions didn't except, forbids didn'tforbid. Both functions now normalize the path argument (patterns were already slash-form).
mcp/tools_contract_bridge.go— adjacency ranking.path.Dir(a) == path.Dir(b)mangles bothsides to the repo name on Windows, so every same-repo pair scored as same-directory and the
4-tier proximity ladder lost a tier. Now compares
graphpath.Dir.Testing
filepath.FromSlash, so the new tests run onevery OS: all-slash on POSIX (exercising the already-correct behavior), reproducing the mixed
store shape on Windows. I watched each one fail on Windows before its fix. The POSIX build is
green at every commit in the branch.
storetest.RunConformance.main (the known path-separator buckets), zero new failures.
Normis a no-op there, andPrefixFormscollapses to asingle spelling.
Relation to #529
Independent branch, same class: #529 revived the import-adjacency projection fast-path this same
guard pattern had disabled; these four are the remaining members my audit found. The Windows CI
offer in #529 would catch the whole class at commit time — happy to help with that either way.