fix: coverage-domain artifacts stale on Windows incremental (path-spelling split)

[pbednarcik]

On Windows, editing an indexed file leaves its old todo node and annotated
edge in the store: the todo keeps the pre-edit tag and text forever, and a
removed marker never disappears. I hit this live when a counted acceptance
cell in my fixture repo went one-over on annotated edges after an edit, and
the store showed the stale row surviving an incremental patch.

Root cause is a path-spelling split inside one file's own artifacts. The
ingest pipeline keys file identity by the exact relPath spelling the indexer
uses - OS-native separators for subdirectory files on Windows - and both
removal lanes (the structural replace commit and deleted-file eviction) match
nodes by that spelling and remove edges by evicted-endpoint touch. The
coverage-domain builders (todos, licenses, codeowners, codegen, fixtures,
modules) re-spelled the caller's relPath with filepath.ToSlash before minting
node IDs, FilePath fields, and edge endpoints. Nothing downstream ever
matches that spelling again on Windows, so:

• todo nodes and their annotated edges survive every incremental replacement
  and deletion of their file (the stale-tag repro above);
• the standalone fixture node, whose ID deliberately reuses the file path so
  it merges with the file identity, duplicates it instead;
• licensed_as / owns / generated_by / depends_on_module edges for
  subdirectory files reference a file-node ID spelling that does not exist,
  so they dangle from birth (the modules lane even mints its synthetic
  manifest node natively two lines below the re-spelled edge).

Store evidence from the live repro repo (my counted acceptance fixtures):
2,323 native-spelled edges vs exactly 1 forward-slash edge - the one stale
annotated edge the incremental patch left behind.

Fix, first commit: the six builders preserve the caller's spelling verbatim,
so coverage artifacts share extraction identity with everything else and ride
the normal replace/evict machinery by construction. POSIX behavior is
byte-identical (ToSlash is the identity there). Each builder carries a
contract comment, and a spelling-preservation test per package pins the
contract with a platform-native path.

Fix, second commit: healing for stores written before the first commit. Both
removal lanes funnel through evictFilesBatched, so it now also evicts the
forward-slash twin of any path whose spelling differs. Only pre-fix coverage
rows can match the twin (nothing else ever minted a re-spelled FilePath), and
on POSIX the twin equals the native path and the sweep adds nothing. A stale
row therefore heals on its file's next edit or deletion - exactly the moments
its content could have changed; untouched files keep serving their
still-accurate rows. Two end-to-end regressions seed a pre-fix-spelled todo
row and prove both lanes sweep it (and that the fresh extraction's
native-spelled todo lands in its place).

Residue this deliberately does not chase: pre-fix licensed_as / owns /
generated_by / depends_on_module edges whose only anchor is the re-spelled
file endpoint. They are endpoint-dangling, so no node-anchored traversal can
reach them; they only inflate raw edge counts until a store rebuild. Sweeping
them would need an edge-by-file_path eviction primitive on both backends,
which did not seem worth the surface for invisible rows - happy to add it if
you disagree.

One sibling I noticed but left out of scope: the dotnet project extractor's
cross-file ProjectReference endpoints are forward-slashed by design so both
sides agree on an ID, but the referenced project's own file node is native-
spelled, so the agreement fails on Windows for subdirectory projects. That is
a cross-file ID-agreement question (which side normalizes), not a staleness
bug, so I can file it separately if useful.

Full race suite on Windows: fail-name set matches my baseline except two
spinner-glyph tests that fail identically on a clean checkout of the merge
base (upstream drift, unrelated), and eight baseline names that now pass.
The six builder packages and the indexer package are clean.

[zzet]

Thanks for working through this Windows path-spelling issue. I confirmed the scanner-side caller-path preservation is consistent, but there are correctness problems in the incremental healing path that need to be addressed before merge.

P1 — Generic slash-twin eviction can corrupt shared coverage artifacts

internal/indexer/incremental_batch.go:924 sends the legacy slash spelling through generic endpoint-touch eviction. Coverage targets such as license::MIT, owners, generators, and modules can be shared by multiple files.

I reproduced both outcomes with the in-memory and SQLite backends using native A = src\\a.go, legacy A = src/a.go, native B = src\\b.go, with A and B both linked to license::MIT:

• If the shared node is anchored to A's legacy path, evicting A removes B's valid licensed_as edge (expected 1, actual 0).
• If the shared node is anchored to B, A's stale legacy edge survives (expected 0, actual 1).

Please avoid passing legacy spellings to generic file eviction. Delete legacy coverage edges selectively by provenance/Edge.FilePath and edge kind, preserve shared targets while referenced, and remove those targets only when orphaned. Please add Graph and SQLite tests covering both anchoring orders.

P1 — Existing Windows stores are not migrated

The healing runs only for changed/deleted paths, and this PR has no extractor/store-policy version bump or startup migration. Upgrading and running an incremental pass without file changes therefore leaves legacy TODO/fixture nodes and licensed_as, owns, generated_by, and depends_on_module edges indefinitely. Edge-only residue can also survive a later edit when neither endpoint is selected for removal.

Please add a versioned coverage-artifact migration, or bump a global policy version to force one safe rebuild. A persisted SQLite pre-fix-store -> new-binary reopen test with no changed files would cover this.

P2 — The added tests do not exercise the Windows path split

In internal/indexer/incremental_reindex_test.go, the tests derive native paths with filepath.Join and legacy paths with filepath.ToSlash. On hosted Ubuntu/macOS those strings are identical, so the new tests would pass before this patch and never execute the twin-path branch. The Windows CI job only cross-compiles; it does not run the tests.

Please run the affected tests on Windows, or extract a platform-parametric helper testable with explicit src\\a.go and src/a.go spellings. Include Graph and SQLite persisted-store upgrade cases.

The affected-package race tests and go vet pass, and I found no security or dead-code concerns. The remaining blocker is persisted graph correctness.

[pbednarcik]

Thanks for the repro on the eviction path - both directions land, and the second one (shared node anchored elsewhere, stale edge survives) is the half I had talked myself out of during the build.

P1-1: the twin sweep is gone

I reverted it rather than making it selective in place. Generic endpoint-touch eviction has no way to tell a per-file artifact from a shared coverage target, and pushing that knowledge into evictFilesBatched would put coverage-domain policy in the generic removal path for every backend.

internal/indexer/incremental_reindex_test.go pins the contract instead: a file's eviction touches that file's own spelling and nothing else. It runs on both backends and carries both anchoring orders - one shared license node whose first-sighting FilePath is the legacy spelling, one anchored to a surviving file. With the sweep restored it fails on both backends, on the legacy-anchored target: b's valid licensed_as edge disappears and the shared node itself is deleted too, which is a degree worse than the edge loss you measured. The natively anchored target reproduces your second outcome instead - the stale legacy edge simply survives the sweep - and the test pins that as the migration's business rather than eviction's.

P1-2: healing moved to schema migration v13

purgeLegacyCoverageSpellings in internal/graph/store_sqlite/coverage_spelling_purge.go does the selective work you asked for, once, at upgrade:

• legacy coverage edges selected by kind AND their own Edge.FilePath spelling, never by touching an evicted endpoint. There is exactly one endpoint-keyed delete after that: the follow-up clearing whatever edges still hang off a node already doomed by kind and path. It is safe precisely where the twin sweep was not, because those nodes belong to one file each;
• per-file todo / fixture nodes deleted with their remaining edges, their symbol_fts rows, and their node-keyed sidecars (ref_facts by both endpoints, vectors, constant values, and the churn / coverage / release / blame enrichment tables). FTS and those sidecars are separate calls on the eviction path rather than part of EvictFiles, so the migration mirrors them explicitly;
• a shared target (license, team, module, generator) removed only after the purge leaves it with no references at all - it is never selected by path;
• the persisted analysis generation invalidated when its tables exist, for the same reason evictByPredicateResult invalidates it.

