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..690f15d87 100644 --- a/internal/codegen/scanner_test.go +++ b/internal/codegen/scanner_test.go @@ -84,6 +84,26 @@ 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. + // 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)) + } + 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..c7c2d3787 100644 --- a/internal/codeowners/parser_test.go +++ b/internal/codeowners/parser_test.go @@ -110,3 +110,26 @@ 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. + // 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)) + } + 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..094b0b0a4 100644 --- a/internal/fixtures/scanner_test.go +++ b/internal/fixtures/scanner_test.go @@ -83,6 +83,31 @@ 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). + // 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)) + } + 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) + } +} + func TestReclassifyFileToFixture(t *testing.T) { t.Run("upgrades file to fixture", func(t *testing.T) { n := &graph.Node{ 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 new file mode 100644 index 000000000..86df3f249 --- /dev/null +++ b/internal/graph/store_sqlite/coverage_spelling_purge.go @@ -0,0 +1,438 @@ +package store_sqlite + +import ( + "database/sql" + "fmt" + "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'` + // 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 +// 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. +// 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", + "release_enrichment", + "blame_enrichment", +} + +// 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. 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 +// 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 { + scope, err := windowsWrittenScope(tx) + if err != nil || scope.empty() { + return err + } + // 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. + // + // 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 + ` + 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 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 temp.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 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. + 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, + // 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 + } + + // 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. + 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 + } + // 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) +} + +// 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 + 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 +} + +// 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 WHERE kind = 'file' GROUP BY repo_prefix`) + if err != nil { + return scope, err + } + defer rows.Close() //nolint:errcheck // read-only cursor + for rows.Next() { + var prefix string + 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) + } + } + return scope, rows.Err() +} + +// legacyPathPredicate builds the SQL test "this path is a pre-fix +// 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 (s coverageSpellingScope) legacyPathPredicate(column string) string { + if s.empty() { + return "0" + } + var arms []string + for _, prefix := range s.windowsPrefixes { + lit := quoteSQLLiteral(prefix + "/") + 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 + 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()) + } + return "(" + strings.Join(arms, " OR ") + ") AND instr(" + column + ", '::') = 0" +} + +// 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" + } + pred := s.legacyPathPredicate(column) + + " AND EXISTS (SELECT 1 FROM nodes twin WHERE twin.file_path = " + + 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 +// 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 new file mode 100644 index 000000000..d24feb20e --- /dev/null +++ b/internal/graph/store_sqlite/coverage_spelling_purge_test.go @@ -0,0 +1,706 @@ +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` + nativeFix = `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` + teamSolo = `r/team::solo` + generator = `r/generator::protoc` + symbolA = nativeA + `::Alpha` + ) + + s, err := Open(path) + 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. + {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"}, + // 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}, + {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}, + {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. + 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.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") + 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) + + 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") +} + +// 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") +} + +// 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", "nest"}, + knownPrefixes: []string{"win", "gortex", "gortexish", "nix", "nest", "nest/inner"}, + } + + 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"}, + {`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") + 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")) + }) +} + +// 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 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. +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 a62781733..fc9f648aa 100644 --- a/internal/indexer/incremental_batch.go +++ b/internal/indexer/incremental_batch.go @@ -913,6 +913,14 @@ func restubIncomingRefsFromView( func evictFilesBatched(g graph.Store, paths []string) (int, int) { paths = appendUniqueSorted(nil, paths...) + // 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 e33904198..01d3bd40a 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,6 +205,83 @@ func TestIncrementalReindex_FailedFileSurfacedAndRetried(t *testing.T) { assert.NotEmpty(t, g.FindNodesByName("Bad")) } +// 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) { + // 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` + 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}, + {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: 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 { + 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}) + + 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) + } + }) + } +} + // 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 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..d2925f4cb 100644 --- a/internal/licenses/scanner_test.go +++ b/internal/licenses/scanner_test.go @@ -72,6 +72,29 @@ 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. 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)) + } + 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..5aa336e4e 100644 --- a/internal/modules/scanner_test.go +++ b/internal/modules/scanner_test.go @@ -128,6 +128,31 @@ 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. + // 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 { + 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..60c36fbba 100644 --- a/internal/todos/scanner_test.go +++ b/internal/todos/scanner_test.go @@ -214,6 +214,35 @@ 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. + // + // 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)) + } + 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 {