Skip to content

store_sqlite: accept separator-normalized paths in import adjacency projection - #529

Merged
zzet merged 1 commit into
zzet:mainfrom
pbednarcik:fix/import-projection-path-guard
Aug 11, 2026
Merged

store_sqlite: accept separator-normalized paths in import adjacency projection#529
zzet merged 1 commit into
zzet:mainfrom
pbednarcik:fix/import-projection-path-guard

Conversation

@pbednarcik

Copy link
Copy Markdown
Contributor

Problem

ProjectImportAdjacency returns complete=false for every request on Windows, so the projection fast-path has never run there — every consumer silently takes the legacy per-file fallback.

The canonicality guard rejects any path that filepath.Clean changes:

if path == "" || path == "." || filepath.Clean(path) != path {

On Windows, Clean rewrites / to \. Indexed paths keep / after the repo prefix with the rest OS-native (repo/dir\file.cs), so Clean changes every stored path — even pure-slash ones — and the guard reads that as a canonicality violation. On separator-stable platforms the same comparison is fine, which is why this never showed up outside Windows.

Measured on a cold index of my production C# repo: all 93 resolve pages fell back, every run — roughly 142 s of a ~342 s resolve phase spent in the legacy reads the projection exists to replace. The guard also fails TestImportAdjacencyProjectionSkipsUnrelatedOutgoingRows on Windows (its fixture paths get rejected before the query logic under test is ever reached), which is how it hid inside the known Windows failure bucket.

Fix

Make the canonicality check separator-insensitive: reject only when Clean changes the path in ToSlash form.

if cleaned := filepath.Clean(path); cleaned != path &&
    filepath.ToSlash(cleaned) != filepath.ToSlash(path) {

Structural violations — traversal segments, ./.., duplicate separators, trailing dots — all still produce a ToSlash-divergent form and still reject. A separator-only difference no longer does. Non-Windows behavior is provably unchanged: Clean never rewrites separators there, so the new clause only fires where the old one misfired.

Tests

  • New TestImportAdjacencyProjectionAcceptsMixedSeparatorPaths — the stored-path shape; fails on Windows without the fix (verified red).
  • Repairs TestImportAdjacencyProjectionSkipsUnrelatedOutgoingRows on Windows — pre-existing failure, verified red-on-main / green-here.
  • TestImportAdjacencyProjectionRejectsMalformedProvenance (traversal, blank, mismatched provenance) unchanged and green.

Windows failure count for the package's neighborhood drops by exactly one, zero new failures.

Validation

With the guard fixed, five consecutive cold indexes of my production repo ran the fast-path on 93+ of 96 pages (the remaining fallbacks are genuinely malformed batches — the conservative path working as designed) with outcome counters digit-identical to the fallback path throughout. Follow-up PR builds on this to retain projections across pages; this fix is independently valuable either way.

Aside: Windows CI

This one was hard to see even from a Windows machine — the failing test blended into the ambient Windows failure set, and the silent fallback hid the perf cost. If a small windows-latest job would be useful (even just internal/graph/store_sqlite and internal/resolver), I'd be glad to put one together in a separate PR — no worries if that's already on the radar or not a priority.

@zzet zzet left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pbednarcik the review discovered the gap: the new test cannot fail on any runner that runs it

I reverted the guard to its pre-fix form and re-ran TestImportAdjacencyProjectionAcceptsMixedSeparatorPaths — it still passes. On POSIX, \ is an ordinary filename byte, so Clean leaves pkg/sub\caller.go untouched and the old guard accepts it too. The test only has teeth on Windows.

And build-windows already runs four narrow test steps for exactly this class of platform-divergent semantics — ./internal/agents, ./internal/pathguard, -run TestCloseSidecar ./internal/persistence, ./internal/testenv ./internal/platform. A fifth line would close it:

        - name: Test import adjacency path guard
          run: go test -timeout=5m -count=1 -run TestImportAdjacency ./internal/graph/store_sqlite

This is the same gap I flagged on their #532.

@zzet
zzet merged commit a3f2033 into zzet:main Aug 11, 2026
10 checks passed
@pbednarcik
pbednarcik deleted the fix/import-projection-path-guard branch August 16, 2026 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants