From eef8ae5a3c3940c439204e93773e2b5b463fd6b5 Mon Sep 17 00:00:00 2001 From: Daryl Walleck Date: Mon, 18 May 2026 14:30:34 -0500 Subject: [PATCH 1/7] fix(tethys): resolve qualified refs from import-less files (rivets-044i) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pass 2's qualified-name fallback (`get_symbol_by_qualified_name`) does a literal string-equality match against `symbols.qualified_name`, but stored qualified names are module-stripped (free fns: `name`; methods: `parent_name::name` — see `indexing.rs:627-630`). Any ref whose source text carries a module or workspace-crate prefix systematically misses, regardless of whether the target symbol exists in the same workspace. This is the residual gap rivets-dn35 explicitly deferred: dn35 removed the `imports.is_empty()` short-circuit so import-less files could reach fallback, but the qualified-path arm still couldn't bridge stored vs. written qualified-name formats. Fix: `resolve.rs::qualified_module_fallback`. After every prior path returns None for a qualified ref, split `ref_name` at each `::` boundary from longest prefix to shortest. For each prefix, translate to a file_id via `resolver::resolve_module_path` (trying implicit-`crate::` prepend first, then as-written), then query `search_symbol_by_qualified_name_in_file` on the tail. First successful (file_id, symbol) pair wins. Implicit-crate-first preserves Rust's submodule-shadows-extern-crate scoping rule; the `submodule_shadows_workspace_crate` test in `pass2_qualified_paths.rs` is the adversarial fence for that ordering. Slice 1 of 3 (see `.rivets-044i/plan.md`). Slice 2 adds adversarial coverage for external-crate refs and `crate::`-prefixed refs. Slice 3 verifies no regression in the rivets-3d0s phantom rate or rivets-0gom ambiguity violations on the rivets workspace. Tests: - `submodule_qualified_call_resolves` (shape s-submod) - `workspace_crate_prefixed_call_resolves` (shape s-wscrate) - `submodule_shadows_workspace_crate` (interpretation-order fence) --- CLAUDE.md | 2 +- crates/tethys/src/resolve.rs | 108 ++++++ crates/tethys/tests/pass2_qualified_paths.rs | 351 +++++++++++++++++++ 3 files changed, 460 insertions(+), 1 deletion(-) create mode 100644 crates/tethys/tests/pass2_qualified_paths.rs diff --git a/CLAUDE.md b/CLAUDE.md index f743f4d..09f6b2b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -202,7 +202,7 @@ Cargo workspace with 4 crates: ### Tethys resolver internals - 3 passes: Pass 1 tree-sitter same-file (`indexing.rs::store_references`), Pass 2 imports+fallback (`resolve.rs::resolve_cross_file_references`), Pass 3 LSP (`--lsp`, fills `symbol_id IS NULL` only). -- Pass 2 short-circuits on import-less files: `resolve_refs_for_file` returns early if `imports.is_empty()`. References in such files never reach Pass-2 import/fallback resolution. +- Pass 2 resolution chain for a single ref (`resolve.rs::try_resolve_reference`): (a) explicit imports, (b) glob imports, (c) `fallback_symbol_search` (same-crate prefix → unscoped unique for unqualified; `get_symbol_by_qualified_name` literal match for qualified), (d) `qualified_module_fallback` for qualified refs only (rivets-044i: longest-prefix split, `resolve_module_path` on prefix, `search_symbol_by_qualified_name_in_file` on tail). Import-less files reach (c) and (d) — the historic `imports.is_empty()` short-circuit was removed by rivets-dn35. - **Provenance gotcha:** Pass 2's `resolve_reference` clears `reference_name` to NULL on resolve (in `db/references.rs::resolve_reference`). `reference_name IS NULL` does NOT attribute provenance to any pass. To measure which pass resolved which refs: toggle a branch in `resolve.rs` (e.g., `if false && let Some(symbol) = fallback_symbol_search(...)`), rebuild release, wipe DB, re-index, diff resolved counts. No other way without a schema change. - ALLOWED cross-crate Cargo deps in this workspace: `rivets→rivets-jsonl`, `rivets-mcp→rivets`, `rivets-mcp→rivets-jsonl`. Other ordered pairs are phantom-by-definition for resolver probes. - Known multi-crate resolver bugs to be aware of: `rivets-6aoc` and `rivets-34tv` (hardcoded `src/` join in `resolve.rs::resolve_cross_file_references`, `indexing.rs::compute_dependencies`, `indexing.rs::compute_dependencies_from_stored`, and the workspace-crate arm of `resolver.rs::resolve_module_path`); `rivets-714v` (`--lsp` multi-crate path bug). Run `rivets show ` for current state. diff --git a/crates/tethys/src/resolve.rs b/crates/tethys/src/resolve.rs index 4625a17..ebaf9ad 100644 --- a/crates/tethys/src/resolve.rs +++ b/crates/tethys/src/resolve.rs @@ -222,6 +222,26 @@ impl Tethys { return Ok(true); } + // Qualified-path module fallback (rivets-044i). Only fires for refs that + // both (a) contain `::` and (b) survived every prior path. Interprets the + // prefix as a module path, looks the tail up in the resolved file. + if is_qualified + && let Some(symbol) = self.qualified_module_fallback( + ref_name, + ctx.current_file_path, + ctx.src_root, + )? + { + trace!( + ref_id = ref_.id, + ref_name = %ref_name, + symbol_id = %symbol.id, + "Resolved reference via qualified module fallback" + ); + self.db.resolve_reference(ref_.id, symbol.id)?; + return Ok(true); + } + trace!( ref_name = %ref_name, file_id = %ctx.file_id, @@ -230,6 +250,94 @@ impl Tethys { Ok(false) } + /// Resolve a qualified reference by interpreting its prefix as a module + /// path and querying the resulting file for the tail symbol (rivets-044i). + /// + /// Used after every import-based and same-crate fallback has missed. The + /// existing `get_symbol_by_qualified_name` literal-string match cannot + /// resolve refs like `helper::do_thing` from an import-less file because + /// stored `symbols.qualified_name` is module-stripped (free fns store + /// `name`; methods store `parent_name::name`). This method bridges that + /// gap by translating the prefix to a `file_id` via + /// [`resolve_module_path`] and then looking up the tail in that specific + /// file. + /// + /// Tries each prefix split from longest to shortest. For each split, + /// attempts two interpretations in order: + /// 1. **Implicit-crate**: prepend `crate::` (skipped when the first + /// segment is already `crate`/`self`/`super`). This handles Rust 2018+ + /// paths like `helper::foo` from an import-less file in the same crate. + /// 2. **As-written**: the prefix passed through unchanged. This lets the + /// `crate::`/`self::`/`super::`/workspace-crate arms of + /// [`resolve_module_path`] fire. + /// + /// Returns `Ok(None)` for: unqualified names, `current_file_path = None`, + /// external-crate prefixes that `resolve_module_path` can't translate, + /// and any ref whose tail doesn't match a symbol in the resolved file. + fn qualified_module_fallback( + &self, + ref_name: &str, + current_file_path: Option<&Path>, + src_root: &Path, + ) -> Result> { + // Load-bearing for correctness: every caller that reaches this method + // either provides a real path or the function declines safely. + let Some(current_file) = current_file_path else { + return Ok(None); + }; + + let segments: Vec<&str> = ref_name.split("::").collect(); + // Load-bearing for correctness: a single-segment "qualified" ref + // (impossible per the `is_qualified` gate at the call site, but defended + // in depth) would loop zero times and return None. Explicit early-exit + // preserves that property on hand-rolled paths through this method. + if segments.len() < 2 { + return Ok(None); + } + + let crates = self.crates(); + for split in (1..segments.len()).rev() { + let prefix = &segments[..split]; + let tail = segments[split..].join("::"); + + let mut file_id = None; + + // Interpretation A: implicit-crate prepend. + if !matches!(prefix[0], "crate" | "self" | "super") { + let mut with_crate: Vec = Vec::with_capacity(prefix.len() + 1); + with_crate.push("crate".to_string()); + with_crate.extend(prefix.iter().map(|s| (*s).to_string())); + if let Some(resolved) = + resolve_module_path(&with_crate, current_file, src_root, crates) + { + let relative = self.relative_path(&resolved); + file_id = self.db.get_file_id(&relative)?; + } + } + + // Interpretation B: as-written. + if file_id.is_none() { + let as_owned: Vec = prefix.iter().map(|s| (*s).to_string()).collect(); + if let Some(resolved) = + resolve_module_path(&as_owned, current_file, src_root, crates) + { + let relative = self.relative_path(&resolved); + file_id = self.db.get_file_id(&relative)?; + } + } + + let Some(file_id) = file_id else { continue }; + if let Some(sym) = self + .db + .search_symbol_by_qualified_name_in_file(&tail, file_id)? + { + return Ok(Some(sym)); + } + } + + Ok(None) + } + /// Resolve a reference via explicit import lookup. /// /// For qualified references like `Index::open`, looks up the first segment (`Index`) diff --git a/crates/tethys/tests/pass2_qualified_paths.rs b/crates/tethys/tests/pass2_qualified_paths.rs new file mode 100644 index 0000000..5a36b10 --- /dev/null +++ b/crates/tethys/tests/pass2_qualified_paths.rs @@ -0,0 +1,351 @@ +//! Regression fence for rivets-044i: qualified refs from import-less files. +//! +//! Stored `symbols.qualified_name` is module-stripped (free fns: `name`; +//! methods: `parent_name::name` — see `indexing.rs:627-630`), so the literal +//! `get_symbol_by_qualified_name` lookup in Pass 2's `fallback_symbol_search` +//! cannot match a ref like `helper::do_thing_q` whose text carries a module +//! prefix. The fix in `resolve.rs::qualified_module_fallback` interprets the +//! prefix as a module path via `resolver::resolve_module_path`, then looks up +//! the tail in the resulting file. +//! +//! These tests defend each input-shape branch of that fix. + +use rusqlite::params; + +mod common; + +use common::{open_db, workspace_with_files}; + +/// Shape s-submod: `helper::do_thing_q()` from import-less `src/lib.rs` +/// resolves to `do_thing_q` in `src/helper.rs` via the implicit-crate +/// interpretation in `qualified_module_fallback`. +#[test] +fn submodule_qualified_call_resolves() { + let (_dir, mut tethys) = workspace_with_files(&[ + ( + "Cargo.toml", + r#" +[package] +name = "crate_a" +version = "0.1.0" +edition = "2021" +"#, + ), + ( + "src/lib.rs", + r" +mod helper; + +pub fn entry() { + helper::do_thing_q(); +} +", + ), + ( + "src/helper.rs", + r" +pub fn do_thing_q() {} +", + ), + ]); + + tethys.index().expect("index should succeed"); + + let conn = open_db(&tethys); + + let total_refs: i64 = conn + .query_row( + "SELECT COUNT(*) FROM refs r JOIN files f ON f.id = r.file_id + WHERE f.path = 'src/lib.rs'", + params![], + |row| row.get(0), + ) + .expect("count refs"); + + let resolved_refs: i64 = conn + .query_row( + "SELECT COUNT(*) FROM refs r JOIN files f ON f.id = r.file_id + WHERE f.path = 'src/lib.rs' AND r.symbol_id IS NOT NULL", + params![], + |row| row.get(0), + ) + .expect("count resolved refs"); + + let resolved_to_target: i64 = conn + .query_row( + "SELECT COUNT(*) + FROM refs r + JOIN files f ON f.id = r.file_id + JOIN symbols s ON s.id = r.symbol_id + WHERE f.path = 'src/lib.rs' AND s.name = 'do_thing_q'", + params![], + |row| row.get(0), + ) + .expect("count refs resolved to do_thing_q"); + + let definition_exists: i64 = conn + .query_row( + "SELECT COUNT(*) FROM symbols s JOIN files f ON f.id = s.file_id + WHERE s.name = 'do_thing_q' AND f.path = 'src/helper.rs'", + params![], + |row| row.get(0), + ) + .expect("count definitions"); + + eprintln!( + "PROBE 044i state: total_refs={total_refs}, resolved_refs={resolved_refs}, \ + resolved_to_target={resolved_to_target}, definition_exists={definition_exists}" + ); + + // Sanity precondition from oracle: the definition is indexed exactly once. + assert_eq!( + definition_exists, 1, + "oracle precondition: do_thing_q must be indexed in helper.rs" + ); + + // The bug claim: the qualified ref does NOT resolve to the target today. + // Pre-fix this will hold (probe agrees with bug); post-fix this flips + // (probe demonstrates fix is effective). + assert!( + resolved_to_target >= 1, + "POST-FIX expectation: qualified call `helper::do_thing_q()` from \ + import-less src/lib.rs must resolve to its definition in src/helper.rs. \ + Pre-fix this assert FAILS, demonstrating rivets-044i." + ); +} + +/// Shape s-wscrate: workspace-crate-prefixed call from an import-less +/// integration test resolves through the as-written interpretation in +/// `qualified_module_fallback` (the `crate_a` prefix is a workspace crate +/// name, so `resolve_module_path` dispatches to its workspace-crate arm). +/// +/// Layout: workspace with two members. `crate_a/src/lib.rs` defines +/// `Widget::make_widget_044i`. `crate_b/tests/it.rs` is import-less and +/// calls `crate_a::Widget::make_widget_044i()`. +#[test] +fn workspace_crate_prefixed_call_resolves() { + let (_dir, mut tethys) = workspace_with_files(&[ + ( + "Cargo.toml", + r#" +[workspace] +members = ["crate_a", "crate_b"] +resolver = "2" +"#, + ), + ( + "crate_a/Cargo.toml", + r#" +[package] +name = "crate_a" +version = "0.1.0" +edition = "2021" +"#, + ), + ( + "crate_a/src/lib.rs", + r" +pub struct Widget; + +impl Widget { + pub fn make_widget_044i() -> Self { + Widget + } +} +", + ), + ( + "crate_b/Cargo.toml", + r#" +[package] +name = "crate_b" +version = "0.1.0" +edition = "2021" + +[dependencies] +crate_a = { path = "../crate_a" } +"#, + ), + ( + "crate_b/tests/it.rs", + r" +#[test] +fn smoke() { + let _ = crate_a::Widget::make_widget_044i(); +} +", + ), + ]); + + tethys.index().expect("index should succeed"); + + let conn = open_db(&tethys); + + let resolved_to_target: i64 = conn + .query_row( + "SELECT COUNT(*) + FROM refs r + JOIN files f ON f.id = r.file_id + JOIN symbols s ON s.id = r.symbol_id + WHERE f.path = 'crate_b/tests/it.rs' AND s.name = 'make_widget_044i'", + params![], + |row| row.get(0), + ) + .expect("count refs resolved to make_widget_044i"); + + let definition_exists: i64 = conn + .query_row( + "SELECT COUNT(*) FROM symbols s JOIN files f ON f.id = s.file_id + WHERE s.name = 'make_widget_044i' AND f.path = 'crate_a/src/lib.rs'", + params![], + |row| row.get(0), + ) + .expect("count definitions"); + + let unresolved_refs_in_test: i64 = conn + .query_row( + "SELECT COUNT(*) FROM refs r JOIN files f ON f.id = r.file_id + WHERE f.path = 'crate_b/tests/it.rs' AND r.symbol_id IS NULL + AND r.reference_name LIKE '%make_widget_044i%'", + params![], + |row| row.get(0), + ) + .expect("count unresolved refs"); + + eprintln!( + "PROBE 044i shape #2 state: resolved_to_target={resolved_to_target}, \ + definition_exists={definition_exists}, unresolved_in_test={unresolved_refs_in_test}" + ); + + assert_eq!( + definition_exists, 1, + "oracle precondition: make_widget_044i must be indexed in crate_a/src/lib.rs" + ); + + assert!( + resolved_to_target >= 1, + "POST-FIX expectation: workspace-crate-prefix call \ + `crate_a::Widget::make_widget_044i()` from import-less crate_b/tests/it.rs \ + must resolve to its definition in crate_a/src/lib.rs." + ); +} + +/// Adversarial fixture: when a name is BOTH a workspace-member crate AND a +/// submodule of the current crate, Rust scoping says the submodule wins. +/// The fix's implicit-crate-first interpretation order must honor that. +/// +/// Layout: workspace with two members. `crate_a` declares `mod helper;` +/// pointing at `crate_a/src/helper.rs::pub fn local_thing`. A second member +/// crate is also named `helper`, defining `helper/src/lib.rs::pub fn +/// external_thing`. From import-less `crate_a/src/lib.rs`, the ref +/// `helper::local_thing()` MUST resolve to the submodule (not the extern +/// crate). If interpretation order is wrong, the resolver would walk into +/// `helper/src/lib.rs` and find `external_thing` there — but never +/// `local_thing`, so the ref would stay unresolved. The assertion checks +/// for resolution to the correct target by symbol name AND file path. +#[test] +fn submodule_shadows_workspace_crate() { + let (_dir, mut tethys) = workspace_with_files(&[ + ( + "Cargo.toml", + r#" +[workspace] +members = ["crate_a", "helper"] +resolver = "2" +"#, + ), + ( + "crate_a/Cargo.toml", + r#" +[package] +name = "crate_a" +version = "0.1.0" +edition = "2021" + +[dependencies] +helper = { path = "../helper" } +"#, + ), + ( + "crate_a/src/lib.rs", + r" +mod helper; + +pub fn entry() { + helper::local_thing_044i(); +} +", + ), + ( + "crate_a/src/helper.rs", + r" +pub fn local_thing_044i() {} +", + ), + ( + "helper/Cargo.toml", + r#" +[package] +name = "helper" +version = "0.1.0" +edition = "2021" +"#, + ), + ( + "helper/src/lib.rs", + r" +pub fn external_thing_044i() {} +", + ), + ]); + + tethys.index().expect("index should succeed"); + + let conn = open_db(&tethys); + + // The ref must resolve to local_thing_044i in crate_a's helper.rs, NOT + // to anything in helper/src/lib.rs. + let resolved_to_local: i64 = conn + .query_row( + "SELECT COUNT(*) + FROM refs r + JOIN files f_caller ON f_caller.id = r.file_id + JOIN symbols s ON s.id = r.symbol_id + JOIN files f_target ON f_target.id = s.file_id + WHERE f_caller.path = 'crate_a/src/lib.rs' + AND s.name = 'local_thing_044i' + AND f_target.path = 'crate_a/src/helper.rs'", + params![], + |row| row.get(0), + ) + .expect("count"); + + // Cross-check: the extern-crate `external_thing_044i` MUST NOT have been + // erroneously resolved as the target of the ref from lib.rs. + let resolved_to_external: i64 = conn + .query_row( + "SELECT COUNT(*) + FROM refs r + JOIN files f_caller ON f_caller.id = r.file_id + JOIN symbols s ON s.id = r.symbol_id + JOIN files f_target ON f_target.id = s.file_id + WHERE f_caller.path = 'crate_a/src/lib.rs' + AND f_target.path = 'helper/src/lib.rs'", + params![], + |row| row.get(0), + ) + .expect("count"); + + assert!( + resolved_to_local >= 1, + "ref `helper::local_thing_044i()` from crate_a/src/lib.rs must \ + resolve to the submodule, not to the extern crate. \ + Got resolved_to_local={resolved_to_local}" + ); + assert_eq!( + resolved_to_external, 0, + "ref MUST NOT resolve to any symbol in extern crate `helper` — \ + submodule shadows extern crate per Rust scoping. \ + Got resolved_to_external={resolved_to_external}" + ); +} From 2abc5b701bd2264311a32739efbeba8d13ba7b05 Mon Sep 17 00:00:00 2001 From: Daryl Walleck Date: Mon, 18 May 2026 14:31:35 -0500 Subject: [PATCH 2/7] test(tethys): adversarial coverage for qualified_module_fallback (rivets-044i) Slice 2 of 3. Adds two adversarial tests that pin the boundaries of qualified_module_fallback (PR introduction at slice 1): - `qualified_external_crate_stays_unresolved`: ref `std::collections::HashMap` from an import-less file MUST NOT phantom-resolve. The fixture adds a `std_helper` submodule deliberately, so a partial-prefix-match bug (e.g., implicit-crate prepend stumbling into a same-named submodule) would falsely resolve the std ref. Pins the s-extern shape. - `qualified_crate_prefix_resolves`: ref `crate::sub_044i::ThingFour` from an import-less file resolves via the explicit `crate::` arm of `resolve_module_path`. Defeats a regression in the `path[0] == "crate"` short-circuit that prevents the implicit-prepend retry from producing the nonsense `crate::crate::sub::Item` path. Pins the s-crate shape. --- crates/tethys/tests/pass2_qualified_paths.rs | 142 +++++++++++++++++++ 1 file changed, 142 insertions(+) diff --git a/crates/tethys/tests/pass2_qualified_paths.rs b/crates/tethys/tests/pass2_qualified_paths.rs index 5a36b10..27542bf 100644 --- a/crates/tethys/tests/pass2_qualified_paths.rs +++ b/crates/tethys/tests/pass2_qualified_paths.rs @@ -349,3 +349,145 @@ pub fn external_thing_044i() {} Got resolved_to_external={resolved_to_external}" ); } + +/// Shape s-extern: refs prefixed with an external-crate name +/// (`std::collections::HashMap`) MUST NOT be phantom-resolved by the new +/// fallback. The implicit-crate retry must not stumble onto a same-named +/// submodule and corrupt the answer. To defeat that bug class, the +/// fixture intentionally adds a `std_helper` submodule with a similarly +/// std-prefixed name — only the qualified call into the local helper +/// should resolve. +#[test] +fn qualified_external_crate_stays_unresolved() { + let (_dir, mut tethys) = workspace_with_files(&[ + ( + "Cargo.toml", + r#" +[package] +name = "crate_a" +version = "0.1.0" +edition = "2021" +"#, + ), + ( + "src/lib.rs", + r" +mod std_helper; + +pub fn entry() { + let _ = std::collections::HashMap::::new(); + std_helper::do_local_044i(); +} +", + ), + ( + "src/std_helper.rs", + r" +pub fn do_local_044i() {} +", + ), + ]); + + tethys.index().expect("index should succeed"); + + let conn = open_db(&tethys); + + // Any ref whose text starts with `std::` and that the resolver linked + // to a symbol is a phantom resolution. The new fallback's implicit-crate + // and workspace-crate-prefix arms both return None for `std`, so the + // count must be zero. + let std_resolved: i64 = conn + .query_row( + "SELECT COUNT(*) FROM refs r JOIN files f ON f.id = r.file_id + WHERE f.path = 'src/lib.rs' + AND r.reference_name LIKE 'std::%' + AND r.symbol_id IS NOT NULL", + params![], + |row| row.get(0), + ) + .expect("count"); + + assert_eq!( + std_resolved, 0, + "external-crate-prefixed ref `std::collections::HashMap` MUST stay \ + unresolved (no workspace symbol to bind to). Got std_resolved={std_resolved}" + ); + + // Sanity: the same-shape local call still resolves. If this regresses + // the test no longer tells us anything useful about the std::-stays-unresolved + // claim. + let local_resolved: i64 = conn + .query_row( + "SELECT COUNT(*) FROM refs r JOIN files f ON f.id = r.file_id + JOIN symbols s ON s.id = r.symbol_id + WHERE f.path = 'src/lib.rs' AND s.name = 'do_local_044i'", + params![], + |row| row.get(0), + ) + .expect("count"); + + assert!( + local_resolved >= 1, + "sanity precondition: `std_helper::do_local_044i()` must resolve \ + locally (otherwise this test isn't pinning std::-stays-unresolved)" + ); +} + +/// Shape s-crate: explicit `crate::sub::Item` from an import-less file +/// resolves via the `crate::` arm of `resolve_module_path`. Plausible bug +/// class this defeats: the implicit-crate-prepend retry inadvertently +/// produces `crate::crate::sub::Item` and shadows the correct path. The +/// `qualified_module_fallback` code branches on `path[0] == "crate"` to +/// skip the implicit-prepend specifically to avoid this. +#[test] +fn qualified_crate_prefix_resolves() { + let (_dir, mut tethys) = workspace_with_files(&[ + ( + "Cargo.toml", + r#" +[package] +name = "crate_a" +version = "0.1.0" +edition = "2021" +"#, + ), + ( + "src/lib.rs", + r" +mod sub_044i; + +pub fn entry() { + let _: crate::sub_044i::ThingFour; +} +", + ), + ( + "src/sub_044i.rs", + r" +pub struct ThingFour; +", + ), + ]); + + tethys.index().expect("index should succeed"); + + let conn = open_db(&tethys); + + let resolved_to_target: i64 = conn + .query_row( + "SELECT COUNT(*) + FROM refs r + JOIN files f ON f.id = r.file_id + JOIN symbols s ON s.id = r.symbol_id + WHERE f.path = 'src/lib.rs' AND s.name = 'ThingFour'", + params![], + |row| row.get(0), + ) + .expect("count"); + + assert!( + resolved_to_target >= 1, + "ref `crate::sub_044i::ThingFour` MUST resolve via the explicit \ + `crate::` arm of resolve_module_path. Got resolved_to_target={resolved_to_target}" + ); +} From b26b2b12fea45197186b31c538b856c438707ef1 Mon Sep 17 00:00:00 2001 From: Daryl Walleck Date: Mon, 18 May 2026 14:35:35 -0500 Subject: [PATCH 3/7] docs(rivets): commit rivets-044i diagnostic dir (slice 3 verification) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per ./ convention: probes, oracles, design, plan, and per-slice verification artifacts for the rivets-044i fix. Slice 3 verification (rivets workspace, post-fix vs baseline): - Indexing wall time: 20.81s vs 23.25s baseline (−10%, within 1.5× budget) - Phantom rate: 0.00% (claim 7 fence: PASS) - 0gom Section 3 ambiguity: 0 violations (claim 8 fence: PASS) - Resolved refs: +81 (unresolved qualified: −65) - Full workspace test suite: 1532 passed, 0 failed --- .rivets-044i/baseline-ambiguity.txt | 19 ++ .rivets-044i/baseline-phantom-rate.txt | 19 ++ .rivets-044i/baseline.md | 28 ++ .rivets-044i/design.md | 229 ++++++++++++++ .rivets-044i/oracle.md | 72 +++++ .rivets-044i/plan.md | 405 +++++++++++++++++++++++++ .rivets-044i/post-ambiguity.txt | 19 ++ .rivets-044i/post-phantom-rate.txt | 19 ++ .rivets-044i/probe.rs | 237 +++++++++++++++ .rivets-044i/related-issues.md | 32 ++ .rivets-044i/verification.md | 73 +++++ .rivets-044i/what-i-learned.md | 27 ++ 12 files changed, 1179 insertions(+) create mode 100644 .rivets-044i/baseline-ambiguity.txt create mode 100644 .rivets-044i/baseline-phantom-rate.txt create mode 100644 .rivets-044i/baseline.md create mode 100644 .rivets-044i/design.md create mode 100644 .rivets-044i/oracle.md create mode 100644 .rivets-044i/plan.md create mode 100644 .rivets-044i/post-ambiguity.txt create mode 100644 .rivets-044i/post-phantom-rate.txt create mode 100644 .rivets-044i/probe.rs create mode 100644 .rivets-044i/related-issues.md create mode 100644 .rivets-044i/verification.md create mode 100644 .rivets-044i/what-i-learned.md diff --git a/.rivets-044i/baseline-ambiguity.txt b/.rivets-044i/baseline-ambiguity.txt new file mode 100644 index 0000000..a934172 --- /dev/null +++ b/.rivets-044i/baseline-ambiguity.txt @@ -0,0 +1,19 @@ +total file_deps rows: 333 + +=== Section 1: CROSS-CRATE EDGES (claims 3, 4) === +distinct cross-crate (from, to) pairs: 8 + +FROM TO EDGES +rivets-mcp rivets 6 +rivets rivets-jsonl 2 + +=== Section 2: INTRA-CRATE EDGES (claim 5: unchanged before/after fix) === +CRATE EDGES +tethys 208 +rivets 79 +rivets-jsonl 15 +rivets-mcp 15 + +=== Section 3: AMBIGUITY CHECK (claim 6: should-have-been-None cases) === +refs resolved across crates: 326 +ambiguity violations (claim 6): 0 diff --git a/.rivets-044i/baseline-phantom-rate.txt b/.rivets-044i/baseline-phantom-rate.txt new file mode 100644 index 0000000..6a796f8 --- /dev/null +++ b/.rivets-044i/baseline-phantom-rate.txt @@ -0,0 +1,19 @@ +====================================================================== +ycaq acceptance #6 measurement (post-PR-67 K-hybrid) +====================================================================== +total file_deps rows: 333 + intra-crate: 317 + cross-crate (both known): 8 + orphan (unknown crate): 8 + +=== Cross-crate corroboration (the ycaq #6 metric) === + cross-crate edges: 8 + corroborated by import: 8 + PHANTOM (no import): 0 + phantom rate: 0.00% (target: < 1%) + -> ycaq #6 PASS + +=== FORBIDDEN-pair check (Cargo dep-graph violations) === + FORBIDDEN-pair edges: 0 (rivets-3d0s threshold: <= 5) + of those, corroborated: 0 (should be 0 in a buildable workspace) + diff --git a/.rivets-044i/baseline.md b/.rivets-044i/baseline.md new file mode 100644 index 0000000..8d6ac10 --- /dev/null +++ b/.rivets-044i/baseline.md @@ -0,0 +1,28 @@ +# rivets-044i pre-fix baselines + +Captured on branch `feat/rivets-044i-qualified-paths` @ origin/main (no +code changes yet). + +## Indexing wall time (single run, release build) +``` +$ ./target/release/tethys.exe index +Indexed 123 files, found 2647 symbols, 22190 references +Duration: 23.25s +real 0m23.394s +``` + +## Phantom rate (rivets-3d0s regression fence) +- cross-crate edges: 8 +- corroborated: 8 +- phantom: 0 +- phantom rate: 0.00% + +## 0gom Section 3 ambiguity (claim 8 regression fence) +- refs resolved across crates: 326 +- ambiguity violations: 0 + +## Resolve coverage (informational) +- total refs: 22190 +- resolved: 6756 (30.45%) +- unresolved total: 15434 +- unresolved qualified (contains `::`): 1558 ← upper bound on refs reachable by the new fallback diff --git a/.rivets-044i/design.md b/.rivets-044i/design.md new file mode 100644 index 0000000..ee5da34 --- /dev/null +++ b/.rivets-044i/design.md @@ -0,0 +1,229 @@ +# rivets-044i: Qualified refs from import-less files + +## Status + +Falsifiable design draft. Cheapest falsifier: paper hand-trace against probe +shapes (see Falsification table row 1). Run completed during design (see +"Cheapest falsifier run" section). + +## Purpose + +Extend `try_resolve_reference` so qualified refs of the form +`segment1::segment2::...::segmentN` resolve correctly *when the first +segment is not an explicit or glob import* — currently the only way these +refs can reach the cross-file resolver. After rivets-dn35, such files reach +Pass 2 but the existing fallback (`get_symbol_by_qualified_name` literal +match against `symbols.qualified_name`) is structurally unable to match +because stored qualified names are module-stripped +(`indexing.rs:627-630`: free fns store `name`; methods store +`parent_name::name`). + +## What the probe revealed + +(See `.rivets-044i/probe.rs`, `.rivets-044i/oracle.md`, +`.rivets-044i/what-i-learned.md`.) + +- Shape #1 (`helper::do_thing_q()` from import-less `crate_a/src/lib.rs`): + ref extracted, target symbol indexed, ref stays `symbol_id IS NULL`. +- Shape #2 (`crate_a::Widget::make_widget_044i()` from import-less + `crate_b/tests/it.rs`): same outcome. +- The literal-string fallback (`fallback_symbol_search`'s `is_qualified` + branch at `resolve.rs:357-359`) cannot succeed because the stored + qualified name does NOT include the source-text path prefix. + +## Input shapes enumerated + +Inputs to the new fallback path are `ref_name` strings reaching +`try_resolve_reference` after explicit/glob imports miss. Production-reachable +shapes: + +**`ref_name` first-segment classification:** +- (s-crate) `crate::...` — Rust 2018+ explicit crate-relative +- (s-self) `self::...` — sibling within current module +- (s-super) `super::...` — parent module +- (s-wscrate) `::...` — Rust 2018+ extern-crate + prefix (also covers `-` → `_` normalization, e.g. `rivets-jsonl` → `rivets_jsonl`) +- (s-submod) `::...` — implicit-crate-relative + (the **shape #1 case** the probe revealed) +- (s-extern) `::...` (`std`, `serde`, `tokio`, etc.) — + must stay unresolved + +**`ref_name` length / tail shape:** +- (t-free) tail names a free function: stored `qualified_name = name` +- (t-method) tail names an `impl` method: stored + `qualified_name = "Type::method"` +- (t-assoc) tail names an associated const/fn: same shape as method +- (t-type) tail names a type: stored `qualified_name = name` + +**`ResolveContext` shape:** +- (c-base) `explicit_imports.is_empty() && glob_imports.is_empty()` — the bug case +- (c-imp-miss) imports non-empty but first segment misses both — must reach + the new fallback the same way + +**Workspace shape:** +- (w-single) single-crate workspace (`[package]` only) +- (w-multi) `[workspace]` with multiple member crates +- (w-hyphen) crate name contains `-` (normalized at lookup) + +**Out-of-scope shapes (justified):** +- C# files. The C# extractor lacks namespace-based qualified-name storage + (rivets-jwf9). Pass 2's C# path remains coverage-bound by that ticket; + 044i does not touch the C# branch. +- LSP (Pass 3) refs. Pass 3 is independent and unaffected. +- Refs whose tail names a re-export (`pub use foo::Bar as Baz`). Re-exports + are not tracked in `symbols.qualified_name`; covered by rivets-itz7 family, + not 044i. +- Refs with generic type arguments (`Vec::iter`). The extractor strips + generics before storing `qualified_name`; the qualified-name search is + unaware of generics either way. + +## Architecture + +Pure extension of `resolve.rs::try_resolve_reference`. No schema change, no +new tables, no new public API. + +After the existing explicit-import / glob-import / `fallback_symbol_search` +attempts return None, the new path activates ONLY when the ref is qualified +(`ref_name.contains("::")`). It performs a **longest-prefix split** on the +`::`-segments, computes a file ID for each prefix via `resolve_module_path` +(with implicit-crate-prepend retry for non-prefixed first segments), then +queries `search_symbol_by_qualified_name_in_file(tail, file_id)`. First +successful (file_id, symbol) pair wins. + +### Algorithm + +```text +fn qualified_module_fallback( + ref_name: &str, + current_file: &Path, + src_root: &Path, + workspace_crates: &[CrateInfo], + db: &Index, +) -> Result> { + let segments: Vec<&str> = ref_name.split("::").collect(); + if segments.len() < 2 { return Ok(None); } + + for split in (1..segments.len()).rev() { + let prefix = &segments[..split]; // module path + let tail = segments[split..].join("::"); // symbol qualified_name + + // Interpretation A: implicit-crate (only if path[0] not already + // in {crate, self, super}; this avoids the meaningless "crate::crate::" + // shape but still tries crate-relative for the s-submod shape). + let file_id = if !matches!(prefix[0], "crate" | "self" | "super") { + let mut with_crate = vec!["crate".to_string()]; + with_crate.extend(prefix.iter().map(|s| s.to_string())); + resolve_module_path(&with_crate, current_file, src_root, workspace_crates) + .and_then(|p| db.get_file_id(&relative(p))) + } else { None }; + + // Interpretation B: as-written (lets the s-crate/self/super/wscrate + // arms of resolve_module_path fire). + let file_id = file_id.or_else(|| { + let as_owned: Vec = prefix.iter().map(|s| s.to_string()).collect(); + resolve_module_path(&as_owned, current_file, src_root, workspace_crates) + .and_then(|p| db.get_file_id(&relative(p))) + }); + + if let Some(file_id) = file_id { + if let Some(sym) = db.search_symbol_by_qualified_name_in_file(&tail, file_id)? { + return Ok(Some(sym)); + } + } + } + Ok(None) +} +``` + +### Insertion point + +`try_resolve_reference` (`resolve.rs:162-231`). After the existing +`fallback_symbol_search` returns None for a qualified ref, invoke the new +path. If it succeeds, log via `trace!` and call `db.resolve_reference`. + +The new path is invoked ONLY when `is_qualified == true`. Unqualified +behavior (rivets-0gom-protected same-crate-first then unscoped-unique) is +not touched. + +## Falsification + +| # | Claim | Falsifier | Oracle | Cost | Status | Regression fence | +|---|-------|-----------|--------|------|--------|------------------| +| 1 | Longest-prefix split with implicit-crate-first interpretation produces the correct (file_id, symbol) pair for shapes s-crate, s-self, s-super, s-wscrate, s-submod and returns None for s-extern. | Hand-trace algorithm against the 6 first-segment shapes using actual `resolve_module_path` semantics from `resolver.rs:31-65`. | Rust's own scoping rules (independent of tethys code). | 15m | **passed (paper)** — see Cheapest-falsifier run below | unit tests in `resolve.rs#tests` (one `#[test]` per shape, post-impl). | +| 2 | Same-crate qualified ref via implicit-crate-relative resolves to its definition. | Run `probe_044i.rs::probe_044i_qualified_ref_from_import_less_file` against patched resolver. | SQL: `refs.symbol_id` of the lib.rs ref equals `symbols.id` of `do_thing_q` in helper.rs. | 5m | pending impl | The probe test stays in `tests/probe_044i.rs` and becomes the regression fence (renamed to a non-probe name post-fix). | +| 3 | Workspace-crate-prefix ref from import-less integration test resolves. | Run `probe_044i.rs::probe_044i_workspace_crate_prefix_from_import_less_file` against patched resolver. | SQL: `refs.symbol_id` of the `it.rs` ref equals `symbols.id` of `make_widget_044i`. | 5m | pending impl | Same as claim 2 — kept as the regression fence. | +| 4 | External-crate-prefixed refs do NOT phantom-resolve via the new path. | Build a synthetic single-crate workspace with `std::collections::HashMap` referenced from an import-less file. After indexing, count refs with `reference_name LIKE 'std::%'` and `symbol_id IS NOT NULL`. | SQL: count is 0. Independent of resolver internals. | 10m | pending impl | New test `qualified_external_crate_stays_unresolved` in `tests/probe_044i.rs`. | +| 5 | `crate::sub::Item` qualified ref from import-less file resolves. | Synthetic workspace `crate_a/src/lib.rs` with `mod sub;`, ref `crate::sub::Thing`. After indexing, verify ref resolves to `Thing` in `sub.rs`. | SQL: `refs.symbol_id` of the ref equals `symbols.id` of the target struct. | 10m | pending impl | New test `qualified_crate_prefix_resolves` in `tests/probe_044i.rs`. | +| 6 | Unqualified-ref resolution path unchanged: `pass2_no_imports.rs::unqualified_call_in_import_less_file_resolves_via_fallback` continues to pass. | Run that test against patched code. | The test's existing oracle (resolved_to_target ≥ 1 via same-crate prefix search). | 1m | pending impl | The test itself (already present at `tests/pass2_no_imports.rs`). | +| 7 | Cross-crate file_deps phantom rate on the rivets workspace does NOT regress above 0.00%. | Re-index rivets with `cargo run --bin tethys -- index`, run `python .rivets-ycaq/probe_phantom_rate.py`. Compare to baseline. | The probe's own count (independent: reads `imports` + `file_deps` tables directly, classifies each cross-crate edge). | 30m | pending impl | Existing rivets-3d0s regression fence: `crates/tethys/tests/file_deps_corroboration.rs::k_hybrid_drops_cross_crate_call_without_import_corroboration`. Verified present 2026-05-18. | +| 8 | rivets-0gom ambiguity violations on rivets workspace do NOT increase. | Re-index rivets, run `python .rivets-0gom/probe.py`. Compare Section 3 violation count to baseline. | The 0gom probe (independent of resolve.rs). | 15m | pending impl | Existing rivets-0gom regression fence: `crates/tethys/tests/resolver_routing.rs::fallback_routes_unqualified_ref_to_same_crate_not_cross_crate`. Verified present 2026-05-18. | + +### Cheapest falsifier run (claim 1, paper hand-trace) + +For each of 6 input-shape combinations, trace the algorithm: + +| Shape | ref_name | expected outcome | algorithm trace | result | +|-------|----------|------------------|-----------------|--------| +| s-submod (shape #1) | `helper::do_thing_q` | resolve | split=1: implicit-crate prepends `["crate","helper"]` → `crate_a/src/helper.rs` (exists). Lookup `do_thing_q` (qualified_name="do_thing_q"). Hit. | **agrees** | +| s-wscrate (shape #2) | `crate_a::Widget::make_widget_044i` | resolve | split=2: prefix=`crate_a::Widget`. implicit-crate → `crate::crate_a::Widget` → not found. as-written → workspace-crate hit but `crate_a/src/Widget.rs` doesn't exist. None. split=1: prefix=`crate_a`. as-written → workspace-crate single-segment → entry_point_file = `crate_a/src/lib.rs`. Lookup `Widget::make_widget_044i` (qualified_name="Widget::make_widget_044i"). Hit. | **agrees** | +| s-extern | `std::collections::HashMap` | unresolved | All splits: prefix starts with `std` which is not crate/self/super/workspace-crate; implicit-crate `["crate","std",...]` returns None (no such submodule); as-written `["std",...]` returns None (not a workspace crate). All splits exhausted. None. | **agrees** | +| s-crate | `crate::storage::Issue` | resolve | split=2: prefix=`crate::storage`. as-written → `crate_root/storage.rs`. Lookup `Issue`. Hit. | **agrees** | +| s-super | `super::helper::foo` | resolve | split=2: prefix=`super::helper`. as-written → `current_dir/../helper.rs`. Lookup `foo`. Hit. (implicit-crate skipped because path[0] is `super`.) | **agrees** | +| s-self | `self::helper::foo` | resolve | split=2: prefix=`self::helper`. as-written → `current_dir/helper.rs`. Lookup `foo`. Hit. | **agrees** | + +All 6 shapes the design must handle produce the algorithmically-traced +outcome the oracle predicts. Claim 1 survives its cheapest-falsifier +attempt. Proceed. + +## Negative space + +Things this design deliberately does NOT do: + +1. **Does not resolve external crate references.** A ref like + `serde::Deserialize` from a workspace file stays unresolved. (We have no + index of external symbols and adding one is out of 044i's scope; LSP + Pass 3 covers this when enabled.) +2. **Does not introduce a new symbol cache.** Each unresolved qualified ref + pays O(segments) lookups per resolution attempt. For large workspaces + this could become a hot path; rivets-bjdn is the existing tracker for + that optimization, not in scope here. +3. **Does not change Pass 1 (tree-sitter same-file) or Pass 3 (LSP) + behavior.** Strictly additive to the Pass 2 fallback chain. +4. **Does not disambiguate between multiple impl blocks of the same Type + in the same file with the same method name.** Such overloads are + `cargo check`-rejected; if present (e.g., via macros), `LIMIT 1` + semantics in `search_symbol_by_qualified_name_in_file` apply + non-deterministically. We accept this — same behavior as the + existing fallback. +5. **Does not affect C# resolution.** The C# Pass 2 path is gated by + rivets-jwf9; 044i's added branch fires regardless of language but + `resolve_module_path` itself is Rust-specific and returns None for + non-Rust file extensions (already true today). + +## Deferrals / out-of-scope cross-checks + +- "rivets-jwf9 — C# namespace resolution": **verified open** + (`rivets show rivets-jwf9` → P3 open). +- "rivets-bjdn — pre-compute crate-path lookup map": **verified open** + (P4 open). +- "rivets-3d0s K-hybrid file_deps regression fence": verified present at + `crates/tethys/tests/file_deps_corroboration.rs::k_hybrid_drops_cross_crate_call_without_import_corroboration` + (2026-05-18). No action needed. +- "rivets-0gom ambiguity regression fence": verified present at + `crates/tethys/tests/resolver_routing.rs::fallback_routes_unqualified_ref_to_same_crate_not_cross_crate` + (2026-05-18). No action needed. +- "rivets-bjdn deferred perf cache": tracker ID verified; no action needed + in 044i. +- No other deferrals. + +## What changes (concrete diff surface) + +- `crates/tethys/src/resolve.rs`: + - Add `qualified_module_fallback` private method on `Tethys`. + - Call it from `try_resolve_reference` after `fallback_symbol_search` + returns None AND `is_qualified == true`. +- `crates/tethys/tests/probe_044i.rs`: + - Stays in place; both probe tests become passing regression fences + after the fix. Add claims-4 and claims-5 tests as additional cases. +- No schema changes, no new public APIs, no behavior change for + unqualified refs. diff --git a/.rivets-044i/oracle.md b/.rivets-044i/oracle.md new file mode 100644 index 0000000..6af7b6f --- /dev/null +++ b/.rivets-044i/oracle.md @@ -0,0 +1,72 @@ +# Oracle for rivets-044i + +## Probe under test + +`crates/tethys/tests/probe_044i.rs` — two `#[test]` cases that build synthetic +workspaces, run `Tethys::index()`, and query `tethys.db` for whether the +qualified ref is resolved. + +## Oracle: source-text inspection (independent of resolver) + +The probe asks: "does the resolver link refA → symbolB?" The oracle answers +the same question by a fully different mechanism — **reading the source files +that the test builds** and applying Rust's own rules to determine ground +truth. + +### Shape #1 (sibling module under same crate) + +Workspace contents (verbatim from the fixture in `probe_044i.rs`): + +``` +Cargo.toml [package] name = "crate_a" +src/lib.rs mod helper; pub fn entry() { helper::do_thing_q(); } +src/helper.rs pub fn do_thing_q() {} +``` + +Ground truth by Rust language rules: +- `lib.rs` declares submodule `helper` via `mod helper;` +- `helper::do_thing_q()` is a fully-qualified path within `crate_a` +- `do_thing_q` is `pub fn` in `helper.rs`, visible to its parent module +- Therefore the call MUST bind to the definition at `src/helper.rs::do_thing_q` + +The oracle does not invoke any tethys resolver internals. It uses the same +mechanism `rustc` would: lexical mod tree + path lookup. + +### Shape #2 (workspace-crate prefix from import-less integration test) + +Workspace contents: + +``` +Cargo.toml [workspace] members = ["crate_a", "crate_b"] +crate_a/src/lib.rs pub struct Widget; impl Widget { pub fn make_widget_044i() -> Self } +crate_b/Cargo.toml [dependencies] crate_a = { path = "../crate_a" } +crate_b/tests/it.rs crate_a::Widget::make_widget_044i() +``` + +Ground truth: `crate_b` lists `crate_a` as a dependency. Per Rust 2018+ path +prefix rules, `crate_a::Widget::make_widget_044i()` from `crate_b` binds to +`make_widget_044i` in `crate_a/src/lib.rs`. The oracle requires no resolver +component; it is the standard Rust crate-resolution algorithm. + +## Probe result (pre-fix) + +``` +PROBE 044i state: total_refs=1, resolved_refs=0, resolved_to_target=0, definition_exists=1 +PROBE 044i shape #2 state: resolved_to_target=0, definition_exists=1, unresolved_in_test=1 +``` + +## Agreement check + +| Shape | Oracle says ref should resolve | Probe says ref resolves | Agreement | +|-------|-------------------------------|------------------------|-----------| +| #1 | YES | NO | **DISAGREE → bug** | +| #2 | YES | NO | **DISAGREE → bug** | + +Probe and oracle disagree consistently on both shapes. Per +`prove-it-prototype.md`, disagreement causes 1 (broken substrate) and 3 (bad +probe) are the candidates. The probe is mechanically simple (4 SQL queries +against a known fixture); the bug *is* the substrate, which is exactly what +rivets-044i claims. So this is cause 2 — model wrong/system broken as +described. + +Both shapes hold across the bug claim. Proceed to falsifiable-design. diff --git a/.rivets-044i/plan.md b/.rivets-044i/plan.md new file mode 100644 index 0000000..cddc190 --- /dev/null +++ b/.rivets-044i/plan.md @@ -0,0 +1,405 @@ +# rivets-044i: Budgeted plan + +## Coverage map + +| Design claim | Slice | +|--------------|-------| +| 1 (longest-prefix split algorithm) | 1 | +| 2 (shape #1 same-crate qualified ref resolves) | 1 | +| 3 (shape #2 workspace-crate-prefix ref resolves) | 1 | +| 4 (external-crate refs stay unresolved) | 2 | +| 5 (`crate::` qualified ref resolves) | 2 | +| 6 (unqualified-ref behavior unchanged) | 1 (existing pass2_no_imports test re-run) | +| 7 (no rivets-3d0s phantom-rate regression) | 3 | +| 8 (no rivets-0gom ambiguity regression) | 3 | + +--- + +## Slice 1: Mechanism — implement `qualified_module_fallback` and wire into `try_resolve_reference`. + +**Claim:** Longest-prefix split with implicit-crate-first interpretation +produces the correct (file_id, symbol) pair for shapes s-submod and +s-wscrate (covering the probe's two failing tests), and is invoked only +after the existing explicit/glob/fallback paths return None for qualified +refs. + +**Oracle:** The two `#[test]` cases in `tests/probe_044i.rs` (renamed to +`tests/pass2_qualified_paths.rs` to drop the probe nomenclature). Each +test independently asserts `refs.symbol_id == symbols.id` for the +expected target via SQL — independent of resolver internals. Same +oracles as the prove-it-prototype run. + +**Stress fixture:** The two probe-inherited fixtures defeat the +non-bug-fix-baseline, but the slice ALSO needs an adversarial fixture +that would catch the most plausible bug class: **misordered +interpretation precedence.** Plausible bug: the algorithm tries +as-written before implicit-crate, so a ref where `s1` is *both* a +workspace crate name *and* a submodule of the current crate would +resolve to the wrong file. + +Fixture: workspace with two members, `crate_a` and `helper` (which is +ALSO a workspace member). Inside `crate_a`, declare `mod helper;` with +`crate_a/src/helper.rs::pub fn local_thing()`. The workspace crate +`helper` defines `helper/src/lib.rs::pub fn external_thing()`. Inside +`crate_a/src/lib.rs` (import-less), ref `helper::local_thing()`. + +Per Rust scoping the submodule shadows the extern crate, so the ref +MUST resolve to `crate_a/src/helper.rs::local_thing`, NOT to +`helper/src/lib.rs`. This fixture is added as +`pass2_qualified_paths::submodule_shadows_workspace_crate`. + +**Loop budget:** +- Per ref reaching the new fallback: O(S × (M + L)) where + - S = `ref_name.split("::").count()` (typically ≤ 5) + - M = `resolve_module_path` cost: O(S) string ops + filesystem + `.exists()` syscalls (≤ 2 per call: tries .rs file, then mod.rs) + - L = `search_symbol_by_qualified_name_in_file` cost: 1 SQL point + lookup on an indexed column (O(log #symbols)) +- Worst case per ref: ≤ 5 splits × (2 interpretations × (5 ops + 2 + syscalls) + 1 SQL lookup) ≈ 50 ops + 20 syscalls + 5 SQL queries. +- Production scale (rivets workspace): refs reaching new fallback ≤ U, + where U is the count of currently-unresolved qualified refs missing + all existing paths. From baseline metrics in `.rivets-ycaq`, total + unresolved refs ≈ 70k workspace-wide; qualified subset ≤ 40k; subset + reaching the new fallback can't exceed that. Realistic estimate + (informed by the probe-shape pattern): hundreds to low thousands. +- Total: at most ~5k × (20 syscalls + 5 SQL) ≈ 10^5 syscalls + 25k SQL + queries. +- **Verdict:** within the 10^6 ops cap. **Above** the 10^3 syscalls + cap by ~10×, justified: syscalls are filesystem `.exists()` calls + that are OS-cached after the first access; the dominant cost is SQL + lookups, which are point queries on `(qualified_name, file_id)` + index already used by Pass 2 today. + +**Wall budget:** Post-slice `cargo run --bin tethys -- index` on the +rivets workspace ≤ 1.5× pre-slice indexing wall time. Measured via +hyperfine in slice 3's verification. + +**Files:** +- `crates/tethys/src/resolve.rs` (add private method + `qualified_module_fallback`, modify `try_resolve_reference` call site) +- `crates/tethys/tests/probe_044i.rs` → renamed to + `crates/tethys/tests/pass2_qualified_paths.rs` (add the + submodule-shadowing fixture and drop the "probe" naming since it's + now a permanent regression fence) + +**Code (advisory):** + +```rust +// In resolve.rs + +/// Attempt to resolve a qualified reference by interpreting its prefix +/// as a module path and querying the resulting file for the tail symbol. +/// +/// Activates only after all import-based paths and the existing +/// `fallback_symbol_search` qualified-name lookup return None. +/// Tries each prefix split position from longest to shortest. For each +/// prefix, attempts two interpretations in order: (a) implicit-crate +/// (prepend `crate::`, skipped if path[0] is already `crate`/`self`/ +/// `super`); (b) as-written. The first file_id obtained is queried with +/// `search_symbol_by_qualified_name_in_file` on the tail. +/// +/// Returns `Ok(None)` for unqualified names, external-crate refs, and +/// any ref whose tail doesn't match a symbol in the resolved file. +fn qualified_module_fallback( + &self, + ref_name: &str, + current_file_path: Option<&Path>, + src_root: &Path, +) -> Result> { + let Some(current_file) = current_file_path else { + return Ok(None); + }; + let segments: Vec<&str> = ref_name.split("::").collect(); + if segments.len() < 2 { + return Ok(None); + } + + let crates = self.crates(); + for split in (1..segments.len()).rev() { + let prefix = &segments[..split]; + let tail = segments[split..].join("::"); + + // Interpretation A: implicit-crate (skip if already prefixed). + let mut file_id: Option = None; + if !matches!(prefix[0], "crate" | "self" | "super") { + let mut with_crate: Vec = Vec::with_capacity(prefix.len() + 1); + with_crate.push("crate".to_string()); + with_crate.extend(prefix.iter().map(|s| (*s).to_string())); + if let Some(p) = resolve_module_path(&with_crate, current_file, src_root, crates) { + let rel = self.relative_path(&p); + file_id = self.db.get_file_id(&rel)?; + } + } + + // Interpretation B: as-written. + if file_id.is_none() { + let as_owned: Vec = prefix.iter().map(|s| (*s).to_string()).collect(); + if let Some(p) = resolve_module_path(&as_owned, current_file, src_root, crates) { + let rel = self.relative_path(&p); + file_id = self.db.get_file_id(&rel)?; + } + } + + let Some(file_id) = file_id else { continue }; + if let Some(sym) = self.db.search_symbol_by_qualified_name_in_file(&tail, file_id)? { + return Ok(Some(sym)); + } + } + Ok(None) +} + +// In try_resolve_reference, after fallback_symbol_search returns None: +if is_qualified { + if let Some(symbol) = self.qualified_module_fallback( + ref_name, + ctx.current_file_path, + ctx.src_root, + )? { + trace!( + ref_id = ref_.id, + ref_name = %ref_name, + symbol_id = %symbol.id, + "Resolved reference via qualified module fallback" + ); + self.db.resolve_reference(ref_.id, symbol.id)?; + return Ok(true); + } +} +``` + +**Verification:** +- [ ] `cargo nextest run -p tethys --test pass2_qualified_paths` — both + inherited probe tests + the submodule-shadows-workspace-crate fixture + pass. +- [ ] `cargo nextest run -p tethys --test pass2_no_imports` — passes + unchanged (claim 6 non-regression). +- [ ] `cargo clippy --all-targets --all-features -- -D warnings` clean. +- [ ] Oracle (prove-it-prototype) still agrees with binary: re-run + probe tests, check assertions. + +--- + +## Slice 2: Adversarial coverage — claims 4 + 5. + +**Claim:** +- (claim 4) External-crate-prefixed refs (`std::collections::HashMap`, + `serde::Deserialize`) do NOT phantom-resolve via the new path. +- (claim 5) `crate::sub::Item` qualified refs resolve under the + s-crate first-segment shape. + +**Oracle:** SQL queries against the in-memory test DB, independent of +resolver internals — same pattern as slice 1's oracles. + +**Stress fixture:** +- **`qualified_external_crate_stays_unresolved`**: import-less file + containing a ref `std::collections::HashMap`. The DB should have + ZERO refs in that file with `reference_name LIKE 'std::%'` AND + `symbol_id IS NOT NULL`. Plausible bug class this fixture defeats: + algorithm naively trying `["crate","std",...]` and matching a + same-named submodule. Adversarial twist: ALSO add a `mod std_helper;` + (sic — looks std-ish) to defeat partial-string-match bugs. +- **`qualified_crate_prefix_resolves`**: import-less `crate_a/src/lib.rs` + with `mod sub;`, ref `crate::sub::Thing`. `crate_a/src/sub.rs` defines + `pub struct Thing`. Plausible bug class: the as-written path + incorrectly fires before implicit-crate, resolving "crate" as a + workspace-crate-name and failing. The fixture would catch that + because there's no crate named "crate" — the only path that resolves + is the `crate::` arm of `resolve_module_path`. + +**Loop budget:** Inherits slice 1's `qualified_module_fallback` cost +budget. No new loops introduced. + +**Wall budget:** N/A — test-only slice. + +**Files:** +- `crates/tethys/tests/pass2_qualified_paths.rs` (add 2 new + `#[test]` cases). + +**Code (advisory):** + +```rust +#[test] +fn qualified_external_crate_stays_unresolved() { + let (_dir, mut tethys) = workspace_with_files(&[ + ("Cargo.toml", /* crate_a single-package */), + ("src/lib.rs", r" +mod std_helper; +pub fn entry() { + let _ = std::collections::HashMap::::new(); + let _ = std_helper::do_local(); +} +"), + ("src/std_helper.rs", "pub fn do_local() -> i32 { 1 }"), + ]); + tethys.index().expect("index"); + let conn = open_db(&tethys); + + // External-crate prefix MUST NOT resolve. + let std_resolved: i64 = conn.query_row( + "SELECT COUNT(*) FROM refs r JOIN files f ON f.id = r.file_id + WHERE f.path = 'src/lib.rs' + AND r.symbol_id IS NOT NULL + AND (r.reference_name LIKE 'std::%' OR EXISTS ( + SELECT 1 FROM symbols s WHERE s.id = r.symbol_id + AND s.name IN ('HashMap','collections')))", + params![], |r| r.get(0), + ).expect("count"); + assert_eq!(std_resolved, 0, + "std::collections::HashMap must stay unresolved"); + + // Local lookalike submodule still resolves (sanity). + let local_resolved: i64 = conn.query_row( + "SELECT COUNT(*) FROM refs r JOIN files f ON f.id=r.file_id + JOIN symbols s ON s.id=r.symbol_id + WHERE f.path='src/lib.rs' AND s.name='do_local'", + params![], |r| r.get(0), + ).expect("count"); + assert!(local_resolved >= 1, "std_helper::do_local must resolve locally"); +} + +#[test] +fn qualified_crate_prefix_resolves() { + let (_dir, mut tethys) = workspace_with_files(&[ + ("Cargo.toml", /* crate_a single-package */), + ("src/lib.rs", r" +mod sub; +pub fn entry() { let _: crate::sub::Thing; } +"), + ("src/sub.rs", "pub struct Thing;"), + ]); + tethys.index().expect("index"); + let conn = open_db(&tethys); + let resolved_to_target: i64 = conn.query_row( + "SELECT COUNT(*) FROM refs r JOIN files f ON f.id=r.file_id + JOIN symbols s ON s.id=r.symbol_id + WHERE f.path='src/lib.rs' AND s.name='Thing'", + params![], |r| r.get(0), + ).expect("count"); + assert!(resolved_to_target >= 1, + "crate::sub::Thing must resolve to Thing in sub.rs"); +} +``` + +**Verification:** +- [ ] Both new tests pass with the slice-1 mechanism in place. +- [ ] All existing tests still pass. + +--- + +## Slice 3: Workspace verification — claims 7 + 8 + wall-budget check. + +**Claim:** The PR introduces no regression in: +- Cross-crate file_deps phantom rate (rivets-3d0s; must stay 0.00%). +- rivets-0gom ambiguity violations (must not increase). +- Indexing wall time (must stay within 1.5× pre-fix baseline). + +**Oracle:** +- Phantom-rate: `.rivets-ycaq/probe_phantom_rate.py` (independent SQL + classification of file_deps edges). +- Ambiguity: `.rivets-0gom/probe.py` Section 3 (independent ref-vs-symbol + cross-crate classification). +- Wall time: `hyperfine` against `cargo run --bin tethys -- index` with + warmup. + +**Stress fixture:** The rivets workspace itself. This is the production +input the design's measurement-based claims target. No synthetic +fixture; we re-index real code. + +**Loop budget:** N/A — verification slice, no new loops introduced. + +**Wall budget:** +- Indexing post-fix ≤ 1.5× pre-fix baseline (measured fresh in this + slice to set a real number; pre-fix snapshot is checked into + `.rivets-044i/wall-time-baseline.txt`). + +**Files:** +- `.rivets-044i/wall-time-baseline.txt` (new — pre-fix indexing time + from hyperfine). +- `.rivets-044i/wall-time-post.txt` (new — post-fix). +- `.rivets-044i/phantom-rate-post.txt` (new — output of probe). +- `.rivets-044i/ambiguity-post.txt` (new — output of 0gom Section 3 + probe). +- **No production code change in this slice.** + +**Code (advisory):** Shell-script-style verification steps: + +```sh +# Pre-slice-1: baseline (must be captured BEFORE slice 1 lands). +hyperfine --warmup 1 --runs 3 'cargo run --release --bin tethys -- index' \ + --export-markdown .rivets-044i/wall-time-baseline.txt + +# Post-slice-1+2: re-measure. +hyperfine --warmup 1 --runs 3 'cargo run --release --bin tethys -- index' \ + --export-markdown .rivets-044i/wall-time-post.txt + +# Phantom-rate (regression fence for claim 7). +python .rivets-ycaq/probe_phantom_rate.py > .rivets-044i/phantom-rate-post.txt +grep '0.00' .rivets-044i/phantom-rate-post.txt # must show 0.00% + +# Ambiguity (regression fence for claim 8). +python .rivets-0gom/probe.py > .rivets-044i/ambiguity-post.txt +# Section 3 violation count must be ≤ baseline (read from .rivets-0gom/measurement-* if extant). +``` + +**Verification:** +- [ ] `phantom-rate-post.txt` shows cross-crate phantom rate = 0.00%. +- [ ] `ambiguity-post.txt` Section 3 violation count ≤ baseline. +- [ ] `wall-time-post.txt` mean ≤ 1.5× `wall-time-baseline.txt` mean. +- [ ] CI regression fences pass: + `cargo nextest run -p tethys --test file_deps_corroboration --test resolver_routing`. + +--- + +## Plan self-review + +### List 1: every loop, its complexity statement, and budget verdict + +| Loop | Complexity | Production scale | Verdict | +|------|-----------|------------------|---------| +| `qualified_module_fallback`'s `for split in (1..segments.len()).rev()` | O(S) per ref where S ≤ 5 typical | Hundreds to low thousands of refs reach this fallback on rivets | within ops cap; syscall cap exceeded ~10× but OS-cached | + +No other new loops. Existing loops (`for (file_id, refs)`, `for ref_ in +refs`) unchanged by this PR. + +### List 2: every fixture, the bug class it defeats + +| Slice | Fixture | Plausible bug it defeats | +|-------|---------|--------------------------| +| 1 | `probe_044i_qualified_ref_from_import_less_file` (inherited) | "qualified ref from import-less file misses fallback" | +| 1 | `probe_044i_workspace_crate_prefix_from_import_less_file` (inherited) | "workspace-crate-prefix path bypasses qualified lookup" | +| 1 | `submodule_shadows_workspace_crate` (new) | "interpretation-precedence inverted: extern crate found before submodule" | +| 2 | `qualified_external_crate_stays_unresolved` | "implicit-crate prepend phantoms an external-crate ref" | +| 2 | `qualified_crate_prefix_resolves` | "implicit-crate retry shadows the explicit `crate::` path" | +| 3 | rivets workspace as a whole | "fix introduces phantom file_deps or 0gom-class ambiguity" | + +No happy-path-only fixtures. + +### List 3: every doc-comment precondition + +| Function | Precondition | Class | Enforcement | +|----------|--------------|-------|-------------| +| `qualified_module_fallback` | "Activates only after import-based paths and `fallback_symbol_search` return None." | Sanity hint (caller contract — fires from one site only). | No assertion needed; single call site in `try_resolve_reference`. | +| `qualified_module_fallback` | Returns `Ok(None)` for unqualified names. | Load-bearing for correctness — wrong output if violated would be a phantom resolution. | Runtime check: `if segments.len() < 2 { return Ok(None); }`. Survives release builds. | +| `qualified_module_fallback` | Returns `Ok(None)` if `current_file_path` is `None`. | Load-bearing — `resolve_module_path` requires a current file path. | Runtime check: `let Some(current_file) = current_file_path else { return Ok(None); }`. | + +### List 4: every write target + +| Write | Stream | Class | +|-------|--------|-------| +| `trace!(... "Resolved reference via qualified module fallback")` | stderr (tracing default) | Diagnostic | +| `db.resolve_reference(...)` | SQLite refs table | Data (intended persistent output) | + +No `println!`/`eprintln!` introduced. + +### List 5: tracker references + +| Reference | Tracker ID | Verified | +|-----------|------------|----------| +| Perf-cache deferral for `qualified_module_fallback` | `rivets-bjdn` | Open, P4 (verified 2026-05-18) | +| C# Pass-2 namespace gap | `rivets-jwf9` | Open, P3 (verified 2026-05-18) | +| Phantom-rate regression fence | `crates/tethys/tests/file_deps_corroboration.rs::k_hybrid_drops_cross_crate_call_without_import_corroboration` | Present (verified 2026-05-18) | +| Ambiguity regression fence | `crates/tethys/tests/resolver_routing.rs::fallback_routes_unqualified_ref_to_same_crate_not_cross_crate` | Present (verified 2026-05-18) | +| Parent issue | `rivets-044i` | Open, P3 (verified 2026-05-18) | + +No un-tracked deferrals. diff --git a/.rivets-044i/post-ambiguity.txt b/.rivets-044i/post-ambiguity.txt new file mode 100644 index 0000000..d2ef4cd --- /dev/null +++ b/.rivets-044i/post-ambiguity.txt @@ -0,0 +1,19 @@ +total file_deps rows: 347 + +=== Section 1: CROSS-CRATE EDGES (claims 3, 4) === +distinct cross-crate (from, to) pairs: 9 + +FROM TO EDGES +rivets-mcp rivets 7 +rivets rivets-jsonl 2 + +=== Section 2: INTRA-CRATE EDGES (claim 5: unchanged before/after fix) === +CRATE EDGES +tethys 221 +rivets 79 +rivets-jsonl 15 +rivets-mcp 15 + +=== Section 3: AMBIGUITY CHECK (claim 6: should-have-been-None cases) === +refs resolved across crates: 330 +ambiguity violations (claim 6): 0 diff --git a/.rivets-044i/post-phantom-rate.txt b/.rivets-044i/post-phantom-rate.txt new file mode 100644 index 0000000..eb5b82f --- /dev/null +++ b/.rivets-044i/post-phantom-rate.txt @@ -0,0 +1,19 @@ +====================================================================== +ycaq acceptance #6 measurement (post-PR-67 K-hybrid) +====================================================================== +total file_deps rows: 347 + intra-crate: 330 + cross-crate (both known): 9 + orphan (unknown crate): 8 + +=== Cross-crate corroboration (the ycaq #6 metric) === + cross-crate edges: 9 + corroborated by import: 9 + PHANTOM (no import): 0 + phantom rate: 0.00% (target: < 1%) + -> ycaq #6 PASS + +=== FORBIDDEN-pair check (Cargo dep-graph violations) === + FORBIDDEN-pair edges: 0 (rivets-3d0s threshold: <= 5) + of those, corroborated: 0 (should be 0 in a buildable workspace) + diff --git a/.rivets-044i/probe.rs b/.rivets-044i/probe.rs new file mode 100644 index 0000000..95ee652 --- /dev/null +++ b/.rivets-044i/probe.rs @@ -0,0 +1,237 @@ +//! Probe for rivets-044i (prove-it-prototype). +//! +//! Question: For an import-less `src/lib.rs` containing +//! `mod helper; pub fn entry() { helper::do_thing_q(); }`, with `do_thing_q` +//! defined in `src/helper.rs`, does Tethys resolve the `helper::do_thing_q` +//! reference today (pre-fix)? +//! +//! Oracle: source inspection. There is exactly one definition of +//! `do_thing_q` in the workspace, exactly one call site, both files are +//! indexed, and they live in the same crate. Therefore the ref MUST resolve +//! if the resolver is correct. +//! +//! Expected probe result (pre-fix): ref stays unresolved +//! (`refs.symbol_id IS NULL` for the row at `src/lib.rs:4`). +//! +//! This test is meant to FAIL pre-fix; after the rivets-044i fix lands it +//! will pass and serve as the regression fence. + +use rusqlite::params; + +mod common; + +use common::{open_db, workspace_with_files}; + +#[test] +fn probe_044i_qualified_ref_from_import_less_file() { + let (_dir, mut tethys) = workspace_with_files(&[ + ( + "Cargo.toml", + r#" +[package] +name = "crate_a" +version = "0.1.0" +edition = "2021" +"#, + ), + ( + "src/lib.rs", + r" +mod helper; + +pub fn entry() { + helper::do_thing_q(); +} +", + ), + ( + "src/helper.rs", + r" +pub fn do_thing_q() {} +", + ), + ]); + + tethys.index().expect("index should succeed"); + + let conn = open_db(&tethys); + + let total_refs: i64 = conn + .query_row( + "SELECT COUNT(*) FROM refs r JOIN files f ON f.id = r.file_id + WHERE f.path = 'src/lib.rs'", + params![], + |row| row.get(0), + ) + .expect("count refs"); + + let resolved_refs: i64 = conn + .query_row( + "SELECT COUNT(*) FROM refs r JOIN files f ON f.id = r.file_id + WHERE f.path = 'src/lib.rs' AND r.symbol_id IS NOT NULL", + params![], + |row| row.get(0), + ) + .expect("count resolved refs"); + + let resolved_to_target: i64 = conn + .query_row( + "SELECT COUNT(*) + FROM refs r + JOIN files f ON f.id = r.file_id + JOIN symbols s ON s.id = r.symbol_id + WHERE f.path = 'src/lib.rs' AND s.name = 'do_thing_q'", + params![], + |row| row.get(0), + ) + .expect("count refs resolved to do_thing_q"); + + let definition_exists: i64 = conn + .query_row( + "SELECT COUNT(*) FROM symbols s JOIN files f ON f.id = s.file_id + WHERE s.name = 'do_thing_q' AND f.path = 'src/helper.rs'", + params![], + |row| row.get(0), + ) + .expect("count definitions"); + + eprintln!( + "PROBE 044i state: total_refs={total_refs}, resolved_refs={resolved_refs}, \ + resolved_to_target={resolved_to_target}, definition_exists={definition_exists}" + ); + + // Sanity precondition from oracle: the definition is indexed exactly once. + assert_eq!( + definition_exists, 1, + "oracle precondition: do_thing_q must be indexed in helper.rs" + ); + + // The bug claim: the qualified ref does NOT resolve to the target today. + // Pre-fix this will hold (probe agrees with bug); post-fix this flips + // (probe demonstrates fix is effective). + assert!( + resolved_to_target >= 1, + "POST-FIX expectation: qualified call `helper::do_thing_q()` from \ + import-less src/lib.rs must resolve to its definition in src/helper.rs. \ + Pre-fix this assert FAILS, demonstrating rivets-044i." + ); +} + +/// Probe shape #2: workspace-crate-prefix call from an import-less integration test. +/// +/// Layout: workspace with two members. `crate_a/src/lib.rs` defines +/// `pub struct Widget; impl Widget { pub fn new() -> Self { Widget } }`. +/// `crate_b/tests/it.rs` is import-less and calls `crate_a::Widget::new()`. +/// +/// Oracle: there is exactly one `Widget::new` symbol in the workspace. +/// The ref's first segment is the workspace-crate name `crate_a`, which +/// `resolver::resolve_module_path` already handles (resolver.rs:45-63). +/// Therefore a correct resolver should link the ref to `Widget::new` in +/// `crate_a/src/lib.rs`. +#[test] +fn probe_044i_workspace_crate_prefix_from_import_less_file() { + let (_dir, mut tethys) = workspace_with_files(&[ + ( + "Cargo.toml", + r#" +[workspace] +members = ["crate_a", "crate_b"] +resolver = "2" +"#, + ), + ( + "crate_a/Cargo.toml", + r#" +[package] +name = "crate_a" +version = "0.1.0" +edition = "2021" +"#, + ), + ( + "crate_a/src/lib.rs", + r" +pub struct Widget; + +impl Widget { + pub fn make_widget_044i() -> Self { + Widget + } +} +", + ), + ( + "crate_b/Cargo.toml", + r#" +[package] +name = "crate_b" +version = "0.1.0" +edition = "2021" + +[dependencies] +crate_a = { path = "../crate_a" } +"#, + ), + ( + "crate_b/tests/it.rs", + r" +#[test] +fn smoke() { + let _ = crate_a::Widget::make_widget_044i(); +} +", + ), + ]); + + tethys.index().expect("index should succeed"); + + let conn = open_db(&tethys); + + let resolved_to_target: i64 = conn + .query_row( + "SELECT COUNT(*) + FROM refs r + JOIN files f ON f.id = r.file_id + JOIN symbols s ON s.id = r.symbol_id + WHERE f.path = 'crate_b/tests/it.rs' AND s.name = 'make_widget_044i'", + params![], + |row| row.get(0), + ) + .expect("count refs resolved to make_widget_044i"); + + let definition_exists: i64 = conn + .query_row( + "SELECT COUNT(*) FROM symbols s JOIN files f ON f.id = s.file_id + WHERE s.name = 'make_widget_044i' AND f.path = 'crate_a/src/lib.rs'", + params![], + |row| row.get(0), + ) + .expect("count definitions"); + + let unresolved_refs_in_test: i64 = conn + .query_row( + "SELECT COUNT(*) FROM refs r JOIN files f ON f.id = r.file_id + WHERE f.path = 'crate_b/tests/it.rs' AND r.symbol_id IS NULL + AND r.reference_name LIKE '%make_widget_044i%'", + params![], + |row| row.get(0), + ) + .expect("count unresolved refs"); + + eprintln!( + "PROBE 044i shape #2 state: resolved_to_target={resolved_to_target}, \ + definition_exists={definition_exists}, unresolved_in_test={unresolved_refs_in_test}" + ); + + assert_eq!( + definition_exists, 1, + "oracle precondition: make_widget_044i must be indexed in crate_a/src/lib.rs" + ); + + assert!( + resolved_to_target >= 1, + "POST-FIX expectation: workspace-crate-prefix call \ + `crate_a::Widget::make_widget_044i()` from import-less crate_b/tests/it.rs \ + must resolve to its definition in crate_a/src/lib.rs." + ); +} diff --git a/.rivets-044i/related-issues.md b/.rivets-044i/related-issues.md new file mode 100644 index 0000000..ffdadd6 --- /dev/null +++ b/.rivets-044i/related-issues.md @@ -0,0 +1,32 @@ +# Related issues (5-min tracker scan) + +Scan performed 2026-05-18 via `rivets list` against keywords "qualified", +"resolve", "import-less", "dn35", "ycaq". + +## Direct ancestors / context + +- **rivets-dn35** (closed PR #69): removed the `imports.is_empty()` + short-circuit in `resolve_refs_for_file`. 044i was filed at dn35's close as + the residual gap. The `pass2_no_imports.rs` regression fence covers the + unqualified-fallback shape; the qualified path is what 044i extends. +- **rivets-ycaq** (parent): resolver-correctness containment epic. 044i is + the named follow-up coverage gap. +- **rivets-0gom** (closed): un-ambiguation drift. The risk callout in 044i's + design notes flags this — any new resolution path could re-introduce + phantom resolutions analogous to 0gom. Mitigation: K-hybrid file_deps + filter (PR #67, rivets-3d0s). +- **rivets-3d0s** (closed): K-hybrid file_deps phantom-rate floor. AC #3 + requires it stay at 0.00% — we already have a probe at + `.rivets-ycaq/probe_phantom_rate.py` (per memory note). + +## Adjacent / not blocking + +- **rivets-6aoc / rivets-34tv**: hardcoded `src/` join bugs in resolver + paths. Not blocking 044i (different file paths) but worth noting in case + the fix touches `compute_dependencies*`. +- **rivets-714v**: `--lsp` multi-crate path bug. 044i is Pass-2 only; LSP + pass is separate. + +## Conclusion + +No prior art for the qualified-path fix itself. 044i is the canonical issue. diff --git a/.rivets-044i/verification.md b/.rivets-044i/verification.md new file mode 100644 index 0000000..c515a3a --- /dev/null +++ b/.rivets-044i/verification.md @@ -0,0 +1,73 @@ +# rivets-044i slice 3 verification + +Post-fix measurements on the rivets workspace. + +## Indexing wall time + +| metric | pre-fix | post-fix | delta | +|---|---|---|---| +| files | 123 | 123 | – | +| symbols | 2647 | 2651 | +4 (new test file symbols) | +| references | 22190 | 22263 | +73 (new test file refs) | +| wall time | 23.25s | 20.81s | **−10%** | +| `time real` | 23.394s | 20.939s | **−10%** | + +Budget: ≤ 1.5× baseline. **PASS** — post-fix is actually faster (within +noise). The new path is gated behind every existing fallback, so it +adds work only on the refs that would have stayed unresolved anyway. + +## Phantom rate (rivets-3d0s regression fence, claim 7) + +| metric | pre-fix | post-fix | +|---|---|---| +| cross-crate edges | 8 | 9 | +| corroborated | 8 | 9 | +| phantom edges | 0 | **0** | +| phantom rate | 0.00% | **0.00%** | +| FORBIDDEN-pair edges | 0 | 0 | + +**PASS.** The one extra cross-crate edge is corroborated by an import, +consistent with the K-hybrid filter's behavior. + +CI fence: `cargo nextest run -p tethys --test file_deps_corroboration` +all 2 tests pass. + +## 0gom Section 3 ambiguity (claim 8) + +| metric | pre-fix | post-fix | +|---|---|---| +| refs resolved across crates | 326 | 330 | +| ambiguity violations | 0 | **0** | + +**PASS.** Four new cross-crate resolutions, zero new ambiguity violations. + +CI fence: `cargo nextest run -p tethys --test resolver_routing` all 4 tests pass. + +## Resolve coverage delta (informational) + +| metric | pre-fix | post-fix | delta | +|---|---|---|---| +| total refs | 22190 | 22263 | +73 (new test refs added in slices 1+2) | +| resolved | 6756 | 6837 | **+81** | +| unresolved total | 15434 | 15426 | −8 | +| unresolved qualified | 1558 | 1493 | **−65** | + +So qualified-ref resolution improved by 65 refs on the rivets workspace +itself (+ several refs in the new test files that also got resolved +through the new path). The +81 resolved count exceeds the −65 +unresolved-qualified delta because the new test files added some refs +that resolved purely via Pass 1 (same-file). + +## Final integration + +All probes, falsifiers, and regression fences pass against the +post-fix binary: + +- `pass2_qualified_paths.rs` (5 tests, all pass — claims 1, 2, 3, 4, 5) +- `pass2_no_imports.rs` (1 test, passes — claim 6) +- `file_deps_corroboration.rs` (2 tests, pass — claim 7) +- `resolver_routing.rs` (4 tests, pass — claim 8) +- Full `cargo nextest run -p tethys` (631 tests, all pass) +- Phantom-rate probe (claim 7 oracle) — 0.00% +- 0gom Section 3 probe (claim 8 oracle) — 0 violations +- Indexing wall time (claim 1 budget) — −10% vs baseline diff --git a/.rivets-044i/what-i-learned.md b/.rivets-044i/what-i-learned.md new file mode 100644 index 0000000..b246ceb --- /dev/null +++ b/.rivets-044i/what-i-learned.md @@ -0,0 +1,27 @@ +# What I learned (one sentence) + +The dn35 fix is necessary but not sufficient: `try_resolve_reference`'s +fallback `get_symbol_by_qualified_name` does a literal string-equality match +against `symbols.qualified_name`, but the database stores qualified names +**module-stripped** (`indexing.rs:627-630`: free fns store `qualified_name = +name`; methods store `parent_name::name` only) — so any ref whose source text +carries a module or workspace-crate prefix systematically misses the fallback +*regardless* of whether the target symbol exists in the same workspace. + +## Why this matters for the design + +The fix is not "loosen the lookup" — that would re-introduce ambiguity drift +(rivets-0gom). It has to be "translate the ref's path to a target file via +`resolver::resolve_module_path`, then look up the *unqualified* tail in that +specific file." Option (a) in the issue description is the right shape, +because the workspace-crate-prefix arm of `resolve_module_path` already +exists; we just don't currently dispatch to it from `try_resolve_reference`'s +qualified branch unless the first segment matches an explicit import. + +## Bonus learning (shape #2) + +`make_widget_044i` is an `impl` method, so its stored `qualified_name` is +`Widget::make_widget_044i` (parent_name = `Widget`). The lookup key is +`crate_a::Widget::make_widget_044i`. The mismatch is the crate-prefix +segment, not the method-prefix segment. So even methods (which DO get a +prefix) miss this fallback when called with a crate prefix. From 7ce89437af37662eac81b2ce551a75c8df322ce3 Mon Sep 17 00:00:00 2001 From: Daryl Walleck Date: Mon, 18 May 2026 19:25:51 -0500 Subject: [PATCH 4/7] test(tethys): address PR #74 review-feedback round 1 (rivets-044i) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tighten regression fences and apply doc/log polish per the assessing- review-feedback pass on PR #74. Decision log committed at `.rivets-044i/review-decisions-round-1.md` (7 accept, 4 modify, 4 reject of 15 findings). Test changes: - I1 `qualified_external_crate_stays_unresolved`: rewrite the negative assertion. The prior filter `reference_name LIKE 'std::%' AND symbol_id IS NOT NULL` is structurally vacuous — `resolve_reference` atomically clears `reference_name` to NULL when setting `symbol_id`, so both predicates can never both hold. New filter joins through the symbols table targeting `s.name IN ('HashMap', 'new')`, which would match if `std::*` had phantom-resolved. - I4 `qualified_crate_prefix_resolves`: lay a phantom-resolution trap at `src/crate/sub_044i.rs` (literal directory name). Change ref shape from a type-position path (`let _: ...`) to a free-fn call so tree- sitter preserves the qualified `reference_name` and the ref actually exercises `qualified_module_fallback` — the prior type-position form was resolving via the unqualified workspace-wide search, bypassing the fallback entirely. With the `prefix[0]==\"crate\"` gate working, the ref binds to `src/sub_044i.rs`; with the gate broken (dropping `\"crate\"` from the matches), it phantom-resolves to the trap. The two new assertions distinguish those cases. TDD-verified. - I2 new test `longest_prefix_wins_over_shorter`: 3-segment ref where both `outer::inner` (deeper) and `outer` (shallower) resolve to files containing tail-matching symbols. The deeper path resolves to a free fn; the shallower path resolves to a method on an `impl inner` whose stored `qualified_name = inner::deep_thing_044i`. Fences the longest-first loop direction. TDD-verified by inverting `.rev()` to forward iteration — test fails. - I3 new test `prefix_resolves_but_tail_missing_stays_unresolved`: `helper.rs` exists with one fn, but the caller references a different fn. Asserts the ref remains unresolved (prefix resolved + tail miss → no phantom bind). - S3 partial: new test `self_and_super_paths_resolve_via_as_written` pins that the `matches!` gate set in `qualified_module_fallback` permits both `self::*` and `super::*` to fire the as-written interpretation. Fixture lays files where tethys's filesystem-walk semantics expect them (`super` from `src/parent/child.rs` reaches `src/cousin.rs`, not `src/parent/cousin.rs`). - S4: drop `eprintln!(\"PROBE 044i state: ...\")` probe-era diagnostics from tests 1 and 2; rephrase \"Pre-fix this will hold\" comments as steady-state regression-fence wording. - S10: replace `indexing.rs:627-630` line-anchor in module doc with the symbol-relative `indexing.rs::store_references` free-fn arm. Production code changes (`crates/tethys/src/resolve.rs`): - I5: extend `qualified_module_fallback` rustdoc with an \"Acceptable ambiguity\" section documenting the bounded same-named file + same- named tail collision class, noting it's the same shape as `fallback_symbol_search`'s same-crate prefix arm per design-v3 C5/C6. - S1: emit `trace!(ref_name, segments, ...)` on the \"all prefixes exhausted\" path before the final `Ok(None)` for forensic replay. - S5: drop redundant `// Interpretation A:` / `// Interpretation B:` inline labels — the rustdoc already enumerates them. - S9: tighten the two `// Load-bearing for correctness:` comments to preserve the WHY (synthetic-ref None propagation, defense-in-depth against future hand-rolled callers) and drop the WHAT-restatements. Rejected findings (rationale in the decision log): - S2 `relative_path` warn-spam — not observable in practice; outside- workspace resolved paths require `resolve_module_path` to return a path it can't currently construct from in-workspace prefixes. - S6 `(*s).to_string()` — clippy doesn't flag it. - S7 closure for two Vec blocks — premature abstraction. - S8 hoist `Option<&Path>` — signature change without simplification. Verification: - `cargo nextest run -p tethys` → 636/636 pass (+4 new fences). - `cargo clippy --all-targets --all-features -- -D warnings` → clean. - TDD inversions: shortest-first loop fails I2; dropped-\"crate\" gate fails I4. Both inversions reverted. --- .rivets-044i/review-decisions-round-1.md | 58 +++ crates/tethys/src/resolve.rs | 31 +- crates/tethys/tests/pass2_qualified_paths.rs | 457 +++++++++++++++++-- 3 files changed, 489 insertions(+), 57 deletions(-) create mode 100644 .rivets-044i/review-decisions-round-1.md diff --git a/.rivets-044i/review-decisions-round-1.md b/.rivets-044i/review-decisions-round-1.md new file mode 100644 index 0000000..34f56d3 --- /dev/null +++ b/.rivets-044i/review-decisions-round-1.md @@ -0,0 +1,58 @@ +# Review-feedback decisions — round 1 + +Source: `/pr-review-toolkit:review-pr 74` (all 6 agents) — code-reviewer, comment-analyzer, +test-analyzer, silent-failure-hunter, type-design-analyzer, code-simplifier. + +Verified against current branch `feat/rivets-044i-qualified-paths` (HEAD `e559eee`). + +## Important Issues + +| # | Finding (one line) | Reviewer | Category | Verified? | Decision | Note | +|---|---|---|---|---|---|---| +| I1 | Test query filters `reference_name LIKE 'std::%'` AND `symbol_id IS NOT NULL` — `resolve_reference` clears `reference_name` to NULL atomically, so the two predicates can never both hold. Negative assertion is vacuous. | code-reviewer | Bug | YES — `db/references.rs:156-159` UPDATE sets `symbol_id = ?2, reference_name = NULL` in one statement. The test at `pass2_qualified_paths.rs:399-408` would pass even if `std::*` got phantom-resolved. | **Accept** | Tighten the query to use `s.name IN ('HashMap','new')` + a `LEFT JOIN symbols` and assert no `std::`-shaped lookup phantom-resolved. The local-resolves sanity at L416-433 stays as-is. | +| I2 | Longest-prefix iteration (`(1..segments.len()).rev()`) is untested. No fixture has both `a::b::c` AND `a::b` resolvable; direction inversion would silently pass. | test-analyzer (rating 8) | Bug | YES — read all 5 fixtures: each has exactly one resolvable prefix length. Test 2 (`crate_a::Widget::make_widget_044i`) has 3 segments but only `crate_a` is a module (Widget is a struct). Test 5 (`crate::sub_044i::ThingFour`) has 3 segments but only `crate::sub_044i` resolves. Flipping the loop to `1..segments.len()` (shortest-first) does not fail any current test. | **Accept** | New test `longest_prefix_wins_over_shorter` — 3-segment ref where both `a::b` and `a::b::c` resolve to *different* files, assert resolution lands in the deeper file. | +| I3 | "Prefix resolves but tail doesn't exist" branch (line 329-335) is untested. | test-analyzer (rating 7) | Bug | YES — no fixture has `helper::nonexistent_fn()` or equivalent. | **Accept** | New small test `prefix_resolves_but_tail_missing_stays_unresolved` — `mod helper;` exists, helper.rs has a different fn, ref is `helper::missing()`. Assert ref `symbol_id IS NULL`. | +| I4 | `qualified_crate_prefix_resolves` (test 5) asserts only `resolved_to_target >= 1` — even if the `prefix[0]=="crate"` gate were broken, Interpretation B (as-written) would still resolve `crate::sub_044i::ThingFour`. The test cannot distinguish "gate working" from "gate broken but B saves us." | test-analyzer + code-reviewer | Bug | YES — traced the algorithm: with a broken gate, Interpretation A tries `crate::crate::sub_044i` which `resolve_module_path` returns None for; falls through to Interpretation B which succeeds. Existing assertion can't fail. | **Modify** | Reviewer suggested "negative assertion against double-`crate` artifact path." Better fix: assert `resolved_to_target == 1` (exact) and add a count check on resolution path — but the framing is tricky because the path provenance isn't stored. Simpler modification: add a sibling test `crate_prefix_skips_implicit_crate` that *would* break under a malformed gate by setting up a fixture where the doubled-crate prepend would phantom-resolve to a same-named symbol (e.g., a `crate/` directory with a colliding symbol). If unfeasible without contortion, ship with `== 1` exact-count and note the limitation in the docstring. | +| I5 | Phantom-resolution risk on Interpretation A: in a workspace with `helper.rs` at crate root AND `helper.rs` as a sibling module, Interpretation A always wins via `resolve_module_path`'s first match. Risk bounded to exact-name tail collision. Reviewer asked to document as "known acceptable" mirroring `fallback_symbol_search`. | silent-failure-hunter | Design (doc) | YES the risk exists. NO precedent in `fallback_symbol_search` — its rustdoc (resolve.rs:452-458) does NOT document its prefix collision risk either. The "mirroring precedent" framing is incorrect. | **Modify** | Document the bounded risk in `qualified_module_fallback`'s rustdoc as a known acceptable ambiguity (it's real), but drop the "matching fallback_symbol_search precedent" framing because no such precedent exists. The acceptable-ambiguity language is justified by design-v3 C5/C6 which the audit dir documents. | + +## Suggestions + +| # | Finding (one line) | Reviewer | Category | Verified? | Decision | Note | +|---|---|---|---|---|---|---| +| S1 | No `trace!` on `qualified_module_fallback`'s final `Ok(None)` — every other resolution arm logs on miss. | silent-failure-hunter | Polish | YES — confirmed at `resolve.rs:338`. Other miss paths at `resolve.rs:245-249` and `resolve.rs:441-446` do log. | **Accept** | Add `trace!(ref_name, segments = segments.len(), "qualified_module_fallback: no prefix split resolved")` before the final `Ok(None)`. Forensic debugging value, near-zero release cost. | +| S2 | `relative_path` emits a `warn!` when `resolved` is outside workspace; can fire 2N times per qualified ref. | silent-failure-hunter | Polish | PARTIAL — `lib.rs:220-224` does emit the `warn!`, but `resolve_module_path` for in-workspace prefixes returns paths inside the workspace by construction. Outside-workspace `resolved` is essentially impossible on this code path (external prefixes hit the `None` branch in `resolve_module_path` first). | **Reject (not deferred)** | Not observed in practice. The `warn!` exists for genuinely-misconfigured workspace roots, which is a wholly different operational scenario. Adding a gate would add noise without observable benefit. Rejection rationale stands; no tracker entry needed because the concern isn't real. | +| S3 | Untested defense-in-depth branches: `current_file_path = None` (L285), `segments.len() < 2` (L294), `self::*`/`super::*` paths. | test-analyzer | Bug (coverage) | YES — none of the three branches has direct test coverage. But: the first two are unreachable from the only call site (the `is_qualified` gate at L228 already enforces `contains("::")`, and `ctx.current_file_path` is always `Some` at the call site for normal Pass-2 input). The `self::*`/`super::*` path is the only one with realistic reachability. | **Modify (partial)** | Accept the `self::*`/`super::*` coverage (add to the prefix-resolves-tail-missing test or new mini-test). Reject the other two: testing unreachable defense-in-depth branches inverts the cost/value ratio and amounts to mocking the call site. The doc comments already explain why those guards exist. | +| S4 | Probe-era `eprintln!("PROBE 044i state: ...")` and "Pre-fix this will hold" comments in tests 1 and 2. | comment-analyzer + simplifier | Style | YES — `pass2_qualified_paths.rs:95-98, 106-108, 113, 215-218`. | **Accept** | Cosmetic but real noise on green runs. Remove `eprintln!`s; rewrite the "pre-fix this will hold" framing as steady-state regression-fence wording. | +| S5 | Redundant `// Interpretation A:` / `// Interpretation B:` inline labels restate the rustdoc bullets. | comment-analyzer | Style | YES — `resolve.rs:305, 318` exactly match rustdoc enumeration at L267, L270. | **Accept** | Drop both inline labels. Rustdoc already names them. | +| S6 | Unidiomatic `(*s).to_string()` at L309, L320; may trip `clippy::explicit_auto_deref`. | simplifier | Style | NO — verified `cargo clippy -p tethys --tests --all-features -- -D warnings -W clippy::explicit_auto_deref` produces no warnings. | **Reject** | Clippy doesn't flag it. The simplifier's qualifier ("may trip") was honest; it doesn't, so this is a stylistic preference without machine backing. Skip. | +| S7 | Two `Vec` allocation blocks could share a closure/helper. | simplifier | Design | YES — duplication is real. | **Reject** | Simplifier itself flagged this as "Borderline." Per CLAUDE.md: three similar lines beats premature abstraction. The two blocks differ in their composition (chain-once vs straight-collect) and live within one short loop — extracting would obscure, not clarify. | +| S8 | Hoist `Option<&Path>` check to the call site so the helper signature can be `&Path`. | type-design | Design | YES — the early-exit is a propagated `Option`. | **Reject** | The helper's `Option<&Path>` mirrors `ResolveContext::current_file_path` honestly. Hoisting moves a 3-line early-exit and a load-bearing rustdoc note to the call site without simplifying either site materially. Net wash. | +| S9 | Two `// Load-bearing for correctness:` comments lean toward restating WHAT. | comment-analyzer | Style | PARTIAL — comment at L283-284 IS mostly WHAT. Comment at L290-293 has both WHAT (`would loop zero times and return None`) and WHY (`defended in depth ... preserves that property on hand-rolled paths`). | **Modify** | Tighten both — first comment to name the synthetic-ref case explicitly; second comment to drop the "would loop zero times" sentence and keep only the defense-in-depth rationale. | +| S10 | Module-doc anchor `indexing.rs:627-630` is rot-prone. | comment-analyzer | Style | YES — `pass2_qualified_paths.rs:4` cites the line range. | **Accept** | Rephrase to symbol-relative: `indexing.rs::store_references` free-fn arm. Line-range citation rots on the next reformat. | + +## Summary of decisions + +- **Accept:** I1, I2, I3, S1, S4, S5, S10 (7 findings) +- **Modify:** I4, I5, S3 (partial), S9 (4 findings) +- **Reject:** S2, S6, S7, S8 (4 findings — none requires a tracker entry; rationale recorded above) + +15 findings reviewed. Distribution 7/4/4 (accept/modify/reject) sits in the healthy range — not "accept everything" theater. + +## Tracker discipline check + +Per `feedback_tracker_entries_for_deferrals` and the skill rule: every Reject(defer) or Modify(deferred work) names a tracker ID. **No findings are deferred to follow-up work** — every accept/modify above is in-scope for this branch's next commit. Rejections are rejections (concern not real or fix worse than the disease), not deferrals. Therefore no new rivets issues need to be filed. + +## Application plan + +1. Add `trace!` on final `Ok(None)` (S1) — `resolve.rs:338`. +2. Drop `// Interpretation A/B:` inline labels (S5) — `resolve.rs:305, 318`. +3. Tighten two `// Load-bearing for correctness:` comments (S9) — `resolve.rs:283-284, 290-293`. +4. Add bounded-ambiguity paragraph to `qualified_module_fallback` rustdoc (I5) — `resolve.rs:253-276`. +5. Remove `eprintln!` and rephrase pre-fix comments in tests 1, 2 (S4) — `pass2_qualified_paths.rs:95-98, 106-108, 113, 215-218`. +6. Rephrase module-doc anchor (S10) — `pass2_qualified_paths.rs:4`. +7. Rewrite test 4's negative assertion (I1) — `pass2_qualified_paths.rs:399-414`. +8. Strengthen test 5 (I4) — `pass2_qualified_paths.rs:488-492`. +9. New test `longest_prefix_wins_over_shorter` (I2). +10. New test `prefix_resolves_but_tail_missing_stays_unresolved`, with `self::*` / `super::*` coverage absorbed (I3 + S3 partial). + +Steps 1-6 are pure doc/comment/log edits and can land in one commit. Steps 7-10 are behavioral test changes and land in a second commit (TDD discipline: tests must fail in a meaningful way before being added — verify by temporarily inverting the relevant invariant in `qualified_module_fallback`). diff --git a/crates/tethys/src/resolve.rs b/crates/tethys/src/resolve.rs index ebaf9ad..6253c3a 100644 --- a/crates/tethys/src/resolve.rs +++ b/crates/tethys/src/resolve.rs @@ -274,23 +274,35 @@ impl Tethys { /// Returns `Ok(None)` for: unqualified names, `current_file_path = None`, /// external-crate prefixes that `resolve_module_path` can't translate, /// and any ref whose tail doesn't match a symbol in the resolved file. + /// + /// # Acceptable ambiguity + /// + /// When a workspace contains both `/.rs` AND a sibling + /// module file along the same prefix, Interpretation A (the implicit-crate + /// retry) wins because [`resolve_module_path`] returns the first on-disk + /// match. The resolved tail must additionally exist in that file as a + /// symbol with the exact `qualified_name` produced by the indexer, so a + /// phantom resolution requires *both* a colliding file path *and* a + /// colliding tail symbol — the same bounded-ambiguity class as + /// `fallback_symbol_search`'s same-crate prefix arm, and accepted under + /// design-v3 C5/C6. fn qualified_module_fallback( &self, ref_name: &str, current_file_path: Option<&Path>, src_root: &Path, ) -> Result> { - // Load-bearing for correctness: every caller that reaches this method - // either provides a real path or the function declines safely. + // `current_file_path` is Option because Pass-2 sometimes lacks a path + // for synthetic refs; silent decline keeps resolution best-effort + // rather than panicking on a propagated None. let Some(current_file) = current_file_path else { return Ok(None); }; let segments: Vec<&str> = ref_name.split("::").collect(); - // Load-bearing for correctness: a single-segment "qualified" ref - // (impossible per the `is_qualified` gate at the call site, but defended - // in depth) would loop zero times and return None. Explicit early-exit - // preserves that property on hand-rolled paths through this method. + // Defense-in-depth: the `is_qualified` gate at the only current call + // site guarantees `segments.len() >= 2`. Hand-rolled future callers + // get a safe decline instead of an empty-loop silent success. if segments.len() < 2 { return Ok(None); } @@ -302,7 +314,6 @@ impl Tethys { let mut file_id = None; - // Interpretation A: implicit-crate prepend. if !matches!(prefix[0], "crate" | "self" | "super") { let mut with_crate: Vec = Vec::with_capacity(prefix.len() + 1); with_crate.push("crate".to_string()); @@ -315,7 +326,6 @@ impl Tethys { } } - // Interpretation B: as-written. if file_id.is_none() { let as_owned: Vec = prefix.iter().map(|s| (*s).to_string()).collect(); if let Some(resolved) = @@ -335,6 +345,11 @@ impl Tethys { } } + trace!( + ref_name = %ref_name, + segments = segments.len(), + "qualified_module_fallback: no prefix split resolved" + ); Ok(None) } diff --git a/crates/tethys/tests/pass2_qualified_paths.rs b/crates/tethys/tests/pass2_qualified_paths.rs index 27542bf..b3d808d 100644 --- a/crates/tethys/tests/pass2_qualified_paths.rs +++ b/crates/tethys/tests/pass2_qualified_paths.rs @@ -1,12 +1,13 @@ //! Regression fence for rivets-044i: qualified refs from import-less files. //! -//! Stored `symbols.qualified_name` is module-stripped (free fns: `name`; -//! methods: `parent_name::name` — see `indexing.rs:627-630`), so the literal -//! `get_symbol_by_qualified_name` lookup in Pass 2's `fallback_symbol_search` -//! cannot match a ref like `helper::do_thing_q` whose text carries a module -//! prefix. The fix in `resolve.rs::qualified_module_fallback` interprets the -//! prefix as a module path via `resolver::resolve_module_path`, then looks up -//! the tail in the resulting file. +//! Stored `symbols.qualified_name` is module-stripped — the free-fn arm of +//! `indexing.rs::store_references` writes `name` only; the method arm writes +//! `parent_name::name`. So the literal `get_symbol_by_qualified_name` lookup +//! in Pass 2's `fallback_symbol_search` cannot match a ref like +//! `helper::do_thing_q` whose text carries a module prefix. The fix in +//! `resolve.rs::qualified_module_fallback` interprets the prefix as a module +//! path via `resolver::resolve_module_path`, then looks up the tail in the +//! resulting file. //! //! These tests defend each input-shape branch of that fix. @@ -92,25 +93,22 @@ pub fn do_thing_q() {} ) .expect("count definitions"); - eprintln!( - "PROBE 044i state: total_refs={total_refs}, resolved_refs={resolved_refs}, \ - resolved_to_target={resolved_to_target}, definition_exists={definition_exists}" - ); - // Sanity precondition from oracle: the definition is indexed exactly once. assert_eq!( definition_exists, 1, "oracle precondition: do_thing_q must be indexed in helper.rs" ); + assert!( + total_refs >= 1, + "oracle precondition: at least one ref must be recorded in src/lib.rs \ + (got total_refs={total_refs}, resolved_refs={resolved_refs})" + ); - // The bug claim: the qualified ref does NOT resolve to the target today. - // Pre-fix this will hold (probe agrees with bug); post-fix this flips - // (probe demonstrates fix is effective). assert!( resolved_to_target >= 1, - "POST-FIX expectation: qualified call `helper::do_thing_q()` from \ - import-less src/lib.rs must resolve to its definition in src/helper.rs. \ - Pre-fix this assert FAILS, demonstrating rivets-044i." + "qualified call `helper::do_thing_q()` from import-less src/lib.rs \ + must resolve to its definition in src/helper.rs (regression fence \ + for rivets-044i)." ); } @@ -212,21 +210,21 @@ fn smoke() { ) .expect("count unresolved refs"); - eprintln!( - "PROBE 044i shape #2 state: resolved_to_target={resolved_to_target}, \ - definition_exists={definition_exists}, unresolved_in_test={unresolved_refs_in_test}" - ); - assert_eq!( definition_exists, 1, "oracle precondition: make_widget_044i must be indexed in crate_a/src/lib.rs" ); + assert!( + unresolved_refs_in_test == 0 || resolved_to_target >= 1, + "either the ref resolves to make_widget_044i (got {resolved_to_target}) or \ + it remains unresolved without phantoms (got unresolved={unresolved_refs_in_test})" + ); assert!( resolved_to_target >= 1, - "POST-FIX expectation: workspace-crate-prefix call \ - `crate_a::Widget::make_widget_044i()` from import-less crate_b/tests/it.rs \ - must resolve to its definition in crate_a/src/lib.rs." + "workspace-crate-prefix call `crate_a::Widget::make_widget_044i()` from \ + import-less crate_b/tests/it.rs must resolve to its definition in \ + crate_a/src/lib.rs (regression fence for rivets-044i)." ); } @@ -392,25 +390,31 @@ pub fn do_local_044i() {} let conn = open_db(&tethys); - // Any ref whose text starts with `std::` and that the resolver linked - // to a symbol is a phantom resolution. The new fallback's implicit-crate - // and workspace-crate-prefix arms both return None for `std`, so the - // count must be zero. - let std_resolved: i64 = conn + // `resolve_reference` clears `reference_name` to NULL atomically with + // setting `symbol_id`, so filtering resolved rows by `reference_name LIKE + // 'std::%'` would be vacuous (the predicate is never true after + // resolution). Instead, target the tail-symbol names that a phantom + // resolution would have to bind to: any workspace symbol named `HashMap` + // or `new` reached from src/lib.rs would be a phantom for `std::*`. + // The fixture introduces no such workspace symbols, so the count must be + // zero — and if it isn't, we know the new fallback phantom-resolved. + let std_phantom_resolved: i64 = conn .query_row( - "SELECT COUNT(*) FROM refs r JOIN files f ON f.id = r.file_id + "SELECT COUNT(*) FROM refs r + JOIN files f ON f.id = r.file_id + JOIN symbols s ON s.id = r.symbol_id WHERE f.path = 'src/lib.rs' - AND r.reference_name LIKE 'std::%' - AND r.symbol_id IS NOT NULL", + AND s.name IN ('HashMap', 'new')", params![], |row| row.get(0), ) .expect("count"); assert_eq!( - std_resolved, 0, - "external-crate-prefixed ref `std::collections::HashMap` MUST stay \ - unresolved (no workspace symbol to bind to). Got std_resolved={std_resolved}" + std_phantom_resolved, 0, + "external-crate-prefixed ref `std::collections::HashMap::::new()` \ + MUST stay unresolved (no workspace symbol to bind to). Got \ + std_phantom_resolved={std_phantom_resolved}" ); // Sanity: the same-shape local call still resolves. If this regresses @@ -433,12 +437,31 @@ pub fn do_local_044i() {} ); } -/// Shape s-crate: explicit `crate::sub::Item` from an import-less file -/// resolves via the `crate::` arm of `resolve_module_path`. Plausible bug -/// class this defeats: the implicit-crate-prepend retry inadvertently -/// produces `crate::crate::sub::Item` and shadows the correct path. The -/// `qualified_module_fallback` code branches on `path[0] == "crate"` to +/// Shape s-crate: explicit `crate::sub::fn()` from an import-less file +/// resolves via the `crate::` arm of `resolve_module_path`. The plausible +/// bug class this defeats is the implicit-crate-prepend retry inadvertently +/// producing `crate::crate::sub::fn` and shadowing the correct path. The +/// `qualified_module_fallback` code branches on `prefix[0] == "crate"` to /// skip the implicit-prepend specifically to avoid this. +/// +/// The fixture lays a phantom-resolution trap to fence the gate: a literal +/// `src/crate/sub_044i.rs` (directory named "crate" on disk — possible at +/// the filesystem level even though `mod crate;` is not legal Rust) carries +/// a `do_crate_thing_044i` free fn with the same name as the real target in +/// `src/sub_044i.rs`. With the gate working, Interpretation A is skipped +/// and Interpretation B resolves `["crate","sub_044i"]` to the real file. +/// With the gate broken (e.g., dropping `"crate"` from the `matches!` set), +/// Interpretation A builds `["crate","crate","sub_044i"]`, which +/// `resolve_module_path` walks as a filesystem path and finds the trap +/// `src/crate/sub_044i.rs`. The tail then phantom-resolves to the trap. The +/// negative assertion below catches that path. +/// +/// The call shape `crate::sub_044i::do_crate_thing_044i()` (free-fn call) +/// is chosen so tree-sitter records the qualified `reference_name`. A +/// type-position path like `let _: crate::sub_044i::ThingFour;` is recorded +/// by the rust extractor as just `ThingFour` (the leaf), which would +/// resolve via the unqualified workspace-wide search instead — bypassing +/// `qualified_module_fallback` entirely and giving false confidence. #[test] fn qualified_crate_prefix_resolves() { let (_dir, mut tethys) = workspace_with_files(&[ @@ -457,14 +480,24 @@ edition = "2021" mod sub_044i; pub fn entry() { - let _: crate::sub_044i::ThingFour; + crate::sub_044i::do_crate_thing_044i(); } ", ), ( "src/sub_044i.rs", r" -pub struct ThingFour; +pub fn do_crate_thing_044i() {} +", + ), + // Phantom-resolution trap. Directory literally named "crate" is + // possible on disk; `qualified_module_fallback`'s implicit-crate + // retry would walk into this file under a broken gate. Same-named + // function ensures the tail lookup would succeed and bind here. + ( + "src/crate/sub_044i.rs", + r" +pub fn do_crate_thing_044i() {} ", ), ]); @@ -473,21 +506,347 @@ pub struct ThingFour; let conn = open_db(&tethys); - let resolved_to_target: i64 = conn + let resolved_to_real_target: i64 = conn + .query_row( + "SELECT COUNT(*) + FROM refs r + JOIN files f_caller ON f_caller.id = r.file_id + JOIN symbols s ON s.id = r.symbol_id + JOIN files f_target ON f_target.id = s.file_id + WHERE f_caller.path = 'src/lib.rs' + AND s.name = 'do_crate_thing_044i' + AND f_target.path = 'src/sub_044i.rs'", + params![], + |row| row.get(0), + ) + .expect("count"); + + let resolved_to_trap: i64 = conn + .query_row( + "SELECT COUNT(*) + FROM refs r + JOIN files f_caller ON f_caller.id = r.file_id + JOIN symbols s ON s.id = r.symbol_id + JOIN files f_target ON f_target.id = s.file_id + WHERE f_caller.path = 'src/lib.rs' + AND s.name = 'do_crate_thing_044i' + AND f_target.path = 'src/crate/sub_044i.rs'", + params![], + |row| row.get(0), + ) + .expect("count"); + + assert!( + resolved_to_real_target >= 1, + "ref `crate::sub_044i::do_crate_thing_044i()` MUST resolve via the \ + as-written `crate::` arm of resolve_module_path to the free fn in \ + src/sub_044i.rs. Got resolved_to_real_target={resolved_to_real_target}" + ); + + assert_eq!( + resolved_to_trap, 0, + "ref MUST NOT phantom-resolve to the free fn in the literal-`crate` \ + trap directory — the `prefix[0]==\"crate\"` gate in \ + qualified_module_fallback must skip the implicit-crate prepend. \ + Got resolved_to_trap={resolved_to_trap}" + ); +} + +/// Pin the longest-prefix-first iteration order in `qualified_module_fallback`. +/// +/// For ref `outer::inner::deep_thing_044i`, both `outer::inner` (deeper) and +/// `outer` (shallower) resolve to module files that contain a tail-matching +/// symbol — by design: +/// +/// - `src/outer/inner.rs::deep_thing_044i` (free fn, stored `qualified_name` +/// = `deep_thing_044i`). Reached by split=2 with tail `deep_thing_044i`. +/// - `src/outer.rs::inner::deep_thing_044i` (method on `impl inner`, stored +/// `qualified_name` = `inner::deep_thing_044i` per the method arm of +/// `indexing.rs::store_references`). Reached by split=1 with tail +/// `inner::deep_thing_044i`. +/// +/// With the working loop `(1..segments.len()).rev()` (longest first), split=2 +/// fires first, finds the free fn, and returns. The ref binds to the file at +/// `src/outer/inner.rs`. If the loop direction inverted to `1..segments.len()` +/// (shortest first), split=1 would phantom-resolve to the method on `inner` +/// in `src/outer.rs`. The two assertions distinguish those cases. +#[test] +fn longest_prefix_wins_over_shorter() { + let (_dir, mut tethys) = workspace_with_files(&[ + ( + "Cargo.toml", + r#" +[package] +name = "crate_a" +version = "0.1.0" +edition = "2021" +"#, + ), + ( + "src/lib.rs", + r" +mod outer; + +pub fn entry() { + outer::inner::deep_thing_044i(); +} +", + ), + // Shallow-prefix target: an `impl inner` whose method stores + // qualified_name = `inner::deep_thing_044i`. If the fallback's loop + // tried split=1 first, it would resolve here. + ( + "src/outer.rs", + r" +#[allow(non_camel_case_types)] +pub struct inner; + +impl inner { + pub fn deep_thing_044i() {} +} +", + ), + // Deep-prefix target: a free fn whose qualified_name is just + // `deep_thing_044i`. The fallback's longest-first loop must reach + // here first. + ( + "src/outer/inner.rs", + r" +pub fn deep_thing_044i() {} +", + ), + ]); + + tethys.index().expect("index should succeed"); + + let conn = open_db(&tethys); + + let resolved_to_deeper: i64 = conn + .query_row( + "SELECT COUNT(*) + FROM refs r + JOIN files f_caller ON f_caller.id = r.file_id + JOIN symbols s ON s.id = r.symbol_id + JOIN files f_target ON f_target.id = s.file_id + WHERE f_caller.path = 'src/lib.rs' + AND s.name = 'deep_thing_044i' + AND f_target.path = 'src/outer/inner.rs'", + params![], + |row| row.get(0), + ) + .expect("count"); + + let resolved_to_shallower: i64 = conn + .query_row( + "SELECT COUNT(*) + FROM refs r + JOIN files f_caller ON f_caller.id = r.file_id + JOIN symbols s ON s.id = r.symbol_id + JOIN files f_target ON f_target.id = s.file_id + WHERE f_caller.path = 'src/lib.rs' + AND s.name = 'deep_thing_044i' + AND f_target.path = 'src/outer.rs'", + params![], + |row| row.get(0), + ) + .expect("count"); + + assert!( + resolved_to_deeper >= 1, + "longest-prefix-first iteration must resolve `outer::inner::deep_thing_044i` \ + to the free fn in src/outer/inner.rs. Got resolved_to_deeper={resolved_to_deeper}" + ); + assert_eq!( + resolved_to_shallower, 0, + "loop direction is wrong: the shorter prefix `outer` resolved first and \ + phantom-resolved the ref to the method on `impl inner` in src/outer.rs. \ + Got resolved_to_shallower={resolved_to_shallower}" + ); +} + +/// Pin that a successfully-resolved prefix whose tail symbol does not exist +/// in the resolved file leaves the ref unresolved instead of phantom-binding +/// to something else. +/// +/// The fixture provides `mod helper;` with a single fn `other_thing_044i` — +/// but the caller references `helper::nonexistent_044i`. The fallback +/// resolves the prefix to `src/helper.rs`, calls +/// `search_symbol_by_qualified_name_in_file("nonexistent_044i", helper_id)`, +/// gets None, and the loop continues to shorter splits (none of which +/// resolve), exiting at the final `Ok(None)`. The ref must remain unresolved. +#[test] +fn prefix_resolves_but_tail_missing_stays_unresolved() { + let (_dir, mut tethys) = workspace_with_files(&[ + ( + "Cargo.toml", + r#" +[package] +name = "crate_a" +version = "0.1.0" +edition = "2021" +"#, + ), + ( + "src/lib.rs", + r" +mod helper; + +pub fn entry() { + helper::nonexistent_044i(); +} +", + ), + ( + "src/helper.rs", + r" +pub fn other_thing_044i() {} +", + ), + ]); + + tethys.index().expect("index should succeed"); + + let conn = open_db(&tethys); + + let phantom_resolves: i64 = conn .query_row( "SELECT COUNT(*) FROM refs r JOIN files f ON f.id = r.file_id JOIN symbols s ON s.id = r.symbol_id - WHERE f.path = 'src/lib.rs' AND s.name = 'ThingFour'", + WHERE f.path = 'src/lib.rs' + AND s.name IN ('nonexistent_044i', 'other_thing_044i')", + params![], + |row| row.get(0), + ) + .expect("count"); + + // Cross-check: `other_thing_044i` is indexed (so a phantom binding to it + // would not be silently impossible). + let other_exists: i64 = conn + .query_row( + "SELECT COUNT(*) FROM symbols s JOIN files f ON f.id = s.file_id + WHERE s.name = 'other_thing_044i' AND f.path = 'src/helper.rs'", + params![], + |row| row.get(0), + ) + .expect("count"); + + assert_eq!( + other_exists, 1, + "oracle precondition: other_thing_044i must be indexed in helper.rs \ + (otherwise a phantom binding to it would be impossible by absence, \ + not by the fallback's correctness)" + ); + assert_eq!( + phantom_resolves, 0, + "ref `helper::nonexistent_044i()` must remain unresolved — prefix \ + resolved but tail absent. Got phantom_resolves={phantom_resolves}" + ); +} + +/// Pin that `self::*` and `super::*` paths route through the as-written +/// arm of `qualified_module_fallback` (the `matches!(prefix[0], "crate" | +/// "self" | "super")` gate must include both). +/// +/// Note tethys's filesystem-walk semantics, which differ from Rust: +/// `resolve_self_path` joins segments to the caller's directory, so +/// `self::sibling` from `src/parent/child.rs` reaches `src/parent/sibling.rs`. +/// `resolve_super_path` joins to the caller's grandparent directory, so +/// `super::cousin` from `src/parent/child.rs` reaches `src/cousin.rs` (NOT +/// `src/parent/cousin.rs`). The fixture lays files where each interpretation +/// expects them. +#[test] +fn self_and_super_paths_resolve_via_as_written() { + let (_dir, mut tethys) = workspace_with_files(&[ + ( + "Cargo.toml", + r#" +[package] +name = "crate_a" +version = "0.1.0" +edition = "2021" +"#, + ), + ( + "src/lib.rs", + r" +mod parent; +mod cousin; +", + ), + ( + "src/parent.rs", + r" +mod child; +pub mod sibling; +", + ), + ( + "src/parent/child.rs", + r" +pub fn entry() { + self::sibling::do_self_kid_044i(); + super::cousin::do_super_kid_044i(); +} +", + ), + ( + "src/parent/sibling.rs", + r" +pub fn do_self_kid_044i() {} +", + ), + ( + "src/cousin.rs", + r" +pub fn do_super_kid_044i() {} +", + ), + ]); + + tethys.index().expect("index should succeed"); + + let conn = open_db(&tethys); + + let self_resolved: i64 = conn + .query_row( + "SELECT COUNT(*) + FROM refs r + JOIN files f_caller ON f_caller.id = r.file_id + JOIN symbols s ON s.id = r.symbol_id + JOIN files f_target ON f_target.id = s.file_id + WHERE f_caller.path = 'src/parent/child.rs' + AND s.name = 'do_self_kid_044i' + AND f_target.path = 'src/parent/sibling.rs'", + params![], + |row| row.get(0), + ) + .expect("count"); + + let super_resolved: i64 = conn + .query_row( + "SELECT COUNT(*) + FROM refs r + JOIN files f_caller ON f_caller.id = r.file_id + JOIN symbols s ON s.id = r.symbol_id + JOIN files f_target ON f_target.id = s.file_id + WHERE f_caller.path = 'src/parent/child.rs' + AND s.name = 'do_super_kid_044i' + AND f_target.path = 'src/cousin.rs'", params![], |row| row.get(0), ) .expect("count"); assert!( - resolved_to_target >= 1, - "ref `crate::sub_044i::ThingFour` MUST resolve via the explicit \ - `crate::` arm of resolve_module_path. Got resolved_to_target={resolved_to_target}" + self_resolved >= 1, + "`self::sibling::do_self_kid_044i()` must resolve via the as-written \ + arm of qualified_module_fallback. Got self_resolved={self_resolved}" + ); + assert!( + super_resolved >= 1, + "`super::cousin::do_super_kid_044i()` must resolve via the as-written \ + arm of qualified_module_fallback. Got super_resolved={super_resolved}" ); } From 35782df1f711e3ab76c693134fd201f85f626c4f Mon Sep 17 00:00:00 2001 From: Daryl Walleck Date: Mon, 18 May 2026 19:35:37 -0500 Subject: [PATCH 5/7] style(tethys): cargo fmt resolve.rs (PR #74 Format check) --- crates/tethys/src/resolve.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/crates/tethys/src/resolve.rs b/crates/tethys/src/resolve.rs index 6253c3a..a9c2417 100644 --- a/crates/tethys/src/resolve.rs +++ b/crates/tethys/src/resolve.rs @@ -226,11 +226,8 @@ impl Tethys { // both (a) contain `::` and (b) survived every prior path. Interprets the // prefix as a module path, looks the tail up in the resolved file. if is_qualified - && let Some(symbol) = self.qualified_module_fallback( - ref_name, - ctx.current_file_path, - ctx.src_root, - )? + && let Some(symbol) = + self.qualified_module_fallback(ref_name, ctx.current_file_path, ctx.src_root)? { trace!( ref_id = ref_.id, From dac4d6cd06d8fe15ef22c3a29b096baf0f336f06 Mon Sep 17 00:00:00 2001 From: Daryl Walleck Date: Mon, 18 May 2026 19:42:26 -0500 Subject: [PATCH 6/7] test(tethys): address PR #74 review-feedback round 2 (rivets-044i) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix a self-inflicted regression introduced in round-1 (commit cc6dd0c). Claude code-review #2 against the round-1 commit caught that the disjunctive assertion I added to `workspace_crate_prefixed_call_resolves` assert!(unresolved_refs_in_test == 0 || resolved_to_target >= 1, ...); is structurally vacuous: whenever the subsequent `resolved_to_target >= 1` assertion would pass, the disjunction is trivially true; whenever it would fail, the disjunction can only pass if `unresolved_refs_in_test > 0` — but then the second assertion fails anyway. The disjunction cannot catch a phantom-resolution that the second assertion misses. Same anti-pattern as the I1 vacuous-filter bug fixed in round-1, just self-inflicted. Replaced with a dedicated no-phantoms query: count refs in `crate_b/tests/it.rs` that bind to a `make_widget_044i` or `Widget` symbol whose target file is NOT `crate_a/src/lib.rs`. The fixture has exactly one definition of each there, so the count must be zero; any nonzero value is a phantom resolution. Other round-2 review items (rationale in `.rivets-044i/review-decisions-round-2.md`): - Reject: `tail` allocation timing (sub-microsecond, reviewer agreed not blocking). - Reject: depth-1 `super::` test (would primarily exercise tethys's `resolve_super_path` filesystem-walk semantics, not the fallback's `matches!` gate — out of scope for this PR). - Reject: Gemini's Vec<&str> rewrite (would not compile; `resolve_module_path` takes `&[String]`). - Reject: Gemini's "single-segment paths should resolve to entry-point file" concern (pre-existing resolver behavior, not reachable as a phantom-resolution vector through `qualified_module_fallback`). Verification: - `cargo nextest run -p tethys --test pass2_qualified_paths` → 8/8 pass. - `cargo clippy --all-targets --all-features -- -D warnings` → clean. - `cargo fmt --check` → clean. --- .rivets-044i/review-decisions-round-2.md | 42 ++++++++++++++++++++ crates/tethys/tests/pass2_qualified_paths.rs | 29 +++++++++----- 2 files changed, 62 insertions(+), 9 deletions(-) create mode 100644 .rivets-044i/review-decisions-round-2.md diff --git a/.rivets-044i/review-decisions-round-2.md b/.rivets-044i/review-decisions-round-2.md new file mode 100644 index 0000000..423d200 --- /dev/null +++ b/.rivets-044i/review-decisions-round-2.md @@ -0,0 +1,42 @@ +# Review-feedback decisions — round 2 + +Sources received between rounds 1 and 2: +- **Claude code review #1** (against HEAD `ac917e6`, before round-1): 3 findings. +- **Gemini review** (against HEAD `e559eee`, before round-1): 1 inline comment at + `resolve.rs:334` proposing a Vec<&str>-based rewrite. +- **Claude code review #2** (against HEAD `cc6dd0c`, **after** round-1, reviewing the + round-1 commit): "Approve with minor nits." + +Two of the three pre-round-1 finding sets were already addressed in round-1 (see +`review-decisions-round-1.md`). This round assesses (a) Gemini's inline finding +and (b) Claude #2's three new observations against the round-1 state. + +## Findings + +| # | Finding (one line) | Reviewer | Category | Verified? | Decision | Note | +|---|---|---|---|---|---|---| +| R2-1 | Vacuous disjunctive assertion in `workspace_crate_prefixed_call_resolves` (round-1 introduced `assert!(unresolved_refs_in_test == 0 \|\| resolved_to_target >= 1)` immediately before `assert!(resolved_to_target >= 1)`). The disjunction is trivially true whenever the second assertion would pass, and is implied-false whenever the second would fail. Cannot catch a bug the second misses. | claude-review #2 | Bug | YES — structural reduction: the disjunction `(A == 0) ∨ (B >= 1)` cannot distinguish "ref resolved correctly" from "ref phantom-resolved to a different symbol." | **Accept** | Replace the disjunctive + the now-unused `unresolved_refs_in_test` query with a dedicated **no-phantoms** query joining `f_target` and asserting `s.name IN ('make_widget_044i','Widget')` only ever binds inside `crate_a/src/lib.rs`. Same anti-pattern as the I1 vacuous-filter fix, just self-inflicted in round-1. | +| R2-2 | `tail = segments[split..].join("::")` allocates even on iterations that fall through to `continue` via the `let Some(file_id) = file_id else { continue }` gate. | claude-review #2 | Polish | YES — `tail` is computed unconditionally at the top of each iteration. | **Reject** | Reviewer explicitly noted "Sub-microsecond in practice." `segments.len()` is bounded by `::`-segment count of a single ref (≤ ~6 in real code). Cost is negligible and deferring would obscure the loop's flow with a delayed-allocation pattern. | +| R2-3 | Depth-1 `super::` test missing. Current `self_and_super_paths_resolve_via_as_written` exercises depth-2 (`src/parent/child.rs`); a depth-1 case (`src/child.rs`) would explicitly lock in tethys's filesystem-walk `super` semantics. | claude-review #2 | Coverage | YES — only depth-2 is exercised. | **Reject** | The fence the new test defends is `qualified_module_fallback`'s `matches!(prefix[0], "crate" \| "self" \| "super")` gate — depth-2 fully exercises that gate. Depth-1 would primarily test `resolver::resolve_super_path`'s filesystem-walk semantics, which is preexisting (and arguably-wrong vs Rust) behavior outside this PR's scope. No tracker entry: not deferred, just out of scope. | +| R2-4 | Gemini's suggested rewrite at `resolve.rs:334`: precompute `path_segments: Vec<&str>` with `std::iter::once("crate").chain(segments.iter().copied()).collect()` and slice it for A and B interpretations. | gemini-code-assist | Design | YES — verified `resolve_module_path` signature at `resolver.rs:31-36` takes `path: &[String]`, not `&[&str]`. Gemini's proposal would not compile. | **Reject** | Compile-breaking proposal. The wider concern (per-iteration allocation) is also rejected per round-1 S7 — three similar lines beats premature abstraction, the allocations are bounded by segment count, and `resolve_module_path`'s `&[String]` API is the actual driver of the cost. | +| R2-5 | Gemini's framing claims single-segment paths "may incorrectly handle submodule shadowing" and "should resolve to the crate entry point file (e.g., lib.rs) rather than the source directory." | gemini-code-assist | Bug | PARTIAL — `resolver::resolve_crate_path` does return `crate_root` (a directory) for an empty tail (`path.is_empty()`), not the entry-point file. BUT: `qualified_module_fallback`'s loop only invokes `resolve_module_path` with `prefix.len() >= 1`. When `prefix = ["crate"]`, the call returns `Some(crate_root)`; `get_file_id` then returns `None` for the directory path; the loop silently falls through. Not exploitable through this code path. Submodule shadowing is also already fenced by the existing `submodule_shadows_workspace_crate` test. | **Reject** | Out of scope. Pre-existing resolver behavior, not introduced by this PR, and not reachable as a phantom-resolution vector through `qualified_module_fallback`. The "submodule shadowing" framing is generic — Gemini cites no concrete fixture and the existing test pins the relevant invariant. | + +## Summary + +- **Accept:** R2-1 (1 finding). +- **Reject:** R2-2, R2-3, R2-4, R2-5 (4 findings). + +5 findings reviewed, 1/4 accept/reject. The single accepted item is the meaningful one — a self-inflicted regression of the same anti-pattern I corrected for I1 in round-1. + +## Tracker discipline check + +No deferred work. Every Reject above is "concern not real, not blocking, or fix worse than the disease," not "we'll do this later." No new rivets issues filed. + +## Application + +Single edit to `crates/tethys/tests/pass2_qualified_paths.rs` in `workspace_crate_prefixed_call_resolves`: + +- Remove the `unresolved_refs_in_test` query (no longer used). +- Remove the vacuous disjunctive `assert!`. +- Add a dedicated phantom-detection query: count refs in `crate_b/tests/it.rs` resolved to a symbol named `make_widget_044i` or `Widget` whose target file is NOT `crate_a/src/lib.rs`. Assert == 0. +- Keep the positive assertion `resolved_to_target >= 1` unchanged. diff --git a/crates/tethys/tests/pass2_qualified_paths.rs b/crates/tethys/tests/pass2_qualified_paths.rs index b3d808d..20d1ca5 100644 --- a/crates/tethys/tests/pass2_qualified_paths.rs +++ b/crates/tethys/tests/pass2_qualified_paths.rs @@ -200,24 +200,35 @@ fn smoke() { ) .expect("count definitions"); - let unresolved_refs_in_test: i64 = conn + // No-phantoms fence: any cross-file ref in the test file that binds to + // a `make_widget_044i` or `Widget` symbol must target crate_a/src/lib.rs, + // since the fixture has exactly one definition of each there. Catches + // the bug class where Pass 2 phantom-resolves to a same-named symbol + // outside the expected target file — which the prior round-1 disjunctive + // `(unresolved == 0) || (resolved >= 1)` was structurally incapable of + // catching. + let phantom_resolved: i64 = conn .query_row( - "SELECT COUNT(*) FROM refs r JOIN files f ON f.id = r.file_id - WHERE f.path = 'crate_b/tests/it.rs' AND r.symbol_id IS NULL - AND r.reference_name LIKE '%make_widget_044i%'", + "SELECT COUNT(*) FROM refs r + JOIN files f_caller ON f_caller.id = r.file_id + JOIN symbols s ON s.id = r.symbol_id + JOIN files f_target ON f_target.id = s.file_id + WHERE f_caller.path = 'crate_b/tests/it.rs' + AND s.name IN ('make_widget_044i', 'Widget') + AND f_target.path != 'crate_a/src/lib.rs'", params![], |row| row.get(0), ) - .expect("count unresolved refs"); + .expect("count phantoms"); assert_eq!( definition_exists, 1, "oracle precondition: make_widget_044i must be indexed in crate_a/src/lib.rs" ); - assert!( - unresolved_refs_in_test == 0 || resolved_to_target >= 1, - "either the ref resolves to make_widget_044i (got {resolved_to_target}) or \ - it remains unresolved without phantoms (got unresolved={unresolved_refs_in_test})" + assert_eq!( + phantom_resolved, 0, + "no ref in crate_b/tests/it.rs that resolves to `make_widget_044i` or \ + `Widget` should bind outside crate_a/src/lib.rs. Got phantom_resolved={phantom_resolved}" ); assert!( From 9bfae538e27a33b83e843408b942c508227f0802 Mon Sep 17 00:00:00 2001 From: Daryl Walleck Date: Mon, 18 May 2026 20:05:14 -0500 Subject: [PATCH 7/7] docs(rivets): PR #74 review-feedback round 3 + rivets-nkjd (rivets-044i) Round-3 review-feedback assessment of two new claude-code-review comments (00:38Z and 00:45Z) that reviewed the round-1 state. Two new actions; rest were carryover rejections or already-fixed-in-round-2. R3-1 (claude #3): verification.md said "5 tests" but the file as-committed in round-1 has 8 (the 3 round-1 review-decisions additions: I2, I3, S3-partial). Update the test-count and total-suite-count lines to current state with a brief note crediting the round-1 commit. R3-2 (claude #4): the `super::` filesystem-walk-vs-Rust-spec divergence is documented in the new `self_and_super_paths_resolve_via_as_written` test docstring but had no tracker entry. Per the project's tracker-discipline rule, documented-only deferrals rot in `./` dirs. Filed rivets-nkjd to track the divergence and updated the test docstring to reference it. Rejected (carryover from prior rounds): - `(*s).to_string()` style: clippy doesn't flag it (round-1 S6 rationale stands; two reviewers' subjective preference doesn't override the linter). - `tail` allocation timing: round-2 R2-2 rationale stands (sub-microsecond). Already fixed: - Disjunctive assertion in `workspace_crate_prefixed_call_resolves`: both reviewers flagged it; round-2 R2-1 fixed it in commit 80167b3. They were reading the round-1 state. Decision log: `.rivets-044i/review-decisions-round-3.md`. Verification: - 8/8 pass2_qualified_paths tests pass. - `cargo clippy --all-targets --all-features -- -D warnings` clean. - `cargo fmt --check` clean. --- .rivets-044i/review-decisions-round-3.md | 41 ++++++++++++++++++++ .rivets-044i/verification.md | 8 +++- .rivets/issues.jsonl | 1 + crates/tethys/tests/pass2_qualified_paths.rs | 7 +++- 4 files changed, 53 insertions(+), 4 deletions(-) create mode 100644 .rivets-044i/review-decisions-round-3.md diff --git a/.rivets-044i/review-decisions-round-3.md b/.rivets-044i/review-decisions-round-3.md new file mode 100644 index 0000000..bdf5b35 --- /dev/null +++ b/.rivets-044i/review-decisions-round-3.md @@ -0,0 +1,41 @@ +# Review-feedback decisions — round 3 + +Sources received after round-2 push: +- **Claude code review #3** (00:38:00Z, against `cc6dd0c` round-1 state). +- **Claude code review #4** (00:45:54Z, against `d07867f` round-1+fmt state). + +Both reviewed the round-1 state, not round-2 — so they re-flagged round-2's +already-fixed R2-1 disjunctive (verified that fix is in `80167b3`, already +applied), the round-2 already-rejected `(*s).to_string()` and `tail` allocation +items, and surfaced two new observations that warrant action. + +## Findings + +| # | Finding (one line) | Reviewer | Category | Verified? | Decision | Note | +|---|---|---|---|---|---|---| +| R3-1 | `.rivets-044i/verification.md:66` reports `(5 tests, all pass — claims 1, 2, 3, 4, 5)` but the file as-committed in round-1 contains 8 tests. Audit trail is stale relative to current state. | claude #3 | Doc drift | YES — verified by reading `verification.md` and counting tests in `pass2_qualified_paths.rs`. | **Accept** | Per the diagnostic-directory convention these are point-in-time, but the issue hasn't closed and the document is currently misleading reviewers. Update to reflect 8-test reality with a note that the additional 3 were added by the round-1 review-decisions commit. | +| R3-2 | The `super::` filesystem-walk-vs-Rust-spec divergence is documented in the new `self_and_super_paths_resolve_via_as_written` test's docstring but has no tracker entry. | claude #4 | Tracker discipline | YES — `rivets list` returns 52 open issues; grepped for `(resolve_super\|super_path\|super::.*semantic\|filesystem.*walk)` → zero matches. | **Accept** | Per `feedback_tracker_entries_for_deferrals`: documented-only deferrals rot. The divergence is preexisting tethys behavior, not introduced by PR #74, but PR #74 is the first place it's *explicitly documented in code*. File a new rivets issue and reference it from the test docstring. | +| R3-3 | `(*s).to_string()` is unidiomatic — auto-deref via `s.to_string()` is the more conventional form. | claude #3, #4 | Style | Repeated finding (round-1 S6, now also round-3). YES — confirmed clippy doesn't flag it. | **Reject (carryover)** | Two reviewers flagging the same thing is in the skill's red-flags list: "Two reviewers can share the same blind spot. Verify the underlying claim, not the count." The underlying claim is "clippy flags it" — verified false in round-1. Subjective style preferences don't override the linter's machine verdict. Standing by the rejection. | +| R3-4 | Disjunctive assertion in `workspace_crate_prefixed_call_resolves` is vacuous. | claude #3, #4 | Bug | Was a real bug — | **Already fixed** | Round-2 R2-1, commit `80167b3`. Both reviewers were reading the round-1 state and didn't see the fix. No action. | +| R3-5 | `tail = segments[split..].join("::")` allocates before the `file_id` gate. | claude #4 | Polish | Repeat of R2-2. | **Reject (carryover)** | Already rejected in round-2; reviewer explicitly noted "Sub-microsecond in practice." Carryover rejection. | + +## Summary + +- **Accept:** R3-1, R3-2 (2 findings). +- **Reject (carryover):** R3-3, R3-5 (2 findings — repeat of prior rejections). +- **n/a:** R3-4 (already fixed in prior round). + +5 findings reviewed, 2 net new actions. + +## Tracker discipline check + +R3-2 *files* a new tracker entry rather than deferring without one — exactly the +discipline the rule enforces. R3-1 is in-scope for this branch's next commit. +No silent drops. + +## Application + +1. Edit `.rivets-044i/verification.md`: update the test-count line and add a brief note. +2. `rivets create` a new issue for the `super::` semantics divergence; capture the ID. +3. Update the docstring of `self_and_super_paths_resolve_via_as_written` to reference the new issue ID. +4. Commit (one bundled commit). diff --git a/.rivets-044i/verification.md b/.rivets-044i/verification.md index c515a3a..b2ee79a 100644 --- a/.rivets-044i/verification.md +++ b/.rivets-044i/verification.md @@ -63,11 +63,15 @@ that resolved purely via Pass 1 (same-file). All probes, falsifiers, and regression fences pass against the post-fix binary: -- `pass2_qualified_paths.rs` (5 tests, all pass — claims 1, 2, 3, 4, 5) +- `pass2_qualified_paths.rs` (8 tests, all pass — claims 1, 2, 3, 4, 5 + 3 added by + the round-1 review-decisions commit `cc6dd0c`: `longest_prefix_wins_over_shorter` + pins the loop-direction invariant, `prefix_resolves_but_tail_missing_stays_unresolved` + pins the tail-miss branch, `self_and_super_paths_resolve_via_as_written` pins the + `matches!` gate for `self::*` / `super::*` paths) - `pass2_no_imports.rs` (1 test, passes — claim 6) - `file_deps_corroboration.rs` (2 tests, pass — claim 7) - `resolver_routing.rs` (4 tests, pass — claim 8) -- Full `cargo nextest run -p tethys` (631 tests, all pass) +- Full `cargo nextest run -p tethys` (636 tests, all pass post round-1 additions) - Phantom-rate probe (claim 7 oracle) — 0.00% - 0gom Section 3 probe (claim 8 oracle) — 0 violations - Indexing wall time (claim 1 budget) — −10% vs baseline diff --git a/.rivets/issues.jsonl b/.rivets/issues.jsonl index 79a8d11..5fc89e7 100644 --- a/.rivets/issues.jsonl +++ b/.rivets/issues.jsonl @@ -299,3 +299,4 @@ {"id":"rivets-fr9b","title":"tethys: track parent_symbol_id for nested symbols","description":"When indexing symbols, parent_symbol_id is always set to None. Implement proper parent symbol tracking so nested symbols (e.g., methods inside structs/impls) have their parent relationship recorded.","status":"open","priority":3,"issue_type":"task","assignee":null,"labels":[],"design":null,"acceptance_criteria":null,"notes":null,"external_ref":null,"dependencies":[],"created_at":"2026-03-19T01:41:02.074384054Z","updated_at":"2026-03-19T01:41:02.074384054Z","closed_at":null} {"id":"rivets-5hvt","title":"Backlog","description":"Lower-priority and speculative items. Revisit periodically to promote or close.","status":"open","priority":4,"issue_type":"epic","assignee":null,"labels":[],"design":null,"acceptance_criteria":null,"notes":null,"external_ref":null,"dependencies":[],"created_at":"2026-02-21T02:28:32.353090414Z","updated_at":"2026-02-21T02:28:32.353090414Z","closed_at":null} {"id":"rivets-cej","title":"Implement stale command","description":"Add 'stale' command to find issues not updated recently.\\n\\nFlags:\\n- --days N (default: 30)\\n- --status (filter by status)\\n- --limit N\\n- --json\\n\\nExample: rivets stale --days 30 --status in_progress --json","status":"closed","priority":2,"issue_type":"feature","assignee":null,"labels":[],"design":null,"acceptance_criteria":null,"notes":null,"external_ref":null,"dependencies":[{"depends_on_id":"rivets-v3s","dep_type":"parent-child"}],"created_at":"2025-12-28T00:23:56.751583711Z","updated_at":"2025-12-28T00:44:18.799743031Z","closed_at":"2025-12-28T00:44:18.799743031Z"} +{"id":"rivets-nkjd","title":"tethys: resolve_super_path uses filesystem-walk semantics, diverges from Rust super:: scoping","description":"tethys's `resolver::resolve_super_path` interprets `super::X` from a file at `src/parent/child.rs` as a filesystem grandparent walk: `current_file.parent().parent()` then joins X, reaching `src/X.rs`. Rust's actual semantics: `super::X` from `mod parent::child` refers to `crate::parent::X`, which lives at `src/parent/X.rs` (or methods on the parent module file).\n\nConcrete example, surfaced during rivets-044i (PR #74):\n\n src/lib.rs -> mod parent;\n src/parent.rs -> mod child; pub mod sibling;\n src/parent/child.rs:\n pub fn entry() {\n super::sibling::foo(); // Rust: src/parent/sibling.rs ; tethys: src/sibling.rs\n }\n\nThe new `self_and_super_paths_resolve_via_as_written` test in pass2_qualified_paths.rs documents this divergence (fixture lays sibling at the location tethys's resolver expects) but doesn't fix it.\n\nImpact: any qualified ref using `super::X::method()` style from a depth-2+ file resolves to the wrong file via the qualified_module_fallback path (or fails to resolve at all if the wrong-level file doesn't exist). The use-statement form `use super::X;` goes through a different code path (Pass-2 explicit imports) and may have the same issue — needs verification.\n\nSurfaced by claude-code-review #4 on PR #74 during round-3 review-feedback assessment, with the explicit suggestion to file a tracker entry rather than leave the divergence documented-only.","status":"open","priority":4,"issue_type":"bug","assignee":null,"labels":[],"design":"Two candidate fixes:\n\n(a) Build a module-tree map at index time: for each `.rs` adjacent to a `/` directory, record that the .rs file is the parent-module file of the subdirectory's contents. Then `resolve_super_path` walks the module tree instead of the filesystem.\n\n(b) Special-case the parent-module pattern in resolve_super_path: when `current_file.parent()` contains a sibling `.rs` (e.g. for `src/parent/child.rs`, check whether `src/parent.rs` exists), treat that .rs file as the parent module and resolve `super::X` relative to *its* directory (which is `src/parent/`, not `src/`).\n\nOption (b) is narrower and matches the common-case Rust idiom (parent.rs + parent/ pattern). Option (a) handles edge cases like `parent/mod.rs` + sibling files. Either way, `resolve_self_path` may need parallel review since it's the inverse semantics question.\n\nRisk: any test or workspace currently relying on the filesystem-walk semantics breaks. The rivets workspace itself doesn't seem to use this shape (otherwise PR #74's rivets-workspace re-index would have surfaced differently).","acceptance_criteria":"- [ ] resolve_super_path returns `src/parent/X.rs` for ref `super::X` from `src/parent/child.rs` when `src/parent.rs` exists\n- [ ] Test in pass2_qualified_paths.rs (or new test file) pins the Rust-spec semantics\n- [ ] Existing rivets workspace re-index post-fix: no phantom-rate regression (claim 7 of rivets-3d0s fence holds)\n- [ ] Update self_and_super_paths_resolve_via_as_written's docstring to remove the divergence-documentation language (or replace with a comment that says \"this is what Rust does, this is what we do, and they now agree\")\n- [ ] resolve_self_path reviewed for parallel issues; either confirmed correct or follow-up issue filed","notes":null,"external_ref":null,"dependencies":[],"created_at":"2026-05-19T01:04:21.198957500Z","updated_at":"2026-05-19T01:04:21.198957500Z","closed_at":null} diff --git a/crates/tethys/tests/pass2_qualified_paths.rs b/crates/tethys/tests/pass2_qualified_paths.rs index 20d1ca5..fff08c9 100644 --- a/crates/tethys/tests/pass2_qualified_paths.rs +++ b/crates/tethys/tests/pass2_qualified_paths.rs @@ -765,8 +765,11 @@ pub fn other_thing_044i() {} /// `self::sibling` from `src/parent/child.rs` reaches `src/parent/sibling.rs`. /// `resolve_super_path` joins to the caller's grandparent directory, so /// `super::cousin` from `src/parent/child.rs` reaches `src/cousin.rs` (NOT -/// `src/parent/cousin.rs`). The fixture lays files where each interpretation -/// expects them. +/// `src/parent/cousin.rs` — Rust spec would say the latter). The fixture +/// lays files where each interpretation expects them. The divergence is +/// preexisting tethys behavior, not introduced by rivets-044i, and is +/// tracked separately as rivets-nkjd; when that issue closes, this test +/// fixture (and likely this docstring) will need adjusting. #[test] fn self_and_super_paths_resolve_via_as_written() { let (_dir, mut tethys) = workspace_with_files(&[