fix(packages): F-005 must also guard the frozen/lockfile plan path

Review follow-up on F-005. The basename-collision check only ran in
ResolverState::into_plan, which covers fresh resolves and UpdateOne — but
UpdatePolicy::Frozen returns Lockfile::to_resolve_plan(...) directly,
building a ResolvePlan without the check. A pre-existing or hand-edited
lockfile containing two distinct packages that share an install basename
(e.g. owner/magit and other/magit) would produce one plan and install both
to <root>/<basename>, silently colliding.

Make find_basename_collision (and its message helper) pub(crate) and apply
it in Lockfile::to_resolve_plan too — up front, before any fetch, so a
colliding lockfile fails fast via a new LockfileError::BasenameCollision
(surfaced through the Frozen path as ResolveError::Lockfile). Both
plan-construction sites now reject; to_resolve_plan is pub and has direct
callers, so guarding the method (not just the resolve_with_policy branch)
covers them all.

New unit test builds a two-entry colliding lockfile and asserts
to_resolve_plan rejects it before touching the fetcher.

Validated: fmt clean; clippy --all-targets clean under both Lua flavors;
1437 lib tests pass (incl. the new frozen-path test).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014TXbAwk27agwhrNNrhLi2U

docs/package-manager-hardening-framing.md

@@ -115,13 +115,19 @@ CI.
 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-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
@@ -142,8 +148,9 @@ Landed as framed; all five in `src/packages/` (F-011 also touches
   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.
+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)
 

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`
@@ -816,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>,
+    },
 }
 
 // ---------------------------------------------------------------------------
@@ -826,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

@@ -1329,9 +1329,9 @@ pub enum ResolveError {
     },
 }
 
-/// Comma-join package names for a [`ResolveError::BasenameCollision`]
-/// message.
-fn format_package_name_list(names: &[PackageName]) -> String {
+/// 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())
@@ -1342,9 +1342,10 @@ fn format_package_name_list(names: &[PackageName]) -> String {
 /// 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(
+/// 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();