fix(core): disambiguate genuinely-new SCIP symbols sharing a (file, name) key - #74
Conversation
Adds a new Symbol.scip_id field (the full SCIP descriptor path,
including disambiguator) so Phase 2 of the symbol-identity-and-
scoping-hardening spec can correlate SCIP occurrences to tree-sitter
symbols via a direct, exact map lookup instead of re-deriving
correlation on every pass. Revives originally-deferred item (a) from
the original 4-part ask ("persist SCIP descriptor path") now that
Phase 1 establishes the stable-id precondition the spec said was
needed first.
Mirrors the change across all three graph backends per explicit
request, not just the default Kuzu path:
- Kuzu: scip_id added to CREATE_SCHEMA's Symbol table (new DBs) and
as an ALTER TABLE migration (existing DBs), following the exact
precedent parameters/return_type/category already set.
- Neo4j: scip_id added to the schemaless property-map upsert (both
the per-file and bulk write paths).
- Cozo: scip_id added to the :create symbol relation and
import_symbols' column list, hardcoded to "" like category already
is there -- this migration utility was never fully wired for that
column either, so this matches its existing precedent rather than
introducing an inconsistency.
The actual correlation logic (Pass 1 populating scip_id, Pass 2/3
consuming it) is a separate follow-up -- this commit only adds the
column and threads None/"" through every existing Symbol construction
site, verified via full-workspace + --features remote compilation.
Caught and fixed along the way: store_parquet.rs's `COPY Symbol
(id, name, ...)` statement lists its target columns explicitly,
separate from the parquet file's own arrow schema -- missing this on
the first pass broke 9 daemon::drain tests with a Kuzu column-count
mismatch (fixed by adding scip_id to that explicit list too).
… walk tree-sitter-rust's impl_item has no "name" field (only body/trait/type/ type_parameters, verified against its real node-types.json), so every Rust impl method -- inherent or trait -- fell back to a flat, unscoped file::method id. Two types in the same file with a same-named method (e.g. both implementing Display, or both having new()) silently collapsed into one symbol, with the second dropped entirely by store_parquet.rs's sym_seen dedup guard. Fix: rust/entities.scm now captures the impl's own `type:` field as @method.parent; entities.rs consumes it via the same decompose-query mechanism already used for compound INHERITS-edge bases (generic/ qualified impl targets), falling back to the existing ancestor walk when absent -- same capture-with-fallback idiom as @func.params/ @func.return_type. Also consolidates find_parent_class (entities.rs) and find_enclosing_class (relations.rs), which had drifted into two independently-buggy copies of the same class-scoping walk (one had struct_specifier and the C++ out-of-line-method branch, the other had Elixir's defmodule and Pascal's declClass/declIntf) -- now one shared function in extract/mod.rs, used by both. Known residual gap, not fixed here (tracked as #125): a single type with two same-named methods from different sources (e.g. an inherent impl and a trait impl of the same type) still collide, since both resolve to the same parent name. That needs the separate disambiguation work in #126. Part of the symbol-identity-and-scoping-hardening spec's Phase 1 (docs/superpowers/specs/2026-08-30-symbol-identity-and-scoping-hardening-design.md).
Part A of Phase 2 (docs/superpowers/specs/2026-08-30-symbol-identity- and-scoping-hardening-design.md), fixes #126's Findings 1 & 2 at the entities.rs source rather than downstream: extract_entities' final dedup pass used to merge every symbol sharing an id string unconditionally -- correct for the same declaration matched by multiple query patterns (identical span), but wrong for two genuinely different declarations that happen to compute the same id (e.g. two same-named methods under the same parent). The latter were silently collapsed into one, with the second dropped entirely by store_parquet.rs's sym_seen guard downstream. Fix: group by id, then sub-group by exact span. Same span merges as before (unchanged behavior). More than one distinct span in a group is a genuine collision -- sort by (start_line, start_col) and append a 1-indexed ordinal suffix (#1, #2, ...) so each distinct declaration gets its own id. Symbols with no collision are completely unaffected (no suffix, same id as always). This resolves #125 as a side effect: Bar's inherent-impl and trait-impl `x` methods (identical id, different spans) now get src/main.rs::Bar::x#1/#2 instead of colliding into one. Updated that test to assert the fix; added a 3-way collision test to confirm ordinal assignment generalizes past pairs and follows source order. Verified: full workspace + infigraph-languages test suites green (inc. all 62 bundled languages' extraction tests, confirming no non-colliding symbol anywhere gets a spurious suffix), fmt/clippy clean.
Part B of Phase 2 (docs/superpowers/specs/2026-08-30-symbol-identity- and-scoping-hardening-design.md), completes #126's Finding 1: SCIP enrichment's file_name_to_ids lookup was keyed on bare (file, name), so a docstring enrichment blasted every same-named symbol in a file, and CALLS/INHERITS edge resolution picked an arbitrary one via .first() -- e.g. a reference to `B::foo` could resolve to `A::foo` just because it was inserted first. Fix, in two parts: - Pass 1 now picks the SPECIFIC same-named tree-sitter symbol whose span contains a given definition occurrence (file_name_to_ids' value type grew to carry each candidate's span), instead of enriching every same-named candidate. This depends on Phase 2 Part A's ordinal disambiguation (already landed) giving genuinely distinct same-named symbols distinct, non-overlapping spans to disambiguate by. - As each occurrence is correlated, Pass 1 now also builds scip_sym_to_ts_id: HashMap<String, String> (SCIP moniker -> resolved Symbol.id) and persists the moniker onto Symbol.scip_id (via SET for enrichment, via CREATE for SCIP-only new symbols). Since a SCIP moniker is self-consistent across every occurrence of that symbol within one index (definition and every reference, disambiguator included), Pass 2 (CALLS) and Pass 3 (INHERITS) now resolve targets via a direct, exact lookup on that map instead of re-deriving resolution through the lossy (file, name) chain -- eliminating the .first()-picks-an-arbitrary-match bug entirely rather than reducing its odds. Also extended scip/mod.rs's own new-symbols write path (previously still only 14 columns, missing category and scip_id) to include scip_id in both its arrow schema and its explicit `COPY Symbol (col1, col2, ...)` list, and the UNWIND CREATE fallback -- the exact column-list bug already caught once in store_parquet.rs during the scip_id schema commit. New regression test: two distinct types (A, B) each with their own `foo` method (same extracted name via scip_sym_to_name, different monikers, different spans) -- a reference to specifically B::foo now resolves there, not to A::foo just because it was inserted first. Verified: full workspace test suite green (475 infigraph-core tests, 14 scip:: tests including the new one), --features remote compiles, fmt/clippy clean (added NameCandidates/NewSymbolRow type aliases to satisfy a new clippy::type_complexity lint the grown tuple types triggered).
Two consecutive /// blocks with no item between them merge into one doc string on whichever item follows both. NameCandidates/NewSymbolRow's own doc comments sat directly between is_member_of_known_symbol's doc block and the function itself, so rustdoc attached the whole combined block to NameCandidates and left the function undocumented. Reordered the type aliases (with their own docs) ahead of the original block. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HdmNge4878WaicQ4QPfZme
…ame) key Phase 2's Part B (d14c982) correctly added span-containment matching to SCIP enrichment's Pass 1, but kept a `.first()` fallback for "a genuine span-computation edge case" against a single pre-existing candidate. That fallback fired far too broadly: `file_name_to_ids` is mutated in-place as Pass 1 runs, so a second brand-new same-named symbol in the same import batch sees the first one (inserted moments earlier in the same loop) as its only candidate, fails containment, and silently merges into it as an enrichment instead of becoming its own symbol. Confirmed via a real production incident: sittir's daemon repeatedly hit "duplicated primary key" COPY failures during SCIP enrichment, traced to two distinct same-named symbols (e.g. two same-named methods on different types) colliding into one id. Fix, in two parts: - Scope the `.first()` fallback to a snapshot of what existed in the graph BEFORE this import pass (`preexisting_file_name_to_ids`), never against a candidate gained during this same Pass 1 loop, and only when pre-existing ambiguity is a single candidate. - Apply entities.rs's own ordinal-disambiguation scheme (`#1`, `#2`, ...) to genuinely-new same-named symbols instead of relying on the `seen_ids` dedup, which silently kept only one and dropped the rest. Ordinals continue past whatever count already existed for that key, so a new symbol can never collide with one already in the graph. Two new regression tests: two brand-new same-named symbols in one file both survive with distinct ordinal ids; a new symbol colliding with two pre-existing same-named symbols continues the ordinal sequence instead of colliding with either. Verified: full scip:: suite (16 tests) and full infigraph-core lib suite (490 tests) green serially, fmt/clippy clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HdmNge4878WaicQ4QPfZme
…owing them Root cause of the sittir graph-explosion incident: import_scip_index()'s two preload queries (existing CALLS edges, existing Symbol rows) used `if let Ok(rows) = conn.query(...)`, which silently degraded to an empty map on any query failure rather than erroring. On a large multi-language graph (tens of thousands of symbol rows), that preload MATCH scan is exactly the kind of query that can fail under real-world resource pressure (buffer-manager contention with the daemon's own concurrent watcher writes) -- and once it does, every already-indexed symbol looks brand-new to the importer and gets re-inserted via COPY. The failure never self-heals: the same query fails again on the next enrichment cycle against the same (now even larger) graph state. Confirmed against sittir's own watch.log: four consecutive background SCIP enrichment cycles each reported ~25,200 "new" scip-typescript symbols and ~2,150 "new" rust-analyzer symbols -- essentially the full symbol set, every single time, instead of converging toward zero after the first successful import. Both preload queries now propagate failures via `?`, so a broken import surfaces as a clear "Auto-SCIP: ... import failed: ..." log line (the caller in infigraph-cli already handles this correctly) instead of silently corrupting the graph. Added two regression tests that force each preload query to fail (dropping the CALLS table / a Symbol column) and assert the import now returns an error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QRZkpP9WC2nDpBDcbsbvmA
murari316
left a comment
murari316
left a comment
There was a problem hiding this comment.
Automated review (Fable, adversarially verified). Recommendation: request-changes.
The PR disambiguates same-(file,name) symbols in both the tree-sitter path (ordinal-suffixed ids in entities.rs) and the SCIP import (span-containment matching, moniker-keyed edge resolution, a scip_id column, and loud preload failures), with substantial regression tests. The design is sound and well-documented, but the core containment mechanism compares SCIP's 0-based occurrence lines against tree-sitter's 1-based stored spans, so it fails in the dominant first-line-identifier case — masked by the single-candidate fallback on typical repos and by tests that hand-craft rows in the 0-based convention — and the ordinal rename leaves stale ids in the Pass-2 caller-attribution map. Confidence: High — every claim traced to the diff plus entities.rs L196 (row+1) and parse_range L560-573 (no conversion) on main. Recommendation: hold for fixes.
[BLOCKER] correctness — crates/infigraph-core/src/scip/mod.rs:246
The new containment match compares a SCIP occurrence's 0-based start_line (parse_range, L560-573, passes SCIP lines through unchanged) against tree-sitter symbol spans stored 1-based (entities.rs L196/L263: row + 1), so containment is off by one and fails whenever the definition identifier sits on the symbol's first line — the dominant case (def foo():, fn foo(). Single-candidate keys are rescued by the .first() fallback, so typical repos look fine; but for multi-candidate keys — the exact scenario this PR exists to fix — the fallback refuses (c.len() != 1), the occurrence takes the new-symbol path, and a duplicate Symbol row is inserted for an already-indexed symbol. That is a regression vs. main, which over-enriched but never duplicated. Note for the fix: parse_range output is also the stored span of SCIP-created symbols, so the graph holds mixed-base rows (tree-sitter 1-based, SCIP-created 0-based); a naive +1 at the comparison site alone breaks re-import containment against previously stored SCIP rows.
Failure scenario: File has impl A { fn foo... } (tree-sitter span [10,15], id a::A::foo) and impl B { fn foo... } ([20,25], a::B::foo). SCIP emits definition occurrences at 0-based lines 9 and 19. Containment: 9>=10 false, 19>=20 false; fallback sees 2 pre-existing candidates and refuses; both defs are treated as brand-new, ordinal-suffixed rows a.rs::foo#3/a.rs::foo#4 are COPY-inserted alongside the two real symbols — duplicate nodes, and neither real symbol gets its docstring/scip_id enrichment.
[MAJOR] test-coverage — crates/infigraph-core/src/scip/mod.rs:1420
The new tests encode the same off-by-one as the code instead of catching it: they hand-CREATE Symbol rows whose start_line values match the raw 0-based SCIP occurrence lines (e.g. calls_edge_resolves_to_the_specific... creates A::foo with start_line 1 and passes range: vec![1,4,7], with a comment claiming the parsed value is '1-based line 2' — parse_range performs no such conversion). No test runs real tree-sitter extraction followed by a real SCIP import, which is what would expose the base mismatch; the repo convention of extending tests/fixtures/microservices (which exercise the full extract→import pipeline) would have caught it.
Failure scenario: All new tests pass while the shipped containment logic never matches a genuinely tree-sitter-extracted symbol whose identifier is on its span's first line; the bug reaches production undetected.
[MAJOR] correctness — crates/infigraph-core/src/scip/mod.rs:322
The ordinal-rename block mutates only new_symbols[i].0 and scip_sym_to_ts_id; the file_symbols (and file_name_to_ids) entries pushed for those symbols during Pass 1 keep the stale unsuffixed id. Pass 2's caller attribution (unchanged in this PR) resolves container_id from file_symbols by containment, producing a CALLS edge FROM a node id that no longer exists after the rename; copy_edges_with_bad_record_retry then drops it as a bad-PK record with no warning. Because new SCIP symbols' stored spans are the identifier token (~1 line), this fires when a reference shares the definition's line — e.g. one-line functions/arrow functions, which are common in TS/JS — and finding #1 inflates how many symbols take the new-symbol path in the first place. A sibling symptom: a second in-batch definition occurrence that containment-matches a stale candidate pushes an enrichment row targeting the dead unsuffixed id (silent no-op UNWIND).
Failure scenario: index.scip contains new same-named one-line symbols const foo = () => bar() in classes A and B; both are renamed to test.ts::foo#1/#2, but file_symbols still says test.ts::foo. The reference to bar on foo#1's line attributes container_id = 'test.ts::foo' (nonexistent) → COPY CALLS fails on that record → the edge is silently dropped.
[MINOR] correctness — crates/infigraph-core/src/scip/mod.rs:265
The .first() fallback's c.len() == 1 guard counts pre-existing candidates but not how many distinct monikers use the fallback, so any number of genuinely-new same-named symbols whose spans fail containment all merge into the single lone pre-existing symbol — the same silent-collapse failure mode the PR fixes for the no-preexisting case. Each pushes an enrichment (last scip_id wins) and maps its moniker to the same ts id, misdirecting Pass 2/3 edges.
Failure scenario: Graph has one tree-sitter foo at [1,5] (e.g. macro-heavy Rust file where tree-sitter found only one of three impls). SCIP defines A#foo (line 2), B#foo (line 10), C#foo (line 20). B and C fail containment, both hit the single-candidate fallback, both 'enrich' the [1,5] symbol; no new rows are created and all references to B::foo/C::foo resolve to the wrong node.
[MINOR] correctness — crates/infigraph-core/src/scip/mod.rs:345
Ordinal continuation uses existing_count (candidate count) rather than the max existing #n suffix, so a gapped ordinal set — e.g. foo#2,foo#3 remaining after a prior run's bad-PK record drop or a partial delete — makes the new id file::foo#3 collide with a pre-existing row; the COPY retry loop then drops the new symbol silently.
Failure scenario: Prior state: test.ts::foo#2 and test.ts::foo#3 exist (foo#1 was dropped by an earlier COPY bad-record retry). New D#foo arrives; existing_count=2 → id test.ts::foo#3 → duplicated primary key → extract_bad_copy_value drops it → symbol lost with only an eprintln.
[MINOR] correctness — crates/infigraph-core/src/scip/mod.rs:430
The learned-correction detection derives call_name via target_id.rsplit("::").next(), which now yields ordinal-suffixed names like x#2 for disambiguated targets; the comparison against tree-sitter targets' names (which may carry a different or no suffix) can no longer match, so corrections involving disambiguated symbols are silently never learned (or spuriously learned).
Failure scenario: SCIP resolves a call to test.ts::Bar::x#2 while tree-sitter had an edge to test.ts::Bar::x#1; call_name 'x#2' != 'x#1' so ts_had_different is false and the correction is not recorded.
[MINOR] consistency — crates/infigraph-core/src/graph/cozo_store.rs:713
CozoStore::import_symbols adds the scip_id column but always writes a hardcoded empty string (the 14-tuple input carries no scip_id), so any migration/export through the Cozo path silently drops SCIP enrichment data that the kuzu and neo4j backends persist.
Failure scenario: A project with SCIP-enriched symbols is migrated via import_symbols into Cozo; every row's scip_id comes back '' and moniker-based lookups against the Cozo backend find nothing.
[NIT] style — crates/infigraph-core/src/extract/entities.rs:1806
Contradictory comments on the rust_entity_query test helper: the test doc comment above it claims it 'Uses the real bundled Rust language pack (entities.scm + inherit_decompose_query), not a hand-rolled query', while the helper's own comment says it 'hand-builds the query rather than loading the registry' and is 'kept in sync manually'. One of the two must be corrected so future readers know the queries can drift from the shipped .scm files.
Failure scenario: rust/entities.scm changes its impl-method pattern; the test keeps passing against the stale inline copy while readers of the doc comment believe the shipped query is exercised end-to-end.
[NIT] scope — crates/infigraph-core/src/embed/mod.rs:399
The chunks_exact -> as_chunks::<4>() refactor in load_embeddings is unrelated to this PR's purpose and silently raises the effective MSRV to Rust 1.88 (slice::as_chunks stabilization); since CI floats on dtolnay stable this passes today, but it belongs in its own change, not a correctness PR.
Failure scenario: A contributor or downstream consumer pinned below 1.88 gets a compile error in a PR ostensibly about SCIP symbol disambiguation.
Generated with Claude Code (Fable). Findings verified by an adversarial refute pass; false positives removed.
2 participants
Depends on #72
This PR continues directly from #72 (`fix(core): scope Rust impl methods to their type, dedup class-scoping walk`), which is still open. #72's own description tracks the remaining work as pradeepmouli#125 and #126 — this PR is that work, plus one more fix on top.
This branch includes #72's commit (
7f05b59there, re-applied here as its own commit of identical content) purely so the branch builds and its tests pass in isolation. Once #72 merges, this should be rebased onto the then-currentmain— the diff will shrink to just the commits below, with no duplication. Please don't merge this before #72, or hold off reviewing the first commit specifically (it's a re-run of #72's own diff, not new).Summary
The symbol-identity-and-scoping-hardening spec's Phase 2 (Parts A & B) added ordinal disambiguation for genuinely-distinct symbols that share a derived id — but left one residual gap: SCIP enrichment's
.first()fallback (a deliberate narrow escape hatch for "a genuine span-computation edge case against a single pre-existing symbol") fires far too broadly, becausefile_name_to_idsis mutated during the same Pass 1 loop that reads it. A second brand-new same-named symbol in the same import batch sees the first one (inserted moments earlier, same loop) as its only candidate, fails span-containment, and silently merges into it as an enrichment instead of becoming its own symbol.Found via a real production incident: sittir's daemon repeatedly hit "duplicated primary key" COPY failures during SCIP enrichment, traced to two distinct same-named symbols (two same-named methods on different types) colliding into one id.
Fix, in two parts:
.first()fallback to a snapshot of what existed in the graph before this import pass, never against a candidate gained during the same Pass 1 loop, and only when pre-existing ambiguity is a single candidate.entities.rs, Phase 2 Part A) to genuinely-new same-named symbols, instead of relying on theseen_idsdedup, which silently kept only one and dropped the rest. Ordinals continue past whatever count already existed for that key, so a new symbol can never collide with one already in the graph.Also included (found necessary during independent verification of this branch against a fresh checkout of
main):fix(scip): propagate preload query failures instead of silently swallowing them— a pre-existing gap in the same file that two of this branch's own regression tests caught when run against a fresh base; without it,import_scip_indexsilently swallows a failed preload query instead of surfacing it.chunks_exact→as_chunks) inembed/mod.rs, needed only to satisfy this repo's pre-commit hook on the base commit — zero functional change.Two new regression tests: two brand-new same-named symbols in one file both survive with distinct ordinal ids; a new symbol colliding with two pre-existing same-named symbols continues the ordinal sequence instead of colliding with either.
Verification
Independently verified on a fresh branch off
upstream/main(not just cherry-picked and trusted): full workspace build,cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings, and the fullinfigraph-corelib suite (318 tests) all green.🤖 Generated with Claude Code
https://claude.ai/code/session_01HdmNge4878WaicQ4QPfZme