Conversation
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? |
|
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 (5)
📜 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). (7)
🧰 Additional context used📓 Path-based instructions (1)pacquet/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
🧠 Learnings (2)📚 Learning: 2026-05-20T19:40:55.051ZApplied to files:
📚 Learning: 2026-05-20T23:07:58.444ZApplied to files:
🔇 Additional comments (5)
📝 WalkthroughWalkthroughA new CLI flag ChangesManifest check bypass feature
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
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 |
…nifest gate Surfaces a narrow CLI flag on `pacquet install` that gates only `satisfies_package_manifest`. Settings-drift checks (`overrides`, `ignoredOptionalDependencies`, …) still fire, and the broader `--ignore-package-manifest` name is reserved for a future port of pnpm's `pnpm fetch` semantics (which skip linking / hoisting / pruning too). Intended for the pnpm CLI's `configDependencies` delegation path (issue #11797): pnpm resolves and writes the lockfile, then hands materialization to pacquet but hasn't yet written the post-mutation `package.json`. With the flag set, the freshness gate skips the per-importer manifest check that would otherwise reject every `pnpm up` / `add` / `remove` with `ERR_PNPM_OUTDATED_LOCKFILE`. The matching pnpm-side change to forward the flag lands separately. Refs #11797. --- Written by an agent (Claude Code, claude-opus-4-7).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pacquet/crates/package-manager/src/install/tests.rs (1)
1493-1497: ⚡ Quick winTighten the bypass assertion to prove the install reached downstream frozen-path work.
!matches!(..., OutdatedLockfile)is too broad; unrelated early errors would also pass this test. Assert a downstream frozen-lockfile error shape instead.Proposed test assertion tightening
let err = result.expect_err("bogus tarball URL must still surface a downstream error"); - assert!( - !matches!(err, InstallError::OutdatedLockfile { .. }), - "ignore_manifest_check should bypass the freshness gate, got OutdatedLockfile: {err:?}", - ); + assert!( + matches!(err, InstallError::FrozenLockfile(_)), + "expected downstream frozen-lockfile failure after bypass, got {err:?}", + );🤖 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/package-manager/src/install/tests.rs` around lines 1493 - 1497, Replace the broad negative check that err is not InstallError::OutdatedLockfile with a positive assertion that the failure came from the downstream frozen-lockfile path; specifically assert that err matches the frozen-lockfile install error variant (e.g. assert!(matches!(err, InstallError::FrozenLockfile { .. }))) so the test proves we reached the frozen-path logic when ignore_manifest_check is used.
🤖 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.
Nitpick comments:
In `@pacquet/crates/package-manager/src/install/tests.rs`:
- Around line 1493-1497: Replace the broad negative check that err is not
InstallError::OutdatedLockfile with a positive assertion that the failure came
from the downstream frozen-lockfile path; specifically assert that err matches
the frozen-lockfile install error variant (e.g. assert!(matches!(err,
InstallError::FrozenLockfile { .. }))) so the test proves we reached the
frozen-path logic when ignore_manifest_check is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 35caace8-c07e-4853-a0bb-bc1bfd21963d
📒 Files selected for processing (7)
.changeset/fix-pacquet-outdated-lockfile-on-update.mdinstalling/commands/src/runPacquet.tspacquet/crates/cli/src/cli_args/install.rspacquet/crates/cli/src/cli_args/install/tests.rspacquet/crates/package-manager/src/add.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/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). (9)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Analyze (javascript)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Dylint
- GitHub Check: Code Coverage
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: When porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(), orstreamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plainStringor&str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only viaTryFrom<String>and/orFromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallibleFrom<String>(andFrom<&str>when convenient).
If upstream occasionally constructs without validation, exposefrom_str_uncheckedas an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization.
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls. Fall back to manualimplonly when conversion needs custom logic.
String-literal unions should becomeenums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;. Two forms stay allowed: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-...
Files:
pacquet/crates/cli/src/cli_args/install/tests.rspacquet/crates/package-manager/src/add.rspacquet/crates/package-manager/src/install.rspacquet/crates/cli/src/cli_args/install.rspacquet/crates/package-manager/src/install/tests.rs
🧠 Learnings (3)
📚 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
📚 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/install/tests.rspacquet/crates/package-manager/src/add.rspacquet/crates/package-manager/src/install.rspacquet/crates/cli/src/cli_args/install.rspacquet/crates/package-manager/src/install/tests.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/install/tests.rspacquet/crates/package-manager/src/add.rspacquet/crates/package-manager/src/install.rspacquet/crates/cli/src/cli_args/install.rspacquet/crates/package-manager/src/install/tests.rs
🔇 Additional comments (7)
.changeset/fix-pacquet-outdated-lockfile-on-update.md (1)
5-5: LGTM!installing/commands/src/runPacquet.ts (1)
77-92: LGTM!pacquet/crates/cli/src/cli_args/install.rs (1)
83-101: LGTM!Also applies to: 163-163, 207-207
pacquet/crates/cli/src/cli_args/install/tests.rs (1)
112-123: LGTM!pacquet/crates/package-manager/src/install.rs (1)
67-78: LGTM!Also applies to: 252-253, 526-544
pacquet/crates/package-manager/src/add.rs (1)
97-97: LGTM!pacquet/crates/package-manager/src/install/tests.rs (1)
61-61: LGTM!Also applies to: 119-119, 160-160, 230-230, 293-293, 373-373, 438-438, 523-523, 665-665, 763-763, 878-878, 998-998, 1093-1093, 1196-1196, 1243-1243, 1332-1332, 1423-1423, 1441-1492, 1499-1500, 1546-1546, 1634-1634, 1692-1692, 1777-1777, 1852-1852, 1933-1933, 2132-2132, 2253-2253, 2350-2350, 2453-2453, 2541-2541, 2632-2632, 2720-2720, 2805-2805, 2896-2896, 2985-2985, 3080-3080, 3177-3177
`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).
Micro-Benchmark ResultsLinux |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11811 +/- ##
==========================================
+ Coverage 87.39% 87.47% +0.07%
==========================================
Files 200 202 +2
Lines 23514 23704 +190
==========================================
+ Hits 20550 20735 +185
- Misses 2964 2969 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
`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).
…1781) * fix(installing.commands): forward `pnpm install` flags to pacquet 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`. * fix(installing.commands): pass --ignore-manifest-check to pacquet `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). * fix: update pacquet in tests * fix(installing.commands): strip positionals + always-injected flags when 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). * fix(installing.commands): make argv.cooked/remain optional on InstallCommandOptions 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. * fix(installing.commands): strip any user-supplied --reporter when forwarding 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. * fix(installing.commands): drop negated/value forms of always-injected 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. * fix(installing.commands): match positionals by index, hide reporter from 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
--ignore-manifest-checktopacquet install. The flag gates onlysatisfies_package_manifest; settings-drift checks (overrides,ignoredOptionalDependencies, …) still fire. The broader--ignore-package-manifestname is reserved for a later port of pnpm'spnpm fetchsemantics (which would skip linking / hoisting / pruning too).configDependenciesdelegation path: pnpm resolves and writes the lockfile, then hands materialization to pacquet but hasn't yet written the post-mutationpackage.json. With the flag, the freshness gate skips the per-importer manifest check that would otherwise reject everypnpm up/add/removewithERR_PNPM_OUTDATED_LOCKFILE.The matching pnpm-side change (forwarding the flag from
installing/commands/src/runPacquet.ts) lands separately on theforward-install-flags-to-pacquetbranch.Refs #11797.
Test plan
cargo nextest run -p pacquet-cli -p pacquet-package-manager— 367 tests pass, including the newignore_manifest_check_bypasses_manifest_freshness_gate(uses the existing drift fixture; asserts the install moves past the freshness gate instead of returningOutdatedLockfile) andignore_manifest_check_flag_parses(CLI surface coverage).cargo clippy -p pacquet-cli -p pacquet-package-manager --all-targets -- --deny warningsclean.cargo fmt --all -- --checkclean.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Release Notes
New Features
--ignore-manifest-checkCLI flag for the install command, allowing you to skip the package manifest freshness validation check during installation.Tests