Skip to content

fix(tethys): resolve qualified refs from import-less files (rivets-044i)#74

Merged
dwalleck merged 7 commits into
mainfrom
feat/rivets-044i-qualified-paths
May 19, 2026
Merged

fix(tethys): resolve qualified refs from import-less files (rivets-044i)#74
dwalleck merged 7 commits into
mainfrom
feat/rivets-044i-qualified-paths

Conversation

@dwalleck
Copy link
Copy Markdown
Owner

Summary

  • Fixes rivets-044i: qualified refs of the form helper::do_thing or crate_a::Widget::method from import-less files now resolve via a new qualified_module_fallback in Pass 2.
  • Built using the gilfoyle workflow (probe → falsifiable design → budgeted plan → checkpointed build). Full audit trail in .rivets-044i/.
  • Closes the qualified-path gap that rivets-dn35 (PR fix(tethys): drop Pass-2 short-circuit on import-less files (rivets-dn35) #69) explicitly left for follow-up.

The bug

Stored symbols.qualified_name is module-stripped (indexing.rs:627-630: free fns store name; methods store parent_name::name). The existing Pass 2 qualified-name fallback (get_symbol_by_qualified_name) does a literal string-equality match, so any ref whose source text carries a module or workspace-crate prefix systematically misses — regardless of whether the target symbol is indexed.

This was the residual gap dn35 deferred: dn35 removed the imports.is_empty() short-circuit so import-less files reached fallback, but the qualified-path arm still couldn't bridge stored vs. written qualified-name formats.

The fix

resolve.rs::qualified_module_fallback (new private method, ~80 lines). After every prior path returns None for a qualified ref:

  1. Split ref_name at each :: boundary from longest prefix to shortest.
  2. For each prefix, translate to a file_id via resolver::resolve_module_path, trying:
    • Implicit-crate first (prepend crate::, skipped when path[0] is already crate/self/super) — handles Rust 2018+ implicit-crate-relative paths like helper::foo.
    • As-written — lets resolve_module_path's crate/self/super/workspace-crate arms fire.
  3. Query search_symbol_by_qualified_name_in_file on the tail.
  4. First successful (file_id, symbol) pair wins.

The implicit-crate-first order preserves Rust's submodule-shadows-extern-crate scoping rule — pinned by the submodule_shadows_workspace_crate test.

Slices

Slice Commit Purpose
1 5b950d3 Implement qualified_module_fallback + 3 tests covering s-submod, s-wscrate, and the submodule-shadows-extern-crate adversarial fence. CLAUDE.md updated to reflect post-dn35 + post-044i Pass-2 chain.
2 ac917e6 2 adversarial tests: qualified_external_crate_stays_unresolved (pins s-extern boundary — std::* must not phantom-resolve) and qualified_crate_prefix_resolves (pins the path[0] == "crate" short-circuit).
3 e559eee .rivets-044i/ diagnostic dir (probe, oracle, design, plan, verification). No production code change.

Verification (rivets workspace)

Metric Pre-fix Post-fix Notes
Total tests 1530 1532 +2 new tests in pass2_qualified_paths.rs
Indexing wall time 23.25s 20.81s −10% (under 1.5× budget)
Phantom rate (rivets-3d0s) 0.00% 0.00% claim 7 fence: PASS
Ambiguity violations (rivets-0gom) 0 0 claim 8 fence: PASS
Resolved refs 6756 6837 +81
Unresolved qualified refs 1558 1493 −65

The post-fix wall time is faster than baseline because resolving more refs earlier trims downstream work.

Falsifiable design

All 8 design claims and their falsifiers are documented in .rivets-044i/design.md. Each claim has a regression fence — either a new test in pass2_qualified_paths.rs or an existing CI fence (file_deps_corroboration.rs, resolver_routing.rs, pass2_no_imports.rs).

Test plan

  • cargo nextest run — 1532/1532 pass workspace-wide
  • cargo clippy --all-targets --all-features -- -D warnings — clean
  • Re-index rivets workspace — phantom rate stays 0.00%
  • Re-index rivets workspace — 0gom ambiguity violations stay 0
  • Wall time within 1.5× baseline budget
  • CI regression fences pass: file_deps_corroboration (2 tests), resolver_routing (4 tests), pass2_no_imports (1 test)

@claude
Copy link
Copy Markdown

claude Bot commented May 18, 2026

Code Review: fix(tethys): resolve qualified refs from import-less files (rivets-044i)

Overall this is a well-scoped, carefully reasoned fix. The implementation is clean, additive-only, and backed by a rigorous audit trail. A few items worth discussing below, but none are blockers.


What the PR does

Adds qualified_module_fallback to resolve.rs — a new last-resort arm in try_resolve_reference that fires only for qualified refs (containing ::) that have survived every prior Pass 2 path. It works by splitting the ref on :: from longest-prefix to shortest, translating each prefix to a file_id via resolve_module_path, then looking up the tail in that file. The implicit-crate-first interpretation order correctly models Rust's submodule-shadows-extern-crate scoping rule.


Strengths

  • Correctly scoped. Strictly additive to the existing fallback chain. Unqualified refs, Pass 1, and Pass 3 are not touched. No schema change.
  • Right interpretation ordering. Trying crate::<prefix> before <prefix> as-written preserves Rust's submodule-shadows-extern-crate rule. The submodule_shadows_workspace_crate adversarial test pins this invariant precisely.
  • Five tests, each defending a distinct bug class. The adversarial angle on qualified_external_crate_stays_unresolved (adding a std_helper submodule to defeat partial-match bugs) is particularly good.
  • Good doc comment. Explains the structural mismatch between stored qualified_name and written ref text — the WHY — without re-stating what the code already shows.
  • Measured, non-regressing. The PR shows wall-time improved (−10%), phantom rate stayed 0.00%, and 0gom ambiguity violations stayed at 0.

Issues

1. Leftover eprintln! probe output in two permanent regression fences

submodule_qualified_call_resolves and workspace_crate_prefixed_call_resolves both contain:

eprintln!(
    "PROBE 044i state: total_refs={total_refs}, resolved_refs={resolved_refs}, \
     resolved_to_target={resolved_to_target}, definition_exists={definition_exists}"
);

These look like carry-over from the probe phase. The other three tests in the same file don't have this. In nextest (process-per-test, output suppressed on pass), it's not harmful — but it's inconsistent and adds noise to failure output. Consider removing, or at minimum removing the "PROBE 044i state:" prefix and keeping just the diagnostic values as a failure hint attached to the assert!.

2. s-self and s-super input shapes not covered by dedicated tests

The design table lists 6 first-segment shapes; the implementation handles all 6. The paper hand-trace (falsification claim 1) verified correctness for self::helper::foo and super::helper::foo, but there are no regression fences for these two shapes in pass2_qualified_paths.rs. The other 4 shapes each have dedicated tests.

These shapes are less common in practice (explicit self:: / super:: from import-less files), but they're non-trivial code paths in resolve_module_path. A future change to resolve_module_path's self/super arms would not be caught by the current test file. Low-priority but worth noting for completeness.

3. Minor: explicit (*s).to_string() is unnecessary

Two lines in qualified_module_fallback:

with_crate.extend(prefix.iter().map(|s| (*s).to_string()));
// and
let as_owned: Vec<String> = prefix.iter().map(|s| (*s).to_string()).collect();

prefix is &[&str], so iteration gives &&str. Both s.to_string() (auto-deref applies in method calls) and .map(|&s| s.to_string()) or .map(ToString::to_string) work and are slightly idiomatic. Clippy with pedantic may or may not flag this (it's not a correctness issue), but it's mildly surprising for a reader expecting the pattern used elsewhere.


CLAUDE.md update

The updated resolver-internals bullet is accurate and a genuine improvement — it now correctly describes the full Pass 2 chain post-dn35 and post-044i. Good.


Summary

The implementation is correct, well-tested across its input-shape space, and consistent with the project's pedantic lint bar. Items 2 and 3 are nice-to-haves; item 1 is a minor cleanup that would improve the test file's consistency. None of these hold up a merge.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements a fallback resolution mechanism for qualified references in import-less Rust files, addressing a limitation where module-stripped stored names caused lookup failures. The new qualified_module_fallback method employs a longest-prefix split strategy to resolve path segments to specific files. Review feedback identifies opportunities to optimize performance by reducing redundant allocations within the resolution loop and emphasizes ensuring that single-segment paths correctly resolve to crate entry points to maintain dependency graph integrity.

Comment on lines +298 to +327
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<String> = 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<String> = 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)?;
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current implementation performs redundant allocations inside the loop and may incorrectly handle submodule shadowing. In Rust module resolution, ensure single-segment paths resolve to the crate entry point file (such as lib.rs) rather than the source directory to prevent corrupting dependency graphs. To improve performance, avoid unnecessary allocations in loops by using borrowed slices instead of owned String objects, delaying conversion until necessary. While clarity is prioritized over micro-optimization, these changes address both correctness and efficiency.

        let crates = self.crates();
        let path_segments: Vec<&str> = std::iter::once("crate").chain(segments.iter().copied()).collect();

        for split in (1..segments.len()).rev() {
            let mut file_id = None;
            let mut module_resolved = false;

            if !matches!(segments[0], "crate" | "self" | "super") {
                if let Some(resolved) = resolve_module_path(&path_segments[..split + 1], current_file, src_root, crates) {
                    module_resolved = true;
                    file_id = self.db.get_file_id(&self.relative_path(&resolved))?;
                }
            }

            if !module_resolved {
                if let Some(resolved) = resolve_module_path(&path_segments[1..split + 1], current_file, src_root, crates) {
                    file_id = self.db.get_file_id(&self.relative_path(&resolved))?;
                }
            }
        }
References
  1. In Rust module resolution, ensure single-segment paths resolve to the crate's entry point file (e.g., lib.rs) rather than the source directory to prevent corrupting dependency graphs that expect file-level edges.
  2. To improve performance, avoid unnecessary allocations in loops. When using Cow types (like Cow), perform lookups and comparisons on the borrowed variant (e.g., using .as_ref()) and delay conversion to the owned variant (e.g., using .into_owned()) until the value needs to be stored.
  3. Prioritize code clarity over micro-optimizations like avoiding small allocations in loops unless profiling demonstrates a performance issue, especially for small data sets. Consider pre-normalizing data at construction time if optimization is required.

dwalleck added a commit that referenced this pull request May 19, 2026
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<String> 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.
@claude
Copy link
Copy Markdown

claude Bot commented May 19, 2026

Code Review — fix(tethys): resolve qualified refs from import-less files (rivets-044i)

Overview

This PR closes the qualified-path resolution gap that dn35 (PR #69) explicitly deferred: refs like helper::do_thing or crate_a::Widget::method from import-less files now route through a new qualified_module_fallback pass. Production change: ~130 lines in resolve.rs. Tests: 852 lines, 5 adversarial regression fences. Numbers: +81 resolved refs, −10% wall time, 0.00% phantom rate maintained.


Production code: crates/tethys/src/resolve.rs

Strengths:

  • Rustdoc on qualified_module_fallback is excellent — especially the Acceptable ambiguity section explicitly bounding the phantom-resolution class relative to the design claims.
  • Longest-prefix-first iteration is correct and pinned by a dedicated test.
  • Implicit-crate-first interpretation order correctly encodes Rust's submodule-shadows-extern-crate scoping rule, defended by the adversarial submodule_shadows_workspace_crate fixture.
  • crates() is hoisted out of the per-split loop — no per-iteration DB round-trip.
  • All error paths propagate via ?. No panics or .unwrap() in production code.
  • Structured tracing fields match project convention (%ref_name, %symbol.id).

Minor nits:

  1. Redundant dereference in with_crate constructionprefix.iter().map(|s| (*s).to_string()): s: &&str, so *s: &str. The deref is correct but s.to_string() is identical via auto-deref. Not clippy-pedantic-flagged; purely cosmetic.

  2. as_owned allocation even when file_id is already Some — the if file_id.is_none() guard is correct. At 2–4 iterations max, allocation cost is negligible. Noting for completeness.

  3. tail allocation before the file_id gatesegments[split..].join("::") allocates before let Some(file_id) = file_id. Could be deferred past the early-continue. Sub-microsecond in practice.


Tests: crates/tethys/tests/pass2_qualified_paths.rs

Rigorous, adversarial, follows the project fixture pattern well. Positive + negative assertions on each input shape.

One substantive observation — workspace_crate_prefixed_call_resolves:

The test contains two sequential assertions:

assert!(
    unresolved_refs_in_test == 0 || resolved_to_target >= 1,
    "either the ref resolves or it remains unresolved without phantoms"
);
assert!(
    resolved_to_target >= 1,
    "workspace-crate-prefix call must resolve"
);

The first assertion is vacuous once the second is present. If resolved_to_target >= 1, the disjunction is trivially true. If resolved_to_target == 0, the first only passes when unresolved_refs_in_test > 0 — but the second fails anyway. The first assertion can never catch a bug the second misses. A dedicated no-phantoms check would be more informative:

let phantom_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 = 'crate_b/tests/it.rs' AND s.name != 'make_widget_044i'",
    params![],
    |row| row.get(0),
).expect("count phantoms");
assert_eq!(phantom_resolved, 0, "no phantom bindings for the test file");

Not a blocker.

Potential follow-up test: super:: from a one-level-deep module (e.g. from src/child.rs rather than src/parent/child.rs). The existing self_and_super_paths_resolve_via_as_written starts at depth-2, leaving the one-level case uncovered. The doc comment notes tethys's resolve_super_path has intentional Rust-semantics deviation — a one-level test would lock in that behavior explicitly.


Diagnostic artifacts and CLAUDE.md

.rivets-044i/ follows the project convention correctly. Pre/post ambiguity and phantom-rate snapshots are good anchors for future regression detection. The CLAUDE.md Pass-2 chain description accurately reflects the post-dn35 + post-044i sequence — clear improvement over the stale short-circuit description.


Summary

Approve with minor nits. Implementation is correct, invariants are well-documented and defended by adversarial tests, and the empirical numbers hold (0.00% phantom rate, +81 resolved refs, −10% wall time). The vacuous disjunctive assertion in workspace_crate_prefixed_call_resolves is the only thing worth cleaning up before merge — everything else is cosmetic.

@claude
Copy link
Copy Markdown

claude Bot commented May 19, 2026

Code Review: fix(tethys): resolve qualified refs from import-less files (rivets-044i)

Overview

This PR adds qualified_module_fallback to Pass 2's resolver chain, bridging the gap left by rivets-dn35. The root cause is well-diagnosed: symbols.qualified_name is module-stripped in storage (free fns store name; methods store parent_name::name), so the existing literal-string get_symbol_by_qualified_name lookup structurally cannot match a ref like helper::do_thing from an import-less file. The fix translates the prefix segment(s) to a file_id via the existing resolve_module_path, then looks up the tail in that specific file.

The gilfoyle audit trail (.rivets-044i/) is thorough and the round-1 review decisions have all been applied. Overall this is ready-to-ship quality — the findings below are minor.


Implementation (crates/tethys/src/resolve.rs)

Correctness

The algorithm is correct:

  • Longest-prefix-first ((1..segments.len()).rev()) matches Rust's most-specific-path-wins scoping rule.
  • Implicit-crate-first ordering correctly implements submodule-shadows-extern-crate precedence.
  • The !matches!(prefix[0], "crate" | "self" | "super") gate prevents the double-crate:: construction.
  • External crate refs fall through all splits and exit at Ok(None) because resolve_module_path returns None for unrecognised first segments.
  • trace! on both the success path and the final Ok(None) exit (S1) is present and uses structured fields per project convention.

One minor nit — (*s).to_string() idiom:

At lines with_crate.extend(prefix.iter().map(|s| (*s).to_string())) and the as_owned block, prefix.iter() yields &&str, so the explicit deref (*s) is needed before calling .to_string(). Clippy doesn't flag it (S6 rejection was correct), but the same result is achieved with the slightly more idiomatic .map(|s| s.to_string()) — auto-deref applies for method calls. Strictly cosmetic; not a blocking issue.

Performance acknowledgement:

Each split iteration allocates a Vec<String>. For a ref with N segments, this is O(N) allocations of growing size. The plan correctly tracks this as a deferral to rivets-bjdn. The post-fix wall-time measurement (-10% vs baseline) confirms this is fine at production scale.


Tests (crates/tethys/tests/pass2_qualified_paths.rs)

8 tests covering all the input shapes and adversarial cases:

Test Shape Adversarial element
submodule_qualified_call_resolves s-submod
workspace_crate_prefixed_call_resolves s-wscrate
submodule_shadows_workspace_crate s-submod vs s-wscrate Name collision: same string is both workspace-crate and local submodule
qualified_external_crate_stays_unresolved s-extern std_helper lookalike submodule to defeat partial-string bugs
qualified_crate_prefix_resolves s-crate Literal src/crate/sub_044i.rs trap directory to catch broken gate
longest_prefix_wins_over_shorter iteration order Both outer and outer::inner resolve to different files with same-named symbols
prefix_resolves_but_tail_missing_stays_unresolved tail-miss Prefix resolves; tail symbol absent in the file
self_and_super_paths_resolve_via_as_written s-self / s-super As-written arm must fire; implicit-crate gate must suppress retry

These are proper falsifiable tests — each one would fail under the specific bug class it's designed to detect.

One observation — redundant intermediate assertion in workspace_crate_prefixed_call_resolves:

assert!(
    unresolved_refs_in_test == 0 || resolved_to_target >= 1,
    ...
);
assert!(
    resolved_to_target >= 1,
    ...
);

The || assertion is logically subsumed by the strong assertion below it: if resolved_to_target >= 1, both pass; if resolved_to_target == 0 and unresolved_refs_in_test == 0 (ref dropped from DB entirely), the first assertion passes but the second correctly fails. So the intermediate assertion adds no new information and could be removed for clarity. Not a correctness issue.

Documentation note — self::*/super::* filesystem semantics:

The self_and_super_paths_resolve_via_as_written test doc comment correctly calls out that tethys's resolve_super_path walks the filesystem (grandparent directory), which differs from Rust's actual super:: semantics (parent module in the module tree). This is pre-existing behaviour in resolve_module_path, not introduced here. Good that it's documented.


Metrics

Metric Pre-fix Post-fix
Wall time 23.25s 20.81s (-10%)
Phantom rate 0.00% 0.00%
0gom violations 0 0
Resolved refs 6756 6837 (+81)
Unresolved qualified 1558 1493 (-65)

All regression fences pass. The -10% wall-time improvement is consistent with resolving more refs early (shorter subsequent lookup chains).

Minor note: verification.md reports 5 tests in pass2_qualified_paths.rs, but the file as-committed contains 8. The three additional tests (I2, I3, S3 partial) were added in the round-1 review-decisions commit. The diagnostic directory is point-in-time per convention, so this is expected — just worth noting if you cross-reference the numbers.


CLAUDE.md update

The one-line pass-2 chain description is accurate and appreciably more useful than the prior entry. The symbol-relative anchor (indexing.rs::store_references) avoids the line-range rot problem (S10 applied correctly).


Summary

Approve. The implementation is correct, the test suite is adversarially comprehensive, all prior review feedback has been addressed, and the regression metrics hold. The two notes above (redundant intermediate assertion, cosmetic (*s).to_string() style) are both non-blocking.

dwalleck added a commit that referenced this pull request May 19, 2026
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.
@claude
Copy link
Copy Markdown

claude Bot commented May 19, 2026

Code Review — fix(tethys): qualified refs from import-less files (rivets-044i)

Overall: Approve with minor notes. The algorithm design is correct, the scope is tight, and the adversarial test suite is genuinely thorough. Notes below are mostly informational or stylistic — none block merge.


Algorithm Correctness

The longest-prefix-first + implicit-crate-first interpretation order correctly implements Rust's scoping rule (submodule shadows extern crate). The matches!(prefix[0], "crate" | "self" | "super") gate on the implicit-crate arm is necessary and correct — without it, crate::sub::fn would prepend a second crate:: and fail to resolve.

The loop direction (1..segments.len()).rev() is pinned by longest_prefix_wins_over_shorter, which is a well-constructed test: both the shallower and deeper prefix resolve, so an inverted loop produces a wrong answer rather than a miss. That's exactly the right way to fence this invariant.


Test Quality

The adversarial cases are the standout feature of this PR:

  • submodule_shadows_workspace_crate — pins interpretation precedence where the wrong answer isn't "unresolved" but "resolved to the wrong file." This is the hardest category of bug to catch with happy-path tests.
  • qualified_crate_prefix_resolves — the on-disk src/crate/ trap directory is an excellent false-positive fence. A naive path-join implementation would match the wrong file.
  • qualified_external_crate_stays_unresolved — correctly avoids the reference_name LIKE 'std::%' vacuous query (caught in round-1 review as I1), using s.name IN ('HashMap', 'new') against indexed symbols instead.

One thing worth documenting (may already be noted in the design file but not visible in the diff): the qualified_external_crate_stays_unresolved test can only assert "no symbol from the indexed workspace was spuriously matched." It can't enumerate all possible phantom targets. That's fine — the layered defense via the workspace-level file_deps_corroboration phantom-rate gate covers the gap. Just confirming this is understood as intentional.


Minor Code Observations

(*s).to_string() at lines ~1476 and ~1486 in resolve.rs

with_crate.extend(prefix.iter().map(|s| (*s).to_string()));
// and
let as_owned: Vec<String> = prefix.iter().map(|s| (*s).to_string()).collect();

The explicit deref (*s) is not wrong — clippy doesn't flag it — but idiomatic Rust uses auto-deref: s.to_string(). I see the round-1 rejection of S6 ("clippy doesn't flag it") which is correct, but I'd still lean toward the cleaner form. Take or leave.

tail allocation on continued iterations

let tail = segments[split..].join("::");
// ... later, if file_id is None, we `continue` without using tail

Round-2 flagged this (R2-2). The rejection ("sub-microsecond in practice, segments.len() ≤ ~6") is correct. No action needed — just noting it's a known tradeoff.


Pre-existing Behavior Note: super:: Semantics

The self_and_super_paths_resolve_via_as_written test doc comment correctly notes:

tethys's filesystem-walk semantics differ from Rust: 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).

This is pre-existing (not introduced here), and the test correctly documents it rather than papering over it. Worth tracking as a separate issue if tethys aspires to Rust-spec super:: semantics — but that's out of scope for this PR.


CI

Validate Commits, Format, and Lint are all green. Test/Build matrix is still in progress at review time. Given the author reports cargo nextest run 1532/1532 and cargo clippy -- -D warnings clean locally, I'd expect these to pass — but merge after the matrix is confirmed green.


Summary

Area Assessment
Algorithm correctness ✅ Sound — longest-prefix-first + implicit-crate-first is the right order
Test coverage ✅ Adversarial fences cover the key failure modes
Scope ✅ One production file changed, additive only
Performance ✅ −10% wall time (counter-intuitive but explained: more early resolutions = less downstream work)
Phantom safety ✅ Layered: per-test + workspace-level phantom-rate gate
Style ⚠️ (*s).to_string() is non-idiomatic (non-blocking)
CI ⏳ Matrix still running — merge after green

LGTM pending CI green.

dwalleck added a commit that referenced this pull request May 19, 2026
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 `.<issue-id>/` 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.
dwalleck added 7 commits May 18, 2026 20:11
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)
…ets-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.
Per .<issue-id>/ 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
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<String> 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.
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.
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 `.<issue-id>/` 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.
@dwalleck dwalleck force-pushed the feat/rivets-044i-qualified-paths branch from 305ca2c to 9bfae53 Compare May 19, 2026 01:12
@claude
Copy link
Copy Markdown

