refactor(tethys): extract Tethys::src_root_for_file helper (rivets-6jxv)

[dwalleck]

Summary

• Dedupes the per-file crate_root derivation across three production sites by extracting a single pub(crate) fn crate_root_for_file(file, caller) -> PathBuf on Tethys.
• Sites converted:
  • resolve.rs::resolve_refs_for_file
  • indexing.rs::compute_dependencies
  • indexing.rs::compute_dependencies_from_stored
• Side benefit: the three sites previously diverged in their fallback debug! shape — one included a file_id field, the other two embedded the caller name in the message string. The helper unifies them with a structured operation = caller field, matching the project's "structured fields over string interpolation" convention from CLAUDE.md.

Net diff: -46 / +49 lines across 3 files (3 inline derivations × ~12 lines removed; 1 helper ~25 lines incl. doc-comment added).

Naming clarification

The helper's name crate_root_for_file reflects the variable name at the three call sites and matches the rivets-6jxv acceptance text verbatim. It's visually close to the pre-existing Tethys::get_crate_root_for_file (which returns the crate's directory, i.e. Cargo.toml's parent) but has distinct semantics — the new helper returns CrateInfo::src_root() (lib_path-derived) with a parent-dir sentinel fallback. The doc-comment on the new method has an explicit Naming note block flagging the differentiation for future maintainers.

If reviewers prefer a clearer name (e.g. src_root_for_file or module_resolution_anchor_for_file), it's a 4-instance rename — happy to apply in a follow-up commit.

Acceptance criteria (from rivets-6jxv)

• Tethys::crate_root_for_file helper extracted with consistent caller-name parameter
• resolve.rs::resolve_refs_for_file uses the helper
• indexing.rs::compute_dependencies uses the helper
• indexing.rs::compute_dependencies_from_stored uses the helper
• All tethys tests still pass — 627 pass (up from 627 baseline; no new tests added because this is pure dedup of behavior already covered by existing integration tests)
• cargo clippy --all-targets --all-features -- -D warnings clean
• cargo fmt --check clean (verified locally pre-commit)

ycaq epic status post-merge