Three guards bound what is judged. A repository is judged only when its OWN file nodes are backslash-spelled, so a repository indexed on POSIX is untouched even when it shares a store with a Windows-indexed one. Within such a repository, only a forward slash BELOW the repo prefix marks a row no current builder could have produced, so top-level files - which were never damaged - never match. And selection is by the six coverage domains' own kinds; nothing else is ever a candidate.

Those three are necessary and not sufficient, which took me two rounds of review to accept. Path shape cannot decide staleness on its own: a store written on Windows and then re-indexed on POSIX keeps its stale Windows rows, since eviction is spelling-exact - this migration's own premise - and those stale rows then both set the repository's verdict and vouch for the new forward-slash rows as their twin. At that point a live POSIX row and a stale legacy row are identical in every path field. A POSIX filename that legally contains a backslash produces the same collision inside a single-platform store.

So the predicate asks an ownership question instead. When the indexer parses a file it records that path in files and hangs the file node and every symbol in it off that same path; a coverage builder's re-spelled path was never any of those things. A row is legacy only when its natively spelled twin exists, files does not record its path, and no node outside the coverage domains claims it. The candidate excludes itself from that last test, because a fixture node IS its own path - which is also what the files check is for, since a fixture with no symbols in it has no other claimant.

(One trap worth recording if you touch this: file_mtimes looks like the natural ownership test and is exactly wrong for it. It is keyed repo-relative and slash-normalized, so a legacy path matches it and a native path does not.)

This also closes the residue the incremental sweep left by design: a file nobody edits again never healed, because healing only ran for changed or deleted paths.

Two notes on it:

• Version collision. resolver: derived tests edges never re-resolve, and test-linkage skips unresolved calls #679 also claims v13. Whichever lands first keeps it and I renumber the other.
• In-memory backend. There is no Graph-side migration because checkBackend retired the in-memory backend as a persistent store: an in-memory graph is built fresh every run, so legacy rows can only exist in a SQLite file. I could not find any path that saves or loads one. The Graph coverage is on the eviction contract above, where the corruption actually lived - I mention it because your ask said "Graph and SQLite upgrade cases" and I do not want the omission to look like an oversight.

The migration's tests are in coverage_spelling_purge_test.go. The main one is the upgrade case you asked for: a persisted store seeded with both spellings, reset to user_version = 12 and reopened with no changed files, asserting purged rows, survivors (including a shared target kept alive by another file's edge), orphan removal through the owns and generated_by arms, a non-coverage edge carrying the legacy spelling that must survive, FTS rows following their node, and both anchoring orders. Eight more cover the single-repo shape, a mixed-platform store, synthetic namespaces in both prefixed and unprefixed form, a POSIX store left untouched, the defines fail-safe, sidecar-list drift against the live schema, and idempotency across two passes. There is also a direct table test of the path predicate: it takes both spellings as literals and no store at all, which makes it the platform-parametric helper you asked for, and it is where the multi-repository shape is covered - several Windows repositories, a POSIX one beside them, a prefix that is a leading substring of another, and a repository nested under another.

Three things a review round of my own turned up

I put this branch through an adversarial pass before replying, and it found ways the first version of the migration could have deleted live rows. All are fixed, all are now regression tests verified to fail against the corresponding mutation, and I would rather hand you the findings than have you find them.

The first was a data-loss path. A fixture node reuses the file node's ID by design (internal/fixtures: the fixture is the file, and ReclassifyFileToFixture upgrades a file node in place). My guard asked only whether the STORE held a backslash path anywhere, so a store carrying a POSIX-indexed repository beside a Windows-indexed one judged both by the Windows rule, and deleted the POSIX repository's fixture nodes - which ARE its file nodes - orphaning every symbol they define. That is what the per-repository scoping above is for; forcing the scope back to store-wide fails the new test.

