From b5151fa31bfaf51e53206a91f172506147266ad7 Mon Sep 17 00:00:00 2001 From: pbednarcik <29061766+pbednarcik@users.noreply.github.com> Date: Thu, 27 Aug 2026 17:29:31 +0200 Subject: [PATCH 1/9] fix: coverage-domain artifacts preserve the extractor path spelling The todos/licenses/codeowners/codegen/fixtures/modules builders re-spelled the caller's relPath with filepath.ToSlash before minting node IDs, FilePath fields, and edge endpoints. Everything else in the ingest pipeline keys file identity by the exact relPath spelling the indexer uses - OS-native separators for subdirectory files on Windows (see graphRelKey) - so on Windows these artifacts were invisible to eviction (nodes are matched by file_path, edges by evicted-endpoint touch) and their file-side endpoints dangled from nonexistent node IDs. Observable damage on Windows: editing a file left its stale todo node (old tag/text) and annotated edge in the store; the standalone fixture node duplicated the file identity instead of sharing it; licensed_as / owns / generated_by / depends_on_module edges for subdirectory files dangled from birth. POSIX behavior is byte-identical (ToSlash is the identity there). The builders now preserve the caller's spelling verbatim; each carries a contract comment saying why. One spelling-preservation test per builder pins the contract with a platform-native path. --- internal/codegen/scanner.go | 4 +++- internal/codegen/scanner_test.go | 18 ++++++++++++++++++ internal/codeowners/parser.go | 5 +++-- internal/codeowners/parser_test.go | 20 ++++++++++++++++++++ internal/fixtures/scanner.go | 5 +++-- internal/fixtures/scanner_test.go | 22 ++++++++++++++++++++++ internal/licenses/scanner.go | 6 +++--- internal/licenses/scanner_test.go | 21 +++++++++++++++++++++ internal/modules/scanner.go | 7 ++++--- internal/modules/scanner_test.go | 23 +++++++++++++++++++++++ internal/todos/scanner.go | 8 +++++--- internal/todos/scanner_test.go | 25 +++++++++++++++++++++++++ 12 files changed, 150 insertions(+), 14 deletions(-) diff --git a/internal/codegen/scanner.go b/internal/codegen/scanner.go index cb2bff814..afe06c242 100644 --- a/internal/codegen/scanner.go +++ b/internal/codegen/scanner.go @@ -158,11 +158,13 @@ func Scan(source []byte) Marker { // // The returned edge slice is appended by the caller; the file node // has meta.generated stamped by the caller using the returned Marker. +// filePath's spelling is preserved verbatim — the edge endpoint must +// match the extractor's file-node ID spelling (OS-native separators +// on Windows) or the edge dangles. func BuildGraphArtifacts(filePath string, marker Marker) []*graph.Edge { if !marker.Generated { return nil } - filePath = filepath.ToSlash(filePath) target := generatorNodeID(marker) return []*graph.Edge{{ From: filePath, diff --git a/internal/codegen/scanner_test.go b/internal/codegen/scanner_test.go index 7d2fafe75..f3988b1fb 100644 --- a/internal/codegen/scanner_test.go +++ b/internal/codegen/scanner_test.go @@ -1,6 +1,7 @@ package codegen import ( + "path/filepath" "testing" "github.com/zzet/gortex/internal/graph" @@ -84,6 +85,23 @@ func TestScan_Variants(t *testing.T) { } } +func TestBuildGraphArtifacts_PreservesCallerPathSpelling(t *testing.T) { + // The indexer keys eviction and incremental replacement by the + // exact relPath spelling it hands the builder; a re-spelled edge + // endpoint dangles from a nonexistent file node on Windows. + rel := filepath.Join("src", "gen", "foo.pb.go") + edges := BuildGraphArtifacts(rel, Marker{Generated: true, Tool: "protoc-gen-go"}) + if len(edges) != 1 { + t.Fatalf("edges = %d", len(edges)) + } + if edges[0].From != rel { + t.Errorf("edge.From = %q, want %q", edges[0].From, rel) + } + if edges[0].FilePath != rel { + t.Errorf("edge file path = %q, want %q", edges[0].FilePath, rel) + } +} + func TestBuildGraphArtifacts(t *testing.T) { t.Run("with source path", func(t *testing.T) { edges := BuildGraphArtifacts("pkg/foo.pb.go", Marker{ diff --git a/internal/codeowners/parser.go b/internal/codeowners/parser.go index 7b57275f6..678f04191 100644 --- a/internal/codeowners/parser.go +++ b/internal/codeowners/parser.go @@ -142,12 +142,13 @@ func LoadFromRepo(repoRoot string) (rules []Rule, sourcePath string, ok bool) { // an email) is a person. // // filePath is the unprefixed path; applyRepoPrefix downstream -// handles multi-repo namespacing. +// handles multi-repo namespacing. Its spelling is preserved verbatim — +// the edge endpoint must match the extractor's file-node ID spelling +// (OS-native separators on Windows) or the edge dangles. func BuildGraphArtifacts(filePath string, owners []string, language string) ([]*graph.Node, []*graph.Edge) { if len(owners) == 0 { return nil, nil } - filePath = filepath.ToSlash(filePath) nodes := make([]*graph.Node, 0, len(owners)) edges := make([]*graph.Edge, 0, len(owners)) for _, owner := range owners { diff --git a/internal/codeowners/parser_test.go b/internal/codeowners/parser_test.go index 70f33554c..51b0ad598 100644 --- a/internal/codeowners/parser_test.go +++ b/internal/codeowners/parser_test.go @@ -110,3 +110,23 @@ func TestBuildGraphArtifacts(t *testing.T) { t.Errorf("edge endpoints wrong: %s -> %s", edges[0].From, edges[0].To) } } + +func TestBuildGraphArtifacts_PreservesCallerPathSpelling(t *testing.T) { + // The indexer keys eviction and incremental replacement by the + // exact relPath spelling it hands the builder; a re-spelled edge + // endpoint dangles from a nonexistent file node on Windows. + rel := filepath.Join("src", "data", "foo.go") + nodes, edges := BuildGraphArtifacts(rel, []string{"@alice"}, "go") + if len(nodes) != 1 || len(edges) != 1 { + t.Fatalf("nodes = %d, edges = %d", len(nodes), len(edges)) + } + if nodes[0].FilePath != rel { + t.Errorf("node file path = %q, want %q", nodes[0].FilePath, rel) + } + if edges[0].To != rel { + t.Errorf("edge.To = %q, want %q", edges[0].To, rel) + } + if edges[0].FilePath != rel { + t.Errorf("edge file path = %q, want %q", edges[0].FilePath, rel) + } +} diff --git a/internal/fixtures/scanner.go b/internal/fixtures/scanner.go index c2293fe5e..1f344a2d9 100644 --- a/internal/fixtures/scanner.go +++ b/internal/fixtures/scanner.go @@ -56,12 +56,13 @@ func IsFixturePath(filePath string) bool { // the file. Keeping a single ID keeps cross-referencing simple // (any edge that lands on the file path also lands on the fixture // classification) and avoids the de-dup gymnastics that emitting a -// twin synthetic ID would require. +// twin synthetic ID would require. That sharing only holds when the +// spelling matches the extractor's relPath exactly (OS-native +// separators on Windows), so filePath is preserved verbatim. func BuildGraphArtifacts(filePath, language string) []*graph.Node { if !IsFixturePath(filePath) { return nil } - filePath = filepath.ToSlash(filePath) return []*graph.Node{{ ID: filePath, Kind: graph.KindFixture, diff --git a/internal/fixtures/scanner_test.go b/internal/fixtures/scanner_test.go index 4525b317f..895089ed7 100644 --- a/internal/fixtures/scanner_test.go +++ b/internal/fixtures/scanner_test.go @@ -1,6 +1,7 @@ package fixtures import ( + "path/filepath" "testing" "github.com/zzet/gortex/internal/graph" @@ -83,6 +84,27 @@ func TestBuildGraphArtifacts(t *testing.T) { }) } +func TestBuildGraphArtifacts_PreservesCallerPathSpelling(t *testing.T) { + // The standalone fixture node deliberately reuses the file path + // as its node ID so it merges with the file identity; that only + // works when the spelling matches the extractor's relPath exactly + // (OS-native separators for subdirectory files on Windows). + rel := filepath.Join("pkg", "testdata", "foo.bin") + nodes := BuildGraphArtifacts(rel, "binary") + if len(nodes) != 1 { + t.Fatalf("nodes = %d", len(nodes)) + } + if nodes[0].ID != rel { + t.Errorf("node id = %q, want %q", nodes[0].ID, rel) + } + if nodes[0].FilePath != rel { + t.Errorf("node file path = %q, want %q", nodes[0].FilePath, rel) + } + if nodes[0].Name != "foo.bin" { + t.Errorf("node name = %q", nodes[0].Name) + } +} + func TestReclassifyFileToFixture(t *testing.T) { t.Run("upgrades file to fixture", func(t *testing.T) { n := &graph.Node{ diff --git a/internal/licenses/scanner.go b/internal/licenses/scanner.go index 208804922..050ac2733 100644 --- a/internal/licenses/scanner.go +++ b/internal/licenses/scanner.go @@ -15,7 +15,6 @@ package licenses import ( "bufio" "bytes" - "path/filepath" "regexp" "strings" "sync" @@ -88,12 +87,13 @@ func Scan(source []byte) string { // out of scope for v1. // // filePath is the unprefixed path; applyRepoPrefix handles multi- -// repo namespacing downstream. +// repo namespacing downstream. Its spelling is preserved verbatim — +// the edge endpoint must match the extractor's file-node ID spelling +// (OS-native separators on Windows) or the edge dangles. func BuildGraphArtifacts(filePath, spdx, language string) ([]*graph.Node, []*graph.Edge) { if spdx == "" { return nil, nil } - filePath = filepath.ToSlash(filePath) licenseID := LicenseNodeID(spdx) licenseNode := &graph.Node{ ID: licenseID, diff --git a/internal/licenses/scanner_test.go b/internal/licenses/scanner_test.go index 0a72a7392..099a753ee 100644 --- a/internal/licenses/scanner_test.go +++ b/internal/licenses/scanner_test.go @@ -1,6 +1,7 @@ package licenses import ( + "path/filepath" "testing" "github.com/zzet/gortex/internal/graph" @@ -72,6 +73,26 @@ func TestBuildGraphArtifacts(t *testing.T) { } } +func TestBuildGraphArtifacts_PreservesCallerPathSpelling(t *testing.T) { + // The indexer keys eviction and incremental replacement by the + // exact relPath spelling it hands the builder; a re-spelled edge + // endpoint dangles from a nonexistent file node on Windows. + rel := filepath.Join("src", "data", "foo.go") + nodes, edges := BuildGraphArtifacts(rel, "MIT", "go") + if len(nodes) != 1 || len(edges) != 1 { + t.Fatalf("nodes = %d, edges = %d", len(nodes), len(edges)) + } + if nodes[0].FilePath != rel { + t.Errorf("node file path = %q, want %q", nodes[0].FilePath, rel) + } + if edges[0].From != rel { + t.Errorf("edge.From = %q, want %q", edges[0].From, rel) + } + if edges[0].FilePath != rel { + t.Errorf("edge file path = %q, want %q", edges[0].FilePath, rel) + } +} + func TestBuildGraphArtifacts_NoSPDXReturnsEmpty(t *testing.T) { nodes, edges := BuildGraphArtifacts("pkg/foo.go", "", "go") if len(nodes) != 0 || len(edges) != 0 { diff --git a/internal/modules/scanner.go b/internal/modules/scanner.go index 15f17833b..260e3e48e 100644 --- a/internal/modules/scanner.go +++ b/internal/modules/scanner.go @@ -14,7 +14,6 @@ import ( "bytes" "encoding/json" "encoding/xml" - "path/filepath" "regexp" "sort" "strings" @@ -888,7 +887,10 @@ func ModuleNodeID(ecosystem, path, version string) string { // per (ecosystem, path, version) tuple is guaranteed even when the // caller appends from multiple manifest files. // -// filePath is the unprefixed manifest path (typically "go.mod"). +// filePath is the unprefixed manifest path (typically "go.mod"), +// preserved verbatim — the edge endpoint must match the synthetic +// manifest file node the indexer mints from the same spelling +// (OS-native separators on Windows) or the edge dangles. // applyRepoPrefix downstream handles multi-repo namespacing for // the file→module edge, but module IDs themselves do not get // prefixed — the synthetic `module::` prefix matches the existing @@ -898,7 +900,6 @@ func BuildGraphArtifacts(filePath string, specs []Spec) ([]*graph.Node, []*graph if len(specs) == 0 { return nil, nil } - filePath = filepath.ToSlash(filePath) seen := make(map[string]struct{}, len(specs)) nodes := make([]*graph.Node, 0, len(specs)) edges := make([]*graph.Edge, 0, len(specs)) diff --git a/internal/modules/scanner_test.go b/internal/modules/scanner_test.go index 19c590652..28ee97f28 100644 --- a/internal/modules/scanner_test.go +++ b/internal/modules/scanner_test.go @@ -2,6 +2,7 @@ package modules import ( "fmt" + "path/filepath" "strings" "testing" @@ -128,6 +129,28 @@ func TestBuildGraphArtifacts(t *testing.T) { } } +func TestBuildGraphArtifacts_PreservesCallerPathSpelling(t *testing.T) { + // The indexer mints the synthetic manifest file node with the + // exact relPath spelling; a re-spelled EdgeDependsOnModule From + // endpoint dangles from a nonexistent node on Windows for any + // manifest below the repo root. + rel := filepath.Join("services", "api", "go.mod") + specs := []Spec{{Ecosystem: "go", Path: "github.com/foo/bar", Version: "v1.0.0", Line: 5}} + nodes, edges := BuildGraphArtifacts(rel, specs) + if len(nodes) != 1 || len(edges) != 1 { + t.Fatalf("nodes = %d, edges = %d", len(nodes), len(edges)) + } + if nodes[0].FilePath != rel { + t.Errorf("node file path = %q, want %q", nodes[0].FilePath, rel) + } + if edges[0].From != rel { + t.Errorf("edge.From = %q, want %q", edges[0].From, rel) + } + if edges[0].FilePath != rel { + t.Errorf("edge file path = %q, want %q", edges[0].FilePath, rel) + } +} + func TestParsePackageJSON_AllBlocks(t *testing.T) { src := []byte(`{ "name": "my-app", diff --git a/internal/todos/scanner.go b/internal/todos/scanner.go index 098a48dc8..55b5fc81d 100644 --- a/internal/todos/scanner.go +++ b/internal/todos/scanner.go @@ -15,7 +15,6 @@ package todos import ( "bufio" "bytes" - "path/filepath" "regexp" "strings" "sync" @@ -211,13 +210,16 @@ func parseRest(rest string) (assignee, due, text string) { // unique. // // filePath is the unprefixed (per-file extractor) path; the indexer -// adds the repo prefix downstream via applyRepoPrefix. +// adds the repo prefix downstream via applyRepoPrefix. Its spelling +// is preserved verbatim — eviction and incremental replacement key +// nodes and edges by the exact relPath spelling the indexer uses +// (OS-native separators on Windows), so a re-spelled artifact would +// never be swept and goes stale. func BuildGraphArtifacts(filePath string, findings []Finding, language string) ([]*graph.Node, []*graph.Edge) { if len(findings) == 0 { return nil, nil } - filePath = filepath.ToSlash(filePath) fileID := filePath nodes := make([]*graph.Node, 0, len(findings)) edges := make([]*graph.Edge, 0, len(findings)) diff --git a/internal/todos/scanner_test.go b/internal/todos/scanner_test.go index b9159ba0b..4610daef9 100644 --- a/internal/todos/scanner_test.go +++ b/internal/todos/scanner_test.go @@ -2,6 +2,7 @@ package todos import ( "bytes" + "path/filepath" "reflect" "testing" @@ -214,6 +215,30 @@ func TestBuildGraphArtifacts_DisambiguatesSameLine(t *testing.T) { } } +func TestBuildGraphArtifacts_PreservesCallerPathSpelling(t *testing.T) { + // The indexer keys node identity, eviction, and incremental + // replacement by the exact relPath spelling it hands the builder + // (OS-native separators for subdirectory files on Windows). A + // re-spelled artifact is invisible to those sweeps and goes stale. + rel := filepath.Join("src", "data", "foo.go") + nodes, edges := BuildGraphArtifacts(rel, []Finding{{Tag: "TODO", Text: "x", Line: 7}}, "go") + if len(nodes) != 1 || len(edges) != 1 { + t.Fatalf("nodes = %d, edges = %d", len(nodes), len(edges)) + } + if nodes[0].ID != rel+"::todo:7" { + t.Errorf("node id = %q, want %q", nodes[0].ID, rel+"::todo:7") + } + if nodes[0].FilePath != rel { + t.Errorf("node file path = %q, want %q", nodes[0].FilePath, rel) + } + if edges[0].From != rel { + t.Errorf("edge.From = %q, want %q", edges[0].From, rel) + } + if edges[0].FilePath != rel { + t.Errorf("edge file path = %q, want %q", edges[0].FilePath, rel) + } +} + func repeat(s string, n int) string { out := make([]byte, 0, len(s)*n) for range n { From 83e99056b8acd0fc549900bd27825183c8484db5 Mon Sep 17 00:00:00 2001 From: pbednarcik <29061766+pbednarcik@users.noreply.github.com> Date: Thu, 27 Aug 2026 17:33:58 +0200 Subject: [PATCH 2/9] fix: file eviction sweeps the forward-slash twin spelling for healing Stores written on Windows before the previous commit hold coverage rows (todo nodes, fixture nodes, and their edges) keyed by the forward-slash spelling of subdirectory files. Both removal lanes - the structural replace commit and the deleted-file eviction - funnel through evictFilesBatched, so the twin sweep lives there: for every path whose ToSlash form differs, the slash spelling is evicted too. Nothing else ever minted a re-spelled FilePath, so the twin can only match pre-fix coverage rows; on POSIX the twin equals the native spelling and the sweep adds nothing. With this, a stale slash-spelled todo row heals on the file''s next edit or deletion - exactly the moments its content could have changed. Two end-to-end regressions pin both lanes: a seeded pre-fix row is swept on replacement (and the fresh extraction''s native-spelled todo lands) and on deletion. --- internal/indexer/incremental_batch.go | 15 +++ internal/indexer/incremental_reindex_test.go | 101 +++++++++++++++++++ 2 files changed, 116 insertions(+) diff --git a/internal/indexer/incremental_batch.go b/internal/indexer/incremental_batch.go index a62781733..e1e556cc3 100644 --- a/internal/indexer/incremental_batch.go +++ b/internal/indexer/incremental_batch.go @@ -913,6 +913,21 @@ func restubIncomingRefsFromView( func evictFilesBatched(g graph.Store, paths []string) (int, int) { paths = appendUniqueSorted(nil, paths...) + // Stores written before the coverage-domain builders preserved the + // extractor's path spelling hold todo/fixture rows keyed by the + // forward-slash spelling of these same files (nothing else ever + // minted a re-spelled FilePath). Sweep that twin spelling so a + // file's replacement or deletion heals its stale rows; on POSIX + // the twin equals the native spelling and adds nothing. + var twins []string + for _, path := range paths { + if twin := filepath.ToSlash(path); twin != path { + twins = append(twins, twin) + } + } + if len(twins) > 0 { + paths = appendUniqueSorted(paths, twins...) + } if len(paths) == 0 { return 0, 0 } diff --git a/internal/indexer/incremental_reindex_test.go b/internal/indexer/incremental_reindex_test.go index e33904198..afa2658e8 100644 --- a/internal/indexer/incremental_reindex_test.go +++ b/internal/indexer/incremental_reindex_test.go @@ -204,6 +204,107 @@ func TestIncrementalReindex_FailedFileSurfacedAndRetried(t *testing.T) { assert.NotEmpty(t, g.FindNodesByName("Bad")) } +// seedSlashSpelledTodoRow plants the coverage-domain rows a pre-fix +// store holds on Windows: the builders re-spelled the extractor's +// relPath with forward slashes, so a subdirectory file's todo node and +// annotated edge were keyed by a spelling native eviction never sweeps. +// Returns the stale node ID. +func seedSlashSpelledTodoRow(g *graph.Graph, nativeRel string) string { + slashRel := filepath.ToSlash(nativeRel) + staleID := slashRel + "::todo:99" + g.AddNode(&graph.Node{ + ID: staleID, + Kind: graph.KindTodo, + Name: "todo:99", + FilePath: slashRel, + StartLine: 99, + EndLine: 99, + Language: "go", + Meta: map[string]any{"tag": "NOTE", "text": "stale spelling"}, + }) + g.AddEdge(&graph.Edge{ + From: slashRel, + To: staleID, + Kind: graph.EdgeAnnotated, + FilePath: slashRel, + Line: 99, + Origin: graph.OriginASTResolved, + }) + return staleID +} + +// edgesTouching returns the graph's edges whose From or To equals id. +func edgesTouching(g graph.Store, id string) []*graph.Edge { + var out []*graph.Edge + for _, e := range g.AllEdges() { + if e != nil && (e.From == id || e.To == id) { + out = append(out, e) + } + } + return out +} + +// TestIncrementalReindex_SweepsSlashSpelledCoverageRows: stores written +// before the coverage-domain builders preserved the extractor's path +// spelling hold todo rows keyed by the forward-slash spelling of +// subdirectory files. Replacing the file must sweep that twin spelling +// too, or the stale row shadows the file forever on Windows. +func TestIncrementalReindex_SweepsSlashSpelledCoverageRows(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "pkg"), 0o755)) + rel := filepath.Join("pkg", "util.go") + writeFile(t, filepath.Join(dir, rel), + "package pkg\n\n// TODO: original marker\nfunc Util() {}\n") + + g := graph.New() + idx := newTestIndexer(g) + _, err := idx.Index(dir) + require.NoError(t, err) + + staleID := seedSlashSpelledTodoRow(g, rel) + require.NotNil(t, g.GetNode(staleID)) + + bumpMtime(t, filepath.Join(dir, rel), + "package pkg\n\n// TODO: edited marker\nfunc Util() {}\n") + _, err = idx.IncrementalReindexPaths(dir, nil) + require.NoError(t, err) + + assert.Nil(t, g.GetNode(staleID), + "the slash-spelled stale todo node must be swept on replacement") + assert.Empty(t, edgesTouching(g, staleID), + "the stale annotated edge must be swept with its node") + assert.NotNil(t, g.GetNode(rel+"::todo:3"), + "the fresh extraction's todo node rides the native spelling") +} + +// TestIncrementalReindex_SweepsSlashSpelledCoverageRowsOnDelete is the +// delete-lane sibling: removing the file from disk must also sweep the +// twin-spelled stale rows. +func TestIncrementalReindex_SweepsSlashSpelledCoverageRowsOnDelete(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "pkg"), 0o755)) + rel := filepath.Join("pkg", "util.go") + writeFile(t, filepath.Join(dir, rel), + "package pkg\n\n// TODO: original marker\nfunc Util() {}\n") + + g := graph.New() + idx := newTestIndexer(g) + _, err := idx.Index(dir) + require.NoError(t, err) + + staleID := seedSlashSpelledTodoRow(g, rel) + require.NotNil(t, g.GetNode(staleID)) + + require.NoError(t, os.Remove(filepath.Join(dir, rel))) + _, err = idx.IncrementalReindexPaths(dir, nil) + require.NoError(t, err) + + assert.Nil(t, g.GetNode(staleID), + "the slash-spelled stale todo node must be swept on deletion") + assert.Empty(t, edgesTouching(g, staleID), + "the stale annotated edge must be swept with its node") +} + // TestIncrementalReindex_MerkleMode exercises the BLAKE3 Merkle change // detector: a content edit is re-indexed, but a file merely touched // (new mtime, identical content) is not — the content-addressed tree From c8fd8fb8635ed7274fbc6fd8235307a8dc94ceca Mon Sep 17 00:00:00 2001 From: pbednarcik <29061766+pbednarcik@users.noreply.github.com> Date: Fri, 28 Aug 2026 13:29:35 +0200 Subject: [PATCH 3/9] fix: heal legacy coverage spellings by migration, not by twin eviction Review found that routing the forward-slash twin through generic file eviction corrupts shared coverage targets. Endpoint-touch eviction cannot tell a per-file artifact from a license/team/module node that many files reference: with the shared node anchored to the legacy path the sweep deletes it along with OTHER files' valid edges, and with it anchored elsewhere the stale legacy edge survives regardless. The reviewer reproduced both directions; a both-backends regression here reproduces them too (and shows the shared node itself disappearing). The sweep is reverted. Legacy rows are instead healed once, by schema migration v13, which is what the incremental path could never be: selective. It deletes coverage edges by kind AND their own FilePath spelling, clears the per-file todo/fixture nodes with their endpoints and their symbol-FTS rows, and removes a shared target only after the purge leaves it with no references at all. 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. Three guards keep the purge off rows it must not touch. It runs only on a store holding backslash-bearing paths (a store written on POSIX is untouched); it judges a path only below the repo prefix, where a Windows-written store spells separators with a backslash, so top-level files - never damaged - never match; and it considers only the six coverage domains' own kinds. --- .../store_sqlite/coverage_spelling_purge.go | 206 ++++++++++++++ .../coverage_spelling_purge_test.go | 257 ++++++++++++++++++ internal/graph/store_sqlite/schema_version.go | 3 +- internal/indexer/incremental_batch.go | 23 +- internal/indexer/incremental_reindex_test.go | 161 +++++------ 5 files changed, 537 insertions(+), 113 deletions(-) create mode 100644 internal/graph/store_sqlite/coverage_spelling_purge.go create mode 100644 internal/graph/store_sqlite/coverage_spelling_purge_test.go diff --git a/internal/graph/store_sqlite/coverage_spelling_purge.go b/internal/graph/store_sqlite/coverage_spelling_purge.go new file mode 100644 index 000000000..5536272fd --- /dev/null +++ b/internal/graph/store_sqlite/coverage_spelling_purge.go @@ -0,0 +1,206 @@ +package store_sqlite + +import ( + "database/sql" + "strings" +) + +// Coverage-domain artifact kinds, split by ownership. +// +// - per-file artifacts belong to exactly one file: the file's spelling IS +// their identity, so a legacy-spelled row is unambiguously garbage. +// - shared targets (a license, a team, a module) are referenced by many +// files. Their FilePath is a first-sighting breadcrumb, never an +// ownership claim, so they are NEVER selected by path — they leave only +// when the purge has removed their last reference. +const ( + coveragePerFileNodeKinds = `'todo','fixture'` + coverageEdgeKinds = `'annotated','licensed_as','owns','generated_by','depends_on_module'` +) + +// purgeLegacyCoverageSpellings removes the coverage-domain rows that +// pre-fix binaries minted under a re-spelled file path. +// +// Until the builders preserved the extractor's spelling, todos / licenses / +// ownership / codegen / fixtures / modules ran their relPath through +// filepath.ToSlash before minting node IDs, FilePath fields, and edge +// endpoints. Everything else in the pipeline keys file identity by the +// indexer's exact spelling — OS-native separators below the repo prefix — +// so on Windows these rows are invisible to eviction, which matches nodes by +// file_path and edges by evicted-endpoint touch. They are never swept, never +// replaced, and never re-created in the new spelling until their file is +// re-parsed, so an upgraded store keeps serving stale TODO text and dangling +// licensed_as / owns / generated_by / depends_on_module edges indefinitely — +// including for files nothing touches again. +// +// Scope, deliberately narrow in three directions: +// +// 1. Store-level guard. The purge runs only when the store holds at least +// one backslash-bearing file_path, i.e. it was written by a Windows +// indexer. On a store written on POSIX every path IS the native +// spelling and the whole migration is a no-op. +// 2. Path-level predicate. Below the repo prefix a Windows-written store +// spells separators with a backslash, so a forward slash there marks a +// row no current builder could have produced. Top-level files (no +// separator below the prefix) were never damaged and never match. +// 3. Kind-level predicate. Only the six coverage domains' own kinds are +// considered. A shared target is removed only after the edge purge +// leaves it with no references at all — never because a purged file +// happened to be its first sighting. +// +// Idempotent: a second run finds no legacy-spelled rows. Bounded to the +// coverage kinds: language-extractor nodes and edges are never candidates. +func purgeLegacyCoverageSpellings(tx *sql.Tx) error { + windowsWritten, err := storeHasNativeBackslashPaths(tx) + if err != nil || !windowsWritten { + return err + } + prefixes, err := storeRepoPrefixes(tx) + if err != nil { + return err + } + legacyNodePath := legacyPathPredicate("nodes.file_path", prefixes) + legacyEdgePath := legacyPathPredicate("edges.file_path", prefixes) + + // Per-file artifacts whose own spelling is legacy. Collected before any + // delete so the edge sweep below can clear their endpoints too. The + // DROPs make the step re-entrant on a connection that carried a temp + // table over from an earlier attempt. + if _, err := tx.Exec(`DROP TABLE IF EXISTS covdom_doomed_nodes`); err != nil { + return err + } + if _, err := tx.Exec(`CREATE TEMP TABLE covdom_doomed_nodes AS + SELECT id FROM nodes + WHERE kind IN (` + coveragePerFileNodeKinds + `) AND ` + legacyNodePath); err != nil { + return err + } + defer func() { _, _ = tx.Exec(`DROP TABLE IF EXISTS covdom_doomed_nodes`) }() + + // Shared targets the legacy edges point at. Snapshotted BEFORE the edge + // delete, because afterwards nothing links them to this purge. + if _, err := tx.Exec(`DROP TABLE IF EXISTS covdom_shared_targets`); err != nil { + return err + } + if _, err := tx.Exec(`CREATE TEMP TABLE covdom_shared_targets AS + SELECT DISTINCT to_id AS id FROM edges + WHERE kind IN ('licensed_as','generated_by','depends_on_module') AND ` + legacyEdgePath + ` + UNION + SELECT DISTINCT from_id AS id FROM edges + WHERE kind = 'owns' AND ` + legacyEdgePath); err != nil { + return err + } + defer func() { _, _ = tx.Exec(`DROP TABLE IF EXISTS covdom_shared_targets`) }() + + // Legacy coverage edges, selected by kind AND their own FilePath + // spelling — never by touching an evicted endpoint, which would take + // a shared target's other, still-valid edges with it. + if _, err := tx.Exec(`DELETE FROM edges + WHERE kind IN (` + coverageEdgeKinds + `) AND ` + legacyEdgePath); err != nil { + return err + } + // Any remaining edge on a doomed per-file artifact: its node is going, + // so leaving the edge would strand a dangling endpoint. Safe here (and + // only here) because these nodes are owned by exactly one file. Two + // statements rather than one OR: each side then seeks through its own + // endpoint index instead of scanning the edge table. + for _, column := range []string{"from_id", "to_id"} { + if _, err := tx.Exec(`DELETE FROM edges + WHERE ` + column + ` IN (SELECT id FROM covdom_doomed_nodes)`); err != nil { + return err + } + } + + // A shared target joins the doomed set only once the purge has left it + // with no references at all. The two NOT EXISTS probes are likewise + // kept apart so each rides an endpoint index. + if _, err := tx.Exec(`INSERT INTO covdom_doomed_nodes(id) + SELECT t.id FROM covdom_shared_targets t + WHERE EXISTS (SELECT 1 FROM nodes n WHERE n.id = t.id) + AND t.id NOT IN (SELECT id FROM covdom_doomed_nodes) + AND NOT EXISTS (SELECT 1 FROM edges e WHERE e.from_id = t.id) + AND NOT EXISTS (SELECT 1 FROM edges e WHERE e.to_id = t.id)`); err != nil { + return err + } + + // Symbol FTS rows outlive their node unless deleted explicitly (see + // BatchDeleteSymbolFTS, the eviction lane's equivalent) — a purged todo + // would otherwise keep answering searches with its stale text. + if _, err := tx.Exec(`DELETE FROM symbol_fts WHERE rowid IN ( + SELECT fts_rowid FROM symbol_fts_rowid + WHERE node_id IN (SELECT id FROM covdom_doomed_nodes))`); err != nil { + return err + } + if _, err := tx.Exec(`DELETE FROM symbol_fts_rowid + WHERE node_id IN (SELECT id FROM covdom_doomed_nodes)`); err != nil { + return err + } + _, err = tx.Exec(`DELETE FROM nodes WHERE id IN (SELECT id FROM covdom_doomed_nodes)`) + return err +} + +// storeHasNativeBackslashPaths reports whether any node path carries a +// backslash, i.e. the store was written by an indexer on a platform whose +// separator is not '/'. It is the migration's outer guard: on a store +// written on POSIX no path can be a re-spelled twin of another, so the +// purge must not run at all. +func storeHasNativeBackslashPaths(tx *sql.Tx) (bool, error) { + var present int + err := tx.QueryRow(`SELECT EXISTS(SELECT 1 FROM nodes WHERE instr(file_path, '\') > 0)`).Scan(&present) + return present == 1, err +} + +// storeRepoPrefixes returns the distinct repository prefixes the store's +// nodes carry. Multi-repo IDs and paths are `/`: that single +// separator is always a forward slash regardless of platform, so it must be +// stripped before a path is judged on its remaining separators. +func storeRepoPrefixes(tx *sql.Tx) ([]string, error) { + rows, err := tx.Query(`SELECT DISTINCT repo_prefix FROM nodes WHERE repo_prefix <> ''`) + if err != nil { + return nil, err + } + defer rows.Close() //nolint:errcheck // read-only cursor + var prefixes []string + for rows.Next() { + var prefix string + if err := rows.Scan(&prefix); err != nil { + return nil, err + } + prefixes = append(prefixes, prefix) + } + return prefixes, rows.Err() +} + +// legacyPathPredicate builds the SQL test "this path is a pre-fix +// re-spelling": strip the `/` prefix when one applies, then look for a +// forward slash in what remains. On a Windows-written store (the only place +// this runs — see storeHasNativeBackslashPaths) the remainder's separators +// are backslashes, so a forward slash there cannot come from a current +// builder. +// +// Prefixes are embedded as escaped literals rather than bound parameters +// because the predicate is spliced into CREATE TEMP TABLE ... AS SELECT +// statements. The values are the store's own repo_prefix column, and +// quoteSQLLiteral doubles any embedded quote. Exact string comparison, not +// LIKE, so a prefix containing '%' or '_' cannot match a sibling repo. +func legacyPathPredicate(column string, prefixes []string) string { + // A single-repo store carries no prefix: the whole path is the portion + // under test. (SQL CASE requires at least one WHEN, so this branch is + // not merely an optimisation.) + if len(prefixes) == 0 { + return "instr(" + column + ", '/') > 0" + } + var b strings.Builder + b.WriteString("instr(CASE") + for _, prefix := range prefixes { + lit := quoteSQLLiteral(prefix + "/") + b.WriteString(" WHEN substr(" + column + ", 1, length(" + lit + ")) = " + lit + + " THEN substr(" + column + ", length(" + lit + ") + 1)") + } + b.WriteString(" ELSE " + column + " END, '/') > 0") + return b.String() +} + +// quoteSQLLiteral renders s as a single-quoted SQLite string literal. +func quoteSQLLiteral(s string) string { + return "'" + strings.ReplaceAll(s, "'", "''") + "'" +} diff --git a/internal/graph/store_sqlite/coverage_spelling_purge_test.go b/internal/graph/store_sqlite/coverage_spelling_purge_test.go new file mode 100644 index 000000000..b05df392f --- /dev/null +++ b/internal/graph/store_sqlite/coverage_spelling_purge_test.go @@ -0,0 +1,257 @@ +package store_sqlite + +import ( + "database/sql" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/zzet/gortex/internal/graph" +) + +// countFTSRowidRows returns the symbol_fts_rowid rows mapped to nodeID. +func countFTSRowidRows(t *testing.T, path, nodeID string) int { + t.Helper() + var n int + withRawDB(t, path, func(db *sql.DB) { + require.NoError(t, db.QueryRow( + `SELECT COUNT(*) FROM symbol_fts_rowid WHERE node_id = ?`, nodeID).Scan(&n)) + }) + return n +} + +// TestOpenPurgesLegacyCoverageSpellings is the upgrade proof for the +// coverage-domain path-spelling purge. Stores written on Windows before +// the builders preserved the extractor's path spelling hold todo/fixture +// nodes and licensed_as / owns / generated_by / depends_on_module / +// annotated edges keyed by the forward-slash twin of the native +// backslash spelling. Nothing evicts those rows (eviction is +// spelling-exact), so a versioned migration removes them: per-file +// artifact nodes and coverage edges selectively by kind + FilePath +// spelling, shared targets only once nothing references them. Every +// native-spelled row must survive untouched. +func TestOpenPurgesLegacyCoverageSpellings(t *testing.T) { + path := filepath.Join(t.TempDir(), "store.sqlite") + + const ( + nativeA = `r/src\a.go` + nativeB = `r/src\b.go` + legacyA = `r/src/a.go` + + nativeTodo = nativeA + `::todo:5` + legacyTodo = legacyA + `::todo:3` + legacyFix = `r/testdata/x.json` + licMIT = `r/license::MIT` + licGPL = `r/license::GPL-3.0` + teamCore = `r/team::core` + moduleX = `r/module::go::example.com/x@v1` + nativeMod = `r/sub\go.mod` + legacyMod = `r/sub/go.mod` + genExternal = `r/external::protoc` + ) + + s, err := Open(path) + require.NoError(t, err) + s.AddBatch([]*graph.Node{ + // Native file nodes: prove the store keys paths with backslashes. + {ID: nativeA, Kind: graph.KindFile, Name: "a.go", FilePath: nativeA, RepoPrefix: "r"}, + {ID: nativeB, Kind: graph.KindFile, Name: "b.go", FilePath: nativeB, RepoPrefix: "r"}, + // Native todo: must survive. + {ID: nativeTodo, Kind: graph.KindTodo, Name: "todo:5", FilePath: nativeA, RepoPrefix: "r"}, + // Legacy per-file artifacts: must be purged. + {ID: legacyTodo, Kind: graph.KindTodo, Name: "todo:3", FilePath: legacyA, RepoPrefix: "r"}, + {ID: legacyFix, Kind: graph.KindFixture, Name: "x.json", FilePath: legacyFix, RepoPrefix: "r"}, + // Shared target anchored to the LEGACY spelling but still + // referenced natively by b: node must survive, anchor and all. + {ID: licMIT, Kind: graph.KindLicense, Name: "MIT", FilePath: legacyA, RepoPrefix: "r"}, + // Shared target whose only reference is legacy: orphaned after + // the edge purge, so the node goes too. + {ID: licGPL, Kind: graph.KindLicense, Name: "GPL-3.0", FilePath: legacyA, RepoPrefix: "r"}, + // Team referenced by a non-coverage authored edge: the legacy + // owns edge is purged but the node must survive. + {ID: teamCore, Kind: graph.KindTeam, Name: "core", FilePath: legacyA, RepoPrefix: "r"}, + // Shared target anchored NATIVELY with one legacy + one native + // edge: node and native edge survive. + {ID: moduleX, Kind: graph.KindModule, Name: "x", FilePath: nativeMod, RepoPrefix: "r"}, + }, []*graph.Edge{ + // Native rows: every one must survive. + {From: nativeA, To: nativeTodo, Kind: graph.EdgeAnnotated, FilePath: nativeA, Line: 5}, + {From: nativeB, To: licMIT, Kind: graph.EdgeLicensedAs, FilePath: nativeB}, + {From: teamCore, To: nativeB, Kind: graph.EdgeAuthored, FilePath: nativeB}, + {From: nativeMod, To: moduleX, Kind: graph.EdgeDependsOnModule, FilePath: nativeMod, Line: 2}, + // Legacy rows: every one must be purged. + {From: legacyA, To: legacyTodo, Kind: graph.EdgeAnnotated, FilePath: legacyA, Line: 3}, + {From: legacyA, To: licMIT, Kind: graph.EdgeLicensedAs, FilePath: legacyA}, + {From: legacyA, To: licGPL, Kind: graph.EdgeLicensedAs, FilePath: legacyA}, + {From: teamCore, To: legacyA, Kind: graph.EdgeOwns, FilePath: legacyA}, + {From: legacyA, To: genExternal, Kind: graph.EdgeGeneratedBy, FilePath: legacyA}, + {From: legacyMod, To: moduleX, Kind: graph.EdgeDependsOnModule, FilePath: legacyMod, Line: 2}, + }) + // Legacy artifact nodes were FTS-indexed by the old binary; the purge + // must clear their search rows so no ghost hits outlive the nodes. + require.NoError(t, s.UpsertSymbolFTS(legacyTodo, "stale marker")) + require.NoError(t, s.UpsertSymbolFTS(nativeTodo, "live marker")) + require.NoError(t, s.Close()) + + // Simulate a store written before the purge shipped. + withRawDB(t, path, func(db *sql.DB) { + _, err := db.Exec(`PRAGMA user_version = 12`) + require.NoError(t, err, "reset to the pre-purge version") + }) + + s2, err := Open(path) + require.NoError(t, err) + + // Purged rows. + require.Nil(t, s2.GetNode(legacyTodo), "legacy todo node must be purged") + require.Nil(t, s2.GetNode(legacyFix), "legacy fixture node must be purged") + require.Nil(t, s2.GetNode(licGPL), "orphaned shared license must be removed") + require.Empty(t, s2.GetOutEdges(legacyA), "no legacy-spelled coverage edge may survive") + // Survivors. + require.NotNil(t, s2.GetNode(nativeTodo), "native todo node must survive") + require.NotNil(t, s2.GetNode(licMIT), "shared license anchored to a legacy path stays while referenced") + require.NotNil(t, s2.GetNode(teamCore), "team referenced by an authored edge stays") + require.NotNil(t, s2.GetNode(moduleX), "natively referenced module stays") + + inMIT := s2.GetInEdges(licMIT) + require.Len(t, inMIT, 1, "exactly b's native licensed_as edge remains on MIT") + require.Equal(t, nativeB, inMIT[0].From) + + outTeam := s2.GetOutEdges(teamCore) + require.Len(t, outTeam, 1, "only the authored edge remains on the team") + require.Equal(t, graph.EdgeAuthored, outTeam[0].Kind) + + inMod := s2.GetInEdges(moduleX) + require.Len(t, inMod, 1, "exactly the native depends_on_module edge remains") + require.Equal(t, nativeMod, inMod[0].From) + + outA := s2.GetOutEdges(nativeA) + require.Len(t, outA, 1, "the native annotated edge survives") + require.Equal(t, graph.EdgeAnnotated, outA[0].Kind) + + require.NoError(t, s2.Close()) + require.Zero(t, countFTSRowidRows(t, path, legacyTodo), "purged node's FTS rows must go with it") + require.NotZero(t, countFTSRowidRows(t, path, nativeTodo), "surviving node keeps its FTS rows") +} + +// TestOpenPurgesLegacyCoverageSpellingsSingleRepo covers the unprefixed +// store shape: no repo prefix means the whole FilePath is the path +// portion, so any forward slash marks the legacy twin on a +// backslash-keyed store. +func TestOpenPurgesLegacyCoverageSpellingsSingleRepo(t *testing.T) { + path := filepath.Join(t.TempDir(), "store.sqlite") + + const ( + nativeA = `src\a.go` + legacyA = `src/a.go` + nativeTodo = nativeA + `::todo:5` + legacyTodo = legacyA + `::todo:3` + ) + + s, err := Open(path) + require.NoError(t, err) + s.AddBatch([]*graph.Node{ + {ID: nativeA, Kind: graph.KindFile, Name: "a.go", FilePath: nativeA}, + {ID: nativeTodo, Kind: graph.KindTodo, Name: "todo:5", FilePath: nativeA}, + {ID: legacyTodo, Kind: graph.KindTodo, Name: "todo:3", FilePath: legacyA}, + }, []*graph.Edge{ + {From: nativeA, To: nativeTodo, Kind: graph.EdgeAnnotated, FilePath: nativeA, Line: 5}, + {From: legacyA, To: legacyTodo, Kind: graph.EdgeAnnotated, FilePath: legacyA, Line: 3}, + }) + require.NoError(t, s.Close()) + + withRawDB(t, path, func(db *sql.DB) { + _, err := db.Exec(`PRAGMA user_version = 12`) + require.NoError(t, err, "reset to the pre-purge version") + }) + + s2, err := Open(path) + require.NoError(t, err) + t.Cleanup(func() { _ = s2.Close() }) + + require.Nil(t, s2.GetNode(legacyTodo), "legacy todo node must be purged") + require.Empty(t, s2.GetOutEdges(legacyA), "legacy annotated edge must be purged") + require.NotNil(t, s2.GetNode(nativeTodo), "native todo node must survive") + require.Len(t, s2.GetOutEdges(nativeA), 1, "native annotated edge must survive") +} + +// TestPurgeLegacyCoverageSpellingsIsIdempotent runs the step twice on +// one connection: the second pass must find nothing left to remove and +// must not trip over the temp tables the first pass created. +func TestPurgeLegacyCoverageSpellingsIsIdempotent(t *testing.T) { + path := filepath.Join(t.TempDir(), "store.sqlite") + + const ( + nativeA = `r/src\a.go` + legacyA = `r/src/a.go` + legacyTodo = legacyA + `::todo:3` + lic = `r/license::MIT` + ) + + s, err := Open(path) + require.NoError(t, err) + s.AddBatch([]*graph.Node{ + {ID: nativeA, Kind: graph.KindFile, Name: "a.go", FilePath: nativeA, RepoPrefix: "r"}, + {ID: legacyTodo, Kind: graph.KindTodo, Name: "todo:3", FilePath: legacyA, RepoPrefix: "r"}, + {ID: lic, Kind: graph.KindLicense, Name: "MIT", FilePath: legacyA, RepoPrefix: "r"}, + }, []*graph.Edge{ + {From: legacyA, To: legacyTodo, Kind: graph.EdgeAnnotated, FilePath: legacyA, Line: 3}, + {From: legacyA, To: lic, Kind: graph.EdgeLicensedAs, FilePath: legacyA}, + }) + require.NoError(t, s.Close()) + + withRawDB(t, path, func(db *sql.DB) { + nodesAfter := func() int { + var n int + require.NoError(t, db.QueryRow(`SELECT COUNT(*) FROM nodes`).Scan(&n)) + return n + } + for pass := 1; pass <= 2; pass++ { + tx, err := db.Begin() + require.NoError(t, err) + require.NoError(t, purgeLegacyCoverageSpellings(tx), "pass %d", pass) + require.NoError(t, tx.Commit()) + } + require.Equal(t, 1, nodesAfter(), "only the native file node survives both passes") + }) +} + +// TestOpenLeavesPosixCoverageRowsUntouched pins the guard: on a store +// with no backslash-keyed paths every row IS the native spelling, so +// the purge must not run at all. Without the guard the migration would +// eat every coverage artifact on every POSIX store. +func TestOpenLeavesPosixCoverageRowsUntouched(t *testing.T) { + path := filepath.Join(t.TempDir(), "store.sqlite") + + const ( + fileA = `r/src/a.go` + todoA = `r/src/a.go::todo:3` + lic = `r/license::MIT` + ) + + s, err := Open(path) + require.NoError(t, err) + s.AddBatch([]*graph.Node{ + {ID: fileA, Kind: graph.KindFile, Name: "a.go", FilePath: fileA, RepoPrefix: "r"}, + {ID: todoA, Kind: graph.KindTodo, Name: "todo:3", FilePath: fileA, RepoPrefix: "r"}, + {ID: lic, Kind: graph.KindLicense, Name: "MIT", FilePath: fileA, RepoPrefix: "r"}, + }, []*graph.Edge{ + {From: fileA, To: todoA, Kind: graph.EdgeAnnotated, FilePath: fileA, Line: 3}, + {From: fileA, To: lic, Kind: graph.EdgeLicensedAs, FilePath: fileA}, + }) + require.NoError(t, s.Close()) + + withRawDB(t, path, func(db *sql.DB) { + _, err := db.Exec(`PRAGMA user_version = 12`) + require.NoError(t, err, "reset to the pre-purge version") + }) + + s2, err := Open(path) + require.NoError(t, err) + t.Cleanup(func() { _ = s2.Close() }) + + require.NotNil(t, s2.GetNode(todoA), "POSIX todo node must survive") + require.NotNil(t, s2.GetNode(lic), "POSIX license node must survive") + require.Len(t, s2.GetOutEdges(fileA), 2, "both POSIX coverage edges must survive") +} diff --git a/internal/graph/store_sqlite/schema_version.go b/internal/graph/store_sqlite/schema_version.go index 22ce83d7f..3d89118ca 100644 --- a/internal/graph/store_sqlite/schema_version.go +++ b/internal/graph/store_sqlite/schema_version.go @@ -32,7 +32,7 @@ import ( // index changes in a way an old on-disk DB would not already have, and append a // matching schemaMigrations entry describing how to bring an older store // forward (in place, or by rebuild). -const currentSchemaVersion = 12 +const currentSchemaVersion = 13 // schemaMigration is one forward step. Exactly one strategy applies: // - rebuild=true: the change introduces structure/data that can only come @@ -80,6 +80,7 @@ var schemaMigrations = []schemaMigration{ {version: 10, name: "rebuild vector corpus ownership and parents", inPlace: rebuildVectorCorpusSchema}, {version: 11, name: "add symbol FTS normalization state", inPlace: createSymbolFTSNormalizationStateTable}, {version: 12, name: "normalize dir column separators", inPlace: normalizeDirColumnSeparators}, + {version: 13, name: "purge legacy slash-spelled coverage artifacts", inPlace: purgeLegacyCoverageSpellings}, } // normalizeDirColumnSeparators rebuilds the two generated dir columns whose diff --git a/internal/indexer/incremental_batch.go b/internal/indexer/incremental_batch.go index e1e556cc3..fc9f648aa 100644 --- a/internal/indexer/incremental_batch.go +++ b/internal/indexer/incremental_batch.go @@ -913,21 +913,14 @@ func restubIncomingRefsFromView( func evictFilesBatched(g graph.Store, paths []string) (int, int) { paths = appendUniqueSorted(nil, paths...) - // Stores written before the coverage-domain builders preserved the - // extractor's path spelling hold todo/fixture rows keyed by the - // forward-slash spelling of these same files (nothing else ever - // minted a re-spelled FilePath). Sweep that twin spelling so a - // file's replacement or deletion heals its stale rows; on POSIX - // the twin equals the native spelling and adds nothing. - var twins []string - for _, path := range paths { - if twin := filepath.ToSlash(path); twin != path { - twins = append(twins, twin) - } - } - if len(twins) > 0 { - paths = appendUniqueSorted(paths, twins...) - } + // Only the caller's own spellings are evicted. Rows a pre-fix binary + // wrote under a re-spelled path are healed once, by schema migration + // v13 (purgeLegacyCoverageSpellings): sweeping a twin spelling here + // would route it through endpoint-touch eviction, which cannot tell a + // shared coverage target (a license, a team, a module) from a + // per-file artifact and so would take other files' valid edges with + // it — or miss the stale ones, depending on which file the shared + // node happened to be anchored to. if len(paths) == 0 { return 0, 0 } diff --git a/internal/indexer/incremental_reindex_test.go b/internal/indexer/incremental_reindex_test.go index afa2658e8..6f8d4c8bc 100644 --- a/internal/indexer/incremental_reindex_test.go +++ b/internal/indexer/incremental_reindex_test.go @@ -14,6 +14,7 @@ import ( "github.com/zzet/gortex/internal/excludes" "github.com/zzet/gortex/internal/graph" + "github.com/zzet/gortex/internal/graph/store_sqlite" ) // TestIncrementalReindex_EvictsExcludedFiles is the regression for #321: @@ -204,105 +205,71 @@ func TestIncrementalReindex_FailedFileSurfacedAndRetried(t *testing.T) { assert.NotEmpty(t, g.FindNodesByName("Bad")) } -// seedSlashSpelledTodoRow plants the coverage-domain rows a pre-fix -// store holds on Windows: the builders re-spelled the extractor's -// relPath with forward slashes, so a subdirectory file's todo node and -// annotated edge were keyed by a spelling native eviction never sweeps. -// Returns the stale node ID. -func seedSlashSpelledTodoRow(g *graph.Graph, nativeRel string) string { - slashRel := filepath.ToSlash(nativeRel) - staleID := slashRel + "::todo:99" - g.AddNode(&graph.Node{ - ID: staleID, - Kind: graph.KindTodo, - Name: "todo:99", - FilePath: slashRel, - StartLine: 99, - EndLine: 99, - Language: "go", - Meta: map[string]any{"tag": "NOTE", "text": "stale spelling"}, - }) - g.AddEdge(&graph.Edge{ - From: slashRel, - To: staleID, - Kind: graph.EdgeAnnotated, - FilePath: slashRel, - Line: 99, - Origin: graph.OriginASTResolved, - }) - return staleID -} - -// edgesTouching returns the graph's edges whose From or To equals id. -func edgesTouching(g graph.Store, id string) []*graph.Edge { - var out []*graph.Edge - for _, e := range g.AllEdges() { - if e != nil && (e.From == id || e.To == id) { - out = append(out, e) - } +// TestEvictFilesBatched_EvictsOnlyTheCallerSpelling pins the eviction +// contract both storage backends must honour: a file's eviction touches +// that file's own spelling and nothing else. +// +// The shape is the one a Windows store written by a pre-fix binary has: +// native `src\a.go` and `src\b.go` both hold a licensed_as edge into the +// SHARED `license::MIT` node, and a legacy `src/a.go` edge lingers beside +// them. Sweeping a's forward-slash twin through file eviction would +// delete every edge touching whichever node that sweep evicts — so with +// the shared node anchored to the legacy path it takes b's valid edge +// down with it, and with it anchored elsewhere the legacy edge survives +// anyway. Legacy rows are healed once by schema migration v13 +// (purgeLegacyCoverageSpellings), which deletes by edge kind + FilePath +// and drops a shared target only when nothing references it. +func TestEvictFilesBatched_EvictsOnlyTheCallerSpelling(t *testing.T) { + const ( + nativeA = `src\a.go` + nativeB = `src\b.go` + legacyA = `src/a.go` + license = "license::MIT" + ) + nodes := []*graph.Node{ + {ID: nativeA, Kind: graph.KindFile, Name: "a.go", FilePath: nativeA}, + {ID: nativeB, Kind: graph.KindFile, Name: "b.go", FilePath: nativeB}, + // Anchored to the legacy spelling — the first-sighting FilePath + // of a shared coverage target is a breadcrumb, not ownership. + {ID: license, Kind: graph.KindLicense, Name: "MIT", FilePath: legacyA}, + } + edges := []*graph.Edge{ + {From: nativeA, To: license, Kind: graph.EdgeLicensedAs, FilePath: nativeA}, + {From: nativeB, To: license, Kind: graph.EdgeLicensedAs, FilePath: nativeB}, + {From: legacyA, To: license, Kind: graph.EdgeLicensedAs, FilePath: legacyA}, } - return out -} - -// TestIncrementalReindex_SweepsSlashSpelledCoverageRows: stores written -// before the coverage-domain builders preserved the extractor's path -// spelling hold todo rows keyed by the forward-slash spelling of -// subdirectory files. Replacing the file must sweep that twin spelling -// too, or the stale row shadows the file forever on Windows. -func TestIncrementalReindex_SweepsSlashSpelledCoverageRows(t *testing.T) { - dir := t.TempDir() - require.NoError(t, os.MkdirAll(filepath.Join(dir, "pkg"), 0o755)) - rel := filepath.Join("pkg", "util.go") - writeFile(t, filepath.Join(dir, rel), - "package pkg\n\n// TODO: original marker\nfunc Util() {}\n") - - g := graph.New() - idx := newTestIndexer(g) - _, err := idx.Index(dir) - require.NoError(t, err) - - staleID := seedSlashSpelledTodoRow(g, rel) - require.NotNil(t, g.GetNode(staleID)) - - bumpMtime(t, filepath.Join(dir, rel), - "package pkg\n\n// TODO: edited marker\nfunc Util() {}\n") - _, err = idx.IncrementalReindexPaths(dir, nil) - require.NoError(t, err) - - assert.Nil(t, g.GetNode(staleID), - "the slash-spelled stale todo node must be swept on replacement") - assert.Empty(t, edgesTouching(g, staleID), - "the stale annotated edge must be swept with its node") - assert.NotNil(t, g.GetNode(rel+"::todo:3"), - "the fresh extraction's todo node rides the native spelling") -} - -// TestIncrementalReindex_SweepsSlashSpelledCoverageRowsOnDelete is the -// delete-lane sibling: removing the file from disk must also sweep the -// twin-spelled stale rows. -func TestIncrementalReindex_SweepsSlashSpelledCoverageRowsOnDelete(t *testing.T) { - dir := t.TempDir() - require.NoError(t, os.MkdirAll(filepath.Join(dir, "pkg"), 0o755)) - rel := filepath.Join("pkg", "util.go") - writeFile(t, filepath.Join(dir, rel), - "package pkg\n\n// TODO: original marker\nfunc Util() {}\n") - - g := graph.New() - idx := newTestIndexer(g) - _, err := idx.Index(dir) - require.NoError(t, err) - - staleID := seedSlashSpelledTodoRow(g, rel) - require.NotNil(t, g.GetNode(staleID)) - - require.NoError(t, os.Remove(filepath.Join(dir, rel))) - _, err = idx.IncrementalReindexPaths(dir, nil) - require.NoError(t, err) - assert.Nil(t, g.GetNode(staleID), - "the slash-spelled stale todo node must be swept on deletion") - assert.Empty(t, edgesTouching(g, staleID), - "the stale annotated edge must be swept with its node") + for _, backend := range []struct { + name string + open func(t *testing.T) graph.Store + }{ + {"graph", func(t *testing.T) graph.Store { return graph.New() }}, + {"sqlite", func(t *testing.T) graph.Store { + s, err := store_sqlite.Open(filepath.Join(t.TempDir(), "store.sqlite")) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + return s + }}, + } { + t.Run(backend.name, func(t *testing.T) { + g := backend.open(t) + g.AddBatch(nodes, edges) + + evictFilesBatched(g, []string{nativeA}) + + var froms []string + for _, e := range g.GetInEdges(license) { + if e != nil { + froms = append(froms, e.From) + } + } + assert.ElementsMatch(t, []string{nativeB, legacyA}, froms, + "only a's own spelling is evicted: b keeps its valid edge, "+ + "and the legacy row is the migration's business") + assert.NotNil(t, g.GetNode(license), + "the shared license node outlives one of its files") + }) + } } // TestIncrementalReindex_MerkleMode exercises the BLAKE3 Merkle change From f62145788a1d91cd075e684bee45749141c964b2 Mon Sep 17 00:00:00 2001 From: pbednarcik <29061766+pbednarcik@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:50:48 +0200 Subject: [PATCH 4/9] test: builder spelling contracts take an explicit Windows path Review found the six tests inert on the hosted runners, and the reason generalizes: they built the path with filepath.Join, but the call the fix removed was filepath.ToSlash, which is the identity on POSIX. Both spellings collapse to one string there, so the assertions held with or without the fix, and the Windows CI job only cross-compiles. Each test now names the spelling under test as a literal carrying a backslash. That does not make the tests bind the ToSlash regression on POSIX - nothing can, since the removed call was a no-op there - but it does pin a byte-identity contract that holds on every platform: the builder returns the caller's spelling verbatim. Any future unconditional re-spelling, whether ToSlash or strings.ReplaceAll or path.Clean, fails these on every runner rather than only on Windows. Binding the original regression itself remains a Windows-only run. The fixtures case keeps a forward slash on the qualifying testdata segment, because IsFixturePath normalizes through filepath.ToSlash, and asserts only ID and FilePath: Name comes from filepath.Base, whose separator set is the running platform's, and TestBuildGraphArtifacts already covers it. --- internal/codegen/scanner_test.go | 6 ++++-- internal/codeowners/parser_test.go | 5 ++++- internal/fixtures/scanner_test.go | 13 ++++++++----- internal/licenses/scanner_test.go | 8 +++++--- internal/modules/scanner_test.go | 6 ++++-- internal/todos/scanner_test.go | 8 ++++++-- 6 files changed, 31 insertions(+), 15 deletions(-) diff --git a/internal/codegen/scanner_test.go b/internal/codegen/scanner_test.go index f3988b1fb..690f15d87 100644 --- a/internal/codegen/scanner_test.go +++ b/internal/codegen/scanner_test.go @@ -1,7 +1,6 @@ package codegen import ( - "path/filepath" "testing" "github.com/zzet/gortex/internal/graph" @@ -89,7 +88,10 @@ func TestBuildGraphArtifacts_PreservesCallerPathSpelling(t *testing.T) { // The indexer keys eviction and incremental replacement by the // exact relPath spelling it hands the builder; a re-spelled edge // endpoint dangles from a nonexistent file node on Windows. - rel := filepath.Join("src", "gen", "foo.pb.go") + // Written out, not composed with filepath.Join: on a POSIX runner + // Join yields exactly what the pre-fix ToSlash call returned, so + // the assertion would hold with or without the fix. + const rel = `src\gen\foo.pb.go` edges := BuildGraphArtifacts(rel, Marker{Generated: true, Tool: "protoc-gen-go"}) if len(edges) != 1 { t.Fatalf("edges = %d", len(edges)) diff --git a/internal/codeowners/parser_test.go b/internal/codeowners/parser_test.go index 51b0ad598..c7c2d3787 100644 --- a/internal/codeowners/parser_test.go +++ b/internal/codeowners/parser_test.go @@ -115,7 +115,10 @@ func TestBuildGraphArtifacts_PreservesCallerPathSpelling(t *testing.T) { // The indexer keys eviction and incremental replacement by the // exact relPath spelling it hands the builder; a re-spelled edge // endpoint dangles from a nonexistent file node on Windows. - rel := filepath.Join("src", "data", "foo.go") + // Written out, not composed with filepath.Join: on a POSIX runner + // Join yields exactly what the pre-fix ToSlash call returned, so + // the assertion would hold with or without the fix. + const rel = `src\data\foo.go` nodes, edges := BuildGraphArtifacts(rel, []string{"@alice"}, "go") if len(nodes) != 1 || len(edges) != 1 { t.Fatalf("nodes = %d, edges = %d", len(nodes), len(edges)) diff --git a/internal/fixtures/scanner_test.go b/internal/fixtures/scanner_test.go index 895089ed7..094b0b0a4 100644 --- a/internal/fixtures/scanner_test.go +++ b/internal/fixtures/scanner_test.go @@ -1,7 +1,6 @@ package fixtures import ( - "path/filepath" "testing" "github.com/zzet/gortex/internal/graph" @@ -89,7 +88,14 @@ func TestBuildGraphArtifacts_PreservesCallerPathSpelling(t *testing.T) { // as its node ID so it merges with the file identity; that only // works when the spelling matches the extractor's relPath exactly // (OS-native separators for subdirectory files on Windows). - rel := filepath.Join("pkg", "testdata", "foo.bin") + // The spelling is written out rather than composed with + // filepath.Join so a backslash is present on every runner. The + // qualifying `testdata/` segment keeps its forward slash because + // IsFixturePath normalizes through filepath.ToSlash, which is the + // identity on POSIX. Name is asserted by TestBuildGraphArtifacts: + // it comes from filepath.Base, whose separator set is the running + // platform's. + const rel = `testdata/sub\foo.bin` nodes := BuildGraphArtifacts(rel, "binary") if len(nodes) != 1 { t.Fatalf("nodes = %d", len(nodes)) @@ -100,9 +106,6 @@ func TestBuildGraphArtifacts_PreservesCallerPathSpelling(t *testing.T) { if nodes[0].FilePath != rel { t.Errorf("node file path = %q, want %q", nodes[0].FilePath, rel) } - if nodes[0].Name != "foo.bin" { - t.Errorf("node name = %q", nodes[0].Name) - } } func TestReclassifyFileToFixture(t *testing.T) { diff --git a/internal/licenses/scanner_test.go b/internal/licenses/scanner_test.go index 099a753ee..d2925f4cb 100644 --- a/internal/licenses/scanner_test.go +++ b/internal/licenses/scanner_test.go @@ -1,7 +1,6 @@ package licenses import ( - "path/filepath" "testing" "github.com/zzet/gortex/internal/graph" @@ -76,8 +75,11 @@ func TestBuildGraphArtifacts(t *testing.T) { func TestBuildGraphArtifacts_PreservesCallerPathSpelling(t *testing.T) { // The indexer keys eviction and incremental replacement by the // exact relPath spelling it hands the builder; a re-spelled edge - // endpoint dangles from a nonexistent file node on Windows. - rel := filepath.Join("src", "data", "foo.go") + // endpoint dangles from a nonexistent file node on Windows. The + // spelling is written out, not composed with filepath.Join: on a + // POSIX runner Join yields exactly what the pre-fix ToSlash call + // returned, so the assertion would hold either way. + const rel = `src\data\foo.go` nodes, edges := BuildGraphArtifacts(rel, "MIT", "go") if len(nodes) != 1 || len(edges) != 1 { t.Fatalf("nodes = %d, edges = %d", len(nodes), len(edges)) diff --git a/internal/modules/scanner_test.go b/internal/modules/scanner_test.go index 28ee97f28..5aa336e4e 100644 --- a/internal/modules/scanner_test.go +++ b/internal/modules/scanner_test.go @@ -2,7 +2,6 @@ package modules import ( "fmt" - "path/filepath" "strings" "testing" @@ -134,7 +133,10 @@ func TestBuildGraphArtifacts_PreservesCallerPathSpelling(t *testing.T) { // exact relPath spelling; a re-spelled EdgeDependsOnModule From // endpoint dangles from a nonexistent node on Windows for any // manifest below the repo root. - rel := filepath.Join("services", "api", "go.mod") + // Written out, not composed with filepath.Join: on a POSIX runner + // Join yields exactly what the pre-fix ToSlash call returned, so + // the assertion would hold with or without the fix. + const rel = `services\api\go.mod` specs := []Spec{{Ecosystem: "go", Path: "github.com/foo/bar", Version: "v1.0.0", Line: 5}} nodes, edges := BuildGraphArtifacts(rel, specs) if len(nodes) != 1 || len(edges) != 1 { diff --git a/internal/todos/scanner_test.go b/internal/todos/scanner_test.go index 4610daef9..60c36fbba 100644 --- a/internal/todos/scanner_test.go +++ b/internal/todos/scanner_test.go @@ -2,7 +2,6 @@ package todos import ( "bytes" - "path/filepath" "reflect" "testing" @@ -220,7 +219,12 @@ func TestBuildGraphArtifacts_PreservesCallerPathSpelling(t *testing.T) { // replacement by the exact relPath spelling it hands the builder // (OS-native separators for subdirectory files on Windows). A // re-spelled artifact is invisible to those sweeps and goes stale. - rel := filepath.Join("src", "data", "foo.go") + // + // The spelling under test is written out rather than composed with + // filepath.Join: on a POSIX runner Join yields the forward-slash + // form, which the pre-fix ToSlash call also returned, so the + // assertion would hold with or without the fix. + const rel = `src\data\foo.go` nodes, edges := BuildGraphArtifacts(rel, []Finding{{Tag: "TODO", Text: "x", Line: 7}}, "go") if len(nodes) != 1 || len(edges) != 1 { t.Fatalf("nodes = %d, edges = %d", len(nodes), len(edges)) From d47170d8c8ba2a9dfcc770b698a3865ea2b73f90 Mon Sep 17 00:00:00 2001 From: pbednarcik <29061766+pbednarcik@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:50:48 +0200 Subject: [PATCH 5/9] test: eviction contract covers both shared-target anchoring orders The regression carries two shared license nodes - one whose first-sighting FilePath is the legacy spelling, one anchored to a surviving file - and asserts the same outcome for both. Which file a shared coverage target happens to name must not change what evicting another file removes. Both storage backends run the case. Restoring the twin sweep fails it on both backends: b's valid licensed_as edge disappears and the shared node itself is deleted. --- internal/indexer/incremental_reindex_test.go | 48 ++++++++++++-------- 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/internal/indexer/incremental_reindex_test.go b/internal/indexer/incremental_reindex_test.go index 6f8d4c8bc..01d3bd40a 100644 --- a/internal/indexer/incremental_reindex_test.go +++ b/internal/indexer/incremental_reindex_test.go @@ -220,23 +220,30 @@ func TestIncrementalReindex_FailedFileSurfacedAndRetried(t *testing.T) { // (purgeLegacyCoverageSpellings), which deletes by edge kind + FilePath // and drops a shared target only when nothing references it. func TestEvictFilesBatched_EvictsOnlyTheCallerSpelling(t *testing.T) { + // Both anchoring orders are present: a shared target whose + // first-sighting FilePath is the legacy spelling, and one anchored to + // a surviving file. Neither anchor may decide what an eviction of a + // removes — the FilePath of a shared coverage target is a breadcrumb, + // not ownership. const ( - nativeA = `src\a.go` - nativeB = `src\b.go` - legacyA = `src/a.go` - license = "license::MIT" + nativeA = `src\a.go` + nativeB = `src\b.go` + legacyA = `src/a.go` + licAnchorA = "license::MIT" + licAnchorB = "license::Apache-2.0" ) nodes := []*graph.Node{ {ID: nativeA, Kind: graph.KindFile, Name: "a.go", FilePath: nativeA}, {ID: nativeB, Kind: graph.KindFile, Name: "b.go", FilePath: nativeB}, - // Anchored to the legacy spelling — the first-sighting FilePath - // of a shared coverage target is a breadcrumb, not ownership. - {ID: license, Kind: graph.KindLicense, Name: "MIT", FilePath: legacyA}, + {ID: licAnchorA, Kind: graph.KindLicense, Name: "MIT", FilePath: legacyA}, + {ID: licAnchorB, Kind: graph.KindLicense, Name: "Apache-2.0", FilePath: nativeB}, } edges := []*graph.Edge{ - {From: nativeA, To: license, Kind: graph.EdgeLicensedAs, FilePath: nativeA}, - {From: nativeB, To: license, Kind: graph.EdgeLicensedAs, FilePath: nativeB}, - {From: legacyA, To: license, Kind: graph.EdgeLicensedAs, FilePath: legacyA}, + {From: nativeA, To: licAnchorA, Kind: graph.EdgeLicensedAs, FilePath: nativeA}, + {From: nativeB, To: licAnchorA, Kind: graph.EdgeLicensedAs, FilePath: nativeB}, + {From: legacyA, To: licAnchorA, Kind: graph.EdgeLicensedAs, FilePath: legacyA}, + {From: nativeB, To: licAnchorB, Kind: graph.EdgeLicensedAs, FilePath: nativeB}, + {From: legacyA, To: licAnchorB, Kind: graph.EdgeLicensedAs, FilePath: legacyA}, } for _, backend := range []struct { @@ -257,17 +264,20 @@ func TestEvictFilesBatched_EvictsOnlyTheCallerSpelling(t *testing.T) { evictFilesBatched(g, []string{nativeA}) - var froms []string - for _, e := range g.GetInEdges(license) { - if e != nil { - froms = append(froms, e.From) + for _, shared := range []string{licAnchorA, licAnchorB} { + var froms []string + for _, e := range g.GetInEdges(shared) { + if e != nil { + froms = append(froms, e.From) + } } + want := []string{nativeB, legacyA} + assert.ElementsMatch(t, want, froms, + "%s: only a's own spelling is evicted — b keeps its valid "+ + "edge, and the legacy row is the migration's business", shared) + assert.NotNil(t, g.GetNode(shared), + "%s: the shared license node outlives one of its files", shared) } - assert.ElementsMatch(t, []string{nativeB, legacyA}, froms, - "only a's own spelling is evicted: b keeps its valid edge, "+ - "and the legacy row is the migration's business") - assert.NotNil(t, g.GetNode(license), - "the shared license node outlives one of its files") }) } } From 616f94e65f6d4fa4b488d0413df95f3a470ac8a7 Mon Sep 17 00:00:00 2001 From: pbednarcik <29061766+pbednarcik@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:51:16 +0200 Subject: [PATCH 6/9] store_sqlite: scope the coverage purge per repository, not per store Adversarial review of the v13 migration found two ways it could delete live rows, both traced to judging a path by a store-wide rule. The first is 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). The 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. The scope is now per repository: a repository is judged only when its own paths are backslash-spelled. The second is a near miss. The Go externals lane mints live depends_on_module edges whose FilePath is a synthetic namespace, external::go: (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: that sequence marks a stub namespace, not a file. On a single-repo store, where the unprefixed arm is active, this was live third-party attribution one upgrade away from deletion. Two smaller gaps closed while here. Node-keyed sidecars are cleaned for purged nodes - ref_facts by both endpoints, plus vectors, constant values and the churn / coverage / release / blame enrichment tables - mirroring what the eviction path clears through its callers and what purgeUnprefixedRepoRows does for vectors. And the persisted analysis generation is invalidated when its tables exist, for the same reason evictByPredicateResult invalidates it: it was computed over rows the purge just removed. The tests grew with the code: a mixed-platform store whose POSIX repository must come through untouched, the synthetic-namespace cases in both prefixed and single-repo form, a non-coverage edge carrying the legacy spelling that must survive, orphan removal through the owns and generated_by arms, and a direct table test of the predicate across several repositories including a prefix that is a leading substring of another. Each was verified to fail against the corresponding mutation. --- .../store_sqlite/coverage_spelling_purge.go | 204 +++++++++---- .../coverage_spelling_purge_test.go | 285 +++++++++++++++++- 2 files changed, 425 insertions(+), 64 deletions(-) diff --git a/internal/graph/store_sqlite/coverage_spelling_purge.go b/internal/graph/store_sqlite/coverage_spelling_purge.go index 5536272fd..379d5dca1 100644 --- a/internal/graph/store_sqlite/coverage_spelling_purge.go +++ b/internal/graph/store_sqlite/coverage_spelling_purge.go @@ -2,6 +2,7 @@ package store_sqlite import ( "database/sql" + "fmt" "strings" ) @@ -18,6 +19,20 @@ const ( coverageEdgeKinds = `'annotated','licensed_as','owns','generated_by','depends_on_module'` ) +// coverageNodeSidecarTables are the node_id-keyed sidecars a removed node +// would otherwise dangle in. The eviction path clears them through its own +// callers (deleteEnrichmentByNodeIDs, the constant-value writer, the vector +// store); a migration has no such caller, so it deletes them inline — the +// same reasoning purgeUnprefixedRepoRows applies to vectors. +var coverageNodeSidecarTables = []string{ + "vectors", + "constant_values", + "churn_enrichment", + "coverage_enrichment", + "release_enrichment", + "blame_enrichment", +} + // purgeLegacyCoverageSpellings removes the coverage-domain rows that // pre-fix binaries minted under a re-spelled file path. // @@ -35,32 +50,35 @@ const ( // // Scope, deliberately narrow in three directions: // -// 1. Store-level guard. The purge runs only when the store holds at least -// one backslash-bearing file_path, i.e. it was written by a Windows -// indexer. On a store written on POSIX every path IS the native -// spelling and the whole migration is a no-op. -// 2. Path-level predicate. Below the repo prefix a Windows-written store -// spells separators with a backslash, so a forward slash there marks a -// row no current builder could have produced. Top-level files (no -// separator below the prefix) were never damaged and never match. +// 1. Per-repository guard. A repository's rows are judged only when THAT +// repository's own paths are backslash-spelled, i.e. it was indexed on +// Windows. A repository indexed on POSIX is never touched, even when it +// shares a store with a Windows-indexed one — the scope is deliberately +// per-repo rather than store-wide, because `fixture` nodes reuse the +// file node's ID (see internal/fixtures: "the fixture is the file", and +// ReclassifyFileToFixture upgrades a file node in place). Judging a +// POSIX repository by the Windows rule would therefore delete live file +// nodes and orphan every symbol they define. +// 2. Path-level predicate. Below the repo prefix a Windows-written +// repository spells separators with a backslash, so a forward slash +// there marks a row no current builder could have produced. Top-level +// files (no separator below the prefix) were never damaged and never +// match. Synthetic paths are excluded outright: a `::` in a FilePath +// marks a stub namespace (external::, module::, license::), not a file. // 3. Kind-level predicate. Only the six coverage domains' own kinds are -// considered. A shared target is removed only after the edge purge -// leaves it with no references at all — never because a purged file -// happened to be its first sighting. +// selected. A shared target is removed only after the edge purge leaves +// it with no references at all — never because a purged file happened +// to be its first sighting. // // Idempotent: a second run finds no legacy-spelled rows. Bounded to the // coverage kinds: language-extractor nodes and edges are never candidates. func purgeLegacyCoverageSpellings(tx *sql.Tx) error { - windowsWritten, err := storeHasNativeBackslashPaths(tx) - if err != nil || !windowsWritten { + scope, err := windowsWrittenScope(tx) + if err != nil || scope.empty() { return err } - prefixes, err := storeRepoPrefixes(tx) - if err != nil { - return err - } - legacyNodePath := legacyPathPredicate("nodes.file_path", prefixes) - legacyEdgePath := legacyPathPredicate("edges.file_path", prefixes) + legacyNodePath := scope.legacyPathPredicate("nodes.file_path") + legacyEdgePath := scope.legacyPathPredicate("edges.file_path") // Per-file artifacts whose own spelling is legacy. Collected before any // delete so the edge sweep below can clear their endpoints too. The @@ -134,70 +152,136 @@ func purgeLegacyCoverageSpellings(tx *sql.Tx) error { WHERE node_id IN (SELECT id FROM covdom_doomed_nodes)`); err != nil { return err } - _, err = tx.Exec(`DELETE FROM nodes WHERE id IN (SELECT id FROM covdom_doomed_nodes)`) - return err + // Reference facts are keyed by the endpoint ids, not by file. + for _, column := range []string{"from_id", "to_id"} { + if _, err := tx.Exec(`DELETE FROM ref_facts + WHERE ` + column + ` IN (SELECT id FROM covdom_doomed_nodes)`); err != nil { + return err + } + } + // Node-keyed sidecars, deleted while the nodes still exist. + for _, table := range coverageNodeSidecarTables { + if _, err := tx.Exec(`DELETE FROM ` + table + ` + WHERE node_id IN (SELECT id FROM covdom_doomed_nodes)`); err != nil { + return fmt.Errorf("delete purged coverage rows from %s: %w", table, err) + } + } + if _, err := tx.Exec(`DELETE FROM nodes WHERE id IN (SELECT id FROM covdom_doomed_nodes)`); err != nil { + return err + } + // The persisted analysis generation was computed over the rows just + // removed, so it is stale by construction. Eviction invalidates it for + // exactly this reason (see evictByPredicateResult); a migration has no + // store handle to do that through, so it clears the marker directly. + return invalidateAnalysisGenerationIfPresent(tx) } -// storeHasNativeBackslashPaths reports whether any node path carries a -// backslash, i.e. the store was written by an indexer on a platform whose -// separator is not '/'. It is the migration's outer guard: on a store -// written on POSIX no path can be a re-spelled twin of another, so the -// purge must not run at all. -func storeHasNativeBackslashPaths(tx *sql.Tx) (bool, error) { +// invalidateAnalysisGenerationIfPresent drops the active analysis generation +// when the analysis tables exist. A store upgraded from a version that +// predates them has none, so their absence is not an error. +func invalidateAnalysisGenerationIfPresent(tx *sql.Tx) error { var present int - err := tx.QueryRow(`SELECT EXISTS(SELECT 1 FROM nodes WHERE instr(file_path, '\') > 0)`).Scan(&present) - return present == 1, err + if err := tx.QueryRow(`SELECT COUNT(*) FROM sqlite_master + WHERE type = 'table' AND name = 'analysis_active_generation'`).Scan(&present); err != nil { + return err + } + if present == 0 { + return nil + } + return invalidateAnalysisGenerationTx(tx) +} + +// coverageSpellingScope names the repositories whose paths are judged. A +// repository qualifies only when its OWN nodes carry backslash-spelled +// paths; one Windows-indexed repository must never put a POSIX-indexed +// neighbour in the same store at risk. +type coverageSpellingScope struct { + // windowsPrefixes are the repo prefixes whose own paths are + // backslash-spelled. + windowsPrefixes []string + // unprefixedIsWindows reports the same for rows carrying no repo + // prefix at all (a single-repo store). + unprefixedIsWindows bool + // knownPrefixes is every repo prefix present, Windows-written or not. + // The unprefixed arm has to exclude all of them, or a POSIX + // repository's rows would be judged as if they had no prefix. + knownPrefixes []string +} + +func (s coverageSpellingScope) empty() bool { + return len(s.windowsPrefixes) == 0 && !s.unprefixedIsWindows } -// storeRepoPrefixes returns the distinct repository prefixes the store's -// nodes carry. Multi-repo IDs and paths are `/`: that single -// separator is always a forward slash regardless of platform, so it must be -// stripped before a path is judged on its remaining separators. -func storeRepoPrefixes(tx *sql.Tx) ([]string, error) { - rows, err := tx.Query(`SELECT DISTINCT repo_prefix FROM nodes WHERE repo_prefix <> ''`) +// windowsWrittenScope groups the store's nodes by repository and reports +// which ones were written by an indexer whose separator is not '/'. One +// pass: the predicate is not indexable, so grouping beats a probe per +// repository. +func windowsWrittenScope(tx *sql.Tx) (coverageSpellingScope, error) { + var scope coverageSpellingScope + rows, err := tx.Query(`SELECT repo_prefix, + MAX(CASE WHEN instr(file_path, '\') > 0 THEN 1 ELSE 0 END) + FROM nodes GROUP BY repo_prefix`) if err != nil { - return nil, err + return scope, err } defer rows.Close() //nolint:errcheck // read-only cursor - var prefixes []string for rows.Next() { var prefix string - if err := rows.Scan(&prefix); err != nil { - return nil, err + var windows int + if err := rows.Scan(&prefix, &windows); err != nil { + return scope, err + } + if prefix == "" { + scope.unprefixedIsWindows = windows == 1 + continue + } + scope.knownPrefixes = append(scope.knownPrefixes, prefix) + if windows == 1 { + scope.windowsPrefixes = append(scope.windowsPrefixes, prefix) } - prefixes = append(prefixes, prefix) } - return prefixes, rows.Err() + return scope, rows.Err() } // legacyPathPredicate builds the SQL test "this path is a pre-fix -// re-spelling": strip the `/` prefix when one applies, then look for a -// forward slash in what remains. On a Windows-written store (the only place -// this runs — see storeHasNativeBackslashPaths) the remainder's separators -// are backslashes, so a forward slash there cannot come from a current -// builder. +// re-spelling". A path qualifies when it belongs to a Windows-written +// repository AND carries a forward slash below that repository's prefix: +// there, separators are backslashes, so a forward slash cannot come from a +// current builder. Multi-repo paths are `/` and that first +// separator is always a forward slash on every platform, so it is stripped +// before the remainder is judged. +// +// Paths containing `::` are excluded on every arm. That sequence marks a +// synthetic stub namespace (`external::`, `module::`, `license::`) rather +// than a file, and such a value can carry forward slashes of its own — +// an import path, for instance — which have nothing to do with separators. // // Prefixes are embedded as escaped literals rather than bound parameters // because the predicate is spliced into CREATE TEMP TABLE ... AS SELECT // statements. The values are the store's own repo_prefix column, and // quoteSQLLiteral doubles any embedded quote. Exact string comparison, not // LIKE, so a prefix containing '%' or '_' cannot match a sibling repo. -func legacyPathPredicate(column string, prefixes []string) string { - // A single-repo store carries no prefix: the whole path is the portion - // under test. (SQL CASE requires at least one WHEN, so this branch is - // not merely an optimisation.) - if len(prefixes) == 0 { - return "instr(" + column + ", '/') > 0" - } - var b strings.Builder - b.WriteString("instr(CASE") - for _, prefix := range prefixes { +func (s coverageSpellingScope) legacyPathPredicate(column string) string { + if s.empty() { + return "0" + } + var arms []string + for _, prefix := range s.windowsPrefixes { lit := quoteSQLLiteral(prefix + "/") - b.WriteString(" WHEN substr(" + column + ", 1, length(" + lit + ")) = " + lit + - " THEN substr(" + column + ", length(" + lit + ") + 1)") + arms = append(arms, "(substr("+column+", 1, length("+lit+")) = "+lit+ + " AND instr(substr("+column+", length("+lit+") + 1), '/') > 0)") + } + if s.unprefixedIsWindows { + var b strings.Builder + b.WriteString("(instr(" + column + ", '/') > 0") + for _, prefix := range s.knownPrefixes { + lit := quoteSQLLiteral(prefix + "/") + b.WriteString(" AND substr(" + column + ", 1, length(" + lit + ")) <> " + lit) + } + b.WriteString(")") + arms = append(arms, b.String()) } - b.WriteString(" ELSE " + column + " END, '/') > 0") - return b.String() + return "(" + strings.Join(arms, " OR ") + ") AND instr(" + column + ", '::') = 0" } // quoteSQLLiteral renders s as a single-quoted SQLite string literal. diff --git a/internal/graph/store_sqlite/coverage_spelling_purge_test.go b/internal/graph/store_sqlite/coverage_spelling_purge_test.go index b05df392f..d768cfc86 100644 --- a/internal/graph/store_sqlite/coverage_spelling_purge_test.go +++ b/internal/graph/store_sqlite/coverage_spelling_purge_test.go @@ -49,6 +49,9 @@ func TestOpenPurgesLegacyCoverageSpellings(t *testing.T) { nativeMod = `r/sub\go.mod` legacyMod = `r/sub/go.mod` genExternal = `r/external::protoc` + teamSolo = `r/team::solo` + generator = `r/generator::protoc` + symbolA = nativeA + `::Alpha` ) s, err := Open(path) @@ -74,6 +77,18 @@ func TestOpenPurgesLegacyCoverageSpellings(t *testing.T) { // Shared target anchored NATIVELY with one legacy + one native // edge: node and native edge survive. {ID: moduleX, Kind: graph.KindModule, Name: "x", FilePath: nativeMod, RepoPrefix: "r"}, + // A team whose ONLY reference is a legacy owns edge: orphaned by + // the purge, so it must go. Without it the owns arm of the + // shared-target snapshot is unobservable. + {ID: teamSolo, Kind: graph.KindTeam, Name: "solo", FilePath: legacyA, RepoPrefix: "r"}, + // A materialized generator stub referenced only by a legacy + // generated_by edge: likewise orphaned. (Generator targets are + // synthetic `external::` ids; the exporter materializes them as + // artifact-shaped stubs.) + {ID: generator, Kind: graph.KindArtifact, Name: "protoc", FilePath: legacyA, RepoPrefix: "r"}, + // A symbol under the NATIVE file, used below to prove a + // non-coverage edge is never selected by kind. + {ID: symbolA, Kind: graph.KindFunction, Name: "Alpha", FilePath: nativeA, RepoPrefix: "r"}, }, []*graph.Edge{ // Native rows: every one must survive. {From: nativeA, To: nativeTodo, Kind: graph.EdgeAnnotated, FilePath: nativeA, Line: 5}, @@ -87,6 +102,12 @@ func TestOpenPurgesLegacyCoverageSpellings(t *testing.T) { {From: teamCore, To: legacyA, Kind: graph.EdgeOwns, FilePath: legacyA}, {From: legacyA, To: genExternal, Kind: graph.EdgeGeneratedBy, FilePath: legacyA}, {From: legacyMod, To: moduleX, Kind: graph.EdgeDependsOnModule, FilePath: legacyMod, Line: 2}, + {From: teamSolo, To: legacyA, Kind: graph.EdgeOwns, FilePath: legacyA}, + {From: legacyA, To: generator, Kind: graph.EdgeGeneratedBy, FilePath: legacyA}, + // A NON-coverage edge carrying the legacy spelling. Selection is + // by kind, so this must survive untouched: without it, widening + // coverageEdgeKinds to a structural kind would go unnoticed. + {From: symbolA, To: moduleX, Kind: graph.EdgeReferences, FilePath: legacyA, Line: 9}, }) // Legacy artifact nodes were FTS-indexed by the old binary; the purge // must clear their search rows so no ghost hits outlive the nodes. @@ -107,7 +128,8 @@ func TestOpenPurgesLegacyCoverageSpellings(t *testing.T) { require.Nil(t, s2.GetNode(legacyTodo), "legacy todo node must be purged") require.Nil(t, s2.GetNode(legacyFix), "legacy fixture node must be purged") require.Nil(t, s2.GetNode(licGPL), "orphaned shared license must be removed") - require.Empty(t, s2.GetOutEdges(legacyA), "no legacy-spelled coverage edge may survive") + require.Nil(t, s2.GetNode(teamSolo), "a team left with no references at all must be removed") + require.Nil(t, s2.GetNode(generator), "a generator left with no references at all must be removed") // Survivors. require.NotNil(t, s2.GetNode(nativeTodo), "native todo node must survive") require.NotNil(t, s2.GetNode(licMIT), "shared license anchored to a legacy path stays while referenced") @@ -122,14 +144,33 @@ func TestOpenPurgesLegacyCoverageSpellings(t *testing.T) { require.Len(t, outTeam, 1, "only the authored edge remains on the team") require.Equal(t, graph.EdgeAuthored, outTeam[0].Kind) - inMod := s2.GetInEdges(moduleX) - require.Len(t, inMod, 1, "exactly the native depends_on_module edge remains") - require.Equal(t, nativeMod, inMod[0].From) + var modFroms []string + for _, e := range s2.GetInEdges(moduleX) { + if e != nil && e.Kind == graph.EdgeDependsOnModule { + modFroms = append(modFroms, e.From) + } + } + require.Equal(t, []string{nativeMod}, modFroms, + "exactly the native depends_on_module edge remains on the module") outA := s2.GetOutEdges(nativeA) require.Len(t, outA, 1, "the native annotated edge survives") require.Equal(t, graph.EdgeAnnotated, outA[0].Kind) + // Selection is by kind: a structural edge is untouched even when it + // carries the legacy spelling. + outSym := s2.GetOutEdges(symbolA) + require.Len(t, outSym, 1, "a non-coverage edge is never selected, whatever its path spelling") + require.Equal(t, graph.EdgeReferences, outSym[0].Kind) + + var legacyCoverage []string + for _, e := range s2.GetOutEdges(legacyA) { + if e != nil { + legacyCoverage = append(legacyCoverage, string(e.Kind)) + } + } + require.Empty(t, legacyCoverage, "no legacy-spelled coverage edge may survive") + require.NoError(t, s2.Close()) require.Zero(t, countFTSRowidRows(t, path, legacyTodo), "purged node's FTS rows must go with it") require.NotZero(t, countFTSRowidRows(t, path, nativeTodo), "surviving node keeps its FTS rows") @@ -176,6 +217,242 @@ func TestOpenPurgesLegacyCoverageSpellingsSingleRepo(t *testing.T) { require.Len(t, s2.GetOutEdges(nativeA), 1, "native annotated edge must survive") } +// TestOpenLeavesPosixRepoUntouchedInMixedStore is the guard against the +// worst failure this migration could have: judging one repository by +// another's separator. +// +// A `fixture` node reuses the file node's ID by design (internal/fixtures: +// "the fixture is the file"; ReclassifyFileToFixture upgrades a file node +// in place). So if a POSIX-indexed repository's paths were judged by the +// Windows rule, the purge would delete a LIVE file node and orphan every +// symbol it defines. The scope is therefore per-repository: a store holding +// a Windows-indexed repo beside a POSIX-indexed one heals the first and +// must not touch a single row of the second. +func TestOpenLeavesPosixRepoUntouchedInMixedStore(t *testing.T) { + path := filepath.Join(t.TempDir(), "store.sqlite") + + const ( + winFile = `win/src\a.go` + winLegacy = `win/src/a.go` + winTodo = winLegacy + `::todo:3` + nixFixture = `nix/testdata/golden.json` // the fixture IS the file node + nixSymbol = `nix/testdata/golden.json::Root` + nixFile = `nix/src/b.go` + nixTodo = nixFile + `::todo:7` + nixLicense = `nix/license::MIT` + ) + + s, err := Open(path) + require.NoError(t, err) + s.AddBatch([]*graph.Node{ + {ID: winFile, Kind: graph.KindFile, Name: "a.go", FilePath: winFile, RepoPrefix: "win"}, + {ID: winTodo, Kind: graph.KindTodo, Name: "todo:3", FilePath: winLegacy, RepoPrefix: "win"}, + {ID: nixFixture, Kind: graph.KindFixture, Name: "golden.json", FilePath: nixFixture, RepoPrefix: "nix"}, + {ID: nixSymbol, Kind: graph.KindVariable, Name: "Root", FilePath: nixFixture, RepoPrefix: "nix"}, + {ID: nixFile, Kind: graph.KindFile, Name: "b.go", FilePath: nixFile, RepoPrefix: "nix"}, + {ID: nixTodo, Kind: graph.KindTodo, Name: "todo:7", FilePath: nixFile, RepoPrefix: "nix"}, + {ID: nixLicense, Kind: graph.KindLicense, Name: "MIT", FilePath: nixFile, RepoPrefix: "nix"}, + }, []*graph.Edge{ + {From: winLegacy, To: winTodo, Kind: graph.EdgeAnnotated, FilePath: winLegacy, Line: 3}, + {From: nixFixture, To: nixSymbol, Kind: graph.EdgeDefines, FilePath: nixFixture, Line: 1}, + {From: nixFile, To: nixTodo, Kind: graph.EdgeAnnotated, FilePath: nixFile, Line: 7}, + {From: nixFile, To: nixLicense, Kind: graph.EdgeLicensedAs, FilePath: nixFile}, + }) + require.NoError(t, s.Close()) + + withRawDB(t, path, func(db *sql.DB) { + _, err := db.Exec(`PRAGMA user_version = 12`) + require.NoError(t, err, "reset to the pre-purge version") + }) + + s2, err := Open(path) + require.NoError(t, err) + t.Cleanup(func() { _ = s2.Close() }) + + require.Nil(t, s2.GetNode(winTodo), "the Windows repo's legacy row still heals") + + require.NotNil(t, s2.GetNode(nixFixture), + "a POSIX repo's fixture node IS its file node and must survive") + require.Len(t, s2.GetOutEdges(nixFixture), 1, + "the POSIX file's defines edge must survive, or its symbols are orphaned") + require.NotNil(t, s2.GetNode(nixSymbol), "the defined symbol must keep its parent") + require.NotNil(t, s2.GetNode(nixTodo), "a POSIX repo's todo is correctly spelled, not legacy") + require.NotNil(t, s2.GetNode(nixLicense), "a POSIX repo's license must survive") + require.Len(t, s2.GetOutEdges(nixFile), 2, "both POSIX coverage edges must survive") +} + +// TestOpenLeavesSyntheticPathsUntouched pins the synthetic-namespace +// exclusion. A stub node's FilePath is not a file: `external::go:x/y` and +// `module::go:x/y@v1` carry forward slashes that are part of an import +// path, not separators, and a depends_on_module edge can be minted against +// them (see modules.LinkImports). Judging those by the separator rule would +// delete live third-party attribution. +func TestOpenLeavesSyntheticPathsUntouched(t *testing.T) { + path := filepath.Join(t.TempDir(), "store.sqlite") + + const ( + nativeA = `r/src\a.go` + extPath = `r/external::go:github.com/pkg/errors` + extSym = `r/external::go:github.com/pkg/errors::New` + modNode = `r/module::go:github.com/pkg/errors@v0.9.1` + ) + + s, err := Open(path) + require.NoError(t, err) + s.AddBatch([]*graph.Node{ + {ID: nativeA, Kind: graph.KindFile, Name: "a.go", FilePath: nativeA, RepoPrefix: "r"}, + {ID: extSym, Kind: graph.KindFunction, Name: "New", FilePath: extPath, RepoPrefix: "r"}, + {ID: modNode, Kind: graph.KindModule, Name: "errors", FilePath: extPath, RepoPrefix: "r"}, + }, []*graph.Edge{ + {From: extSym, To: modNode, Kind: graph.EdgeDependsOnModule, FilePath: extPath}, + {From: nativeA, To: extSym, Kind: graph.EdgeCalls, FilePath: nativeA, Line: 3}, + }) + require.NoError(t, s.Close()) + + withRawDB(t, path, func(db *sql.DB) { + _, err := db.Exec(`PRAGMA user_version = 12`) + require.NoError(t, err, "reset to the pre-purge version") + }) + + s2, err := Open(path) + require.NoError(t, err) + t.Cleanup(func() { _ = s2.Close() }) + + require.NotNil(t, s2.GetNode(modNode), "a module stub is not a file and must survive") + require.Len(t, s2.GetOutEdges(extSym), 1, + "the module attribution edge must survive: its FilePath is a stub namespace, not a path") +} + +// TestOpenLeavesUnprefixedSyntheticPathsUntouched is the single-repo form +// of the synthetic-path exclusion, and the one that matters in production: +// the Go externals lane mints live depends_on_module edges whose FilePath +// is `external::go:` (internal/semantic/goanalysis: +// externalFilePath). In a single-repo store the unprefixed arm of the +// predicate is active, so without the `::` exclusion that import path's own +// forward slashes would read as separators and the purge would delete live +// third-party attribution. +func TestOpenLeavesUnprefixedSyntheticPathsUntouched(t *testing.T) { + path := filepath.Join(t.TempDir(), "store.sqlite") + + const ( + nativeA = `src\a.go` + extPath = `external::go:github.com/pkg/errors` + bareExt = `external::go` + extSym = `external::go:github.com/pkg/errors::New` + modErr = `module::go:github.com/pkg/errors@v0.9.1` + modStd = `module::go:std` + stdSym = `external::go::Println` + legacyTd = `src/a.go::todo:3` + ) + + s, err := Open(path) + require.NoError(t, err) + s.AddBatch([]*graph.Node{ + {ID: nativeA, Kind: graph.KindFile, Name: "a.go", FilePath: nativeA}, + {ID: legacyTd, Kind: graph.KindTodo, Name: "todo:3", FilePath: `src/a.go`}, + {ID: extSym, Kind: graph.KindFunction, Name: "New", FilePath: extPath}, + {ID: modErr, Kind: graph.KindModule, Name: "errors", FilePath: extPath}, + {ID: stdSym, Kind: graph.KindFunction, Name: "Println", FilePath: bareExt}, + {ID: modStd, Kind: graph.KindModule, Name: "stdlib", FilePath: bareExt}, + }, []*graph.Edge{ + {From: `src/a.go`, To: legacyTd, Kind: graph.EdgeAnnotated, FilePath: `src/a.go`, Line: 3}, + {From: extSym, To: modErr, Kind: graph.EdgeDependsOnModule, FilePath: extPath}, + {From: stdSym, To: modStd, Kind: graph.EdgeDependsOnModule, FilePath: bareExt}, + }) + require.NoError(t, s.Close()) + + withRawDB(t, path, func(db *sql.DB) { + _, err := db.Exec(`PRAGMA user_version = 12`) + require.NoError(t, err, "reset to the pre-purge version") + }) + + s2, err := Open(path) + require.NoError(t, err) + t.Cleanup(func() { _ = s2.Close() }) + + require.Nil(t, s2.GetNode(legacyTd), "the store's real legacy row still heals") + + require.NotNil(t, s2.GetNode(modErr), "a module stub is not a file and must survive") + require.NotNil(t, s2.GetNode(modStd), "the stdlib module stub must survive") + require.Len(t, s2.GetOutEdges(extSym), 1, + "module attribution survives: the import path's slashes are not separators") + require.Len(t, s2.GetOutEdges(stdSym), 1, "the bare external namespace survives too") +} + +// TestLegacyPathPredicateAcrossRepoScopes exercises the predicate directly, +// with explicit spellings and no store. This is the platform-parametric +// helper the review asked for: it takes both spellings as literals, so it +// exercises the Windows split identically on every runner, and it is the +// only place the multi-arm shape (several Windows repos, a POSIX repo +// beside them, a prefix that is a prefix of another) is covered. +func TestLegacyPathPredicateAcrossRepoScopes(t *testing.T) { + scope := coverageSpellingScope{ + windowsPrefixes: []string{"win", "gortex"}, + knownPrefixes: []string{"win", "gortex", "gortexish", "nix"}, + } + + cases := []struct { + path string + want bool + why string + }{ + {`win/src/a.go`, true, "legacy spelling inside a Windows repo"}, + {`win/src\a.go`, false, "native spelling inside a Windows repo"}, + {`win/main.go`, false, "top-level file: no separator below the prefix"}, + {`gortex/internal/foo.go`, true, "second Windows repo is judged too"}, + {`gortexish/internal/foo.go`, false, "a repo whose prefix merely shares a leading string is not"}, + {`nix/src/b.go`, false, "a POSIX repo is never judged"}, + {`nix/testdata/golden.json`, false, "including its fixture, which IS its file node"}, + {`win/external::go:github.com/pkg/errors`, false, "synthetic namespace, not a path"}, + {`unknown/src/c.go`, false, "no matching Windows prefix and the unprefixed arm is off"}, + } + + path := filepath.Join(t.TempDir(), "store.sqlite") + s, err := Open(path) + require.NoError(t, err) + require.NoError(t, s.Close()) + withRawDB(t, path, func(db *sql.DB) { + pred := scope.legacyPathPredicate("file_path") + for _, tc := range cases { + var got bool + require.NoError(t, + db.QueryRow(`WITH p(file_path) AS (VALUES (?)) SELECT (`+pred+`) FROM p`, tc.path).Scan(&got)) + require.Equal(t, tc.want, got, "%s: %s", tc.path, tc.why) + } + }) + + t.Run("single repo store judges the whole path", func(t *testing.T) { + solo := coverageSpellingScope{unprefixedIsWindows: true} + soloPath := filepath.Join(t.TempDir(), "store.sqlite") + s, err := Open(soloPath) + require.NoError(t, err) + require.NoError(t, s.Close()) + withRawDB(t, soloPath, func(db *sql.DB) { + pred := solo.legacyPathPredicate("file_path") + for _, tc := range []struct { + path string + want bool + }{ + {`src/a.go`, true}, + {`src\a.go`, false}, + {`main.go`, false}, + {`external::go:github.com/pkg/errors`, false}, + } { + var got bool + require.NoError(t, + db.QueryRow(`WITH p(file_path) AS (VALUES (?)) SELECT (`+pred+`) FROM p`, tc.path).Scan(&got)) + require.Equal(t, tc.want, got, tc.path) + } + }) + }) + + t.Run("no windows repo disables the purge entirely", func(t *testing.T) { + none := coverageSpellingScope{knownPrefixes: []string{"nix"}} + require.True(t, none.empty()) + require.Equal(t, "0", none.legacyPathPredicate("file_path")) + }) +} + // TestPurgeLegacyCoverageSpellingsIsIdempotent runs the step twice on // one connection: the second pass must find nothing left to remove and // must not trip over the temp tables the first pass created. From b2a45504ee6e31af426cb29dec7d2e67796fd209 Mon Sep 17 00:00:00 2001 From: pbednarcik <29061766+pbednarcik@users.noreply.github.com> Date: Fri, 28 Aug 2026 15:13:02 +0200 Subject: [PATCH 7/9] store_sqlite: bound the coverage purge against a wrong repository verdict A second review pass found the per-repository verdict is sticky. It is drawn from rows eviction never removes - that is this migration's whole premise - so a repository once indexed on Windows keeps that verdict even after being re-indexed on POSIX inside the same store, which is reachable through a synced or container-mounted home directory. Since a fixture node reuses the file node's ID, the purge would then delete a live file and orphan every symbol in it. Rather than chase the classification, the node selection now excludes any node that defines symbols. A legacy artifact node defines nothing; a file node with symbols in it always does. The damage from a wrong verdict is bounded to zero at a cost of one NOT EXISTS. Two path-shape fixes alongside it. The scope query reads only file nodes: they are the only rows that evidence the indexer's separator, and the restriction lets it ride the covering nodes_repo_files index instead of fetching every row of a table whose payload carries the meta blob (measured 4.9x cheaper). And a repository prefix may itself contain a separator, so each arm now excludes the paths of any repository nested beneath it, which would otherwise have their prefix separator read as a legacy spelling. Finally, the node-keyed cleanup is skipped outright when nothing is doomed. It runs before healPlannerStats, so on a store with no sqlite_stat1 the ref_facts deletes plan as full scans of a table holding one row per resolved reference edge - work no store with nothing to heal should pay. clone_shingles joins the sidecar list, and a test now derives the required set from the live schema so the list cannot drift. --- .../store_sqlite/coverage_spelling_purge.go | 91 ++++++++++++--- .../coverage_spelling_purge_test.go | 109 +++++++++++++++++- 2 files changed, 181 insertions(+), 19 deletions(-) diff --git a/internal/graph/store_sqlite/coverage_spelling_purge.go b/internal/graph/store_sqlite/coverage_spelling_purge.go index 379d5dca1..8e193495b 100644 --- a/internal/graph/store_sqlite/coverage_spelling_purge.go +++ b/internal/graph/store_sqlite/coverage_spelling_purge.go @@ -24,8 +24,12 @@ const ( // callers (deleteEnrichmentByNodeIDs, the constant-value writer, the vector // store); a migration has no such caller, so it deletes them inline — the // same reasoning purgeUnprefixedRepoRows applies to vectors. +// It must list every node_id-keyed table in schemaSQL; +// TestCoverageNodeSidecarTablesCoverSchema fails when a new one appears, +// so the list cannot drift the way a hand-maintained list usually does. var coverageNodeSidecarTables = []string{ "vectors", + "clone_shingles", "constant_values", "churn_enrichment", "coverage_enrichment", @@ -81,22 +85,37 @@ func purgeLegacyCoverageSpellings(tx *sql.Tx) error { legacyEdgePath := scope.legacyPathPredicate("edges.file_path") // Per-file artifacts whose own spelling is legacy. Collected before any - // delete so the edge sweep below can clear their endpoints too. The - // DROPs make the step re-entrant on a connection that carried a temp - // table over from an earlier attempt. - if _, err := tx.Exec(`DROP TABLE IF EXISTS covdom_doomed_nodes`); err != nil { + // delete so the edge sweep below can clear their endpoints too. + // + // The `defines` exclusion is the last line of defence against deleting + // a live file. A `fixture` node reuses the file node's ID, and a + // repository's Windows verdict is drawn from rows that eviction never + // removes, so a repository re-indexed on POSIX inside a store that + // still holds its old Windows rows would keep being judged by the + // Windows rule. A legacy artifact node defines nothing; a file node + // with symbols in it always does. Cheap, and it fails safe in exactly + // the case where the classification is wrong. + // + // The DROPs make the step re-entrant on a connection that carried a + // temp table over from an earlier attempt; they name `temp.` so they + // can never reach a same-named table in the main schema. Their + // deferred errors are dropped deliberately: nothing else consults + // these names, and the leading DROP makes a survivor harmless. + if _, err := tx.Exec(`DROP TABLE IF EXISTS temp.covdom_doomed_nodes`); err != nil { return err } if _, err := tx.Exec(`CREATE TEMP TABLE covdom_doomed_nodes AS SELECT id FROM nodes - WHERE kind IN (` + coveragePerFileNodeKinds + `) AND ` + legacyNodePath); err != nil { + WHERE kind IN (` + coveragePerFileNodeKinds + `) AND ` + legacyNodePath + ` + AND NOT EXISTS (SELECT 1 FROM edges e + WHERE e.from_id = nodes.id AND e.kind = 'defines')`); err != nil { return err } - defer func() { _, _ = tx.Exec(`DROP TABLE IF EXISTS covdom_doomed_nodes`) }() + defer func() { _, _ = tx.Exec(`DROP TABLE IF EXISTS temp.covdom_doomed_nodes`) }() // Shared targets the legacy edges point at. Snapshotted BEFORE the edge // delete, because afterwards nothing links them to this purge. - if _, err := tx.Exec(`DROP TABLE IF EXISTS covdom_shared_targets`); err != nil { + if _, err := tx.Exec(`DROP TABLE IF EXISTS temp.covdom_shared_targets`); err != nil { return err } if _, err := tx.Exec(`CREATE TEMP TABLE covdom_shared_targets AS @@ -107,13 +126,18 @@ func purgeLegacyCoverageSpellings(tx *sql.Tx) error { WHERE kind = 'owns' AND ` + legacyEdgePath); err != nil { return err } - defer func() { _, _ = tx.Exec(`DROP TABLE IF EXISTS covdom_shared_targets`) }() + defer func() { _, _ = tx.Exec(`DROP TABLE IF EXISTS temp.covdom_shared_targets`) }() // Legacy coverage edges, selected by kind AND their own FilePath // spelling — never by touching an evicted endpoint, which would take // a shared target's other, still-valid edges with it. - if _, err := tx.Exec(`DELETE FROM edges - WHERE kind IN (` + coverageEdgeKinds + `) AND ` + legacyEdgePath); err != nil { + result, err := tx.Exec(`DELETE FROM edges + WHERE kind IN (` + coverageEdgeKinds + `) AND ` + legacyEdgePath) + if err != nil { + return err + } + removedEdges, err := result.RowsAffected() + if err != nil { return err } // Any remaining edge on a doomed per-file artifact: its node is going, @@ -140,6 +164,24 @@ func purgeLegacyCoverageSpellings(tx *sql.Tx) error { return err } + // Everything below is keyed by the doomed node ids, so an empty set + // makes all of it a no-op. Skipping it outright matters because this + // step runs before healPlannerStats: on a store with no sqlite_stat1 + // the ref_facts deletes plan as full scans of a table that holds one + // row per resolved reference edge. + var doomed int + if err := tx.QueryRow(`SELECT COUNT(*) FROM covdom_doomed_nodes`).Scan(&doomed); err != nil { + return err + } + if doomed == 0 { + if removedEdges == 0 { + // The store held nothing legacy: leave the persisted analysis + // generation alone rather than forcing a needless recompute. + return nil + } + return invalidateAnalysisGenerationIfPresent(tx) + } + // Symbol FTS rows outlive their node unless deleted explicitly (see // BatchDeleteSymbolFTS, the eviction lane's equivalent) — a purged todo // would otherwise keep answering searches with its stale text. @@ -212,15 +254,18 @@ func (s coverageSpellingScope) empty() bool { return len(s.windowsPrefixes) == 0 && !s.unprefixedIsWindows } -// windowsWrittenScope groups the store's nodes by repository and reports -// which ones were written by an indexer whose separator is not '/'. One -// pass: the predicate is not indexable, so grouping beats a probe per -// repository. +// windowsWrittenScope groups the store's FILE nodes by repository and +// reports which ones were written by an indexer whose separator is not +// '/'. One pass rather than a probe per repository, and restricted to +// kind='file' for two reasons: only file-backed nodes evidence the +// indexer's separator at all, and the restriction lets the scan ride the +// covering nodes_repo_files index instead of fetching every row in a +// table whose payload includes the meta blob. func windowsWrittenScope(tx *sql.Tx) (coverageSpellingScope, error) { var scope coverageSpellingScope rows, err := tx.Query(`SELECT repo_prefix, MAX(CASE WHEN instr(file_path, '\') > 0 THEN 1 ELSE 0 END) - FROM nodes GROUP BY repo_prefix`) + FROM nodes WHERE kind = 'file' GROUP BY repo_prefix`) if err != nil { return scope, err } @@ -268,8 +313,20 @@ func (s coverageSpellingScope) legacyPathPredicate(column string) string { var arms []string for _, prefix := range s.windowsPrefixes { lit := quoteSQLLiteral(prefix + "/") - arms = append(arms, "(substr("+column+", 1, length("+lit+")) = "+lit+ - " AND instr(substr("+column+", length("+lit+") + 1), '/') > 0)") + arm := "substr(" + column + ", 1, length(" + lit + ")) = " + lit + // A repo prefix may itself contain a separator, so one prefix can + // be a leading substring of another ("foo" and "foo/bar"). Without + // this exclusion the shorter repo's arm would judge the longer + // repo's paths and read its prefix separator as a legacy spelling. + for _, nested := range s.knownPrefixes { + if nested == prefix || !strings.HasPrefix(nested, prefix+"/") { + continue + } + nestedLit := quoteSQLLiteral(nested + "/") + arm += " AND substr(" + column + ", 1, length(" + nestedLit + ")) <> " + nestedLit + } + arm += " AND instr(substr(" + column + ", length(" + lit + ") + 1), '/') > 0" + arms = append(arms, "("+arm+")") } if s.unprefixedIsWindows { var b strings.Builder diff --git a/internal/graph/store_sqlite/coverage_spelling_purge_test.go b/internal/graph/store_sqlite/coverage_spelling_purge_test.go index d768cfc86..080b7b0fe 100644 --- a/internal/graph/store_sqlite/coverage_spelling_purge_test.go +++ b/internal/graph/store_sqlite/coverage_spelling_purge_test.go @@ -387,8 +387,8 @@ func TestOpenLeavesUnprefixedSyntheticPathsUntouched(t *testing.T) { // beside them, a prefix that is a prefix of another) is covered. func TestLegacyPathPredicateAcrossRepoScopes(t *testing.T) { scope := coverageSpellingScope{ - windowsPrefixes: []string{"win", "gortex"}, - knownPrefixes: []string{"win", "gortex", "gortexish", "nix"}, + windowsPrefixes: []string{"win", "gortex", "nest"}, + knownPrefixes: []string{"win", "gortex", "gortexish", "nix", "nest", "nest/inner"}, } cases := []struct { @@ -405,6 +405,9 @@ func TestLegacyPathPredicateAcrossRepoScopes(t *testing.T) { {`nix/testdata/golden.json`, false, "including its fixture, which IS its file node"}, {`win/external::go:github.com/pkg/errors`, false, "synthetic namespace, not a path"}, {`unknown/src/c.go`, false, "no matching Windows prefix and the unprefixed arm is off"}, + {`nest/inner/pkg\a.go`, false, + "a nested repo's native path is not judged by its parent repo's arm"}, + {`nest/pkg/a.go`, true, "the parent repo itself is still judged normally"}, } path := filepath.Join(t.TempDir(), "store.sqlite") @@ -453,6 +456,108 @@ func TestLegacyPathPredicateAcrossRepoScopes(t *testing.T) { }) } +// TestCoverageNodeSidecarTablesCoverSchema keeps the sidecar list from +// drifting: any table whose primary key is node_id must be cleaned when a +// node is purged, so a newly added one has to appear in the list. Derived +// from the live schema rather than from a second hand-written list. +func TestCoverageNodeSidecarTablesCoverSchema(t *testing.T) { + path := filepath.Join(t.TempDir(), "store.sqlite") + s, err := Open(path) + require.NoError(t, err) + require.NoError(t, s.Close()) + + listed := make(map[string]bool, len(coverageNodeSidecarTables)) + for _, table := range coverageNodeSidecarTables { + listed[table] = true + } + // Cleaned by name elsewhere in the purge rather than through the list. + handled := map[string]bool{"symbol_fts_rowid": true, "nodes": true} + + withRawDB(t, path, func(db *sql.DB) { + rows, err := db.Query(`SELECT name FROM sqlite_master WHERE type = 'table'`) + require.NoError(t, err) + defer rows.Close() //nolint:errcheck // read-only cursor + var tables []string + for rows.Next() { + var name string + require.NoError(t, rows.Scan(&name)) + tables = append(tables, name) + } + require.NoError(t, rows.Err()) + + for _, table := range tables { + if listed[table] || handled[table] { + continue + } + info, err := db.Query(`SELECT name, pk FROM pragma_table_info(?)`, table) + require.NoError(t, err) + keyedByNodeID := false + for info.Next() { + var name string + var pk int + require.NoError(t, info.Scan(&name, &pk)) + if name == "node_id" && pk > 0 { + keyedByNodeID = true + } + } + require.NoError(t, info.Err()) + require.NoError(t, info.Close()) + require.False(t, keyedByNodeID, + "table %q is keyed by node_id but is not in coverageNodeSidecarTables: "+ + "a purged node would leave a dangling row there", table) + } + }) +} + +// TestOpenNeverPurgesANodeThatDefinesSymbols is the fail-safe for a wrong +// repository verdict. A repository's Windows classification is drawn from +// rows eviction never removes, so a repository re-indexed on POSIX inside +// a store that still holds its old Windows rows keeps being judged by the +// Windows rule. Since a `fixture` node reuses the file node's ID, that +// would delete a live file and orphan its symbols. A legacy artifact node +// defines nothing, so excluding nodes that do costs nothing and bounds the +// damage of a misclassification to zero. +func TestOpenNeverPurgesANodeThatDefinesSymbols(t *testing.T) { + path := filepath.Join(t.TempDir(), "store.sqlite") + + const ( + staleWin = `r/src\old.go` // leftover Windows row: sets the verdict + liveFix = `r/testdata/golden.json` + liveSym = `r/testdata/golden.json::Root` + liveTodo = `r/src/b.go::todo:2` + liveFileB = `r/src/b.go` + ) + + s, err := Open(path) + require.NoError(t, err) + s.AddBatch([]*graph.Node{ + {ID: staleWin, Kind: graph.KindFile, Name: "old.go", FilePath: staleWin, RepoPrefix: "r"}, + // Re-indexed on POSIX: this fixture node IS the file node. + {ID: liveFix, Kind: graph.KindFixture, Name: "golden.json", FilePath: liveFix, RepoPrefix: "r"}, + {ID: liveSym, Kind: graph.KindVariable, Name: "Root", FilePath: liveFix, RepoPrefix: "r"}, + {ID: liveFileB, Kind: graph.KindFile, Name: "b.go", FilePath: liveFileB, RepoPrefix: "r"}, + {ID: liveTodo, Kind: graph.KindTodo, Name: "todo:2", FilePath: liveFileB, RepoPrefix: "r"}, + }, []*graph.Edge{ + {From: liveFix, To: liveSym, Kind: graph.EdgeDefines, FilePath: liveFix, Line: 1}, + {From: liveFileB, To: liveTodo, Kind: graph.EdgeAnnotated, FilePath: liveFileB, Line: 2}, + }) + require.NoError(t, s.Close()) + + withRawDB(t, path, func(db *sql.DB) { + _, err := db.Exec(`PRAGMA user_version = 12`) + require.NoError(t, err, "reset to the pre-purge version") + }) + + s2, err := Open(path) + require.NoError(t, err) + t.Cleanup(func() { _ = s2.Close() }) + + require.NotNil(t, s2.GetNode(liveFix), + "a node that defines symbols is never purged, whatever the path verdict") + require.Len(t, s2.GetOutEdges(liveFix), 1, "its defines edge must survive with it") + require.NotNil(t, s2.GetNode(liveSym), "the defined symbol keeps its parent") +} + // TestPurgeLegacyCoverageSpellingsIsIdempotent runs the step twice on // one connection: the second pass must find nothing left to remove and // must not trip over the temp tables the first pass created. From c8f210b0f70e1a8224e1cc8b59ed8d62cd9a2a73 Mon Sep 17 00:00:00 2001 From: pbednarcik <29061766+pbednarcik@users.noreply.github.com> Date: Fri, 28 Aug 2026 15:28:37 +0200 Subject: [PATCH 8/9] store_sqlite: purge only rows whose natively spelled twin is present The defines fail-safe from the previous commit turned out to guard almost nothing. Writing the review brief for an independent pass made the hole obvious: a defines edge only exists if the file has parsed symbols, and the fixture files this is meant to protect are data, not code. Measured on a real store: only 34 of 1,126 legacy artifact rows carry a defines edge, and 1 of 35 live fixture nodes has none - exactly the case the guard was written for. The requirement is now the one that actually separates the two populations. A legacy row is a RE-spelling of a file the indexer also recorded natively, so the natively spelled twin is in the store; a file merely indexed on POSIX has no twin, because its own spelling IS the native one. Requiring the twin removes exactly the duplicates and can never remove a live row, fixture aliasing included, whatever verdict the repository scope reached. It costs no healing: all 1,126 legacy-spelled artifact rows on that store have their twin, and the delete set is unchanged at 1,088 nodes and 1,088 edges. What it gives up is a legacy row whose file was deleted before the upgrade, whose twin went with it. That row is unreachable residue either way, and trading it for the guarantee that no live file is ever removed is the right side of the bargain. The path arms stay in legacyPathPredicate so they remain independently testable; legacyRowPredicate is what the migration selects on. --- .../store_sqlite/coverage_spelling_purge.go | 52 +++++++++++++- .../coverage_spelling_purge_test.go | 67 +++++++++++++++++++ 2 files changed, 117 insertions(+), 2 deletions(-) diff --git a/internal/graph/store_sqlite/coverage_spelling_purge.go b/internal/graph/store_sqlite/coverage_spelling_purge.go index 8e193495b..989b2cb5a 100644 --- a/internal/graph/store_sqlite/coverage_spelling_purge.go +++ b/internal/graph/store_sqlite/coverage_spelling_purge.go @@ -81,8 +81,8 @@ func purgeLegacyCoverageSpellings(tx *sql.Tx) error { if err != nil || scope.empty() { return err } - legacyNodePath := scope.legacyPathPredicate("nodes.file_path") - legacyEdgePath := scope.legacyPathPredicate("edges.file_path") + legacyNodePath := scope.legacyRowPredicate("nodes.file_path") + legacyEdgePath := scope.legacyRowPredicate("edges.file_path") // Per-file artifacts whose own spelling is legacy. Collected before any // delete so the edge sweep below can clear their endpoints too. @@ -341,6 +341,54 @@ func (s coverageSpellingScope) legacyPathPredicate(column string) string { return "(" + strings.Join(arms, " OR ") + ") AND instr(" + column + ", '::') = 0" } +// legacyRowPredicate is what the migration actually selects on: a legacy +// SPELLING whose natively spelled twin is present in the store. Kept +// separate from legacyPathPredicate so the path arms stay independently +// testable. +func (s coverageSpellingScope) legacyRowPredicate(column string) string { + if s.empty() { + return "0" + } + return s.legacyPathPredicate(column) + + " AND EXISTS (SELECT 1 FROM nodes twin WHERE twin.file_path = " + + s.nativeTwinExpr(column) + ")" +} + +// nativeTwinExpr renders the path a row WOULD have carried had the builder +// preserved the indexer's spelling: the repo prefix kept verbatim, every +// forward slash below it turned back into a separator. +// +// Requiring that twin to exist is what makes the purge safe rather than +// merely narrow. A legacy row is by definition a RE-spelling of a file the +// indexer also recorded natively, so its twin is present; a row that is +// simply a POSIX-indexed file has no twin, because its own spelling is the +// native one. So a repository misjudged as Windows-written - the sticky +// verdict a store carried across platforms can produce - loses nothing, +// including the fixture nodes that reuse a file node's ID. +// +// Measured against a real Windows store before adopting it: all 1,126 +// legacy-spelled artifact rows had their twin, so the stricter rule costs +// no healing. What it does give up is a row whose file has since been +// deleted, whose twin went with it. That row is unreachable residue either +// way, and trading it for the guarantee that no live file is ever removed +// is the right side of that bargain. +func (s coverageSpellingScope) nativeTwinExpr(column string) string { + // A single-repo store carries no prefix to preserve, and SQL CASE + // requires at least one WHEN, so this branch is not just a shortcut. + if len(s.windowsPrefixes) == 0 { + return "replace(" + column + ", '/', '\\')" + } + var b strings.Builder + b.WriteString("CASE") + for _, prefix := range s.windowsPrefixes { + lit := quoteSQLLiteral(prefix + "/") + b.WriteString(" WHEN substr(" + column + ", 1, length(" + lit + ")) = " + lit + + " THEN " + lit + " || replace(substr(" + column + ", length(" + lit + ") + 1), '/', '\\')") + } + b.WriteString(" ELSE replace(" + column + ", '/', '\\') END") + return b.String() +} + // quoteSQLLiteral renders s as a single-quoted SQLite string literal. func quoteSQLLiteral(s string) string { return "'" + strings.ReplaceAll(s, "'", "''") + "'" diff --git a/internal/graph/store_sqlite/coverage_spelling_purge_test.go b/internal/graph/store_sqlite/coverage_spelling_purge_test.go index 080b7b0fe..d24feb20e 100644 --- a/internal/graph/store_sqlite/coverage_spelling_purge_test.go +++ b/internal/graph/store_sqlite/coverage_spelling_purge_test.go @@ -42,6 +42,7 @@ func TestOpenPurgesLegacyCoverageSpellings(t *testing.T) { nativeTodo = nativeA + `::todo:5` legacyTodo = legacyA + `::todo:3` legacyFix = `r/testdata/x.json` + nativeFix = `r/testdata\x.json` licMIT = `r/license::MIT` licGPL = `r/license::GPL-3.0` teamCore = `r/team::core` @@ -58,8 +59,13 @@ func TestOpenPurgesLegacyCoverageSpellings(t *testing.T) { require.NoError(t, err) s.AddBatch([]*graph.Node{ // Native file nodes: prove the store keys paths with backslashes. + // Every legacy row below is a RE-spelling of one of these, which is + // what a real pre-fix store looks like and what the purge requires + // before it removes anything. {ID: nativeA, Kind: graph.KindFile, Name: "a.go", FilePath: nativeA, RepoPrefix: "r"}, {ID: nativeB, Kind: graph.KindFile, Name: "b.go", FilePath: nativeB, RepoPrefix: "r"}, + {ID: nativeFix, Kind: graph.KindFile, Name: "x.json", FilePath: nativeFix, RepoPrefix: "r"}, + {ID: nativeMod, Kind: graph.KindFile, Name: "go.mod", FilePath: nativeMod, RepoPrefix: "r"}, // Native todo: must survive. {ID: nativeTodo, Kind: graph.KindTodo, Name: "todo:5", FilePath: nativeA, RepoPrefix: "r"}, // Legacy per-file artifacts: must be purged. @@ -456,6 +462,67 @@ func TestLegacyPathPredicateAcrossRepoScopes(t *testing.T) { }) } +// TestOpenPurgesOnlyRowsWhoseNativeTwinExists is the strongest of the +// safety properties, and the one that makes a wrong repository verdict +// harmless rather than merely unlikely. +// +// A legacy row is by definition a RE-spelling of a file the indexer also +// recorded natively, so the natively spelled twin is in the store. A file +// that was simply indexed on POSIX has no twin, because its own spelling +// IS the native one. Requiring the twin therefore removes exactly the +// duplicates and never a live row - including a fixture node, which +// reuses its file node's ID and so cannot be told apart any other way. +// +// The store here is judged Windows-written (the stale `r/old\thing.go` +// row sets the verdict) and every path below is slash-spelled, so only +// the twin requirement separates them. +func TestOpenPurgesOnlyRowsWhoseNativeTwinExists(t *testing.T) { + path := filepath.Join(t.TempDir(), "store.sqlite") + + const ( + staleWin = `r/old\thing.go` + + twinned = `r/src\a.go` + twinnedSlur = `r/src/a.go` + twinnedTodo = `r/src/a.go::todo:4` + + lonelyFix = `r/testdata/golden.json` // POSIX-indexed, no twin + lonelyTodo = `r/pkg/only.go::todo:9` // its file was deleted + ) + + s, err := Open(path) + require.NoError(t, err) + s.AddBatch([]*graph.Node{ + {ID: staleWin, Kind: graph.KindFile, Name: "thing.go", FilePath: staleWin, RepoPrefix: "r"}, + {ID: twinned, Kind: graph.KindFile, Name: "a.go", FilePath: twinned, RepoPrefix: "r"}, + {ID: twinnedTodo, Kind: graph.KindTodo, Name: "todo:4", FilePath: twinnedSlur, RepoPrefix: "r"}, + {ID: lonelyFix, Kind: graph.KindFixture, Name: "golden.json", FilePath: lonelyFix, RepoPrefix: "r"}, + {ID: lonelyTodo, Kind: graph.KindTodo, Name: "todo:9", FilePath: `r/pkg/only.go`, RepoPrefix: "r"}, + }, []*graph.Edge{ + {From: twinnedSlur, To: twinnedTodo, Kind: graph.EdgeAnnotated, FilePath: twinnedSlur, Line: 4}, + {From: `r/pkg/only.go`, To: lonelyTodo, Kind: graph.EdgeAnnotated, FilePath: `r/pkg/only.go`, Line: 9}, + }) + require.NoError(t, s.Close()) + + withRawDB(t, path, func(db *sql.DB) { + _, err := db.Exec(`PRAGMA user_version = 12`) + require.NoError(t, err, "reset to the pre-purge version") + }) + + s2, err := Open(path) + require.NoError(t, err) + t.Cleanup(func() { _ = s2.Close() }) + + require.Nil(t, s2.GetNode(twinnedTodo), + "a row whose native twin is present is a duplicate and heals") + require.NotNil(t, s2.GetNode(lonelyFix), + "a fixture with no native twin was indexed on POSIX and must survive") + require.NotNil(t, s2.GetNode(lonelyTodo), + "a row with no twin is unreachable residue, not a duplicate: left alone rather than risked") + require.Len(t, s2.GetOutEdges(`r/pkg/only.go`), 1, + "and its edge stays with it") +} + // TestCoverageNodeSidecarTablesCoverSchema keeps the sidecar list from // drifting: any table whose primary key is node_id must be cleaned when a // node is purged, so a newly added one has to appear in the list. Derived From e5cea45c46f8c611aa46d1e135fe00b0963401f9 Mon Sep 17 00:00:00 2001 From: pbednarcik <29061766+pbednarcik@users.noreply.github.com> Date: Fri, 28 Aug 2026 15:53:55 +0200 Subject: [PATCH 9/9] store_sqlite: decide staleness by ownership, not by path shape An independent review pass found the twin requirement unsound, with a counterexample I could reproduce. A store written on Windows and then re-indexed on POSIX - a synced home directory, a container mounting the host store - keeps its stale Windows rows, because eviction is spelling-exact and that is this migration's own premise. Those stale rows then set the repository's Windows verdict AND vouch for the new forward-slash rows as their twin, so a LIVE POSIX todo and a stale legacy todo become identical in every path field. The live one was deleted, with its coverage edges. A POSIX filename legally containing a backslash produces the same collision in a single-platform store. Both are now regression tests, and both failed before this commit. Path shape cannot answer the question, so the predicate no longer asks it alone. 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-spelling 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, since a fixture node IS its own path - which is also the case the `files` check covers, because a fixture with no symbols in it has no other claimant. Measured on a real Windows store: of 1,088 legacy artifact rows, zero had their path in `files` and zero were claimed by a non-coverage node, while all 1,088 twins had both. The delete set is unchanged at 1,088 nodes and 1,088 edges, selected in 18 ms and 40 ms against a 4.3 GB store, and the ambiguous cases are excluded by construction rather than by argument. Note for anyone extending this: file_mtimes is NOT usable as the ownership test. It is keyed repo-relative and slash-normalized, so a legacy path matches it and a native path does not - the exact inversion of the signal wanted here. --- .../coverage_spelling_crossplatform_test.go | 122 ++++++++++++++++++ .../store_sqlite/coverage_spelling_purge.go | 61 +++++++-- 2 files changed, 174 insertions(+), 9 deletions(-) create mode 100644 internal/graph/store_sqlite/coverage_spelling_crossplatform_test.go diff --git a/internal/graph/store_sqlite/coverage_spelling_crossplatform_test.go b/internal/graph/store_sqlite/coverage_spelling_crossplatform_test.go new file mode 100644 index 000000000..92da34afb --- /dev/null +++ b/internal/graph/store_sqlite/coverage_spelling_crossplatform_test.go @@ -0,0 +1,122 @@ +package store_sqlite + +import ( + "database/sql" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/zzet/gortex/internal/graph" +) + +// TestOpenKeepsLiveRowsWhenAStoreCrossesPlatforms is the case that decides +// whether the purge may delete anything at all on a path-shape argument. +// +// A store written on Windows and then carried to POSIX - a synced home +// directory, a container mounting the host's store - keeps its Windows +// rows, because eviction is spelling-exact and that is this migration's +// own premise. Re-indexing there produces LIVE forward-slash rows for the +// same logical files, while the stale backslash rows remain to vouch for +// them: they set the repository's Windows verdict and they satisfy the +// native-twin test. At that point a live POSIX row and a stale Windows +// legacy row are identical in every path field. +// +// What separates them is not the spelling but whether anything else in the +// graph still claims that path. A legacy artifact is the only thing that +// ever carried its re-spelled path; a live file's path is also carried by +// its file node and by every symbol in it. +func TestOpenKeepsLiveRowsWhenAStorePlatformChanges(t *testing.T) { + path := filepath.Join(t.TempDir(), "store.sqlite") + + const ( + staleWinFile = `r/a\b.go` // left over from the Windows run, never evicted + liveFile = `r/a/b.go` // the same logical file, re-indexed on POSIX + liveSymbol = `r/a/b.go::Fn` + liveTodo = `r/a/b.go::todo:3` + liveLicense = `r/license::MIT` + + // A genuine legacy row in the same store: its path is claimed by + // nothing but itself, so it still heals. + deadTodo = `r/c/d.go::todo:9` + deadPath = `r/c/d.go` + liveDead = `r/c\d.go` + ) + + s, err := Open(path) + require.NoError(t, err) + s.AddBatch([]*graph.Node{ + {ID: staleWinFile, Kind: graph.KindFile, Name: "b.go", FilePath: staleWinFile, RepoPrefix: "r"}, + {ID: liveFile, Kind: graph.KindFile, Name: "b.go", FilePath: liveFile, RepoPrefix: "r"}, + {ID: liveSymbol, Kind: graph.KindFunction, Name: "Fn", FilePath: liveFile, RepoPrefix: "r"}, + {ID: liveTodo, Kind: graph.KindTodo, Name: "todo:3", FilePath: liveFile, RepoPrefix: "r"}, + {ID: liveLicense, Kind: graph.KindLicense, Name: "MIT", FilePath: liveFile, RepoPrefix: "r"}, + {ID: liveDead, Kind: graph.KindFile, Name: "d.go", FilePath: liveDead, RepoPrefix: "r"}, + {ID: deadTodo, Kind: graph.KindTodo, Name: "todo:9", FilePath: deadPath, RepoPrefix: "r"}, + }, []*graph.Edge{ + {From: liveFile, To: liveSymbol, Kind: graph.EdgeDefines, FilePath: liveFile, Line: 1}, + {From: liveFile, To: liveTodo, Kind: graph.EdgeAnnotated, FilePath: liveFile, Line: 3}, + {From: liveFile, To: liveLicense, Kind: graph.EdgeLicensedAs, FilePath: liveFile}, + {From: deadPath, To: deadTodo, Kind: graph.EdgeAnnotated, FilePath: deadPath, Line: 9}, + }) + require.NoError(t, s.Close()) + + withRawDB(t, path, func(db *sql.DB) { + _, err := db.Exec(`PRAGMA user_version = 12`) + require.NoError(t, err, "reset to the pre-purge version") + }) + + s2, err := Open(path) + require.NoError(t, err) + t.Cleanup(func() { _ = s2.Close() }) + + require.NotNil(t, s2.GetNode(liveTodo), + "a live POSIX todo must survive: its path is claimed by a real file node") + require.NotNil(t, s2.GetNode(liveLicense), + "and the license it points at must survive with it") + require.Len(t, s2.GetOutEdges(liveFile), 3, + "every live edge on that file must survive") + + require.Nil(t, s2.GetNode(deadTodo), + "a genuine legacy row, whose path nothing else claims, still heals") +} + +// TestOpenKeepsLiveRowsWhenAPosixFilenameContainsABackslash is the +// pathological but legal shape: a backslash is a valid character in a POSIX +// filename, so one such file makes its repository look Windows-written and +// doubles as the native twin of a genuinely different file. +func TestOpenKeepsLiveRowsWhenAPosixFilenameContainsABackslash(t *testing.T) { + path := filepath.Join(t.TempDir(), "store.sqlite") + + const ( + oddFile = `r/a\b.go` // ONE real POSIX file whose name contains a backslash + realFile = `r/a/b.go` // a different real file + realSym = `r/a/b.go::Fn` + realTodo = `r/a/b.go::todo:7` + ) + + s, err := Open(path) + require.NoError(t, err) + s.AddBatch([]*graph.Node{ + {ID: oddFile, Kind: graph.KindFile, Name: `a\b.go`, FilePath: oddFile, RepoPrefix: "r"}, + {ID: realFile, Kind: graph.KindFile, Name: "b.go", FilePath: realFile, RepoPrefix: "r"}, + {ID: realSym, Kind: graph.KindFunction, Name: "Fn", FilePath: realFile, RepoPrefix: "r"}, + {ID: realTodo, Kind: graph.KindTodo, Name: "todo:7", FilePath: realFile, RepoPrefix: "r"}, + }, []*graph.Edge{ + {From: realFile, To: realSym, Kind: graph.EdgeDefines, FilePath: realFile, Line: 1}, + {From: realFile, To: realTodo, Kind: graph.EdgeAnnotated, FilePath: realFile, Line: 7}, + }) + require.NoError(t, s.Close()) + + withRawDB(t, path, func(db *sql.DB) { + _, err := db.Exec(`PRAGMA user_version = 12`) + require.NoError(t, err, "reset to the pre-purge version") + }) + + s2, err := Open(path) + require.NoError(t, err) + t.Cleanup(func() { _ = s2.Close() }) + + require.NotNil(t, s2.GetNode(realTodo), "a live todo must survive") + require.Len(t, s2.GetOutEdges(realFile), 2, "its live edges must survive") +} diff --git a/internal/graph/store_sqlite/coverage_spelling_purge.go b/internal/graph/store_sqlite/coverage_spelling_purge.go index 989b2cb5a..86df3f249 100644 --- a/internal/graph/store_sqlite/coverage_spelling_purge.go +++ b/internal/graph/store_sqlite/coverage_spelling_purge.go @@ -17,6 +17,12 @@ import ( const ( coveragePerFileNodeKinds = `'todo','fixture'` coverageEdgeKinds = `'annotated','licensed_as','owns','generated_by','depends_on_module'` + // coverageOwnedNodeKinds are the kinds whose FilePath may legitimately + // carry a coverage builder's re-spelling: the per-file artifacts + // themselves, and the shared targets whose FilePath is a first-sighting + // breadcrumb rather than an ownership claim. Any OTHER kind at a path + // means the indexer really parsed a file there. + coverageOwnedNodeKinds = `'todo','fixture','license','team','module','artifact'` ) // coverageNodeSidecarTables are the node_id-keyed sidecars a removed node @@ -81,8 +87,15 @@ func purgeLegacyCoverageSpellings(tx *sql.Tx) error { if err != nil || scope.empty() { return err } - legacyNodePath := scope.legacyRowPredicate("nodes.file_path") - legacyEdgePath := scope.legacyRowPredicate("edges.file_path") + // The node candidate additionally has to be absent from `files`, the + // indexer's own record of the paths it parsed. That is the check that + // covers a fixture with no symbols in it, which nothing else can tell + // apart from a legacy artifact. Matched on (repo_prefix, file_path) so + // it seeks the primary key. + legacyNodePath := scope.legacyRowPredicate("nodes.file_path", "nodes.id") + + ` AND NOT EXISTS (SELECT 1 FROM files f + WHERE f.repo_prefix = nodes.repo_prefix AND f.file_path = nodes.file_path)` + legacyEdgePath := scope.legacyRowPredicate("edges.file_path", "") // Per-file artifacts whose own spelling is legacy. Collected before any // delete so the edge sweep below can clear their endpoints too. @@ -341,17 +354,47 @@ func (s coverageSpellingScope) legacyPathPredicate(column string) string { return "(" + strings.Join(arms, " OR ") + ") AND instr(" + column + ", '::') = 0" } -// legacyRowPredicate is what the migration actually selects on: a legacy -// SPELLING whose natively spelled twin is present in the store. Kept -// separate from legacyPathPredicate so the path arms stay independently -// testable. -func (s coverageSpellingScope) legacyRowPredicate(column string) string { +// legacyRowPredicate is what the migration actually selects on. The path +// shape alone is NOT enough, and that is the whole lesson of this step: a +// store written on Windows and then re-indexed on POSIX (a synced home, a +// container mounting the host's store) keeps its stale Windows rows, +// because eviction is spelling-exact. Those stale rows set the +// repository's verdict AND vouch for the new forward-slash rows as their +// "twin", at which point a LIVE POSIX row and a stale legacy row are +// identical in every path field. +// +// What actually separates them is ownership. When the indexer parses a +// file it records that path in `files` and hangs the file node and every +// symbol in it off the same path. A coverage builder's re-spelled path was +// never any of those things: it was minted by the builder and referenced +// by nothing else. So a row is legacy only when +// +// - its natively spelled twin exists (it is a re-spelling of something), AND +// - `files` does not record its path (the indexer never parsed a file there), AND +// - no node outside the coverage domains claims that path (no file node, +// no symbol) - selfID excludes the candidate row itself, because a +// fixture node IS its own path. +// +// Verified against a real Windows store: of 1,088 legacy artifact rows, +// zero had their path in `files` and zero were claimed by a non-coverage +// node, while all 1,088 of their twins had both. The delete set is +// unchanged and the ambiguous cases are now excluded by construction. +// +// selfID is the candidate's id column, or "" where the candidate is not a +// node (the edge sweep). +func (s coverageSpellingScope) legacyRowPredicate(column, selfID string) string { if s.empty() { return "0" } - return s.legacyPathPredicate(column) + + pred := s.legacyPathPredicate(column) + " AND EXISTS (SELECT 1 FROM nodes twin WHERE twin.file_path = " + - s.nativeTwinExpr(column) + ")" + s.nativeTwinExpr(column) + ")" + + " AND NOT EXISTS (SELECT 1 FROM nodes claimant WHERE claimant.file_path = " + column + + " AND claimant.kind NOT IN (" + coverageOwnedNodeKinds + ")" + if selfID != "" { + pred += " AND claimant.id <> " + selfID + } + return pred + ")" } // nativeTwinExpr renders the path a row WOULD have carried had the builder