From 1694908e9c4f4a651196d3d458066ded93e008c0 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Fri, 3 Jul 2026 18:47:23 -0400 Subject: [PATCH] =?UTF-8?q?fix(packages):=20basename-collision=20reject,?= =?UTF-8?q?=20SHA-256=20cache=20key,=20timeout=20thread=20join,=20commit?= =?UTF-8?q?=E2=86=92revision,=20dead-code=20(F-005/F-009=E2=80=93F-012)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Package-manager hardening sweep from the repo audit — one Medium + four Lows, all in src/packages/ (F-011 also renames across lua_bindings + tests). F-005 (Medium) — install dirs are named by package basename and require routes by basename, so two distinct packages `owner/magit` and `other/magit` collapse to one dir with most-recent-install silently winning. Reject a resolve plan that contains distinct names sharing a basename: new ResolveError::BasenameCollision + find_basename_collision() in into_plan (the one place holding every name at once). The loader's *intended* cross-scope override (project- vs user-scope, most-recent-first) is untouched — its test still passes. Namespace-preserving layout and cross-resolve install-time detection are named-deferred. F-009 (Low) — the fetch bare-mirror cache dir was keyed by 64-bit FNV-1a of the (attacker-adjacent) repo URL — trivially collidable. Swap to SHA-256 (sha2, already a dep for lockfile hashing). normalize_url still folds equivalent URLs to one entry; only the digest changes (re-clones once, it's a cache). F-010 (Low) — on a git subprocess timeout, run_with_timeout returned before joining the stdout/stderr drain threads (joined only on the normal path), leaving detached readers. Restructure to break the wait loop with a Result, reap the child on every path, and join both threads at one point before propagating. F-011 (Low) — ResolvedPackage.commit was documented "Full 40-character commit hash" but commit_for_tag() puts a tag string there (the resolver works against commit-ishes by design, deferring SHA resolution to the installer/lockfile). Rename the field to `revision` + honest doc. Compiler-driven rename hit exactly the ResolvedPackage sites; the Lua-visible "commit" record key is unchanged. F-012 (Low) — the topo sort built an indegree map, argued in comments it was backwards, and rebuilt it. Delete the dead first block + the meandering narration. Framing/as-built: docs/package-manager-hardening-framing.md. Validated: fmt clean; clippy --all-targets clean under both Lua flavors; 1436 lib unit tests pass (incl. new F-005/F-009 tests, the F-010 timeout test, and the loader override test). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_014TXbAwk27agwhrNNrhLi2U --- docs/package-manager-hardening-framing.md | 156 ++++++++++++++++++++++ src/lua_bindings.rs | 8 +- src/packages/fetcher.rs | 62 ++++++--- src/packages/lockfile.rs | 6 +- src/packages/resolver.rs | 150 ++++++++++++++++----- tests/m7_10_acceptance.rs | 2 +- tests/m7_5_acceptance.rs | 12 +- tests/m7_6_acceptance.rs | 2 +- 8 files changed, 331 insertions(+), 67 deletions(-) create mode 100644 docs/package-manager-hardening-framing.md diff --git a/docs/package-manager-hardening-framing.md b/docs/package-manager-hardening-framing.md new file mode 100644 index 0000000..30019fa --- /dev/null +++ b/docs/package-manager-hardening-framing.md @@ -0,0 +1,156 @@ +# Package-manager hardening — framing + as-built + +Five audit findings in `src/packages/`, taken as one sweep: one Medium +correctness issue (F-005) and four Lows (F-009–F-012) that range from a +security-adjacent cache-key upgrade to dead-code deletion. All in the +`pmacs` core crate; no wire-protocol or frontend change. + +- **F-005** (Medium) — install dirs are named by package *basename*, and + require-lookup routes by basename. Two distinct packages `owner/magit` + and `other/magit` collapse to one install dir; most-recent-install + silently wins. +- **F-009** (Low) — the fetch bare-mirror cache dir is keyed by 64-bit + FNV-1a of the URL — trivially collidable, and URLs are + attacker-adjacent. +- **F-010** (Low) — on a git subprocess timeout, `run_with_timeout` + returns before joining the stdout/stderr drain threads (they're joined + only on the normal path). +- **F-011** (Low) — `ResolvedPackage.commit` is documented "Full + 40-character commit hash" but `commit_for_tag()` puts a **tag string** + there; the resolver deliberately works against commit-ishes and defers + SHA resolution to the installer. +- **F-012** (Low) — the resolver's topological sort builds an `indegree` + map, reasons in comments that it's backwards, and rebuilds it — leaving + the first block dead. + +## What the recon established + +- **F-005 must not break the loader's *intended* override.** The loader + documents that a project-scope install may share a basename with a + user-scope one, and the Lua searcher picks most-recent-first *on + purpose* (`loader.rs:43`). So the collision to reject is the one with no + override semantics: **two distinct package names inside a single resolve + plan** sharing a basename — they'd collide on disk with no way to tell + which the caller meant. `into_plan` (`resolver.rs:678`) builds the plan + and holds every name at once — the one place that can see the collision. + The install marker (`.pmacs-install`) records only the commit, not the + canonical name, so cross-resolve install-time detection would need a + marker-format change — deferred. +- **F-009 has `sha2` already.** `sha2 = "0.10"` is a direct dep (M7.6 + lockfile hashing, `lockfile.rs:84`), so SHA-256 is a swap, not a new + dependency. The cache key is a dir name; changing it re-clones once + (it's a cache) — acceptable. +- **F-010 is reap-then-join.** After `kill()` + `wait()` the child's pipes + close, so the drain threads finish promptly and are safe to join. The + fix is to reap on *every* exit path and join at a single point. +- **F-011's field genuinely holds a commit-ish** and `ResolvePlan` is + never serialized (only feeds `Lockfile::from_plan`), so a rename is + serde-safe. Field access is concrete-typed, so a compiler-driven rename + touches exactly the `ResolvedPackage` sites, not the other `.commit` + fields (`ChosenTag`, the internal entry, `InstalledPackage`, + `LockedPackage`). + +## The rules + +**Q#PM1 — F-005: reject same-basename distinct names in one plan.** After +`into_plan` assembles `packages`, group by `package_basename(name)` +(`installer.rs:1046`, already `pub(crate)`); if any basename maps to more +than one distinct `PackageName`, return a new +`ResolveError::BasenameCollision { basename, names }`. This blocks the +install before it can silently shadow, without touching the loader's +cross-scope override behavior. Install-time (cross-resolve) detection is +named-deferred (needs the canonical name in the marker). + +**Q#PM2 — F-009: SHA-256 the cache key.** Replace `fnv1a_hex` with +`sha256_hex` (`sha2::Sha256`, full 64-char hex) for the bare-mirror dir / +lock names. Keep `normalize_url` in front (case/`.git`/trailing-slash +folding) so the *same* repo still maps to one entry; only the hash +function changes. Update the three hash unit tests. + +**Q#PM3 — F-010: reap on every path, join once.** Restructure +`run_with_timeout` so the wait loop `break`s with a `Result` — killing + reaping the child on the timeout and `try_wait`- +error paths — then joins both drain threads unconditionally and only then +propagates the error. No detached readers survive a timeout. + +**Q#PM4 — F-011: rename `commit` → `revision`, tell the truth.** Rename +the field and document it as "a commit-ish (a SHA, or a tag/branch name); +the installer/lockfile resolves it to a concrete SHA." Compiler-driven: +fix each flagged `ResolvedPackage` read (`resolver`/`lockfile`/ +`lua_bindings`). The Lua-visible record key stays `"commit"` (user API +unchanged); only the Rust contract gets honest. + +**Q#PM5 — F-012: delete the dead indegree block.** Remove the first +`indegree` construction (`resolver.rs:707-715`) and collapse the +three-paragraph "reset / no, backwards / cleaner" narration into one +comment stating the final algorithm (outgoing-edge counts + a `dependents` +reverse-adjacency, peeled by Kahn's). Pure cleanup — the existing +topo-sort tests are the guard. + +## Categorical bets + +- **Reject, don't guess (F-005).** With two same-basename packages in one + plan and no override semantics to disambiguate, a clear error beats + silently installing whichever lands last. A namespace-preserving layout + is the real answer later; failing loud is the right v0.1 floor. +- **Crypto hash for an adversary-adjacent key (F-009).** URLs are + effectively attacker-controlled; only a cryptographic digest resists a + *deliberate* cache-path collision. `sha2` is already in the tree. +- **A rename is worth it over a doc patch (F-011).** The name is the trap + ("trusts `.commit` as a SHA"); renaming removes it at the type level, + and the compiler makes the change safe and exhaustive. + +## Validation implication + +All five are unit-testable in-crate (no GPU, no daemon): a resolve plan +with a synthesized basename collision errors (F-005); `sha256_hex` is +stable/normalized (F-009); a timeout leaves no unjoined threads and still +returns `Timeout` (F-010 — assert via the existing timeout test path); +rename is compile-checked + existing resolve/lockfile tests (F-011); topo +order unchanged (F-012, existing tests). Runs under both Lua flavors in +CI. + +## As-built + +Landed as framed; all five in `src/packages/` (F-011 also touches +`lua_bindings.rs` + acceptance tests via the rename). No serde/wire change. + +- **F-005** (`resolver.rs`): new `ResolveError::BasenameCollision { + basename, names }`; a free `find_basename_collision(names)` (testable, + used by `into_plan`) groups the plan's names by `package_basename` and + returns the first basename with >1 distinct name. Two unit tests; the + existing `roster_lookup_picks_most_recent_on_basename_collision` loader + test still passes, confirming the intended cross-scope override is + untouched. +- **F-009** (`fetcher.rs`): `fnv1a_hex` → `sha256_hex` (`sha2::Sha256`, + 64-char hex) for the bare-mirror dir/lock key; `normalize_url` unchanged + in front. Hash test asserts stability, normalization, 64-hex shape, and + distinctness. +- **F-010** (`fetcher.rs`): `run_with_timeout` now `break`s the wait loop + with a `Result`, reaping the child (`kill` + `wait`) on both the timeout + and `try_wait`-error paths, then joins both drain threads at a single + point before propagating — no detached readers survive a timeout. The + existing `timeout_kills_long_running_command` test covers the path. +- **F-011** (`resolver.rs` + `lockfile.rs` + `lua_bindings.rs` + m7_5/m7_6/ + m7_10 tests): `ResolvedPackage.commit` → `revision`, documented as a + commit-ish. Compiler-driven rename hit exactly the `ResolvedPackage` + sites; the Lua-visible record key stays `"commit"`, and the other + structs' `commit` fields (`ChosenTag`, internal entry, `InstalledPackage`, + `LockedPackage`) were correctly left alone. +- **F-012** (`resolver.rs`): deleted the dead first `indegree` map + the + three-paragraph "reset/backwards/cleaner" narration; one comment now + states the actual algorithm. + +Validated: `cargo fmt` clean; `clippy --all-targets` clean under **both** +Lua flavors (luajit + lua54); 1436 lib unit tests pass, incl. the new +F-005/F-009 tests, the F-010 timeout test, and the loader override test. + +## Deferred (named) + +- **F-005 install-time collision detection.** Catching a collision across + *separate* resolves (install `owner/magit`, later `other/magit`) needs + the canonical name recorded in `.pmacs-install` and an installer check — + a marker-format change out of this batch's scope. +- **Namespace-preserving install layout.** The structural fix for F-005 + (install under `//`, require by canonical name) is a larger + design change; rejecting is the interim floor. diff --git a/src/lua_bindings.rs b/src/lua_bindings.rs index d9289e9..32d759b 100644 --- a/src/lua_bindings.rs +++ b/src/lua_bindings.rs @@ -3536,11 +3536,11 @@ fn do_install(lua: &Lua, spec: &InstallSpec, scope: &InstallScope) -> mlua::Resu } else { InstallSpec { address: rp.address.clone(), - pin: crate::packages::InstallPin::Commit(rp.commit.clone()), + pin: crate::packages::InstallPin::Commit(rp.revision.clone()), } }; let installed = installer - .install_at_commit(&install_spec, &rp.commit) + .install_at_commit(&install_spec, &rp.revision) .map_err(|e| mlua::Error::external(BindingError::from(e)))?; if let Some(parent) = installed.install_path.parent() { @@ -3749,13 +3749,13 @@ fn do_update(lua: &Lua, target: Option<&str>) -> mlua::Result { let pin_to_use = rp .top_level_pin .clone() - .unwrap_or_else(|| crate::packages::InstallPin::Commit(rp.commit.clone())); + .unwrap_or_else(|| crate::packages::InstallPin::Commit(rp.revision.clone())); let install_spec = InstallSpec { address: rp.address.clone(), pin: pin_to_use, }; let installed = installer - .replace_at_commit(&install_spec, &rp.commit) + .replace_at_commit(&install_spec, &rp.revision) .map_err(|e| mlua::Error::external(BindingError::from(e)))?; if let Some(parent) = installed.install_path.parent() { prepend_package_path(lua, parent)?; diff --git a/src/packages/fetcher.rs b/src/packages/fetcher.rs index de9298c..e2ec2d7 100644 --- a/src/packages/fetcher.rs +++ b/src/packages/fetcher.rs @@ -62,6 +62,7 @@ use std::process::{Child, Command, ExitStatus, Stdio}; use std::thread; use std::time::{Duration, Instant}; +use sha2::{Digest, Sha256}; use thiserror::Error; // --------------------------------------------------------------------------- @@ -160,7 +161,7 @@ impl Fetcher { /// processes (durable on-disk cache). pub fn fetch(&self, url: &str) -> Result { let normalized = normalize_url(url); - let hash = fnv1a_hex(&normalized); + let hash = sha256_hex(&normalized); let repo_path = self.cache_dir.join(format!("{hash}.git")); let lock_path = self.cache_dir.join(format!("{hash}.git.lock")); @@ -507,15 +508,22 @@ fn dot_git_strip_applies(u: &str) -> bool { false } -/// 64-bit FNV-1a as 16 hex characters. Deterministic across processes, -/// non-cryptographic but collision-resistant enough for a cache key. -fn fnv1a_hex(s: &str) -> String { - let mut h: u64 = 0xcbf2_9ce4_8422_2325; - for &b in s.as_bytes() { - h ^= u64::from(b); - h = h.wrapping_mul(0x0000_0100_0000_01b3); +/// SHA-256 of the string as 64 lowercase hex characters (audit F-009). +/// The cache dir is keyed by a hash of the (attacker-adjacent) repo URL, +/// so a *cryptographic* digest is used: a non-cryptographic hash like the +/// former 64-bit FNV-1a is trivially collidable, and a deliberate +/// collision would make two URLs share one bare mirror + lock file. `sha2` +/// is already a dependency (M7.6 lockfile content hashing). +fn sha256_hex(s: &str) -> String { + use std::fmt::Write as _; + let mut hasher = Sha256::new(); + hasher.update(s.as_bytes()); + let digest = hasher.finalize(); + let mut out = String::with_capacity(digest.len() * 2); + for b in digest { + let _ = write!(out, "{b:02x}"); } - format!("{h:016x}") + out } // --------------------------------------------------------------------------- @@ -603,15 +611,21 @@ fn run_with_timeout( let stdout_thread = thread::spawn(move || drain_to_vec(stdout_handle)); let stderr_thread = thread::spawn(move || drain_to_vec(stderr_handle)); + // Break out of the wait loop with the outcome instead of returning + // early, so the child is *reaped on every path* (normal exit, timeout + // kill, or a `try_wait` error) and the drain threads are joined at the + // single point below. Returning straight from a timeout used to leave + // the two reader threads detached until their pipe reads happened to + // finish (audit F-010) — nondeterministic and hard to test. let started = Instant::now(); - let status = loop { + let outcome: Result = loop { match child.try_wait() { - Ok(Some(s)) => break s, + Ok(Some(s)) => break Ok(s), Ok(None) => { if started.elapsed() > timeout { let _ = child.kill(); let _ = child.wait(); - return Err(FetchError::Timeout { + break Err(FetchError::Timeout { url: url_tag.to_string(), after: timeout, }); @@ -619,7 +633,11 @@ fn run_with_timeout( thread::sleep(Duration::from_millis(50)); } Err(e) => { - return Err(FetchError::GitSpawn { + // Reap before bailing so the pipes close and the joins + // below can't block on a still-running child. + let _ = child.kill(); + let _ = child.wait(); + break Err(FetchError::GitSpawn { url: url_tag.to_string(), source: e, }); @@ -627,8 +645,12 @@ fn run_with_timeout( } }; + // The child is reaped on every path above, so its stdout/stderr are at + // EOF and these joins return promptly. Always join before propagating + // — no detached reader survives a timeout. let stdout = stdout_thread.join().unwrap_or_default(); let stderr = stderr_thread.join().unwrap_or_default(); + let status = outcome?; Ok(CapturedOutput { status, stdout, @@ -680,11 +702,15 @@ mod tests { #[test] fn normalize_collapses_dual_address_to_one_key() { - let a = fnv1a_hex(&normalize_url("https://github.com/foo/bar.git")); - let b = fnv1a_hex(&normalize_url("https://github.com/foo/bar")); - let c = fnv1a_hex(&normalize_url("https://GitHub.com/foo/bar/")); + let a = sha256_hex(&normalize_url("https://github.com/foo/bar.git")); + let b = sha256_hex(&normalize_url("https://github.com/foo/bar")); + let c = sha256_hex(&normalize_url("https://GitHub.com/foo/bar/")); assert_eq!(a, b); assert_eq!(b, c); + // SHA-256 hex: 64 lowercase hex chars, and distinct URLs differ. + assert_eq!(a.len(), 64); + assert!(a.chars().all(|c| c.is_ascii_hexdigit())); + assert_ne!(a, sha256_hex(&normalize_url("https://github.com/foo/baz"))); } #[test] @@ -708,7 +734,7 @@ mod tests { let bare = normalize_url("file:///tmp/foo.git"); let work = normalize_url("file:///tmp/foo"); assert_ne!(bare, work); - assert_ne!(fnv1a_hex(&bare), fnv1a_hex(&work)); + assert_ne!(sha256_hex(&bare), sha256_hex(&work)); } #[test] @@ -872,7 +898,7 @@ mod tests { let repo = fetcher.fetch(&url).unwrap(); assert!(repo.join("HEAD").exists(), "bare clone should have HEAD"); // Cache hash is deterministic. - let expected_hash = fnv1a_hex(&normalize_url(&url)); + let expected_hash = sha256_hex(&normalize_url(&url)); assert_eq!( repo.file_name().unwrap(), format!("{expected_hash}.git").as_str() diff --git a/src/packages/lockfile.rs b/src/packages/lockfile.rs index 1d520fa..1cb8fc0 100644 --- a/src/packages/lockfile.rs +++ b/src/packages/lockfile.rs @@ -534,7 +534,9 @@ impl Lockfile { packages.push(ResolvedPackage { name: entry.name.clone(), address, - commit: entry.commit.clone(), + // A lockfile entry's `commit` is a real SHA, a valid + // commit-ish for the renamed field (audit F-011). + revision: entry.commit.clone(), version: entry.version.clone(), manifest, top_level_pin, @@ -573,7 +575,7 @@ fn build_entry( // Resolve the chosen commit-ish (which may be a tag string for // version pins) to a 40-char SHA. let sha = fetcher - .resolve(&repo, &RefSpec::Commit(rp.commit.clone())) + .resolve(&repo, &RefSpec::Commit(rp.revision.clone())) .map_err(|source| LockfileError::Fetch { url: url.clone(), source, diff --git a/src/packages/resolver.rs b/src/packages/resolver.rs index 90de882..00bcd2a 100644 --- a/src/packages/resolver.rs +++ b/src/packages/resolver.rs @@ -83,7 +83,7 @@ use thiserror::Error; use super::address::{Address, AddressError}; use super::fetcher::{FetchError, Fetcher, RefSpec}; -use super::installer::{InstallPin, running_pmacs_version}; +use super::installer::{InstallPin, package_basename, running_pmacs_version}; use super::lockfile::{Lockfile, LockfileError, UpdatePolicy}; use super::manifest::{ManifestError, PackageManifest, PackageName}; @@ -140,11 +140,16 @@ pub struct ResolvedPackage { /// Address used to fetch this package. For transitive deps this /// is whatever string the parent manifest recorded. pub address: Address, - /// Full 40-character commit hash this package resolves to. - pub commit: String, - /// Version parsed from the manifest at `commit`. + /// The commit-ish this package resolves to: a 40-char SHA, or a + /// tag/branch name (the resolver deliberately works against + /// commit-ishes and defers concrete-SHA resolution to the installer / + /// lockfile — see `TagCandidate::commit_for_tag`). **Not guaranteed to + /// be a SHA**; do not treat it as immutable. Renamed from `commit` + /// (audit F-011), whose name falsely promised a hash. + pub revision: String, + /// Version parsed from the manifest at `revision`. pub version: Version, - /// Manifest at `commit`, captured during resolution. + /// Manifest at `revision`, captured during resolution. pub manifest: PackageManifest, /// `Some(pin)` if this entry came directly from a top-level /// [`ResolveRequest`]; `None` if it was pulled in transitively @@ -702,36 +707,14 @@ impl<'a> ResolverState<'a> { deps_of.insert(name.clone(), set); } - // Kahn's topological sort. `BTreeMap` + `BTreeSet` keep - // tie-breaking deterministic (alphabetical by package name). - let mut indegree: BTreeMap = - entries.keys().map(|n| (n.clone(), 0)).collect(); - for deps in deps_of.values() { - for d in deps { - if let Some(slot) = indegree.get_mut(d) { - *slot += 1; - } - } - } - - // Note: edge direction. We treat "X depends on Y" as an edge - // from X to Y. Topological order with Kahn's algorithm needs - // dependencies *before* dependents, which means we process - // nodes whose in-degree (number of dependents) is zero — - // i.e., leaves of the depender graph, which are the most - // fundamental dependencies. We then peel layers outward. - // - // Reframe: indegree[Y] = number of X such that X depends on Y. - // No, that's backwards. Indegree[Y] = number of edges pointing - // *into* Y. If "X depends on Y" is X → Y, then indegree[Y] - // counts dependents. We want to emit Y first when nothing - // points to it from a not-yet-emitted node. Reset. - // - // Cleaner: indegree[X] = count of X's outgoing edges still - // pending = number of X's deps not yet emitted. Start with - // indegree[X] = |deps_of[X]|; each time we emit Y, decrement - // indegree of every X that depends on Y. Emit X when its - // indegree hits 0. + // Kahn's topological sort, dependencies before dependents. + // Edge direction: "X depends on Y" is X → Y, and Y must be + // emitted before X. Track each node's *outgoing* pending edges — + // `indegree[X]` = the number of X's deps not yet emitted, seeded + // to `|deps_of[X]|` — and when a node Y is emitted, decrement + // every X that depends on Y; emit X once its count reaches 0. + // `BTreeMap` / `BTreeSet` keep tie-breaking deterministic + // (alphabetical by package name). let mut indegree: BTreeMap = deps_of .iter() .map(|(n, deps)| (n.clone(), deps.len())) @@ -790,7 +773,7 @@ impl<'a> ResolverState<'a> { ResolvedPackage { name, address: entry.address, - commit: entry.commit, + revision: entry.commit, version: entry.version, manifest: entry.manifest, top_level_pin: entry.top_level_pin, @@ -798,6 +781,18 @@ impl<'a> ResolverState<'a> { }) .collect(); + // Reject distinct packages that share an install basename (F-005). + // The plan is the one place holding every resolved name at once; + // installing by basename would otherwise let two of them (e.g. + // `owner/magit` and `other/magit`) collide on disk, most-recent + // winning. (This does not touch the loader's *intended* cross-scope + // override, which shares a basename across separate installs.) + if let Some((basename, names)) = + find_basename_collision(packages.iter().map(|p| p.name.clone())) + { + return Err(ResolveError::BasenameCollision { basename, names }); + } + Ok(ResolvePlan { packages }) } @@ -1312,6 +1307,54 @@ pub enum ResolveError { /// A lockfile-aware path produced an underlying [`LockfileError`]. #[error("lockfile: {0}")] Lockfile(#[from] LockfileError), + + /// Two or more distinct packages in one resolve plan share an install + /// basename (audit F-005). pmacs installs each package under + /// `//` and routes `require` by basename, so + /// distinct packages with the same last `/`-segment (e.g. `owner/magit` + /// and `other/magit`) would overwrite each other on disk with the + /// most-recent install silently winning. Reject rather than collapse + /// them — a namespace-preserving layout is the real fix (deferred). + #[error( + "packages [{names}] share the install basename `{basename}`: pmacs \ + installs and requires by basename, so these distinct packages would \ + collide on disk. Rename one, or install them in separate roots.", + names = format_package_name_list(.names) + )] + BasenameCollision { + /// The colliding last-path-segment. + basename: String, + /// The distinct package names that map to it (sorted). + names: Vec, + }, +} + +/// Comma-join package names for a [`ResolveError::BasenameCollision`] +/// message. +fn format_package_name_list(names: &[PackageName]) -> String { + names + .iter() + .map(|n| n.as_str().to_string()) + .collect::>() + .join(", ") +} + +/// Find an install basename shared by more than one distinct package name +/// (audit F-005). Returns the colliding basename and its names (sorted by +/// the `BTreeMap`/first-seen order), or `None` when every basename is +/// unique. Used by [`into_plan`] to reject a plan that would collide on +/// disk before anything is installed. +fn find_basename_collision( + names: impl Iterator, +) -> Option<(String, Vec)> { + let mut by_basename: BTreeMap> = BTreeMap::new(); + for name in names { + by_basename + .entry(package_basename(name.as_str()).to_string()) + .or_default() + .push(name); + } + by_basename.into_iter().find(|(_, names)| names.len() > 1) } // --------------------------------------------------------------------------- @@ -1414,6 +1457,41 @@ mod tests { assert_eq!(v, Version::new(1, 2, 3)); } + #[test] + fn basename_collision_flags_distinct_same_basename_packages() { + let pn = |s: &str| PackageName::new(s).expect("valid name"); + // `owner/magit` and `other/magit` both install as `magit` → collide. + let collision = find_basename_collision( + [pn("owner/magit"), pn("other/magit"), pn("ripgrep")].into_iter(), + ); + let (basename, names) = collision.expect("collision detected"); + assert_eq!(basename, "magit"); + assert_eq!(names.len(), 2); + assert!(names.contains(&pn("owner/magit")) && names.contains(&pn("other/magit"))); + // The message names both offenders. + let err = ResolveError::BasenameCollision { basename, names }; + let msg = err.to_string(); + assert!( + msg.contains("owner/magit") && msg.contains("other/magit"), + "{msg}" + ); + } + + #[test] + fn basename_collision_ignores_unique_and_same_name_repeats() { + let pn = |s: &str| PackageName::new(s).expect("valid name"); + // Distinct basenames: no collision. + assert!( + find_basename_collision([pn("owner/magit"), pn("owner/forge")].into_iter()).is_none() + ); + // A namespaced and a bare package with different basenames are fine. + assert!(find_basename_collision([pn("owner/magit"), pn("ripgrep")].into_iter()).is_none()); + // The plan's names are already distinct (keyed by PackageName), so + // an empty or singleton set never collides. + assert!(find_basename_collision(std::iter::empty()).is_none()); + assert!(find_basename_collision([pn("owner/magit")].into_iter()).is_none()); + } + #[test] fn parse_tag_accepts_bare_version() { let v = parse_tag_as_version("1.2.3").expect("parse"); diff --git a/tests/m7_10_acceptance.rs b/tests/m7_10_acceptance.rs index 59290d9..648e9a0 100644 --- a/tests/m7_10_acceptance.rs +++ b/tests/m7_10_acceptance.rs @@ -200,7 +200,7 @@ fn install_plan( for rp in &plan.packages { let spec = InstallSpec { address: rp.address.clone(), - pin: InstallPin::Commit(rp.commit.clone()), + pin: InstallPin::Commit(rp.revision.clone()), }; installed.push(installer.install(&spec).expect("install")); } diff --git a/tests/m7_5_acceptance.rs b/tests/m7_5_acceptance.rs index 798d153..68abaf4 100644 --- a/tests/m7_5_acceptance.rs +++ b/tests/m7_5_acceptance.rs @@ -369,9 +369,11 @@ fn resolver_resolves_transitive_dependency_chain() { for entry in &plan.packages { assert_eq!(entry.version, Version::new(1, 0, 0)); assert!( - entry.commit.is_empty() || entry.commit.len() == 40 || entry.commit.starts_with('v'), - "expected commit to be a 40-char SHA or tag-resolved, got {:?}", - entry.commit, + entry.revision.is_empty() + || entry.revision.len() == 40 + || entry.revision.starts_with('v'), + "expected revision to be a 40-char SHA or tag-resolved, got {:?}", + entry.revision, ); } @@ -625,7 +627,7 @@ fn resolver_branch_pin_records_pin_and_resolved_commit() { assert_eq!(plan.packages.len(), 1); let entry = &plan.packages[0]; assert_eq!( - entry.commit, feature_head, + entry.revision, feature_head, "branch HEAD must be the resolved commit" ); assert!( @@ -650,7 +652,7 @@ fn resolver_commit_pin_uses_exact_revision() { assert_eq!(plan.packages.len(), 1); let entry = &plan.packages[0]; - assert_eq!(entry.commit, feature_head); + assert_eq!(entry.revision, feature_head); assert!( matches!(entry.top_level_pin, Some(InstallPin::Commit(ref c)) if c == &feature_head), "plan must record the original commit pin, got {:?}", diff --git a/tests/m7_6_acceptance.rs b/tests/m7_6_acceptance.rs index 7182e6e..d408435 100644 --- a/tests/m7_6_acceptance.rs +++ b/tests/m7_6_acceptance.rs @@ -380,7 +380,7 @@ fn frozen_resolve_yields_lockfile_commits_on_a_second_machine() { let mut commits_b: Vec<(String, String)> = plan_b .packages .iter() - .map(|p| (p.name.as_str().to_string(), p.commit.clone())) + .map(|p| (p.name.as_str().to_string(), p.revision.clone())) .collect(); commits_b.sort(); assert_eq!(