feat(pacquet/cli): initial implementations of run, exec, dlx#11938
Conversation
Port the non-recursive path of pnpm's `exec` command (exec/commands/src/exec.ts) to pacquet. Resolves the command against the project's node_modules/.bin via the `which` crate, prepends that bin dir to PATH (mirroring makeEnv / prependDirsToPath), stamps npm_config_user_agent and PNPM_PACKAGE_NAME, supports `--shell-mode`/ `-c`, and propagates the child's exit code. The recursive / workspace-filtered variant (project graph, topological sort, --workspace-concurrency, --resume-from, --report-summary) is not ported yet because pacquet's CLI has no workspace-selection plumbing. https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
📝 WalkthroughWalkthroughAdds pacquet exec and dlx commands, refactors run to a lifecycle-aware runner using a new run_script executor, introduces shared recursive workspace execution utilities, extends Config/env/workspace settings, updates CLI wiring and docs, and adds comprehensive unit/integration tests. ChangesRun, Exec, and Dlx with lifecycle and runtime support
Sequence Diagram(s)sequenceDiagram
participant User
participant RunScript
participant Shell
participant Child
User->>RunScript: run_script(stage, script, args, env, path_prepends)
RunScript->>RunScript: build_command(script, args)
RunScript->>RunScript: select_shell_or_exec()
RunScript->>RunScript: build_env_and_extend_path()
RunScript->>Shell: spawn (env_clear + envs, PATH set)
Shell->>Child: execute
Child-->>RunScript: ExitStatus
sequenceDiagram
participant User
participant Dlx
participant Cache
participant Installer
participant Bin
User->>Dlx: dlx request (specs, flags)
Dlx->>Dlx: create cache key(specs, registries, allow_build)
Dlx->>Cache: get_valid_cache_dir(key, max_age)
alt cache hit
Cache-->>Dlx: symlink -> prepared dir
else cache miss
Dlx->>Dlx: create prepare dir (timestamp/pid)
Dlx->>Installer: install_into_cache(prepare_dir, specs)
Installer-->>Dlx: installed node_modules
Dlx->>Cache: create/update symlink -> prepare
end
Dlx->>Dlx: get_bin_name(installed package)
Dlx->>Bin: spawn bin with PATH prepended (node_modules/.bin + extra)
Bin-->>User: exit code
sequenceDiagram
participant User
participant Exec
participant Resolver
participant Child
User->>Exec: exec command [args] (-c optional)
Exec->>Exec: validate command present
Exec->>Exec: build PATH (node_modules/.bin + extra_bin_paths)
alt shell_mode
Exec->>Child: spawn shell with joined command
else direct
Exec->>Resolver: which_in(command, PATH)
Resolver-->>Exec: executable path or not found
Exec->>Child: spawn executable with env
end
Child-->>Exec: exit code
Exec->>User: exit with child's code (or 1)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11938 +/- ##
==========================================
+ Coverage 87.47% 87.50% +0.03%
==========================================
Files 261 266 +5
Lines 29660 30436 +776
==========================================
+ Hits 25944 26634 +690
- Misses 3716 3802 +86 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.09761167282,
"stddev": 0.21528239087353518,
"median": 2.03990232392,
"user": 2.60160768,
"system": 3.2013112599999998,
"min": 1.9282816414200001,
"max": 2.65241094742,
"times": [
2.0454907694199997,
2.2241851314199996,
2.10347005142,
2.00819740742,
1.9282816414200001,
2.03431387842,
1.95021186142,
2.65241094742,
1.92986372142,
2.0996913184199997
]
},
{
"command": "pacquet@main",
"mean": 2.0033734318199996,
"stddev": 0.06028868859560986,
"median": 2.00495922942,
"user": 2.6282037799999998,
"system": 3.19120586,
"min": 1.9409382194200002,
"max": 2.1149519464199997,
"times": [
2.06462328342,
2.0318302394199996,
2.1149519464199997,
1.94441570842,
1.9434838034200002,
2.00356393542,
1.9409382194200002,
2.0063545234199998,
2.0390253654199997,
1.94454729342
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.65221959192,
"stddev": 0.026959620077109142,
"median": 0.64495182832,
"user": 0.35090291999999995,
"system": 1.3131301800000001,
"min": 0.63336433132,
"max": 0.7260998803200001,
"times": [
0.7260998803200001,
0.63574566432,
0.6455742103200001,
0.65477476132,
0.6398336413200001,
0.63336433132,
0.6480191323200001,
0.64432944632,
0.63940635132,
0.6550485003200001
]
},
{
"command": "pacquet@main",
"mean": 0.6585496557199999,
"stddev": 0.03371268305193607,
"median": 0.64691931682,
"user": 0.34486301999999996,
"system": 1.32823498,
"min": 0.63409358532,
"max": 0.7506465803200001,
"times": [
0.7506465803200001,
0.64689901332,
0.6584613623200001,
0.6544736173200001,
0.6469396203200001,
0.6669069253200001,
0.64424826832,
0.63409358532,
0.64169711732,
0.64113046732
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.2918494084,
"stddev": 0.03806486714942354,
"median": 2.3016373501,
"user": 3.7394061800000005,
"system": 3.0235431999999998,
"min": 2.2086453591,
"max": 2.3464005461000004,
"times": [
2.3118675631000003,
2.2978026341000004,
2.3054720661,
2.2943624871000003,
2.2501503681000004,
2.2086453591,
2.3119519921,
2.3089115851,
2.3464005461000004,
2.2829294831
]
},
{
"command": "pacquet@main",
"mean": 2.2713282161999997,
"stddev": 0.027521155903004097,
"median": 2.2770755876,
"user": 3.7480835800000003,
"system": 2.9944721999999997,
"min": 2.2255297751,
"max": 2.3042689511,
"times": [
2.3004700981000004,
2.2933762701,
2.2652200821000004,
2.2526384941,
2.2891298421000004,
2.2255297751,
2.2402296741,
2.2534878821000004,
2.3042689511,
2.2889310931
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.4981821710999998,
"stddev": 0.014141397599215004,
"median": 1.4989812245,
"user": 1.6624747599999998,
"system": 1.8456289999999995,
"min": 1.4755808225,
"max": 1.5207381524999999,
"times": [
1.4804320605,
1.4755808225,
1.5207381524999999,
1.5008718205,
1.5160337065,
1.4885660265,
1.5023693055,
1.5033039715,
1.4968352165,
1.4970906285
]
},
{
"command": "pacquet@main",
"mean": 1.5267905821000003,
"stddev": 0.058422299867258186,
"median": 1.5075054430000001,
"user": 1.6895711599999998,
"system": 1.8512345,
"min": 1.4934742795,
"max": 1.6852487905,
"times": [
1.4989402765,
1.4961278305,
1.6852487905,
1.5529349235,
1.4934742795,
1.5113493555000002,
1.5036615305,
1.5138869775,
1.4937427045,
1.5185391525
]
}
]
} |
|
Bencher Report| Branch | pr/11938 |
| Testbed | pacquet |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,291.85 ms(-2.92%)Baseline: 2,360.87 ms | 2,833.05 ms (80.90%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,498.18 ms(-1.75%)Baseline: 1,524.79 ms | 1,829.75 ms (81.88%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,097.61 ms(+0.94%)Baseline: 2,078.05 ms | 2,493.67 ms (84.12%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 652.22 ms(-0.98%)Baseline: 658.70 ms | 790.44 ms (82.51%) |
The exec integration tests gated everything Windows can't run behind `#[cfg(unix)]`, which compiled out the only users of `std::fs` and `assert_cmd::prelude` on Windows and tripped `-D warnings` (unused imports). Per pacquet/AGENTS.md, use `#[cfg(unix)]` only where the body genuinely cannot compile on Windows (the `PermissionsExt` chmod helper), and `#[cfg_attr(target_os = "windows", ignore = "...")]` for the runtime- locked shell-mode tests so they stay compiled and visible (as ignored) in the Windows test report. Gate the `std::fs` import to unix, where its only users live. https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
CONTRIBUTING.md's writing-style rule asks for complete sentences without em-dash mid-sentence breaks. Rephrase the PATH-prepend comment accordingly. https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
The missing-command and command-not-found tests only checked for a non-zero exit, so an incidental crash would have satisfied them. Assert the actual diagnostic text (with the stderr logged first, per the style guide's logging rule for non-`assert_eq!` assertions) so the tests fail only for the right reason. Verified by temporarily breaking each path. Also move the `#[cfg(unix)]` import into its own block, as the style guide's import-organization section requires. https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
Add the settings the script-runner commands consume: enablePrePostScripts, scriptShell, nodeOptions, extraBinPaths, and dlxCacheMaxAge. Each is wired into the pnpm-workspace.yaml mapping and the PNPM_CONFIG_* env overlay, with defaults matching pnpm (dlxCacheMaxAge=1440, the rest false/None/empty). https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
Port the stdio:'inherit' path of pnpm's runLifecycleHook as run_script: it sets up node_modules/.bin on PATH and the npm_* environment via the existing build_env/extend_path/select_shell, shell-quotes appended args (shlex on POSIX, JSON.stringify on Windows), echoes `$ <script>` unless silent, and returns the script's exit status for the caller to propagate. https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
Rework `pnpm run` to match pnpm: it sets up node_modules/.bin on PATH and the npm_* env via the new executor run_script, runs pre/post scripts under enablePrePostScripts, shell-quotes trailing args, lists scripts when no name is given, filters hidden scripts, honors --if-present, and propagates the script's exit code. Thread extraBinPaths and nodeOptions into `exec`. The `/regexp/` script selector and the fuzzy "did you mean" hint are not ported (pacquet has no regex/levenshtein dependency). https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
Pull the package-add body out of AddArgs::run into a `pub(crate)` add_package function so `pacquet dlx` can drive the same install path into its cache directory. No behavior change for `pacquet add`. https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
Port the single-package path of pnpm's dlx: resolve a cache key from the package specs, install into a TTL cache directory under cacheDir/dlx via the shared add_package (anchoring Config's modules_dir there), then run the resolved bin with the cache's node_modules/.bin on PATH. Supports --package, --shell-mode, and exit-code propagation. Deferred (documented in the module): the cache key uses raw specs rather than resolved ids, the interactive approve-builds prompt, and per-axis --cpu/--os/--libc overrides. The resolve/install/run happy path needs the mocked registry and is exercised in CI; the pure helpers (cache key, cache validity, prepare dir, bin selection) are unit-tested here. https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
The exhaustive Config literal in install_package_from_registry tests needs the run/exec/dlx settings added in this branch. https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
Rename `key_ba` to `key_reversed`; the `ba` token tripped the typos spell-check (`ba` -> `by`/`be`). https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
- Use raw strings for the shlex escape and the no-placeholder display / help messages that contained `\"` escapes (perfectionist::prefer_raw_string). - Add the trailing comma to the multi-line format! in the run listing (perfectionist::macro_trailing_comma). https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
run:
- Add AllHidden error variant for when every matched script is hidden
(completes the throw_or_filter_hidden_scripts contract for the future
regex-selector path)
- Emit pnpm's ELIFECYCLE line to stderr before propagating a failing
script's exit code, matching pnpm's runLifecycleHook behavior
- Fix init_cwd: use current_dir() rather than pkg_root so INIT_CWD
reflects the user's invocation directory, not the package root
- Pass user_agent = Some("pnpm") to run_script so npm_config_user_agent
is stamped in the child environment
- Refactor specified_scripts and render_project_commands to take &Value
directly (removes the PackageManifest wrapper, enables unit testing)
- Change ALL_LIFECYCLE_SCRIPTS from [&str; 31] to &[&str] slice
- Add 8 unit tests for the private run helpers (specified_scripts,
throw_or_filter_hidden_scripts, render_project_commands)
dlx:
- Add --allow-build flag; entries are inserted into allow_builds on the
cache config so lifecycle scripts for those packages run during install
- Include named registries (not just the default) in the cache key,
matching pnpm's createCacheKey inputs
- Include allow_build list in the cache key so different allow-sets
get distinct cache directories
- Add dunce::canonicalize after creating the cache dir so a relative
cacheDir setting cannot confuse the install's workspace-root walk
- Replace the custom symlink_overwrite with pacquet_fs::force_symlink_dir
and treat its result as best-effort (a racing dlx process may win the
link; both installs are equally fresh, so run from our own prepare dir)
- Add 4 new cache-key unit tests for the expanded signature
https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
…-free The dlx cache install reuses the caller's loaded Config (already read from the filesystem at --dir, so registry/auth/named-registries are honored). But two inherited fields could leak into the throwaway install: - enable_global_virtual_store: if the caller enabled it via pnpm-workspace.yaml, the dlx install would use the global virtual store instead of the per-prepare-dir one, breaking the self-contained cache. - workspace_dir: if the caller runs inside a workspace, the throwaway install would be treated as part of that workspace. Pin both off in install_into_cache, mirroring pnpm's dlx anchoring. https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
run: - `run start` with no `start` script now requires `server.js` to exist, raising NO_SCRIPT_OR_SERVER otherwise, instead of unconditionally spawning `node server.js`. Mirrors runLifecycleHook.ts:77-82. - The `npx only-allow pnpm` no-op guard now applies to every lifecycle stage (pre/main/post), not just the main stage, matching runLifecycleHook.ts:100. dlx: - Remove the dead `config.workspace_dir = None` write: the install re-derives the workspace via find_workspace_dir(prepare_dir) and never reads config.workspace_dir, so the assignment had no effect. - Clarify why `enable_global_virtual_store` is forced off (pacquet symlinks the whole self-contained prepare dir, so its store must live inside it — a deliberate deviation from pnpm's `?? true`). - Reject bin/extra dirs containing the PATH delimiter with ERR_PNPM_BAD_PATH_DIR, matching exec and pnpm's makeEnv guard. https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
run:
- The `npx only-allow pnpm` skip now fires only when the stage has no
trailing args. pnpm appends args to the script body before the
`=== 'npx only-allow pnpm'` check (runLifecycleHook.ts:91-100), so a
main stage invoked with args is never skipped; pre/post always pass no
args, so their behavior is unchanged.
- The `run start` server.js fallback now probes `server.js` relative to
the process cwd (init_cwd), matching pnpm's `existsSync('server.js')`
(runLifecycleHook.ts:79), rather than the project dir.
- The failed-script line now reads `[ELIFECYCLE] Command failed with
exit code N.`, matching pnpm's default-reporter formatErrorSummary
(`[<code>] <message>`) instead of the legacy ` ELIFECYCLE ` form.
config:
- Stop exposing `extraBinPaths` as a `pnpm-workspace.yaml` key and a
`PNPM_CONFIG_EXTRA_BIN_PATHS` env override. pnpm never reads it from
config — it computes it (workspace root's node_modules/.bin, or empty)
at config/reader/src/index.ts:572-574. The Config field stays (run/
exec/dlx read it) and defaults empty until workspace support lands.
https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
run:
- A failing `test` script now prints "[ELIFECYCLE] Test failed. See above
for more details." instead of the generic exit-code line, matching
pnpm's reportLifecycleError test special-case (reportError.ts:371-378).
- Empty script bodies are skipped in run_stage, matching pnpm's
`!m.scripts[stage]` no-op (runLifecycleHook.ts:100) and the truthiness
gate on pre/post hooks (run.ts:403,411).
dlx:
- Build a fresh build-script allow-list for the cache install (the dlx
packages' aliases + CLI --allow-build) instead of inheriting the
caller project's allow_builds / dangerously_allow_all_builds. Mirrors
pnpm's `allowBuilds = {...resolvedPkgAliases, ...allowBuild}`
(dlx.ts:168). This also makes the cache key (which hashes only pkgs +
CLI allow_build) fully capture the effective build policy.
- Select the default bin among many by the installed package's own
`name` field (scopeless), not the dependency alias it was installed
under — they differ for aliased specs (dlx.ts:286).
- Remove the partially-installed prepare dir when the cache install
fails, instead of leaking it across failed runs (pnpm fs.rm on error).
https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
run:
- An empty `start` script ("") now falls back to `node server.js` (and
the NO_SCRIPT_OR_SERVER guard) like a missing one, since pnpm's guard
is `!m.scripts.start` and an empty string is falsy
(runLifecycleHook.ts:78). Previously the empty body was no-op'd.
- A signal-terminated script (no exit code) now prints
"[ELIFECYCLE] Command failed." with no number, matching pnpm's third
reportLifecycleError branch (reportError.ts:374-378), instead of
fabricating "exit code 1".
dlx:
- Run the resolved bin in the process working directory, matching pnpm's
`cwd: process.cwd()` (dlx.ts:231), independent of `--dir`.
https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
…pnpm For `run start` with no `start` script but a `prestart`/`poststart` and enablePrePostScripts, pnpm crashes with a TypeError (undefined.includes); pacquet runs the hooks around the `node server.js` fallback instead. Document why we don't replicate the upstream crash. https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
# Conflicts: # pacquet/crates/cli/Cargo.toml # pacquet/crates/cli/src/cli_args/add.rs # pacquet/crates/config/src/workspace_yaml.rs
…ral doc - Re-pad the [dependencies] table so every `=` aligns to the widest key (pacquet-resolving-parse-wanted-dependency, added by this PR). The merge from origin/main introduced a new dep that broke the uniform column alignment taplo enforces. - Update exec.rs's deferral doc: the workspace-projects-filter / workspace-projects-graph crates and the global --filter/--recursive flags landed on main (#11959, #12000), so the selection layer now exists; only the recursive runner is still missing. https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
The perfectionist 0.0.0-rc.17 upgrade on main flags bare `#NNN` references as ambiguous (could be issue / PR / colour / etc.). Replace the two #11959 / #12000 references with explicit pull-request URLs, matching pacquet's existing doc-comment convention. https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
There was a problem hiding this comment.
Pull request overview
This PR ports pnpm’s run, exec, and dlx commands into the Rust-based pacquet CLI, including new executor support for running lifecycle scripts in the foreground and new config plumbing to match pnpm defaults/overlays.
Changes:
- Add a new
pacquet_executor::run_scriptforeground runner (PATH +npm_*env stamping, shell selection, arg quoting) and wire it intopacquet run(single-project + recursive). - Introduce
pacquet exec(PATH prepending +whichresolution, shell mode, env stamping) andpacquet dlx(TTL cache dir install + run, isolated build allow-list, cache key). - Extend config/workspace/env overlay support for
enablePrePostScripts,scriptShell,nodeOptions,dlxCacheMaxAge, and add integration/unit coverage.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pacquet/crates/package-manager/src/install_package_from_registry/tests.rs | Updates test config constructor for newly added Config fields. |
| pacquet/crates/executor/src/run_script.rs | Adds new foreground script runner used by run (and shared behavior). |
| pacquet/crates/executor/src/run_script/tests.rs | Unit tests for quoting/build_command and env/PATH behavior in run_script. |
| pacquet/crates/executor/src/lib.rs | Exposes run_script APIs from the executor crate. |
| pacquet/crates/config/src/workspace_yaml.rs | Adds workspace-yaml parsing/apply support for new CLI/script-related config keys. |
| pacquet/crates/config/src/workspace_yaml/tests.rs | Tests parsing/apply of scriptShell + nodeOptions from workspace yaml. |
| pacquet/crates/config/src/env_overlay.rs | Adds PNPM_CONFIG_* overlay support for the new config keys. |
| pacquet/crates/config/src/lib.rs | Extends Config with enable_pre_post_scripts, script_shell, node_options, extra_bin_paths, dlx_cache_max_age. |
| pacquet/crates/config/src/pnpm_default_parity.rs | Maps new keys into the pnpm-default parity contract. |
| pacquet/crates/cli/src/cli_args/run.rs | Reworks run to list scripts, enforce hidden-script rules, implement start fallback, and run pre/main/post stages via executor. |
| pacquet/crates/cli/src/cli_args/run/recursive.rs | Hardens recursive run behavior (no-ops, recursion guard, hidden script precedence) and reuses shared run-stage machinery. |
| pacquet/crates/cli/src/cli_args/run/tests.rs | Unit tests for script selection, hidden script filtering, and script-list rendering. |
| pacquet/crates/cli/src/cli_args/exec.rs | Adds exec command implementation with PATH setup, which resolution, shell-mode option, and env stamping. |
| pacquet/crates/cli/src/cli_args/dlx.rs | Adds dlx cache/install/run implementation with TTL cache keying and isolated build allow-list behavior. |
| pacquet/crates/cli/src/cli_args/dlx/tests.rs | Unit tests for dlx cache keying, TTL validation, and bin selection behavior. |
| pacquet/crates/cli/src/cli_args/add.rs | Refactors add into reusable add_package helper for dlx installs. |
| pacquet/crates/cli/src/cli_args.rs | Wires new Exec and Dlx subcommands into top-level CLI dispatch. |
| pacquet/crates/cli/tests/run.rs | Integration coverage for single-project run behaviors (listing, pre/post, start fallback, exit code propagation, PATH behavior). |
| pacquet/crates/cli/tests/run_recursive.rs | Expands recursive-run integration coverage for PATH/local bins, hidden scripts, recursion guard, and empty-script skipping. |
| pacquet/crates/cli/tests/exec.rs | Integration tests for exec PATH behavior, shell mode, env stamping, and error surfaces. |
| pacquet/crates/cli/tests/dlx.rs | Integration test coverage for dlx missing-command and mocked-registry happy path. |
| pacquet/crates/cli/README.md | Updates CLI feature checklist for run/exec/dlx support status. |
| pacquet/crates/cli/Cargo.toml | Adds new CLI dependencies needed by exec/dlx (e.g. which, cmd shim, hashing, spec parsing). |
| Cargo.lock | Locks new transitive dependencies for the added crates/features. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pacquet/crates/cli/src/cli_args/exec.rs (1)
179-207: ⚖️ Poor tradeoffConsider extracting
prepend_dirs_to_pathto a shared helper.This function is nearly identical to the one in
dlx.rs(lines 440-468 in context snippet), differing only in the error type. Both validate delimiter presence and construct PATH the same way.A shared helper in
crates/fsor similar could take a generic error conversion, reducing duplication. Not blocking since both implementations are correct.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pacquet/crates/cli/src/cli_args/exec.rs` around lines 179 - 207, The two nearly identical functions (prepend_dirs_to_path in exec.rs and the one in dlx.rs) should be consolidated into a single shared helper in a common crate (e.g., crates/fs); create a function (e.g., prepend_dirs_to_path_common) that accepts dirs: &[PathBuf], returns Result<OsString, E> or Result<OsString, T> where the error type is generic (use a type parameter with From<ExecErrorKind> or accept a conversion closure Fn(ExecErrorKind)->E) so callers can map to their specific error enums, move the delimiter validation and PATH construction there, and then replace the local prepend_dirs_to_path implementations in exec.rs and dlx.rs with thin wrappers that call the shared helper and convert/map the error into their respective error types.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pacquet/crates/cli/src/cli_args/dlx.rs`:
- Around line 281-288: The shell-mode branch in dlx.rs builds a single string
with joined.join(" ") which allows whitespace/metacharacters to be reinterpreted
by the shell; instead shell-escape each argument token (the bin_name and every
element of args) before joining so the shell receives a safely quoted command
like pnpm does. Update the code that builds joined (used in the
Command::new(&shell.program) invocation) to map each token through a
platform-appropriate shell-escaping function (create or reuse a helper to escape
for sh on Unix and CMD/PowerShell on Windows) and then join the escaped tokens
with a space, preserving the rest of the logic that uses shell.args and
arg(...).
- Around line 356-360: The cache-hit branch currently uses fs::canonicalize to
resolve the symlink target (after calling fs::symlink_metadata on cache_link and
checking meta.file_type().is_symlink()), which produces a different path format
than the fresh-install branch; change the resolution to use dunce::canonicalize
for the same variable (target) so both branches produce the same canonical path
form fed into bins_dir/PATH; update the call site that assigns target (replacing
fs::canonicalize(cache_link).ok()? with dunce::canonicalize(cache_link).ok()?)
and keep the same error handling semantics.
In `@pacquet/crates/config/src/lib.rs`:
- Around line 959-966: The extra_bin_paths field is never populated, so update
the code that builds/loading Config (specifically Config::current) to compute
and set extra_bin_paths once the workspace root is known: detect the workspace
root path, append its node_modules/.bin
(workspace_root.join("node_modules").join(".bin")) to extra_bin_paths when
inside a workspace and leave it empty otherwise, ensuring Config::current
returns this populated Vec<PathBuf> so callers (e.g., exec.rs and run_script.rs)
observe the same workspace-root bin behavior as pnpm.
In `@pacquet/crates/executor/src/run_script.rs`:
- Around line 135-142: run_script() currently always uses .arg(&command) which
ignores shell.windows_verbatim_args on Windows; change the code that builds the
Command (the Command::new(&shell.program) ... chain) to push the script argument
via the same helper used in lifecycle.rs — call push_script_arg(&mut cmd,
&command, shell.windows_verbatim_args) (or the equivalent push_script_arg
invocation used there) instead of .arg(&command), so that when
shell.windows_verbatim_args is true it uses CommandExt::raw_arg(script); also
ensure the CommandExt trait is in scope (use
std::os::windows::process::CommandExt) and match the argument-pushing logic
exactly as in lifecycle.rs to preserve Windows quoting/escaping behavior.
---
Nitpick comments:
In `@pacquet/crates/cli/src/cli_args/exec.rs`:
- Around line 179-207: The two nearly identical functions (prepend_dirs_to_path
in exec.rs and the one in dlx.rs) should be consolidated into a single shared
helper in a common crate (e.g., crates/fs); create a function (e.g.,
prepend_dirs_to_path_common) that accepts dirs: &[PathBuf], returns
Result<OsString, E> or Result<OsString, T> where the error type is generic (use
a type parameter with From<ExecErrorKind> or accept a conversion closure
Fn(ExecErrorKind)->E) so callers can map to their specific error enums, move the
delimiter validation and PATH construction there, and then replace the local
prepend_dirs_to_path implementations in exec.rs and dlx.rs with thin wrappers
that call the shared helper and convert/map the error into their respective
error types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ed38a995-3b8f-4eda-b595-888b3cc5ad27
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
pacquet/crates/cli/Cargo.tomlpacquet/crates/cli/README.mdpacquet/crates/cli/src/cli_args.rspacquet/crates/cli/src/cli_args/add.rspacquet/crates/cli/src/cli_args/dlx.rspacquet/crates/cli/src/cli_args/dlx/tests.rspacquet/crates/cli/src/cli_args/exec.rspacquet/crates/cli/src/cli_args/run.rspacquet/crates/cli/src/cli_args/run/recursive.rspacquet/crates/cli/src/cli_args/run/tests.rspacquet/crates/cli/tests/dlx.rspacquet/crates/cli/tests/exec.rspacquet/crates/cli/tests/run.rspacquet/crates/cli/tests/run_recursive.rspacquet/crates/config/src/env_overlay.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/pnpm_default_parity.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/executor/src/lib.rspacquet/crates/executor/src/run_script.rspacquet/crates/executor/src/run_script/tests.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🧰 Additional context used
📓 Path-based instructions (3)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: Log emissions are part of matching pnpm — when porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/executor/src/lib.rspacquet/crates/cli/src/cli_args/add.rspacquet/crates/config/src/env_overlay.rspacquet/crates/cli/tests/dlx.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/cli/src/cli_args/run/tests.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/cli/src/cli_args.rspacquet/crates/config/src/lib.rspacquet/crates/cli/src/cli_args/run/recursive.rspacquet/crates/cli/src/cli_args/exec.rspacquet/crates/config/src/pnpm_default_parity.rspacquet/crates/cli/tests/exec.rspacquet/crates/cli/src/cli_args/dlx.rspacquet/crates/executor/src/run_script.rspacquet/crates/cli/tests/run.rspacquet/crates/cli/tests/run_recursive.rspacquet/crates/executor/src/run_script/tests.rspacquet/crates/cli/src/cli_args/dlx/tests.rspacquet/crates/cli/src/cli_args/run.rs
pacquet/**/Cargo.toml
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/Cargo.toml: Check whether the workspace already depends on something suitable in[workspace.dependencies]in the rootCargo.tomlbefore adding a new dependency
Keep dependencies at the right level — add a new dependency to the specific crate that needs it, not to the workspace root or shared crate unless multiple crates depend on it
Files:
pacquet/crates/cli/Cargo.toml
pacquet/**/tests/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/tests/**/*.rs: Prefer real fixtures over dependency-injection seams — usetempfile::TempDir, the mocked registry, or integration tests spawning the actual binary for happy paths and most error paths; use the DI seam only for filesystem error kinds, deterministic time, shared process-global state, or external-service happy paths
Follow test-logging guidance — log before non-assert_eq!assertions,dbg!complex structures, skip logging for simple scalarassert_eq!
Tests must not be tolerant of missing build/runtime environment by silently returning early when a tool isn't found — let the test panic when a required tool is absent
Prefer#[cfg_attr(target_os = "windows", ignore = "...")](or matching#[cfg(unix)]gates) over runtime probe-and-skip helpers for platform-locked tools
Port relevant pnpm tests to Rust tests whenever they translate when porting behavior from pnpm
Use snapshot tests withinstaand carefully review diffs when intentional changes alter snapshots; accept withcargo insta reviewonly after careful review
Tests that need the mocked registry should startpnprthroughpacquet-testing-utils;cargo test/cargo nextest runshould not require a separatejust registry-mock launchstep
Files:
pacquet/crates/cli/tests/dlx.rspacquet/crates/cli/tests/exec.rspacquet/crates/cli/tests/run.rspacquet/crates/cli/tests/run_recursive.rs
🧠 Learnings (3)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/executor/src/lib.rspacquet/crates/cli/src/cli_args/add.rspacquet/crates/config/src/env_overlay.rspacquet/crates/cli/tests/dlx.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/cli/src/cli_args/run/tests.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/cli/src/cli_args.rspacquet/crates/config/src/lib.rspacquet/crates/cli/src/cli_args/run/recursive.rspacquet/crates/cli/src/cli_args/exec.rspacquet/crates/config/src/pnpm_default_parity.rspacquet/crates/cli/tests/exec.rspacquet/crates/cli/src/cli_args/dlx.rspacquet/crates/executor/src/run_script.rspacquet/crates/cli/tests/run.rspacquet/crates/cli/tests/run_recursive.rspacquet/crates/executor/src/run_script/tests.rspacquet/crates/cli/src/cli_args/dlx/tests.rspacquet/crates/cli/src/cli_args/run.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/executor/src/lib.rspacquet/crates/cli/src/cli_args/add.rspacquet/crates/config/src/env_overlay.rspacquet/crates/cli/tests/dlx.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/cli/src/cli_args/run/tests.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/cli/src/cli_args.rspacquet/crates/config/src/lib.rspacquet/crates/cli/src/cli_args/run/recursive.rspacquet/crates/cli/src/cli_args/exec.rspacquet/crates/config/src/pnpm_default_parity.rspacquet/crates/cli/tests/exec.rspacquet/crates/cli/src/cli_args/dlx.rspacquet/crates/executor/src/run_script.rspacquet/crates/cli/tests/run.rspacquet/crates/cli/tests/run_recursive.rspacquet/crates/executor/src/run_script/tests.rspacquet/crates/cli/src/cli_args/dlx/tests.rspacquet/crates/cli/src/cli_args/run.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.
Applied to files:
pacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/executor/src/lib.rspacquet/crates/cli/src/cli_args/add.rspacquet/crates/config/src/env_overlay.rspacquet/crates/cli/tests/dlx.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/cli/src/cli_args/run/tests.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/cli/src/cli_args.rspacquet/crates/config/src/lib.rspacquet/crates/cli/src/cli_args/run/recursive.rspacquet/crates/cli/src/cli_args/exec.rspacquet/crates/config/src/pnpm_default_parity.rspacquet/crates/cli/tests/exec.rspacquet/crates/cli/src/cli_args/dlx.rspacquet/crates/executor/src/run_script.rspacquet/crates/cli/tests/run.rspacquet/crates/cli/tests/run_recursive.rspacquet/crates/executor/src/run_script/tests.rspacquet/crates/cli/src/cli_args/dlx/tests.rspacquet/crates/cli/src/cli_args/run.rs
🔇 Additional comments (24)
pacquet/crates/cli/src/cli_args/exec.rs (5)
1-11: LGTM!
13-38: LGTM!
72-155: LGTM!
157-168: LGTM!
59-61: ⚡ Quick winUpdate CommandNotFound to keep
ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL(even for non-recursive exec)pnpm assigns
err['code'] = 'ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL'inexec/commands/src/exec.tswhenopts.bailand the error code doesn’t already start withERR_PNPM_—this is not conditioned onopts.recursive. The recursive e2e test asserts this code; the non-recursive command-not-found e2e tests validatemessage/hint(without assertingerror.code), but the handler logic still sets the same code.pacquet/crates/cli/tests/exec.rs (4)
1-15: LGTM!
17-56: LGTM!
58-96: LGTM!
98-173: LGTM!pacquet/crates/cli/src/cli_args/add.rs (1)
97-115: LGTM!Also applies to: 118-161
pacquet/crates/cli/src/cli_args/dlx.rs (1)
311-468: LGTM!pacquet/crates/cli/src/cli_args/dlx/tests.rs (1)
1-181: LGTM!pacquet/crates/cli/tests/dlx.rs (1)
1-54: LGTM!pacquet/crates/cli/src/cli_args.rs (2)
2-3: LGTM!Also applies to: 16-17, 127-130
283-290: LGTM!pacquet/crates/cli/README.md (1)
63-100: LGTM!pacquet/crates/cli/Cargo.toml (1)
53-53: ⚡ Quick win
whichworkspace dependency exists
which = { workspace = true }is valid because the rootCargo.toml’s[workspace.dependencies]includeswhich = { version = "8.0.2" }.pacquet/crates/config/src/workspace_yaml/tests.rs (1)
1301-1321: LGTM!pacquet/crates/executor/src/run_script/tests.rs (1)
1-98: LGTM!pacquet/crates/cli/src/cli_args/run.rs (1)
1-507: LGTM!pacquet/crates/cli/src/cli_args/run/tests.rs (1)
1-62: LGTM!pacquet/crates/cli/tests/run.rs (1)
7-386: LGTM!pacquet/crates/cli/src/cli_args/run/recursive.rs (1)
19-402: LGTM!pacquet/crates/cli/tests/run_recursive.rs (1)
10-572: LGTM!
| /// `extraBinPaths`: directories prepended to `PATH` (after the | ||
| /// project's own `node_modules/.bin`) when running scripts and | ||
| /// `pnpm exec`. pnpm computes this as the workspace root's | ||
| /// `node_modules/.bin` inside a workspace and leaves it empty | ||
| /// otherwise. pacquet defaults it empty until workspace support | ||
| /// lands. Mirrors pnpm's | ||
| /// [`Config.extraBinPaths`](https://github.com/pnpm/pnpm/blob/3b62f9da31/config/reader/src/Config.ts#L72). | ||
| pub extra_bin_paths: Vec<PathBuf>, |
There was a problem hiding this comment.
extra_bin_paths is introduced here but never derived anywhere in the reviewed config path.
This field is not loadable from yaml/env, and Config::current never populates it, so the reviewed consumers in pacquet/crates/cli/src/cli_args/exec.rs and pacquet/crates/executor/src/run_script.rs will always see []. That drops the workspace-root node_modules/.bin from PATH, which breaks pnpm parity for workspace run/exec flows that rely on root bins. Please derive it once the workspace root is known instead of leaving it permanently empty.
Based on learnings: only match pnpm’s behavior exactly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pacquet/crates/config/src/lib.rs` around lines 959 - 966, The extra_bin_paths
field is never populated, so update the code that builds/loading Config
(specifically Config::current) to compute and set extra_bin_paths once the
workspace root is known: detect the workspace root path, append its
node_modules/.bin (workspace_root.join("node_modules").join(".bin")) to
extra_bin_paths when inside a workspace and leave it empty otherwise, ensuring
Config::current returns this populated Vec<PathBuf> so callers (e.g., exec.rs
and run_script.rs) observe the same workspace-root bin behavior as pnpm.
There was a problem hiding this comment.
This is an intentional, documented deferral rather than an oversight. extraBinPaths in pnpm is the workspace-root node_modules/.bin, which is only non-empty inside a workspace; pnpm leaves it empty otherwise. pacquet's workspace fan-out for run/exec isn't implemented yet (it's in the PR's Deferred section), so there's no built workspace-root bin dir to point at — populating it now would put a non-existent path on PATH. It'll be derived once workspace support lands, matching pnpm's Config.extraBinPaths. Keeping this thread open as the tracking anchor.
— Claude Code (agent)
Generated by Claude Code
There was a problem hiding this comment.
@KSXGitHub, understood — the field doc comment already calls this out and the PR's Deferred section locks it in as a future work item. Populating a not-yet-resolved workspace root would be worse than leaving it empty, so the current state is correct. Thanks for the clarification.
Would you like me to open a GitHub issue to track the derivation of extra_bin_paths once workspace fan-out support lands, so it doesn't get lost?
✏️ Learnings added
Learnt from: KSXGitHub
Repo: pnpm/pnpm PR: 11938
File: pacquet/crates/config/src/lib.rs:959-966
Timestamp: 2026-06-02T14:39:19.809Z
Learning: In the pnpm/pnpm pacquet Rust port (`pacquet/crates/config/src/lib.rs`), `Config.extra_bin_paths` is intentionally left empty (`Vec::new()`) until workspace fan-out support for `run`/`exec` is implemented. It mirrors pnpm's `Config.extraBinPaths` (the workspace-root `node_modules/.bin`), which is also empty outside a workspace. Populating it before the workspace-root is resolved would put a non-existent path on `PATH`, so it should only be derived once workspace support lands. Do not flag this as a bug or missing derivation in reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
…script run_script appended the script body with a plain `.arg()`, ignoring the `windows_verbatim_args` flag that select_shell sets for the default `cmd /d /s /c` path. On Windows that escapes embedded quoting and breaks commands like `node -e "..."`. Route the append through the existing `push_script_arg` helper (shared with the lifecycle runner), which uses `CommandExt::raw_arg` when verbatim args are required.
The fresh-install branch canonicalizes the prepare dir with `dunce::canonicalize`, but `get_valid_cache_dir` resolved the cache symlink with `fs::canonicalize`. On Windows the latter yields a `\\?\` verbatim path, so a cache hit fed a different `node_modules/.bin` string into PATH than a fresh install. Use `dunce::canonicalize` in both branches.
Port the recursive path of pnpm's exec handler: `pacquet -r exec <cmd>` runs the command in every workspace project, topologically sorted and sequential, with --resume-from / --report-summary / --no-bail. Extract the shared graph/summary machinery (sort_projects, resume-from chunking, pnpm-exec-summary.json writing, the GraphPkg adapter, ExecutionStatus) out of run/recursive.rs into a shared cli_args::recursive module that both run and exec use. Matches pnpm's error codes (ERR_PNPM_RECURSIVE_EXEC_NO_PACKAGE, ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL, ERR_PNPM_RECURSIVE_FAIL). --filter and --workspace-concurrency stay deferred, as they are for recursive run.
Add the per-axis `--cpu` / `--os` / `--libc` flags to `pacquet dlx`, reusing `SupportedArchitecturesArgs::apply_to` to override the dlx install's `supportedArchitectures` (mirrors pnpm's overrideSupportedArchitecturesWithCLI). The flags take a single comma-separable value per occurrence rather than the greedy num_args=1.. shape install/add use, so dlx's trailing command positional is not swallowed — covered by a parse test.
|
@coderabbitai full review. |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pacquet/crates/cli/src/cli_args/dlx.rs`:
- Around line 245-249: The dlx cache key generation is missing the effective
architecture overrides so different CLI overrides (e.g., via
supported_architectures.apply_to) can produce identical cache keys; update the
dlx install cache key builder to include the post-override value
(config.supported_architectures after calling supported_architectures.apply_to)
— include a stable representation of that structure (e.g., cpu/os/libc fields or
a canonical string/hash) in the hash input used when constructing the dlx cache
key so --cpu/--os/--libc produce distinct cache entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a67da9fc-b954-4fc1-8e60-2e2490dfbd8d
📒 Files selected for processing (12)
pacquet/crates/cli/README.mdpacquet/crates/cli/src/cli_args.rspacquet/crates/cli/src/cli_args/dlx.rspacquet/crates/cli/src/cli_args/dlx/tests.rspacquet/crates/cli/src/cli_args/exec.rspacquet/crates/cli/src/cli_args/exec/recursive.rspacquet/crates/cli/src/cli_args/recursive.rspacquet/crates/cli/src/cli_args/run/recursive.rspacquet/crates/cli/tests/exec_recursive.rspacquet/crates/cli/tests/run.rspacquet/crates/executor/src/lifecycle.rspacquet/crates/executor/src/run_script.rs
✅ Files skipped from review due to trivial changes (2)
- pacquet/crates/executor/src/lifecycle.rs
- pacquet/crates/cli/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- pacquet/crates/executor/src/run_script.rs
- pacquet/crates/cli/tests/run.rs
- pacquet/crates/cli/src/cli_args/dlx/tests.rs
- pacquet/crates/cli/src/cli_args/run/recursive.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: ubuntu-latest / Node.js 24 / Test
🧰 Additional context used
📓 Path-based instructions (2)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: Log emissions are part of matching pnpm — when porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/cli/src/cli_args/exec/recursive.rspacquet/crates/cli/tests/exec_recursive.rspacquet/crates/cli/src/cli_args.rspacquet/crates/cli/src/cli_args/dlx.rspacquet/crates/cli/src/cli_args/recursive.rspacquet/crates/cli/src/cli_args/exec.rs
pacquet/**/tests/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/tests/**/*.rs: Prefer real fixtures over dependency-injection seams — usetempfile::TempDir, the mocked registry, or integration tests spawning the actual binary for happy paths and most error paths; use the DI seam only for filesystem error kinds, deterministic time, shared process-global state, or external-service happy paths
Follow test-logging guidance — log before non-assert_eq!assertions,dbg!complex structures, skip logging for simple scalarassert_eq!
Tests must not be tolerant of missing build/runtime environment by silently returning early when a tool isn't found — let the test panic when a required tool is absent
Prefer#[cfg_attr(target_os = "windows", ignore = "...")](or matching#[cfg(unix)]gates) over runtime probe-and-skip helpers for platform-locked tools
Port relevant pnpm tests to Rust tests whenever they translate when porting behavior from pnpm
Use snapshot tests withinstaand carefully review diffs when intentional changes alter snapshots; accept withcargo insta reviewonly after careful review
Tests that need the mocked registry should startpnprthroughpacquet-testing-utils;cargo test/cargo nextest runshould not require a separatejust registry-mock launchstep
Files:
pacquet/crates/cli/tests/exec_recursive.rs
🧠 Learnings (3)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/cli/src/cli_args/exec/recursive.rspacquet/crates/cli/tests/exec_recursive.rspacquet/crates/cli/src/cli_args.rspacquet/crates/cli/src/cli_args/dlx.rspacquet/crates/cli/src/cli_args/recursive.rspacquet/crates/cli/src/cli_args/exec.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/cli/src/cli_args/exec/recursive.rspacquet/crates/cli/tests/exec_recursive.rspacquet/crates/cli/src/cli_args.rspacquet/crates/cli/src/cli_args/dlx.rspacquet/crates/cli/src/cli_args/recursive.rspacquet/crates/cli/src/cli_args/exec.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.
Applied to files:
pacquet/crates/cli/src/cli_args/exec/recursive.rspacquet/crates/cli/tests/exec_recursive.rspacquet/crates/cli/src/cli_args.rspacquet/crates/cli/src/cli_args/dlx.rspacquet/crates/cli/src/cli_args/recursive.rspacquet/crates/cli/src/cli_args/exec.rs
🔇 Additional comments (8)
pacquet/crates/cli/src/cli_args/recursive.rs (2)
48-120: LGTM!
144-162: LGTM!pacquet/crates/cli/src/cli_args/exec.rs (3)
91-100: LGTM!Also applies to: 115-123
133-188: LGTM!
72-74: ⚡ Quick win
CommandNotFoundusingERR_PNPM_RECURSIVE_EXEC_FIRST_FAILmatches pnpm for non-recursiveexec
pnpm’sexec/commands/src/exec.tssetserr['code'] = 'ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL'in theexecfailure handler whenever the error doesn’t already have anERR_PNPM_*code, without gating this assignment onopts.recursive(only the non-recursive numericexitCodeearly-return is exception). So single-projectpnpm exec <missing>uses the same diagnostic code, making this mapping consistent.> Likely an incorrect or invalid review comment.pacquet/crates/cli/src/cli_args/exec/recursive.rs (1)
63-140: LGTM!pacquet/crates/cli/tests/exec_recursive.rs (1)
14-147: LGTM!pacquet/crates/cli/src/cli_args.rs (1)
287-293: LGTM!
exec's `--shell-mode` appended the joined command with a plain `.arg()`, ignoring the `windows_verbatim_args` flag that select_shell sets for the default `cmd /d /s /c` path — so on Windows embedded quoting (e.g. `node -e "..."`) was escaped, diverging from execa's `windowsVerbatimArguments`. Route the append through the shared `push_script_arg` helper (now exported from pacquet-executor). Covered by a cross-platform regression test.
Two Windows/correctness fixes for dlx: - `run_bin`'s shell mode now appends the joined command through `push_script_arg`, honoring `windows_verbatim_args` like exec/run. - Fold the effective `--cpu`/`--os`/`--libc` overrides into the dlx cache key (per-axis cpu/libc/os, deduped+sorted), mirroring pnpm's `createCacheKey`. Without this, `--cpu arm64` and `--cpu x64` collided on the same cache entry and could reuse the wrong platform-specific install.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pacquet/crates/cli/src/cli_args/dlx.rs (1)
245-249:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftArchitecture overrides must be included in the cache key.
The effective
supported_architectures(after applying CLI overrides) changes the installednode_modulescontents, butcreate_cache_keyat Line 171 only hashes packages, registries, andallow_build. A run with--cpu arm64can reuse the same cached install as--cpu x64, returning the wrong optional dependency set until the cache expires.🔧 Recommended fix
Include the post-override
supported_architecturesin the cache key:let cache_dir = config.cache_dir.clone(); let max_age = config.dlx_cache_max_age; let registries = build_registries_map(config); -let cache_key = create_cache_key(&pkgs, ®istries, &allow_build); let extra_bin_paths = config.extra_bin_paths.clone(); + +// Apply architecture overrides before computing the cache key. +let supported_architectures = + supported_architectures.apply_to(config.supported_architectures.clone()); +let cache_key = create_cache_key(&pkgs, ®istries, &allow_build, supported_architectures.as_ref());Then update
create_cache_keysignature and implementation:fn create_cache_key( pkgs: &[String], registries: &BTreeMap<String, String>, allow_build: &[String], + supported_architectures: Option<&pacquet_package_is_installable::SupportedArchitectures>, ) -> String { let mut sorted: Vec<&str> = pkgs.iter().map(String::as_str).collect(); sorted.sort_unstable(); let registry_pairs: Vec<(&str, &str)> = registries.iter().map(|(k, v)| (k.as_str(), v.as_str())).collect(); let mut args = vec![json!(sorted), json!(registry_pairs)]; if !allow_build.is_empty() { let mut sorted_allow: Vec<&str> = allow_build.iter().map(String::as_str).collect(); sorted_allow.sort_unstable(); args.push(json!({ "allowBuild": sorted_allow })); } + if let Some(arch) = supported_architectures { + args.push(json!({ + "cpu": arch.cpu, + "os": arch.os, + "libc": arch.libc, + })); + } create_short_hash(&serde_json::to_string(&args).expect("serialize cache key inputs")) }Also update the test calls to
create_cache_keyindlx/tests.rsto passNonefor the new parameter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pacquet/crates/cli/src/cli_args/dlx.rs` around lines 245 - 249, The effective supported_architectures (after applying CLI overrides via supported_architectures.apply_to(...)) must be included in the cache key; modify create_cache_key to accept an additional parameter (e.g., Option<SupportedArchitectures> or the concrete supported_architectures value) and incorporate it into the hash computation alongside packages, registries, and allow_build; update all callers (including tests in dlx/tests.rs) to pass the post-override supported_architectures (or None where appropriate) so that a run with --cpu/--os/--libc produces a distinct cache key reflecting the overridden architectures.
🧹 Nitpick comments (1)
pacquet/crates/cli/tests/exec_recursive.rs (1)
82-92: 💤 Low valueClarify that
trueis the executed command, not a flag value.
--report-summaryis a boolean flag, so the trailingtrueis parsed as the command to run (the POSIXtruebinary) rather than an argument to--report-summary. This is correct, but it reads like--report-summary=true. A one-line comment (or switching to an explicit command liketouch ran.txt) would prevent a future maintainer from "fixing" it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pacquet/crates/cli/tests/exec_recursive.rs` around lines 82 - 92, Clarify the recursive exec test setup in exec_recursive.rs so it doesn’t look like `--report-summary` takes a value: the trailing `true` is the command being executed, not a boolean flag argument. Add a brief inline comment near the `pacquet.with_arg("-r").with_arg("exec")...` chain, or replace `true` with a more explicit no-op command like `touch`/a visible command name, so future readers understand the intent and do not “fix” the invocation incorrectly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pacquet/crates/config/src/workspace_yaml.rs`:
- Around line 242-252: Change the two fields that currently lose the “explicit
null” distinction (script_shell and node_options) to a tri-state type
(Option<Option<String>>) wherever they appear (e.g., in the
WorkspaceSettings/WorkspaceYaml structs) so you can represent missing, explicit
null, and a string; then update the apply_to() implementation to match pnpm:
when the outer Option is None do nothing (inherit), when Some(None) write None
to clear the inherited value, and when Some(Some(s)) write the given string.
Apply the same type and logic to the other occurrence of these fields mentioned
in the review.
In `@pacquet/crates/executor/src/run_script.rs`:
- Around line 96-112: EnvOptions currently sets script to the rendered
`&command` which causes npm_lifecycle_script to differ from package.json when
forwarded args are present; change EnvOptions.script to use the raw
`opts.script` (e.g., script: opts.script) so the lifecycle env is stamped with
the original package.json script, while keeping the rendered `command` for the
actual shell spawn and leaving the call to build_env(&env_opts, ...) unchanged.
---
Duplicate comments:
In `@pacquet/crates/cli/src/cli_args/dlx.rs`:
- Around line 245-249: The effective supported_architectures (after applying CLI
overrides via supported_architectures.apply_to(...)) must be included in the
cache key; modify create_cache_key to accept an additional parameter (e.g.,
Option<SupportedArchitectures> or the concrete supported_architectures value)
and incorporate it into the hash computation alongside packages, registries, and
allow_build; update all callers (including tests in dlx/tests.rs) to pass the
post-override supported_architectures (or None where appropriate) so that a run
with --cpu/--os/--libc produces a distinct cache key reflecting the overridden
architectures.
---
Nitpick comments:
In `@pacquet/crates/cli/tests/exec_recursive.rs`:
- Around line 82-92: Clarify the recursive exec test setup in exec_recursive.rs
so it doesn’t look like `--report-summary` takes a value: the trailing `true` is
the command being executed, not a boolean flag argument. Add a brief inline
comment near the `pacquet.with_arg("-r").with_arg("exec")...` chain, or replace
`true` with a more explicit no-op command like `touch`/a visible command name,
so future readers understand the intent and do not “fix” the invocation
incorrectly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 34afa7a7-15fa-4568-9a88-baa0af673453
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
pacquet/crates/cli/Cargo.tomlpacquet/crates/cli/README.mdpacquet/crates/cli/src/cli_args.rspacquet/crates/cli/src/cli_args/add.rspacquet/crates/cli/src/cli_args/dlx.rspacquet/crates/cli/src/cli_args/dlx/tests.rspacquet/crates/cli/src/cli_args/exec.rspacquet/crates/cli/src/cli_args/exec/recursive.rspacquet/crates/cli/src/cli_args/recursive.rspacquet/crates/cli/src/cli_args/run.rspacquet/crates/cli/src/cli_args/run/recursive.rspacquet/crates/cli/src/cli_args/run/tests.rspacquet/crates/cli/tests/dlx.rspacquet/crates/cli/tests/exec.rspacquet/crates/cli/tests/exec_recursive.rspacquet/crates/cli/tests/run.rspacquet/crates/cli/tests/run_recursive.rspacquet/crates/config/src/env_overlay.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/pnpm_default_parity.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/executor/src/lib.rspacquet/crates/executor/src/lifecycle.rspacquet/crates/executor/src/run_script.rspacquet/crates/executor/src/run_script/tests.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: ubuntu-latest / Node.js 22.13.0 / Test
- GitHub Check: ubuntu-latest / Node.js 26.3.0 / Test
- GitHub Check: windows-latest / Node.js 22.13.0 / Test
🧰 Additional context used
📓 Path-based instructions (3)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: Log emissions are part of matching pnpm — when porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/executor/src/lifecycle.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/config/src/env_overlay.rspacquet/crates/executor/src/lib.rspacquet/crates/cli/src/cli_args/run/tests.rspacquet/crates/config/src/pnpm_default_parity.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/cli/tests/dlx.rspacquet/crates/cli/tests/exec.rspacquet/crates/executor/src/run_script.rspacquet/crates/cli/src/cli_args/dlx/tests.rspacquet/crates/executor/src/run_script/tests.rspacquet/crates/cli/src/cli_args/add.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/cli/src/cli_args/exec/recursive.rspacquet/crates/config/src/lib.rspacquet/crates/cli/src/cli_args/exec.rspacquet/crates/cli/tests/run_recursive.rspacquet/crates/cli/src/cli_args/recursive.rspacquet/crates/cli/tests/exec_recursive.rspacquet/crates/cli/src/cli_args.rspacquet/crates/cli/tests/run.rspacquet/crates/cli/src/cli_args/run.rspacquet/crates/cli/src/cli_args/dlx.rspacquet/crates/cli/src/cli_args/run/recursive.rs
pacquet/**/tests/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/tests/**/*.rs: Prefer real fixtures over dependency-injection seams — usetempfile::TempDir, the mocked registry, or integration tests spawning the actual binary for happy paths and most error paths; use the DI seam only for filesystem error kinds, deterministic time, shared process-global state, or external-service happy paths
Follow test-logging guidance — log before non-assert_eq!assertions,dbg!complex structures, skip logging for simple scalarassert_eq!
Tests must not be tolerant of missing build/runtime environment by silently returning early when a tool isn't found — let the test panic when a required tool is absent
Prefer#[cfg_attr(target_os = "windows", ignore = "...")](or matching#[cfg(unix)]gates) over runtime probe-and-skip helpers for platform-locked tools
Port relevant pnpm tests to Rust tests whenever they translate when porting behavior from pnpm
Use snapshot tests withinstaand carefully review diffs when intentional changes alter snapshots; accept withcargo insta reviewonly after careful review
Tests that need the mocked registry should startpnprthroughpacquet-testing-utils;cargo test/cargo nextest runshould not require a separatejust registry-mock launchstep
Files:
pacquet/crates/cli/tests/dlx.rspacquet/crates/cli/tests/exec.rspacquet/crates/cli/tests/run_recursive.rspacquet/crates/cli/tests/exec_recursive.rspacquet/crates/cli/tests/run.rs
pacquet/**/Cargo.toml
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/Cargo.toml: Check whether the workspace already depends on something suitable in[workspace.dependencies]in the rootCargo.tomlbefore adding a new dependency
Keep dependencies at the right level — add a new dependency to the specific crate that needs it, not to the workspace root or shared crate unless multiple crates depend on it
Files:
pacquet/crates/cli/Cargo.toml
🧠 Learnings (3)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/executor/src/lifecycle.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/config/src/env_overlay.rspacquet/crates/executor/src/lib.rspacquet/crates/cli/src/cli_args/run/tests.rspacquet/crates/config/src/pnpm_default_parity.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/cli/tests/dlx.rspacquet/crates/cli/tests/exec.rspacquet/crates/executor/src/run_script.rspacquet/crates/cli/src/cli_args/dlx/tests.rspacquet/crates/executor/src/run_script/tests.rspacquet/crates/cli/src/cli_args/add.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/cli/src/cli_args/exec/recursive.rspacquet/crates/config/src/lib.rspacquet/crates/cli/src/cli_args/exec.rspacquet/crates/cli/tests/run_recursive.rspacquet/crates/cli/src/cli_args/recursive.rspacquet/crates/cli/tests/exec_recursive.rspacquet/crates/cli/src/cli_args.rspacquet/crates/cli/tests/run.rspacquet/crates/cli/src/cli_args/run.rspacquet/crates/cli/src/cli_args/dlx.rspacquet/crates/cli/src/cli_args/run/recursive.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/executor/src/lifecycle.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/config/src/env_overlay.rspacquet/crates/executor/src/lib.rspacquet/crates/cli/src/cli_args/run/tests.rspacquet/crates/config/src/pnpm_default_parity.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/cli/tests/dlx.rspacquet/crates/cli/tests/exec.rspacquet/crates/executor/src/run_script.rspacquet/crates/cli/src/cli_args/dlx/tests.rspacquet/crates/executor/src/run_script/tests.rspacquet/crates/cli/src/cli_args/add.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/cli/src/cli_args/exec/recursive.rspacquet/crates/config/src/lib.rspacquet/crates/cli/src/cli_args/exec.rspacquet/crates/cli/tests/run_recursive.rspacquet/crates/cli/src/cli_args/recursive.rspacquet/crates/cli/tests/exec_recursive.rspacquet/crates/cli/src/cli_args.rspacquet/crates/cli/tests/run.rspacquet/crates/cli/src/cli_args/run.rspacquet/crates/cli/src/cli_args/dlx.rspacquet/crates/cli/src/cli_args/run/recursive.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.
Applied to files:
pacquet/crates/executor/src/lifecycle.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/config/src/env_overlay.rspacquet/crates/executor/src/lib.rspacquet/crates/cli/src/cli_args/run/tests.rspacquet/crates/config/src/pnpm_default_parity.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/cli/tests/dlx.rspacquet/crates/cli/tests/exec.rspacquet/crates/executor/src/run_script.rspacquet/crates/cli/src/cli_args/dlx/tests.rspacquet/crates/executor/src/run_script/tests.rspacquet/crates/cli/src/cli_args/add.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/cli/src/cli_args/exec/recursive.rspacquet/crates/config/src/lib.rspacquet/crates/cli/src/cli_args/exec.rspacquet/crates/cli/tests/run_recursive.rspacquet/crates/cli/src/cli_args/recursive.rspacquet/crates/cli/tests/exec_recursive.rspacquet/crates/cli/src/cli_args.rspacquet/crates/cli/tests/run.rspacquet/crates/cli/src/cli_args/run.rspacquet/crates/cli/src/cli_args/dlx.rspacquet/crates/cli/src/cli_args/run/recursive.rs
🔇 Additional comments (21)
pacquet/crates/cli/src/cli_args/exec.rs (2)
26-51: LGTM!Also applies to: 85-108, 115-123, 133-188, 194-240
72-74: ⚡ Quick winERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL isn’t recursive-only for
pnpm execcommand-not-foundpnpm sets
err['code'] = 'ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL'in the catch/bail path for command-not-found failures regardless ofopts.recursive, soExecError::CommandNotFoundusingERR_PNPM_RECURSIVE_EXEC_FIRST_FAILmatches pnpm’s behavior.> Likely an incorrect or invalid review comment.pacquet/crates/cli/src/cli_args/exec/recursive.rs (1)
36-140: LGTM!pacquet/crates/cli/tests/exec.rs (1)
1-173: LGTM!pacquet/crates/cli/tests/exec_recursive.rs (1)
14-73: LGTM!Also applies to: 99-147
pacquet/crates/cli/src/cli_args/add.rs (1)
97-161: LGTM!pacquet/crates/cli/src/cli_args/dlx.rs (2)
232-301: LGTM!
305-509: LGTM!pacquet/crates/cli/src/cli_args/dlx/tests.rs (1)
1-228: LGTM!pacquet/crates/cli/tests/dlx.rs (1)
1-54: LGTM!pacquet/crates/cli/Cargo.toml (1)
18-53: LGTM!pacquet/crates/cli/src/cli_args.rs (1)
1-313: LGTM!pacquet/crates/cli/README.md (1)
63-101: LGTM!pacquet/crates/config/src/pnpm_default_parity.rs (1)
113-113: LGTM!Also applies to: 151-151
pacquet/crates/executor/src/run_script/tests.rs (1)
1-99: LGTM!pacquet/crates/cli/src/cli_args/run.rs (1)
1-507: LGTM!pacquet/crates/cli/src/cli_args/run/tests.rs (1)
1-62: LGTM!pacquet/crates/cli/tests/run.rs (1)
7-411: LGTM!pacquet/crates/cli/src/cli_args/recursive.rs (1)
1-201: LGTM!pacquet/crates/cli/src/cli_args/run/recursive.rs (1)
1-229: LGTM!pacquet/crates/cli/tests/run_recursive.rs (1)
10-10: LGTM!Also applies to: 328-573
Model scriptShell and nodeOptions as tri-state Option<Option<String>> in WorkspaceSettings (like hoistPattern/publicHoistPattern) so an explicit `scriptShell: null` / `nodeOptions: null` in pnpm-workspace.yaml clears a value inherited from global config.yaml, while an absent key inherits. Matches pnpm's addSettingsFromWorkspaceManifestToConfig, which assigns every present settings key (including null) onto the merged config.
The script_shell field doc linked to the private deserialize_double_option helper, which fails `cargo doc` under `-D warnings` (rustdoc::private_intra_doc_links). Refer to it in plain prose, matching the hoist_pattern doc.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
pacquet/crates/cli/src/cli_args/exec/recursive.rs:52
- Recursive exec first-failure message currently reports
"pnpm recursive exec" ...even though users ranpacquet -r exec. Update the command name in the diagnostic text.
#[display("\"pnpm recursive exec\" failed in {prefix}")]
#[diagnostic(code(ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL))]
RecursiveExecFirstFail {
Warning
Scope note. Per
pacquet/CONTRIBUTING.md, pacquet's current focus is Stage 1 (the headless installer);execanddlxare new top-level commands, so this PR sits outside Stage 1 and is opened for review/discussion (roadmap pnpm/pacquet#299).Summary
Ports of
run,exec, anddlxfrom the TypeScript pnpm CLI.run: runs scripts through a new foregroundrun_scriptinpacquet-executor(sets upnode_modules/.binonPATH+ thenpm_*env). Handlespre/postscripts underenablePrePostScripts, arg shell-quoting (with the Windowscmd /d /s /cverbatimraw_argpath), script listing, hidden (.-prefixed) scripts,--if-present, thestart→server.jsfallback (with the NO_SCRIPT_OR_SERVER guard, including empty-stringstart), the[ELIFECYCLE]failure line (with thetest-stage and signal-killed variants), and exit-code propagation. The recursive runner's scaffolding (--resume-from/--report-summary) landed separately onmainvia #12093; this PR dispatches to it when-ris set and hardens it to match pnpm — per-projectpre/post, thePNPM_SCRIPT_SRC_DIRrecursion guard, pnpm's per-stage no-op guards, hidden-script handling, and--no-bail.exec: runs a command withnode_modules/.bin+extraBinPathsonPATH(resolved viawhich), stampsnpm_config_user_agent/PNPM_PACKAGE_NAME/NODE_OPTIONS, supports--shell-mode(the joined command goes through the sharedpush_script_arghelper, so the Windowscmd /d /s /cverbatim path usesraw_argand embedded quoting survives), rejects delimiter-containing dirs (ERR_PNPM_BAD_PATH_DIR). The recursive variant (-r) runs the command in every workspace project, topologically sorted and sequential, with--resume-from/--report-summary/--no-bailand pnpm's error codes (ERR_PNPM_RECURSIVE_EXEC_NO_PACKAGE/ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL/ERR_PNPM_RECURSIVE_FAIL). The workspace-graph / summary machinery is shared with recursiverunthrough a newcli_args::recursivemodule.dlx: installs the package(s) into a TTL cache dir (reusing the install pipeline, anchored at the cache dir, with a fresh per-install build-script allow-list — caller'sallow_builds/dangerouslyAllowAllBuildsdon't leak in), then runs the resolved bin in the process cwd. Supports--package,--allow-build,--shell-mode(samepush_script_argverbatim path as exec),--cpu/--os/--libcarchitecture overrides (folded into the per-axissupportedArchitecturesof the dlx install and into the cache key, so different overrides don't share a cached install),dlxCacheMaxAge; same PATH guard as exec.New
Configsettings:enablePrePostScripts,scriptShell,nodeOptions,dlxCacheMaxAge(wired intopnpm-workspace.yaml+ thePNPM_CONFIG_*overlay). Their defaults match pnpm and are asserted by thepnpm_default_paritycontract test — this PR movesenablePrePostScripts(which pnpm defaults totrue, a breaking change in #7634 shipped in v9) anddlxCacheMaxAgeinto its mapped rows.extraBinPathsis kept as a computed field (empty until workspace support lands), matching pnpm — it is not a user-settable key.Deferred (documented in code)
--filterand--workspace-concurrency. Recursiverunandexecrun every workspace project sequentially; the--filterpackage-selector subsystem and--workspace-concurrencyparallelism are not ported yet (the global--filter/--recursiveflags are accepted via clap but--filteris not consumed).dlxstays single-package by design (matches pnpm).run: the/regexp/script selector and the fuzzy "did you mean" hint are not ported (no regex/levenshtein dep);scriptsPrependNodePath: alwayscan't prepend the node dir (pacquet resolves no node execpath anywhere yet).dlx: the cache key uses raw specs (not resolved ids); noapprove-buildsprompt.Tests
run(14 integration + 8 unit), recursiverun(15 integration),exec(8 integration), recursiveexec(4 integration),dlx(16 unit + 2 integration; the install/run happy path needs the mocked registry and runs in CI),pacquet-executorrun_script(8 units).cargo fmt --check,clippy --all-targets -- --deny warnings, and dylint clean. The cliREADME.mdoptions checklist is updated forrun/exec/dlx.Written by an agent (Claude Code).
Summary by CodeRabbit
New Features
Configuration
Documentation
Tests