claude Bot commented May 19, 2026

Code Review: fix(tethys): resolve qualified refs from import-less files (rivets-044i)

Overview

This PR closes the qualified-path resolution gap that rivets-dn35 (PR #69) explicitly deferred. Import-less files now reach qualified_module_fallback — a new Pass-2 fallback that splits a qualified ref_name at :: boundaries, translates each prefix to a file_id via resolve_module_path, and queries search_symbol_by_qualified_name_in_file on the tail. The algorithm favors implicit-crate interpretation over as-written to honor Rust's submodule-shadows-extern-crate scoping rule.

The full gilfoyle workflow (probe → falsifiable design → budgeted plan → checkpointed build) is present in .rivets-044i/, and three rounds of review decisions are documented. The approach is additive — no schema change, no new public API, no behavior change for unqualified refs.


Production code (resolve.rs)

Strengths

  • Defense-in-depth segments.len() < 2 guard is correct even though the single call site's is_qualified gate guarantees it at runtime. The doc comment explaining why it exists ("Hand-rolled future callers...") is exactly the right level of annotation.
  • current_file_path = None is declined gracefully rather than panicked. The comment explaining that Pass-2 sometimes lacks a path for synthetic refs makes the reasoning durable.
  • The interpretation ordering (implicit-crate-first for non-prefixed first-segments, as-written otherwise) faithfully encodes Rust's scoping rule. The submodule_shadows_workspace_crate adversarial test pins this invariant independently of the doc comment.
  • Structured trace! logging at both the resolve and non-resolve exit paths.

Minor observations

  1. (*s).to_string() explicit dereference — I see this has been rejected twice already (R1-S6, R3-3) on the grounds that clippy doesn't flag it. That's the right call in this codebase where clippy pedantic is the machine oracle. Flagging it a third time only for completeness: s.to_string() would compile identically via auto-deref from &&str → &str. Not a blocker.

  2. tail allocation before the file_id gatetail = segments[split..].join("::") is computed on every iteration whether or not file_id resolves. Rejected in R2-2 / R3-5 with sound justification (bounded by segments.len() ≤ ~6, sub-microsecond in practice). Noted only because two reviewers flagged it; the rejection stands.

  3. trace! field naming at the non-resolve exitsegments = segments.len() is a count logged with the name segments. A reader might briefly wonder whether it's the full vector. segment_count = segments.len() would be unambiguous, though this is trivially minor.


Tests (pass2_qualified_paths.rs)

8 tests covering all enumerated input shapes:

Test Shape defended
submodule_qualified_call_resolves s-submod — implicit-crate interpretation
workspace_crate_prefixed_call_resolves s-wscrate — as-written workspace-crate arm
submodule_shadows_workspace_crate adversarial — interpretation order must honor submodule-shadows-extern-crate
qualified_external_crate_stays_unresolved s-extern — must not phantom-resolve; std_helper lookalike defeats partial-string-match bugs
qualified_crate_prefix_resolves s-crate — filesystem trap src/crate/sub_044i.rs pins the prefix[0]=="crate" skip gate
longest_prefix_wins_over_shorter loop order — two competing matching files, correct binding requires reverse iteration
prefix_resolves_but_tail_missing_stays_unresolved graceful non-resolution when prefix resolves but tail symbol absent
self_and_super_paths_resolve_via_as_written s-self / s-super — as-written arm; documents known super:: filesystem-walk divergence, now tracked as rivets-nkjd

Strengths

  • Every test includes a negative phantom-detection assertion alongside the positive resolution assertion. This is stronger than simple "did it resolve?" checks — it verifies where the ref binds. The workspace_crate_prefixed_call_resolves fix from R2-1 (replacing the vacuous disjunction with a f_target.path != 'crate_a/src/lib.rs' phantom query) is correct and important.
  • Oracle precondition checks (definition_exists == 1, other_exists == 1) prevent false negatives by confirming the fixture is indexed as expected before asserting resolution outcome.
  • The longest_prefix_wins_over_shorter fixture is notably clever: it constructs a workspace where both a shorter and a longer prefix resolve, and uses distinct target files and symbol types (method vs. free fn) to prove which one won. Loop-order bugs cannot hide here.
  • The qualified_crate_prefix_resolves trap directory (src/crate/sub_044i.rs) is exactly the right adversarial fixture for the prefix[0]=="crate" gate.

One gap worth noting

The self_and_super_paths_resolve_via_as_written test documents a pre-existing super:: semantic divergence (tethys walks to grandparent directory; Rust spec says parent's parent module). The test is written to match tethys's actual behavior — correctly — and the divergence is now tracked in rivets-nkjd. When rivets-nkjd closes, this fixture will need updating. The docstring says as much, which is the right pattern. Just flagging it so reviewers are not surprised when the super_resolved assertion someday starts testing a different file path.


Performance & regressions

The PR table shows a −10% indexing wall time improvement post-fix. The explanation (resolving more refs earlier trims downstream work) is plausible and the data is backed by hyperfine output in .rivets-044i/. Phantom rate stays at 0.00%, ambiguity violations stay at 0 — both fences pass as expected.

The design acknowledges the per-ref syscall budget exceeds the 10^3 cap by ~10× with the justification that .exists() calls are OS-cached after the first access. This is a reasonable approximation for warm-cache indexing (the dominant production use case), but cold-cache first-run scenarios (CI agents, fresh clones) could see a higher relative cost. That optimization is deferred to rivets-bjdn.


Audit trail quality

The .rivets-044i/ directory is the strongest part of this PR. The oracle/probe separation, falsification table with run status, three rounds of review decisions with clear accept/reject rationale, and the commitment to filing tracker entries (R3-2 → rivets-nkjd) rather than leaving undocumented deferrals — all of this reflects the standard the CLAUDE.md diagnostic-directory convention sets, executed well.


Summary

This is a clean, well-bounded fix with excellent test coverage and a rigorous audit trail. The three minor observations above are all non-blocking; the production code is correct and the test suite is meaningfully adversarial. Approve.

@dwalleck dwalleck merged commit 9c54b17 into main May 19, 2026
14 checks passed
@dwalleck dwalleck deleted the feat/rivets-044i-qualified-paths branch May 19, 2026 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant