Merge pull request #81 from levineuwirth/session-pkg-manager-hardening

fix(packages): basename reject, SHA-256 cache key, timeout join, commit→revision, dead-code (F-005/F-009–F-012)

docs/package-manager-hardening-framing.md

@@ -0,0 +1,163 @@
+# 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<ExitStatus,
+FetchError>` — 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` + `lockfile.rs`): new
+  `ResolveError::BasenameCollision { basename, names }`; a `pub(crate)`
+  `find_basename_collision(names)` groups the plan's names by
+  `package_basename` and returns the first basename with >1 distinct name.
+  **Guards *both* plan-construction paths** (review follow-up): `into_plan`
+  (fresh / `UpdateOne`) *and* `Lockfile::to_resolve_plan` (the frozen /
+  lockfile-derived path, which returns its own
+  `LockfileError::BasenameCollision`). The frozen check runs up front,
+  before any fetch, so a hand-edited or pre-existing lockfile with two
+  same-basename packages fails fast. Three unit tests (detection, negative
+  cases, and the frozen-path rejection); 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); 1437 lib unit tests pass, incl. the new
+F-005 tests (fresh + frozen paths), the F-009 hash test, 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 `<owner>/<name>/`, require by canonical name) is a larger
+  design change; rejecting is the interim floor.

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<Table> {
             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)?;

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<PathBuf, FetchError> {
         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,
+/// SHA-256 of the string as 64 lowercase hex characters (audit F-009).
-/// non-cryptographic but collision-resistant enough for a cache key.
+/// The cache dir is keyed by a hash of the (attacker-adjacent) repo URL,
-fn fnv1a_hex(s: &str) -> String {
+/// so a *cryptographic* digest is used: a non-cryptographic hash like the
-    let mut h: u64 = 0xcbf2_9ce4_8422_2325;
+/// former 64-bit FNV-1a is trivially collidable, and a deliberate
-    for &b in s.as_bytes() {
+/// collision would make two URLs share one bare mirror + lock file. `sha2`
-        h ^= u64::from(b);
+/// is already a dependency (M7.6 lockfile content hashing).
-        h = h.wrapping_mul(0x0000_0100_0000_01b3);
+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<ExitStatus, FetchError> = 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 a = sha256_hex(&normalize_url("https://github.com/foo/bar.git"));
-        let b = fnv1a_hex(&normalize_url("https://github.com/foo/bar"));
+        let b = sha256_hex(&normalize_url("https://github.com/foo/bar"));
-        let c = fnv1a_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()

src/packages/lockfile.rs

@@ -88,7 +88,9 @@ use super::address::Address;
 use super::fetcher::{FetchError, Fetcher, RefSpec};
 use super::installer::InstallPin;
 use super::manifest::PackageName;
-use super::resolver::{ResolvePlan, ResolvedPackage};
+use super::resolver::{
+    ResolvePlan, ResolvedPackage, find_basename_collision, format_package_name_list,
+};
 
 /// Schema version emitted by [`Lockfile::to_bytes`]. Bumped only on
 /// incompatible format changes.
@@ -489,6 +491,16 @@ impl Lockfile {
     /// (reproducible installs across machines) on the install path
     /// itself, not just at lockfile-write time.
     pub fn to_resolve_plan(&self, fetcher: &Fetcher) -> Result<ResolvePlan, LockfileError> {
+        // Reject basename collisions up front (audit F-005), before any
+        // fetch: a hand-edited or pre-existing lockfile with two distinct
+        // packages sharing a basename (e.g. `owner/magit` + `other/magit`)
+        // would otherwise install both to `<root>/<basename>`. The
+        // fresh-resolve path guards this identically in `into_plan`.
+        if let Some((basename, names)) =
+            find_basename_collision(self.packages.iter().map(|e| e.name.clone()))
+        {
+            return Err(LockfileError::BasenameCollision { basename, names });
+        }
         let mut packages = Vec::with_capacity(self.packages.len());
         for entry in &self.packages {
             // Verify before reading the manifest. `verify_entry`
@@ -534,7 +546,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 +587,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,
@@ -814,6 +828,24 @@ pub enum LockfileError {
         /// The name that was passed to `UpdateOne`.
         name: PackageName,
     },
+    /// Two or more distinct lockfile entries share an install basename
+    /// (audit F-005). A hand-edited or pre-existing lockfile with e.g.
+    /// `owner/magit` and `other/magit` would install both to
+    /// `<root>/magit`. The frozen/lockfile plan path must reject this just
+    /// like the fresh-resolve path does.
+    #[error(
+        "lockfile has packages [{names}] sharing the install basename \
+         `{basename}`: pmacs installs and requires by basename, so these \
+         distinct packages would collide on disk. Regenerate the lockfile \
+         after renaming 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.
+        names: Vec<PackageName>,
+    },
 }
 
 // ---------------------------------------------------------------------------
@@ -824,6 +856,42 @@ pub enum LockfileError {
 mod tests {
     use super::*;
 
+    #[test]
+    fn to_resolve_plan_rejects_basename_collision_before_fetching() {
+        // A hand-edited / pre-existing lockfile with two distinct packages
+        // that share an install basename (audit F-005). The frozen path
+        // must reject it just like the fresh-resolve path, and it does so
+        // *before* any fetch — so the throwaway cache dir is never touched.
+        let entry = |name: &str, url: &str| LockfileEntry {
+            name: PackageName::new(name).unwrap(),
+            url: url.into(),
+            commit: "0".repeat(40),
+            version: Version::new(1, 0, 0),
+            content_hash: ContentHash::sha256_of(name.as_bytes()),
+            top_level_pin: None,
+            dependencies: vec![],
+        };
+        let lock = Lockfile {
+            schema_version: LOCKFILE_SCHEMA_VERSION,
+            generator: "pmacs test".into(),
+            packages: vec![
+                entry("owner/magit", "https://example.com/owner/magit.git"),
+                entry("other/magit", "https://example.com/other/magit.git"),
+            ],
+        };
+        let fetcher = Fetcher::with_cache_dir(std::env::temp_dir());
+        let err = lock
+            .to_resolve_plan(&fetcher)
+            .expect_err("colliding lockfile must be rejected");
+        match err {
+            LockfileError::BasenameCollision { basename, names } => {
+                assert_eq!(basename, "magit");
+                assert_eq!(names.len(), 2);
+            }
+            other => panic!("expected BasenameCollision, got {other:?}"),
+        }
+    }
+
     #[test]
     fn content_hash_sha256_known_vector() {
         // RFC 6234 §8.5: SHA-256 of "abc"

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.
+    /// The commit-ish this package resolves to: a 40-char SHA, or a
-    pub commit: String,
+    /// tag/branch name (the resolver deliberately works against
-    /// Version parsed from the manifest at `commit`.
+    /// 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
+        // Kahn's topological sort, dependencies before dependents.
-        // tie-breaking deterministic (alphabetical by package name).
+        // Edge direction: "X depends on Y" is X → Y, and Y must be
-        let mut indegree: BTreeMap<PackageName, usize> =
+        // emitted before X. Track each node's *outgoing* pending edges —
-            entries.keys().map(|n| (n.clone(), 0)).collect();
+        // `indegree[X]` = the number of X's deps not yet emitted, seeded
-        for deps in deps_of.values() {
+        // to `|deps_of[X]|` — and when a node Y is emitted, decrement
-            for d in deps {
+        // every X that depends on Y; emit X once its count reaches 0.
-                if let Some(slot) = indegree.get_mut(d) {
+        // `BTreeMap` / `BTreeSet` keep tie-breaking deterministic
-                    *slot += 1;
+        // (alphabetical by package name).
-                }
-            }
-        }
-
-        // 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.
         let mut indegree: BTreeMap<PackageName, usize> = 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,55 @@ 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
+    /// `<install_root>/<basename>/` 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<PackageName>,
+    },
+}
+
+/// Comma-join package names for a basename-collision message. Shared with
+/// [`super::lockfile`], which guards the frozen/lockfile plan path.
+pub(crate) fn format_package_name_list(names: &[PackageName]) -> String {
+    names
+        .iter()
+        .map(|n| n.as_str().to_string())
+        .collect::<Vec<_>>()
+        .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. Guards both plan-construction paths: `into_plan` (fresh /
+/// `UpdateOne` resolves) and [`super::lockfile::Lockfile::to_resolve_plan`]
+/// (the frozen / lockfile-derived path).
+pub(crate) fn find_basename_collision(
+    names: impl Iterator<Item = PackageName>,
+) -> Option<(String, Vec<PackageName>)> {
+    let mut by_basename: BTreeMap<String, Vec<PackageName>> = 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 +1458,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");

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"));
     }

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'),
+            entry.revision.is_empty()
-            "expected commit to be a 40-char SHA or tag-resolved, got {:?}",
+                || entry.revision.len() == 40
-            entry.commit,
+                || 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 {:?}",

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!(