If this merges, all 6 acceptance criteria for rivets-ycaq are met: lcb6 (#65), 0gom (#66), 3d0s (#67), measurement (#68), dn35 (#69), i8qn (#70), 6jxv (this PR). The resolver-correctness epic is closeable.

Test plan

• CI: full workspace build / test / lint / cargo-deny across Linux+macOS+Windows + claude-review
• No new tests needed: the 3 call sites are exercised by resolver_routing.rs, module_path_integration.rs, cargo_discovery.rs, file_deps_corroboration.rs, etc., all passing post-refactor

[claude]

Code Review

Clean refactor overall — the deduplication is justified (three identical 12-line blocks → one canonical implementation) and the structured logging alignment matches project conventions. A few observations:

---

Substantive: file_id diagnostic field dropped in resolve_refs_for_file

The original resolve.rs fallback log had a file_id structured field:

debug!(
    file_id = %file_id,
    file = %current_file_path.display(),
    "File not in any known crate; using file parent as sentinel crate_root"
);

The new helper emits only operation + file. Since file_id is the primary correlation key when tracking resolver behaviour back to a specific DB row, losing it from the fallback path could make orphan-file warnings harder to diagnose in practice. The PR description calls this "no behavioral changes" but the log output did change.

Options:

1. Add a post-call log at the resolve_refs_for_file call site that emits file_id at trace! level when the helper is likely to have triggered the fallback (e.g., when crate_root == current_file_path.parent()).
2. Accept the tradeoff — file = %...display() is usually enough to identify the file, and the caller name now replaces the site-specific context. Fine if the team agrees.

Worth an explicit decision either way rather than a silent change.

---

Minor: naming tension worth acknowledging

The doc comment does the right thing by flagging the difference from get_crate_root_for_file, but the name crate_root_for_file still implies "root of the crate" when it actually returns the source root (CrateInfo::src_root(), not the crate dir). The existing get_crate_root_for_file is the "real" crate-root getter.

Consider src_root_for_file or module_anchor_for_file to make the distinction obvious at the call site, not just in the doc comment. Not a blocker, but may prevent future confusion when someone reaches for the wrong one.

---

Nit: doc comment length vs. utility

The doc comment is comprehensive and the naming note is genuinely useful. One observation: the paragraph on crate::* semantic no-ops for orphan files is already captured by the call-site comment in resolve.rs. If the doc comment is the authoritative source, the call-site prose comment might as well trim to just the helper reference (which it now does — so this is actually already handled well, noting it only to confirm the decision looks intentional).

---

Positive callouts

• cargo::get_crate_for_file(file, &self.crates) — accessing self.crates directly rather than via self.crates() getter inside the method is correct (avoids a double-borrow path that would appear if crates() returned &[CrateInfo] from self).
• &'static str for caller is the right call: zero allocation, clear intent, and plays nicely with tracing's field infrastructure. #[track_caller] would give file+line, not a readable operation name, so the explicit string is better here.
• pub(crate) visibility is appropriate — all three call sites are within the tethys lib crate, not in the bin or external test targets, so the CLAUDE.md pub-vs-pub(crate) gotcha doesn't apply.

---

Summary: Approve-with-suggestion. The file_id loss is the only substantive point; the naming is worth discussing but not blocking. The rest is clean.

[gemini-code-assist]

Code Review

This pull request centralizes the logic for determining a file's source-root anchor by introducing a new helper method, crate_root_for_file, and refactoring call sites in indexing.rs and resolve.rs. The review feedback suggests renaming this method to entry_point_for_file to avoid confusion with the existing get_crate_root_for_file method and to better align with Rust module resolution rules, which resolve paths to entry point files rather than source directories. Suggestions were also provided to update the associated log messages and call sites to maintain consistency with this renaming.

[gemini-code-assist]

The name crate_root_for_file is confusingly similar to the existing get_crate_root_for_file method, which returns the crate's directory. Furthermore, project rules for Rust module resolution require that single-segment paths resolve to the crate's entry point file (e.g., lib.rs) rather than the source directory. Renaming this to entry_point_for_file better reflects its purpose as a file-level anchor for resolution and avoids confusion.

Suggested change

	pub(crate) fn crate_root_for_file(&self, file: &Path, caller: &'static str) -> PathBuf {
	pub(crate) fn entry_point_for_file(

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.

[gemini-code-assist]

If the method is renamed to reflect its purpose as an entry point provider, the log message should also be updated for consistency.

Suggested change

	}
	"File not in any known crate; using file parent as sentinel entry_point"

[gemini-code-assist]

Renaming the helper call to entry_point_for_file to align with the suggested method rename in lib.rs.

        let crate_root = self.entry_point_for_file(current_file, "compute_dependencies");

[gemini-code-assist]

Renaming the helper call to entry_point_for_file to align with the suggested method rename in lib.rs.

            self.entry_point_for_file(current_file, "compute_dependencies_from_stored");

[gemini-code-assist]

Renaming the helper call to entry_point_for_file to align with the suggested method rename in lib.rs.

Suggested change

	let crate_root = self.crate_root_for_file(&current_file_path, "resolve_refs_for_file");
	let crate_root = self.entry_point_for_file(&current_file_path, "resolve_refs_for_file");

[claude]

Code Review — PR #71: refactor(tethys): extract Tethys::crate_root_for_file helper

Overview

Clean dedup refactor. The three call sites had identical logic with only superficial differences (one used self.crates(), two used &self.crates; one had a file_id field in the fallback log, two didn't). Extracting them into a single helper is the right call — the three sites will now stay in sync automatically if the sentinel fallback semantics ever need updating.

---

What's Good

caller: &'static str parameter — zero allocation at the call sites, the caller names are string literals baked into the binary. Good pattern for this kind of structured log enrichment.

Structured logging unification — the three sites previously had divergent fallback log shapes. Unifying to operation = caller matches the CLAUDE.md convention ("structured fields over string interpolation") and makes the logs filterable by operation name.

Doc-comment quality — the explicit "Naming note" block and [Tethys::get_crate_root_for_file] cross-link are exactly what a future reader needs. Appreciated.

pub(crate) is correct here — this is an internal helper used only within the tethys crate's own compilation unit (lib.rs, resolve.rs, indexing.rs). No test files are calling it directly, so the CLAUDE.md pub(crate) gotcha about lib-crate visibility doesn't apply.

---

Issues Worth Discussing

1. Lost file_id field in the resolve.rs fallback log (minor)

The original resolve.rs fallback included file_id = %file_id alongside file = %current_file_path.display(). The helper drops file_id.

// Before (resolve.rs):
debug!(
    file_id = %file_id,
    file = %current_file_path.display(),
    "File not in any known crate; ..."
);

// After (helper):
debug!(
    operation = caller,
    file = %file.display(),
    "File not in any known crate; ..."
);

file_id is the database FK — when debugging a resolver anomaly you'd correlate this log line against the files table, and having the ID makes that a direct lookup rather than a path search. The PR description frames the unified shape as a pure improvement ("structured operation = caller field"), but it's actually a modest information downgrade for the resolve call site.

Suggestion: If you want to preserve it without complicating the helper's signature, you could keep it as a local annotation at the call site:

let crate_root = self.crate_root_for_file(&current_file_path, "resolve_refs_for_file");
// file_id is available here if the fallback fires — the debug! inside the helper
// doesn't have it, but it's rare enough that this is acceptable

Or just document the tradeoff in the PR — it's a conscious choice, not an oversight, and the file path is human-readable anyway.

2. Naming — strong vote for src_root_for_file (non-blocking)

The PR description acknowledges the crate_root_for_file / get_crate_root_for_file ambiguity and offers src_root_for_file as an alternative. I think that alternative is clearly better:

• crate_root_for_file implies "the root of the crate containing this file" — which is exactly what get_crate_root_for_file already returns (Cargo.toml's parent).
• src_root_for_file maps directly to what it actually returns: CrateInfo::src_root() (i.e., the lib_path.parent()-derived directory), which is distinct from the crate directory.

The doc-comment's "Naming note" is helpful, but it's compensating for a confusing name rather than avoiding the confusion in the first place. Since this is pub(crate), a 4-instance rename is low risk.

My recommendation: rename to src_root_for_file in a follow-up commit — worth the 4-line churn to reduce future confusion.

---

Minor Observations

&self.crates vs self.crates(): The helper uses cargo::get_crate_for_file(file, &self.crates) (direct field access), while the old indexing.rs used self.crates() (the public accessor). Both coerce to &[CrateInfo] — no behavioral difference, just noting the minor inconsistency was silently resolved by the refactor.

No new tests needed: Confirmed. The three call sites are exercised by resolver_routing.rs, module_path_integration.rs, cargo_discovery.rs, and file_deps_corroboration.rs. A pure dedup refactor with passing integration test coverage is sufficient.

---

Summary

This is a solid, well-motivated refactor. The two real items are:

1. The lost file_id log field is a minor but real information downgrade for the resolve call site — worth a conscious decision either way.
2. src_root_for_file is a materially clearer name than crate_root_for_file and worth a follow-up rename.

Neither is a blocker. The code is correct, clippy-clean, and the doc-comment work is above average. Approve with the naming suggestion as a follow-up.

[claude]

Code Review — PR #71: Extract Tethys::crate_root_for_file

Overall this is a clean, well-motivated refactor. The deduplication is real (~36 lines removed across 3 copy-paste blocks), the structured-logging unification matches CLAUDE.md conventions, and the doc comment is thorough. A few items worth discussing before merge:

---

Observability regression (minor)

The original resolve.rs call site included file_id = %file_id in the fallback debug! log — the other two indexing.rs sites didn't. The helper drops file_id, and the inline comment acknowledges this:

"Recover the file_id via path lookup if needed during incident triage."

That's extra manual work during a live triage. Consider whether the helper should accept an optional file_id: Option<u64> (or just a second optional extra_fields callback) so the resolve.rs caller can restore its field without breaking the log-shape parity goal. Even keeping file_id only in resolve.rs's call-site comment as something to add back to the helper later would be worth noting in the issue.

---

Naming — worth resolving now, not later

The PR description already flags the crate_root_for_file vs get_crate_root_for_file ambiguity and offers alternatives (src_root_for_file). Given that:

• This is a pub(crate) method (only 4 usages)
• The doc comment needs an explicit Naming note block to distinguish it
• The existing name genuinely describes a different thing (Cargo.toml parent vs lib_path-derived anchor)

src_root_for_file would be clearer and eliminate the need for the naming-note entirely. Happy either way, but "we'll rename in a follow-up" is easy to forget once the PR is merged and the issue is closed.

---

pub(crate) visibility — CLAUDE.md flag

CLAUDE.md explicitly notes:

"pub(crate) in a lib crate is NOT visible to the bin crate's tests in the same package."
"If a constructor needs to be reachable from #[cfg(test)] blocks in a bin module of a lib+bin package (e.g. tethys), keep it pub."

No new tests call this helper directly, so pub(crate) is fine right now. But tethys is a lib+bin package, so any future test that needs this from a #[cfg(test)] block in the tethys bin will silently not find it. A brief note in the doc comment (or a // NOTE: promote to pub if bin-crate tests need this) would save the next contributor a head-scratch. Not a blocker, just a courtesy.

---

What's working well

• The inline comment explaining why cargo::get_crate_for_file is used instead of self.get_crate_for_file (no canonicalization / wasted syscalls) is exactly the kind of "WHY" comment CLAUDE.md calls for.
• caller: &'static str avoids allocations at call sites and is the right choice for log labels.
• The operation = caller structured field replaces three divergent log shapes correctly.
• The refactor is behavior-preserving: same src_root() → parent-sentinel fallback logic at all three sites.

---

Summary

Issue	Severity
Lost file_id field on resolve.rs fallback log	Minor / nice-to-have to restore
Naming ambiguity with get_crate_root_for_file	Minor, but worth fixing before merge given the low cost
pub(crate) note for future test authors	Nit

The logic is correct, CI should stay green, and the existing integration tests cover all three sites. Happy to approve with the naming resolved.

[claude]

Code Review

Overall this is a clean, well-motivated refactor. Eliminating three near-identical 12-line blocks while improving log-field consistency is a good use of a helper. A few notes below.

---

The good

• src_root_for_file name is right. The PR body correctly notes the tension with crate_root_for_file (the acceptance-criteria text) vs. get_crate_root_for_file (the existing method). src_root_for_file is the clearest of the three options — it avoids the ambiguity entirely. I'd recommend updating the rivets-6jxv acceptance checklist to reflect the final name.
• caller: &'static str is the right type. Zero allocation, enforced compile-time string literal. All three call sites comply naturally.
• Structured operation = caller field follows the project's tracing convention (structured fields over string interpolation). The log shape is now consistent across all three sites.
• Doc comment quality. The explicit "Not to be confused with get_crate_root_for_file" block and the explanation of the fallback semantics are exactly what future maintainers need. Good precedent.

---

Issues

1. Minor observability regression in resolve.rs (low severity, acknowledged)

The old resolve.rs fallback debug log included file_id = %file_id. The helper can't carry that because file_id isn't on the file path:

// old
debug!(
    file_id = %file_id,
    file = %current_file_path.display(),
    "File not in any known crate; using file parent as sentinel crate_root"
);

The inline comment in the diff documents the workaround ("recover via path lookup if needed during incident triage"), which is acceptable for a debug-only log. But it's worth noting that if orphan-file warnings become a frequent triage path, the helper's signature would need an Option<i64> for file_id or the resolver would need to wrap the helper call with its own debug! after the fact.

No action required for this PR — just keeping it visible.

---

2. Naming inconsistency: src_root at call sites, crate_root in ResolverContext (nit)

In resolve.rs the local is now src_root, but the struct field it feeds is still named crate_root:

let src_root = self.src_root_for_file(...);
// ...
crate_root: &src_root,   // <-- struct field still uses old name

This isn't a bug, but a reader will do a double-take. If ResolverContext::crate_root is in scope for renaming (it's presumably pub(crate) or internal), updating it to src_root would make the data flow read naturally. Happy to treat as a follow-up if touching ResolverContext is out of scope here.

---

3. Inline comment in the helper body (style suggestion)

The comment explaining self.crates vs self.crates() is useful context for the avoidance of a syscall, but it's quite long for an inline comment on a single if let arm. Consider condensing to:

// Intentionally skips self.get_crate_for_file's canonicalize() — callers
// work with pre-canonicalized paths and the extra syscall per file adds up.
if let Some(crate_info) = cargo::get_crate_for_file(file, &self.crates) {

That keeps the "why" without re-narrating the whole syscall story.

---

Summary

The core extraction is correct and the test coverage rationale (pure dedup of already-exercised behavior) is sound. The only actionable items are:

• The crate_root → src_root rename in ResolverContext (nit, follow-up is fine)
• Condensing the inline comment in the helper (style)

The file_id observability loss is a known tradeoff that's adequately documented. LGTM pending the above.

[claude]

Code Review: refactor(tethys): extract Tethys::crate_root_for_file helper

Overall this is a clean, well-motivated refactor. The extraction is mechanically correct, the doc comment on the helper is thorough, and the **Not to be confused with** disambiguation block proactively addresses the naming hazard between src_root_for_file and get_crate_root_for_file. Net -60/+60 with no behavioral change is exactly what a dedup PR should look like.

---

Actionable: Stale doc comments in indexing.rs

The doc comments on compute_dependencies and compute_dependencies_from_stored still reference the old local variable name crate_root and describe the derivation logic inline — but that logic now lives in Tethys::src_root_for_file. The cross-reference at compute_dependencies_from_stored also says "see compute_dependencies for the crate_root derivation rationale," which points to an explanation that itself uses the stale term.

These should be updated to say src_root and redirect to Tethys::src_root_for_file for the contract. Stale doc comments are the main risk with dedup refactors, since the prose doesn't get updated by the mechanical rename.

---

Non-blocking: file_id dropped from the orphan-file debug! log in resolve.rs

The old orphan-file log included a file_id field; the extraction drops it for log-shape parity with the indexing.rs sites. This is disclosed in the code comment and the PR description calls it out explicitly — so it's a deliberate tradeoff, not an oversight.

That said, since file_id is available in scope at the call site and costs nothing, it would make incident triage easier without requiring a DB round-trip. One option: call the helper, then emit one additional structured tracing field at the resolve.rs call site only. But if log-shape consistency is the goal, the current approach is defensible. Just noting it.

---

Confirmed correct

• pub(crate) visibility: Correct for the current usage pattern. CLAUDE.md's warning about pub(crate) in lib+bin crates applies when bin-crate tests need access — no such tests exist here yet. The existing relative_path precedent in lib.rs follows the same pattern.
• caller: &'static str parameter: Idiomatic — all call sites pass string literals, no allocation, no issue.
• No new unit tests: The helper is a two-branch if-let delegating to already-tested functions (cargo::get_crate_for_file, CrateInfo::src_root). Integration coverage via the three call sites is sufficient. The acceptance checklist documents this explicitly.
• Naming tension (src_root_for_file vs get_crate_root_for_file): The current name is clearer and shorter; the disambiguation block in the doc comment handles the confusion risk adequately.

---

Recommend: Approve once the stale crate_root references in the indexing.rs doc comments are updated to src_root / Tethys::src_root_for_file.

[claude]

Code Review — PR #71: refactor(tethys): extract Tethys::src_root_for_file helper

Overview

This PR deduplicates per-file src_root derivation logic that was copy-pasted across three production sites in resolve.rs and indexing.rs. The extracted helper centralises the logic, unifies the fallback debug-log shape, and renames crate_root → src_root throughout ResolveContext for semantic accuracy. Net: cleaner, DRY, and the structured-logging alignment with CLAUDE.md is a genuine improvement.

---

What's working well

• Clean dedup. Three ~12-line blocks become one 25-line helper — the right call for logic this stable.
• Structured logging improvement. The unified operation = caller field instead of embedding the caller name in the message string matches the project's convention from CLAUDE.md.
• caller: &'static str type. Correct choice — all three callers pass string literals, so 'static is free and avoids heap allocation.
• ResolveContext rename. crate_root → src_root is semantically accurate (it's the lib-path-derived source root, not the crate directory) and done consistently throughout the struct and all its field accesses.
• Doc comment on the helper. The "Not to be confused with get_crate_root_for_file" block and the fallback-sentinel rationale are exactly the kind of non-obvious WHY that's worth writing down.

---

Issues

1. PR description/title don't match the implementation (misleading)

The PR title says extract Tethys::crate_root_for_file helper and the description says the helper name is crate_root_for_file — but the actual diff introduces src_root_for_file. The acceptance criteria checkbox also reads:

Tethys::crate_root_for_file helper extracted with consistent caller-name parameter

This suggests the rename happened during development but the PR description was never updated. Not a code problem, but reviewers skimming the title will look for the wrong symbol.

Suggestion: Update the PR title and the acceptance criterion to src_root_for_file to match the code.

2. Inline comment at the resolve.rs call site references the issue ID

// See `Tethys::src_root_for_file` for the contract. The orphan-file
// `debug!` it emits previously included a `file_id` field at this
// call site; the rivets-6jxv extraction dropped that for log-shape
// parity with the indexing.rs sites. Recover the file_id via path
// lookup if needed during incident triage.

CLAUDE.md explicitly calls this out as an anti-pattern: "Don't reference the current task, fix, or callers … since those belong in the PR description and rot as the codebase evolves." The rivets-6jxv reference will be meaningless to a future reader.

Suggestion: Trim this to:

// `file_id` is not forwarded to the helper's debug log; recover it via
// path lookup during incident triage if needed.

The first sentence (See src_root_for_file for the contract) is redundant — the call site already shows that.

3. file_id field silently dropped from the orphan-file debug log

The original resolve_refs_for_file debug log included file_id = %file_id. The new helper emits only operation and file. This is a deliberate observability tradeoff acknowledged by the PR, but it's worth a note: orphan-file warnings from the resolver previously let you go directly from a log line to the DB row. That correlation now requires a join on path.

Not a blocker — the PR acknowledges it and the fallback path is uncommon — but worth explicitly surfacing to the author.

---

Suggestions (non-blocking)

4. Consider a unit test for the helper's two branches

The PR correctly notes the three call sites are covered by existing integration tests. That said, a targeted unit test for src_root_for_file itself would:

• Pin the fallback sentinel contract (file's parent, or workspace root if at filesystem root)
• Guard against regressions if cargo::get_crate_for_file signature changes

Something minimal:

#[test]
fn src_root_for_file_falls_back_to_parent_for_unknown_file() {
    let tethys = /* minimal fixture */;
    let orphan = tethys.workspace_root.join("not_a_crate/foo.rs");
    let result = tethys.src_root_for_file(&orphan, "test");
    assert_eq!(result, tethys.workspace_root.join("not_a_crate"));
}

---

Summary

Correctness	✅ No semantic changes — pure dedup
Logging	✅ Improved (structured fields, unified shape)
Naming	✅ src_root is more accurate than crate_root
PR description accuracy	⚠️ Title/description use old name crate_root_for_file
Comment convention	⚠️ Issue-ID reference in inline comment
Test coverage	ℹ️ Existing integration tests cover it; a unit test would strengthen it

The code changes themselves are correct and an improvement. The two items to fix before merge are the PR description mismatch (cosmetic but confusing) and the inline comment anti-pattern (easy one-line trim).

[claude]

Code Review — PR #71: refactor(tethys): extract Tethys::src_root_for_file helper

Clean, well-motivated refactor. The three call sites were identical modulo superficial surface differences; extracting them ensures future fallback-semantics changes propagate automatically. Overall this is approvable with a couple of minor items.

---

Positive callouts

• &'static str for caller is the right call — zero allocation, clear intent, and the string literal shows up verbatim in structured log output without any allocation per call.
• pub(crate) visibility is correct; all three call sites are within the tethys lib, so the CLAUDE.md pub(crate)-vs-pub gotcha doesn't apply here.
• Accessing self.crates directly inside the helper (rather than through self.crates()) avoids a double-borrow issue and skips the canonicalize step that get_crate_for_file applies. The inline comment explaining the skip is exactly the kind of "non-obvious WHY" comment CLAUDE.md asks for.
• Naming: src_root_for_file is clearer than the originally proposed crate_root_for_file. The **Not to be confused with** note in the doc comment is a good safety net for future readers who might reach for the wrong method.

---

Minor: Decorative separator comments in the test block

The new test section adds:

    // ========================================================================
    // src_root_for_file Tests
    // ========================================================================

CLAUDE.md's extended guidelines list "Avoid decorative divider lines" as an explicit anti-pattern. The // === ... === separators appear elsewhere in the test module (pre-existing), but adding a new instance in this PR extends the inconsistency. Consider a simple // src_root_for_file heading comment or none at all.

---

Minor: PR description references old name

The description body still mentions crate_root_for_file (e.g. "Dedupes… by extracting a single pub(crate) fn crate_root_for_file") while the actual diff and PR title correctly use src_root_for_file. The code is right; the description just needs a quick edit before merging for archival accuracy.

---

file_id diagnostic field — explicit tradeoff acceptance

The previous review flagged this. The PR author added a call-site comment acknowledging it:

// `file_id` is not forwarded to the helper's `debug!`; recover it via
// path lookup during incident triage if needed.

That comment is fine as a rationale. My read: the tradeoff is acceptable — file = %...display() is enough to correlate with the DB row via a simple query, and adding a second log call at the call site to emit file_id would partially undo the dedup benefit. Accept as-is.

---

Summary

Two minor issues (decorative separator, stale PR description prose) and no substantive concerns. The refactor is correct, the test covers the previously-untested orphan-file branch, and the structured logging is now consistent across all three sites. Ready to merge after the description is tidied.

[claude]

Code Review — Current Diff State

The prior claude reviews referenced crate_root_for_file; the diff as it stands already uses src_root_for_file throughout — the naming concern from those reviews was pre-incorporated. Reviewing the current state:

---

What's good

• Deduplication is correct. Three semantically identical 12-line blocks → one 25-line canonical helper. No behavioral change.
• Naming is now clean. src_root_for_file doesn't shadow get_crate_root_for_file's meaning. The "Naming note" in the doc comment is belt-and-suspenders, which is fine.
• Structured logging unification. operation = caller replaces three divergent fallback log shapes. Matches CLAUDE.md's "structured fields over string interpolation" rule.
• pub(crate) is correct. All call sites (resolve.rs, indexing.rs, lib.rs tests) are within the tethys lib compilation unit. The CLAUDE.md pub(crate) gotcha doesn't apply here.
• caller: &'static str is the right type: zero allocation, all call sites pass string literals, plays well with tracing's field infrastructure.
• Unit test covers the orphan-file fallback path. Integration tests cover the success arm — the split is appropriate.

---

One remaining item: file_id not forwarded to the fallback debug!

The original resolve.rs fallback emitted file_id = %file_id. The helper drops it (acknowledged by the inline comment). This is an observable regression: file_id is the DB FK and makes the fallback log directly joinable against the files table during incident triage. The file path is still present and is human-readable, so this is not blocking — but it should be a conscious decision, not just noted and moved on.

Recommendation: Add a sentence to the PR description (or a follow-up commit) explicitly calling this a known tradeoff, so future readers of the commit history know it was intentional.

---

Verdict

Approve. The refactor is correct, the naming is now unambiguous, and the code is clean. The file_id item is the only remaining discussion point and is non-blocking.