Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughpnpm now aborts installs when a tarball's integrity differs from the committed lockfile by default; locked integrity is only refreshed when the caller explicitly opts into repair via ChangesIntegrity-Mismatch Hard Failure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
Review Summary by QodoFail by default on tarball integrity mismatches against lockfile
WalkthroughsDescription• Fail by default on tarball integrity mismatches against lockfile - Previously silently re-resolved and overwrote locked integrity - Now requires explicit opt-in via --fix-lockfile, --force, or pnpm update • Add lockfileRepairOptedIn() function to gate integrity recovery • Update error hint to explain supply-chain implications and recovery options • Enhance test coverage for integrity mismatch scenarios Diagramflowchart LR
A["Tarball Integrity Mismatch"] --> B{Repair Opted In?}
B -->|No| C["Throw Error"]
B -->|Yes| D["Re-resolve & Update Lockfile"]
E["--fix-lockfile<br/>--force<br/>pnpm update"] --> B
F["--frozen-lockfile"] --> C
File Changes1. installing/deps-installer/src/install/index.ts
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
installing/deps-installer/test/brokenLockfileIntegrity.ts (1)
17-67: ⚡ Quick winAdd an explicit
update: truerecovery-path assertion.These tests now cover default failure,
fixLockfile, andforce, but not the documentedpnpm updateopt-in path. Adding one assertion here would lock that contract.✅ Minimal test addition
+ await mutateModulesInSingleProject({ + manifest, + mutation: 'install', + rootDir: process.cwd() as ProjectRootDir, + }, testDefaults({ update: true }, { retry: { retries: 0 } }))Also applies to: 69-110
🤖 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/deps-installer/test/brokenLockfileIntegrity.ts` around lines 17 - 67, Add an explicit recovery-path assertion that exercising the "pnpm update" opt-in (update: true) also repairs the corrupted lockfile: in the test function test('installation fails by default if the lockfile contains a wrong checksum, but --fix-lockfile recovers') after the existing force-path block (the mutateModulesInSingleProject call using testDefaults({ force: true })), add another mutateModulesInSingleProject invocation with testDefaults({ update: true }, { retry: { retries: 0 } }) and then assert that project.readLockfile() isStrictEqual to correctLockfile; repeat the same insertion for the sibling test block referenced at the second location (lines 69-110) so both tests assert the update: true recovery path.
🤖 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 `@worker/src/index.ts`:
- Around line 142-149: The integrity hint string (the hint property in the code
block around where the tarball integrity message is defined) only mentions
"--fix-lockfile"; update that hint to list all supported explicit recovery
opt-ins by adding "--force" and "pnpm update" alongside the existing
"--fix-lockfile" and "pnpm store prune" suggestions so the CLI guidance is
complete and consistent.
---
Nitpick comments:
In `@installing/deps-installer/test/brokenLockfileIntegrity.ts`:
- Around line 17-67: Add an explicit recovery-path assertion that exercising the
"pnpm update" opt-in (update: true) also repairs the corrupted lockfile: in the
test function test('installation fails by default if the lockfile contains a
wrong checksum, but --fix-lockfile recovers') after the existing force-path
block (the mutateModulesInSingleProject call using testDefaults({ force: true
})), add another mutateModulesInSingleProject invocation with testDefaults({
update: true }, { retry: { retries: 0 } }) and then assert that
project.readLockfile() isStrictEqual to correctLockfile; repeat the same
insertion for the sibling test block referenced at the second location (lines
69-110) so both tests assert the update: true recovery path.
🪄 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: e299ba00-0aee-4d5b-9a43-5039b3974168
📒 Files selected for processing (4)
.changeset/integrity-mismatch-fails-by-default.mdinstalling/deps-installer/src/install/index.tsinstalling/deps-installer/test/brokenLockfileIntegrity.tsworker/src/index.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}: Follow Standard Style with trailing commas, preferring functions over classes, and declaring functions after they are used (relying on hoisting)
Use a single options object instead of multiple parameters when a function needs more than two or three arguments
Follow Import Order: Standard libraries first, then external dependencies (alphabetically), then relative imports
Write self-documenting code where function names, parameters, and types explain what a function does without requiring prose comments
Do not write comments that restate what the code already says; refactor via renaming, splitting helpers, or restructuring instead
Do not repeat documentation at call sites that already exists in JSDoc on the callee; update JSDoc once for all call sites to benefit
Use JSDoc only for a function's contract (preconditions, postconditions, edge cases, why the function exists), not for re-narrating the body
Do not record past implementation shape, refactor history, or 'the previous code did X' framing in code; use git log and git blame instead
Write comments only when: the reason for code is non-obvious (hidden invariant, workaround for known bug, deliberate exception), or the right name doesn't fit (temporary technical constraint)
Files:
installing/deps-installer/test/brokenLockfileIntegrity.tsworker/src/index.tsinstalling/deps-installer/src/install/index.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/deps-installer/test/brokenLockfileIntegrity.tsworker/src/index.tsinstalling/deps-installer/src/install/index.ts
🔇 Additional comments (2)
installing/deps-installer/src/install/index.ts (1)
118-129: LGTM!Also applies to: 1063-1071, 2006-2010
.changeset/integrity-mismatch-fails-by-default.md (1)
1-10: LGTM!
Previously, plain `pnpm install` (non-frozen) reacted to `ERR_PNPM_TARBALL_INTEGRITY` by silently re-resolving from the registry and overwriting the locked integrity. A compromised registry, proxy, or republished version could substitute attacker-controlled content on a clean machine even though the project shipped a committed lockfile. Treat integrity mismatches as a hard failure unless the caller opted in via `--fix-lockfile`, `--force`, or `pnpm update`. `--frozen-lockfile` behavior is unchanged. Co-authored-by: Claude <noreply@anthropic.com>
Both flags also opt into lockfile repair. Listing them keeps the hint consistent with the install-side logic that accepts them as recovery opt-ins.
…e-checksums flag --fix-lockfile is documented for filling in *missing* lockfile entries. Reusing it to overwrite an existing-but-mismatched integrity conflates two different operations: someone running --fix-lockfile to recover from a half-merged lockfile shouldn't also be rubber-stamping registry- tampered content. Add --update-checksums (matching yarn's flag of the same name), scoped narrowly to refreshing the locked integrity values from what the registry currently serves. Make it the explicit opt-in for the integrity-mismatch recovery path, drop --fix-lockfile from that path, and update the TarballIntegrityError hint and tests accordingly. --force and `pnpm update` remain implicit opt-ins.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
installing/deps-installer/test/brokenLockfileIntegrity.ts (1)
17-67: ⚡ Quick winAdd coverage for the remaining lockfile-repair branches.
These tests lock in default failure plus the
updateChecksumsandforceopt-ins, but they still miss the other two load-bearing edges of this change:update: trueshould recover, andfixLockfile: trueshould still fail. One extra regression case for each would make this policy much harder to accidentally loosen later.Possible test additions
+ await mutateModulesInSingleProject({ + manifest, + mutation: 'install', + rootDir: process.cwd() as ProjectRootDir, + update: true, + }, testDefaults({}, { retry: { retries: 0 } })) + + await expect(mutateModulesInSingleProject({ + manifest, + mutation: 'install', + rootDir: process.cwd() as ProjectRootDir, + }, testDefaults({ fixLockfile: true }, { retry: { retries: 0 } }))).rejects.toThrow(/Got unexpected checksum/)Also applies to: 69-110
🤖 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/deps-installer/test/brokenLockfileIntegrity.ts` around lines 17 - 67, The test covers frozenLockfile, updateChecksums and force paths but misses the update:true recovery and the fixLockfile:true failure branches; add two assertions calling mutateModulesInSingleProject with testDefaults({ update: true }, { retry: { retries: 0 } }) and expect it to succeed and update the lockfile to correctLockfile, and another call with testDefaults({ fixLockfile: true }, { retry: { retries: 0 } }) and expect it to reject with /Got unexpected checksum for/; place these near the existing mutateModulesInSingleProject calls that exercise updateChecksums and force so they reference the same manifest/manifest variable and project.readLockfile() checks.
🤖 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/deps-installer/src/install/extendInstallOptions.ts`:
- Around line 54-62: The new required boolean option updateChecksums is not
initialized in defaults(), causing extendOptions()/ProcessedInstallOptions to
possibly return undefined; update defaults() in extendInstallOptions.ts to
include updateChecksums: false alongside the other boolean defaults so
ProcessedInstallOptions always has a boolean value, and ensure any places
merging or validating options (e.g., extendOptions or the defaults object) treat
updateChecksums the same as the existing boolean flags.
---
Nitpick comments:
In `@installing/deps-installer/test/brokenLockfileIntegrity.ts`:
- Around line 17-67: The test covers frozenLockfile, updateChecksums and force
paths but misses the update:true recovery and the fixLockfile:true failure
branches; add two assertions calling mutateModulesInSingleProject with
testDefaults({ update: true }, { retry: { retries: 0 } }) and expect it to
succeed and update the lockfile to correctLockfile, and another call with
testDefaults({ fixLockfile: true }, { retry: { retries: 0 } }) and expect it to
reject with /Got unexpected checksum for/; place these near the existing
mutateModulesInSingleProject calls that exercise updateChecksums and force so
they reference the same manifest/manifest variable and project.readLockfile()
checks.
🪄 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: 510041f6-89a4-42c7-8a15-4e81a71f3698
📒 Files selected for processing (6)
.changeset/integrity-mismatch-fails-by-default.mdinstalling/commands/src/install.tsinstalling/deps-installer/src/install/extendInstallOptions.tsinstalling/deps-installer/src/install/index.tsinstalling/deps-installer/test/brokenLockfileIntegrity.tsworker/src/index.ts
✅ Files skipped from review due to trivial changes (2)
- worker/src/index.ts
- .changeset/integrity-mismatch-fails-by-default.md
📜 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}: Follow Standard Style with trailing commas, preferring functions over classes, and declaring functions after they are used (relying on hoisting)
Use a single options object instead of multiple parameters when a function needs more than two or three arguments
Follow Import Order: Standard libraries first, then external dependencies (alphabetically), then relative imports
Write self-documenting code where function names, parameters, and types explain what a function does without requiring prose comments
Do not write comments that restate what the code already says; refactor via renaming, splitting helpers, or restructuring instead
Do not repeat documentation at call sites that already exists in JSDoc on the callee; update JSDoc once for all call sites to benefit
Use JSDoc only for a function's contract (preconditions, postconditions, edge cases, why the function exists), not for re-narrating the body
Do not record past implementation shape, refactor history, or 'the previous code did X' framing in code; use git log and git blame instead
Write comments only when: the reason for code is non-obvious (hidden invariant, workaround for known bug, deliberate exception), or the right name doesn't fit (temporary technical constraint)
Files:
installing/commands/src/install.tsinstalling/deps-installer/src/install/extendInstallOptions.tsinstalling/deps-installer/test/brokenLockfileIntegrity.tsinstalling/deps-installer/src/install/index.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/install.tsinstalling/deps-installer/src/install/extendInstallOptions.tsinstalling/deps-installer/test/brokenLockfileIntegrity.tsinstalling/deps-installer/src/install/index.ts
…tch recovery --force and `pnpm update` are routine refresh operations; silently overwriting a locked integrity in those flows would erase the protection a committed lockfile is supposed to provide. Reserve the opt-in for --update-checksums, which is explicit about its intent. When the bypass does kick in, surface a dedicated warning that names --update-checksums so the lockfile rewrite stays auditable rather than masquerading as a "broken lockfile" recovery.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
installing/deps-installer/src/install/index.ts (1)
1085-1090:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
--update-checksumscurrently falls into the normal update flow.Setting
project.update = truehere makes the recovery path behave like a real update._installInContext()later sees that flag at Lines 1387-1392 and dropspreferredVersions, so a checksum repair can pull newer semver-matching versions instead of only re-fetching the existing locked package and refreshing its integrity. That makes this recovery path broader than the flag’s contract.Also applies to: 2024-2028
🤖 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/deps-installer/src/install/index.ts` around lines 1085 - 1090, The recovery path for isIntegrityError is wrongly setting (project as InstallMutationOptions).update = true which causes _installInContext to treat it as a full update and drop preferredVersions; instead add a dedicated flag (e.g., refreshIntegrityOnly or forceIntegrityRefresh) to InstallMutationOptions and set that for each project when isIntegrityError is true, then update _installInContext to detect this new flag and perform only integrity re-download/verification without clearing preferredVersions or engaging the normal update flow; also add the new flag to the InstallMutationOptions type so TypeScript recognizes it.
🤖 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.
Outside diff comments:
In `@installing/deps-installer/src/install/index.ts`:
- Around line 1085-1090: The recovery path for isIntegrityError is wrongly
setting (project as InstallMutationOptions).update = true which causes
_installInContext to treat it as a full update and drop preferredVersions;
instead add a dedicated flag (e.g., refreshIntegrityOnly or
forceIntegrityRefresh) to InstallMutationOptions and set that for each project
when isIntegrityError is true, then update _installInContext to detect this new
flag and perform only integrity re-download/verification without clearing
preferredVersions or engaging the normal update flow; also add the new flag to
the InstallMutationOptions type so TypeScript recognizes it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 65a1bb75-8208-4f80-894c-f84c80f5ca63
📒 Files selected for processing (4)
.changeset/integrity-mismatch-fails-by-default.mdinstalling/deps-installer/src/install/index.tsinstalling/deps-installer/test/brokenLockfileIntegrity.tsworker/src/index.ts
✅ Files skipped from review due to trivial changes (2)
- worker/src/index.ts
- .changeset/integrity-mismatch-fails-by-default.md
📜 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}: Follow Standard Style with trailing commas, preferring functions over classes, and declaring functions after they are used (relying on hoisting)
Use a single options object instead of multiple parameters when a function needs more than two or three arguments
Follow Import Order: Standard libraries first, then external dependencies (alphabetically), then relative imports
Write self-documenting code where function names, parameters, and types explain what a function does without requiring prose comments
Do not write comments that restate what the code already says; refactor via renaming, splitting helpers, or restructuring instead
Do not repeat documentation at call sites that already exists in JSDoc on the callee; update JSDoc once for all call sites to benefit
Use JSDoc only for a function's contract (preconditions, postconditions, edge cases, why the function exists), not for re-narrating the body
Do not record past implementation shape, refactor history, or 'the previous code did X' framing in code; use git log and git blame instead
Write comments only when: the reason for code is non-obvious (hidden invariant, workaround for known bug, deliberate exception), or the right name doesn't fit (temporary technical constraint)
Files:
installing/deps-installer/src/install/index.tsinstalling/deps-installer/test/brokenLockfileIntegrity.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/deps-installer/src/install/index.tsinstalling/deps-installer/test/brokenLockfileIntegrity.ts
Port the security-relevant half of #11968 to pacquet so the two CLIs behave identically when refreshing the locked integrity from the registry: - New `--update-checksums` flag on `pacquet install`. Threaded through `Install` and forced onto the fresh-resolve dispatch path: the user has opted into rewriting locked integrity, so the frozen-lockfile fast path (which would refuse on mismatch) is skipped. - `--update-checksums + --frozen-lockfile` is contradictory and now errors out with `pacquet_package_manager::frozen_lockfile_with_outdated_lockfile`, matching upstream's `ERR_PNPM_FROZEN_LOCKFILE_WITH_OUTDATED_LOCKFILE`. Pacquet's existing frozen-path integrity check already fails-hard on mismatch (no auto-repair to remove). The pacquet-side change here is purely about CLI surface parity so `pnpm install --update-checksums` keeps working when pacquet is the materialization target.
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11968 +/- ##
==========================================
+ Coverage 87.93% 88.04% +0.10%
==========================================
Files 228 228
Lines 27810 28084 +274
==========================================
+ Hits 24456 24727 +271
- Misses 3354 3357 +3 ☔ 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": 1.9542787957,
"stddev": 0.08916934332744293,
"median": 1.9158370411,
"user": 2.754301,
"system": 3.2921192599999998,
"min": 1.8699331731000002,
"max": 2.1650845090999997,
"times": [
1.9997691861000002,
1.9748819701000002,
2.0150187611,
1.9162068141000002,
1.9154672681000002,
1.8916359821000002,
2.1650845090999997,
1.8699331731000002,
1.8820219261000002,
1.9127683671000002
]
},
{
"command": "pacquet@main",
"mean": 1.9417368341000003,
"stddev": 0.05454299327247572,
"median": 1.9221472441,
"user": 2.7964125,
"system": 3.2418863599999996,
"min": 1.8994226471000002,
"max": 2.0688454381,
"times": [
1.9110080611000002,
1.9310053671000003,
1.9452644471000002,
2.0688454381,
1.9226616071,
1.9034175911000002,
1.8994226471000002,
1.9216328811,
1.9068068161000002,
2.0073034851
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6458038076800001,
"stddev": 0.04479281631279863,
"median": 0.63366634598,
"user": 0.35731345999999997,
"system": 1.34766282,
"min": 0.62370076298,
"max": 0.77232434298,
"times": [
0.77232434298,
0.6409137499800001,
0.63604480898,
0.6345648659800001,
0.63276782598,
0.63556860298,
0.62758949698,
0.6307115639800001,
0.62370076298,
0.62385205598
]
},
{
"command": "pacquet@main",
"mean": 0.6486854950800001,
"stddev": 0.01545684674654336,
"median": 0.6452477114799999,
"user": 0.37025425999999995,
"system": 1.3504832199999997,
"min": 0.63017197198,
"max": 0.68652222698,
"times": [
0.68652222698,
0.65824169798,
0.63017197198,
0.64302314798,
0.64347263498,
0.64059175298,
0.64702278798,
0.65026475198,
0.6364833439800001,
0.65106063398
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.276156374,
"stddev": 0.027812131754936388,
"median": 2.2793784706,
"user": 3.86260658,
"system": 3.0377359000000004,
"min": 2.2418023756000003,
"max": 2.3272427986000004,
"times": [
2.3272427986000004,
2.2824549306,
2.2419147736,
2.2418023756000003,
2.2898825216,
2.2955959986,
2.2662141336,
2.2453752526,
2.2763020106000003,
2.2947789446
]
},
{
"command": "pacquet@main",
"mean": 2.3439786531,
"stddev": 0.03905237145779966,
"median": 2.3378419116,
"user": 3.976436079999999,
"system": 3.0657900000000007,
"min": 2.2811500836,
"max": 2.3929052306000003,
"times": [
2.2811500836,
2.3313201236000003,
2.3112311346000003,
2.3911554556000003,
2.3108134196,
2.3929052306000003,
2.3839498446,
2.3443636996,
2.3217882626,
2.3711092766000004
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.51558008742,
"stddev": 0.06299410079990962,
"median": 1.50515108542,
"user": 1.78644238,
"system": 1.8510429,
"min": 1.4634676179200001,
"max": 1.68597797692,
"times": [
1.5124577379200002,
1.68597797692,
1.49130711992,
1.5178775799200002,
1.4927408179200001,
1.50476787592,
1.5055342949200001,
1.4644260659200001,
1.5172437869200002,
1.4634676179200001
]
},
{
"command": "pacquet@main",
"mean": 1.4778804275200002,
"stddev": 0.03185364323022722,
"median": 1.4720621454200002,
"user": 1.7694980799999995,
"system": 1.8342413,
"min": 1.4462332659200001,
"max": 1.5397847609200002,
"times": [
1.5397847609200002,
1.4742204309200002,
1.5246944539200002,
1.4491188489200002,
1.4895284999200002,
1.4462332659200001,
1.46990385992,
1.45985155592,
1.47462696692,
1.4508416319200002
]
}
]
} |
|
Bencher Report| Branch | pr/11968 |
| 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,276.16 ms(-9.19%)Baseline: 2,506.59 ms | 3,007.91 ms (75.67%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,515.58 ms(-8.13%)Baseline: 1,649.72 ms | 1,979.66 ms (76.56%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 1,954.28 ms(-3.92%)Baseline: 2,033.93 ms | 2,440.72 ms (80.07%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 645.80 ms(-2.09%)Baseline: 659.62 ms | 791.54 ms (81.59%) |
The recovery-path setting of project.update = true (inherited from pre-existing code) is dead under the new --update-checksums gating: - Inner catch is unreachable for integrity errors because needsFullResolution = true (set by --update-checksums) makes isFrozenInstallPossible false, so tryFrozenInstall returns null without entering its try block. - Outer catch's integrity-recovery branch is unreachable because the preceding _installInContext call (with needsFullResolution = true) re-queries metadata via forceFullResolution → proceed and refreshes the integrity in the wanted lockfile before fetch. Setting project.update would also drop preferredVersions and let the resolver bump locked versions within range — broader than --update-checksums asks for. Remove the lines. Rename lockfileRepairOptedIn → integrityRefreshOptedIn so the call site reads on its own, and trim the surrounding comments and help text that were doing the job a good name and types already do.
The StrictInstallOptions field is non-optional, but extendOptions() was leaving it undefined on the cast — making the runtime value diverge from the type contract. Add an explicit default alongside the other booleans.
Under --update-checksums gating, the inner catch's integrity-error
branch is effectively unreachable: opts.updateChecksums sets
needsFullResolution=true at line 600, which makes tryFrozenInstall
return null without entering its try block. The only path that
reliably reaches this catch is LOCKFILE_MISSING_DEPENDENCY (bad-Git-
merge recovery), where the original 'lockfile is broken' message is
the right one.
Also drop the now-dead `if (isIntegrityError) { needsFullResolution =
true }` block — under the only reachable opt-in path that *would* hit
this catch (ignorePackageManifest combined with --update-checksums),
needsFullResolution is already true from the line-600 assignment.
The outer catch keeps its dedicated 'Refreshing the locked integrity
...' message, since that path is the one that actually exists for
the user-opted-in recovery (TOCTOU race between resolve and fetch).
Mirrors the hint pnpm's TarballIntegrityError now emits when an integrity mismatch is surfaced. Same recovery flag, same supply-chain caveat, so a user hitting the failure from either CLI gets the same guidance.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pacquet/crates/package-manager/src/install.rs (1)
402-410:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
--update-checksumsis bypassed by the optimistic up-to-date short-circuit.When
update_checksumsis true, this early return can still emit “Already up to date” and skip the fresh-resolve path entirely, so checksum refresh is not guaranteed.Suggested fix
- if !frozen_lockfile + if !frozen_lockfile + && !update_checksums && let OptimisticRepeatInstallDecision::UpToDate = check_optimistic_repeat_install( &workspace_root, config, node_linker, included, &project_manifests, workspace_manifest.is_some(), )🤖 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.rs` around lines 402 - 410, The optimistic up-to-date short-circuit currently returns when check_optimistic_repeat_install(...) yields OptimisticRepeatInstallDecision::UpToDate even if update_checksums is true, which lets "--update-checksums" be bypassed; update the early-return conditional around frozen_lockfile and OptimisticRepeatInstallDecision::UpToDate (the if that calls check_optimistic_repeat_install) to also require that update_checksums is false (e.g., consult the config.update_checksums flag) so the short-circuit is skipped when update_checksums is set, ensuring the fresh-resolve/checksum-refresh path always runs when update_checksums is true.
🤖 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.
Outside diff comments:
In `@pacquet/crates/package-manager/src/install.rs`:
- Around line 402-410: The optimistic up-to-date short-circuit currently returns
when check_optimistic_repeat_install(...) yields
OptimisticRepeatInstallDecision::UpToDate even if update_checksums is true,
which lets "--update-checksums" be bypassed; update the early-return conditional
around frozen_lockfile and OptimisticRepeatInstallDecision::UpToDate (the if
that calls check_optimistic_repeat_install) to also require that
update_checksums is false (e.g., consult the config.update_checksums flag) so
the short-circuit is skipped when update_checksums is set, ensuring the
fresh-resolve/checksum-refresh path always runs when update_checksums is true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e99d6330-ed66-42b3-bb54-d89e62789eb8
📒 Files selected for processing (11)
.changeset/integrity-mismatch-fails-by-default.mdinstalling/commands/src/install.tsinstalling/deps-installer/src/install/extendInstallOptions.tsinstalling/deps-installer/src/install/index.tsinstalling/deps-installer/test/brokenLockfileIntegrity.tspacquet/crates/cli/src/cli_args/install.rspacquet/crates/package-manager/src/add.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/tarball/src/lib.rsworker/src/index.ts
✅ Files skipped from review due to trivial changes (3)
- pacquet/crates/tarball/src/lib.rs
- worker/src/index.ts
- .changeset/integrity-mismatch-fails-by-default.md
🚧 Files skipped from review as they are similar to previous changes (3)
- installing/deps-installer/src/install/extendInstallOptions.ts
- installing/deps-installer/test/brokenLockfileIntegrity.ts
- installing/commands/src/install.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: ubuntu-latest / Node.js 24 / Test
🧰 Additional context used
📓 Path-based instructions (2)
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/package-manager/src/add.rspacquet/crates/cli/src/cli_args/install.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.rs
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Follow Standard Style with trailing commas, preferring functions over classes, and declaring functions after they are used (relying on hoisting)
Use a single options object instead of multiple parameters when a function needs more than two or three arguments
Follow Import Order: Standard libraries first, then external dependencies (alphabetically), then relative imports
Write self-documenting code where function names, parameters, and types explain what a function does without requiring prose comments
Do not write comments that restate what the code already says; refactor via renaming, splitting helpers, or restructuring instead
Do not repeat documentation at call sites that already exists in JSDoc on the callee; update JSDoc once for all call sites to benefit
Use JSDoc only for a function's contract (preconditions, postconditions, edge cases, why the function exists), not for re-narrating the body
Do not record past implementation shape, refactor history, or 'the previous code did X' framing in code; use git log and git blame instead
Write comments only when: the reason for code is non-obvious (hidden invariant, workaround for known bug, deliberate exception), or the right name doesn't fit (temporary technical constraint)
Files:
installing/deps-installer/src/install/index.ts
🧠 Learnings (4)
📚 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/package-manager/src/add.rspacquet/crates/cli/src/cli_args/install.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.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/package-manager/src/add.rspacquet/crates/cli/src/cli_args/install.rspacquet/crates/package-manager/src/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/package-manager/src/add.rspacquet/crates/cli/src/cli_args/install.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.rs
📚 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/deps-installer/src/install/index.ts
🔇 Additional comments (8)
installing/deps-installer/src/install/index.ts (3)
592-597: LGTM!
1051-1060: LGTM!
1987-2000: LGTM!pacquet/crates/cli/src/cli_args/install.rs (1)
177-182: LGTM!Also applies to: 215-216, 279-280
pacquet/crates/package-manager/src/add.rs (1)
109-109: LGTM!pacquet/crates/package-manager/src/install.rs (1)
110-113: LGTM!Also applies to: 230-235, 312-313, 608-610, 628-629
pacquet/crates/package-manager/src/install/tests.rs (2)
147-189: LGTM!
66-66: LGTM!Also applies to: 135-135, 252-252, 318-318, 401-401, 469-469, 557-557, 702-702, 803-803, 1002-1002, 1075-1075, 1171-1171, 1301-1301, 1399-1399, 1505-1505, 1555-1555, 1647-1647, 1741-1741, 1805-1805, 1870-1870, 1961-1961, 2068-2068, 2129-2129, 2217-2217, 2324-2324, 2438-2438, 2518-2518, 2718-2718, 2842-2842, 2942-2942, 3048-3048, 3139-3139, 3233-3233, 3324-3324, 3412-3412, 3506-3506, 3598-3598, 3696-3696, 3796-3796, 3882-3882, 3966-3966, 4036-4036, 4102-4102, 4167-4167, 4235-4235, 4319-4319, 4381-4381, 4466-4466, 4517-4517, 4588-4588, 4660-4660, 4726-4726, 4978-4978, 5159-5159, 5307-5307, 5456-5456, 5534-5534, 5585-5585
Without this, the resolver short-circuits the registry when its in-memory or on-disk metadata cache already has the wanted version pinned, handing back whatever integrity was cached. That defeats --update-checksums on dev machines where the cache is the very thing that's potentially stale. Plumb updateChecksums from the install options through resolver-base, deps-resolver, store-controller-types, and npm-resolver into pickPackage, then gate the two fast paths (in-memory metaCache hit at line 209 and exact-version on-disk hit at line 267) on !opts.updateChecksums. With the flag set, every resolution goes through the network fetch with conditional headers, so the registry is the authority on what integrity to record. The conditional-request flow stays intact: if registry content hasn't changed it returns 304 and pacquet reuses cache; if it has, 200 with fresh metadata wins.
…s cache bypass Pacquet's pick_package mirrors upstream pnpm's same four-layer cache walk before any network fetch. Without gating, --update-checksums on pacquet has the same blind spot as pnpm had before the previous commit: a stale in-memory or on-disk cached packument with the wanted version pinned would short-circuit the registry entirely, handing back a possibly-stale integrity. Thread update_checksums from ResolveOptions into PickPackageOptions and gate steps 1 (in-memory cache) and 3 (version-spec on-disk fast path) on !opts.update_checksums. Plumb through Install → InstallWithFreshLockfile → ResolveOptions so the existing pacquet/cli/install --update-checksums flag now also bypasses the metadata cache, matching the pnpm-side behavior. Step 4 (publishedBy mtime shortcut) is intentionally left untouched on both sides — it's the maturity-filter cache, not the integrity-source cache, and the upstream pnpm fix doesn't gate it either.
Benchmark Results
Run 26480539396 · 10 runs per scenario · triggered by @zkochan |
The in-memory metaCache is constructed per-install (empty at start) and only the resolver's own pickPackage writes to it — populating it from network fetches that already respect the disk fast-path gate. Entries are therefore fresh by construction; gating reads from the cache just forces redundant conditional requests for second-and-onward references to the same package. Drop the gate on both pnpm (pickPackage.ts:215) and pacquet (pick_package.rs:419, 461). The disk fast-path gates remain — those are the ones that actually force a registry round-trip on first encounter.
…ty (#11985) Backport of #11968 to release/10. Treats tarball-integrity mismatches against the lockfile as a hard failure by default; `--update-checksums` is the only opt-in. `--force` and `pnpm update` deliberately do not bypass the integrity check. 🤖 Written by an agent (Claude Code, claude-opus-4-7).
Summary
pnpm install(non-frozen) used to react toERR_PNPM_TARBALL_INTEGRITYby logging the error, silently re-resolving from the registry, and overwriting the locked integrity. The lockfile's integrity was effectively advisory by default — a compromised registry, proxy, or republished version could substitute attacker-controlled content on a clean machine even though the project shipped a committedpnpm-lock.yaml.Integrity mismatches against the lockfile now fail by default.
The only opt-in is
pnpm install --update-checksums— a new flag, narrowly scoped to refreshing the locked integrity values. Mirrors yarn's flag of the same name. A warning still prints when the bypass takes effect so the rewrite stays auditable.--forceandpnpm updatedeliberately do not bypass the integrity check. They are routine refresh operations; silently overwriting a locked integrity in those flows would erase the protection a committed lockfile is supposed to provide.--frozen-lockfilebehavior is unchanged.--fix-lockfilekeeps its documented purpose (filling in missing lockfile entries) and is also not a bypass. Combining--frozen-lockfilewith--update-checksumserrors out — frozen mode refuses to rewrite the lockfile, which is exactly what--update-checksumsis for.--update-checksumsalso bypasses the resolver's on-disk metadata cache fast path (pickPackage.ts:271,pick_package.rs:531). Without that, a stale on-disk packument that already contained the pinned version would short-circuit the registry entirely and the flag would silently no-op on dev machines. With the gate, every first-encounter goes through a conditional GET; the in-memory cache is left alone so second-and-onward references within the same install still hit cached fresh data (one network round-trip per unique package, not per reference).Reported by
Reported privately via the security channel. The reproduction:
example-package@1.0.0with contentv1and install with pnpm; lockfile records thev1integrity.1.0.0with contentv2.pnpm install. Before this fix, pnpm loggedERR_PNPM_TARBALL_INTEGRITYbut exited 0 withv2installed and the lockfile rewritten to the new integrity. After this fix, the same install exits non-zero.Prior art
EINTEGRITY. No dedicated override flag — recovery isnpm cache clean --force, manually editing the lockfile, or deleting it.yarn install --update-checksumsflag — pnpm now adopts the same name.Pacquet parity
Pacquet was already fail-hard on integrity mismatch by default (no auto-repair path to remove). This PR brings the rest of the surface into line so
pnpm install --update-checksumskeeps working when pacquet is the materialization target, andpacquet install --update-checksumsbehaves identically standalone:--update-checksumsflag onpacquet install(crates/cli/src/cli_args/install.rs), plumbed throughInstallandInstallWithFreshLockfileinto the resolver.--frozen-lockfile + --update-checksumserrors withpacquet_package_manager::frozen_lockfile_with_outdated_lockfile, mirroring pnpm'sERR_PNPM_FROZEN_LOCKFILE_WITH_OUTDATED_LOCKFILE.pacquet_tarball::verify_checksum_errornow carries a help hint pointing at--update-checksumsand calling out the supply-chain implication, matching the updated pnpmTarballIntegrityError.crates/resolving-npm-resolver/src/pick_package.rs:531, with the flag threaded fromResolveOptions→PickPackageOptions.Test plan
pnpm --filter @pnpm/installing.deps-installer test brokenLockfileIntegrity.ts— passes (covers plain install fails, --update-checksums recovers, --force still fails)pnpm --filter @pnpm/installing.deps-installer test fixLockfile.ts— passes (--fix-lockfile semantics unchanged)pnpm --filter @pnpm/worker test— passespnpm --filter @pnpm/tarball-fetcher test— passescargo nextest run -p pacquet-package-manager install::tests— 79 tests pass, newshould_error_when_frozen_lockfile_and_update_checksums_are_both_setincludedcargo nextest run -p pacquet-resolving-npm-resolver -p pacquet-resolving-deps-resolver -p pacquet-package-manager— 614 tests passcargo clippy --locked --workspace --all-targets -- --deny warnings— cleanWritten by an agent (Claude Code, claude-opus-4-7).