fix(cmd-shim): apply review findings #4 and #5

#4 (cmd-shim — chmod target swallowed all errors): the post-write
\`ensure_executable_bits\` call used \`let _ = ...\` which dropped
\`PermissionDenied\`, \`EROFS\`, AppArmor deny and other real failures
alongside the intentionally-ignored \`NotFound\`. Match \`NotFound\`
explicitly and propagate everything else as \`LinkBinsError::Chmod\`,
mirroring pnpm's \`fixBin\` ENOENT guard. Adds two DI-driven
regression tests:

- \`link_bins_propagates_target_chmod_error_via_di\` — \`PermissionDenied\`
  must surface as \`Chmod\`.
- \`link_bins_swallows_target_chmod_not_found_via_di\` — \`NotFound\`
  must remain swallowed.

#5 (package-manager — peer-resolved slot name parsing): \`find_slot_
own_package_dir\` used \`slot_name.rfind('@')\` to strip the version
tail. That works for \`parent@1.0.0\` and \`@scope+parent@1.0.0\` but
breaks for peer-resolved slots: \`to_virtual_store_name\` produces
shapes like \`ts-node@10.9.1_@types+node@18.7.19_typescript@5.1.6\`
where the last \`@\` is inside a peer's version, not at the
package-name boundary. \`rfind\` then split inside the peer spec, the
slot's own package wasn't found, and the slot was silently skipped —
bins of children of every peer-resolved slot were never linked.

Parse from the left instead, skipping a single leading \`@\` that
belongs to a scoped package. The package-name half can never contain
\`@\` after \`to_virtual_store_name\`'s scope normalization (\`/\` →
\`+\`), so the first non-leading \`@\` is the right boundary. Adds
\`link_virtual_store_bins_handles_peer_resolved_slot_name\` using the
exact \`ts-node@10.9.1_@types+node@18.7.19_typescript@5.1.6\` shape
verified by \`pacquet_lockfile::pkg_name_ver_peer::tests::to_virtual_
store_name\`.

Both fixes verified by re-breaking the relevant code path and
confirming the regression tests fail, then reverting.

Drive-by from #9 (style nit on the chain split in
\`SymlinkDirectDependencies::run\`): the chain was broken in the
earlier \`feat(cmd-shim): port directories.bin discovery and Windows
shim formats\` commit (dfdd33a) because \`direct_deps\` is consumed
twice — once by \`symlink_direct_deps_into_node_modules\`, once by
\`link_direct_dep_bins\`'s \`dep_names\` derivation. That can't sit
inside a single chain. Documenting it here in lieu of amending the
earlier commit.

Test count: cmd-shim 70 → 72, package-manager (lib) 38 → 39.

Author: claude

crates/cmd-shim/src/link_bins.rs

@@ -296,11 +296,19 @@ where
     // mode without rewriting CRLF shebangs (a feature pnpm inherits from
     // npm's `bin-links/lib/fix-bin.js`). Targets shipped by npm already
     // use LF in practice, so the simpler chmod-only path is enough for
-    // the install tests this PR ports. Errors here are swallowed —
-    // a missing target shouldn't fail the install (this is post-warm-skip
-    // territory) and pacquet has already verified `target_path` exists
-    // upstream of `write_shim`.
-    let _ = Api::ensure_executable_bits(target_path);
+    // the install tests this PR ports. `NotFound` is swallowed because
+    // the target may legitimately have been removed by an unrelated
+    // process between extraction and shim linking; everything else
+    // (`PermissionDenied`, `EROFS`, AppArmor deny, foreign uid)
+    // surfaces as `LinkBinsError::Chmod` so real failures don't
+    // disappear silently. Mirrors pnpm's `fixBin` ENOENT guard.
+    match Api::ensure_executable_bits(target_path) {
+        Ok(()) => {}
+        Err(error) if error.kind() == io::ErrorKind::NotFound => {}
+        Err(error) => {
+            return Err(LinkBinsError::Chmod { path: target_path.to_path_buf(), error });
+        }
+    }
 
     Ok(())
 }

crates/cmd-shim/src/link_bins/tests.rs

@@ -449,6 +449,130 @@ fn link_bins_propagates_chmod_error_via_di() {
     assert!(matches!(err, LinkBinsError::Chmod { .. }));
 }
 
+/// `write_shim` propagates a non-`NotFound` IO error from
+/// `ensure_executable_bits` (chmod on the *target* binary, not the
+/// shim). `NotFound` is swallowed by design — the target may have
+/// been removed concurrently — but `PermissionDenied` and friends
+/// must surface as `LinkBinsError::Chmod`. Pins the
+/// guard added in this PR (review finding #4).
+#[test]
+fn link_bins_propagates_target_chmod_error_via_di() {
+    use std::io;
+    struct FailingTargetChmod;
+    impl FsReadDir for FailingTargetChmod {
+        fn read_dir(_: &Path) -> io::Result<Vec<std::path::PathBuf>> {
+            Ok(vec![])
+        }
+    }
+    impl FsReadFile for FailingTargetChmod {
+        fn read_file(_: &Path) -> io::Result<Vec<u8>> {
+            unreachable!()
+        }
+    }
+    impl FsReadString for FailingTargetChmod {
+        fn read_to_string(_: &Path) -> io::Result<String> {
+            Err(io::Error::from(io::ErrorKind::NotFound))
+        }
+    }
+    impl FsReadHead for FailingTargetChmod {
+        fn read_head(_: &Path, _: &mut [u8]) -> io::Result<usize> {
+            Ok(0)
+        }
+    }
+    impl FsCreateDirAll for FailingTargetChmod {
+        fn create_dir_all(_: &Path) -> io::Result<()> {
+            Ok(())
+        }
+    }
+    impl FsWrite for FailingTargetChmod {
+        fn write(_: &Path, _: &[u8]) -> io::Result<()> {
+            Ok(())
+        }
+    }
+    impl FsSetPermissions for FailingTargetChmod {
+        fn set_executable(_: &Path) -> io::Result<()> {
+            Ok(())
+        }
+        fn ensure_executable_bits(_: &Path) -> io::Result<()> {
+            // The target chmod returns a non-`NotFound` error; the
+            // implementation must surface it rather than silently
+            // dropping it.
+            Err(io::Error::from(io::ErrorKind::PermissionDenied))
+        }
+    }
+
+    let manifest = serde_json::json!({"name": "foo", "bin": "cli.js"});
+    let tmp = tempdir().unwrap();
+    let pkg = tmp.path().join("foo");
+    fs::create_dir_all(&pkg).unwrap();
+    fs::write(pkg.join("cli.js"), "").unwrap();
+    let err = link_bins_of_packages::<FailingTargetChmod>(
+        &[PackageBinSource { location: pkg, manifest }],
+        &tmp.path().join(".bin"),
+    )
+    .expect_err("non-NotFound target chmod error must propagate as Chmod");
+    assert!(matches!(err, LinkBinsError::Chmod { .. }));
+}
+
+/// `write_shim` swallows `NotFound` from `ensure_executable_bits`
+/// because the target may legitimately be missing (concurrent removal,
+/// race with another install). Pins this distinction so a future
+/// regression that propagates `NotFound` here would fail the test.
+#[test]
+fn link_bins_swallows_target_chmod_not_found_via_di() {
+    use std::io;
+    struct NotFoundTargetChmod;
+    impl FsReadDir for NotFoundTargetChmod {
+        fn read_dir(_: &Path) -> io::Result<Vec<std::path::PathBuf>> {
+            Ok(vec![])
+        }
+    }
+    impl FsReadFile for NotFoundTargetChmod {
+        fn read_file(_: &Path) -> io::Result<Vec<u8>> {
+            unreachable!()
+        }
+    }
+    impl FsReadString for NotFoundTargetChmod {
+        fn read_to_string(_: &Path) -> io::Result<String> {
+            Err(io::Error::from(io::ErrorKind::NotFound))
+        }
+    }
+    impl FsReadHead for NotFoundTargetChmod {
+        fn read_head(_: &Path, _: &mut [u8]) -> io::Result<usize> {
+            Ok(0)
+        }
+    }
+    impl FsCreateDirAll for NotFoundTargetChmod {
+        fn create_dir_all(_: &Path) -> io::Result<()> {
+            Ok(())
+        }
+    }
+    impl FsWrite for NotFoundTargetChmod {
+        fn write(_: &Path, _: &[u8]) -> io::Result<()> {
+            Ok(())
+        }
+    }
+    impl FsSetPermissions for NotFoundTargetChmod {
+        fn set_executable(_: &Path) -> io::Result<()> {
+            Ok(())
+        }
+        fn ensure_executable_bits(_: &Path) -> io::Result<()> {
+            Err(io::Error::from(io::ErrorKind::NotFound))
+        }
+    }
+
+    let manifest = serde_json::json!({"name": "foo", "bin": "cli.js"});
+    let tmp = tempdir().unwrap();
+    let pkg = tmp.path().join("foo");
+    fs::create_dir_all(&pkg).unwrap();
+    fs::write(pkg.join("cli.js"), "").unwrap();
+    link_bins_of_packages::<NotFoundTargetChmod>(
+        &[PackageBinSource { location: pkg, manifest }],
+        &tmp.path().join(".bin"),
+    )
+    .expect("NotFound on target chmod must be swallowed silently");
+}
+
 /// `link_bins_of_packages` propagates a non-`NotFound` IO error from
 /// `search_script_runtime` (the `ProbeShimSource` variant). Forced via
 /// a fake `FsReadHead` that fails with permission-denied — the wider

crates/package-manager/src/link_bins.rs

@@ -152,17 +152,29 @@ impl<'a> LinkVirtualStoreBins<'a> {
 /// Locate the slot's own package directory inside `<slot>/node_modules`.
 ///
 /// The slot directory's name encodes the package name as
-/// `<scope>+<name>@<version>` (see [`pacquet_lockfile::PkgNameVerPeer::to_virtual_store_name`]).
-/// The own-package directory is the one whose location is a real directory
-/// (not a symlink) and whose path matches the slot name decoded back to
-/// `<scope>/<name>` form.
+/// `<scope>+<name>@<version>` for the simple case (see
+/// [`pacquet_lockfile::PkgNameVerPeer::to_virtual_store_name`]). For
+/// peer-resolved slots the version segment itself contains additional
+/// `@`-separated peer specs joined by `_`, e.g.
+/// `ts-node@10.9.1_@types+node@18.7.19_typescript@5.1.6` — the `@` after
+/// `typescript` is part of a peer's version, not the package-name
+/// boundary. Parsing from the right (`rfind('@')`) would split there
+/// and silently break peer-resolved slots; parse from the left
+/// instead, skipping a leading `@` that belongs to a scoped package.
 fn find_slot_own_package_dir(slot_dir: &Path, modules_dir: &Path) -> Option<PathBuf> {
     let slot_name = slot_dir.file_name()?.to_str()?;
-    // Strip the `@<version>` tail.
-    let at = slot_name.rfind('@')?;
+
+    // The package-name half is everything before the **first** `@`,
+    // ignoring a single leading `@` that belongs to a scoped name
+    // (`@scope+pkg@...` → start the `@` search at offset 1).
+    // After `to_virtual_store_name`, `/` in scoped names becomes `+`,
+    // so the package-name half can never contain `@` itself.
+    let search_start = if slot_name.starts_with('@') { 1 } else { 0 };
+    let at = search_start + slot_name[search_start..].find('@')?;
     let name_part = &slot_name[..at];
-    // `+` separates `<scope>+<name>` for scoped packages; non-scoped names
-    // contain no `+`.
+
+    // `+` separates `<scope>+<name>` for scoped packages; non-scoped
+    // names contain no `+`.
     let pkg_dir = match name_part.split_once('+') {
         Some((scope, name)) => modules_dir.join(scope).join(name),
         None => modules_dir.join(name_part),

crates/package-manager/src/link_bins/tests.rs

@@ -147,6 +147,53 @@ fn link_virtual_store_bins_handles_scoped_slot_name() {
     assert!(shim.exists(), "scoped-slot bin linking must produce a shim at {shim:?}");
 }
 
+/// Peer-resolved slots have version segments that contain additional
+/// `@` characters (one per peer spec). `find_slot_own_package_dir`
+/// must parse the package-name boundary from the **left**, not the
+/// right, otherwise it splits inside a peer spec and silently fails
+/// to locate the own package — bins of children of the slot then
+/// never get linked.
+///
+/// Slot name shape verified against
+/// `pacquet_lockfile::pkg_name_ver_peer::tests::to_virtual_store_name`.
+/// Pins review finding #5.
+#[test]
+fn link_virtual_store_bins_handles_peer_resolved_slot_name() {
+    let tmp = tempdir().unwrap();
+    let virtual_dir = tmp.path().join(".pacquet");
+
+    // pnpm format: `ts-node@10.9.1(@types/node@18.7.19)(typescript@5.1.6)`.
+    // Pacquet's `to_virtual_store_name`: `_` joins peers, `(` and `)`
+    // are stripped, `/` becomes `+`.
+    let slot = virtual_dir.join("ts-node@10.9.1_@types+node@18.7.19_typescript@5.1.6");
+    let modules = slot.join("node_modules");
+    let pkg_dir = modules.join("ts-node");
+    let child_dir = modules.join("child");
+    fs::create_dir_all(&pkg_dir).unwrap();
+    fs::create_dir_all(&child_dir).unwrap();
+
+    fs::write(
+        pkg_dir.join("package.json"),
+        json!({"name": "ts-node", "version": "10.9.1"}).to_string(),
+    )
+    .unwrap();
+    fs::write(
+        child_dir.join("package.json"),
+        json!({"name": "child", "version": "1.0.0", "bin": "cli.js"}).to_string(),
+    )
+    .unwrap();
+    fs::write(child_dir.join("cli.js"), "#!/usr/bin/env node\n").unwrap();
+
+    LinkVirtualStoreBins { virtual_store_dir: &virtual_dir }.run().unwrap();
+
+    let shim = pkg_dir.join("node_modules/.bin/child");
+    assert!(
+        shim.exists(),
+        "peer-resolved slot's child bin must be linked under the own package's `node_modules/.bin`, \
+         not silently dropped because the slot-name parser split inside a peer spec",
+    );
+}
+
 /// A virtual-store slot whose `node_modules/` is missing must be skipped
 /// without error.
 #[test]