The second was a near miss. The Go externals lane mints live depends_on_module edges whose FilePath is a synthetic namespace, external::go:<importPath> (goanalysis.externalFilePath). That value carries the import path's own forward slashes, which the predicate read as separators. Paths containing :: are now excluded on every arm, since that marks a stub namespace rather than a file. On a single-repo store, where the unprefixed arm is active, this was live third-party attribution one upgrade away from deletion, module nodes GC'd along with it.

The third was the sticky verdict: the repository classification is drawn from rows eviction never removes, so a store carried across platforms keeps a stale verdict. I first bounded that with a fail-safe and then, after a fourth pass produced a reproducible counterexample, replaced the whole path-shape argument with the ownership test described above. Two cost fixes came out of the same pass: the scope query reads only file nodes, which is both the honest test and roughly five times cheaper because it then rides a covering index instead of fetching every row of the nodes table; and the node-keyed cleanup is skipped entirely when nothing is doomed, since this step runs before healPlannerStats and a store with no sqlite_stat1 plans those ref_facts deletes as full scans.

I am flagging the review history rather than presenting a clean result because it is the honest summary of how much a destructive migration on someone else's store deserves. Four passes, four findings, three of them mine.

P2: the builder tests

You are right, and the reason generalizes further than the tests. The call the fix removed is filepath.ToSlash, which is the identity on POSIX, so no test of those builders can bind that regression on a Linux runner: the old code and the new code are the same function there. Feeding a backslash literal does not change that.

What the six tests can do, and now do, is pin a byte-identity contract that holds on every platform: the builder returns the caller's spelling verbatim. Any future unconditional re-spelling - ToSlash, strings.ReplaceAll, path.Clean - fails them on every runner instead of only on Windows. Binding the original regression itself stays a Windows-only run, and I have run it. The fixtures one keeps a forward slash on the qualifying testdata segment, because IsFixturePath normalizes through ToSlash, and asserts only ID and FilePath: Name comes from filepath.Base, which is platform-dependent by construction, and TestBuildGraphArtifacts already covers it.

The healing logic is the part that genuinely does not care what platform it runs on, and that is where the coverage now sits. Those tests seed src\a.go and src/a.go explicitly and drive a persisted store through Open; nothing in AddBatch or the migration touches path/filepath, so they exercise the split identically on Ubuntu and on Windows.

One empirical check

I ran the migration's own predicates read-only against my production store before asking you to take it on faith: 4.3 GB, Windows-written, four indexed repositories, roughly 302k nodes and 2.77M edges. The guard fires there, so this is the real path and not the no-op one.

It removes 1,088 legacy artifact nodes and 1,088 legacy coverage edges, and leaves roughly 13,500 native-spelled coverage edges untouched. Nothing in the license / team / module population ends up orphaned, so no shared target is removed at all. No node it removes is also a live file node. The ownership test separates the two populations cleanly there: of those 1,088 rows, zero have their path in files and zero are claimed by a node outside the coverage domains, while all 1,088 of their native twins have both. Those legacy annotated edges point out of a path with no node behind it - they are unreachable rows, not a second live population. Selecting them takes 18 ms and 40 ms respectively against that store.

Of the sidecars, only symbol_fts actually holds rows for the doomed nodes on that store (1,088 of them). The rest of the sidecar cleanup is insurance rather than something I measured a need for, and I would rather it be there than discover otherwise on someone else's store.

Verification

go build ./... is clean. go vet ./internal/... reports only the pre-existing possible misuse of unsafe.Pointer note at internal/parser/languages/grammar_load_windows.go:33, in a file this branch does not touch. internal/graph, internal/graph/store_sqlite, and the six coverage-domain packages pass. internal/indexer under -race is unchanged against my Windows baseline: the same 32 pre-existing failures before and after this branch, and no new ones. These are Windows runs; the Linux side is CI's.