diff --git a/pacquet/.typos.toml b/pacquet/.typos.toml index 780cd57896..e2062767a0 100644 --- a/pacquet/.typos.toml +++ b/pacquet/.typos.toml @@ -7,6 +7,7 @@ extend-exclude = [] [default.extend-words] cafs = "cafs" +pn = "pn" [default.extend-identifiers] PnP = "PnP" diff --git a/pacquet/Cargo.lock b/pacquet/Cargo.lock index 466714e601..dc036b710d 100644 --- a/pacquet/Cargo.lock +++ b/pacquet/Cargo.lock @@ -1880,6 +1880,19 @@ dependencies = [ "walkdir", ] +[[package]] +name = "pacquet-cmd-shim" +version = "0.0.1" +dependencies = [ + "derive_more", + "miette 7.6.0", + "pipe-trait", + "rayon", + "serde_json", + "tempfile", + "walkdir", +] + [[package]] name = "pacquet-diagnostics" version = "0.0.1" @@ -2027,6 +2040,7 @@ dependencies = [ "insta", "miette 7.6.0", "node-semver", + "pacquet-cmd-shim", "pacquet-executor", "pacquet-fs", "pacquet-lockfile", @@ -2154,6 +2168,7 @@ dependencies = [ "pacquet-store-dir", "pipe-trait", "pretty_assertions", + "rayon", "reqwest", "serde", "serde_json", diff --git a/pacquet/Cargo.toml b/pacquet/Cargo.toml index 61de3a8275..d5ddf9ce1d 100644 --- a/pacquet/Cargo.toml +++ b/pacquet/Cargo.toml @@ -14,6 +14,7 @@ repository = "https://github.com/pnpm/pacquet" [workspace.dependencies] # Crates pacquet-cli = { path = "crates/cli" } +pacquet-cmd-shim = { path = "crates/cmd-shim" } pacquet-fs = { path = "crates/fs" } pacquet-registry = { path = "crates/registry" } pacquet-tarball = { path = "crates/tarball" } diff --git a/pacquet/crates/cli/tests/snapshots/add__should_install_all_dependencies.snap b/pacquet/crates/cli/tests/snapshots/add__should_install_all_dependencies.snap index 655fd1d06d..58547ffd3b 100644 --- a/pacquet/crates/cli/tests/snapshots/add__should_install_all_dependencies.snap +++ b/pacquet/crates/cli/tests/snapshots/add__should_install_all_dependencies.snap @@ -5,12 +5,15 @@ expression: get_all_folders(&workspace) --- [ "node_modules", + "node_modules/.bin", "node_modules/.pnpm", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0/node_modules", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0/node_modules/@pnpm.e2e", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin-parent", + "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin-parent/node_modules", + "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin-parent/node_modules/.bin", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules/@pnpm.e2e", diff --git a/pacquet/crates/cli/tests/snapshots/add__should_symlink_correctly.snap b/pacquet/crates/cli/tests/snapshots/add__should_symlink_correctly.snap index d5e30abbda..18f1479c79 100644 --- a/pacquet/crates/cli/tests/snapshots/add__should_symlink_correctly.snap +++ b/pacquet/crates/cli/tests/snapshots/add__should_symlink_correctly.snap @@ -5,12 +5,15 @@ expression: get_all_folders(&workspace) --- [ "node_modules", + "node_modules/.bin", "node_modules/.pnpm", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0/node_modules", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0/node_modules/@pnpm.e2e", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin-parent", + "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin-parent/node_modules", + "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin-parent/node_modules/.bin", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules/@pnpm.e2e", diff --git a/pacquet/crates/cli/tests/snapshots/install__should_install_dependencies.snap b/pacquet/crates/cli/tests/snapshots/install__should_install_dependencies.snap index 4a211cca86..ca0b40c941 100644 --- a/pacquet/crates/cli/tests/snapshots/install__should_install_dependencies.snap +++ b/pacquet/crates/cli/tests/snapshots/install__should_install_dependencies.snap @@ -5,12 +5,15 @@ expression: "(workspace_folders, store_files)" ( [ "node_modules", + "node_modules/.bin", "node_modules/.pnpm", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0/node_modules", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0/node_modules/@pnpm.e2e", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin-parent", + "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin-parent/node_modules", + "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin-parent/node_modules/.bin", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules", "node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules/@pnpm.e2e", diff --git a/pacquet/crates/cmd-shim/Cargo.toml b/pacquet/crates/cmd-shim/Cargo.toml new file mode 100644 index 0000000000..999546ef36 --- /dev/null +++ b/pacquet/crates/cmd-shim/Cargo.toml @@ -0,0 +1,22 @@ +[package] +name = "pacquet-cmd-shim" +description = "Bin field parsing and command-shim generation for pacquet, mirroring pnpm's @pnpm/bins.resolver, @pnpm/bins.linker, and @zkochan/cmd-shim" +version = "0.0.1" +publish = false +authors.workspace = true +edition.workspace = true +homepage.workspace = true +keywords.workspace = true +license.workspace = true +repository.workspace = true + +[dependencies] +derive_more = { workspace = true } +miette = { workspace = true } +pipe-trait = { workspace = true } +rayon = { workspace = true } +serde_json = { workspace = true } +walkdir = { workspace = true } + +[dev-dependencies] +tempfile = { workspace = true } diff --git a/pacquet/crates/cmd-shim/src/bin_resolver.rs b/pacquet/crates/cmd-shim/src/bin_resolver.rs new file mode 100644 index 0000000000..941fa2d7c2 --- /dev/null +++ b/pacquet/crates/cmd-shim/src/bin_resolver.rs @@ -0,0 +1,198 @@ +use crate::{capabilities::FsWalkFiles, path_util::lexical_normalize}; +use serde_json::Value; +use std::path::{Path, PathBuf}; + +/// One bin entry resolved from a package's `package.json`. +/// +/// `name` is the command name as it should appear under `node_modules/.bin/`. +/// `path` is the absolute path to the script the shim invokes. +/// +/// Mirrors `Command` in pnpm v11's `@pnpm/bins.resolver`: +/// . +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Command { + pub name: String, + pub path: PathBuf, +} + +/// Bin names that legitimately ship inside a different package than their own +/// name. Mirrors `BIN_OWNER_OVERRIDES` in +/// . +/// +/// Used by [`pkg_owns_bin`] for conflict resolution between two packages +/// declaring the same bin name. +const BIN_OWNER_OVERRIDES: &[(&str, &[&str])] = &[ + ("npx", &["npm"]), + ("pn", &["pnpm", "@pnpm/exe"]), + ("pnpm", &["@pnpm/exe"]), + ("pnpx", &["pnpm", "@pnpm/exe"]), + ("pnx", &["pnpm", "@pnpm/exe"]), +]; + +/// Whether `pkg_name` is a legitimate owner of the given `bin_name`. The +/// default rule is "the package named `X` owns the `X` bin"; overrides cover +/// cases like `npx` shipping inside `npm`. Mirrors `pkgOwnsBin`. +pub fn pkg_owns_bin(bin_name: &str, pkg_name: &str) -> bool { + if bin_name == pkg_name { + return true; + } + BIN_OWNER_OVERRIDES + .iter() + .find(|(name, _)| *name == bin_name) + .is_some_and(|(_, owners)| owners.contains(&pkg_name)) +} + +/// Read every bin declared by `manifest` and return them as [`Command`]s +/// rooted at `pkg_path`. +/// +/// Handles the three cases pnpm supports, in order: +/// +/// 1. `bin` as a string. The bin name is the package's own `name` (with any +/// `@scope/` prefix stripped). Empty / missing `name` skips the entry, in +/// parity with pnpm's `INVALID_PACKAGE_NAME` guard. +/// 2. `bin` as an object. Each `(commandName, relativePath)` becomes a +/// command, with `@scope/` stripped from the key. +/// 3. Fallback: `directories.bin`. Every regular file under the directory +/// becomes a command, with the file basename as the bin name. The +/// directory itself must resolve under `pkg_path`; a `directories.bin` +/// that escapes via `..` returns an empty list. +/// +/// Validation, exactly mirroring pnpm: +/// +/// - Bin name must be URL-safe (`name == encodeURIComponent(name)`) or be the +/// single-character `$`. This is the path-traversal guard. +/// - Bin path must resolve under `pkg_path`. Prevents a malicious manifest +/// from writing shims that exec a sibling package. +pub fn get_bins_from_package_manifest( + manifest: &Value, + pkg_path: &Path, +) -> Vec { + let pkg_name = manifest.get("name").and_then(Value::as_str); + if let Some(bin) = manifest.get("bin") { + return commands_from_bin(bin, pkg_name, pkg_path); + } + if let Some(bin_dir_rel) = + manifest.get("directories").and_then(|d| d.get("bin")).and_then(Value::as_str) + { + return commands_from_directories_bin::(bin_dir_rel, pkg_path); + } + Vec::new() +} + +/// Walk every regular file under `/` and emit one +/// [`Command`] per file. Mirrors pnpm's `findFiles` + the `directories.bin` +/// branch in `getBinsFromPackageManifest`: +/// . +/// +/// Symlinks are not followed; pnpm uses `tinyglobby` with +/// `followSymbolicLinks: false`. Missing directory degrades to an empty +/// list (pnpm's `ENOENT` short-circuit). +fn commands_from_directories_bin( + bin_dir_rel: &str, + pkg_path: &Path, +) -> Vec { + let bin_dir = pkg_path.join(bin_dir_rel); + if !is_subdir(pkg_path, &bin_dir) { + return Vec::new(); + } + // Treat a top-level walk error as "no bins". This matches pnpm's + // tinyglobby ENOENT short-circuit. The trait's production impl + // already drops per-entry errors inside its iterator, so an `Err` + // here only fires when the walker can't even open `bin_dir`. + let Ok(paths) = Api::walk_files(&bin_dir) else { + return Vec::new(); + }; + let mut commands = Vec::new(); + for path in paths { + let Some(name) = path.file_name().and_then(|s| s.to_str()) else { + continue; + }; + // Same URL-safe-name guard as the keyed-bin path. + if !is_safe_bin_name(name) { + continue; + } + commands.push(Command { name: name.to_string(), path }); + } + commands +} + +fn commands_from_bin(bin: &Value, pkg_name: Option<&str>, pkg_path: &Path) -> Vec { + let mut entries: Vec<(String, String)> = Vec::new(); + match bin { + Value::String(rel_path) => { + let Some(name) = pkg_name else { + return Vec::new(); + }; + entries.push((name.to_string(), rel_path.clone())); + } + Value::Object(map) => { + for (key, value) in map { + let Some(rel_path) = value.as_str() else { + continue; + }; + entries.push((key.clone(), rel_path.to_string())); + } + } + _ => return Vec::new(), + } + + let mut commands = Vec::with_capacity(entries.len()); + for (command_name, bin_relative_path) in entries { + // Strip any `@scope/` prefix. Mirrors `commandsFromBin`'s + // `commandName[0] === '@'` branch. + let bin_name = if command_name.starts_with('@') { + match command_name.find('/') { + Some(slash) => command_name[slash + 1..].to_string(), + None => command_name, + } + } else { + command_name + }; + + if !is_safe_bin_name(&bin_name) { + continue; + } + + let bin_path = pkg_path.join(&bin_relative_path); + if !is_subdir(pkg_path, &bin_path) { + continue; + } + + commands.push(Command { name: bin_name, path: bin_path }); + } + commands +} + +/// Whether `name` matches the URL-safe character set allowed by JavaScript's +/// `encodeURIComponent`, or is the single-character escape hatch `$` pnpm +/// permits for awkward but legitimate bin names. Together these are the only +/// names pnpm allows the linker to write to disk. +/// +/// `encodeURIComponent` leaves the following bytes unescaped: +/// `A-Z a-z 0-9 - _ . ! ~ * ' ( )`. +fn is_safe_bin_name(name: &str) -> bool { + if name == "$" { + return true; + } + if name.is_empty() { + return false; + } + name.bytes().all(|b| { + b.is_ascii_alphanumeric() + || matches!(b, b'-' | b'_' | b'.' | b'!' | b'~' | b'*' | b'\'' | b'(' | b')') + }) +} + +/// Whether `child` resolves to a path under `parent`, after lexically +/// normalising `..` segments. Mirrors `isSubdir(pkgPath, binPath)` from pnpm's +/// `is-subdir`. We deliberately do not canonicalize via the filesystem. +/// The guard runs before the bin file exists at its final location, and +/// pnpm's implementation is purely lexical too. +fn is_subdir(parent: &Path, child: &Path) -> bool { + let parent_norm = lexical_normalize(parent); + let child_norm = lexical_normalize(child); + child_norm.starts_with(&parent_norm) +} + +#[cfg(test)] +mod tests; diff --git a/pacquet/crates/cmd-shim/src/bin_resolver/tests.rs b/pacquet/crates/cmd-shim/src/bin_resolver/tests.rs new file mode 100644 index 0000000000..0e4cba87a8 --- /dev/null +++ b/pacquet/crates/cmd-shim/src/bin_resolver/tests.rs @@ -0,0 +1,503 @@ +use super::{get_bins_from_package_manifest, pkg_owns_bin}; +use crate::{capabilities::RealApi, path_util::lexical_normalize}; +use pipe_trait::Pipe; +use serde_json::json; +use std::{ + fs::{create_dir_all, write as write_file}, + path::{Path, PathBuf}, +}; +use tempfile::tempdir; + +#[test] +fn bin_as_string_uses_package_name() { + let manifest = json!({"name": "foo", "bin": "cli.js"}); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/pkg/foo")); + assert_eq!(commands.len(), 1); + assert_eq!(commands[0].name, "foo"); + assert_eq!(commands[0].path, Path::new("/pkg/foo/cli.js")); +} + +#[test] +fn bin_as_string_strips_scope() { + let manifest = json!({"name": "@scope/foo", "bin": "cli.js"}); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/pkg/foo")); + assert_eq!(commands.len(), 1); + assert_eq!(commands[0].name, "foo"); +} + +#[test] +fn bin_as_object_keeps_keys_and_strips_scope() { + let manifest = json!({ + "name": "tool", + "bin": { + "tool": "bin/tool.js", + "@scope/extra": "bin/extra.js", + }, + }); + let mut commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); + commands.sort_by(|a, b| a.name.cmp(&b.name)); + assert_eq!(commands.len(), 2); + assert_eq!(commands[0].name, "extra"); + assert_eq!(commands[1].name, "tool"); +} + +#[test] +fn rejects_unsafe_bin_names() { + let manifest = json!({ + "name": "x", + "bin": { + "good-name": "ok.js", + "../bad": "evil.js", + "with space": "no.js", + "$": "dollar.js", + }, + }); + let mut names: Vec<_> = get_bins_from_package_manifest::(&manifest, Path::new("/p")) + .into_iter() + .map(|c| c.name) + .collect(); + names.sort(); + assert_eq!(names, vec!["$".to_string(), "good-name".to_string()]); +} + +#[test] +fn rejects_path_traversal_outside_package_root() { + let manifest = json!({ + "name": "x", + "bin": {"x": "../../../etc/passwd"}, + }); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/pkg/x")); + assert!(commands.is_empty(), "must reject `..`-escapes from pkg root"); +} + +#[test] +fn no_bin_field_returns_empty() { + let manifest = json!({"name": "x"}); + assert!(get_bins_from_package_manifest::(&manifest, Path::new("/p")).is_empty()); +} + +#[test] +fn pkg_owns_bin_default_rule() { + assert!(pkg_owns_bin("foo", "foo")); + assert!(!pkg_owns_bin("foo", "bar")); +} + +#[test] +fn pkg_owns_bin_overrides() { + assert!(pkg_owns_bin("npx", "npm")); + assert!(pkg_owns_bin("pnpx", "pnpm")); + assert!(pkg_owns_bin("pnpx", "@pnpm/exe")); + assert!(!pkg_owns_bin("npx", "anything-else")); +} + +/// Mirrors pnpm's "should allow $ as command name" test +/// (). +/// `$` is the documented escape hatch for awkward bin names; it must +/// survive the URL-safe-name guard. +#[test] +fn dollar_is_allowed_as_command_name() { + let manifest = json!({ + "name": "undollar", + "version": "1.0.0", + "bin": {"$": "./undollar.js"}, + }); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); + assert_eq!(commands.len(), 1); + assert_eq!(commands[0].name, "$"); +} + +/// Mirrors pnpm's "skip dangerous bin names" test +/// (). +/// Path-traversal characters in the *key* must be filtered, not just the +/// value. +#[test] +fn skip_dangerous_bin_names() { + let manifest = json!({ + "name": "foo", + "version": "1.0.0", + "bin": { + "../bad": "./bad", + "..\\bad": "./bad", + "good": "./good", + "~/bad": "./bad", + }, + }); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); + assert_eq!(commands.len(), 1); + assert_eq!(commands[0].name, "good"); +} + +/// Mirrors pnpm's "skip dangerous bin locations" test +/// (). +/// `../bad` in the *value* must be filtered by the `is_subdir` check. +#[test] +fn skip_dangerous_bin_locations() { + let manifest = json!({ + "name": "foo", + "version": "1.0.0", + "bin": { + "bad": "../bad", + "good": "./good", + }, + }); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/pkg/foo")); + assert_eq!(commands.len(), 1); + assert_eq!(commands[0].name, "good"); +} + +/// Mirrors pnpm's "get bin from scoped bin name" test +/// (). +/// A scoped key like `@foo/a` collapses to `a` before validation. +#[test] +fn scoped_bin_name_strips_scope_prefix() { + let manifest = json!({ + "name": "@foo/a", + "version": "1.0.0", + "bin": {"@foo/a": "./a"}, + }); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); + assert_eq!(commands.len(), 1); + assert_eq!(commands[0].name, "a"); +} + +/// Mirrors pnpm's "skip scoped bin names with path traversal" test +/// (). +/// After the scope strip, the resulting bare name still has to pass the +/// URL-safe guard. A `@scope/../etc/passwd` collapses to `../etc/passwd` +/// which must be rejected. +#[test] +fn skip_scoped_bin_names_with_path_traversal() { + let manifest = json!({ + "name": "malicious", + "version": "1.0.0", + "bin": { + "@scope/../../.npmrc": "./malicious.js", + "@scope/../etc/passwd": "./evil.js", + "@scope/legit": "./good.js", + }, + }); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); + assert_eq!(commands.len(), 1); + assert_eq!(commands[0].name, "legit"); +} + +/// `bin` as a non-string non-object (number, array, null) is malformed. +/// Pacquet's port must return an empty list rather than panic, mirroring +/// pnpm's silent fall-through to the empty default. +#[test] +fn malformed_bin_type_returns_empty() { + for shape in [json!(42), json!(["a", "b"]), json!(null), json!(true)] { + let manifest = json!({"name": "x", "version": "1.0.0", "bin": shape}); + assert!( + get_bins_from_package_manifest::(&manifest, Path::new("/p")).is_empty(), + "malformed bin shape must be tolerated", + ); + } +} + +/// `bin` as a string requires the manifest to declare `name` (mirrors +/// pnpm's `INVALID_PACKAGE_NAME` guard). Pacquet returns an empty list +/// rather than throwing because the install pipeline would have already +/// surfaced a missing-name failure upstream. +#[test] +fn bin_string_with_missing_package_name_returns_empty() { + let manifest = json!({"bin": "cli.js"}); + assert!(get_bins_from_package_manifest::(&manifest, Path::new("/p")).is_empty()); +} + +/// Object-form bin entries whose values aren't strings (number, null, etc.) +/// are silently skipped. Same defensive shape pnpm has for malformed +/// manifests. +#[test] +fn bin_object_with_non_string_value_is_skipped() { + let manifest = json!({ + "name": "tool", + "version": "1.0.0", + "bin": { + "good": "ok.js", + "bad-num": 42, + "bad-null": null, + }, + }); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); + assert_eq!(commands.len(), 1); + assert_eq!(commands[0].name, "good"); +} + +/// Mirrors pnpm's "find all the bin files from a bin directory" +/// (). +/// Every regular file under `directories.bin`, including files in +/// subdirectories, becomes a command. +#[test] +fn directories_bin_walks_files_recursively() { + let tmp = tempdir().unwrap(); + let pkg = tmp.path().join("pkg"); + let bin_dir = pkg.join("bin-dir"); + create_dir_all(bin_dir.join("subdir")).unwrap(); + write_file(bin_dir.join("rootBin.js"), "").unwrap(); + write_file(bin_dir.join("subdir/subBin.js"), "").unwrap(); + + let manifest = json!({ + "name": "bin-dir", + "version": "1.0.0", + "directories": {"bin": "bin-dir"}, + }); + let mut commands = get_bins_from_package_manifest::(&manifest, &pkg); + commands.sort_by(|a, b| a.name.cmp(&b.name)); + assert_eq!(commands.len(), 2); + assert_eq!(commands[0].name, "rootBin.js"); + assert_eq!(commands[0].path, bin_dir.join("rootBin.js")); + assert_eq!(commands[1].name, "subBin.js"); + assert_eq!(commands[1].path, bin_dir.join("subdir/subBin.js")); +} + +/// Mirrors pnpm's "skip directories.bin with path traversal" +/// (). +/// `directories.bin: '../sibling'` must be rejected by the `is_subdir` +/// guard. The sibling directory is populated with a real file so a +/// regression that disables `is_subdir` would observably emit that file +/// as a command. Without the file the test would pass for the wrong +/// reason (empty dir, hence empty commands). +#[test] +fn directories_bin_rejects_path_traversal() { + let tmp = tempdir().unwrap(); + let pkg = tmp.path().join("pkg"); + create_dir_all(&pkg).unwrap(); + + // Sibling dir reachable via `../siblings` from the package root, + // populated with a "smoking gun" file the resolver would emit if + // it failed to reject the traversal. + let siblings = tmp.path().join("siblings"); + create_dir_all(&siblings).unwrap(); + write_file(siblings.join("smoking-gun"), "").unwrap(); + + let manifest = json!({ + "name": "malicious", + "version": "1.0.0", + "directories": {"bin": "../siblings"}, + }); + assert!( + get_bins_from_package_manifest::(&manifest, &pkg).is_empty(), + "is_subdir guard must reject `..`-escapes from the pkg root, even \ + when the resolved directory exists and has files", + ); +} + +/// Mirrors pnpm's `path-traversal.test.ts` +/// (). +/// A `directories.bin` value that resolves outside the package root via +/// `..` must yield no commands, even though the target dir exists and +/// has files in it. +#[test] +fn directories_bin_rejects_real_path_traversal() { + let tmp = tempdir().unwrap(); + let secret_dir = tmp.path().join("secret"); + create_dir_all(&secret_dir).unwrap(); + write_file(secret_dir.join("secret.sh"), "echo secret").unwrap(); + + let pkg = tmp.path().join("pkg"); + create_dir_all(&pkg).unwrap(); + + // From `/pkg`, `../secret` reaches `/secret`. The + // `is_subdir` guard must reject this even though the resolved path + // exists and contains files. + let manifest = json!({ + "name": "malicious", + "version": "1.0.0", + "directories": {"bin": "../secret"}, + }); + assert!(get_bins_from_package_manifest::(&manifest, &pkg).is_empty()); +} + +/// `directories.bin` pointing at a non-existent subdirectory must +/// degrade to an empty list (pnpm's `ENOENT` swallowing in `findFiles`). +#[test] +fn directories_bin_missing_directory_returns_empty() { + let tmp = tempdir().unwrap(); + let pkg = tmp.path().join("pkg"); + create_dir_all(&pkg).unwrap(); + let manifest = json!({ + "name": "x", + "version": "1.0.0", + "directories": {"bin": "missing-dir"}, + }); + assert!(get_bins_from_package_manifest::(&manifest, &pkg).is_empty()); +} + +/// `directories.bin` filters out files whose basename fails the +/// URL-safe-name guard. Pin via a `..` filename: once the +/// path-traversal guard already passed (the dir was a real subdir), +/// a *file* inside it with an unsafe name still gets dropped. +#[test] +fn directories_bin_filters_unsafe_file_names() { + let tmp = tempdir().unwrap(); + let pkg = tmp.path().join("pkg"); + let bin_dir = pkg.join("bin"); + create_dir_all(&bin_dir).unwrap(); + write_file(bin_dir.join("good"), "").unwrap(); + write_file(bin_dir.join("bad space"), "").unwrap(); + + let manifest = json!({ + "name": "tool", + "version": "1.0.0", + "directories": {"bin": "bin"}, + }); + let mut commands = get_bins_from_package_manifest::(&manifest, &pkg); + commands.sort_by(|a, b| a.name.cmp(&b.name)); + assert_eq!(commands.len(), 1); + assert_eq!(commands[0].name, "good"); +} + +/// Empty bin name returns false via the `is_empty` guard inside +/// `is_safe_bin_name`. Exercised via a `bin` object with an empty key. +#[test] +fn empty_bin_key_is_rejected() { + let manifest = json!({ + "name": "x", + "version": "1.0.0", + "bin": {"": "ok.js", "good": "ok.js"}, + }); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/p")); + assert_eq!(commands.len(), 1); + assert_eq!(commands[0].name, "good"); +} + +/// [`lexical_normalize`] drops `.` (CurDir) segments. This is a direct +/// test on the helper. The integration-style test below covers the same +/// arm via `directories.bin`, but a direct assertion makes the CurDir +/// branch visible to coverage tooling that can't see through inlined +/// call chains. +#[test] +fn lexical_normalize_drops_curdir_segments_directly() { + assert_eq!(lexical_normalize(Path::new("a/./b")), PathBuf::from("a/b")); + assert_eq!(lexical_normalize(Path::new("./a/b")), PathBuf::from("a/b")); + assert_eq!(lexical_normalize(Path::new("a/b/.")), PathBuf::from("a/b")); + assert_eq!(lexical_normalize(Path::new("./.")), PathBuf::new()); +} + +/// On an absolute path, a `..` that would escape the root is dropped +/// instead of being materialised as a literal `..` segment. Mirrors +/// Node's `path.resolve` (and therefore pnpm's `is-subdir`), where +/// `path.resolve("/a/../../a/bin.js") === "/a/bin.js"`. +/// +/// Without this guard, `is_subdir("/a", "/a/../../a/bin.js")` would +/// reject the path as outside `/a` even though it resolves back +/// inside: `lexical_normalize` would have produced `/../a/bin.js`, +/// which fails `starts_with("/a")`. +#[test] +fn lexical_normalize_drops_excess_parent_dirs_on_absolute_paths() { + assert_eq!(lexical_normalize(Path::new("/a/../../a/bin.js")), PathBuf::from("/a/bin.js")); + assert_eq!(lexical_normalize(Path::new("/..")), PathBuf::from("/")); + assert_eq!(lexical_normalize(Path::new("/../..")), PathBuf::from("/")); +} + +/// `is_subdir` accepts a path that goes outside the package root and +/// comes back inside via `..`. Regression coverage for the lexical +/// normalisation bug where `//x/../..//bin.js` was rejected +/// because the `..` past the root was materialised as a literal +/// `/..` segment. Mirrors pnpm's `path.resolve`-based `is-subdir`. +#[test] +fn directories_bin_accepts_excess_parent_dirs_that_resolve_inside_pkg() { + let tmp = tempdir().unwrap(); + let pkg = tmp.path().join("pkg"); + let bin_dir = pkg.join("bin-dir"); + create_dir_all(&bin_dir).unwrap(); + // `x` must exist on disk so the walker can follow the literal + // `/x/../../pkg/bin-dir` path the resolver builds. + create_dir_all(pkg.join("x")).unwrap(); + write_file(bin_dir.join("cli"), "").unwrap(); + + // `/x/../..//bin-dir` resolves back inside ``. + let manifest = json!({ + "name": "tool", + "version": "1.0.0", + "directories": {"bin": "x/../../pkg/bin-dir"}, + }); + let commands = get_bins_from_package_manifest::(&manifest, &pkg); + assert_eq!(commands.len(), 1); + assert_eq!(commands[0].name, "cli"); +} + +/// [`lexical_normalize`] `CurDir` branch drops `.` segments. Visible +/// via [`super::is_subdir`] accepting a target with embedded `./` +/// that resolves inside the package root. +#[test] +fn directories_bin_handles_curdir_in_relative_path() { + let tmp = tempdir().unwrap(); + let pkg = tmp.path().join("pkg"); + let bin_dir = pkg.join("bin-dir"); + create_dir_all(&bin_dir).unwrap(); + write_file(bin_dir.join("cli"), "").unwrap(); + + let manifest = json!({ + "name": "tool", + "version": "1.0.0", + "directories": {"bin": "./bin-dir"}, + }); + let commands = get_bins_from_package_manifest::(&manifest, &pkg); + assert_eq!(commands.len(), 1); + assert_eq!(commands[0].name, "cli"); +} + +/// `commands_from_directories_bin` skips entries whose path doesn't +/// yield a usable file name. That covers the `path.file_name() == None` +/// and `to_str() == None` branches, both of which the real fs hardly +/// ever reaches (file_name() returns None only for paths ending in +/// `..`, and to_str() fails only on non-UTF-8 bytes which are rare on +/// Unix and impossible on Windows). A fake [`FsWalkFiles`] hands back one +/// such path so the `continue` arm gets exercised directly. The +/// regular `cli` entry alongside it confirms that the well-formed +/// path still flows through and emits a [`Command`](super::Command). +#[test] +fn directories_bin_skips_path_without_usable_file_name() { + use crate::capabilities::FsWalkFiles; + use std::io; + + struct EvilWalker; + impl FsWalkFiles for EvilWalker { + fn walk_files(_: &Path) -> io::Result> { + [ + // `file_name()` returns None for a path ending in `..`, + // hitting the `let-else continue` branch. + PathBuf::from("/pkg/bin/.."), + // Well-formed sibling so we can assert the loop's + // happy path still runs after the skip. + PathBuf::from("/pkg/bin/cli"), + ] + .into_iter() + .pipe(Ok) + } + } + + let manifest = json!({ + "name": "tool", + "version": "1.0.0", + "directories": {"bin": "bin"}, + }); + let commands = get_bins_from_package_manifest::(&manifest, Path::new("/pkg")); + assert_eq!(commands.len(), 1, "the `..` entry must be skipped, not crashed on"); + assert_eq!(commands[0].name, "cli"); +} + +/// `bin` field takes precedence over `directories.bin` when both are +/// present. Mirrors upstream's order-of-checks in +/// `getBinsFromPackageManifest`. +#[test] +fn bin_field_takes_precedence_over_directories_bin() { + let tmp = tempdir().unwrap(); + let pkg = tmp.path().join("pkg"); + let bin_dir = pkg.join("legacy-bin"); + create_dir_all(&bin_dir).unwrap(); + write_file(bin_dir.join("ignored.js"), "").unwrap(); + write_file(pkg.join("primary.js"), "").unwrap(); + + let manifest = json!({ + "name": "tool", + "version": "1.0.0", + "bin": "primary.js", + "directories": {"bin": "legacy-bin"}, + }); + let commands = get_bins_from_package_manifest::(&manifest, &pkg); + assert_eq!(commands.len(), 1, "bin field wins, directories.bin is ignored"); + assert_eq!(commands[0].name, "tool"); +} diff --git a/pacquet/crates/cmd-shim/src/capabilities.rs b/pacquet/crates/cmd-shim/src/capabilities.rs new file mode 100644 index 0000000000..c4d824e7ac --- /dev/null +++ b/pacquet/crates/cmd-shim/src/capabilities.rs @@ -0,0 +1,241 @@ +//! Per-capability dependency-injection traits and the production +//! [`RealApi`] provider. Mirrors the pattern documented at +//! : +//! +//! 1. One trait per capability. +//! 2. Functions bind only what they consume (compose bounds). +//! 3. No `&self` on capability methods. +//! 4. Production callers turbofish the real impl explicitly. +//! +//! Tests inject unit-struct fakes to exercise IO error paths that the +//! real filesystem can't reach portably (e.g. permission denied, +//! ENOSPC). + +use pipe_trait::Pipe; +use std::{ + io, + path::{Path, PathBuf}, +}; + +/// Read up to `buf.len()` bytes of `path` starting at byte `offset`. +/// +/// The returned `usize` is the number of bytes actually written into +/// `buf`. Like `std::io::Read::read`, an impl is allowed to return +/// fewer bytes than requested (a "short read") even when more data is +/// available, so callers that need a fully-filled buffer must loop. +/// [`crate::read_head_filled`] supplies that loop while staying +/// generic over this trait, so test fakes do not have to grow. +/// +/// The trait makes no claim about how many syscalls a particular +/// impl will use — the production `RealApi` impl opens the file, +/// seeks to `offset` (if non-zero), and reads, which is more than +/// one. What it does promise is the semantic contract: read up to +/// `buf.len()` bytes starting at `offset` into `buf`. +/// +/// Used by [`crate::search_script_runtime`] (via [`crate::read_head_filled`]) +/// to detect the script runtime via the shebang at the head of a bin +/// file. +pub trait FsReadHead { + fn read_head(path: &Path, offset: u64, buf: &mut [u8]) -> io::Result; +} + +/// Read the entire contents of a file into a `Vec`. Used to read +/// `package.json` files when collecting bin sources. +pub trait FsReadFile { + fn read_file(path: &Path) -> io::Result>; +} + +/// Read the entire contents of a file into a `String`. Used by +/// [`crate::link_bins_of_packages`] to short-circuit on warm reinstalls +/// where the existing shim already targets the same bin file. +pub trait FsReadString { + fn read_to_string(path: &Path) -> io::Result; +} + +/// List the entries of a directory. +/// +/// Returns an `impl Iterator` rather than a +/// `Vec`, so the production impl can stream entries straight +/// out of `fs::ReadDir` without materialising the whole list. The +/// associated-type-free shape also frees fakes from declaring an +/// `Iter` type per impl. Each fake just returns whatever concrete +/// iterator it wants. +/// +/// We deliberately do not expose `fs::ReadDir` directly: its iterator +/// type is platform-specific and yields `io::Result`, +/// which would force every fake to fabricate a `DirEntry` (and tie +/// the trait to libstd's filesystem types). Yielding plain +/// `PathBuf` keeps fakes trivial. +pub trait FsReadDir { + fn read_dir(path: &Path) -> io::Result>; +} + +/// Recursively walk `path` and yield every regular file found beneath +/// it (depth-first, no symlink follow). Used by +/// [`crate::get_bins_from_package_manifest`] to enumerate +/// `directories.bin` entries. +/// +/// Returns an `impl Iterator` rather than a +/// `Vec`, so the production walker streams entries straight +/// out of `walkdir` instead of materialising the whole list up front. +/// `directories.bin` trees are usually tiny in practice, but the +/// abstraction should not bake in an allocation the real +/// implementation does not need. Fakes return whatever concrete +/// iterator they want. [`std::iter::empty`] fits the unreachable-walk +/// case, and [`Vec::into_iter`] fits the case that feeds a fixed list +/// of paths. +/// +/// `walkdir`'s builder exposes many knobs (`follow_links`, `min_depth`, +/// `max_depth`, `sort_by`, and so on); pacquet uses just one +/// (`follow_links = false`). Mirroring the full builder through the +/// trait would be over-engineering for the single call site, so the +/// trait keeps its surface dead-simple and the impl bakes the option +/// in. If a future caller needs different walk options, add a new +/// capability rather than parameterise this one. +pub trait FsWalkFiles { + fn walk_files(path: &Path) -> io::Result>; +} + +/// Create a directory and any missing ancestors. Used to prepare +/// `/.bin` and per-slot `node_modules/.bin` directories. +pub trait FsCreateDirAll { + fn create_dir_all(path: &Path) -> io::Result<()>; +} + +/// Write `bytes` to `path`, replacing the file's contents if it +/// exists. Used to write the three shim flavors (`.sh`, `.cmd`, +/// `.ps1`). +/// +/// **Not atomic.** This trait is the moral equivalent of +/// `std::fs::write`: it opens (or creates and truncates) the file, +/// writes `bytes`, and closes. No tempfile + rename guard, no +/// `fsync`. A SIGINT or crash mid-write can leave a truncated file +/// on disk. Number of syscalls is up to the impl — `std::fs::write` +/// itself is open/(truncate)/write/close, and a fake might loop. +/// If a future caller needs atomic write semantics, build it on top +/// of this trait by writing to a sibling tempfile and then +/// renaming. Hiding that algorithm inside the capability would +/// obscure what each callsite inherits; keeping the trait minimal +/// lets every callsite see exactly what guarantees it gets. +pub trait FsWrite { + fn write(path: &Path, bytes: &[u8]) -> io::Result<()>; +} + +/// Replace the permission bits at `path` with `0o755`. Used to chmod +/// the freshly written shim file so it is executable. +/// +/// The method is always present so callers don't have to +/// `#[cfg(unix)]` every chmod call site. On Windows the production +/// impl is a no-op (Windows has no equivalent permission concept). +pub trait FsSetExecutable { + fn set_executable(path: &Path) -> io::Result<()>; +} + +/// Read the existing permission bits at `path`, OR in `0o111`, and +/// write them back. Used to add the executable bits to the underlying +/// target binary (mirrors pnpm's `fixBin`) without clobbering the +/// existing read/write bits the way [`FsSetExecutable`] would. +/// +/// The method is always present for the same reason as +/// [`FsSetExecutable::set_executable`]; the production impl is a +/// no-op on Windows. +pub trait FsEnsureExecutableBits { + fn ensure_executable_bits(path: &Path) -> io::Result<()>; +} + +/// The production filesystem provider. Every method delegates straight +/// to `std::fs`. +pub struct RealApi; + +impl FsReadHead for RealApi { + fn read_head(path: &Path, offset: u64, buf: &mut [u8]) -> io::Result { + use std::io::{Read, Seek, SeekFrom}; + let mut file = std::fs::File::open(path)?; + if offset > 0 { + file.seek(SeekFrom::Start(offset))?; + } + file.read(buf) + } +} + +impl FsReadFile for RealApi { + fn read_file(path: &Path) -> io::Result> { + std::fs::read(path) + } +} + +impl FsReadString for RealApi { + fn read_to_string(path: &Path) -> io::Result { + std::fs::read_to_string(path) + } +} + +impl FsReadDir for RealApi { + fn read_dir(path: &Path) -> io::Result> { + // `flatten()` silently drops per-entry errors. This matches the + // prior collect-then-flatten shape and the `tinyglobby`-style + // ENOENT-on-subtree behaviour pacquet's callers expect. + std::fs::read_dir(path)?.flatten().map(|entry| entry.path()).pipe(Ok) + } +} + +impl FsWalkFiles for RealApi { + fn walk_files(path: &Path) -> io::Result> { + // `flatten()` silently drops per-entry errors and matches + // pnpm's `tinyglobby` ENOENT-on-subtree behaviour. The + // top-level missing-dir case also flows through here as a + // single dropped `Err`, so a missing `bin_dir` produces an + // empty stream rather than an error. + path.pipe(walkdir::WalkDir::new) + .follow_links(false) + .into_iter() + .flatten() + .filter(|entry| entry.file_type().is_file()) + .map(|entry| entry.path().to_path_buf()) + .pipe(Ok) + } +} + +impl FsCreateDirAll for RealApi { + fn create_dir_all(path: &Path) -> io::Result<()> { + std::fs::create_dir_all(path) + } +} + +impl FsWrite for RealApi { + fn write(path: &Path, bytes: &[u8]) -> io::Result<()> { + std::fs::write(path, bytes) + } +} + +#[cfg(unix)] +impl FsSetExecutable for RealApi { + fn set_executable(path: &Path) -> io::Result<()> { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o755)) + } +} + +#[cfg(not(unix))] +impl FsSetExecutable for RealApi { + fn set_executable(_path: &Path) -> io::Result<()> { + Ok(()) + } +} + +#[cfg(unix)] +impl FsEnsureExecutableBits for RealApi { + fn ensure_executable_bits(path: &Path) -> io::Result<()> { + use std::os::unix::fs::PermissionsExt; + let metadata = std::fs::metadata(path)?; + let mode = metadata.permissions().mode() | 0o111; + std::fs::set_permissions(path, std::fs::Permissions::from_mode(mode)) + } +} + +#[cfg(not(unix))] +impl FsEnsureExecutableBits for RealApi { + fn ensure_executable_bits(_path: &Path) -> io::Result<()> { + Ok(()) + } +} diff --git a/pacquet/crates/cmd-shim/src/lib.rs b/pacquet/crates/cmd-shim/src/lib.rs new file mode 100644 index 0000000000..a949aeaf68 --- /dev/null +++ b/pacquet/crates/cmd-shim/src/lib.rs @@ -0,0 +1,29 @@ +//! Bin field parsing and command-shim generation. +//! +//! Mirrors three pnpm v11 packages: +//! +//! - `@pnpm/bins.resolver` parses the `bin` (and `directories.bin`) field of a +//! package's `package.json` into a list of `(name, path)` commands. See +//! . +//! - `@pnpm/bins.linker` orchestrates the per-`node_modules` linking pass and +//! the conflict resolution between bins of the same name. See +//! . +//! - `@zkochan/cmd-shim` generates the actual shim file contents. See +//! . +//! +//! Pacquet's first iteration covers the direct-dependency path (root project's +//! `node_modules/.bin`) and the per-virtual-store path +//! (`node_modules/.pacquet/@/node_modules//node_modules/.bin`). +//! Hoisted-bin precedence and lifecycle-script-created bins are deferred per +//! `plans/TEST_PORTING.md`. + +mod bin_resolver; +mod capabilities; +mod link_bins; +mod path_util; +mod shim; + +pub use bin_resolver::*; +pub use capabilities::*; +pub use link_bins::*; +pub use shim::*; diff --git a/pacquet/crates/cmd-shim/src/link_bins.rs b/pacquet/crates/cmd-shim/src/link_bins.rs new file mode 100644 index 0000000000..6c7627afe4 --- /dev/null +++ b/pacquet/crates/cmd-shim/src/link_bins.rs @@ -0,0 +1,396 @@ +use crate::{ + bin_resolver::{Command, get_bins_from_package_manifest, pkg_owns_bin}, + capabilities::{ + FsCreateDirAll, FsEnsureExecutableBits, FsReadDir, FsReadFile, FsReadHead, FsReadString, + FsSetExecutable, FsWalkFiles, FsWrite, + }, + shim::{ + generate_cmd_shim, generate_pwsh_shim, generate_sh_shim, is_shim_pointing_at, + search_script_runtime, + }, +}; +use derive_more::{Display, Error}; +use miette::Diagnostic; +use rayon::prelude::*; +use serde_json::Value; +use std::{ + collections::HashMap, + io, + path::{Path, PathBuf}, + sync::Arc, +}; + +/// One package known to be installed at `location`, with its parsed +/// `package.json`. Mirrors the per-package input shape of pnpm's +/// `linkBinsOfPackages`. +/// +/// The manifest is shared via `Arc` rather than owned by value: the +/// lockfile-driven bin-link path looks up the same parsed manifest +/// from a process-wide map, so packing it into a [`PackageBinSource`] +/// is a refcount bump (cheap) rather than a deep clone of the JSON +/// tree (which would have been the bulk of the per-slot CPU work, +/// since the per-install clone count is `slots × children` = +/// thousands of times). +#[derive(Debug, Clone)] +pub struct PackageBinSource { + pub location: PathBuf, + pub manifest: Arc, +} + +/// Error type for [`link_bins_of_packages`]. +#[derive(Debug, Display, Error, Diagnostic)] +pub enum LinkBinsError { + #[display("Failed to create bin directory at {dir:?}: {error}")] + #[diagnostic(code(pacquet_cmd_shim::create_bin_dir))] + CreateBinDir { + dir: PathBuf, + #[error(source)] + error: io::Error, + }, + + #[display("Failed to read modules directory at {dir:?}: {error}")] + #[diagnostic(code(pacquet_cmd_shim::read_modules_dir))] + ReadModulesDir { + dir: PathBuf, + #[error(source)] + error: io::Error, + }, + + #[display("Failed to read package manifest at {path:?}: {error}")] + #[diagnostic(code(pacquet_cmd_shim::read_manifest))] + ReadManifest { + path: PathBuf, + #[error(source)] + error: io::Error, + }, + + #[display("Failed to parse package manifest at {path:?}: {error}")] + #[diagnostic(code(pacquet_cmd_shim::parse_manifest))] + ParseManifest { + path: PathBuf, + #[error(source)] + error: serde_json::Error, + }, + + #[display("Failed to read shim source {path:?}: {error}")] + #[diagnostic(code(pacquet_cmd_shim::probe_shim_source))] + ProbeShimSource { + path: PathBuf, + #[error(source)] + error: io::Error, + }, + + #[display("Failed to write shim file at {path:?}: {error}")] + #[diagnostic(code(pacquet_cmd_shim::write_shim))] + WriteShim { + path: PathBuf, + #[error(source)] + error: io::Error, + }, + + #[display("Failed to chmod {path:?}: {error}")] + #[diagnostic(code(pacquet_cmd_shim::chmod))] + Chmod { + path: PathBuf, + #[error(source)] + error: io::Error, + }, +} + +/// Read `/package.json` for each entry under `modules_dir` and link +/// its bins into `bins_dir`. Mirrors pnpm v11's `linkBins(modulesDir, binsDir)` +/// at . +/// +/// Skips: +/// - The `.bin` and `.pacquet` directories themselves (and any other +/// dot-prefixed entry, matching pnpm). +/// - Entries whose `package.json` cannot be read (legitimate when a directory +/// under `node_modules` happens to not be a package, e.g. an empty scope +/// directory). +/// +/// Scoped packages are recursed: `node_modules/@scope/foo` becomes one +/// candidate. This mirrors `binNamesAndPaths` in upstream `linkBins`. +pub fn link_bins(modules_dir: &Path, bins_dir: &Path) -> Result<(), LinkBinsError> +where + Api: FsReadDir + + FsReadFile + + FsReadString + + FsReadHead + + FsCreateDirAll + + FsWalkFiles + + FsWrite + + FsSetExecutable + + FsEnsureExecutableBits, +{ + let packages = collect_packages_in_modules_dir::(modules_dir)?; + link_bins_of_packages::(&packages, bins_dir) +} + +fn collect_packages_in_modules_dir( + modules_dir: &Path, +) -> Result, LinkBinsError> +where + Api: FsReadDir + FsReadFile, +{ + let mut packages = Vec::new(); + + let entries = match Api::read_dir(modules_dir) { + Ok(entries) => entries, + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(packages), + Err(error) => { + return Err(LinkBinsError::ReadModulesDir { dir: modules_dir.to_path_buf(), error }); + } + }; + + for path in entries { + let Some(name) = path.file_name() else { + continue; + }; + let name_str = name.to_string_lossy(); + if name_str.starts_with('.') { + continue; + } + + if name_str.starts_with('@') { + // Scoped: walk one level deeper. Only `NotFound` is + // plausibly skippable (a concurrent scope-dir delete); + // other errors — `PermissionDenied`, `EIO`, AppArmor + // deny — would silently drop every bin under this + // scope, so surface them as `ReadModulesDir`. Matches + // the policy the per-`modules_dir` read above already + // uses. + let scope_entries = match Api::read_dir(&path) { + Ok(entries) => entries, + Err(error) if error.kind() == io::ErrorKind::NotFound => continue, + Err(error) => { + return Err(LinkBinsError::ReadModulesDir { dir: path.clone(), error }); + } + }; + for sub_path in scope_entries { + if let Some(pkg) = read_package::(&sub_path)? { + packages.push(pkg); + } + } + continue; + } + + if let Some(pkg) = read_package::(&path)? { + packages.push(pkg); + } + } + + Ok(packages) +} + +fn read_package( + location: &Path, +) -> Result, LinkBinsError> { + let manifest_path = location.join("package.json"); + let bytes = match Api::read_file(&manifest_path) { + Ok(bytes) => bytes, + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(None), + Err(error) => return Err(LinkBinsError::ReadManifest { path: manifest_path, error }), + }; + let manifest: Value = serde_json::from_slice(&bytes) + .map_err(|error| LinkBinsError::ParseManifest { path: manifest_path, error })?; + Ok(Some(PackageBinSource { location: location.to_path_buf(), manifest: Arc::new(manifest) })) +} + +/// Link every bin declared by `packages` into `bins_dir`, applying the same +/// conflict resolution upstream uses. +/// +/// Conflict resolution mirrors `resolveCommandConflicts`: +/// +/// 1. Ownership wins. If exactly one package owns the bin name (via +/// [`pkg_owns_bin`]), it wins outright. +/// 2. Otherwise lexical comparison on the package name, lower wins. Stable +/// and deterministic regardless of the order packages were discovered. +/// +/// Pacquet's first iteration does not resolve same-package multi-version +/// conflicts via semver (a feature upstream uses for hoisting), since the +/// virtual-store layout means each bin source is a unique +/// `(package, version)` slot already. +pub fn link_bins_of_packages( + packages: &[PackageBinSource], + bins_dir: &Path, +) -> Result<(), LinkBinsError> +where + Api: FsReadString + + FsReadHead + + FsCreateDirAll + + FsWalkFiles + + FsWrite + + FsSetExecutable + + FsEnsureExecutableBits, +{ + let mut chosen: HashMap = HashMap::new(); + + for pkg in packages { + let pkg_name = pkg.manifest.get("name").and_then(Value::as_str).unwrap_or(""); + let commands = get_bins_from_package_manifest::(&pkg.manifest, &pkg.location); + for command in commands { + match chosen.get(&command.name) { + None => { + chosen.insert(command.name.clone(), (command, pkg)); + } + Some((_, existing)) => { + let existing_name = + existing.manifest.get("name").and_then(Value::as_str).unwrap_or(""); + if pick_winner(&command.name, existing_name, pkg_name) { + chosen.insert(command.name.clone(), (command, pkg)); + } + } + } + } + } + + if chosen.is_empty() { + return Ok(()); + } + + Api::create_dir_all(bins_dir) + .map_err(|error| LinkBinsError::CreateBinDir { dir: bins_dir.to_path_buf(), error })?; + + // Each shim's read-shebang + write-file + chmod sequence is independent + // across bin names. There is no shared state, so drive them on rayon. + // The hot path is per-package-bin; without parallelism the per-shim + // file I/O serialised across the whole `chosen` map. + chosen.par_iter().try_for_each(|(bin_name, (command, _pkg))| { + write_shim::(&command.path, &bins_dir.join(bin_name)) + })?; + + Ok(()) +} + +/// Return `true` when `candidate` should replace `existing` for `bin_name`. +/// Matches the two-step ownership-then-lexical-compare in upstream's +/// `resolveCommandConflicts`. +fn pick_winner(bin_name: &str, existing: &str, candidate: &str) -> bool { + let existing_owns = pkg_owns_bin(bin_name, existing); + let candidate_owns = pkg_owns_bin(bin_name, candidate); + match (existing_owns, candidate_owns) { + (true, false) => false, + (false, true) => true, + // Both own (or neither): fall through to lexical compare. Picking the + // smaller name keeps results deterministic across input orderings. + _ => candidate < existing, + } +} + +/// Write all three shim flavors for `target_path` (the canonical `.sh` +/// at `shim_path`, plus the `.cmd` and `.ps1` siblings) and chmod them +/// executable. Idempotent on warm reinstalls via [`is_shim_pointing_at`]. +/// +/// Pnpm always emits all three flavors per bin (independent of host +/// platform), so a project installed on Linux stays usable when the +/// same `node_modules` is reused from Windows via a network share or +/// a `git clone` of a checked-in install. Pacquet matches that +/// contract here: `generate_sh_shim`, `generate_cmd_shim`, and +/// `generate_pwsh_shim` are unconditional, and the writer emits all +/// three. +/// +/// The chmod step (`set_executable` for the canonical shim and +/// `ensure_executable_bits` for the target binary, matching pnpm's +/// `fixBin(cmd.path, 0o755)` and `chmodShim`) is wired through the +/// [`FsSetExecutable`] / [`FsEnsureExecutableBits`] capability traits. +/// On Unix the production impls run the actual `chmod`; on Windows +/// they are no-ops (Windows has no equivalent permission concept), so +/// the call sites stay portable and don't need their own +/// `#[cfg(unix)]` gating. +fn write_shim(target_path: &Path, shim_path: &Path) -> Result<(), LinkBinsError> +where + Api: FsReadString + FsReadHead + FsWrite + FsSetExecutable + FsEnsureExecutableBits, +{ + let runtime = search_script_runtime::(target_path).map_err(|error| { + LinkBinsError::ProbeShimSource { path: target_path.to_path_buf(), error } + })?; + + let sh_body = generate_sh_shim(target_path, shim_path, runtime.as_ref()); + let cmd_path = with_extension_appended(shim_path, "cmd"); + let ps1_path = with_extension_appended(shim_path, "ps1"); + let cmd_body = generate_cmd_shim(target_path, &cmd_path, runtime.as_ref()); + let ps1_body = generate_pwsh_shim(target_path, &ps1_path, runtime.as_ref()); + + // Idempotent skip only fires when all three flavors are already + // present *and pointing at the right target*. Gating on the `.sh` + // flavor alone (an earlier version of this code) left the upgrade + // path broken: a previous install (e.g. older pacquet, + // partial-write crash) might have written `.sh` correctly but + // never written `.cmd`/`.ps1`, in which case the marker check + // would short-circuit and the missing siblings would never be + // repaired. + // + // The `.sh` flavor carries a `# cmd-shim-target=` trailer + // that [`is_shim_pointing_at`] reads; the `.cmd` and `.ps1` + // flavors don't, so we compare them byte-for-byte against the + // freshly generated body. That catches stale/corrupted siblings + // that an existence-only check would let slip through (Copilot + // flagged this on + // ): + // a manually-edited `.cmd` pointing at a stale target, or an + // earlier pacquet write with a different relative path, would + // bypass the rewrite under the prior `.is_ok()` gate. Generated + // bodies are stable across pacquet versions (only the `` + // segment moves), so byte equality is a sound equivalence check. + let sh_marker_ok = matches!( + Api::read_to_string(shim_path), + Ok(existing) if is_shim_pointing_at(&existing, target_path), + ); + let cmd_ok = matches!( + Api::read_to_string(&cmd_path), + Ok(existing) if existing == cmd_body, + ); + let ps1_ok = matches!( + Api::read_to_string(&ps1_path), + Ok(existing) if existing == ps1_body, + ); + let already_correct = sh_marker_ok && cmd_ok && ps1_ok; + + if !already_correct { + Api::write(shim_path, sh_body.as_bytes()) + .map_err(|error| LinkBinsError::WriteShim { path: shim_path.to_path_buf(), error })?; + Api::write(&cmd_path, cmd_body.as_bytes()) + .map_err(|error| LinkBinsError::WriteShim { path: cmd_path.clone(), error })?; + Api::write(&ps1_path, ps1_body.as_bytes()) + .map_err(|error| LinkBinsError::WriteShim { path: ps1_path.clone(), error })?; + } + + Api::set_executable(shim_path) + .map_err(|error| LinkBinsError::Chmod { path: shim_path.to_path_buf(), error })?; + // Make the underlying script executable too. pnpm calls + // `fixBin(cmd.path, 0o755)` to do this; we apply the same minimum + // 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. `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(()) +} + +/// Append `` to `path` as a *new* extension segment (`foo` becomes +/// `foo.cmd`), regardless of any existing extension. `Path::with_extension` +/// would *replace* the existing extension, which is wrong for our case. +/// The bin name `tsc` keeps its own `tsc` and gains a sibling `tsc.cmd`, +/// rather than turning into `tsc.cmd` and losing the original `.sh` flavor. +fn with_extension_appended(path: &Path, ext: &str) -> PathBuf { + let mut result = path.as_os_str().to_owned(); + result.push("."); + result.push(ext); + result.into() +} + +#[cfg(test)] +mod tests; diff --git a/pacquet/crates/cmd-shim/src/link_bins/tests.rs b/pacquet/crates/cmd-shim/src/link_bins/tests.rs new file mode 100644 index 0000000000..74d1cd5561 --- /dev/null +++ b/pacquet/crates/cmd-shim/src/link_bins/tests.rs @@ -0,0 +1,931 @@ +use super::{LinkBinsError, PackageBinSource, link_bins, link_bins_of_packages}; +use crate::{ + capabilities::{ + FsCreateDirAll, FsEnsureExecutableBits, FsReadDir, FsReadFile, FsReadHead, FsReadString, + FsSetExecutable, FsWalkFiles, FsWrite, RealApi, + }, + shim::is_shim_pointing_at, +}; +use serde_json::{Value, json}; +use std::{ + fs::{ + create_dir_all, metadata, read as read_file, read_to_string, remove_file, + write as write_file, + }, + iter::{Empty, empty}, + path::{Path, PathBuf}, + sync::Arc, +}; +use tempfile::tempdir; + +/// All three shim flavors (`.sh` / no-extension, `.cmd`, `.ps1`) must +/// be written for every linked bin so a project installed on Linux +/// remains usable on Windows after a `git clone`. Mirrors pnpm's +/// always-write-all-flavors behavior. +#[test] +fn writes_all_three_shim_flavors_per_bin() { + let tmp = tempdir().unwrap(); + let pkg_dir = tmp.path().join("node_modules/foo"); + create_dir_all(&pkg_dir).unwrap(); + write_file( + pkg_dir.join("package.json"), + json!({"name": "foo", "version": "1.0.0", "bin": "cli.js"}).to_string(), + ) + .unwrap(); + write_file(pkg_dir.join("cli.js"), "#!/usr/bin/env node\n").unwrap(); + + let bins_dir = tmp.path().join("node_modules/.bin"); + let manifest_value: Value = + serde_json::from_slice(&read_file(pkg_dir.join("package.json")).unwrap()).unwrap(); + link_bins_of_packages::( + &[PackageBinSource { location: pkg_dir.clone(), manifest: Arc::new(manifest_value) }], + &bins_dir, + ) + .unwrap(); + + let sh = bins_dir.join("foo"); + let cmd = bins_dir.join("foo.cmd"); + let ps1 = bins_dir.join("foo.ps1"); + assert!(sh.exists(), "missing .sh shim"); + assert!(cmd.exists(), "missing .cmd shim"); + assert!(ps1.exists(), "missing .ps1 shim"); + + let cmd_body = read_to_string(&cmd).unwrap(); + assert!(cmd_body.starts_with("@SETLOCAL\r\n"), "cmd shim must use CRLF SETLOCAL"); + assert!(cmd_body.contains("\"%~dp0\\..\\foo\\cli.js\""), "cmd target should be windows-style"); + + let ps1_body = read_to_string(&ps1).unwrap(); + assert!(ps1_body.starts_with("#!/usr/bin/env pwsh\n")); + assert!(ps1_body.contains("\"$basedir/../foo/cli.js\"")); +} + +/// End-to-end exercise: a package with a `bin` field has a shim written +/// into the bins dir, the shim references the correct relative path, +/// and (on Unix) both the shim and the target are executable. +#[test] +fn writes_shim_for_bin_string() { + let tmp = tempdir().unwrap(); + let pkg_dir = tmp.path().join("node_modules/foo"); + create_dir_all(pkg_dir.join("bin")).unwrap(); + write_file( + pkg_dir.join("package.json"), + json!({"name": "foo", "version": "1.0.0", "bin": "bin/cli.js"}).to_string(), + ) + .unwrap(); + write_file(pkg_dir.join("bin/cli.js"), "#!/usr/bin/env node\n").unwrap(); + + let bins_dir = tmp.path().join("node_modules/.bin"); + let manifest_value: Value = + serde_json::from_slice(&read_file(pkg_dir.join("package.json")).unwrap()).unwrap(); + link_bins_of_packages::( + &[PackageBinSource { location: pkg_dir.clone(), manifest: Arc::new(manifest_value) }], + &bins_dir, + ) + .unwrap(); + + let shim_path = bins_dir.join("foo"); + assert!(shim_path.exists(), "shim should be created"); + + let body = read_to_string(&shim_path).unwrap(); + assert!(body.contains("\"$basedir/../foo/bin/cli.js\""), "shim body: {body}"); + assert!(is_shim_pointing_at(&body, &pkg_dir.join("bin/cli.js"))); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + assert_eq!( + metadata(&shim_path).unwrap().permissions().mode() & 0o777, + 0o755, + "shim must be 0o755", + ); + assert!( + metadata(pkg_dir.join("bin/cli.js")).unwrap().permissions().mode() & 0o111 != 0, + "target must have at least one executable bit", + ); + } +} + +/// [`link_bins::`](link_bins) walks every package and its scoped +/// children. Both regular and `@scope/...` packages must contribute their +/// bins. +#[test] +fn link_bins_walks_modules_and_scopes() { + let tmp = tempdir().unwrap(); + let modules = tmp.path().join("node_modules"); + // Regular package + create_dir_all(modules.join("foo")).unwrap(); + write_file(modules.join("foo/package.json"), json!({"name": "foo", "bin": "f.js"}).to_string()) + .unwrap(); + write_file(modules.join("foo/f.js"), "#!/usr/bin/env node\n").unwrap(); + // Scoped package + create_dir_all(modules.join("@s/bar")).unwrap(); + write_file( + modules.join("@s/bar/package.json"), + json!({"name": "@s/bar", "bin": "b.js"}).to_string(), + ) + .unwrap(); + write_file(modules.join("@s/bar/b.js"), "#!/usr/bin/env node\n").unwrap(); + // Non-package directory (no package.json) must be ignored, not error. + create_dir_all(modules.join("not-a-package")).unwrap(); + + let bins = modules.join(".bin"); + link_bins::(&modules, &bins).unwrap(); + + assert!(bins.join("foo").exists(), "foo shim must exist"); + assert!(bins.join("bar").exists(), "scoped @s/bar shim must use bare name `bar`"); +} + +/// [`link_bins`] on a missing `node_modules` directory must be a no-op +/// (Ok with empty result), not an error. Real fs returns `NotFound` +/// which the implementation already degrades. +#[test] +fn link_bins_handles_missing_modules_dir() { + let tmp = tempdir().unwrap(); + let bins_dir = tmp.path().join(".bin"); + link_bins::(&tmp.path().join("missing"), &bins_dir) + .expect("missing modules dir is Ok"); + assert!(!bins_dir.exists(), "no shims means no bin dir created"); +} + +/// [`link_bins_of_packages`] with no bins to link is a complete no-op. +/// It must not even create the bins directory. The empty-`chosen` +/// short-circuit guards a slot whose children have no bin field. +#[test] +fn link_bins_of_packages_no_op_when_no_bins() { + let tmp = tempdir().unwrap(); + let pkg = tmp.path().join("pkg"); + create_dir_all(&pkg).unwrap(); + write_file(pkg.join("package.json"), json!({"name": "pkg"}).to_string()).unwrap(); + let bins = tmp.path().join(".bin"); + let manifest: Value = + serde_json::from_slice(&read_file(pkg.join("package.json")).unwrap()).unwrap(); + link_bins_of_packages::( + &[PackageBinSource { location: pkg, manifest: Arc::new(manifest) }], + &bins, + ) + .unwrap(); + assert!(!bins.exists(), "bins dir must not be created when nothing to link"); +} + +/// Same-name bin from two non-owner packages: lexical-compare picks the +/// alphabetically smaller package name. Pins the +/// `resolveCommandConflicts` fallback shape. +#[test] +fn lexical_compare_breaks_tie_when_neither_owns() { + let tmp = tempdir().unwrap(); + let alpha = tmp.path().join("alpha"); + let beta = tmp.path().join("beta"); + for d in [&alpha, &beta] { + create_dir_all(d).unwrap(); + write_file(d.join("cmd.js"), "#!/usr/bin/env node\n").unwrap(); + } + write_file( + alpha.join("package.json"), + json!({"name": "alpha", "bin": {"shared": "cmd.js"}}).to_string(), + ) + .unwrap(); + write_file( + beta.join("package.json"), + json!({"name": "beta", "bin": {"shared": "cmd.js"}}).to_string(), + ) + .unwrap(); + + let manifest_alpha: Value = + serde_json::from_slice(&read_file(alpha.join("package.json")).unwrap()).unwrap(); + let manifest_beta: Value = + serde_json::from_slice(&read_file(beta.join("package.json")).unwrap()).unwrap(); + + let bins = tmp.path().join(".bin"); + // Order beta-then-alpha to verify the choice doesn't depend on + // discovery order. + link_bins_of_packages::( + &[ + PackageBinSource { location: beta.clone(), manifest: Arc::new(manifest_beta) }, + PackageBinSource { location: alpha.clone(), manifest: Arc::new(manifest_alpha) }, + ], + &bins, + ) + .unwrap(); + + let body = read_to_string(bins.join("shared")).unwrap(); + assert!( + body.contains("/alpha/cmd.js"), + "lexically smaller package name `alpha` must win, got body:\n{body}", + ); +} + +/// A malformed `package.json` (invalid JSON) under `` must +/// surface as a [`LinkBinsError::ParseManifest`] error, not silently skip. +#[test] +fn link_bins_propagates_parse_manifest_error() { + let tmp = tempdir().unwrap(); + let modules = tmp.path().join("node_modules"); + create_dir_all(modules.join("broken")).unwrap(); + write_file(modules.join("broken/package.json"), "{ this is not json").unwrap(); + + let bins = modules.join(".bin"); + let err = link_bins::(&modules, &bins).expect_err("invalid manifest must surface"); + assert!( + matches!(err, LinkBinsError::ParseManifest { .. }), + "expected ParseManifest, got {err:?}", + ); +} + +/// [`link_bins`] must idempotently short-circuit when an existing shim +/// already targets the same bin file. Pins [`is_shim_pointing_at`]'s +/// integration with the writer. Mirrors pnpm's +/// "linkBins() skips bins that already reference the correct target": +/// . +#[test] +fn link_bins_skips_existing_shim_with_matching_marker() { + let tmp = tempdir().unwrap(); + let modules = tmp.path().join("node_modules"); + create_dir_all(modules.join("foo")).unwrap(); + write_file(modules.join("foo/package.json"), json!({"name": "foo", "bin": "f.js"}).to_string()) + .unwrap(); + write_file(modules.join("foo/f.js"), "#!/usr/bin/env node\n").unwrap(); + + let bins = modules.join(".bin"); + link_bins::(&modules, &bins).unwrap(); + let original = read_to_string(bins.join("foo")).unwrap(); + // Append a sentinel. If the second pass rewrites the shim, the + // sentinel disappears. + let sentinel = format!("{original}\n# SENTINEL"); + write_file(bins.join("foo"), &sentinel).unwrap(); + + link_bins::(&modules, &bins).unwrap(); + assert_eq!(read_to_string(bins.join("foo")).unwrap(), sentinel); +} + +/// [`link_bins`] must NOT skip when only the canonical `.sh` shim exists. +/// The `.cmd` and `.ps1` siblings could be missing because an older +/// pacquet wrote `.sh`-only or because a partial-write crash interrupted +/// the writer mid-batch. Gating on the `.sh` marker alone (an earlier +/// version of [`super::write_shim`]) caused those upgrade paths to leave +/// the missing siblings permanently absent. +#[test] +fn link_bins_rewrites_when_only_sh_flavor_exists() { + let tmp = tempdir().unwrap(); + let modules = tmp.path().join("node_modules"); + create_dir_all(modules.join("foo")).unwrap(); + write_file(modules.join("foo/package.json"), json!({"name": "foo", "bin": "f.js"}).to_string()) + .unwrap(); + write_file(modules.join("foo/f.js"), "#!/usr/bin/env node\n").unwrap(); + + let bins = modules.join(".bin"); + link_bins::(&modules, &bins).unwrap(); + + // Simulate the partial-write / older-pacquet state: delete the + // .cmd and .ps1 siblings, leaving only the `.sh` shim with its + // (still correct) target marker. + remove_file(bins.join("foo.cmd")).unwrap(); + remove_file(bins.join("foo.ps1")).unwrap(); + + link_bins::(&modules, &bins).unwrap(); + + assert!(bins.join("foo").exists(), ".sh shim must remain"); + assert!(bins.join("foo.cmd").exists(), ".cmd sibling must be re-created on second pass"); + assert!(bins.join("foo.ps1").exists(), ".ps1 sibling must be re-created on second pass"); +} + +/// [`link_bins_of_packages`] propagates a non-`NotFound` `read_dir` +/// error from the calling context. Use a fake `Api` that fails the +/// initial `create_dir_all` to cover the [`LinkBinsError::CreateBinDir`] +/// error variant that real fs can't trigger portably. +#[test] +fn link_bins_propagates_create_bin_dir_error_via_di() { + use std::io; + + struct FailingCreateDir; + impl FsReadDir for FailingCreateDir { + fn read_dir(_: &Path) -> io::Result> { + Ok(empty()) + } + } + impl FsReadFile for FailingCreateDir { + fn read_file(_: &Path) -> io::Result> { + unreachable!("not called when chosen is empty") + } + } + impl FsReadString for FailingCreateDir { + fn read_to_string(_: &Path) -> io::Result { + unreachable!() + } + } + impl FsReadHead for FailingCreateDir { + fn read_head(_: &Path, _: u64, _: &mut [u8]) -> io::Result { + unreachable!() + } + } + impl FsCreateDirAll for FailingCreateDir { + fn create_dir_all(_: &Path) -> io::Result<()> { + Err(io::Error::from(io::ErrorKind::PermissionDenied)) + } + } + impl FsWrite for FailingCreateDir { + fn write(_: &Path, _: &[u8]) -> io::Result<()> { + unreachable!() + } + } + impl FsSetExecutable for FailingCreateDir { + fn set_executable(_: &Path) -> io::Result<()> { + unreachable!() + } + } + impl FsEnsureExecutableBits for FailingCreateDir { + fn ensure_executable_bits(_: &Path) -> io::Result<()> { + unreachable!() + } + } + impl FsWalkFiles for FailingCreateDir { + fn walk_files(_: &Path) -> io::Result> { + unreachable!("directories.bin not exercised by this test"); + #[expect(unreachable_code)] + Ok(empty()) + } + } + + // A package with a bin so `chosen` is non-empty. + let manifest = serde_json::json!({"name": "foo", "bin": "cli.js"}); + let tmp = tempdir().unwrap(); + let pkg = tmp.path().join("foo"); + create_dir_all(&pkg).unwrap(); + write_file(pkg.join("cli.js"), "#!/usr/bin/env node\n").unwrap(); + let err = link_bins_of_packages::( + &[PackageBinSource { location: pkg, manifest: Arc::new(manifest) }], + Path::new("/anything"), + ) + .expect_err("create_dir_all error must propagate"); + assert!(matches!(err, LinkBinsError::CreateBinDir { .. })); +} + +/// [`link_bins_of_packages`] propagates a write failure for the `.sh` +/// shim. Inject a fake [`FsWrite`] that always fails. +#[test] +fn link_bins_propagates_write_shim_error_via_di() { + use std::io; + + struct FailingWrite; + impl FsReadDir for FailingWrite { + fn read_dir(_: &Path) -> io::Result> { + Ok(empty()) + } + } + impl FsReadFile for FailingWrite { + fn read_file(_: &Path) -> io::Result> { + unreachable!() + } + } + impl FsReadString for FailingWrite { + fn read_to_string(_: &Path) -> io::Result { + // Pretend no existing shim, forcing the writer path. + Err(io::Error::from(io::ErrorKind::NotFound)) + } + } + impl FsReadHead for FailingWrite { + fn read_head(_: &Path, _: u64, _: &mut [u8]) -> io::Result { + // Empty content → no shebang, fall through to extension. + Ok(0) + } + } + impl FsCreateDirAll for FailingWrite { + fn create_dir_all(_: &Path) -> io::Result<()> { + Ok(()) + } + } + impl FsWrite for FailingWrite { + fn write(_: &Path, _: &[u8]) -> io::Result<()> { + Err(io::Error::from(io::ErrorKind::PermissionDenied)) + } + } + impl FsSetExecutable for FailingWrite { + fn set_executable(_: &Path) -> io::Result<()> { + unreachable!() + } + } + impl FsEnsureExecutableBits for FailingWrite { + fn ensure_executable_bits(_: &Path) -> io::Result<()> { + unreachable!() + } + } + impl FsWalkFiles for FailingWrite { + fn walk_files(_: &Path) -> io::Result> { + unreachable!("directories.bin not exercised by this test"); + #[expect(unreachable_code)] + Ok(empty()) + } + } + + let manifest = serde_json::json!({"name": "foo", "bin": "cli.js"}); + let tmp = tempdir().unwrap(); + let pkg = tmp.path().join("foo"); + create_dir_all(&pkg).unwrap(); + write_file(pkg.join("cli.js"), "").unwrap(); + let err = link_bins_of_packages::( + &[PackageBinSource { location: pkg, manifest: Arc::new(manifest) }], + &tmp.path().join(".bin"), + ) + .expect_err("write error must propagate"); + assert!(matches!(err, LinkBinsError::WriteShim { .. })); +} + +/// [`link_bins_of_packages`] propagates a chmod failure on the shim. +#[test] +fn link_bins_propagates_chmod_error_via_di() { + use std::io; + + struct FailingChmod; + impl FsReadDir for FailingChmod { + fn read_dir(_: &Path) -> io::Result> { + Ok(empty()) + } + } + impl FsReadFile for FailingChmod { + fn read_file(_: &Path) -> io::Result> { + unreachable!() + } + } + impl FsReadString for FailingChmod { + fn read_to_string(_: &Path) -> io::Result { + Err(io::Error::from(io::ErrorKind::NotFound)) + } + } + impl FsReadHead for FailingChmod { + fn read_head(_: &Path, _: u64, _: &mut [u8]) -> io::Result { + Ok(0) + } + } + impl FsCreateDirAll for FailingChmod { + fn create_dir_all(_: &Path) -> io::Result<()> { + Ok(()) + } + } + impl FsWrite for FailingChmod { + fn write(_: &Path, _: &[u8]) -> io::Result<()> { + Ok(()) + } + } + impl FsSetExecutable for FailingChmod { + fn set_executable(_: &Path) -> io::Result<()> { + Err(io::Error::from(io::ErrorKind::PermissionDenied)) + } + } + impl FsEnsureExecutableBits for FailingChmod { + fn ensure_executable_bits(_: &Path) -> io::Result<()> { + unreachable!() + } + } + impl FsWalkFiles for FailingChmod { + fn walk_files(_: &Path) -> io::Result> { + unreachable!("directories.bin not exercised by this test"); + #[expect(unreachable_code)] + Ok(empty()) + } + } + + let manifest = serde_json::json!({"name": "foo", "bin": "cli.js"}); + let tmp = tempdir().unwrap(); + let pkg = tmp.path().join("foo"); + create_dir_all(&pkg).unwrap(); + write_file(pkg.join("cli.js"), "").unwrap(); + let err = link_bins_of_packages::( + &[PackageBinSource { location: pkg, manifest: Arc::new(manifest) }], + &tmp.path().join(".bin"), + ) + .expect_err("chmod error must propagate"); + assert!(matches!(err, LinkBinsError::Chmod { .. })); +} + +/// [`super::write_shim`] propagates a non-`NotFound` IO error from +/// [`FsSetPermissions::ensure_executable_bits`] (chmod on the *target* +/// binary, not the shim). `NotFound` is swallowed by design, since the +/// target may have been removed concurrently. `PermissionDenied` +/// and friends must instead 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> { + Ok(empty()) + } + } + impl FsReadFile for FailingTargetChmod { + fn read_file(_: &Path) -> io::Result> { + unreachable!() + } + } + impl FsReadString for FailingTargetChmod { + fn read_to_string(_: &Path) -> io::Result { + Err(io::Error::from(io::ErrorKind::NotFound)) + } + } + impl FsReadHead for FailingTargetChmod { + fn read_head(_: &Path, _: u64, _: &mut [u8]) -> io::Result { + 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 FsSetExecutable for FailingTargetChmod { + fn set_executable(_: &Path) -> io::Result<()> { + Ok(()) + } + } + impl FsEnsureExecutableBits for FailingTargetChmod { + 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)) + } + } + impl FsWalkFiles for FailingTargetChmod { + fn walk_files(_: &Path) -> io::Result> { + unreachable!("directories.bin not exercised by this test"); + #[expect(unreachable_code)] + Ok(empty()) + } + } + + let manifest = serde_json::json!({"name": "foo", "bin": "cli.js"}); + let tmp = tempdir().unwrap(); + let pkg = tmp.path().join("foo"); + create_dir_all(&pkg).unwrap(); + write_file(pkg.join("cli.js"), "").unwrap(); + let err = link_bins_of_packages::( + &[PackageBinSource { location: pkg, manifest: Arc::new(manifest) }], + &tmp.path().join(".bin"), + ) + .expect_err("non-NotFound target chmod error must propagate as Chmod"); + assert!(matches!(err, LinkBinsError::Chmod { .. })); +} + +/// [`super::write_shim`] swallows `NotFound` from +/// [`FsSetPermissions::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> { + Ok(empty()) + } + } + impl FsReadFile for NotFoundTargetChmod { + fn read_file(_: &Path) -> io::Result> { + unreachable!() + } + } + impl FsReadString for NotFoundTargetChmod { + fn read_to_string(_: &Path) -> io::Result { + Err(io::Error::from(io::ErrorKind::NotFound)) + } + } + impl FsReadHead for NotFoundTargetChmod { + fn read_head(_: &Path, _: u64, _: &mut [u8]) -> io::Result { + 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 FsSetExecutable for NotFoundTargetChmod { + fn set_executable(_: &Path) -> io::Result<()> { + Ok(()) + } + } + impl FsEnsureExecutableBits for NotFoundTargetChmod { + fn ensure_executable_bits(_: &Path) -> io::Result<()> { + Err(io::Error::from(io::ErrorKind::NotFound)) + } + } + impl FsWalkFiles for NotFoundTargetChmod { + fn walk_files(_: &Path) -> io::Result> { + unreachable!("directories.bin not exercised by this test"); + #[expect(unreachable_code)] + Ok(empty()) + } + } + + let manifest = serde_json::json!({"name": "foo", "bin": "cli.js"}); + let tmp = tempdir().unwrap(); + let pkg = tmp.path().join("foo"); + create_dir_all(&pkg).unwrap(); + write_file(pkg.join("cli.js"), "").unwrap(); + link_bins_of_packages::( + &[PackageBinSource { location: pkg, manifest: Arc::new(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 [`LinkBinsError::ProbeShimSource`] +/// variant). Forced via a fake [`FsReadHead`] that fails with +/// permission-denied. The wider [`super::write_shim`] → +/// [`search_script_runtime`] chain remains unchanged. +#[test] +fn link_bins_propagates_probe_shim_source_error_via_di() { + use std::io; + + struct FailingProbe; + impl FsReadDir for FailingProbe { + fn read_dir(_: &Path) -> io::Result> { + Ok(empty()) + } + } + impl FsReadFile for FailingProbe { + fn read_file(_: &Path) -> io::Result> { + unreachable!() + } + } + impl FsReadString for FailingProbe { + fn read_to_string(_: &Path) -> io::Result { + unreachable!() + } + } + impl FsReadHead for FailingProbe { + fn read_head(_: &Path, _: u64, _: &mut [u8]) -> io::Result { + Err(io::Error::from(io::ErrorKind::PermissionDenied)) + } + } + impl FsCreateDirAll for FailingProbe { + fn create_dir_all(_: &Path) -> io::Result<()> { + Ok(()) + } + } + impl FsWrite for FailingProbe { + fn write(_: &Path, _: &[u8]) -> io::Result<()> { + unreachable!() + } + } + impl FsSetExecutable for FailingProbe { + fn set_executable(_: &Path) -> io::Result<()> { + unreachable!() + } + } + impl FsEnsureExecutableBits for FailingProbe { + fn ensure_executable_bits(_: &Path) -> io::Result<()> { + unreachable!() + } + } + impl FsWalkFiles for FailingProbe { + fn walk_files(_: &Path) -> io::Result> { + unreachable!("directories.bin not exercised by this test"); + #[expect(unreachable_code)] + Ok(empty()) + } + } + + let manifest = serde_json::json!({"name": "foo", "bin": "cli.js"}); + let tmp = tempdir().unwrap(); + let pkg = tmp.path().join("foo"); + create_dir_all(&pkg).unwrap(); + let err = link_bins_of_packages::( + &[PackageBinSource { location: pkg, manifest: Arc::new(manifest) }], + &tmp.path().join(".bin"), + ) + .expect_err("probe error must propagate"); + assert!(matches!(err, LinkBinsError::ProbeShimSource { .. })); +} + +/// [`link_bins`] propagates a non-`NotFound` IO error from reading a +/// child `package.json` (the [`LinkBinsError::ReadManifest`] variant). +/// Forced via a fake [`FsReadFile`] that always returns +/// `PermissionDenied`. +#[test] +fn link_bins_propagates_read_manifest_error_via_di() { + use std::io; + + struct DenyManifestRead; + impl FsReadDir for DenyManifestRead { + fn read_dir(_: &Path) -> io::Result> { + Ok(vec!["foo".into()].into_iter()) + } + } + impl FsReadFile for DenyManifestRead { + fn read_file(_: &Path) -> io::Result> { + Err(io::Error::from(io::ErrorKind::PermissionDenied)) + } + } + impl FsReadString for DenyManifestRead { + fn read_to_string(_: &Path) -> io::Result { + unreachable!() + } + } + impl FsReadHead for DenyManifestRead { + fn read_head(_: &Path, _: u64, _: &mut [u8]) -> io::Result { + unreachable!() + } + } + impl FsCreateDirAll for DenyManifestRead { + fn create_dir_all(_: &Path) -> io::Result<()> { + unreachable!() + } + } + impl FsWrite for DenyManifestRead { + fn write(_: &Path, _: &[u8]) -> io::Result<()> { + unreachable!() + } + } + impl FsSetExecutable for DenyManifestRead { + fn set_executable(_: &Path) -> io::Result<()> { + unreachable!() + } + } + impl FsEnsureExecutableBits for DenyManifestRead { + fn ensure_executable_bits(_: &Path) -> io::Result<()> { + unreachable!() + } + } + impl FsWalkFiles for DenyManifestRead { + fn walk_files(_: &Path) -> io::Result> { + unreachable!("directories.bin not exercised by this test"); + #[expect(unreachable_code)] + Ok(empty()) + } + } + + let err = link_bins::(Path::new("/x"), Path::new("/x/.bin")) + .expect_err("read_manifest error must propagate"); + assert!(matches!(err, LinkBinsError::ReadManifest { .. })); +} + +/// [`super::pick_winner`] `(true, false)` arm. Existing owns, candidate +/// doesn't, so existing wins. The other arm (`(false, true)`) is +/// covered by `ownership_breaks_bin_conflicts` further down. +/// +/// Uses `aaa-other` (lexically less than `npm`) as the non-owner so +/// the test fails when ownership is broken: with the rule disabled +/// the lexical fallback picks `aaa-other`, the assertion observes +/// `/aaa-other/npx` instead of `/npm/npx`. A package named `other` +/// would lexically lose to `npm` regardless, masking the regression. +#[test] +fn ownership_breaks_bin_conflicts_when_existing_owns() { + let tmp = tempdir().unwrap(); + let aaa_other = tmp.path().join("aaa-other"); + let npm = tmp.path().join("npm"); + for d in [&aaa_other, &npm] { + create_dir_all(d).unwrap(); + write_file(d.join("npx"), "#!/usr/bin/env node\n").unwrap(); + } + write_file(npm.join("package.json"), json!({"name": "npm", "bin": {"npx": "npx"}}).to_string()) + .unwrap(); + write_file( + aaa_other.join("package.json"), + json!({"name": "aaa-other", "bin": {"npx": "npx"}}).to_string(), + ) + .unwrap(); + + let manifest_other: Value = + serde_json::from_slice(&read_file(aaa_other.join("package.json")).unwrap()).unwrap(); + let manifest_npm: Value = + serde_json::from_slice(&read_file(npm.join("package.json")).unwrap()).unwrap(); + + // Order npm-first; this exercises the (true, false) arm because + // `npm` (existing) owns and `aaa-other` (candidate) doesn't. + let bins = tmp.path().join(".bin"); + link_bins_of_packages::( + &[ + PackageBinSource { location: npm.clone(), manifest: Arc::new(manifest_npm) }, + PackageBinSource { location: aaa_other.clone(), manifest: Arc::new(manifest_other) }, + ], + &bins, + ) + .unwrap(); + + let body = read_to_string(bins.join("npx")).unwrap(); + assert!(body.contains("/npm/npx"), "existing-owns winner must be `npm`, body:\n{body}"); +} + +/// [`link_bins`] propagates a non-`NotFound` `read_dir` error on +/// `` itself. Real fs can't trigger this portably; the +/// fake forces the variant. +#[test] +fn link_bins_propagates_modules_dir_read_error_via_di() { + use std::io; + + struct FailingModulesRead; + impl FsReadDir for FailingModulesRead { + fn read_dir(_: &Path) -> io::Result> { + Err::, _>(io::Error::from(io::ErrorKind::PermissionDenied)) + } + } + impl FsReadFile for FailingModulesRead { + fn read_file(_: &Path) -> io::Result> { + unreachable!() + } + } + impl FsReadString for FailingModulesRead { + fn read_to_string(_: &Path) -> io::Result { + unreachable!() + } + } + impl FsReadHead for FailingModulesRead { + fn read_head(_: &Path, _: u64, _: &mut [u8]) -> io::Result { + unreachable!() + } + } + impl FsCreateDirAll for FailingModulesRead { + fn create_dir_all(_: &Path) -> io::Result<()> { + unreachable!() + } + } + impl FsWrite for FailingModulesRead { + fn write(_: &Path, _: &[u8]) -> io::Result<()> { + unreachable!() + } + } + impl FsSetExecutable for FailingModulesRead { + fn set_executable(_: &Path) -> io::Result<()> { + unreachable!() + } + } + impl FsEnsureExecutableBits for FailingModulesRead { + fn ensure_executable_bits(_: &Path) -> io::Result<()> { + unreachable!() + } + } + impl FsWalkFiles for FailingModulesRead { + fn walk_files(_: &Path) -> io::Result> { + unreachable!("directories.bin not exercised by this test"); + #[expect(unreachable_code)] + Ok(empty()) + } + } + + let err = link_bins::(Path::new("/x"), Path::new("/x/.bin")) + .expect_err("read_dir error must propagate"); + eprintln!("link_bins_propagates_modules_dir_read_error err={err:?}"); + assert!(matches!(err, LinkBinsError::ReadModulesDir { .. })); +} + +/// Conflict resolution: when two packages declare the same bin name, the +/// owning package wins. +/// +/// Uses `aaa-other` (lexically less than `npm`) as the non-owner so the +/// test fails when ownership is broken: with the rule disabled the +/// lexical fallback picks `aaa-other`, the assertion observes +/// `/aaa-other/npx` instead of `/npm/npx`. A package named `other` +/// would lexically lose to `npm` regardless, masking the regression. +#[test] +fn ownership_breaks_bin_conflicts() { + let tmp = tempdir().unwrap(); + let npm = tmp.path().join("npm"); + let aaa_other = tmp.path().join("aaa-other"); + for d in [&npm, &aaa_other] { + create_dir_all(d).unwrap(); + write_file(d.join("npx"), "#!/usr/bin/env node\n").unwrap(); + } + write_file(npm.join("package.json"), json!({"name": "npm", "bin": {"npx": "npx"}}).to_string()) + .unwrap(); + write_file( + aaa_other.join("package.json"), + json!({"name": "aaa-other", "bin": {"npx": "npx"}}).to_string(), + ) + .unwrap(); + + let manifest_npm: Value = + serde_json::from_slice(&read_file(npm.join("package.json")).unwrap()).unwrap(); + let manifest_other: Value = + serde_json::from_slice(&read_file(aaa_other.join("package.json")).unwrap()).unwrap(); + + let bins = tmp.path().join(".bin"); + link_bins_of_packages::( + &[ + PackageBinSource { location: aaa_other.clone(), manifest: Arc::new(manifest_other) }, + PackageBinSource { location: npm.clone(), manifest: Arc::new(manifest_npm) }, + ], + &bins, + ) + .unwrap(); + + let body = read_to_string(bins.join("npx")).unwrap(); + // npm's `npx` lives at `/npx`; the shim must reference that path. + assert!( + body.contains("/npm/npx") || is_shim_pointing_at(&body, &npm.join("npx")), + "ownership-aware resolution should pick npm's npx, body:\n{body}", + ); +} diff --git a/pacquet/crates/cmd-shim/src/path_util.rs b/pacquet/crates/cmd-shim/src/path_util.rs new file mode 100644 index 0000000000..d374e4e048 --- /dev/null +++ b/pacquet/crates/cmd-shim/src/path_util.rs @@ -0,0 +1,50 @@ +//! Path-manipulation helpers shared across [`crate::bin_resolver`] and +//! [`crate::shim`]. Kept private to the crate. + +use std::path::{Component, Path, PathBuf}; + +/// Lexically resolve `.` and `..` in `path` without touching the filesystem. +/// +/// `.` (CurDir) components are dropped. `..` (ParentDir) components pop +/// the previous component when one exists. Otherwise the rule depends +/// on whether `out` is already anchored: +/// +/// - Anchored (`out` has a root or a Windows `Prefix`): drop the `..`. +/// Node's `path.resolve("/a/../../b")` returns `/b`, not `/../b`, so +/// the lexical normalisation has to match for `is_subdir` containment +/// checks (a `starts_with` against the package root) to agree with +/// pnpm. Without this branch, a path like `/x/../../bin.js` +/// would normalise to `/..//bin.js` and be rejected as outside +/// `` even when it resolves back inside. +/// - Unanchored: push `..` so a leading `..` survives. Relative +/// targets like `../shared/cli` need this for +/// `shim::relative_path_from`. +/// +/// Filesystem-free is the whole point: callers in `bin_resolver::is_subdir` +/// and `shim::relative_path_from` run the check before the target files +/// exist on disk, where `std::fs::canonicalize` cannot help. Mirrors pnpm's +/// `is-subdir`, which is also purely lexical (it uses Node's +/// `path.resolve`). +pub(crate) fn lexical_normalize(path: &Path) -> PathBuf { + let mut out = PathBuf::new(); + for component in path.components() { + match component { + Component::ParentDir => { + if !out.pop() && !is_anchored(&out) { + out.push(".."); + } + } + Component::CurDir => {} + other => out.push(other.as_os_str()), + } + } + out +} + +/// Whether `path` is anchored to a filesystem root: it either starts +/// with a Unix `/` root or a Windows `Prefix` (drive letter, UNC +/// share). Used by [`lexical_normalize`] to decide whether a `..` that +/// cannot pop should be materialised or dropped. +fn is_anchored(path: &Path) -> bool { + path.has_root() || matches!(path.components().next(), Some(Component::Prefix(_))) +} diff --git a/pacquet/crates/cmd-shim/src/shim.rs b/pacquet/crates/cmd-shim/src/shim.rs new file mode 100644 index 0000000000..84e9651cde --- /dev/null +++ b/pacquet/crates/cmd-shim/src/shim.rs @@ -0,0 +1,398 @@ +use crate::{capabilities::FsReadHead, path_util::lexical_normalize}; +use std::{ + io, + path::{Path, PathBuf}, +}; + +/// Detected runtime for a target script. +/// +/// Mirrors the return shape of `searchScriptRuntime` in +/// . +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ScriptRuntime { + /// The interpreter to invoke. `None` means "exec the file directly". + pub prog: Option, + /// Extra arguments declared after the interpreter in the shebang. Empty + /// when the runtime came from the extension fallback. + pub args: String, +} + +/// Map of file extensions to their default runtime when the script lacks a +/// shebang. Mirrors `extensionToProgramMap` in upstream cmd-shim. +fn extension_program(extension: &str) -> Option<&'static str> { + match extension { + "js" | "cjs" | "mjs" => Some("node"), + "cmd" | "bat" => Some("cmd"), + "ps1" => Some("pwsh"), + "sh" => Some("sh"), + _ => None, + } +} + +/// Read up to 512 bytes of `path` and infer the runtime. +/// +/// Order, mirroring `searchScriptRuntime`: +/// +/// 1. If the file exists and starts with a shebang, parse `prog` + `args` from +/// it. +/// 2. Otherwise look up a default runtime by file extension (e.g. `.js` → +/// `node`, `.cmd` → `cmd`). +/// 3. If neither yields a runtime, return `None`. [`generate_sh_shim`] +/// handles that by exec'ing the target directly. +/// +/// `NotFound` reading the file degrades to `Ok(None)` so a missing-bin race +/// doesn't fail the whole install. Other IO errors propagate, since pacquet +/// has already verified the bin path resolves under the package root by +/// this point and a real failure deserves to surface. +pub fn search_script_runtime(path: &Path) -> io::Result> { + let extension = path.extension().and_then(|s| s.to_str()).unwrap_or(""); + + let runtime_from_shebang = read_shebang::(path)?; + if let Some(rt) = runtime_from_shebang { + return Ok(Some(rt)); + } + + if let Some(prog) = extension_program(extension) { + return Ok(Some(ScriptRuntime { prog: Some(prog.to_string()), args: String::new() })); + } + + Ok(None) +} + +fn read_shebang(path: &Path) -> io::Result> { + let mut buffer = [0u8; 512]; + let read = match read_head_filled::(path, &mut buffer) { + Ok(read) => read, + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(None), + Err(error) => return Err(error), + }; + Ok(parse_shebang_from_bytes(&buffer[..read])) +} + +/// Read up to `buf.len()` bytes from `path` into `buf`, looping over +/// the [`FsReadHead`] capability until either the buffer is full or +/// the underlying read returns 0 (EOF). Returns the number of bytes +/// actually filled (which can be `< buf.len()` for a short file). +/// +/// [`FsReadHead::read_head`] mirrors a single `read(2)` syscall, which +/// POSIX permits to return short. This loop collects short reads so +/// the shebang parser sees a complete view of the head of the file +/// even on pseudo-fs paths (`/proc`, `/sys`, FUSE, …) where short +/// reads are common. On regular files at offset 0 the underlying +/// `read` returns the whole prefix in one syscall, so the loop adds +/// no extra syscalls in the hot path. The cost is one extra branch. +/// +/// Kept generic over [`FsReadHead`] so tests can plug in a fake that +/// deliberately returns short and verify the loop accumulates +/// correctly. +pub fn read_head_filled(path: &Path, buf: &mut [u8]) -> io::Result { + let mut total = 0; + while total < buf.len() { + match Api::read_head(path, total as u64, &mut buf[total..])? { + 0 => break, // EOF + n => total += n, + } + } + Ok(total) +} + +/// Parse the runtime out of the first line of a script's content. Pure +/// function over bytes so the caller can plug in any I/O strategy. +/// +/// Does **not** trim leading whitespace before looking for `#!`. The +/// kernel and upstream cmd-shim both treat `#!` as a shebang only when +/// it sits at byte 0 of the file; an earlier +/// `String::from_utf8_lossy(bytes).trim_start()` accepted inputs like +/// `" \n#!/usr/bin/env node"` as a valid shebang and could select the +/// wrong runtime for files that just happen to mention `#!` after some +/// whitespace. The first line is taken exactly as-is (`#!` is matched +/// at column 0 of that line via `strip_prefix` in `parse_shebang`). +pub fn parse_shebang_from_bytes(bytes: &[u8]) -> Option { + let head = String::from_utf8_lossy(bytes); + let first_line = head.split('\n').next().unwrap_or("").trim_end_matches('\r'); + parse_shebang(first_line) +} + +/// Mirrors the shebang regex in upstream cmd-shim: +/// `^#!\s*(?:/usr/bin/env(?:\s+-S\s*)?)?\s*([^ \t]+)(.*)$`. +/// +/// Recognises `#!/usr/bin/env `, `#!/usr/bin/env -S `, and any +/// direct `#!/path/to/` shebang. `args` is captured **including the +/// leading whitespace** that separates it from `prog`. That matches +/// upstream's regex group 2 (`(.*)`), which captures everything from after +/// `prog`'s end-of-match to end of line. Preserving the leading whitespace +/// is what produces the byte-identical shim text upstream emits (e.g. the +/// double space between `$basedir/sh` and `-e` in the rendered exec line). +fn parse_shebang(line: &str) -> Option { + let rest = line.strip_prefix("#!")?.trim_start(); + let (rest, _) = strip_env_prefix(rest); + let rest = rest.trim_start(); + + // Slice at the first space or tab; the args slice keeps the separator + // so the rendered shim matches upstream byte-for-byte. Using `splitn` + // would discard the separator and silently drop one space from the + // `exec` line. + let (prog, args) = match rest.find([' ', '\t']) { + Some(idx) => rest.split_at(idx), + None => (rest, ""), + }; + + if prog.is_empty() { + return None; + } + + Some(ScriptRuntime { prog: Some(prog.to_string()), args: args.to_string() }) +} + +/// Strip a leading `/usr/bin/env`, optionally followed by `-S`, from the +/// shebang body. Returns the remainder and whether `env` was present. +fn strip_env_prefix(input: &str) -> (&str, bool) { + let Some(rest) = input.strip_prefix("/usr/bin/env") else { + return (input, false); + }; + let trimmed = rest.trim_start(); + if let Some(after_dash_s) = trimmed.strip_prefix("-S") { + return (after_dash_s, true); + } + (trimmed, true) +} + +/// Generate the Unix shell-shim contents for `target_path`, written to +/// `shim_path`. Mirrors `generateShShim` in upstream cmd-shim. +/// +/// The shim is a pure `/bin/sh` script that: +/// +/// 1. Resolves `basedir` to its own directory (with a `cygpath` fixup for +/// MSYS-style POSIX shells on Windows). +/// 2. If the runtime program is colocated at `$basedir/` (a rare case, +/// only true when the runtime was bundled alongside the shim), prefer that +/// binary; otherwise fall through to the system PATH. +/// 3. Forwards `"$@"` to the resolved interpreter, with the target script as +/// the first positional argument. +/// +/// When [`search_script_runtime`] returned `None` (no shebang, unknown +/// extension), the shim execs the target directly via the second branch +/// upstream uses for that case. +pub fn generate_sh_shim( + target_path: &Path, + shim_path: &Path, + runtime: Option<&ScriptRuntime>, +) -> String { + let mut sh = String::from(SH_SHIM_HEADER); + + let sh_target = relative_target(target_path, shim_path); + let quoted_target = if Path::new(&sh_target).is_absolute() { + format!("\"{sh_target}\"") + } else { + format!("\"$basedir/{sh_target}\"") + }; + + match runtime { + Some(ScriptRuntime { prog: Some(prog), args }) => { + // `sh_long_prog` is the `"$basedir/"` form upstream uses. + // It always carries the leading `$basedir/` and quotes; never + // just the program name on its own. + let sh_long_prog = format!("\"$basedir/{prog}\""); + sh.push_str(&format!( + "if [ -x {sh_long_prog} ]; then\n exec {sh_long_prog} {args} {quoted_target} \"$@\"\nelse\n exec {prog} {args} {quoted_target} \"$@\"\nfi\n", + )); + } + // No runtime detected, so exec the target directly. Upstream still + // emits `exit $?` on this branch for parity with non-execve POSIX + // shells. + runtime_opt => { + let args = runtime_opt.map(|r| r.args.as_str()).unwrap_or(""); + sh.push_str(&format!("{quoted_target} {args} \"$@\"\nexit $?\n")); + } + } + + sh.push_str(&format!("# {}\n", shim_target_marker(target_path))); + sh +} + +/// Generate the Windows `.cmd` shim contents for `target_path`. Mirrors +/// `generateCmdShim` in upstream cmd-shim. Pacquet skips the +/// `nodePath`/`prependToPath`/`nodeExecPath`/`progArgs` features that +/// upstream supports; we only ever write a "plain" cmd shim. +/// +/// CRLF line endings are part of the on-disk contract for `.cmd` files +/// on Windows, so the template uses literal `\r\n`. +pub fn generate_cmd_shim( + target_path: &Path, + shim_path: &Path, + runtime: Option<&ScriptRuntime>, +) -> String { + let cmd_target_rel = relative_target_windows(target_path, shim_path); + let quoted_target = if Path::new(&cmd_target_rel).is_absolute() { + format!("\"{cmd_target_rel}\"") + } else { + format!("\"%~dp0\\{cmd_target_rel}\"") + }; + + let mut cmd = String::from("@SETLOCAL\r\n"); + + match runtime { + Some(ScriptRuntime { prog: Some(prog), args }) => { + let long_prog = format!("\"%~dp0\\{prog}.exe\""); + cmd.push_str(&format!( + "@IF EXIST {long_prog} (\r\n {long_prog} {args} {quoted_target} %*\r\n) ELSE (\r\n @SET PATHEXT=%PATHEXT:;.JS;=;%\r\n {prog} {args} {quoted_target} %*\r\n)\r\n", + )); + } + runtime_opt => { + let args = runtime_opt.map(|r| r.args.as_str()).unwrap_or(""); + // No runtime detected, so exec the target directly. + cmd.push_str(&format!("@{quoted_target} {args} %*\r\n")); + } + } + + cmd +} + +/// Generate the cross-shell PowerShell `.ps1` shim contents for +/// `target_path`. Mirrors `generatePwshShim` in upstream cmd-shim, +/// minus the `nodePath`/`prependToPath`/`nodeExecPath`/`progArgs` +/// branches we don't use. The shim self-detects Windows vs. POSIX-ish +/// pwsh and adjusts the executable suffix accordingly. +pub fn generate_pwsh_shim( + target_path: &Path, + shim_path: &Path, + runtime: Option<&ScriptRuntime>, +) -> String { + let sh_target = relative_target(target_path, shim_path); + let quoted_target = if Path::new(&sh_target).is_absolute() { + format!("\"{sh_target}\"") + } else { + format!("\"$basedir/{sh_target}\"") + }; + + use std::fmt::Write; + let mut pwsh = String::from(PWSH_SHIM_HEADER); + + match runtime { + Some(ScriptRuntime { prog: Some(prog), args }) => { + let long_prog = format!(r#""$basedir/{prog}$exe""#); + let prog_quoted = format!(r#""{prog}$exe""#); + writeln!(pwsh).unwrap(); + writeln!(pwsh, "$ret=0").unwrap(); + writeln!(pwsh, "if (Test-Path {long_prog}) {{").unwrap(); + writeln!(pwsh, " # Support pipeline input").unwrap(); + writeln!(pwsh, " if ($MyInvocation.ExpectingInput) {{").unwrap(); + writeln!(pwsh, " $input | & {long_prog} {args} {quoted_target} $args").unwrap(); + writeln!(pwsh, " }} else {{").unwrap(); + writeln!(pwsh, " & {long_prog} {args} {quoted_target} $args").unwrap(); + writeln!(pwsh, " }}").unwrap(); + writeln!(pwsh, " $ret=$LASTEXITCODE").unwrap(); + writeln!(pwsh, "}} else {{").unwrap(); + writeln!(pwsh, " # Support pipeline input").unwrap(); + writeln!(pwsh, " if ($MyInvocation.ExpectingInput) {{").unwrap(); + writeln!(pwsh, " $input | & {prog_quoted} {args} {quoted_target} $args").unwrap(); + writeln!(pwsh, " }} else {{").unwrap(); + writeln!(pwsh, " & {prog_quoted} {args} {quoted_target} $args").unwrap(); + writeln!(pwsh, " }}").unwrap(); + writeln!(pwsh, " $ret=$LASTEXITCODE").unwrap(); + writeln!(pwsh, "}}").unwrap(); + writeln!(pwsh, "exit $ret").unwrap(); + } + runtime_opt => { + let args = runtime_opt.map(|r| r.args.as_str()).unwrap_or(""); + writeln!(pwsh).unwrap(); + writeln!(pwsh, "# Support pipeline input").unwrap(); + writeln!(pwsh, "if ($MyInvocation.ExpectingInput) {{").unwrap(); + writeln!(pwsh, " $input | & {quoted_target} {args} $args").unwrap(); + writeln!(pwsh, "}} else {{").unwrap(); + writeln!(pwsh, " & {quoted_target} {args} $args").unwrap(); + writeln!(pwsh, "}}").unwrap(); + writeln!(pwsh, "exit $LASTEXITCODE").unwrap(); + } + } + + pwsh +} + +/// `.ps1` template prelude. Sets up `$basedir` and `$exe` exactly like +/// upstream's `generatePwshShim`. +const PWSH_SHIM_HEADER: &str = r#"#!/usr/bin/env pwsh +$basedir=Split-Path $MyInvocation.MyCommand.Definition -Parent + +$exe="" +if ($PSVersionTable.PSVersion -lt "6.0" -or $IsWindows) { + # Fix case when both the Windows and Linux builds of Node + # are installed in the same directory + $exe=".exe" +}"#; + +/// Compute the Windows-style relative path from `shim_path`'s parent +/// directory to `target_path`. The `.cmd` shim uses backslashes, so we +/// convert the lexical-relative result. Falls back to the absolute path +/// if the relative computation fails. Same shape as +/// [`relative_target`] but with the slash direction flipped. +fn relative_target_windows(target_path: &Path, shim_path: &Path) -> String { + let shim_dir = shim_path.parent().unwrap_or_else(|| Path::new("")); + let rel = relative_path_from(shim_dir, target_path); + rel.to_string_lossy().replace('/', "\\") +} + +const SH_SHIM_HEADER: &str = r#"#!/bin/sh +basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')") + +case `uname` in + *CYGWIN*|*MINGW*|*MSYS*) + if command -v cygpath > /dev/null 2>&1; then + basedir=`cygpath -w "$basedir"` + fi + ;; +esac + +"#; + +/// Trailing `# cmd-shim-target=` marker. Upstream uses it to detect +/// whether an existing shim already targets the same source without +/// re-parsing its body. Pacquet uses [`is_shim_pointing_at`] for the same +/// short-circuit on warm reinstalls. +fn shim_target_marker(target_path: &Path) -> String { + format!("cmd-shim-target={}", target_path.to_string_lossy().replace('\\', "/")) +} + +/// Whether an already-on-disk shim targets `target_path`. Mirrors +/// `isShimPointingAt`. The check looks for the trailing marker line so the +/// header text never has to be byte-identical between cmd-shim versions. +pub fn is_shim_pointing_at(shim_content: &str, target_path: &Path) -> bool { + let marker = format!("# {}", shim_target_marker(target_path)); + shim_content.lines().any(|line| line == marker) +} + +/// Compute the relative path from `shim_path`'s parent directory to +/// `target_path`. Falls back to the absolute target path if the relative +/// computation fails. That matches the `path.isAbsolute(shTarget)` guard in +/// upstream's `generateShShim`. +fn relative_target(target_path: &Path, shim_path: &Path) -> String { + let shim_dir = shim_path.parent().unwrap_or_else(|| Path::new("")); + let rel = relative_path_from(shim_dir, target_path); + rel.to_string_lossy().replace('\\', "/") +} + +fn relative_path_from(from: &Path, to: &Path) -> PathBuf { + let from = lexical_normalize(from); + let to = lexical_normalize(to); + + let from_components: Vec<_> = from.components().collect(); + let to_components: Vec<_> = to.components().collect(); + + let common = + from_components.iter().zip(to_components.iter()).take_while(|(a, b)| a == b).count(); + + let mut result = PathBuf::new(); + for _ in &from_components[common..] { + result.push(".."); + } + for component in &to_components[common..] { + result.push(component.as_os_str()); + } + if result.as_os_str().is_empty() { + result.push("."); + } + result +} + +#[cfg(test)] +mod tests; diff --git a/pacquet/crates/cmd-shim/src/shim/tests.rs b/pacquet/crates/cmd-shim/src/shim/tests.rs new file mode 100644 index 0000000000..e331dd8d93 --- /dev/null +++ b/pacquet/crates/cmd-shim/src/shim/tests.rs @@ -0,0 +1,547 @@ +use super::{ + ScriptRuntime, extension_program, generate_cmd_shim, generate_pwsh_shim, generate_sh_shim, + is_shim_pointing_at, parse_shebang, parse_shebang_from_bytes, read_head_filled, + relative_target, search_script_runtime, +}; +use crate::capabilities::{FsReadHead, RealApi}; +use crate::path_util::lexical_normalize; +use std::{ + io, + path::{Path, PathBuf}, +}; + +#[test] +fn parses_env_node_shebang() { + let rt = parse_shebang("#!/usr/bin/env node").unwrap(); + assert_eq!(rt.prog.as_deref(), Some("node")); + assert_eq!(rt.args, ""); +} + +#[test] +fn parses_env_dash_s_shebang() { + let rt = parse_shebang("#!/usr/bin/env -S node --experimental").unwrap(); + assert_eq!(rt.prog.as_deref(), Some("node")); + // Leading space is preserved because upstream's regex group 2 + // captures the separator. Pinning `" --experimental"` (with the + // space) is what makes the rendered shim's `exec` line match + // upstream byte-for-byte. + assert_eq!(rt.args, " --experimental"); +} + +#[test] +fn parses_direct_shebang() { + let rt = parse_shebang("#!/bin/sh -e").unwrap(); + assert_eq!(rt.prog.as_deref(), Some("/bin/sh")); + // Leading space preserved (see `parses_env_dash_s_shebang`). + assert_eq!(rt.args, " -e"); +} + +#[test] +fn rejects_non_shebang_lines() { + assert!(parse_shebang("just text").is_none()); + assert!(parse_shebang("#! ").is_none()); +} + +#[test] +fn extension_fallback_picks_node_for_js() { + assert_eq!(extension_program("js"), Some("node")); + assert_eq!(extension_program("cjs"), Some("node")); + assert_eq!(extension_program("mjs"), Some("node")); +} + +#[test] +fn relative_target_traverses_into_sibling_package() { + // shim at .../node_modules/.bin/cli; target at .../node_modules/foo/bin/cli.js + let target = Path::new("/proj/node_modules/foo/bin/cli.js"); + let shim = Path::new("/proj/node_modules/.bin/cli"); + assert_eq!(relative_target(target, shim), "../foo/bin/cli.js"); +} + +/// Shim body for the typical `#!/usr/bin/env node` case must match the +/// exec template upstream produces verbatim, including the double space +/// between `$basedir/node` and the quoted target path (upstream's +/// `${args}` interpolates to empty between two literal spaces). +#[test] +fn generate_sh_shim_matches_pnpm_typical_case() { + let target = Path::new("/proj/node_modules/typescript/bin/tsc"); + let shim = Path::new("/proj/node_modules/.bin/tsc"); + let runtime = ScriptRuntime { prog: Some("node".into()), args: String::new() }; + let body = generate_sh_shim(target, shim, Some(&runtime)); + + assert!(body.starts_with("#!/bin/sh\n"), "shebang must come first"); + assert!( + body.contains("if [ -x \"$basedir/node\" ]; then\n exec \"$basedir/node\" \"$basedir/../typescript/bin/tsc\" \"$@\"\nelse\n exec node \"$basedir/../typescript/bin/tsc\" \"$@\"\nfi\n"), + "exec block must match pnpm's generateShShim template, body was:\n{body}", + ); + assert!( + body.ends_with("# cmd-shim-target=/proj/node_modules/typescript/bin/tsc\n"), + "trailing target marker is required for is_shim_pointing_at parity", + ); +} + +#[test] +fn is_shim_pointing_at_round_trips_through_marker() { + let target = Path::new("/p/node_modules/typescript/bin/tsc"); + let shim = Path::new("/p/node_modules/.bin/tsc"); + let runtime = ScriptRuntime { prog: Some("node".into()), args: String::new() }; + let body = generate_sh_shim(target, shim, Some(&runtime)); + assert!(is_shim_pointing_at(&body, target)); + assert!(!is_shim_pointing_at(&body, Path::new("/elsewhere"))); +} + +/// Every fallback extension upstream's `extensionToProgramMap` recognises +/// must round-trip. Guards against silent regression if an extension is +/// removed from the table. +#[test] +fn extension_program_covers_every_known_extension() { + assert_eq!(extension_program("js"), Some("node")); + assert_eq!(extension_program("cjs"), Some("node")); + assert_eq!(extension_program("mjs"), Some("node")); + assert_eq!(extension_program("cmd"), Some("cmd")); + assert_eq!(extension_program("bat"), Some("cmd")); + assert_eq!(extension_program("ps1"), Some("pwsh")); + assert_eq!(extension_program("sh"), Some("sh")); + assert_eq!(extension_program("unknown"), None); + assert_eq!(extension_program(""), None); +} + +/// [`super::parse_shebang`] returns None when the line lacks `#!` entirely +/// (handled elsewhere) or when it is `#!` with only whitespace after. The +/// `prog.is_empty()` guard catches the latter. +#[test] +fn parse_shebang_returns_none_for_empty_prog() { + assert!(parse_shebang("#!\t").is_none()); + assert!(parse_shebang("#!").is_none(), "empty line after #! must yield None"); + assert!(parse_shebang("not a shebang").is_none()); +} + +/// [`parse_shebang_from_bytes`] is the byte-level entry. It must trim a +/// leading BOM-free CRLF first line and survive non-UTF-8 bytes (lossy +/// decoding). +#[test] +fn parse_shebang_from_bytes_handles_crlf_and_lossy_utf8() { + let bytes = b"#!/usr/bin/env node\r\nconsole.log('hi')\n"; + let rt = parse_shebang_from_bytes(bytes).expect("CRLF first line"); + assert_eq!(rt.prog.as_deref(), Some("node")); + + // Non-UTF-8 bytes after the shebang must not break parsing. + let mut bytes = Vec::from(*b"#!/usr/bin/env node\n"); + bytes.extend_from_slice(&[0xff, 0xfe, 0xfd]); + let rt = parse_shebang_from_bytes(&bytes).expect("non-UTF-8 tail tolerated"); + assert_eq!(rt.prog.as_deref(), Some("node")); +} + +/// [`generate_sh_shim`] with `runtime: None` emits the `exit $?` arm that +/// upstream uses when no interpreter could be inferred. The shim execs the +/// target directly. +#[test] +fn generate_sh_shim_emits_direct_exec_when_no_runtime() { + let target = Path::new("/proj/node_modules/foo/bin/cli"); + let shim = Path::new("/proj/node_modules/.bin/cli"); + let body = generate_sh_shim(target, shim, None); + assert!( + body.contains("\"$basedir/../foo/bin/cli\" \"$@\"\nexit $?\n"), + "no-runtime arm must exec the target directly, body:\n{body}", + ); + assert!(body.ends_with("# cmd-shim-target=/proj/node_modules/foo/bin/cli\n")); +} + +/// [`generate_sh_shim`] with `runtime: Some(.. prog: None ..)` uses the same +/// no-runtime arm, but threading the explicit `args`. Mirrors upstream's +/// fallback when `prog` couldn't be inferred but the runtime probe still +/// returned a [`ScriptRuntime`] with `prog: None`. +#[test] +fn generate_sh_shim_threads_args_when_prog_is_none() { + let target = Path::new("/p/cli"); + let shim = Path::new("/p/.bin/cli"); + let runtime = ScriptRuntime { prog: None, args: "--flag".to_string() }; + let body = generate_sh_shim(target, shim, Some(&runtime)); + assert!( + body.contains("\"$basedir/../cli\" --flag \"$@\"\nexit $?\n"), + "args must be threaded into the no-prog arm, body:\n{body}", + ); +} + +/// [`generate_sh_shim`] with a target that lexically resolves to an absolute +/// path takes the `path::isAbsolute(shTarget)` branch upstream uses. The +/// quoted target stays absolute and skips the `$basedir/` prefix. +/// +/// Unix-only: a path like `/abs/elsewhere/cli` is "absolute" only on Unix. +/// On Windows, `Path::is_absolute()` requires a drive letter (e.g. +/// `C:\abs\...`), so the same input takes the relative branch. The shim +/// produced by pacquet is a `/bin/sh` script regardless of host platform, +/// but the absolute-vs-relative classification of bin paths is itself +/// platform-dependent. This test pins behavior on Unix only. +#[cfg(unix)] +#[test] +fn generate_sh_shim_uses_absolute_target_when_no_common_prefix() { + // `relative_path_from` of two paths with no common root produces an + // absolute-ish path that still starts with `/` once joined; force the + // absolute branch by constructing a target that's absolute and a shim + // whose parent is empty. + let target = Path::new("/abs/elsewhere/cli"); + let shim = Path::new("local-shim"); + let runtime = ScriptRuntime { prog: Some("node".into()), args: String::new() }; + let body = generate_sh_shim(target, shim, Some(&runtime)); + assert!( + body.contains("\"/abs/elsewhere/cli\""), + "absolute-target branch must skip $basedir prefix, body:\n{body}", + ); +} + +/// [`super::relative_target`] of `from == to_parent` collapses to `.` (the +/// `result.is_empty()` branch in [`super::relative_path_from`]). +#[test] +fn relative_target_collapses_to_dot_when_paths_share_dir() { + let target = Path::new("/proj/.bin/cli"); + let shim = Path::new("/proj/.bin/wrapper"); + assert_eq!(relative_target(target, shim), "cli"); +} + +/// [`super::relative_path_from`] preserves a single leading `..` in the +/// target (the `out.push("..")` fallback fires when `out.pop()` returns +/// false on an empty buffer). Multiple consecutive leading `..`s aren't +/// tested because [`super::lexical_normalize`] collapses them. `PathBuf::pop` +/// does not treat a trailing `..` component as a parent reference, so the +/// second `..` pops the first. That edge case doesn't occur in pacquet's +/// production paths (which are always absolute under `` or +/// ``), so we test only the single-`..` case where +/// the result is unambiguous. +/// +/// Asserting the exact value catches a regression that returns the raw +/// target unchanged (`../shared/cli`). A weaker substring assertion +/// would pass for both correct and broken outputs. +#[test] +fn lexical_normalize_keeps_leading_parent_segments() { + let target = Path::new("../shared/cli"); + let shim = Path::new("project/.bin/cli"); + let result = relative_target(target, shim); + assert_eq!(result, "../../../shared/cli", "leading `..` must propagate"); +} + +/// [`lexical_normalize`] drops `.` (CurDir) components. This is a direct +/// test on the helper itself. The indirect test below pins the same +/// behavior at the `relative_target` level, but a direct assertion makes +/// the CurDir arm visible to coverage tooling that can't see through +/// inlined call chains. +#[test] +fn lexical_normalize_drops_curdir_segments_directly() { + assert_eq!(lexical_normalize(Path::new("a/./b")), PathBuf::from("a/b")); + assert_eq!(lexical_normalize(Path::new("./a/b")), PathBuf::from("a/b")); + assert_eq!(lexical_normalize(Path::new("a/b/.")), PathBuf::from("a/b")); + assert_eq!(lexical_normalize(Path::new("./.")), PathBuf::new()); +} + +/// [`lexical_normalize`] discards `.` (CurDir) components silently. +/// Verify via [`relative_target`]. A target with embedded `./` +/// resolves the same as without. +#[test] +fn lexical_normalize_drops_curdir_components() { + let with_dot = relative_target(Path::new("/p/foo/./cli"), Path::new("/p/.bin/x")); + let without_dot = relative_target(Path::new("/p/foo/cli"), Path::new("/p/.bin/x")); + assert_eq!(with_dot, without_dot); +} + +/// [`search_script_runtime`] reads a real file with a shebang and returns +/// the prog from it. End-to-end of the production path. +#[test] +fn search_script_runtime_reads_shebang_from_real_file() { + use tempfile::tempdir; + let tmp = tempdir().unwrap(); + let path = tmp.path().join("script"); + std::fs::write(&path, "#!/usr/bin/env node\nbody\n").unwrap(); + let rt = search_script_runtime::(&path).unwrap().expect("runtime detected"); + assert_eq!(rt.prog.as_deref(), Some("node")); +} + +/// [`search_script_runtime`] on a missing file must degrade to `Ok(None)`. +/// Otherwise the install races against bin file extraction. +#[test] +fn search_script_runtime_returns_none_for_missing_file() { + let nonexistent = Path::new("/definitely/not/a/real/path/cli"); + assert_eq!(search_script_runtime::(nonexistent).unwrap(), None); +} + +/// [`search_script_runtime`] falls through to extension lookup when the +/// file has no shebang. A `.js` file without `#!` must still resolve to +/// `node`. +#[test] +fn search_script_runtime_falls_back_to_extension() { + use tempfile::tempdir; + let tmp = tempdir().unwrap(); + let path = tmp.path().join("script.js"); + std::fs::write(&path, "console.log('no shebang')\n").unwrap(); + let rt = search_script_runtime::(&path).unwrap().expect("extension fallback"); + assert_eq!(rt.prog.as_deref(), Some("node")); +} + +/// [`search_script_runtime`] returns `Ok(None)` when neither shebang nor +/// extension yields a runtime. Pure no-runtime path. +#[test] +fn search_script_runtime_returns_none_when_runtime_unknown() { + use tempfile::tempdir; + let tmp = tempdir().unwrap(); + let path = tmp.path().join("script.unknown_ext"); + std::fs::write(&path, "no shebang here\n").unwrap(); + assert_eq!(search_script_runtime::(&path).unwrap(), None); +} + +/// [`search_script_runtime`] propagates IO errors that aren't `NotFound`. +/// Real-fs can't trigger e.g. `PermissionDenied` portably, so plug a +/// fake [`FsReadHead`] per the DI principles in +/// . +#[test] +fn search_script_runtime_propagates_non_not_found_io_errors() { + struct PermissionDeniedApi; + impl FsReadHead for PermissionDeniedApi { + fn read_head(_: &Path, _: u64, _: &mut [u8]) -> io::Result { + Err(io::Error::from(io::ErrorKind::PermissionDenied)) + } + } + let err = search_script_runtime::(Path::new("any")) + .expect_err("non-NotFound IO error must propagate"); + assert_eq!(err.kind(), io::ErrorKind::PermissionDenied); +} + +/// A [`FsReadHead`] that returns 0 bytes (empty file) yields no shebang. +/// The parse step then falls through to the extension fallback. Pin the +/// behavior so a future tweak to the empty-buffer handling stays +/// compatible with the no-shebang case. +#[test] +fn search_script_runtime_reads_zero_bytes_then_falls_through() { + struct EmptyReadApi; + impl FsReadHead for EmptyReadApi { + fn read_head(_: &Path, _: u64, _: &mut [u8]) -> io::Result { + Ok(0) + } + } + // `.js` extension still resolves to `node` even with empty content. + let rt = + search_script_runtime::(Path::new("/x.js")).unwrap().expect("ext fallback"); + assert_eq!(rt.prog.as_deref(), Some("node")); + + // No extension and no shebang → Ok(None). + let rt = search_script_runtime::(Path::new("/x")).unwrap(); + assert_eq!(rt, None); +} + +/// [`RealApi::read_head`](RealApi) is the production capability. Tests +/// that exercise it indirectly cover most paths; this one pins the +/// contract directly. +#[test] +fn real_fs_read_head_reads_up_to_buffer_size() { + use tempfile::tempdir; + let tmp = tempdir().unwrap(); + let path = tmp.path().join("data"); + std::fs::write(&path, "hello world").unwrap(); + let mut buf = [0u8; 1024]; + let read = RealApi::read_head(&path, 0, &mut buf).unwrap(); + assert_eq!(read, 11); + assert_eq!(&buf[..read], b"hello world"); +} + +/// [`RealApi::read_head`](RealApi) propagates `NotFound` so the shebang reader can +/// distinguish a missing file from a real IO error and degrade to +/// `Ok(None)`. +#[test] +fn real_fs_read_head_propagates_not_found() { + let mut buf = [0u8; 16]; + let err = RealApi::read_head(Path::new("/no/such/file"), 0, &mut buf).unwrap_err(); + assert_eq!(err.kind(), io::ErrorKind::NotFound); +} + +/// [`read_head_filled`] against the real filesystem fills the buffer in +/// one underlying syscall (the common case) and returns the exact byte +/// count. +#[test] +fn read_head_filled_real_fs_long_file_fills_buffer() { + use tempfile::tempdir; + let tmp = tempdir().unwrap(); + let path = tmp.path().join("long"); + let payload: Vec = (0..1024).map(|i| (i % 251) as u8).collect(); + std::fs::write(&path, &payload).unwrap(); + + let mut buf = [0u8; 256]; + let read = read_head_filled::(&path, &mut buf).unwrap(); + assert_eq!(read, 256); + assert_eq!(&buf[..], &payload[..256]); +} + +/// [`read_head_filled`] against a file shorter than the buffer returns +/// the partial count (EOF terminates the loop without erroring). +#[test] +fn read_head_filled_real_fs_short_file_returns_partial() { + use tempfile::tempdir; + let tmp = tempdir().unwrap(); + let path = tmp.path().join("short"); + std::fs::write(&path, "#!/bin/sh\n").unwrap(); + + let mut buf = [0u8; 256]; + let read = read_head_filled::(&path, &mut buf).unwrap(); + assert_eq!(read, 10); + assert_eq!(&buf[..read], b"#!/bin/sh\n"); +} + +/// [`read_head_filled`] accumulates short reads from the underlying +/// capability. That is the very behaviour the loop exists to provide. +/// A fake that always returns short proves the loop calls the trait +/// repeatedly with advancing offsets until the buffer is full. +/// +/// Pinning this with a fake is the only way to verify the loop +/// without a pseudo-fs to test against: real filesystems essentially +/// never return short reads at offset 0. +#[test] +fn read_head_filled_accumulates_short_reads_from_fake() { + use std::sync::atomic::{AtomicUsize, Ordering}; + + /// Tracks the offsets each call sees, plus how many bytes the + /// fake produces per call. We deliver the input slice to the + /// caller `chunk_size` bytes at a time so the loop must run + /// multiple iterations to fill its buffer. + static CALL_COUNT: AtomicUsize = AtomicUsize::new(0); + static LAST_OFFSETS: [AtomicUsize; 4] = [ + AtomicUsize::new(usize::MAX), + AtomicUsize::new(usize::MAX), + AtomicUsize::new(usize::MAX), + AtomicUsize::new(usize::MAX), + ]; + const PAYLOAD: &[u8] = b"abcdefghij"; // 10 bytes + const CHUNK_SIZE: usize = 3; + + struct ShortReader; + impl FsReadHead for ShortReader { + fn read_head(_: &Path, offset: u64, buf: &mut [u8]) -> io::Result { + let i = CALL_COUNT.fetch_add(1, Ordering::Relaxed); + if i < LAST_OFFSETS.len() { + LAST_OFFSETS[i].store(offset as usize, Ordering::Relaxed); + } + let off = offset as usize; + if off >= PAYLOAD.len() { + return Ok(0); // EOF + } + let remaining = &PAYLOAD[off..]; + let take = remaining.len().min(buf.len()).min(CHUNK_SIZE); + buf[..take].copy_from_slice(&remaining[..take]); + Ok(take) + } + } + + let mut buf = [0u8; 8]; + let read = read_head_filled::(Path::new("any"), &mut buf).unwrap(); + assert_eq!(read, 8, "loop must accumulate short reads to fill the buffer"); + assert_eq!(&buf[..], b"abcdefgh"); + + // The loop made three calls (3 + 3 + 2 bytes) at offsets 0, 3, 6. + assert_eq!(CALL_COUNT.load(Ordering::Relaxed), 3); + assert_eq!(LAST_OFFSETS[0].load(Ordering::Relaxed), 0); + assert_eq!(LAST_OFFSETS[1].load(Ordering::Relaxed), 3); + assert_eq!(LAST_OFFSETS[2].load(Ordering::Relaxed), 6); +} + +/// [`read_head_filled`] terminates when the underlying capability +/// returns 0 (EOF), exercised here with a file shorter than the buffer. +#[test] +fn read_head_filled_terminates_on_zero_byte_read_from_fake() { + struct EofAfterOne; + impl FsReadHead for EofAfterOne { + fn read_head(_: &Path, offset: u64, buf: &mut [u8]) -> io::Result { + if offset == 0 && !buf.is_empty() { + buf[0] = b'X'; + Ok(1) + } else { + Ok(0) // EOF on subsequent calls + } + } + } + + let mut buf = [0u8; 16]; + let read = read_head_filled::(Path::new("any"), &mut buf).unwrap(); + assert_eq!(read, 1, "loop must stop on EOF, returning the partial count"); + assert_eq!(buf[0], b'X'); +} + +/// [`read_head_filled`] propagates non-EOF errors from the first call +/// without retrying. +#[test] +fn read_head_filled_propagates_io_error_from_fake() { + struct AlwaysErrors; + impl FsReadHead for AlwaysErrors { + fn read_head(_: &Path, _: u64, _: &mut [u8]) -> io::Result { + Err(io::Error::from(io::ErrorKind::PermissionDenied)) + } + } + + let mut buf = [0u8; 16]; + let err = read_head_filled::(Path::new("any"), &mut buf).unwrap_err(); + assert_eq!(err.kind(), io::ErrorKind::PermissionDenied); +} + +/// [`generate_cmd_shim`] produces a Windows `.cmd` shim with CRLF line +/// endings, `%~dp0\` for the target, and the +/// `@IF EXIST … (… ) ELSE ( @SET PATHEXT=... … )` exec block matching +/// upstream's template. +#[test] +fn generate_cmd_shim_matches_pnpm_template() { + let target = Path::new("/proj/node_modules/typescript/bin/tsc"); + let shim = Path::new("/proj/node_modules/.bin/tsc.cmd"); + let runtime = ScriptRuntime { prog: Some("node".into()), args: String::new() }; + let body = generate_cmd_shim(target, shim, Some(&runtime)); + + assert!(body.starts_with("@SETLOCAL\r\n"), "must start with @SETLOCAL CRLF"); + assert!( + body.contains("@IF EXIST \"%~dp0\\node.exe\" (\r\n \"%~dp0\\node.exe\" \"%~dp0\\..\\typescript\\bin\\tsc\" %*\r\n) ELSE (\r\n @SET PATHEXT=%PATHEXT:;.JS;=;%\r\n node \"%~dp0\\..\\typescript\\bin\\tsc\" %*\r\n)\r\n"), + "exec block must match pnpm's generateCmdShim template, body was:\n{body}", + ); +} + +/// [`generate_cmd_shim`] with no runtime exec's the target directly via +/// the `@ %*` shape. +#[test] +fn generate_cmd_shim_emits_direct_exec_when_no_runtime() { + let target = Path::new("/p/cli"); + let shim = Path::new("/p/.bin/cli.cmd"); + let body = generate_cmd_shim(target, shim, None); + assert!( + body.contains("@\"%~dp0\\..\\cli\""), + "no-runtime arm must exec the target directly, body:\n{body}", + ); +} + +/// [`generate_pwsh_shim`] produces a `.ps1` shim with the `$basedir` +/// header, `Test-Path "$basedir/$exe"` exec block, and pipeline- +/// input handling matching upstream. +#[test] +fn generate_pwsh_shim_matches_pnpm_template() { + let target = Path::new("/proj/node_modules/typescript/bin/tsc"); + let shim = Path::new("/proj/node_modules/.bin/tsc.ps1"); + let runtime = ScriptRuntime { prog: Some("node".into()), args: String::new() }; + let body = generate_pwsh_shim(target, shim, Some(&runtime)); + + assert!(body.starts_with("#!/usr/bin/env pwsh\n"), "ps1 shim must start with pwsh shebang"); + assert!( + body.contains("$basedir=Split-Path $MyInvocation.MyCommand.Definition -Parent"), + "must declare $basedir from MyInvocation", + ); + assert!(body.contains("$exe=\".exe\""), "Windows-detection branch must set $exe to .exe"); + assert!( + body.contains( + "if (Test-Path \"$basedir/node$exe\") {\n # Support pipeline input\n if ($MyInvocation.ExpectingInput) {\n $input | & \"$basedir/node$exe\" \"$basedir/../typescript/bin/tsc\" $args\n } else {\n & \"$basedir/node$exe\" \"$basedir/../typescript/bin/tsc\" $args\n }", + ), + "exec-with-basedir-prog block must match pnpm's generatePwshShim template, body was:\n{body}", + ); + assert!(body.ends_with("exit $ret\n")); +} + +/// [`generate_pwsh_shim`] with no runtime falls back to executing the +/// target directly with `$LASTEXITCODE` propagation. +#[test] +fn generate_pwsh_shim_emits_direct_exec_when_no_runtime() { + let target = Path::new("/p/cli"); + let shim = Path::new("/p/.bin/cli.ps1"); + let body = generate_pwsh_shim(target, shim, None); + assert!( + body.contains("& \"$basedir/../cli\""), + "no-runtime arm must exec the target directly, body:\n{body}", + ); + assert!(body.ends_with("exit $LASTEXITCODE\n")); +} diff --git a/pacquet/crates/package-manager/Cargo.toml b/pacquet/crates/package-manager/Cargo.toml index 5d0dc1679b..f52b2c5f4a 100644 --- a/pacquet/crates/package-manager/Cargo.toml +++ b/pacquet/crates/package-manager/Cargo.toml @@ -11,6 +11,7 @@ license.workspace = true repository.workspace = true [dependencies] +pacquet-cmd-shim = { workspace = true } pacquet-executor = { workspace = true } pacquet-fs = { workspace = true } pacquet-lockfile = { workspace = true } @@ -31,8 +32,8 @@ httpdate = { workspace = true } node-semver = { workspace = true } pipe-trait = { workspace = true } rayon = { workspace = true } -serde_json = { workspace = true } reflink-copy = { workspace = true } +serde_json = { workspace = true } tokio = { workspace = true } tracing = { workspace = true } miette = { workspace = true } diff --git a/pacquet/crates/package-manager/src/create_virtual_store.rs b/pacquet/crates/package-manager/src/create_virtual_store.rs index 4d32e70c5f..a9d9c6e43c 100644 --- a/pacquet/crates/package-manager/src/create_virtual_store.rs +++ b/pacquet/crates/package-manager/src/create_virtual_store.rs @@ -4,17 +4,36 @@ use crate::{ use derive_more::{Display, Error}; use futures_util::future; use miette::Diagnostic; -use pacquet_lockfile::{LockfileResolution, PackageKey, PackageMetadata, SnapshotEntry}; +use pacquet_lockfile::{ + LockfileResolution, PackageKey, PackageMetadata, PkgNameVerPeer, SnapshotEntry, +}; use pacquet_network::ThrottledClient; use pacquet_npmrc::Npmrc; use pacquet_reporter::{ LogEvent, LogLevel, ProgressLog, ProgressMessage, Reporter, StatsLog, StatsMessage, }; use pacquet_store_dir::{SharedVerifiedFilesCache, StoreIndex, StoreIndexWriter, store_index_key}; -use pacquet_tarball::prefetch_cas_paths; +use pacquet_tarball::{PrefetchResult, prefetch_cas_paths}; use pipe_trait::Pipe; use std::{collections::HashMap, sync::atomic::AtomicU8}; +/// Bundled package manifests recovered from the SQLite store index +/// during [`CreateVirtualStore::run`], keyed by the same +/// `PkgNameVerPeer` (without peer suffix) that +/// [`pacquet_lockfile::Lockfile::packages`] uses. Consumed by the +/// bin-linker so it doesn't have to re-read `package.json` per child +/// during [`crate::LinkVirtualStoreBins::run`]. +/// +/// Only covers the warm-batch packages (those whose tarball was +/// already in the CAFS at install start). Cold-batch packages — ones +/// pacquet had to download — are absent and the bin linker falls +/// back to disk reads for them. That matches pnpm's behaviour for +/// installs that mix warm and cold packages: pnpm's bin linker +/// reads from `pkgFilesIndex.manifest` for warm fetches and from +/// `dep.fetching()?.bundledManifest` for cold ones, but the cold +/// path's `bundledManifest` isn't plumbed through pacquet yet. +pub type PackageManifests = HashMap>; + /// This subroutine generates filesystem layout for the virtual store at `node_modules/.pacquet`. #[must_use] pub struct CreateVirtualStore<'a> { @@ -49,8 +68,11 @@ pub enum CreateVirtualStoreError { } impl<'a> CreateVirtualStore<'a> { - /// Execute the subroutine. - pub async fn run(self) -> Result<(), CreateVirtualStoreError> { + /// Execute the subroutine. Returns the set of bundled manifests + /// recovered from `index.db` for the warm-batch slots — the + /// bin linker uses these to avoid re-reading `package.json` per + /// child. See [`PackageManifests`]. + pub async fn run(self) -> Result { let CreateVirtualStore { http_client, config, @@ -64,7 +86,7 @@ impl<'a> CreateVirtualStore<'a> { // No snapshots to install. If the lockfile also has no project deps // this is a valid no-op; if it does, pnpm would have populated // `snapshots`, so bailing out here is safe enough for v9. - return Ok(()); + return Ok(PackageManifests::new()); }; let packages = packages.ok_or(CreateVirtualStoreError::MissingPackagesSection)?; @@ -224,14 +246,15 @@ impl<'a> CreateVirtualStore<'a> { cache_key_refs.sort_unstable(); cache_key_refs.dedup(); let cache_keys: Vec = cache_key_refs.into_iter().map(String::from).collect(); - let prefetched = prefetch_cas_paths( - store_index.clone(), - store_dir, - cache_keys, - config.verify_store_integrity, - SharedVerifiedFilesCache::clone(&verified_files_cache), - ) - .await; + let PrefetchResult { cas_paths: prefetched, manifests: prefetched_manifests } = + prefetch_cas_paths( + store_index.clone(), + store_dir, + cache_keys, + config.verify_store_integrity, + SharedVerifiedFilesCache::clone(&verified_files_cache), + ) + .await; // Partition snapshots by whether the prefetch covered them. The // warm batch — every snapshot whose tarball is already in the @@ -266,7 +289,25 @@ impl<'a> CreateVirtualStore<'a> { // local alias drifting (Copilot review on #292). let mut warm = Vec::with_capacity(snapshot_entries.len()); let mut cold: Vec<(&PackageKey, &SnapshotEntry)> = Vec::new(); + // Build a `metadata_key -> manifest` lookup from the prefetched + // index rows. Snapshot keys differ across peer-resolved + // variants of the same package (`react-dom@17.0.2(react@...)`), + // but the bundled manifest is identical across variants + // because every variant resolves to the same tarball. Keying + // by [`PkgNameVerPeer::without_peer`] collapses the variants + // to one entry: same shape as + // [`pacquet_lockfile::Lockfile::packages`], which is what the + // bin linker already looks up by. + let mut package_manifests: PackageManifests = + HashMap::with_capacity(prefetched_manifests.len()); for (snapshot_key, snapshot, cache_key) in &snapshot_entries { + if let Some(cache_key) = cache_key.as_deref() + && let Some(manifest) = prefetched_manifests.get(cache_key) + { + package_manifests + .entry(snapshot_key.without_peer()) + .or_insert_with(|| std::sync::Arc::clone(manifest)); + } match cache_key.as_deref().and_then(|k| prefetched.get(k)) { Some(cas_paths) => warm.push((snapshot_key, snapshot, cas_paths)), None => cold.push((snapshot_key, snapshot)), @@ -376,7 +417,7 @@ impl<'a> CreateVirtualStore<'a> { ), } - Ok(()) + Ok(package_manifests) } } diff --git a/pacquet/crates/package-manager/src/install/snapshots/pacquet_package_manager__install__tests__should_install_dependencies.snap b/pacquet/crates/package-manager/src/install/snapshots/pacquet_package_manager__install__tests__should_install_dependencies.snap index 27a0b5e07a..9856cbdc31 100644 --- a/pacquet/crates/package-manager/src/install/snapshots/pacquet_package_manager__install__tests__should_install_dependencies.snap +++ b/pacquet/crates/package-manager/src/install/snapshots/pacquet_package_manager__install__tests__should_install_dependencies.snap @@ -5,6 +5,7 @@ expression: get_all_folders(&project_root) --- [ "node_modules", + "node_modules/.bin", "node_modules/.pacquet", "node_modules/.pacquet/@pnpm+x@1.0.0", "node_modules/.pacquet/@pnpm+x@1.0.0/node_modules", diff --git a/pacquet/crates/package-manager/src/install_frozen_lockfile.rs b/pacquet/crates/package-manager/src/install_frozen_lockfile.rs index 633dfcb43e..5afc93be78 100644 --- a/pacquet/crates/package-manager/src/install_frozen_lockfile.rs +++ b/pacquet/crates/package-manager/src/install_frozen_lockfile.rs @@ -1,6 +1,7 @@ use crate::{ AllowBuildPolicy, BuildModules, BuildModulesError, CreateVirtualStore, CreateVirtualStoreError, - SymlinkDirectDependencies, SymlinkDirectDependenciesError, + LinkVirtualStoreBins, LinkVirtualStoreBinsError, SymlinkDirectDependencies, + SymlinkDirectDependenciesError, }; use derive_more::{Display, Error}; use miette::Diagnostic; @@ -47,6 +48,9 @@ pub enum InstallFrozenLockfileError { #[diagnostic(transparent)] SymlinkDirectDependencies(#[error(source)] SymlinkDirectDependenciesError), + #[diagnostic(transparent)] + LinkVirtualStoreBins(#[error(source)] LinkVirtualStoreBinsError), + #[diagnostic(transparent)] BuildModules(#[error(source)] BuildModulesError), } @@ -70,16 +74,42 @@ where // TODO: check if the lockfile is out-of-date - CreateVirtualStore { http_client, config, packages, snapshots, logged_methods, requester } - .run::() - .await - .map_err(InstallFrozenLockfileError::CreateVirtualStore)?; + let package_manifests = CreateVirtualStore { + http_client, + config, + packages, + snapshots, + logged_methods, + requester, + } + .run::() + .await + .map_err(InstallFrozenLockfileError::CreateVirtualStore)?; SymlinkDirectDependencies { config, importers, dependency_groups, requester } .run::() .map_err(InstallFrozenLockfileError::SymlinkDirectDependencies)?; - // Mirrors upstream `link.ts:167-170` — `importing_done` fires once + // Link the bins of each virtual-store slot's children into the + // slot's own `node_modules/.bin`. Pnpm runs this from + // `linkBinsOfDependencies` during the headless install. See + // . + // Done before `importing_done` so reporters see the import phase + // close only after every link (including per-slot bins) is in + // place. The manifest map threaded from `CreateVirtualStore` + // lets the linker hit `pkgFilesIndex.manifest` directly + // (matching pnpm's `bundledManifest`-from-CAFS path) instead + // of re-reading every child's `package.json` from disk. + LinkVirtualStoreBins { + virtual_store_dir: &config.virtual_store_dir, + snapshots, + packages, + package_manifests: &package_manifests, + } + .run() + .map_err(InstallFrozenLockfileError::LinkVirtualStoreBins)?; + + // Mirrors upstream `link.ts:167-170`: `importing_done` fires once // extraction and symlink linking are complete, before any build // phase. Reporters use it to close the import progress display so // subsequent `pnpm:lifecycle` events render in their own section. diff --git a/pacquet/crates/package-manager/src/install_without_lockfile.rs b/pacquet/crates/package-manager/src/install_without_lockfile.rs index 6502be70f1..7f66a691cc 100644 --- a/pacquet/crates/package-manager/src/install_without_lockfile.rs +++ b/pacquet/crates/package-manager/src/install_without_lockfile.rs @@ -1,12 +1,13 @@ use crate::{ - InstallPackageFromRegistry, InstallPackageFromRegistryError, - store_init::init_store_dir_best_effort, + InstallPackageFromRegistry, InstallPackageFromRegistryError, LinkVirtualStoreBins, + LinkVirtualStoreBinsError, store_init::init_store_dir_best_effort, }; use async_recursion::async_recursion; use dashmap::DashSet; use derive_more::{Display, Error}; use futures_util::future; use miette::Diagnostic; +use pacquet_cmd_shim::{LinkBinsError, RealApi, link_bins}; use pacquet_network::ThrottledClient; use pacquet_npmrc::Npmrc; use pacquet_package_manifest::{DependencyGroup, PackageManifest}; @@ -52,6 +53,12 @@ pub struct InstallWithoutLockfile<'a, DependencyGroupList> { pub enum InstallWithoutLockfileError { #[diagnostic(transparent)] InstallPackageFromRegistry(#[error(source)] InstallPackageFromRegistryError), + + #[diagnostic(transparent)] + LinkBins(#[error(source)] LinkBinsError), + + #[diagnostic(transparent)] + LinkVirtualStoreBins(#[error(source)] LinkVirtualStoreBinsError), } impl<'a, DependencyGroupList> InstallWithoutLockfile<'a, DependencyGroupList> { @@ -182,7 +189,33 @@ impl<'a, DependencyGroupList> InstallWithoutLockfile<'a, DependencyGroupList> { ), } - // Mirrors upstream `link.ts:167-170` — `importing_done` fires once + // Link bins. Direct dependencies first (root project's + // `node_modules/.bin`) and then per-slot children inside the + // virtual store. Mirrors the same two-call shape as + // `install_frozen_lockfile.rs`. We re-walk `` instead + // of replaying the manifest because the `dependency_groups` + // iterator was already consumed by the install loop above; pnpm's + // own `linkBins(modulesDir, binsDir)` overload uses the same + // strategy. + link_bins::(&config.modules_dir, &config.modules_dir.join(".bin")) + .map_err(InstallWithoutLockfileError::LinkBins)?; + + // No lockfile here, so no prefetched manifests are available — + // fall back to the legacy readdir-driven path (slots discovered + // by walking ``, child manifests read from + // disk). The frozen-lockfile path skips both via + // [`LinkVirtualStoreBins::snapshots`] / `package_manifests`. + let empty_manifests = std::collections::HashMap::new(); + LinkVirtualStoreBins { + virtual_store_dir: &config.virtual_store_dir, + snapshots: None, + packages: None, + package_manifests: &empty_manifests, + } + .run() + .map_err(InstallWithoutLockfileError::LinkVirtualStoreBins)?; + + // Mirrors upstream `link.ts:167-170`: `importing_done` fires once // extraction and symlink linking are complete. The without-lockfile // path does not run lifecycle scripts today, so emitting here also // marks end-of-install for reporters. diff --git a/pacquet/crates/package-manager/src/lib.rs b/pacquet/crates/package-manager/src/lib.rs index 243812dee8..26e6b66f40 100644 --- a/pacquet/crates/package-manager/src/lib.rs +++ b/pacquet/crates/package-manager/src/lib.rs @@ -12,6 +12,7 @@ mod install_frozen_lockfile; mod install_package_by_snapshot; mod install_package_from_registry; mod install_without_lockfile; +mod link_bins; mod link_file; mod retry_config; mod store_init; @@ -32,6 +33,7 @@ pub use install_frozen_lockfile::*; pub use install_package_by_snapshot::*; pub use install_package_from_registry::*; pub use install_without_lockfile::*; +pub use link_bins::*; pub use link_file::*; pub use symlink_direct_dependencies::*; pub use symlink_package::*; diff --git a/pacquet/crates/package-manager/src/link_bins.rs b/pacquet/crates/package-manager/src/link_bins.rs new file mode 100644 index 0000000000..dda6448208 --- /dev/null +++ b/pacquet/crates/package-manager/src/link_bins.rs @@ -0,0 +1,549 @@ +use crate::PackageManifests; +use derive_more::{Display, Error}; +use miette::Diagnostic; +use pacquet_cmd_shim::{ + FsCreateDirAll, FsEnsureExecutableBits, FsReadDir, FsReadFile, FsReadHead, FsReadString, + FsSetExecutable, FsWalkFiles, FsWrite, LinkBinsError, PackageBinSource, RealApi, + link_bins_of_packages, +}; +use pacquet_lockfile::{PackageKey, PackageMetadata, PkgName, SnapshotEntry}; +use rayon::prelude::*; +use std::{ + collections::{HashMap, HashSet}, + fs, io, + path::{Path, PathBuf}, + sync::Arc, +}; + +/// Read the `package.json` of every direct dependency under `modules_dir` +/// and link its bins into `/.bin`. +/// +/// `dep_names` is the list of direct-dependency keys as they appear in +/// `package.json`, the same names already symlinked under `/` +/// by [`crate::SymlinkDirectDependencies`]. We resolve `package.json` via +/// the symlink (`fs::read` follows it transparently) so the read targets +/// the real package contents in the virtual store. +/// +/// Driven on rayon because each location's read+parse is independent. +/// Mirrors pnpm v11's `linkBinsOfPackages` call site for direct deps: +/// . +pub fn link_direct_dep_bins(modules_dir: &Path, dep_names: &[String]) -> Result<(), LinkBinsError> { + let direct_dep_locations: Vec = + dep_names.iter().map(|name| modules_dir.join(name)).collect(); + // Swallow only `NotFound`: a direct-dep symlink target can + // legitimately be missing right after a partial pacquet run, or + // be an in-progress install. Every other IO error (permission + // denied, EIO, etc.) and every JSON parse error must surface as + // `LinkBinsError::{ReadManifest, ParseManifest}` so the failure + // is diagnosable rather than hiding behind a missing `.bin` + // entry. Matches the read-side error policy in + // `pacquet_cmd_shim::link_bins`. + let bin_sources: Vec = direct_dep_locations + .par_iter() + .filter_map(|location| { + let manifest_path = location.join("package.json"); + let bytes = match fs::read(&manifest_path) { + Ok(bytes) => bytes, + Err(error) if error.kind() == io::ErrorKind::NotFound => return None, + Err(error) => { + return Some(Err(LinkBinsError::ReadManifest { path: manifest_path, error })); + } + }; + let manifest: serde_json::Value = match serde_json::from_slice(&bytes) { + Ok(manifest) => manifest, + Err(error) => { + return Some(Err(LinkBinsError::ParseManifest { path: manifest_path, error })); + } + }; + Some(Ok(PackageBinSource { location: location.clone(), manifest: Arc::new(manifest) })) + }) + .collect::>()?; + if bin_sources.is_empty() { + return Ok(()); + } + link_bins_of_packages::(&bin_sources, &modules_dir.join(".bin")) +} + +/// Error type of [`LinkVirtualStoreBins`]. +#[derive(Debug, Display, Error, Diagnostic)] +pub enum LinkVirtualStoreBinsError { + #[display("Failed to read virtual store directory at {dir:?}: {error}")] + #[diagnostic(code(pacquet_package_manager::read_virtual_store))] + ReadVirtualStore { + dir: PathBuf, + #[error(source)] + error: io::Error, + }, + + #[diagnostic(transparent)] + LinkBins(#[error(source)] LinkBinsError), +} + +/// For every package slot under `/@/node_modules`, +/// link the bins of that slot's child packages into the slot's *own* +/// `node_modules/.bin` directory. +/// +/// This mirrors `linkBinsOfDependencies` in pnpm's `building/during-install` +/// (see ). +/// pnpm walks each `depNode`, takes its `children` (its direct deps in the +/// resolved graph) and writes their bins into +/// `/node_modules/.bin`. +/// +/// Pacquet's virtual store layout already exposes a slot's children as +/// siblings via [`create_symlink_layout`](crate::create_symlink_layout()). +/// So once the symlinks exist, walking +/// the slot's `node_modules` and excluding the package itself gives the same +/// child-set pnpm uses, and the bins go into the package's own +/// `node_modules/.bin` (i.e. nested *one level deeper* than the slot's +/// `node_modules` directory). +/// +/// Path layout produced for a slot `A@1.0.0`: +/// +/// ```text +/// /A@1.0.0/node_modules/A/node_modules/.bin/ +/// ``` +/// +/// When `snapshots` is `Some` (the frozen-lockfile case), the slot +/// set is taken from the lockfile and each child's manifest is +/// looked up in `package_manifests` rather than read off disk — +/// matching pnpm's `linkBinsOfDependencies` which consumes +/// `bundledManifest` straight out of the SQLite store index (see +/// ). +/// When `snapshots` is `None` (install without a lockfile), the +/// linker falls back to enumerating slots and reading manifests via +/// the filesystem, the shape this code had before the +/// lockfile-driven path landed. +#[must_use] +pub struct LinkVirtualStoreBins<'a> { + pub virtual_store_dir: &'a Path, + /// `Some` when the install is lockfile-driven. Iterating the + /// snapshot map (instead of `read_dir(virtual_store_dir)`) + /// removes the per-slot directory enumeration and lets us walk + /// each slot's children from its `dependencies` / + /// `optionalDependencies` lists without touching the filesystem. + pub snapshots: Option<&'a HashMap>, + /// Lockfile `packages:` section, indexed by `PkgNameVerPeer` + /// (without peer suffix). Used to filter children by + /// `hasBin == true` *before* any per-child IO — mirrors pnpm's + /// `dep.hasBin` filter in + /// [`linkBinsOfDependencies`](https://github.com/pnpm/pnpm/blob/4750fd370c/building/during-install/src/index.ts#L283). + /// Most packages don't declare a bin, so this short-circuits the + /// bulk of the per-slot work before any path-building or manifest + /// lookup happens. + pub packages: Option<&'a HashMap>, + /// Bundled manifests recovered from the warm-cache prefetch of + /// `index.db` ([`crate::PackageManifests`]). A hit lets the + /// linker skip the `package.json` read for that child entirely; + /// a miss falls back to a disk read so cold-batch packages + /// installed earlier in the same run still get their bins + /// linked. + pub package_manifests: &'a PackageManifests, +} + +impl<'a> LinkVirtualStoreBins<'a> { + pub fn run(self) -> Result<(), LinkVirtualStoreBinsError> { + self.run_with::() + } + + /// DI-driven entry. Production callers go through [`Self::run`] which + /// turbofishes [`RealApi`]; tests inject fakes that fail specific fs + /// operations to cover error paths the real fs can't trigger + /// portably. See the per-capability DI pattern at + /// . + pub fn run_with(self) -> Result<(), LinkVirtualStoreBinsError> + where + Api: FsReadDir + + FsReadFile + + FsReadString + + FsReadHead + + FsCreateDirAll + + FsWalkFiles + + FsWrite + + FsSetExecutable + + FsEnsureExecutableBits, + { + let LinkVirtualStoreBins { virtual_store_dir, snapshots, packages, package_manifests } = + self; + if let Some(snapshots) = snapshots { + let has_bin_set = build_has_bin_set(packages); + run_lockfile_driven::( + virtual_store_dir, + snapshots, + has_bin_set.as_ref(), + package_manifests, + ) + } else { + run_with_readdir::(virtual_store_dir) + } + } +} + +/// Pre-compute the set of package keys whose lockfile metadata sets +/// `hasBin: true`. Mirrors pnpm's filter at +/// [`during-install/src/index.ts:283`](https://github.com/pnpm/pnpm/blob/4750fd370c/building/during-install/src/index.ts#L283): +/// most packages don't declare a bin, so short-circuiting the +/// per-child manifest lookup with this set is the cheapest win on +/// warm-cache installs. +/// +/// Return-value semantics distinguish "lockfile metadata absent" +/// from "lockfile metadata says no package has a bin": +/// +/// - `None` — the lockfile's `packages:` section wasn't supplied +/// (pathological lockfile shape). We have no info, so the bin +/// linker falls back to the conservative "process every child" +/// path and lets the per-package bin resolver sort it out. +/// - `Some(set)` — the section was present and we used it. The +/// `set` contains only entries with `hasBin == Some(true)`; an +/// *empty* `Some(set)` is authoritative: the lockfile says no +/// package has a bin, and every slot should short-circuit +/// immediately. Conflating this case with `None` (the bug Copilot +/// flagged at ) +/// would force per-child work the lockfile already ruled out. +fn build_has_bin_set( + packages: Option<&HashMap>, +) -> Option> { + let packages = packages?; + Some( + packages + .iter() + .filter(|(_, meta)| meta.has_bin == Some(true)) + .map(|(key, _)| key.clone()) + .collect(), + ) +} + +/// Walk the lockfile's `snapshots:` map, build each slot's bin output +/// directory lexically, and link every child's bins into it. The +/// child set comes from `snapshot.dependencies` + +/// `snapshot.optional_dependencies`, filtered by `has_bin_set` so +/// packages that don't declare a bin never make it into the +/// per-slot path-building or manifest-lookup work. The corresponding +/// manifest comes from [`PackageManifests`] (no disk read) or, for +/// cold-batch packages that prefetch missed, a fallback +/// `package.json` read through the existing symlink at +/// `/node_modules/`. +fn run_lockfile_driven( + virtual_store_dir: &Path, + snapshots: &HashMap, + has_bin_set: Option<&HashSet>, + package_manifests: &PackageManifests, +) -> Result<(), LinkVirtualStoreBinsError> +where + Api: FsReadFile + + FsReadString + + FsReadHead + + FsCreateDirAll + + FsWalkFiles + + FsWrite + + FsSetExecutable + + FsEnsureExecutableBits, +{ + // `has_bin_set` is `Some` exactly when the lockfile's `packages:` + // section was present at install start — in which case the set + // is authoritative and every slot is filtered through it (an + // empty `Some(set)` means "no package declares a bin", which + // short-circuits every slot below). When the section was + // missing we have no info and fall through to processing every + // child. See [`build_has_bin_set`] for the rationale. + // Materialise as a `Vec` so rayon can split the work; iterating + // a `HashMap` directly with `par_iter` would require collecting + // anyway, and explicit collection here keeps the parallelism + // contract obvious. + let slot_entries: Vec<(&PackageKey, &SnapshotEntry)> = snapshots.iter().collect(); + slot_entries.par_iter().try_for_each(|(slot_key, snapshot)| { + let children = snapshot + .dependencies + .iter() + .flatten() + .chain(snapshot.optional_dependencies.iter().flatten()); + + // First pass: figure out which children (if any) have a bin + // declared. Cheap — just hash-set lookups against the + // pre-built `has_bin_set` and a `without_peer` materialisation + // per child. If no child has a bin, skip the slot entirely — + // we don't even build the slot's path. Slots in this category + // are the bulk of a real lockfile (~95% in the integrated + // benchmark fixture); skipping them removes the dominant + // chunk of the per-install bin-link work. + let with_bin: Vec<(&PkgName, PackageKey)> = children + .filter_map(|(alias, dep_ref)| { + let child_key = dep_ref.resolve(alias); + let metadata_key = child_key.without_peer(); + let keep = match has_bin_set { + Some(set) => set.contains(&metadata_key), + None => true, + }; + keep.then_some((alias, metadata_key)) + }) + .collect(); + if with_bin.is_empty() { + return Ok(()); + } + + let slot_dir = virtual_store_dir.join(slot_key.to_virtual_store_name()); + let modules_dir = slot_dir.join("node_modules"); + let self_pkg_dir = slot_own_pkg_dir(&modules_dir, slot_key); + let bins_dir = self_pkg_dir.join("node_modules/.bin"); + + let mut bin_sources: Vec = Vec::with_capacity(with_bin.len()); + for (alias, metadata_key) in with_bin { + let child_location = pkg_dir_under(&modules_dir, alias); + if let Some(manifest) = package_manifests.get(&metadata_key) { + // Hot path: parsed manifest already in memory from + // the warm-cache prefetch. Both the prefetch map + // and `PackageBinSource` hold the manifest via + // [`Arc`], so this is a refcount bump rather than a + // deep clone of the JSON tree. Avoids the + // `slots × children`-sized clone fan-out that + // dominated the previous version of this path on + // warm-cache installs. + bin_sources.push(PackageBinSource { + location: child_location, + manifest: Arc::clone(manifest), + }); + } else { + // Cold-batch fallback: package was downloaded + // earlier in the run, so its row isn't in the + // prefetched manifest map yet. Reading from disk + // here is the same code path as the non-lockfile + // install — see [`run_with_readdir`]. + match read_package::(&child_location) { + Ok(Some(pkg)) => bin_sources.push(pkg), + Ok(None) => {} + Err(error) => return Err(LinkVirtualStoreBinsError::LinkBins(error)), + } + } + } + + if bin_sources.is_empty() { + return Ok(()); + } + link_bins_of_packages::(&bin_sources, &bins_dir) + .map_err(LinkVirtualStoreBinsError::LinkBins) + }) +} + +/// Compute `/node_modules/` for the slot's +/// own package. The slot's package name lives on the lockfile key, +/// so no filesystem probing is needed (the directory is an invariant +/// maintained by [`crate::create_virtual_dir_by_snapshot`]). Scoped +/// names land at `/@scope/`, unscoped names at +/// `/`. +fn slot_own_pkg_dir(modules_dir: &Path, slot_key: &PackageKey) -> PathBuf { + pkg_dir_under(modules_dir, &slot_key.name) +} + +/// Join a package name onto a `node_modules` directory, handling the +/// `@scope/name` split into two path components. Operates on the raw +/// [`PkgName`] (whose `scope` and `bare` fields are already split), +/// not on the virtual-store-name form — for instance the input +/// represents `@types/node`, **not** `@types+node`. +fn pkg_dir_under(modules_dir: &Path, name: &PkgName) -> PathBuf { + match &name.scope { + Some(scope) => modules_dir.join(format!("@{scope}")).join(&name.bare), + None => modules_dir.join(&name.bare), + } +} + +/// Fallback (non-lockfile) path: enumerate slots via `read_dir`, +/// then walk each slot's `node_modules` to discover children. Used +/// only by [`crate::InstallWithoutLockfile`] today; the lockfile +/// path bypasses every directory enumeration in here. +fn run_with_readdir(virtual_store_dir: &Path) -> Result<(), LinkVirtualStoreBinsError> +where + Api: FsReadDir + + FsReadFile + + FsReadString + + FsReadHead + + FsCreateDirAll + + FsWalkFiles + + FsWrite + + FsSetExecutable + + FsEnsureExecutableBits, +{ + let slots = match Api::read_dir(virtual_store_dir) { + Ok(slots) => slots, + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(()), + Err(error) => { + return Err(LinkVirtualStoreBinsError::ReadVirtualStore { + dir: virtual_store_dir.to_path_buf(), + error, + }); + } + }; + let slots: Vec = slots.collect(); + slots.par_iter().try_for_each(|slot_dir| { + let modules_dir = slot_dir.join("node_modules"); + let Some(self_pkg_dir) = find_slot_own_package_dir(slot_dir, &modules_dir) else { + return Ok(()); + }; + // Probe the slot's own package directory before walking its + // children. Without the probe, an incomplete slot whose + // `node_modules/` is missing but whose sibling deps are + // still present would have `link_bins_excluding` collect the + // siblings and `create_dir_all` the missing `` chain to + // hold the shims, leaving an orphan package directory on + // disk. This path runs only for [`crate::InstallWithoutLockfile`] + // and visits ~direct-deps slots (small N), so the probe cost + // is trivial; the lockfile-driven path bypasses this by + // treating the slot's own pkg dir as an invariant of + // [`crate::create_virtual_dir_by_snapshot`]. + if Api::read_dir(&self_pkg_dir).is_err() { + return Ok(()); + } + let bins_dir = self_pkg_dir.join("node_modules/.bin"); + link_bins_excluding::(&modules_dir, &bins_dir, &self_pkg_dir) + .map_err(LinkVirtualStoreBinsError::LinkBins) + }) +} + +/// Locate the slot's own package directory inside `/node_modules`. +/// +/// The slot directory's name encodes the package name as +/// `+@` 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. +/// +/// Returns `None` only when the slot name fails to parse — there's no +/// filesystem probe for the resolved candidate. The previous version +/// stat-equivalent-ed the path with `Api::read_dir` to short-circuit +/// missing slots, but on a 1267-package fixture that was 1267 +/// wasted `open(O_DIRECTORY) + close` round-trips on the hot path of +/// every warm install. The slot's own package directory is an +/// invariant of [`crate::create_virtual_dir_by_snapshot`]; the +/// downstream `link_bins_excluding` handles `NotFound` from its own +/// `read_dir` of `/node_modules` cleanly when the invariant +/// ever does break, so the probe is pure overhead. +fn find_slot_own_package_dir(slot_dir: &Path, modules_dir: &Path) -> Option { + let slot_name = slot_dir.file_name()?.to_str()?; + + // 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 scoped = slot_name.starts_with('@'); + let search_start = usize::from(scoped); + let at = search_start + slot_name[search_start..].find('@')?; + let name_part = &slot_name[..at]; + + // `+` separates `+` for scoped packages, and *only* + // for scoped packages. Gating on `scoped` avoids misparsing a + // hypothetical unscoped name that contains `+`: `PkgName::parse` + // does not reject non-URL-safe characters (only npm's + // `validate-npm-package-name` warns about them), so an unscoped + // name like `foo+bar` could in principle reach here and would + // otherwise be split into `foo` / `bar`. + let pkg_dir = match scoped.then(|| name_part.split_once('+')).flatten() { + Some((scope, name)) => modules_dir.join(scope).join(name), + None => modules_dir.join(name_part), + }; + Some(pkg_dir) +} + +/// Like [`pacquet_cmd_shim::link_bins`] but skipping the slot's own package +/// from the candidate set. Without this, a slot for `tsc@5.0.0` would link +/// its own `tsc` bin into its own `node_modules/.bin`, which pnpm doesn't. +fn link_bins_excluding( + modules_dir: &Path, + bins_dir: &Path, + exclude: &Path, +) -> Result<(), LinkBinsError> +where + Api: FsReadDir + + FsReadFile + + FsReadString + + FsReadHead + + FsCreateDirAll + + FsWalkFiles + + FsWrite + + FsSetExecutable + + FsEnsureExecutableBits, +{ + let mut packages: Vec = Vec::new(); + + let entries = match Api::read_dir(modules_dir) { + Ok(entries) => entries, + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(()), + Err(error) => { + return Err(LinkBinsError::ReadModulesDir { dir: modules_dir.to_path_buf(), error }); + } + }; + + for path in entries { + let Some(name) = path.file_name() else { + continue; + }; + let name_str = name.to_string_lossy(); + if name_str.starts_with('.') { + continue; + } + + if name_str.starts_with('@') { + // Only `NotFound` is plausibly skippable here (a + // concurrent scope-dir delete). Other errors — + // permission denied, EIO, AppArmor deny — would mean + // the bins for every package under this scope silently + // disappear, so surface them instead of letting them + // hide. + let scope_entries = match Api::read_dir(&path) { + Ok(entries) => entries, + Err(error) if error.kind() == io::ErrorKind::NotFound => continue, + Err(error) => { + return Err(LinkBinsError::ReadModulesDir { dir: path.clone(), error }); + } + }; + for sub_path in scope_entries { + if paths_eq(&sub_path, exclude) { + continue; + } + if let Some(pkg) = read_package::(&sub_path)? { + packages.push(pkg); + } + } + continue; + } + + if paths_eq(&path, exclude) { + continue; + } + if let Some(pkg) = read_package::(&path)? { + packages.push(pkg); + } + } + + if packages.is_empty() { + return Ok(()); + } + + link_bins_of_packages::(&packages, bins_dir) +} + +fn read_package( + location: &Path, +) -> Result, LinkBinsError> { + let manifest_path = location.join("package.json"); + let bytes = match Api::read_file(&manifest_path) { + Ok(bytes) => bytes, + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(None), + Err(error) => return Err(LinkBinsError::ReadManifest { path: manifest_path, error }), + }; + let manifest: serde_json::Value = serde_json::from_slice(&bytes) + .map_err(|error| LinkBinsError::ParseManifest { path: manifest_path, error })?; + Ok(Some(PackageBinSource { location: location.to_path_buf(), manifest: Arc::new(manifest) })) +} + +fn paths_eq(a: &Path, b: &Path) -> bool { + // Lexical comparison is enough; both paths come from the same + // `node_modules` walk and don't go through canonicalisation. + a == b +} + +#[cfg(test)] +mod tests; diff --git a/pacquet/crates/package-manager/src/link_bins/tests.rs b/pacquet/crates/package-manager/src/link_bins/tests.rs new file mode 100644 index 0000000000..a5ca84caaf --- /dev/null +++ b/pacquet/crates/package-manager/src/link_bins/tests.rs @@ -0,0 +1,475 @@ +use super::{LinkVirtualStoreBins, LinkVirtualStoreBinsError, link_direct_dep_bins}; +use pacquet_cmd_shim::is_shim_pointing_at; +use serde_json::json; +use std::{ + fs::{create_dir_all, read_to_string, write as write_file}, + iter::{Empty, empty}, + path::{Path, PathBuf}, +}; +use tempfile::tempdir; + +/// End-to-end exercise of [`LinkVirtualStoreBins`] against a hand-built +/// virtual store. Slot `parent@1.0.0` has one child `child` declaring a +/// bin; after the run, the child's shim must land at +/// `parent@1.0.0/node_modules/parent/node_modules/.bin/child` and *not* +/// at the slot's own `node_modules/.bin` (which is what would happen if +/// we accidentally pointed at the wrong directory). +#[test] +fn writes_child_bins_into_slot_own_package_node_modules() { + let tmp = tempdir().unwrap(); + let virtual_dir = tmp.path().join(".pacquet"); + + // The slot for `parent@1.0.0`. pnpm uses `+` for scope separator. + let slot = virtual_dir.join("parent@1.0.0"); + let modules = slot.join("node_modules"); + let parent_dir = modules.join("parent"); + let child_dir = modules.join("child"); + create_dir_all(&parent_dir).unwrap(); + create_dir_all(&child_dir).unwrap(); + + write_file( + parent_dir.join("package.json"), + json!({"name": "parent", "version": "1.0.0"}).to_string(), + ) + .unwrap(); + write_file( + child_dir.join("package.json"), + json!({"name": "child", "version": "1.0.0", "bin": "cli.js"}).to_string(), + ) + .unwrap(); + write_file(child_dir.join("cli.js"), "#!/usr/bin/env node\n").unwrap(); + + LinkVirtualStoreBins { + virtual_store_dir: &virtual_dir, + snapshots: None, + packages: None, + package_manifests: &Default::default(), + } + .run() + .unwrap(); + + let shim_path = parent_dir.join("node_modules/.bin/child"); + assert!(shim_path.exists(), "expected shim at {shim_path:?}"); + let body = read_to_string(&shim_path).unwrap(); + // Layout, with shim at A and target at B, relative path `A → B`: + // + // /node_modules/parent/node_modules/.bin/child (shim, A) + // /node_modules/child/cli.js (target, B) + // + // Common prefix is `/node_modules`. A has three extra + // segments after that (`parent`, `node_modules`, `.bin`); B has + // two (`child`, `cli.js`). Relative = `../../../child/cli.js`. + assert!( + body.contains("\"$basedir/../../../child/cli.js\""), + "shim must reference the sibling child via the right number of `..`s, got:\n{body}", + ); +} + +/// A slot whose own package also declares a bin must NOT have that bin +/// linked into its own `node_modules/.bin`. pnpm only links *children* +/// of a slot, so a tsc slot does not redundantly produce a shim for +/// its own tsc binary. +/// +/// To distinguish the exclusion logic from "the slot wasn't processed +/// at all," the slot has a real child (`other`) whose bin SHOULD be +/// linked. The assertions then check both directions: +/// +/// 1. The child bin appears in `/node_modules//node_modules/.bin/`. +/// 2. The slot's own bin does NOT appear there. +/// +/// If [`super::find_slot_own_package_dir`] returns `None` (slot skipped), +/// (1) fails. If the exclusion logic is dropped, (2) fails. Either +/// failure surfaces the regression. +#[test] +fn skips_slot_own_package_when_walking_children() { + let tmp = tempdir().unwrap(); + let virtual_dir = tmp.path().join(".pacquet"); + + let slot = virtual_dir.join("tsc@5.0.0"); + let modules = slot.join("node_modules"); + let pkg_dir = modules.join("tsc"); + let other_dir = modules.join("other"); + create_dir_all(&pkg_dir).unwrap(); + create_dir_all(&other_dir).unwrap(); + + write_file( + pkg_dir.join("package.json"), + json!({"name": "tsc", "version": "5.0.0", "bin": "tsc.js"}).to_string(), + ) + .unwrap(); + write_file(pkg_dir.join("tsc.js"), "#!/usr/bin/env node\n").unwrap(); + + write_file( + other_dir.join("package.json"), + json!({"name": "other", "version": "1.0.0", "bin": "other.js"}).to_string(), + ) + .unwrap(); + write_file(other_dir.join("other.js"), "#!/usr/bin/env node\n").unwrap(); + + LinkVirtualStoreBins { + virtual_store_dir: &virtual_dir, + snapshots: None, + packages: None, + package_manifests: &Default::default(), + } + .run() + .unwrap(); + + let bin_dir = pkg_dir.join("node_modules/.bin"); + assert!( + bin_dir.join("other").exists(), + "child bin `other` must be linked under the slot's own package", + ); + assert!(!bin_dir.join("tsc").exists(), "self-bin `tsc` must not be linked into own slot"); +} + +/// [`LinkVirtualStoreBins`] with a non-existent virtual-store directory +/// must be a no-op (`Ok`). A fresh install where the dir doesn't exist +/// yet must not error out. +#[test] +fn link_virtual_store_bins_no_op_when_dir_missing() { + let tmp = tempdir().unwrap(); + let nonexistent = tmp.path().join("does-not-exist"); + LinkVirtualStoreBins { + virtual_store_dir: &nonexistent, + snapshots: None, + packages: None, + package_manifests: &Default::default(), + } + .run() + .expect("missing dir is Ok"); +} + +/// Slot whose name has a `+` (scope separator) resolves to +/// `node_modules//`. Pins [`super::find_slot_own_package_dir`]'s +/// scoped branch. The un-scoped branch is exercised by the existing +/// `writes_child_bins_into_slot_own_package_node_modules` test. +#[test] +fn link_virtual_store_bins_handles_scoped_slot_name() { + let tmp = tempdir().unwrap(); + let virtual_dir = tmp.path().join(".pacquet"); + let slot = virtual_dir.join("@scope+parent@1.0.0"); + let modules = slot.join("node_modules"); + let parent_dir = modules.join("@scope/parent"); + let child_dir = modules.join("child"); + create_dir_all(&parent_dir).unwrap(); + create_dir_all(&child_dir).unwrap(); + + write_file( + parent_dir.join("package.json"), + json!({"name": "@scope/parent", "version": "1.0.0"}).to_string(), + ) + .unwrap(); + write_file( + child_dir.join("package.json"), + json!({"name": "child", "version": "1.0.0", "bin": "cli.js"}).to_string(), + ) + .unwrap(); + write_file(child_dir.join("cli.js"), "#!/usr/bin/env node\n").unwrap(); + + LinkVirtualStoreBins { + virtual_store_dir: &virtual_dir, + snapshots: None, + packages: None, + package_manifests: &Default::default(), + } + .run() + .unwrap(); + + let shim = parent_dir.join("node_modules/.bin/child"); + 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). [`super::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"); + create_dir_all(&pkg_dir).unwrap(); + create_dir_all(&child_dir).unwrap(); + + write_file( + pkg_dir.join("package.json"), + json!({"name": "ts-node", "version": "10.9.1"}).to_string(), + ) + .unwrap(); + write_file( + child_dir.join("package.json"), + json!({"name": "child", "version": "1.0.0", "bin": "cli.js"}).to_string(), + ) + .unwrap(); + write_file(child_dir.join("cli.js"), "#!/usr/bin/env node\n").unwrap(); + + LinkVirtualStoreBins { + virtual_store_dir: &virtual_dir, + snapshots: None, + packages: None, + package_manifests: &Default::default(), + } + .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", + ); +} + +/// An unscoped slot name that contains `+` must be treated as a +/// single bare package name, not split on `+` like the scoped form +/// `+`. `PkgName::parse` does not reject non-URL-safe +/// characters (npm's `validate-npm-package-name` only emits a +/// warning), so a manifest with `"name": "foo+bar"` reaches +/// `to_virtual_store_name` unchanged and the slot ends up as +/// `foo+bar@1.0.0`. Without gating the `+` split on `scoped`, the +/// own-package lookup would build `node_modules/foo/bar` and miss +/// the actual `node_modules/foo+bar` directory. +#[test] +fn link_virtual_store_bins_handles_unscoped_name_with_plus() { + let tmp = tempdir().unwrap(); + let virtual_dir = tmp.path().join(".pacquet"); + let slot = virtual_dir.join("foo+bar@1.0.0"); + let modules = slot.join("node_modules"); + let pkg_dir = modules.join("foo+bar"); + let child_dir = modules.join("child"); + create_dir_all(&pkg_dir).unwrap(); + create_dir_all(&child_dir).unwrap(); + + write_file( + pkg_dir.join("package.json"), + json!({"name": "foo+bar", "version": "1.0.0"}).to_string(), + ) + .unwrap(); + write_file( + child_dir.join("package.json"), + json!({"name": "child", "version": "1.0.0", "bin": "cli.js"}).to_string(), + ) + .unwrap(); + write_file(child_dir.join("cli.js"), "#!/usr/bin/env node\n").unwrap(); + + LinkVirtualStoreBins { + virtual_store_dir: &virtual_dir, + snapshots: None, + packages: None, + package_manifests: &Default::default(), + } + .run() + .unwrap(); + + let shim = pkg_dir.join("node_modules/.bin/child"); + assert!( + shim.exists(), + "unscoped slot name containing `+` must not be split as `+`; \ + expected shim at {shim:?}", + ); +} + +/// A virtual-store slot whose `node_modules/` is missing must be skipped +/// without error. +#[test] +fn link_virtual_store_bins_skips_slot_without_node_modules() { + let tmp = tempdir().unwrap(); + let virtual_dir = tmp.path().join(".pacquet"); + create_dir_all(virtual_dir.join("incomplete@1.0.0")).unwrap(); + LinkVirtualStoreBins { + virtual_store_dir: &virtual_dir, + snapshots: None, + packages: None, + package_manifests: &Default::default(), + } + .run() + .unwrap(); +} + +/// A slot whose `node_modules/` exists but lacks the expected own-package +/// subdirectory must be skipped without error. The outer `is_dir()` +/// check on `/node_modules` passes, so the slot is processed, +/// but [`super::find_slot_own_package_dir`] returns `None` once +/// `/node_modules/` turns out to not be a directory. That +/// drives the `let-else return Ok(())` at the call site, the +/// uncovered branch real-fs tests with a well-formed slot can never +/// reach. +#[test] +fn link_virtual_store_bins_skips_slot_without_own_package_dir() { + let tmp = tempdir().unwrap(); + let virtual_dir = tmp.path().join(".pacquet"); + // Slot directory and its `node_modules/` exist, but the expected + // `/node_modules/foo` (matching the slot name `foo@1.0.0`) + // is missing, so `find_slot_own_package_dir` returns `None`. + create_dir_all(virtual_dir.join("foo@1.0.0/node_modules")).unwrap(); + LinkVirtualStoreBins { + virtual_store_dir: &virtual_dir, + snapshots: None, + packages: None, + package_manifests: &Default::default(), + } + .run() + .expect("missing own-package dir is skipped silently"); +} + +/// [`link_direct_dep_bins`] walks the project's `node_modules/` +/// symlinks and writes a shim per declared bin. End-to-end exercise of +/// the path that runs after [`crate::SymlinkDirectDependencies`]. +#[test] +fn link_direct_dep_bins_writes_shims_for_each_dep() { + let tmp = tempdir().unwrap(); + let modules = tmp.path().join("node_modules"); + let foo_dir = modules.join("foo"); + create_dir_all(&foo_dir).unwrap(); + write_file(foo_dir.join("package.json"), json!({"name": "foo", "bin": "cli.js"}).to_string()) + .unwrap(); + write_file(foo_dir.join("cli.js"), "#!/usr/bin/env node\n").unwrap(); + + link_direct_dep_bins(&modules, &["foo".to_string()]).unwrap(); + + let shim = modules.join(".bin/foo"); + assert!(shim.exists(), "shim should be created at {shim:?}"); + let body = read_to_string(&shim).unwrap(); + assert!(is_shim_pointing_at(&body, &foo_dir.join("cli.js"))); +} + +/// [`link_direct_dep_bins`] with no deps is a no-op. It must not even +/// create the `.bin` directory. Mirrors the early-return of +/// [`pacquet_cmd_shim::link_bins_of_packages`]. +#[test] +fn link_direct_dep_bins_no_op_for_empty_dep_list() { + let tmp = tempdir().unwrap(); + let modules = tmp.path().join("node_modules"); + create_dir_all(&modules).unwrap(); + link_direct_dep_bins(&modules, &[]).unwrap(); + assert!(!modules.join(".bin").exists()); +} + +/// [`link_direct_dep_bins`] resolves the dep name through the symlink +/// pacquet creates under `/`. Pin that the manifest +/// is read from the symlink's *target*, not the symlink path itself. +#[test] +fn link_direct_dep_bins_follows_symlink_to_real_package() { + let tmp = tempdir().unwrap(); + let modules = tmp.path().join("node_modules"); + create_dir_all(&modules).unwrap(); + + // The "real" package contents live elsewhere (mimics pacquet's + // virtual-store layout). + let real_pkg = tmp.path().join("virtual/foo@1.0.0/node_modules/foo"); + create_dir_all(&real_pkg).unwrap(); + write_file(real_pkg.join("package.json"), json!({"name": "foo", "bin": "cli.js"}).to_string()) + .unwrap(); + write_file(real_pkg.join("cli.js"), "#!/usr/bin/env node\n").unwrap(); + + // Use the same approach pacquet uses in production: symlink on + // Unix, junction on Windows. Plain `std::os::windows::fs::symlink_dir` + // requires the `SeCreateSymbolicLinkPrivilege` (off by default on + // CI runners), so the test would fail there even though production + // never hits that code path. + let symlink = modules.join("foo"); + pacquet_fs::symlink_dir(&real_pkg, &symlink).unwrap(); + + link_direct_dep_bins(&modules, &["foo".to_string()]).unwrap(); + + assert!(modules.join(".bin/foo").exists(), "symlinked dep must produce a shim"); +} + +/// Skip dep names whose symlink points at a non-existent target. +/// [`link_direct_dep_bins`] filters those silently because the +/// surrounding install pipeline has already populated whatever it could. +#[test] +fn link_direct_dep_bins_skips_dep_with_missing_manifest() { + let tmp = tempdir().unwrap(); + let modules = tmp.path().join("node_modules"); + create_dir_all(&modules).unwrap(); + // No `/foo` directory at all. + link_direct_dep_bins(&modules, &["foo".to_string()]).unwrap(); + assert!(!modules.join(".bin").exists()); +} + +/// [`LinkVirtualStoreBins::run_with`] propagates a non-`NotFound` +/// `read_dir` error on the virtual-store directory itself. Real fs +/// can't trigger this portably; the fake forces the +/// [`LinkVirtualStoreBinsError::ReadVirtualStore`] variant. +#[test] +fn link_virtual_store_bins_propagates_read_error_via_di() { + use pacquet_cmd_shim::{ + FsCreateDirAll, FsEnsureExecutableBits, FsReadDir, FsReadFile, FsReadHead, FsReadString, + FsSetExecutable, FsWalkFiles, FsWrite, + }; + use std::io; + + struct DenyVirtualStore; + impl FsReadDir for DenyVirtualStore { + fn read_dir(_: &Path) -> io::Result> { + Err::, _>(io::Error::from(io::ErrorKind::PermissionDenied)) + } + } + impl FsReadFile for DenyVirtualStore { + fn read_file(_: &Path) -> io::Result> { + unreachable!() + } + } + impl FsReadString for DenyVirtualStore { + fn read_to_string(_: &Path) -> io::Result { + unreachable!() + } + } + impl FsReadHead for DenyVirtualStore { + fn read_head(_: &Path, _: u64, _: &mut [u8]) -> io::Result { + unreachable!() + } + } + impl FsCreateDirAll for DenyVirtualStore { + fn create_dir_all(_: &Path) -> io::Result<()> { + unreachable!() + } + } + impl FsWrite for DenyVirtualStore { + fn write(_: &Path, _: &[u8]) -> io::Result<()> { + unreachable!() + } + } + impl FsSetExecutable for DenyVirtualStore { + fn set_executable(_: &Path) -> io::Result<()> { + unreachable!() + } + } + impl FsEnsureExecutableBits for DenyVirtualStore { + fn ensure_executable_bits(_: &Path) -> io::Result<()> { + unreachable!() + } + } + impl FsWalkFiles for DenyVirtualStore { + fn walk_files(_: &Path) -> io::Result> { + unreachable!("directories.bin not exercised by this test"); + #[expect(unreachable_code)] + Ok(empty()) + } + } + + let err = LinkVirtualStoreBins { + virtual_store_dir: Path::new("/anything"), + snapshots: None, + packages: None, + package_manifests: &Default::default(), + } + .run_with::() + .expect_err("read_dir error must propagate"); + assert!(matches!(err, LinkVirtualStoreBinsError::ReadVirtualStore { .. })); +} diff --git a/pacquet/crates/package-manager/src/symlink_direct_dependencies.rs b/pacquet/crates/package-manager/src/symlink_direct_dependencies.rs index 7c78e77f54..e349e07ec3 100644 --- a/pacquet/crates/package-manager/src/symlink_direct_dependencies.rs +++ b/pacquet/crates/package-manager/src/symlink_direct_dependencies.rs @@ -1,6 +1,7 @@ -use crate::symlink_package; +use crate::{link_direct_dep_bins, symlink_package}; use derive_more::{Display, Error}; use miette::Diagnostic; +use pacquet_cmd_shim::LinkBinsError; use pacquet_lockfile::{Lockfile, PkgName, PkgNameVerPeer, ProjectSnapshot}; use pacquet_npmrc::Npmrc; use pacquet_package_manifest::DependencyGroup; @@ -37,6 +38,9 @@ pub enum SymlinkDirectDependenciesError { )] #[diagnostic(code(pacquet_package_manager::missing_root_importer))] MissingRootImporter { root_key: String }, + + #[diagnostic(transparent)] + LinkBins(#[error(source)] LinkBinsError), } impl<'a, DependencyGroupList> SymlinkDirectDependencies<'a, DependencyGroupList> @@ -54,7 +58,7 @@ where })?; // Iterate per group so each emit can label the dependency - // with its [`DependencyType`] — pnpm's reporter renders the + // with its [`DependencyType`]. pnpm's reporter renders the // diff with that hint, so dropping it would silently // misclassify devDependencies as prod. // [`ProjectSnapshot::dependencies_by_groups`] flattens the @@ -65,22 +69,22 @@ where // for peer dependencies (they're materialised through their // host package, not directly under `node_modules/`), and // [`ProjectSnapshot::get_map_by_group`] also returns `None` - // for `Peer` so this filter is belt-and-braces — it lets + // for `Peer` so this filter is belt-and-braces. It lets // the per-group → [`DependencyType`] match below stay // exhaustive without a misleading `Peer` arm that maps to // an "absent" type. // // Dedup with a `HashSet`, first-wins. A v9 lockfile // pnpm itself wrote shouldn't list the same package across - // multiple importer sections — pnpm's resolver normalises - // (a package with `optional: true` lands in + // multiple importer sections (pnpm's resolver normalises: + // a package with `optional: true` lands in // `optionalDependencies` only). But pacquet ingests // user-supplied lockfiles, and a malformed one with the same // key in two sections would race two `symlink_package` calls // to the same `node_modules/` and emit duplicate // `pnpm:root added` events. First-wins picks up the highest- // priority group from the caller-supplied - // `dependency_groups` order — the CLI today passes + // `dependency_groups` order. The CLI today passes // `[Prod, Dev, Optional]`, matching pnpm's // dependencies-over-optional precedence. let mut seen: HashSet<&PkgName> = HashSet::new(); @@ -127,14 +131,14 @@ where DependencyGroup::Prod => DependencyType::Prod, DependencyGroup::Dev => DependencyType::Dev, DependencyGroup::Optional => DependencyType::Optional, - // Filtered upfront — see the comment on the - // `entries` builder above. + // Filtered upfront. See the comment on the `entries` + // builder above. DependencyGroup::Peer => unreachable!("peers are filtered out before this point"), }; // Pacquet's lockfile snapshot doesn't track the // npm-alias key separately from the resolved package // name at this layer, so `name` and `real_name` carry - // the same value — clone the already-built string + // the same value. Clone the already-built string // instead of formatting `name` a second time. let real_name = name_str.clone(); R::emit(&LogEvent::Root(RootLog { @@ -154,6 +158,14 @@ where })); }); + // After the symlinks exist, walk them to discover each + // direct dep's `package.json` and link declared bins into + // `/.bin`. Mirrors pnpm v11's `linkBinsOfPackages` + // call site for direct deps. + let dep_names: Vec = entries.iter().map(|(name, _, _)| name.to_string()).collect(); + link_direct_dep_bins(&config.modules_dir, &dep_names) + .map_err(SymlinkDirectDependenciesError::LinkBins)?; + Ok(()) } } diff --git a/pacquet/crates/store-dir/src/msgpackr_records.rs b/pacquet/crates/store-dir/src/msgpackr_records.rs index 3a9e2d91a4..b050190786 100644 --- a/pacquet/crates/store-dir/src/msgpackr_records.rs +++ b/pacquet/crates/store-dir/src/msgpackr_records.rs @@ -77,6 +77,7 @@ use crate::{CafsFileInfo, PackageFilesIndex, SideEffectsDiff}; use derive_more::{Display, Error}; use miette::Diagnostic; +use serde_json::Value; use smart_default::SmartDefault; use std::{collections::HashMap, rc::Rc}; @@ -611,11 +612,14 @@ fn write_str(w: &mut Vec, s: &str) { /// ## Optional-field handling /// /// - **`PackageFilesIndex`**: `algo` and `files` are always emitted; -/// `requires_build` and `side_effects` are included in the record -/// schema only when `Some`. `manifest` is always `None` in pacquet -/// today and not yet wired through; the encoder returns -/// [`EncodeError::ManifestNotSupported`] if it ever gets a `Some`, -/// which is louder than silently dropping it. +/// `requires_build`, `manifest`, and `side_effects` are included +/// in the record schema only when `Some`. The `manifest` +/// ([`serde_json::Value`]) is encoded recursively, with every +/// nested JSON object record-encoded so a pnpm reader sees them as +/// JS `Object`s (which `manifest.bin` / `manifest.directories?.bin` +/// property access can reach) rather than plain msgpack maps +/// (which msgpackr decodes as JS `Map`s, leaving those property +/// reads `undefined`). /// - **`CafsFileInfo`**: optional `checkedAt` is omitted from the /// record schema entirely when `None` rather than written as `nil`, /// so the presence of `checkedAt` determines the shape and thus @@ -643,25 +647,16 @@ pub fn encode_package_files_index(index: &PackageFilesIndex) -> Result, #[derive(Debug, Display, Error, Diagnostic)] #[non_exhaustive] pub enum EncodeError { - #[display( - "PackageFilesIndex.manifest is Some, but the msgpackr-records \ - encoder doesn't yet know how to serialize `serde_json::Value` \ - — pacquet doesn't populate this field today, so the code path \ - is unimplemented. Add manifest encoding if/when pacquet starts \ - writing bundled manifests." - )] - #[diagnostic(code(pacquet_store_dir::msgpackr_records::manifest_not_supported))] - ManifestNotSupported, - #[display( "Ran out of msgpackr record slots: encountered more than \ {max} distinct record shapes (slot range is 0x41..=0x7f). \ - This shouldn't happen for current pacquet payloads — \ - `CafsFileInfo` has at most 2 shapes and `SideEffectsDiff` \ - at most 4. Reaching this error likely means a new record \ - type was added to the encoder without bumping the shape \ - accounting, or the encoder is being reused for a schema \ - it wasn't designed for." + `CafsFileInfo` contributes at most 2 shapes and \ + `SideEffectsDiff` at most 4; the rest are allocated lazily \ + from `PackageFilesIndex.manifest`'s nested object shapes, \ + which in practice fit comfortably inside the remaining \ + range for a single tarball's manifest. Reaching this error \ + likely means the encoder is being reused for a payload it \ + wasn't designed for." )] #[diagnostic(code(pacquet_store_dir::msgpackr_records::out_of_record_slots))] OutOfRecordSlots { max: usize }, @@ -698,6 +693,17 @@ struct EncodeState { /// Same for `SideEffectsDiff`, indexed by [`side_effects_shape`] /// (4 possible values). side_effects_slots: [Option; 4], + /// Field-name vector → slot for every JSON-object shape seen so + /// far inside a manifest value. Shape keys are owned `Vec` + /// because the field names are read from a borrowed + /// `serde_json::Map` whose lifetime ends before the next encode + /// call wants the lookup. msgpackr does the equivalent thing for + /// arbitrary JS objects under `useRecords: true`; pacquet has to + /// match so a pnpm reader sees the manifest's nested objects as JS + /// `Object`s (record-decoded) rather than `Map`s (plain-msgpack- + /// decoded), which is what pnpm's bin linker reads with + /// `manifest.bin` / `manifest.directories?.bin` property access. + json_object_slots: HashMap, u8>, /// Next unused slot in the 0x41..=0x7f range. Starts above /// `PKG_FILES_INDEX_SLOT` because the top-level record always /// takes slot 0x40. @@ -736,23 +742,18 @@ fn encode_pkg_files_index_value( state: &mut EncodeState, idx: &PackageFilesIndex, ) -> Result<(), EncodeError> { - if idx.manifest.is_some() { - return Err(EncodeError::ManifestNotSupported); - } - - // Field order `[algo, requiresBuild?, files, sideEffects?]` - // matches what pnpm's msgpackr emits, as verified by the - // `decodes_requires_build_true` and `decodes_side_effects` - // fixtures captured from msgpackr 1.11.8 right here in this - // module. Optional fields are omitted from the schema when - // `None`. `manifest` stays out of the schema entirely until - // pacquet starts populating it (the `Some` check above bails - // before we get here). - let mut fields: Vec<&str> = Vec::with_capacity(4); + // Field order `[algo, requiresBuild?, manifest?, files, sideEffects?]`. + // Optional fields are omitted from the schema when `None`, matching + // msgpackr's field-omit-when-absent shape so a pnpm reader sees the + // same JS object regardless of whether pacquet or pnpm wrote the row. + let mut fields: Vec<&str> = Vec::with_capacity(5); fields.push("algo"); if idx.requires_build.is_some() { fields.push("requiresBuild"); } + if idx.manifest.is_some() { + fields.push("manifest"); + } fields.push("files"); if idx.side_effects.is_some() { fields.push("sideEffects"); @@ -765,6 +766,9 @@ fn encode_pkg_files_index_value( if let Some(rb) = idx.requires_build { write_bool(w, rb); } + if let Some(manifest) = &idx.manifest { + encode_json_value(w, state, manifest)?; + } write_map_header(w, idx.files.len()); for (name, info) in &idx.files { write_str(w, name); @@ -781,6 +785,94 @@ fn encode_pkg_files_index_value( Ok(()) } +/// Emit one JSON value as msgpack inside an active records stream. +/// Scalars use the smallest slot-safe encoding (no bare positive +/// fixints in `0x40..=0x7f`, which would otherwise be misread as +/// record-slot references — see [`write_uint`]). Arrays are plain +/// msgpack `fixarray` / `array16` / `array32`. Objects are +/// **record-encoded** via [`encode_json_object`] so that +/// `useRecords: true` decoders see them as JS `Object` rather than +/// JS `Map` — necessary for pnpm's bin linker to find +/// `manifest.bin` / `manifest.directories?.bin` via property +/// access. +fn encode_json_value( + w: &mut Vec, + state: &mut EncodeState, + value: &Value, +) -> Result<(), EncodeError> { + match value { + Value::Null => w.push(0xc0), + Value::Bool(b) => write_bool(w, *b), + Value::Number(n) => encode_json_number(w, n), + Value::String(s) => write_str(w, s), + Value::Array(arr) => { + write_array_header(w, arr.len()); + for item in arr { + encode_json_value(w, state, item)?; + } + } + Value::Object(obj) => encode_json_object(w, state, obj)?, + } + Ok(()) +} + +/// Record-encode a JSON object: allocate one slot per distinct key +/// set seen in the current stream, emit a record def the first time +/// each shape appears, and emit a bare slot byte on subsequent +/// instances of the same shape. The slot table lives on +/// [`EncodeState::json_object_slots`] so reuse compresses repeated +/// nested-object shapes (e.g. multiple `bin: { command: path }` +/// objects with the same single command name) the same way +/// msgpackr's records mode does. +/// +/// Field iteration order is the [`serde_json::Map`]'s own order; +/// pacquet builds with `serde_json/preserve_order`, so that's the +/// insertion order from parsing the original `package.json` — the +/// same order pnpm itself observes when packing the manifest. +fn encode_json_object( + w: &mut Vec, + state: &mut EncodeState, + obj: &serde_json::Map, +) -> Result<(), EncodeError> { + let fields: Vec = obj.keys().cloned().collect(); + if let Some(&slot) = state.json_object_slots.get(&fields) { + w.push(slot); + } else { + let slot = state.allocate_slot()?; + let field_refs: Vec<&str> = fields.iter().map(String::as_str).collect(); + write_record_def_header(w, slot, &field_refs); + state.json_object_slots.insert(fields, slot); + } + for value in obj.values() { + encode_json_value(w, state, value)?; + } + Ok(()) +} + +/// Encode a [`serde_json::Number`] using the smallest slot-safe +/// MessagePack form. The branch order matches what pnpm's msgpackr +/// itself picks: any integer value first (so a JSON `1.0` parsed as +/// `Number(1)` stays an integer on the wire), falling through to +/// `float 64` only when the number genuinely needs the precision. +fn encode_json_number(w: &mut Vec, n: &serde_json::Number) { + if let Some(u) = n.as_u64() { + write_uint(w, u); + return; + } + if let Some(i) = n.as_i64() { + write_int(w, i); + return; + } + if let Some(f) = n.as_f64() { + write_float64(w, f); + } + // Unreachable: a `serde_json::Number` is always one of the three + // cases above. If it isn't (a future serde_json release adds a + // new representation), the wire output would be missing a value + // for this field, which would surface as a deserialize error on + // round-trip — louder than silent corruption. +} + fn encode_cafs_file_info( w: &mut Vec, state: &mut EncodeState, @@ -924,6 +1016,32 @@ fn write_float64(w: &mut Vec, v: f64) { w.extend_from_slice(&v.to_be_bytes()); } +/// Write a signed integer in the smallest MessagePack encoding. +/// Negative values use the int 8/16/32/64 family (`0xd0..=0xd3`), +/// whose header bytes are outside the records-mode slot range so +/// they're always safe to emit. Non-negative values delegate to +/// [`write_uint`] which handles the slot-byte fixint promotion. +fn write_int(w: &mut Vec, v: i64) { + if v >= 0 { + write_uint(w, v as u64); + } else if v >= -32 { + // Negative fixint `0xe0..=0xff`; outside slot range. + w.push(v as i8 as u8); + } else if v >= i8::MIN as i64 { + w.push(0xd0); + w.push(v as i8 as u8); + } else if v >= i16::MIN as i64 { + w.push(0xd1); + w.extend_from_slice(&(v as i16).to_be_bytes()); + } else if v >= i32::MIN as i64 { + w.push(0xd2); + w.extend_from_slice(&(v as i32).to_be_bytes()); + } else { + w.push(0xd3); + w.extend_from_slice(&v.to_be_bytes()); + } +} + fn write_bool(w: &mut Vec, b: bool) { w.push(if b { 0xc3 } else { 0xc2 }); } diff --git a/pacquet/crates/store-dir/src/msgpackr_records/tests.rs b/pacquet/crates/store-dir/src/msgpackr_records/tests.rs index e765d1792c..177af3d0d4 100644 --- a/pacquet/crates/store-dir/src/msgpackr_records/tests.rs +++ b/pacquet/crates/store-dir/src/msgpackr_records/tests.rs @@ -594,19 +594,137 @@ fn allocate_slot_returns_error_past_0x7f() { assert!(matches!(err, EncodeError::OutOfRecordSlots { max: 63 }), "got {err:?}"); } +/// A `manifest: Some(_)` must round-trip through encode → +/// transcode → `rmp_serde::from_slice` unchanged. This is the basic +/// "the encoder doesn't drop or mangle JSON values" smoke test. #[test] -fn encode_rejects_manifest_some() { - // pacquet doesn't populate `manifest` today; encoding a - // `Some` value is unimplemented. Fail loudly rather than - // silently dropping it — if/when the field starts carrying - // real data, this test trips and we implement the path. - let idx = PackageFilesIndex { - manifest: Some(serde_json::json!({ "name": "x" })), +fn encode_roundtrips_simple_manifest() { + let manifest = serde_json::json!({ + "name": "pkg", + "version": "1.0.0", + "bin": "cli.js", + }); + let original = PackageFilesIndex { + manifest: Some(manifest), requires_build: None, algo: "sha512".to_string(), files: HashMap::new(), side_effects: None, }; - let err = encode_package_files_index(&idx).unwrap_err(); - assert!(matches!(err, EncodeError::ManifestNotSupported), "got {err:?}"); + assert_eq!(roundtrip(&original), original); +} + +/// Nested objects inside the manifest must be **record-encoded**, not +/// emitted as plain msgpack maps — otherwise msgpackr in +/// `useRecords: true` mode would decode them as JS `Map` and +/// `manifest.bin.tsc` (the property access pnpm's bin linker uses +/// for object-form `bin` fields) would be `undefined`. The encoder +/// signals this by emitting one `d4 72 ` def per distinct +/// nested-object shape. +#[test] +fn encode_record_encodes_nested_objects_in_manifest() { + let manifest = serde_json::json!({ + "name": "tsc", + "version": "5.0.0", + "bin": { "tsc": "bin/tsc.js", "tsserver": "bin/tsserver.js" }, + "directories": { "bin": "bin" }, + }); + let idx = PackageFilesIndex { + manifest: Some(manifest), + requires_build: None, + algo: "sha512".to_string(), + files: HashMap::new(), + side_effects: None, + }; + let bytes = encode_package_files_index(&idx).unwrap(); + + // Outer PackageFilesIndex def + nested manifest object def + nested + // `bin` object def + nested `directories` object def = 4 defs. + let record_defs = bytes.windows(2).filter(|w| *w == [0xd4, RECORD_DEF_EXT_TYPE]).count(); + assert_eq!( + record_defs, 4, + "expected 4 record defs (outer + manifest + bin + directories), got bytes {bytes:02x?}", + ); + + // Round-trip the manifest through the transcoder to verify the + // bytes a msgpackr 1.11.8 reader would consume parse cleanly. + assert_eq!(roundtrip(&idx), idx); +} + +/// Two nested objects with the **same** key set within the same +/// manifest must share a slot. Verifies record-reuse — the whole +/// point of records. Shape here is the *key set*, not just the +/// arity: `{left-pad}` and `{right-pad}` are different shapes +/// (different keys), so the test uses two objects that genuinely +/// share keys. +#[test] +fn encode_shares_slot_for_same_shaped_nested_objects() { + let manifest = serde_json::json!({ + "bin": { "cli": "bin/cli.js" }, + "directories": { "cli": "src" }, + }); + let idx = PackageFilesIndex { + manifest: Some(manifest), + requires_build: None, + algo: "sha512".to_string(), + files: HashMap::new(), + side_effects: None, + }; + let bytes = encode_package_files_index(&idx).unwrap(); + let record_defs = bytes.windows(2).filter(|w| *w == [0xd4, RECORD_DEF_EXT_TYPE]).count(); + // Outer + manifest + ONE shape `{ cli }` shared by both nested + // maps = 3 defs. If the encoder allocated a new slot per + // instance instead of sharing, this would be 4. + assert_eq!( + record_defs, 3, + "expected slot reuse for same-shape objects, got bytes {bytes:02x?}", + ); + assert_eq!(roundtrip(&idx), idx); +} + +/// All JSON value kinds (null, bool, number, string, array, object) +/// must round-trip. Numbers cover the slot-byte fixint range so the +/// `0x40..=0x7f` → `uint 8` promotion in [`write_uint`] is exercised +/// from inside the manifest encoding path too. +#[test] +fn encode_roundtrips_all_json_value_kinds() { + let manifest = serde_json::json!({ + "string": "hello", + "null": null, + "true": true, + "false": false, + "small_int": 0, + "slot_range_int": 0x42, + "big_int": 1_000_000_u64, + "negative_int": -5, + "float": 3.5, + "array": ["a", 1, null, true, [1, 2], { "k": "v" }], + "empty_array": [], + "empty_object": {}, + }); + let idx = PackageFilesIndex { + manifest: Some(manifest), + requires_build: None, + algo: "sha512".to_string(), + files: HashMap::new(), + side_effects: None, + }; + assert_eq!(roundtrip(&idx), idx); +} + +/// Manifest must round-trip alongside `requiresBuild`, `files`, and +/// `sideEffects` — the encoder has to emit all five fields in the +/// expected order and the decoder still needs to recover each. +#[test] +fn encode_roundtrips_manifest_with_other_fields() { + let mut files = HashMap::new(); + files.insert("package.json".to_string(), sample_cafs(42, false)); + let original = PackageFilesIndex { + manifest: Some(serde_json::json!({ "name": "x", "bin": "cli.js" })), + requires_build: Some(true), + algo: "sha512".to_string(), + files, + side_effects: None, + }; + assert_eq!(roundtrip(&original), original); } diff --git a/pacquet/crates/store-dir/src/store_index.rs b/pacquet/crates/store-dir/src/store_index.rs index c48d7bb765..800af81ca3 100644 --- a/pacquet/crates/store-dir/src/store_index.rs +++ b/pacquet/crates/store-dir/src/store_index.rs @@ -357,7 +357,39 @@ impl StoreIndex { &self, keys: &[String], ) -> Result, StoreIndexError> { - let mut out = HashMap::with_capacity(keys.len()); + let raw = self.get_many_raw(keys)?; + let mut out = HashMap::with_capacity(raw.len()); + for (key, bytes) in raw { + match decode_index_value(&bytes) { + Ok(entry) => { + out.insert(key, entry); + } + Err(error) => tracing::debug!( + target: "pacquet::store_index", + ?key, + ?error, + "skipping undecodable package_index row in get_many", + ), + } + } + Ok(out) + } + + /// Batched read that returns the **undecoded** value bytes for each + /// hit. Callers run `decode_index_value` themselves — either inline + /// (matching the old [`Self::get_many`] shape) or, more usefully, + /// in parallel across a rayon pool after releasing the + /// [`SharedReadonlyStoreIndex`] mutex. + /// + /// The decode is the dominant CPU cost of [`Self::get_many`] for + /// rows that carry a `manifest` field — msgpackr-records transcode + /// plus a `rmp_serde::from_slice` of a nested JSON tree per row, + /// times ~1k rows on a real lockfile. Doing that work under the + /// `SharedReadonlyStoreIndex` lock serialises N installs back to + /// one thread; doing it after the lock releases lets each prefetch + /// fan out across the rayon pool. + pub fn get_many_raw(&self, keys: &[String]) -> Result)>, StoreIndexError> { + let mut out = Vec::with_capacity(keys.len()); if keys.is_empty() { return Ok(out); } @@ -381,18 +413,8 @@ impl StoreIndex { .query_map(params, |row| Ok((row.get::<_, String>(0)?, row.get::<_, Vec>(1)?))) .map_err(|source| StoreIndexError::Read { source })?; for row in rows { - let (key, bytes) = row.map_err(|source| StoreIndexError::Read { source })?; - match decode_index_value(&bytes) { - Ok(entry) => { - out.insert(key, entry); - } - Err(error) => tracing::debug!( - target: "pacquet::store_index", - ?key, - ?error, - "skipping undecodable package_index row in get_many", - ), - } + let row = row.map_err(|source| StoreIndexError::Read { source })?; + out.push(row); } } Ok(out) @@ -508,6 +530,16 @@ impl StoreIndex { } } +/// Decode one `package_index.data` blob into a [`PackageFilesIndex`]. +/// Exposed publicly so callers reading raw rows via +/// [`StoreIndex::get_many_raw`] can run the decode outside the +/// store-index mutex (typically across a rayon pool — the decode +/// is the dominant CPU cost for rows that carry a `manifest` +/// field). +pub fn decode_package_files_index(bytes: &[u8]) -> Result { + decode_index_value(bytes) +} + fn decode_index_value(bytes: &[u8]) -> Result { // `transcode_to_plain_msgpack` tracks records-mode internally and // only reinterprets `0x40..=0x7f` as slot references after a record diff --git a/pacquet/crates/tarball/Cargo.toml b/pacquet/crates/tarball/Cargo.toml index 3309236a5a..d5445354a6 100644 --- a/pacquet/crates/tarball/Cargo.toml +++ b/pacquet/crates/tarball/Cargo.toml @@ -23,6 +23,7 @@ futures-util = { workspace = true } miette = { workspace = true } num_cpus = { workspace = true } pipe-trait = { workspace = true } +rayon = { workspace = true } reqwest = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } diff --git a/pacquet/crates/tarball/src/lib.rs b/pacquet/crates/tarball/src/lib.rs index 2f3631714c..af05c9e73f 100644 --- a/pacquet/crates/tarball/src/lib.rs +++ b/pacquet/crates/tarball/src/lib.rs @@ -20,6 +20,7 @@ use pacquet_store_dir::{ StoreIndexError, StoreIndexWriter, WriteCasFileError, store_index_key, }; use pipe_trait::Pipe; +use rayon::prelude::*; use smart_default::SmartDefault; use ssri::Integrity; use tar::Archive; @@ -294,6 +295,101 @@ fn decompress_gzip(gz_data: &[u8], unpacked_size: Option) -> Result: +/// pick the subset of `package.json` fields that downstream code +/// (bin linking, dependency resolution, build-script detection) +/// actually reads, and discard the rest. Two reasons for the +/// subset: (1) `index.db` row size on disk — a full manifest can be +/// tens of KB; (2) msgpackr-records slot pressure on the encoder +/// (record slots top out at `0x7f`, see [`pacquet_store_dir::EncodeError::OutOfRecordSlots`]). +/// +/// `scripts` is further narrowed to just the three lifecycle hooks +/// pnpm actually executes — `preinstall`, `install`, `postinstall`. +/// Other script keys (test, build, lint, etc.) are dev ergonomics +/// that the installer never invokes. +/// +/// Returns `None` when nothing survives the pick. In practice this +/// only happens for inputs that aren't a JSON object at all (the +/// type guard at the top of the function), or for objects whose +/// every field is either absent or `null`. A real `package.json` +/// from an npm tarball always carries at least `name` and `version` +/// (both kept by the pick), so the typical npm-published manifest +/// surfaces as `Some(...)`. Matches upstream's +/// `if (!result && !scripts) return undefined` shape: empty inputs +/// degrade to `None` rather than a `Some(Object({}))` that would +/// round-trip as a zero-field record def. +fn normalize_bundled_manifest(value: &serde_json::Value) -> Option { + /// Fields kept verbatim from the source manifest. + /// + /// Order matters for the on-wire byte sequence — msgpackr emits + /// fields in JS object insertion order, and pacquet's encoder + /// follows the [`serde_json::Map`] iteration order — but it + /// does *not* matter for property-access correctness on the + /// pnpm side. The order below mirrors pnpm's + /// `BUNDLED_MANIFEST_FIELDS` array so a side-by-side byte diff + /// against a pnpm-written row is shallower. + const BUNDLED_MANIFEST_FIELDS: &[&str] = &[ + "bin", + "bundledDependencies", + "bundleDependencies", + "cpu", + "dependencies", + "devDependencies", + "directories", + "engines", + "libc", + "name", + "optionalDependencies", + "os", + "peerDependencies", + "peerDependenciesMeta", + ]; + const LIFECYCLE_SCRIPTS: &[&str] = &["preinstall", "install", "postinstall"]; + + let serde_json::Value::Object(map) = value else { return None }; + let mut picked = serde_json::Map::new(); + + // pnpm emits `version` first regardless of whether it was first + // in the source object. Keep the same ordering so a byte diff + // against a pnpm-written row stays minimal. Version normalization + // via `semver.clean(...)` (pnpm only loose-cleans for the bundled + // row, not for resolution) is intentionally skipped: the inputs + // from a real npm tarball are already semver-clean in practice, + // and pulling `node-semver` into `pacquet-tarball` purely for + // this normalization would carry more risk than the deviation it + // closes. + if let Some(v) = map.get("version") + && !v.is_null() + { + picked.insert("version".to_string(), v.clone()); + } + + for &key in BUNDLED_MANIFEST_FIELDS { + if let Some(v) = map.get(key) + && !v.is_null() + { + picked.insert(key.to_string(), v.clone()); + } + } + + if let Some(serde_json::Value::Object(scripts)) = map.get("scripts") { + let mut sub = serde_json::Map::new(); + for &key in LIFECYCLE_SCRIPTS { + if let Some(s) = scripts.get(key) + && !s.is_null() + { + sub.insert(key.to_string(), s.clone()); + } + } + if !sub.is_empty() { + picked.insert("scripts".to_string(), serde_json::Value::Object(sub)); + } + } + + if picked.is_empty() { None } else { Some(serde_json::Value::Object(picked)) } +} + /// Walk a decompressed tar archive, writing each regular-file entry /// into the CAFS and returning the `{in-tarball path → CAFS path}` map /// plus the per-tarball [`PackageFilesIndex`] row to hand off to the @@ -411,6 +507,44 @@ fn extract_tarball_entries( tracing::warn!(?previous, "Duplication detected. Old entry has been ejected"); } + // Capture the parsed manifest whenever we see `package.json`. + // Mirrors pnpm's `bundledManifest` pass-through at + // [pnpm/pnpm@4750fd370c]: pnpm stuffs the narrowed manifest + // into `pkgFilesIndex.manifest` so install-side consumers + // (notably `linkBinsOfDependencies`) can avoid re-reading + // the file from disk. The [`normalize_bundled_manifest`] + // pick drops fields downstream code doesn't use, keeping + // `index.db` rows tight. + // + // **Last-entry wins.** Pnpm's [`addFilesFromTarball`] always + // overwrites `manifestBuffer = fileBuffer` per `package.json` + // entry (no `if (manifestBuffer === undefined)` guard), so + // when a tarball contains duplicate `package.json` entries + // the final one is canonical — same shape as + // `filesIndex.set(...)` which already overwrites duplicates. + // Real npm tarballs never publish multiple `package.json` + // entries, but the consistency with the `files` map is what + // matters: `manifest` and `files` must describe the same + // file. Failed JSON parses degrade the field to `None` (the + // manifest is best-effort; a corrupt `package.json` is the + // publisher's fault and downstream code can fall back to + // disk reads). + // + // [pnpm/pnpm@4750fd370c]: https://github.com/pnpm/pnpm/blob/4750fd370c/worker/src/start.ts#L218 + // [`addFilesFromTarball`]: https://github.com/pnpm/pnpm/blob/4750fd370c/store/cafs/src/addFilesFromTarball.ts#L41-L43 + if cleaned_entry_path == "package.json" { + match serde_json::from_slice::(&buffer) { + Ok(parsed) => pkg_files_idx.manifest = normalize_bundled_manifest(&parsed), + Err(error) => { + tracing::debug!( + ?error, + "package.json in tarball failed to parse as JSON; bundled manifest cleared", + ); + pkg_files_idx.manifest = None; + } + } + } + // `as_millis()` returns `u128`; narrow to `u64` to match the // store index schema — see `CafsFileInfo::checked_at` for why // `u64` is used. Using `u64::try_from` rather than `as u64` @@ -470,6 +604,35 @@ fn extract_tarball_entries( /// as a hot-path cost). pub type PrefetchedCasPaths = HashMap>>; +/// Bundled package manifests recovered from the SQLite store index, +/// keyed by the same `\t` string [`PrefetchedCasPaths`] +/// uses. Mirrors pnpm's `bundledManifest` cache in +/// [`worker/src/start.ts`](https://github.com/pnpm/pnpm/blob/4750fd370c/worker/src/start.ts#L144): +/// pnpm reads the parsed manifest out of `pkgFilesIndex.manifest` so +/// `linkBinsOfDependencies` doesn't have to re-read `package.json` +/// from disk per child. Each value is `Arc`-wrapped so multiple +/// bin-link consumers can hold the same parsed manifest without +/// deep-cloning. +/// +/// Only keys whose row carried a manifest blob appear in the map — +/// a missing key means either "row exists but has no manifest" (old +/// pacquet write, or a tarball whose `package.json` failed to +/// parse) or "package wasn't prefetched at all". Callers that need +/// to tell those apart cross-reference with [`PrefetchedCasPaths`] +/// from the same [`PrefetchResult`]. +pub type PrefetchedManifests = HashMap>; + +/// Output of [`prefetch_cas_paths`]: the warm-cache filesystem map +/// plus any bundled manifests recovered from the same `index.db` +/// rows. Bundled in a single struct so callers can destructure both +/// after one `await`, rather than the function having to thread two +/// separate spawn_blocking round-trips through. +#[derive(Default)] +pub struct PrefetchResult { + pub cas_paths: PrefetchedCasPaths, + pub manifests: PrefetchedManifests, +} + /// Batch the entire warm-cache lookup phase into one `spawn_blocking` /// task at install start: collect every row the lockfile is going to /// ask about under a single `index.lock()` round-trip, drop the lock, @@ -505,55 +668,100 @@ pub async fn prefetch_cas_paths( cache_keys: Vec, verify_store_integrity: bool, verified_files_cache: SharedVerifiedFilesCache, -) -> PrefetchedCasPaths { - let Some(index) = index else { return HashMap::new() }; +) -> PrefetchResult { + let Some(index) = index else { return PrefetchResult::default() }; if cache_keys.is_empty() { - return HashMap::new(); + return PrefetchResult::default(); } - let result = tokio::task::spawn_blocking(move || -> PrefetchedCasPaths { - // Phase 1: read every row under the mutex; drop the guard - // before running any filesystem work. One batched - // `SELECT ... WHERE key IN (?, ?, ...)` per `GET_MANY_CHUNK` - // (see `StoreIndex::get_many`) collapses what used to be N - // round-trips into one — see #294 for the cold-cache regression - // the per-key loop introduced when every key missed. - let entries: HashMap = { + let result = tokio::task::spawn_blocking(move || -> PrefetchResult { + // Phase 1: read every row's *raw bytes* under the mutex. + // Splitting raw-read from decode means the + // `SharedReadonlyStoreIndex` lock is held only for the + // SELECT loop, not for the per-row msgpackr decode — which + // is the dominant CPU cost once rows carry a `manifest` + // field (transcode + `rmp_serde::from_slice` of a nested + // JSON tree per row, times ~1k rows on a real lockfile). + // Doing the decode after the guard drops lets it fan out + // across rayon below. + // + // One batched `SELECT ... WHERE key IN (?, ?, ...)` per + // `GET_MANY_CHUNK` (see `StoreIndex::get_many_raw`) + // collapses what used to be N round-trips into one — see + // #294 for the cold-cache regression the per-key loop + // introduced when every key missed. + let raw: Vec<(String, Vec)> = { let Ok(guard) = index.lock() else { tracing::debug!( target: "pacquet::download", "store-index mutex poisoned at prefetch start; falling back to per-snapshot lookups", ); - return HashMap::new(); + return PrefetchResult::default(); }; - match guard.get_many(&cache_keys) { - Ok(map) => map, + match guard.get_many_raw(&cache_keys) { + Ok(rows) => rows, Err(error) => { tracing::debug!( target: "pacquet::download", ?error, "store-index batched read failed at prefetch start; falling back to per-snapshot lookups", ); - return HashMap::new(); + return PrefetchResult::default(); } } }; - // Phase 2: integrity-check each entry without holding the lock. - let mut out = HashMap::with_capacity(entries.len()); - for (cache_key, entry) in entries { - let verify_result = if verify_store_integrity { - pacquet_store_dir::check_pkg_files_integrity( - store_dir, - entry, - &verified_files_cache, - ) - } else { - pacquet_store_dir::build_file_maps_from_index(store_dir, entry) - }; + // Phase 2: decode each row's msgpackr-records bytes into a + // `PackageFilesIndex`, then run the integrity check. Both + // steps are per-row CPU work with no shared state, so we + // fan out across rayon. With manifests included in the + // payload, decoding 1k+ rows serially had become the + // dominant chunk of the prefetch wall (single-threaded + // `spawn_blocking`); the par-iter recovers the per-row + // parallelism the warm-batch link phase already uses. + // + // The bundled manifest is split off the decoded entry via + // `Option::take` so it travels back to the caller without + // an intermediate `Value::clone` of the JSON tree — the + // verify function only inspects `files`, never `manifest`. + let decoded: Vec<(String, Option>, pacquet_store_dir::VerifyResult)> = raw + .into_par_iter() + .filter_map(|(cache_key, bytes)| { + let mut entry: PackageFilesIndex = match pacquet_store_dir::decode_package_files_index(&bytes) { + Ok(entry) => entry, + Err(error) => { + tracing::debug!( + target: "pacquet::download", + ?cache_key, + ?error, + "skipping undecodable package_index row at prefetch", + ); + return None; + } + }; + let manifest = entry.manifest.take().map(Arc::new); + let verify_result = if verify_store_integrity { + pacquet_store_dir::check_pkg_files_integrity( + store_dir, + entry, + &verified_files_cache, + ) + } else { + pacquet_store_dir::build_file_maps_from_index(store_dir, entry) + }; + Some((cache_key, manifest, verify_result)) + }) + .collect(); + + let mut cas_paths = HashMap::with_capacity(decoded.len()); + let mut manifests = HashMap::new(); + for (cache_key, manifest, verify_result) in decoded { if verify_result.passed { - out.insert(cache_key, Arc::new(verify_result.files_map)); + if let Some(manifest) = manifest { + manifests.insert(cache_key.clone(), manifest); + } + cas_paths.insert(cache_key, Arc::new(verify_result.files_map)); } } - out + PrefetchResult { cas_paths, manifests } }) .await; result.unwrap_or_else(|error| { @@ -562,7 +770,7 @@ pub async fn prefetch_cas_paths( ?error, "store-index prefetch task failed; falling back to per-snapshot lookups", ); - HashMap::new() + PrefetchResult::default() }) } diff --git a/pacquet/crates/tarball/src/tests.rs b/pacquet/crates/tarball/src/tests.rs index 3cafe2ec9d..20359fc404 100644 --- a/pacquet/crates/tarball/src/tests.rs +++ b/pacquet/crates/tarball/src/tests.rs @@ -411,7 +411,7 @@ async fn prefetch_cas_paths_returns_hits_for_live_index_rows() { ) .await; - let map = prefetched.get(&index_key).expect("hit"); + let map = prefetched.cas_paths.get(&index_key).expect("hit"); assert_eq!(map.get("package.json"), Some(&pkg_json_path)); drop(store_dir); } @@ -467,7 +467,7 @@ async fn prefetch_cas_paths_omits_failed_integrity_entries() { .await; assert!( - !prefetched.contains_key(&index_key), + !prefetched.cas_paths.contains_key(&index_key), "row that fails integrity must not appear in prefetch result", ); drop(store_dir); @@ -523,7 +523,7 @@ async fn prefetch_cas_paths_skips_filesystem_checks_when_verify_disabled() { ) .await; - let map = prefetched.get(&index_key).expect( + let map = prefetched.cas_paths.get(&index_key).expect( "verify=false should trust the index row and surface the entry without checking disk", ); assert!(map.contains_key("package.json"));