Always mint a repo prefix; remove single-repo mode - #396
Merged
Conversation
A node's repo prefix is the graph's ownership key: it keys the byRepo bucket, the per-repo memory estimates, the nodes_by_repo partial index and every repo-scoped query. When it is wrong nothing errors. The daemon serves a subset, or two copies of one repo under different ids, and index_health still reports green. Add an ownership audit and wire it to the places that would otherwise stay quiet: - graph.ClassifyNodePrefix / PrefixDiagnostics partition file-backed nodes into owned, unowned and misprefixed. FilePath is the discriminator: synthetic namespaces (dep::, external::, module::, builtin::) carry no source file and are not path-prefixed, so they are not audited. - store_sqlite answers the same audit with aggregate predicates rather than a node pull; a storetest conformance case fences the two backends against drift, since a divergence would make the detector report clean on exactly the backend the daemon runs. - index_health gains a repo_ownership block, and a recommendation when the store holds both prefixed and unprefixed copies of the same code. - The daemon logs the audit after warmup: Info when clean, Error with named samples when not. - priorMtimesFromStore warns when it finds no mtimes for a repo but the store holds them under other prefixes. That is a prefix-keying mismatch forcing a full cold re-index (and, with an API embedder, a paid re-embed), and the existing Info line reported it identically to a first index. - pendingRepoPrefixes warns when a pass derives its repo scope from unprefixed ids. RepoPrefixOfID returns the leading path segment, so "internal/foo.go::Bar" yields "internal" and the dep/provides/dir indexes get built under a key nothing matches. - surface_memories explains an empty anchored result by probing whether the stored anchors still name live nodes, distinguishing an id-convention mismatch from ordinary rename drift. The rerank fence caught a live bug and it is fixed here: crossRepoRerank counted the empty prefix as a repository, so a result set mixing one repo's hits with synthetic global externals looked multi-repo, renumbered, and promoted the top external stub to rank 0 beside the top first-party hit. Only non-empty prefixes count now.
The empty repo prefix is overloaded three ways, and only one of them is a
single-repo artefact. Before removing that one, make the other two explicit
so the deletion sweep cannot take them with it.
WILDCARD "no repo filter" — ChurnRows(""), CoverageRows(""),
ReleaseRows(""), BlameRows(""), GetRepoNonContentNodes(""),
NodeIDNamesByKindsSeq(""), ContentRepoHasRows("").
GLOBAL a synthetic external no repository owns — dep::, external::,
non-Go module::. Visible from every repo by construction.
Narrowing either does not fail; it returns an empty slice and the global
pass built on it quietly stops working. Table tests now fence both,
including the trap that GetRepoNonContentNodes and GetRepoContentNodes are
siblings in one file whose empty-prefix argument means opposite things.
Comment corrections:
- RepoPrefixOfID claimed it returns "" for unprefixed ids. It does not: it
is a syntactic split on the first "/", so "internal/foo.go::Bar" yields
"internal" — a directory name that callers then use as a repository.
- FindNodesByNameInRepo's empty-prefix disjunct is documented as the
cross-repo visibility rule for global externals, not a single-repo
accommodation, since dropping it would make every third-party target
unresolvable from every repo at once.
- ContentRepoHasRows("") is a wildcard over the whole sidecar, not the
"single-repository convention" its comment claimed.
- store_sqlite.EvictRepo records why it deliberately diverges from
Graph.EvictRepo on "" (the solo warm-restart bulk reload depends on the
delete) and the condition under which the divergence expires.
A repo narrow is keyed on registry prefixes, so it can never match a node that carries no prefix — every such predicate has to decide what to do with one, and the server answered that question two different ways. filterNodes, filterSubGraph and query.ScopeAllows ADMITTED prefix-less nodes. The explore verb's two post-filters — the resilience ladder's repoAllowed and exploreNodeWithinQueryScope — REJECTED them, because `RepoAllow[""]` is simply false. Under a scoped session that made the ladder pointless: explore would relax to unscoped ranked, then unscoped BM25, and the post-filter it re-applied to preserve multi-repo hygiene threw the results away again. The verb answered "nothing matched" for an index that had matches. Admit is the correct reading. Synthetic global externals (dep::, external::, non-Go module::) model third-party symbols owned by no repository and visible from every repo by construction, and workspace / project scoping still bounds them. Rejecting does not narrow a result set, it empties one. Introduce repoNarrowAdmits as the single definition and route the MCP predicates through it. query/subgraph.go keeps its own copy (different package) but its justification is corrected: the durable reason is global externals, not single-repo-mode nodes, which go away shortly. Fenced by a cross-package test asserting filterNodes, exploreNodeWithinQueryScope and ScopeAllows return the same verdict for an in-repo node, an out-of-repo node and a prefix-less global.
Gortex had two indexing modes producing two ID schemes for one graph. A
workspace's only repo was indexed WITHOUT a prefix — nodes carried an empty
RepoPrefix and raw repo-relative ids ("internal/foo.go::Bar") — while any
workspace with two or more repos prefixed everything.
The mechanism was four places choosing not to call SetRepoPrefix:
indexSingleRepo, and the willBeMultiRepo gates in TrackRepoCtx and
ReconcileRepoCtx, mirrored on the warm-restart side by warmMtimePrefix. The
cost was spread far wider: a re-mint migration for when a second repo
joined, an Unprefixed provenance flag threaded through eviction and scoping,
whole-graph scan fallbacks where a byRepo bucket lookup would do, and per-repo
counters that skipped the lone repo entirely — the root cause of #261/#270,
where a tracked repo rendered a near-empty status row.
Prefix unconditionally. A lone repo is the first tracked repo and takes the
same path as any other, so one ID scheme covers the graph at every workspace
size. Repo COUNT still decides IsMultiRepo() and whether an unqualified path
is ambiguous; it no longer decides ID SHAPE.
Deleted: indexSingleRepo, both willBeMultiRepo gates,
migrateLoneUnprefixedRepoCtx and its call sites, foldDistinctRepoCount (it
existed only to backstop the gate), the IndexScoped len==1 branch, and
warmMtimePrefix's repo-count mirror. RepoMetadata.Unprefixed stops being
stamped but stays for now so this commit is reviewable; it goes next.
Two things the flip forces, which cannot wait:
- loneRepoLocked now means "the sole tracked repo" rather than "the sole repo
that was indexed unprefixed". Left as-is it would be dead, and RepoRoot("")
would stop resolving — breaking every bare repo-relative path an agent,
hook or review pack supplies.
- ResolveFilePath's stat()-based collision guard is removed. It disambiguated
a lone repo's raw relative paths; against prefixed paths it would prefer a
raw join whenever a repo contained a directory matching its own name,
returning the wrong file. Prefix stripping is now unambiguous.
Schema v7 purges the on-disk residue. A store written before this holds a solo
repo's file-backed nodes under an empty prefix that nothing can reach —
PurgeRepo refuses it, OrphanRepoPrefixes exempts it — so the first
post-upgrade warmup would write a prefixed copy beside them and serve the
graph twice with index_health green. The step is in-place rather than
rebuild:true because planSchemaMigrationWith reads that flag statically, and
every multi-repo workspace would otherwise eat a cold index for a change that
cannot affect it. Synthetic global externals survive: they carry no file path,
which is exactly what the predicate keys on.
Test churn is the mode's own footprint. The single→multi transition tests
become "adding a repo changes nothing"; the ID-format-by-mode property test
asserts one invariant at every workspace size; and the #261/#270 status tests
now assert that per-repo rows account for the whole store exactly, with no
unattributed pool to guess back.
With every repo prefixed the flag is always false, so every branch keyed on it is dead. Removing them collapses a lot of compensation: - UntrackRepo loses its file-by-file EvictFile loop. That branch sat ABOVE the PurgeRepo capability probe, so a solo untrack skipped the sidecar-aware purge entirely and leaked fifteen repo_prefix-keyed tables every cycle. - tsAliasMapFor stops treating an empty prefix as "the lone repo". A tracked repo always carries one; a metadata entry without one is malformed and cannot claim the file. - search_text attribution drops its prefix-stripping retry. GrepTextForRepos stamps the registry prefix and graph file keys carry it, so the two agree by construction rather than by a provenance-gated fixup. - `daemon status` loses ~130 lines. Every tracked repo now has a live per-prefix bucket, so the row is a lookup. Two separate reconstructions existed to undo the missing bucket — whole-store attribution for a sole repo, and an "unaccounted pool" heuristic for a multi-repo workspace holding one empty-bucket repo — plus an expectedGap fudge so the counters-below-tracked diagnostic would not fire on the expected shortfall. A short bucket is now always a defect and always warns. Also fixes an agent-facing bug the flip exposed. resolveFilePath echoed a caller-supplied bare path back as relPath, but relPath is what downstream node lookups key on and graph file keys are prefixed — so get_file_summary resolved the bytes and then answered file_not_indexed for a file that was indexed. It now returns the graph's spelling. Bare repo-relative input keeps working in a solo workspace, including for files that do not exist yet (write_file / edit_file create targets), which existence-gated anchoring alone could not resolve; soleTrackedRepo is the explicit lookup behind it. The prefixed-path collision guard in resolveFilePath goes with its sibling in MultiIndexer.ResolveFilePath: it preferred a raw join when the file existed, which against prefixed paths returns <root>/api/handlers.go where the caller meant <root>/handlers.go for a repo named after its own subdirectory.
Removing single-repo mode makes every remaining `repoPrefix == ""` branch in
the graph layer look like residue. Deleting them would break a live path.
Only MultiIndexer was made to always prefix. The STANDALONE Indexer —
indexer.New with no SetRepoPrefix — still mints unprefixed nodes by design,
and it backs `gortex init`, the eval harnesses, bench, pkg/gortex's public
API and serverstack's embedded indexer. GetRepoNodes(""),
GetRepoNodesByLanguage("", lang) and GetRepoContentNodes("") are that
indexer's own repo, not dead code, and EvictRepo("") on the SQLite backend is
how its warm-restart bulk reload avoids duplicating the graph.
Correct the doc comments, which all attributed the empty-prefix branch to
"the single-repository case", and add a test that indexes through the
standalone path and asserts those reads still return its nodes. The failure
they guard is silent: a deletion here does not error, it just returns nothing
and every standalone caller goes blind.
This replaces the planned deletion of these scans — they were assumed
unreachable and are not.
Three places inferred "another repo" from an empty repo prefix. That was harmless while a lone repo indexed unprefixed — caller and candidate were both "" so the same-repo path claimed synthetic global externals first — and becomes wrong the moment every repo carries a prefix, because those globals now fall through to the cross-repo machinery instead. None of it errors. - resolver/cross_repo.go: every Edge.CrossRepo stamp routes through a new isCrossRepoHop(callerRepo, targetRepo). An empty target is not a boundary crossing: dep::, external:: and non-Go module:: nodes are owned by no repository and visible from every one. Without the guard each such bind was counted as a cross-repo edge and filed under stats.ByRepo[""] — a key naming no repo — so `analyze kind=cross_repo` reported crossings that never happened. The reachability gate admits an empty target unconditionally, so nothing else would have caught it. - resolver/resolve_all_pending.go: an ID-derived repo prefix is now checked against the store's known prefixes and dropped when it matches none. RepoPrefixOfID is a syntactic split on the first "/", so for a node id the pass cannot hydrate it happily yields a directory name — "internal" — and scoping ensureDep / ensureProvides / ensureDir to that key built indexes nothing could match. They came back empty with no error. Rejected guesses are logged rather than silently substituted. - search/rerank: the same-repo affinity arm of CommunitySignal saturates at 1.0 and overrides the cluster score, which is correct when candidates span repositories and useless when they do not. On a single-repo batch every first-party candidate is in the home repo, so the signal flattened to a constant and discarded the cohesion score that still separated them. It was unreachable on a solo workspace only by accident — candidates had no prefix and the ctx.RepoPrefix != "" guard failed. Contexts now precompute whether the batch actually spans repos (counting owned candidates only, so a global external cannot make a solo batch look multi-repo) and gate the arm on it.
A pass over the doc comments that attributed the empty repo prefix to "single-
repo mode". The daemon has no such mode any more, so every one of them now
sends a reader looking for a branch that does not exist — or, worse, invites
them to delete a branch that is still load-bearing.
The empty prefix survives in exactly two shapes, and the rewritten comments
name whichever applies:
the standalone Indexer — indexer.New with no SetRepoPrefix, backing
`gortex init`, the eval harnesses, bench, pkg/gortex and serverstack's
embedded indexer. It mints unprefixed ids by design.
graph/store.go (BulkSetSymbols), analysis/diffmap.go, analysis/scaffold.go,
mcp/field_query.go (expandPathPrefixesWithRepos), releases/releases.go,
contracts/spring_config.go
a synthetic global external — dep::, external::, non-Go module:: — owned
by no repository.
mcp/field_query.go (applyFieldFilters)
Also retires two references to machinery this branch deleted:
store_purge.go's rekey tables described themselves in terms of the
solo→multi re-mint, whose only caller (migrateLoneUnprefixedRepoCtx) is gone;
RekeyRepoPrefix stays as the general re-key primitive and now says so.
No behaviour change. This replaces the planned removal of the Spring
config-key namespace guard: scopedSpringConfigKeyID is reached from
Indexer.runContractExtractorsForFile with idx.repoPrefix, which the standalone
Indexer leaves empty, so the unscoped `cfg::spring::<key>` form is live.
Notes, memories, notebooks and suppressions anchor to symbol ids, and
surfacing matches those ids against the caller's by string equality. Making
repo prefixes unconditional changed every id's shape, so an anchor written as
`internal/foo.go::Bar` stops matching `gortex/internal/foo.go::Bar` and the
memory stops surfacing. Nothing errors: surface_memories returns Total: 0,
which reads exactly like "nothing relevant to this task". Notes and memories
are irreplaceable user data, so this has to be migrated rather than accepted.
SidecarStore.PrefixSymbolIDs rewrites the anchors for one repo in a single
transaction, marked once per repo_key in migration_marks — the same mechanism
the legacy gob import already used. It covers notes.symbol_id,
suppressions.symbol_id, memories.symbol_ids, notebooks.symbol_ids, and
memories.auto_links (the auto-linker's derived anchors go stale identically).
A partial rewrite would leave a repo split across two conventions, which is
worse than either, hence the one transaction.
The store does not decide what to rewrite — the caller does, because only it
can see the graph. Server.symbolAnchorRewriter is deliberately conservative
on all three axes:
- an id that still names a live node is never touched, whatever its shape.
That keeps the standalone Indexer's genuinely-unprefixed anchors intact.
- exactly one tracked prefix must produce a live node. Zero means the symbol
was renamed or deleted, which is not a convention change; several means
the same repo-relative path exists in more than one repo and nothing
distinguishes them.
- anything else is left exactly as stored.
The daemon runs it after warmup, next to the ownership audit, for the same
reason: the decision is a graph lookup, so it needs the settled graph.
The end-to-end test asserts the before state too — that the pre-migration
anchor fails to match AND that surface_memories' anchor diagnostic names the
convention mismatch — so the silent version of this failure stays impossible.
The invariant that would have caught most of this bug class in one assertion: a node's stamped repo prefix, its id, and its file path all carry the same prefix. The prefix is the graph's ownership key — byRepo bucket, per-repo memory estimates, the nodes_by_repo partial index, every repo-scoped query — and when the three disagree nothing errors. Repo-scoped reads return a subset, per-repo counts stop agreeing with whole-store counts, and index_health reports green over all of it. TestRepoPrefixParityAfterIndexing runs a real index through TrackRepoCtx and asserts the ownership audit is clean, for one tracked repo and for three. Both arms matter: a lone repo is exactly the case that used to be exempt. That completes the chain the audit is built on — graph.ClassifyNodePrefix is the shared definition, store_sqlite transcribes it to SQL, a storetest conformance case fences the two backends against drift, and this asserts the result on a real index. docs/architecture.md and docs/multi-repo.md described node IDs as prefixed "in multi-repo mode" with single-repo mode keeping a bare form. They now describe one ID shape at every workspace size, name the two things an empty repo_prefix still means (a synthetic global external, or a graph built by the standalone Indexer), and record that tools still accept an unqualified path when exactly one repo is tracked.
zzet
force-pushed
the
refactor/always-prefix-repo-node-ids
branch
from
July 28, 2026 18:37
5747a74 to
e5e3dd5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #375, Fixes #393, Fixes #309
Why
Gortex had two indexing modes producing two ID schemes for one graph. A workspace's only repo was indexed without a prefix — nodes carried an empty
RepoPrefixand raw repo-relative ids (internal/foo.go::Bar) — while any workspace with two or more repos prefixed everything.The mechanism was small: four places choosing not to call
SetRepoPrefix. The cost was not. It bought a re-mint migration for when a second repo joined, anUnprefixedprovenance flag threaded through eviction and scoping, whole-graph scan fallbacks where abyRepobucket lookup would do, ~130 lines of reconstruction indaemon status, and per-repo counters that skipped the lone repo entirely — the root cause of the near-empty status row in #261/#270.This makes prefixing unconditional. A lone repo is simply the first tracked repo, so one ID scheme covers the graph at every workspace size. Repo count still decides
IsMultiRepo()and whether an unqualified path is ambiguous; it no longer decides ID shape.The hard part is that every failure mode here is silent
None of these produce an error — they produce a subset, or a duplicate, while
index_healthstays green. So the first commit ships the detection, against unchanged behaviour, before anything moves:graph.ClassifyNodePrefix/PrefixDiagnosticspartition file-backed nodes into owned / unowned / misprefixed.store_sqliteanswers the same audit with aggregate SQL, and astoretestconformance case fences the two backends — a divergence there would make the detector report clean on exactly the backend the daemon runs.index_healthgains arepo_ownershipblock; the daemon logs the audit after warmup (Errorwith named samples when dirty).priorMtimesFromStorewarns when a repo has no mtimes but the store holds them under other prefixes — a prefix-keying mismatch forcing a cold re-index (and a paid re-embed), previously reported identically to a first index.surface_memoriesexplains an empty anchored result by probing whether stored anchors still name live nodes.That fence immediately caught a live bug on
main:crossRepoRerankcounted the empty prefix as a repository, so a result set mixing one repo's hits with synthetic global externals looked multi-repo, renumbered, and promoted the topexternal::stub to rank 0 beside the top first-party hit. Fixed in the same commit.Upgrade path
Schema v7 purges the on-disk residue. A pre-change store holds a solo repo's nodes under an empty prefix that nothing can reach —
PurgeReporefuses it,OrphanRepoPrefixesexempts it — so the first post-upgrade warmup would write a prefixed copy beside them and serve the graph twice, green. The step is in-place rather thanrebuild: truebecauseplanSchemaMigrationWithreads that flag statically, and every multi-repo workspace would otherwise eat a full cold index for a change that cannot affect it.Synthetic global externals survive: they carry no file path, which is exactly what the predicate keys on.
Stored note/memory anchors are migrated too (
SidecarStore.PrefixSymbolIDs, marked once per repo). Surfacing matches ids by string equality, so the shape change would have severed every anchor at once and returnedTotal: 0— indistinguishable from "nothing relevant". The rewriter is deliberately conservative: an id that still resolves is never touched, exactly one tracked prefix must produce a live node, and anything else is left as stored.Verified against a real pre-change store (indexed with
origin/main, opened with this branch):Then on restart:
priorMtimesFromStore loaded prefix=repo count=2,reconciled repo from snapshot,repo ownership audit clean owned=8 unowned=0 misprefixed=0. One cold index on upgrade, warm thereafter.Two plan items that turned out to be wrong
The audit assumed the remaining
repoPrefix == ""branches in the graph layer were dead. They are not — the standaloneIndexer(indexer.Newwith noSetRepoPrefix) still mints unprefixed nodes by design, and it backsgortex init, the eval harnesses, bench,pkg/gortex's public API and serverstack's embedded indexer. OnlyMultiIndexerwas changed.GetRepoNodes("")and friends are that indexer's own repo; deleting them would have silently blinded every standalone caller. Same forscopedSpringConfigKeyID's unscoped branch. Both became fences and doc corrections instead of deletions.Compatibility
write_filecreate targets) — existence-gated anchoring alone could not resolve those.resolveFilePathnow returnsrelPathin the graph's spelling. Echoing the caller's bare input back resolved the bytes and then reportedfile_not_indexedfor a file that was indexed; that bug is fixed here.UntrackRepo'sEvictFileloop also fixes a sidecar leak: that branch sat above thePurgeRepocapability probe, so a solo untrack skipped the sidecar-aware purge and leaked fifteen tables every cycle.Known residue
A pre-change solo store keeps a handful of bare
stdlib::/module::stub nodes alongside the freshly minted<repo>::-scoped ones (2 in the rehearsal above). They are in the global class — same symbol, both resolvable, visible from every repo — so they are duplicates rather than wrong answers. v7 does not purge them because a baremodule::is legitimately global for non-Go languages and the two are not distinguishable by id shape alone.Testing
Full suite green (
go test ./...andgo test -race ./...).bench/index-perf's wall-clock gate flakes under full-suite contention and passes 3/3 in isolation; it builds a bareindexer.Newand is untouched by this change.New fences, each targeting a specific silent failure:
TestRepoPrefixParityAfterIndexingstoretestPrefixDiagnosticsTestOpenV6PurgesUnprefixedSoloRepoRowsTestStandaloneIndexerKeepsTheEmptyPrefixTestEmptyPrefixIs{Wildcard,Exact}*""into an exact matchTestRepoNarrowPredicatesAgreeTestCrossRepoRerankSoloWorkspaceIsNoOpTestCommunitySignal*TestMigrateSymbolAnchors*TestResolveFilePath_LoneRepoBareRelative