fix(installing.commands): forward pnpm install flags to pacquet#11781
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent 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)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📚 Learning: 2026-05-14T09:04:00.133ZApplied to files:
🔇 Additional comments (2)
📝 WalkthroughWalkthroughThe PR forwards user ChangesPacquet Install Delegation with Flag Forwarding
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
When the install engine is delegated to pacquet via configDependencies, pnpm hard-coded the args to `install --frozen-lockfile --reporter=ndjson` and silently dropped the user's other CLI flags. `pnpm install --no-runtime` therefore still installed the workspace's runtime devDependency, clobbering the Node version the surrounding tooling had set up — visible as the `Verify Node version` failure on PR #11765 where setup-pnpm provisions Node 24.0.0 but pacquet then materializes node 24.6.0. Pacquet's `install` subcommand already mirrors pnpm's surface for the common flags (`--no-runtime`, `--prod`, `--dev`, `--no-optional`, `--node-linker`, `--offline`, `--prefer-offline`, `--cpu`/`--os`/`--libc`). Forward the user's argv verbatim when the command is `install`/`i`; `add`/`update`/`dedupe` still don't forward — their flag surfaces don't line up with pacquet's `install`.
`pnpm up` / `add` / `remove` were aborting with `pacquet_package_manager::outdated_lockfile` whenever pacquet was declared in `configDependencies`. After resolving and writing the updated lockfile, pnpm hands materialization off to pacquet but hasn't yet written the post-mutation `package.json` — that write happens after `mutateModules` returns. Pacquet's frozen-lockfile freshness gate then saw the new lockfile paired with the pre-mutation manifest and refused to install. Pass pacquet's new `--ignore-manifest-check` flag (pacquet PR #11811) on every delegation. The flag is narrow: it only skips `satisfies_package_manifest`. Settings drift like `overrides` is still enforced, and pnpm already re-validated the lockfile before delegating, so re-checking the manifest here was redundant work that only ever fired false positives on the mutate-then-materialize path. Requires a pacquet release that ships the flag; bump `PACQUET_VERSION` in `pnpm/test/install/pacquet.ts` once it does, or the existing e2e tests will fail against pacquet 0.2.2-9 (which doesn't recognize the flag and clap would reject). Closes #11797. --- Written by an agent (Claude Code, claude-opus-4-7).
2244fc8 to
90e43cd
Compare
…hen forwarding to pacquet `collectForwardedFlags` checked `argv[0] === 'install'` to find the command token to strip. Any global flag the user typed before `install` (e.g. `--config.registry=...` in the e2e test) shifted the token out of position, so the function returned the full argv and pacquet saw `install` twice — `error: unexpected argument 'install' found`. Use the parsed argv that `@pnpm/cli.parse-cli-args` already produced: `remain` lists positionals (the `install`/`i` token and nothing else on this code path, since `isInstallCommand` is only true when no package params are present), and `original` preserves the user's exact tokens. Drop positionals + the flags we always inject (`--reporter=ndjson`, `--frozen-lockfile`, `--ignore-manifest-check`) so clap doesn't reject duplicates either. `original` over `cooked` deliberately: nopt's `cooked` splits `--key=value` into two tokens, which would break pacquet's `--config.<key>=<value>` parser (it requires the `=` form).
…CommandOptions
Widening these to required broke test fixtures elsewhere (publish/pack/
deprecate/dist-tag/deploy) that construct minimal `argv: { original }`
options for code paths that never reach pacquet. Only the pacquet
delegation actually reads `remain`, so make the two new fields optional
on the shared options type and supply a default at the runPacquet call
site. The runtime path through main.ts already populates all three.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pnpm/test/install/pacquet.ts (1)
127-131: 💤 Low valueConsider updating the comment referencing the old version.
The comment at lines 129-130 states "The pinned
0.2.2-9doesn't publish that mirror yet" but the version has now been updated to0.2.2. This comment may be outdated and could confuse future readers about the current state.🤖 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 `@pnpm/test/install/pacquet.ts` around lines 127 - 131, Update the outdated inline comment about the pinned pacquet version: locate the comment mentioning the pinned version string (the lines referencing "`0.2.2-9`" / "`0.2.2`" in pnpm/test/install/pacquet.ts) and either correct the version text to reflect the current pinned version or reword/remove the sentence so it no longer references the old `0.2.2-9` notation and accurately describes the current release/mirror status.
🤖 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 `@installing/commands/src/runPacquet.ts`:
- Around line 95-106: Forwarded reporter flags can still override the enforced
NDJSON reporter because only the exact '--reporter=ndjson' was filtered; update
the logic that builds forwardedFlags (or immediately after collecting it) to
drop any flag that sets a reporter (e.g., anything matching /^--reporter(\b|=)/
or /^-r(\b|=)/) before constructing args so args = ['--reporter=ndjson', ...]
cannot be overridden; modify the use of collectForwardedFlags/forwardedFlags in
runPacquet.ts (also replicate the same filter for the second occurrence around
lines 203-212) to filter out all reporter flags rather than only the exact
'--reporter=ndjson'.
---
Nitpick comments:
In `@pnpm/test/install/pacquet.ts`:
- Around line 127-131: Update the outdated inline comment about the pinned
pacquet version: locate the comment mentioning the pinned version string (the
lines referencing "`0.2.2-9`" / "`0.2.2`" in pnpm/test/install/pacquet.ts) and
either correct the version text to reflect the current pinned version or
reword/remove the sentence so it no longer references the old `0.2.2-9` notation
and accurately describes the current release/mirror status.
🪄 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: e6f2f80b-4d01-4e1c-b4d3-e7a4b2aa095b
📒 Files selected for processing (6)
.changeset/fix-pacquet-outdated-lockfile-on-update.md.changeset/forward-install-flags-to-pacquet.mdinstalling/commands/src/install.tsinstalling/commands/src/installDeps.tsinstalling/commands/src/runPacquet.tspnpm/test/install/pacquet.ts
📜 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: windows-latest / Node.js 22.13.0 / Test
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use trailing commas in TypeScript code
Prefer functions over classes in TypeScript code
Declare functions after they are used, relying on function hoisting in TypeScript
Functions should have no more than two or three arguments; use a single options object instead for multiple parameters in TypeScript
Follow import order in TypeScript: 1) standard libraries, 2) external dependencies (alphabetically sorted), 3) relative imports
Write self-explanatory code in TypeScript; avoid comments that restate what the code already says. Use comments only for non-obvious reasons, hidden invariants, or workarounds
Use JSDoc for function contracts (preconditions, postconditions, edge cases), not for re-narrating the function body in TypeScript
Files:
pnpm/test/install/pacquet.tsinstalling/commands/src/installDeps.tsinstalling/commands/src/install.tsinstalling/commands/src/runPacquet.ts
🧠 Learnings (1)
📚 Learning: 2026-05-14T09:04:00.133Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.
Applied to files:
pnpm/test/install/pacquet.tsinstalling/commands/src/installDeps.tsinstalling/commands/src/install.tsinstalling/commands/src/runPacquet.ts
🔇 Additional comments (6)
installing/commands/src/install.ts (1)
353-355: LGTM!Also applies to: 393-393
installing/commands/src/installDeps.ts (1)
130-133: LGTM!Also applies to: 165-171, 228-230
installing/commands/src/runPacquet.ts (1)
30-50: LGTM!Also applies to: 86-94
.changeset/fix-pacquet-outdated-lockfile-on-update.md (1)
1-6: LGTM!.changeset/forward-install-flags-to-pacquet.md (1)
1-7: LGTM!pnpm/test/install/pacquet.ts (1)
14-14: ⚡ Quick winMissing required context: Provide the original review comment text inside the
<review_comment>tags (and any diff/code snippet it references) so it can be rewritten correctly with updated, evidence-based fixes.
…warding to pacquet Pacquet's `--reporter` is a clap value option with last-value-wins semantics, so `pnpm install --reporter=silent` (or `--reporter silent` two-token form) reached pacquet and overrode the `--reporter=ndjson` pnpm injects, breaking the NDJSON-to- streamParser plumbing the default reporter depends on. The previous filter only matched the exact `--reporter=ndjson` token. Walk argv with a lookahead so both `--reporter=<value>` and `--reporter <value>` are dropped without consuming an adjacent flag.
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 `@installing/commands/src/runPacquet.ts`:
- Line 210: The check that skips already-injected flags only matches exact
positive forms (ALWAYS_INJECTED) and therefore lets negated forms like
--no-frozen-lockfile slip through; normalize boolean flags before comparing by
treating a leading --no- as the same flag (e.g., strip the --no- prefix or map
it to the positive form) and use that normalized token in the membership check
against ALWAYS_INJECTED and positionals so negated user flags are recognized and
skipped; update the logic around ALWAYS_INJECTED/positionals in runPacquet.ts
(the arg filtering code) to compare normalized names rather than raw arg strings
(and apply the same fix to the other similar block at lines ~222-225).
🪄 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: 75957cd0-77d0-48d5-a42d-d904385030c2
📒 Files selected for processing (2)
installing/commands/src/runPacquet.tspnpm/test/install/pacquet.ts
📜 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: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use trailing commas in TypeScript code
Prefer functions over classes in TypeScript code
Declare functions after they are used, relying on function hoisting in TypeScript
Functions should have no more than two or three arguments; use a single options object instead for multiple parameters in TypeScript
Follow import order in TypeScript: 1) standard libraries, 2) external dependencies (alphabetically sorted), 3) relative imports
Write self-explanatory code in TypeScript; avoid comments that restate what the code already says. Use comments only for non-obvious reasons, hidden invariants, or workarounds
Use JSDoc for function contracts (preconditions, postconditions, edge cases), not for re-narrating the function body in TypeScript
Files:
pnpm/test/install/pacquet.tsinstalling/commands/src/runPacquet.ts
🧠 Learnings (1)
📚 Learning: 2026-05-14T09:04:00.133Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.
Applied to files:
pnpm/test/install/pacquet.tsinstalling/commands/src/runPacquet.ts
🔇 Additional comments (4)
pnpm/test/install/pacquet.ts (1)
129-131: LGTM!installing/commands/src/runPacquet.ts (3)
18-51: LGTM!
95-106: LGTM!
241-248: LGTM!
… flags `collectForwardedFlags` only matched the exact positive tokens `--frozen-lockfile` and `--ignore-manifest-check`, so a user typing `pnpm install --no-frozen-lockfile` (or `--frozen-lockfile=false`) forwarded the negation to pacquet, which then saw both our injected `--frozen-lockfile` and the user's `--no-frozen-lockfile` and crashed clap with "unexpected argument". Match every shape the user can write the same flag in: positive, `--no-` negated, and any `=value` form. Can't blindly strip `--no-` either way — pacquet has flags whose literal name starts with `no-` (`--no-runtime`, `--no-optional`); those must still forward. The user's `--no-frozen-lockfile` intent is honored upstream — pnpm did a fresh resolve before delegating; pacquet's role here is just lockfile-driven materialization, which is always frozen.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
installing/commands/src/runPacquet.ts (1)
255-261:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFilter reporter flags out of the non-install warning.
collectDroppedFlags()still reports--reporter/--reporter <value>as "may not be honored" onadd/update/dedupe, but reporter selection is already honored by pnpm's own reporter pipeline. That makes the warning noisy and misleading.Suggested fix
function collectDroppedFlags (argv: { original: string[] }): string[] { - return argv.original.filter((arg) => { - if (!arg.startsWith('-')) return false - if (arg === '--frozen-lockfile' || arg === '--reporter=ndjson') return false - if (arg.startsWith('--config.')) return false - return true - }) + const result: string[] = [] + for (let i = 0; i < argv.original.length; i++) { + const arg = argv.original[i] + if (!arg.startsWith('-')) continue + if (arg === '--frozen-lockfile' || arg === '--reporter=ndjson') continue + if (arg.startsWith('--reporter=')) continue + if (arg === '--reporter') { + i++ + continue + } + if (arg.startsWith('--config.')) continue + result.push(arg) + } + return result }🤖 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 `@installing/commands/src/runPacquet.ts` around lines 255 - 261, collectDroppedFlags currently only ignores '--reporter=ndjson' but still reports other reporter forms; update collectDroppedFlags to iterate argv.original by index (not just filter) and skip any arg that is '--reporter' (and also skip the following token as the reporter value) or startsWith('--reporter=') so both `--reporter value` and `--reporter=value` are excluded, and keep existing exclusions for '--frozen-lockfile' and '--config.'; reference the function name collectDroppedFlags to locate and replace the simple filter with an index-based loop that returns the filtered array.
🤖 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 `@installing/commands/src/runPacquet.ts`:
- Around line 213-219: The current collectForwardedFlags uses value-based
matching (new Set(argv.remain)) which strips options when a positional has the
same text; change to index-aware positional matching by walking argv.original
with a pointer through argv.remain to mark positional indexes (i.e., treat
argv.remain as a subsequence and advance a j pointer when argv.original[i] ===
argv.remain[j], recording i as positional), then skip those recorded indexes and
calls to isAlwaysInjected(arg) when building result; update
collectForwardedFlags to use this index-based check instead of
positionals.has(arg).
---
Outside diff comments:
In `@installing/commands/src/runPacquet.ts`:
- Around line 255-261: collectDroppedFlags currently only ignores
'--reporter=ndjson' but still reports other reporter forms; update
collectDroppedFlags to iterate argv.original by index (not just filter) and skip
any arg that is '--reporter' (and also skip the following token as the reporter
value) or startsWith('--reporter=') so both `--reporter value` and
`--reporter=value` are excluded, and keep existing exclusions for
'--frozen-lockfile' and '--config.'; reference the function name
collectDroppedFlags to locate and replace the simple filter with an index-based
loop that returns the filtered array.
🪄 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: 0f600045-245c-41b8-a3eb-ce247f6e4191
📒 Files selected for processing (1)
installing/commands/src/runPacquet.ts
📜 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: Analyze (javascript)
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use trailing commas in TypeScript code
Prefer functions over classes in TypeScript code
Declare functions after they are used, relying on function hoisting in TypeScript
Functions should have no more than two or three arguments; use a single options object instead for multiple parameters in TypeScript
Follow import order in TypeScript: 1) standard libraries, 2) external dependencies (alphabetically sorted), 3) relative imports
Write self-explanatory code in TypeScript; avoid comments that restate what the code already says. Use comments only for non-obvious reasons, hidden invariants, or workarounds
Use JSDoc for function contracts (preconditions, postconditions, edge cases), not for re-narrating the function body in TypeScript
Files:
installing/commands/src/runPacquet.ts
🧠 Learnings (1)
📚 Learning: 2026-05-14T09:04:00.133Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.
Applied to files:
installing/commands/src/runPacquet.ts
…rom dropped-flags warning `collectForwardedFlags` matched positionals via `new Set(argv.remain)`, which strips by value: a flag value that happened to equal a positional token (e.g. `pnpm install --node-linker install`) was wrongly dropped from the forwarded list, costing pacquet the value of `--node-linker`. Walk `argv.original` with a subsequence pointer into `argv.remain` so only the actual positional indexes get skipped. `collectDroppedFlags` still surfaced `--reporter foo` / `--reporter=foo` in the "may not be honored" warning on `add`/`update`/`dedupe`, but pnpm honors reporter selection itself before delegation — so the warning was misleading. Route both helpers through the same `isAlwaysInjected` check and consume `--reporter` and its value the same way `collectForwardedFlags` already does.
Summary
When the install engine is delegated to pacquet via
configDependencies, pnpm was hard-coding the args toinstall --frozen-lockfile --reporter=ndjsonand silently dropping every other CLI flag. As a result,pnpm install --no-runtimestill installed the workspace's runtime devDependency.This surfaced as the
Verify Node versionfailure on theuse-pacquet-config-depCI job for #11765: setup-pnpm provisions Node 24.0.0, thenpnpm install --no-runtimeruns through pacquet, pacquet doesn't see--no-runtime, and node 24.6.0 from devDependencies clobbers the runtime onPATH. See the failing run.Pacquet's
installsubcommand already mirrors pnpm's surface for the common flags (--no-runtime,--prod,--dev,--no-optional,--node-linker,--offline,--prefer-offline,--cpu/--os/--libc,--frozen-lockfile). Forward the user's argv verbatim when the command isinstall/i;add/update/dedupestill don't forward — their flag surfaces don't line up with pacquet'sinstall.Changes
installing/commands/src/runPacquet.ts— newisInstallCommandopt onMakeRunPacquetOpts. When true,collectForwardedFlags()strips the leadinginstall/itoken from argv and appends the rest to pacquet's args (dropping a user-supplied--reporter=ndjsonso our own copy wins). The dropped-flags warning only fires on non-install commands.installing/commands/src/installDeps.ts—isInstallCommand?: booleanonInstallDepsOptions, threaded intomakeRunPacquet.installing/commands/src/install.ts— handler setsisInstallCommand: true.Test plan
ubuntu-latest / Node.js 24 / Testjob'sVerify Node versionstep should pass (the warningThe following CLI flags are not forwarded to pacquet and may not be honored: --no-runtimeshould disappear, and the workspace'snode 24.6.0devDependency should no longer overwrite the setup-pnpm runtime).pnpm/test/install/pacquet.tssuite still passes (no behavior change when no flags are passed).Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Bug Fixes
New Features
Tests