Extract try_canonicalize helper in package.rs (BT-2149)

[jamesc]

Summary

Extract a private try_canonicalize(path: &Path) -> PathBuf helper in crates/beamtalk-core/src/project/package.rs to replace 5 identical inline occurrences of std::fs::canonicalize(p).unwrap_or_else(|_| p.to_path_buf()).

Changes

• Added try_canonicalize helper with doc comment explaining fallback semantics
• Replaced all 5 call sites in find_package_root, collect_package_source_files_with_errors, and resolve_extraction_files
• Pure mechanical refactor — zero behavior change

Verification

• All 3347 unit tests + 31 doc tests pass (cargo test -p beamtalk-core)
• cargo clippy clean (no warnings)
• cargo fmt --check clean

Linear Issue

https://linear.app/beamtalk/issue/BT-2149/extract-try-canonicalize-helper-in-packagers-5x-duplication

https://claude.ai/code/session_01HwXCZnFJb8Ckp75DKFSzsb

---

Generated by Claude Code

Summary by CodeRabbit

• Refactor
  • Standardized path canonicalization to improve package root discovery, file selection, and deduplication behavior—reducing duplicate or missing files during operations.
  • No changes to public APIs or visible interfaces; behavior improvements are internal and aim for more reliable package handling.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f4b1d258-2bcd-4985-aafe-b166461fea4a

📥 Commits

Reviewing files that changed from the base of the PR and between 8cbb58c and e461b9c.

📒 Files selected for processing (1)

• crates/beamtalk-core/src/project/package.rs

---

📝 Walkthrough

Walkthrough

Refactors internal path canonicalization in crates/beamtalk-core/src/project/package.rs by adding try_canonicalize(&Path) -> PathBuf and replacing inline canonicalize-with-fallback calls in find_package_root, collect_package_source_files_with_errors, and resolve_extraction_files.

Changes

Path Canonicalization Consolidation

Layer / File(s)	Summary
Helper Definition crates/beamtalk-core/src/project/package.rs	Add try_canonicalize(&Path) -> PathBuf that returns std::fs::canonicalize(path) on success or the original path.to_path_buf() on error.
Root Discovery crates/beamtalk-core/src/project/package.rs	find_package_root now derives its canonical path using try_canonicalize(start) instead of an inline canonicalize(...).unwrap_or_else(...) pattern.
Source Collection Dedup Key crates/beamtalk-core/src/project/package.rs	collect_package_source_files_with_errors computes its deduplication key with try_canonicalize(&f) rather than ad-hoc canonicalize-with-fallback code.
Extraction Target Set crates/beamtalk-core/src/project/package.rs	resolve_extraction_files builds target_set from source_files using try_canonicalize for each entry and canonicalizes collected package files into pkg_canonical via try_canonicalize.
Explicit Target Membership/Dedup crates/beamtalk-core/src/project/package/package.rs (same file)	When deciding whether to append explicitly targeted files, membership/dedup comparisons use try_canonicalize(f) against the canonicalized package set.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: extracting a try_canonicalize helper function into package.rs.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch BT-2149-extract-try-canonicalize

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

Refactors crates/beamtalk-core/src/project/package.rs by extracting a small private helper to centralize the “best-effort canonicalize” behavior used during package-root discovery and source-file deduplication.

Changes:

• Added a private try_canonicalize(&Path) -> PathBuf helper that falls back to the original path on std::fs::canonicalize errors.
• Replaced repeated inline canonicalize(...).unwrap_or_else(...) call sites in find_package_root, collect_package_source_files_with_errors, and resolve_extraction_files.

[coderabbitai]

🧹 Nitpick comments (1)

crates/beamtalk-core/src/project/package.rs (1)

141-149: ⚡ Quick win

Dedup appended targets by updating pkg_canonical during the loop.

If source_files contains duplicate external paths, they can be pushed multiple times because pkg_canonical is computed once and never updated after push.

Proposed tweak

-            let pkg_canonical: HashSet<PathBuf> =
+            let mut pkg_canonical: HashSet<PathBuf> =
                 pkg_files.iter().map(|p| try_canonicalize(p)).collect();
@@
             for f in source_files {
                 let key = try_canonicalize(f);
                 if !pkg_canonical.contains(&key) {
+                    pkg_canonical.insert(key);
                     pkg_files.push(f.clone());
                 }
             }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/beamtalk-core/src/project/package.rs` around lines 141 - 149, The loop
that ensures explicitly-targeted files are included uses a snapshot HashSet
pkg_canonical computed once, so pushing a new file into pkg_files can still
allow duplicates when source_files contains the same external path multiple
times; update pkg_canonical as you append (or check/insert into the set instead
of only checking) so duplicates are prevented: when iterating source_files,
compute key = try_canonicalize(f), if pkg_canonical does not contain key then
push f.clone() into pkg_files and also insert key into pkg_canonical (using the
same try_canonicalize result) to keep the set current.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@crates/beamtalk-core/src/project/package.rs`:
- Around line 141-149: The loop that ensures explicitly-targeted files are
included uses a snapshot HashSet pkg_canonical computed once, so pushing a new
file into pkg_files can still allow duplicates when source_files contains the
same external path multiple times; update pkg_canonical as you append (or
check/insert into the set instead of only checking) so duplicates are prevented:
when iterating source_files, compute key = try_canonicalize(f), if pkg_canonical
does not contain key then push f.clone() into pkg_files and also insert key into
pkg_canonical (using the same try_canonicalize result) to keep the set current.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8b3663de-2746-4a6f-a0b2-33ca64eeaf8a

📥 Commits

Reviewing files that changed from the base of the PR and between 80cef8d and 8cbb58c.

📒 Files selected for processing (1)

• crates/beamtalk-core/src/project/package.rs