fix: verify lockfile tarball URL matches registry metadata#12134
Conversation
📝 WalkthroughWalkthroughAdds an unconditional, fail-closed tarball-URL binding check during lockfile resolution verification: when a lockfile pins an explicit registry tarball URL, pnpm fetches abbreviated registry metadata for name@version, canonicalizes and compares the registry’s dist.tarball to the lockfile URL, and rejects mismatches with TARBALL_URL_MISMATCH before applying age/trust rules. ChangesTarball URL binding verification
Sequence Diagram — high-level verification flow: sequenceDiagram
participant Installer as verifyLockfileResolutions
participant Verifier as NpmResolutionVerifier
participant Registry as LookupContext
participant Cache as PolicyCache
Installer->>Verifier: verify(resolution, { name, version, nonSemverVersion })
Verifier->>Registry: fetch abbreviated metadata (version -> dist.tarball)
Registry-->>Verifier: version_tarballs
Verifier->>Verifier: canonicalize & compare tarball URLs
Verifier->>Cache: record tarballUrlBinding on accepted checks
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Micro-Benchmark ResultsLinux |
Integrated-Benchmark Report (Linux)Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.05849975752,
"stddev": 0.093949230597713,
"median": 2.01973002962,
"user": 2.53412684,
"system": 3.313409159999999,
"min": 1.9615438626200001,
"max": 2.21994925362,
"times": [
2.0268295206199998,
2.21994925362,
1.96620895262,
2.12358557462,
2.12374147262,
1.9615438626200001,
2.01263053862,
2.1745178456199996,
1.9729528546200001,
2.0030376996199997
]
},
{
"command": "pacquet@main",
"mean": 1.9829472560199999,
"stddev": 0.03230034397626105,
"median": 1.9760882666200001,
"user": 2.5486046399999998,
"system": 3.2939931600000003,
"min": 1.93381401362,
"max": 2.0473770936199998,
"times": [
1.9779956176200002,
1.97418091562,
1.9666177906200002,
1.9720064426200001,
1.93381401362,
1.99178614262,
1.9861265026200001,
2.0226100316199997,
1.95695800962,
2.0473770936199998
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.65254018514,
"stddev": 0.029380901920484922,
"median": 0.64417789754,
"user": 0.35313673999999995,
"system": 1.3099816999999998,
"min": 0.63659083054,
"max": 0.73463933554,
"times": [
0.73463933554,
0.64604910354,
0.63694478254,
0.64520808354,
0.6431477115399999,
0.65569776054,
0.63980342254,
0.64659800954,
0.63659083054,
0.64072281154
]
},
{
"command": "pacquet@main",
"mean": 0.64598738164,
"stddev": 0.0190193503987252,
"median": 0.64011640854,
"user": 0.35547564,
"system": 1.3154507,
"min": 0.6299671945399999,
"max": 0.69032284254,
"times": [
0.69032284254,
0.63376397154,
0.66891040354,
0.64222324554,
0.64049439754,
0.6299671945399999,
0.63973841954,
0.64592497554,
0.63394540154,
0.63458296454
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.2960392768,
"stddev": 0.035380841674207464,
"median": 2.2808800157999998,
"user": 3.695135539999999,
"system": 3.0770832599999998,
"min": 2.2655254838,
"max": 2.3791534768,
"times": [
2.2687050257999997,
2.2756004267999996,
2.3791534768,
2.3169441598,
2.3061015638,
2.2861596048,
2.2698972027999997,
2.3178665067999997,
2.2744393167999997,
2.2655254838
]
},
{
"command": "pacquet@main",
"mean": 2.2925103895999994,
"stddev": 0.026878447246652785,
"median": 2.2927390442999998,
"user": 3.676994439999999,
"system": 3.10059926,
"min": 2.2414950067999997,
"max": 2.3348310058,
"times": [
2.3117395218,
2.3065872537999996,
2.2823340977999997,
2.2982099438,
2.2829520558,
2.2414950067999997,
2.3348310058,
2.3150567908,
2.2872681447999996,
2.2646300748
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.4914612192799999,
"stddev": 0.01274120636235909,
"median": 1.49021623468,
"user": 1.6800262600000004,
"system": 1.8521960400000002,
"min": 1.47491459268,
"max": 1.51342713968,
"times": [
1.47645531368,
1.50603697168,
1.48245877768,
1.49517345768,
1.49157859968,
1.5018453706800001,
1.48386809968,
1.51342713968,
1.47491459268,
1.4888538696800002
]
},
{
"command": "pacquet@main",
"mean": 1.4946459304800002,
"stddev": 0.05338817181155543,
"median": 1.4833680611800002,
"user": 1.6911748599999998,
"system": 1.8421776399999998,
"min": 1.4224007096800002,
"max": 1.6293295566800001,
"times": [
1.4224007096800002,
1.4780554186800001,
1.47614819368,
1.6293295566800001,
1.4628112016800001,
1.47634389668,
1.50090772568,
1.48868070368,
1.5061148476800001,
1.50566705068
]
}
]
} |
|
Bencher Report| Branch | pr/12134 |
| 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,296.04 ms(-2.77%)Baseline: 2,361.45 ms | 2,833.74 ms (81.02%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,491.46 ms(-2.12%)Baseline: 1,523.73 ms | 1,828.47 ms (81.57%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,058.50 ms(-0.92%)Baseline: 2,077.52 ms | 2,493.03 ms (82.57%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 652.54 ms(-0.79%)Baseline: 657.71 ms | 789.26 ms (82.68%) |
09dc094 to
3b9a491
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12134 +/- ##
==========================================
- Coverage 87.51% 87.42% -0.09%
==========================================
Files 261 261
Lines 29646 29704 +58
==========================================
+ Hits 25944 25969 +25
- Misses 3702 3735 +33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3b9a491 to
bf0bc34
Compare
Review Summary by QodoVerify lockfile tarball URLs match registry metadata to prevent tampering
WalkthroughsDescription• Adds unconditional tarball URL verification binding lockfile entries to registry metadata • Prevents tampering attacks pairing trusted name@version with attacker-chosen URLs • Rejects mismatches with ERR_PNPM_TARBALL_URL_MISMATCH error code • Applies independently of minimumReleaseAge/trustPolicy policies • Threads nonSemverVersion to skip URL-keyed tarball dependencies from policy checks Diagramflowchart LR
A["Lockfile Entry<br/>with tarball URL"] -->|verify| B["Fetch Registry<br/>Metadata"]
B -->|extract dist.tarball| C["Compare URLs<br/>with tolerance"]
C -->|match| D["✓ Pass"]
C -->|mismatch| E["✗ TARBALL_URL_MISMATCH"]
F["nonSemverVersion set?"] -->|yes| G["Skip verification<br/>non-registry dep"]
A --> F
File Changes1. resolving/npm-resolver/src/createNpmResolutionVerifier.ts
|
There was a problem hiding this comment.
Pull request overview
This PR strengthens lockfile resolution verification by binding lockfile-pinned registry tarball URLs to the registry’s own metadata for the same name@version, preventing a tampered lockfile from swapping in attacker-controlled tarballs while still passing age/trust checks. It also threads nonSemverVersion into the verifier context so URL-keyed (non-registry) tarball dependencies are correctly skipped.
Changes:
- Add an unconditional registry tarball URL ↔ packument
dist.tarballmatch check, emittingTARBALL_URL_MISMATCHon confirmed mismatches. - Extend the
ResolutionVerifier.verify()context withnonSemverVersionand propagate it through lockfile verification fan-out. - Mirror the behavior in pacquet and add/adjust tests + release notes.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| resolving/resolver-base/src/index.ts | Extends verifier context with nonSemverVersion to distinguish URL/git-keyed deps from registry name@version. |
| resolving/npm-resolver/src/createNpmResolutionVerifier.ts | Implements unconditional tarball URL binding + skips URL-keyed deps; verifier now always returned. |
| resolving/npm-resolver/src/violationCodes.ts | Adds TARBALL_URL_MISMATCH violation code. |
| resolving/npm-resolver/test/createNpmResolutionVerifier.test.ts | Adds tests for mismatch detection, non-standard tarball acceptance, URL-keyed skip, and “no policy” behavior. |
| resolving/default-resolver/src/index.ts | Always includes the npm resolution verifier in the verifier list. |
| installing/deps-installer/src/install/verifyLockfileResolutions.ts | Threads nonSemverVersion from lockfile snapshots into verifier calls. |
| pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs | Mirrors tarball URL binding + always-build verifier behavior in Rust. |
| pacquet/crates/resolving-npm-resolver/src/lookup_context.rs | Stores per-version tarball URLs in abbreviated projection for binding + version presence checks. |
| pacquet/crates/resolving-npm-resolver/src/violation_codes.rs | Adds TARBALL_URL_MISMATCH violation code in Rust. |
| pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs | Updates/extends verifier tests for unconditional binding + policy-off scenarios. |
| pacquet/crates/package-manager/src/build_resolution_verifiers.rs | Always pushes npm verifier (no longer Option). |
| pacquet/crates/package-manager/src/install/tests.rs | Adjusts fixtures/tests to set trust_lockfile: true where tripwire tarball URLs would now fail verification. |
| .changeset/tarball-url-binding.md | Documents the security fix and behavior changes; bumps affected packages. |
Comments suppressed due to low confidence (1)
resolving/npm-resolver/src/createNpmResolutionVerifier.ts:112
- The doc comment claims the verifier has fail-closed semantics when metadata can’t be loaded or the version is missing, but the new tarball-URL binding is explicitly positive-only (it passes when metadata is unavailable). This makes the statement inaccurate, especially in the “no age/trust policy active” case where tarball binding is the only check that can run.
* Designed for fail-closed semantics: if the manifest can't be loaded or
* the pinned version is missing from it, the verifier reports a violation
* rather than silently passing. Mirrors the post-resolution gate bun added
* for the same shape of bug in oven-sh/bun#30526.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
resolving/default-resolver/src/index.ts (1)
196-206:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale JSDoc:
createResolutionVerifiersno longer returns an empty array.With line 236 unconditionally pushing
npmVerifier, this factory always returns a one-element array. The "Returns an empty array when no policy is active — callers can cheaply decide whether to iterate at all by checkingverifiers.length" sentence is now incorrect and should be updated, since the tarball-URL binding runs regardless of policy configuration.📝 Suggested doc correction
- * Companion to {`@link` createResolver}. Collects the resolver-specific - * verifier factories (today: npm) into a list. Returns an empty array - * when no policy is active — callers can cheaply decide whether to - * iterate at all by checking `verifiers.length`. + * Companion to {`@link` createResolver}. Collects the resolver-specific + * verifier factories (today: npm) into a list. The npm verifier is + * always included because its tarball-URL binding is unconditional, so + * the returned list is non-empty even when no age/trust policy is set.🤖 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 `@resolving/default-resolver/src/index.ts` around lines 196 - 206, Update the stale JSDoc for createResolutionVerifiers to reflect its current behavior: remove or rewrite the sentence claiming it "Returns an empty array when no policy is active" and instead document that the function always includes the npmVerifier (and thus returns at least one ResolutionVerifier) because the tarball-URL binding runs unconditionally; reference createResolutionVerifiers and npmVerifier in the doc so callers understand they can expect a one-element array even when no policy is configured.pacquet/crates/package-manager/src/build_resolution_verifiers.rs (1)
58-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale doc: the verifier list is no longer ever empty.
Now that line 126 unconditionally pushes the npm verifier,
build_resolution_verifiersalways returns a one-elementVec. The "Returns an emptyVecwhen neither policy is active" contract here (and the matching "lets the install path skip the call entirely when the vec is empty" note in the module doc at lines 7-8) no longer holds and misleads readers about the empty-list short-circuit. Please update both to reflect that the tarball-URL binding makes verifier creation unconditional.As per coding guidelines: "Doc comments document the contract — preconditions, postconditions, panics, and the reason the function exists".📝 Suggested doc correction
-/// Assemble the verifier list for this install. Returns an empty -/// `Vec` when neither policy is active — the runner short-circuits -/// on an empty list, so the caller doesn't need a separate guard. +/// Assemble the verifier list for this install. Always includes the +/// npm resolution verifier: the tarball-URL binding it performs is +/// unconditional (independent of minimumReleaseAge / trustPolicy), so +/// the verifier is created even when neither policy is active.🤖 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/build_resolution_verifiers.rs` around lines 58 - 60, The function doc for build_resolution_verifiers and the module-level note claiming it can return an empty Vec are stale: because the npm verifier is now always pushed (due to the tarball-URL binding), build_resolution_verifiers always returns a non-empty Vec. Update the doc comment on build_resolution_verifiers and the module doc to remove the "Returns an empty `Vec`..." / "short-circuits on an empty list" language and instead state that the function always returns at least the npm verifier (i.e., a non-empty list) and explain why (tarball-URL binding causes unconditional npm verifier creation); keep mention of the runner behavior but remove the empty-list guard contract.pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs (1)
579-615:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis fixture now bypasses the attestation request entirely.
With the matching tarball URL and the old
modifiedtimestamp added here,run_tarball_url_check()can populate abbreviated metadata first, and the age check can then reuse that cached response to returnOkvia the abbreviated shortcut. That makes the/-/npm/v1/attestations/acme@1.0.0mock at Line 573 unreachable, so this test is now expecting a request the verifier should no longer make. Either remove that expectation or make the abbreviated response recent enough that the age check must fall through.🤖 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/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs` around lines 579 - 615, The test's packument timestamp allows run_tarball_url_check() to serve abbreviated metadata that makes the attestation mock unreachable; fix by making the packument's "time" for "1.0.0" a recent timestamp relative to opts.now (e.g., use now_at("2025-12-01T00:00:00Z") or similar) so the age check falls through and the attestation request is issued, or alternatively remove the unreachable attestation mock expectation around create_async() for "/-/npm/v1/attestations/acme@1.0.0" so the test no longer asserts that request. Ensure you update the packument JSON in the test that constructs packument (or drop the attestation mock creation) and keep references to run_tarball_url_check(), create_npm_resolution_verifier, opts, and the packument variable when making the change.
🧹 Nitpick comments (1)
resolving/npm-resolver/test/createNpmResolutionVerifier.test.ts (1)
303-334: ⚡ Quick winCover the canonical-equivalence path explicitly.
This case only stays green on an exact tarball URL match. The production helper also treats
http/httpsand%2f//scoped-name differences as equivalent, and none of the new tests pin that behavior. Please add a scoped-package variant where the lockfile URL and metadata URL differ only by scheme and/or%2fencoding, so a regression insameTarballUrl()cannot slip through.🤖 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 `@resolving/npm-resolver/test/createNpmResolutionVerifier.test.ts` around lines 303 - 334, Add a new test variant in createNpmResolutionVerifier.test that exercises the canonical-equivalence path: create metadata for a scoped package (e.g. `@scope/pkg`) where the version's dist.tarball in the registry metadata uses one scheme and encoding (e.g. 'http' and '%2f' in the path), but call verifier.verify with a lockfile Resolution tarball URL that differs only by scheme/enconding (e.g. 'https' and decoded '/' in the path); ensure the mocked pool intercept for the package name returns that meta and assert verify returns { ok: true }. This will exercise sameTarballUrl() canonicalization via createNpmResolutionVerifier and prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pacquet/crates/package-manager/src/install/tests.rs`:
- Around line 1791-1795: The doc comment is outdated: update the rationale so it
reflects that the tarball-URL binding check is now unconditional (it is no
longer active “via `minimumReleaseAge”`); state that `trust_lockfile` is the
opt-out that skips the verifier and keep the reference to the specific failure
code `TARBALL_URL_MISMATCH` for clarity. Edit the comment above this test to
remove the “via `minimumReleaseAge`” wording and explicitly say the verifier
runs unconditionally unless `trust_lockfile` is set.
In `@pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs`:
- Around line 156-166: The persisted snapshot currently lacks any marker for the
new unconditional tarball URL binding, so older cached snapshots can be reused
and bypass TARBALL_URL_MISMATCH checks; modify the snapshot creation inside
create_npm_resolution_verifier (and/or wherever policy_snapshot is serialized
for NpmResolutionVerifier) to include a verifier_version or explicit
tarballUrlBinding boolean, and update the can_trust_past_check logic to require
that marker/version match the current verifier (rejecting snapshots without the
marker or with an older version) so past checks produced before the rule are
invalidated.
- Around line 302-316: The registry selection and tarball binding must be
skipped for URL-keyed tarball dependencies before calling self.pick_registry and
run_tarball_url_check; add an early guard that if tarball_url is Some then
return/skip resolution for this path (i.e., do not call self.pick_registry or
self.run_tarball_url_check with ctx.name/ctx.version) so remote tarballs that
happen to expose a semver version are not treated as registry-pinned artifacts.
In `@resolving/npm-resolver/src/createNpmResolutionVerifier.ts`:
- Around line 100-107: The persisted policy snapshot returned by
createNpmResolutionVerifier lacks a version/sentinel for the new unconditional
tarball binding, so cached verification records from before this change bypass
TARBALL_URL_MISMATCH; add a constant like TARBALL_BINDING_POLICY_VERSION and
include it on the returned policy object (alongside existing age/trust settings)
and in the persisted snapshot, then update canTrustPastCheck() to require the
incoming cached policy to have the same TARBALL_BINDING_POLICY_VERSION
(reject/trust-false if missing or mismatched) so past checks without the
sentinel are not reused for tarball enforcement; touch symbols:
createNpmResolutionVerifier, policy object shape, canTrustPastCheck(), and
TARBALL_URL_MISMATCH.
---
Outside diff comments:
In `@pacquet/crates/package-manager/src/build_resolution_verifiers.rs`:
- Around line 58-60: The function doc for build_resolution_verifiers and the
module-level note claiming it can return an empty Vec are stale: because the npm
verifier is now always pushed (due to the tarball-URL binding),
build_resolution_verifiers always returns a non-empty Vec. Update the doc
comment on build_resolution_verifiers and the module doc to remove the "Returns
an empty `Vec`..." / "short-circuits on an empty list" language and instead
state that the function always returns at least the npm verifier (i.e., a
non-empty list) and explain why (tarball-URL binding causes unconditional npm
verifier creation); keep mention of the runner behavior but remove the
empty-list guard contract.
In
`@pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs`:
- Around line 579-615: The test's packument timestamp allows
run_tarball_url_check() to serve abbreviated metadata that makes the attestation
mock unreachable; fix by making the packument's "time" for "1.0.0" a recent
timestamp relative to opts.now (e.g., use now_at("2025-12-01T00:00:00Z") or
similar) so the age check falls through and the attestation request is issued,
or alternatively remove the unreachable attestation mock expectation around
create_async() for "/-/npm/v1/attestations/acme@1.0.0" so the test no longer
asserts that request. Ensure you update the packument JSON in the test that
constructs packument (or drop the attestation mock creation) and keep references
to run_tarball_url_check(), create_npm_resolution_verifier, opts, and the
packument variable when making the change.
In `@resolving/default-resolver/src/index.ts`:
- Around line 196-206: Update the stale JSDoc for createResolutionVerifiers to
reflect its current behavior: remove or rewrite the sentence claiming it
"Returns an empty array when no policy is active" and instead document that the
function always includes the npmVerifier (and thus returns at least one
ResolutionVerifier) because the tarball-URL binding runs unconditionally;
reference createResolutionVerifiers and npmVerifier in the doc so callers
understand they can expect a one-element array even when no policy is
configured.
---
Nitpick comments:
In `@resolving/npm-resolver/test/createNpmResolutionVerifier.test.ts`:
- Around line 303-334: Add a new test variant in
createNpmResolutionVerifier.test that exercises the canonical-equivalence path:
create metadata for a scoped package (e.g. `@scope/pkg`) where the version's
dist.tarball in the registry metadata uses one scheme and encoding (e.g. 'http'
and '%2f' in the path), but call verifier.verify with a lockfile Resolution
tarball URL that differs only by scheme/enconding (e.g. 'https' and decoded '/'
in the path); ensure the mocked pool intercept for the package name returns that
meta and assert verify returns { ok: true }. This will exercise sameTarballUrl()
canonicalization via createNpmResolutionVerifier and prevent regressions.
🪄 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: 9f0adaf8-acb7-4bbf-a91f-5c77b713ab17
📒 Files selected for processing (13)
.changeset/tarball-url-binding.mdinstalling/deps-installer/src/install/verifyLockfileResolutions.tspacquet/crates/package-manager/src/build_resolution_verifiers.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/resolving-npm-resolver/src/violation_codes.rsresolving/default-resolver/src/index.tsresolving/npm-resolver/src/createNpmResolutionVerifier.tsresolving/npm-resolver/src/violationCodes.tsresolving/npm-resolver/test/createNpmResolutionVerifier.test.tsresolving/resolver-base/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). (2)
- GitHub Check: Agent
- GitHub Check: ubuntu-latest / Node.js 24 / Test
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:
resolving/npm-resolver/src/violationCodes.tsresolving/default-resolver/src/index.tsresolving/resolver-base/src/index.tsinstalling/deps-installer/src/install/verifyLockfileResolutions.tsresolving/npm-resolver/test/createNpmResolutionVerifier.test.tsresolving/npm-resolver/src/createNpmResolutionVerifier.ts
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: Log emissions are part of matching pnpm — when porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/resolving-npm-resolver/src/violation_codes.rspacquet/crates/package-manager/src/build_resolution_verifiers.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use util.types.isNativeError() instead of instanceof Error for error type checking in Jest tests
Files:
resolving/npm-resolver/test/createNpmResolutionVerifier.test.ts
🧠 Learnings (6)
📚 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:
resolving/npm-resolver/src/violationCodes.tsresolving/default-resolver/src/index.tsresolving/resolver-base/src/index.tsinstalling/deps-installer/src/install/verifyLockfileResolutions.tsresolving/npm-resolver/test/createNpmResolutionVerifier.test.tsresolving/npm-resolver/src/createNpmResolutionVerifier.ts
📚 Learning: 2026-05-23T17:29:56.247Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11878
File: resolving/npm-resolver/src/createNpmResolutionVerifier.ts:381-418
Timestamp: 2026-05-23T17:29:56.247Z
Learning: When reviewing the npm resolver code, note that `PackageMetaCache` is intentionally keyed only by `name` and `name:full` (no registry component). As a result, code that shares this cache (e.g., `createNpmResolutionVerifier.ts` via the shared `validateSharedMeta` guard) can prevent cross-package contamination by matching names, but it cannot distinguish two different registries that serve packages with the same name within a single install. Don’t flag (or attempt to fix) this as a local issue in the verifier alone—correctly distinguishing registries would require a coordinated change to the resolver cache key shape. (The Pacquet/Rust cache is already registry-qualified, unlike the npm-resolver cache.)
Applied to files:
resolving/npm-resolver/src/violationCodes.tsresolving/npm-resolver/src/createNpmResolutionVerifier.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/resolving-npm-resolver/src/violation_codes.rspacquet/crates/package-manager/src/build_resolution_verifiers.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/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/resolving-npm-resolver/src/violation_codes.rspacquet/crates/package-manager/src/build_resolution_verifiers.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/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/resolving-npm-resolver/src/violation_codes.rspacquet/crates/package-manager/src/build_resolution_verifiers.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs
📚 Learning: 2026-05-26T21:01:06.666Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11966
File: .changeset/require-tarball-integrity.md:6-6
Timestamp: 2026-05-26T21:01:06.666Z
Learning: In pnpm lockfile-related release notes/docs (especially changeset markdown), preserve URL hostnames exactly as they appear in pnpm-lock.yaml tarball resolution entries—keep hosts like `codeload.github.com`, `bitbucket.org`, and `gitlab.com` in lowercase. Do not “correct” them to title-case/preserve brand capitalization (e.g., LanguageTool rules like `GITHUB` capitalization) because these are literal URL fragments, not platform brand names.
Applied to files:
.changeset/tarball-url-binding.md
🔇 Additional comments (12)
.changeset/tarball-url-binding.md (1)
1-14: LGTM!pacquet/crates/package-manager/src/build_resolution_verifiers.rs (1)
126-126: LGTM!resolving/default-resolver/src/index.ts (1)
236-236: LGTM!resolving/resolver-base/src/index.ts (1)
114-121: LGTM!installing/deps-installer/src/install/verifyLockfileResolutions.ts (2)
231-236: LGTM!Also applies to: 258-258, 273-273, 281-281
252-252: ⚡ Quick win
nameVerFromPkgSnapshotalready populatesnonSemverVersion, so the npm verifier skip path won’t miss URL-keyed tarball deps.
lockfile/utils/src/nameVerFromPkgSnapshot.tssetsnonSemverVersion: pkgInfo.nonSemverVersionfromdp.parse(depPath).
deps/path/src/index.tsonly fillsnonSemverVersionwhen the “version” portion isn’tsemver.valid(e.g. when it’s a tarball URL/key), otherwise it returns a semverversion.
resolving/npm-resolver/src/createNpmResolutionVerifier.tsshort-circuits withif (nonSemverVersion != null) return { ok: true }, and there’s a unit test asserting URL-keyed tarball deps are skipped even with a semverversion.> Likely an incorrect or invalid review comment.pacquet/crates/resolving-npm-resolver/src/violation_codes.rs (1)
12-12: LGTM!resolving/npm-resolver/src/violationCodes.ts (1)
13-13: LGTM!pacquet/crates/resolving-npm-resolver/src/lookup_context.rs (1)
18-18: LGTM!Also applies to: 30-47
pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs (1)
42-45: LGTM!Also applies to: 370-399, 578-579, 971-1005
resolving/npm-resolver/src/createNpmResolutionVerifier.ts (1)
20-29: LGTM!Also applies to: 116-116, 179-223, 340-382, 695-695, 767-794
pacquet/crates/package-manager/src/install/tests.rs (1)
1370-1370: LGTM!Also applies to: 1471-1471, 1728-1728, 1833-1833, 4800-4800
bf0bc34 to
2a532bf
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs (1)
1008-1014: 💤 Low valueConsider case-insensitive handling for
%2fencoding.URL percent-encoding is case-insensitive per RFC 3986, so
%2Fand%2fare semantically equivalent. The current normalization only handles lowercase. While most implementations use lowercase, a mismatch in casing could cause a false positive.If upstream
toLockfileResolutiononly handles lowercase, this is consistent and can be left as-is. Otherwise, consider:♻️ Suggested fix
fn canonical_tarball_url(url: &str) -> String { - let normalized = url.replace("%2f", "/"); + let normalized = url.replace("%2f", "/").replace("%2F", "/"); match normalized.split_once("://") { Some((_scheme, rest)) => rest.to_string(), None => normalized, } }🤖 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/resolving-npm-resolver/src/create_npm_resolution_verifier.rs` around lines 1008 - 1014, The function canonical_tarball_url should treat percent-encoding case-insensitively; currently canonical_tarball_url only replaces the lowercase "%2f", which misses "%2F" and can cause mismatches. Update canonical_tarball_url to normalize both cases (e.g., replace "%2f" and "%2F") or better yet percent-decode the path portion before returning so all equivalent encodings collapse; locate the canonical_tarball_url function and implement the replacement or decoding there so the returned string is case-insensitively normalized.
🤖 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/resolving-npm-resolver/src/create_npm_resolution_verifier.rs`:
- Around line 1008-1014: The function canonical_tarball_url should treat
percent-encoding case-insensitively; currently canonical_tarball_url only
replaces the lowercase "%2f", which misses "%2F" and can cause mismatches.
Update canonical_tarball_url to normalize both cases (e.g., replace "%2f" and
"%2F") or better yet percent-decode the path portion before returning so all
equivalent encodings collapse; locate the canonical_tarball_url function and
implement the replacement or decoding there so the returned string is
case-insensitively normalized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a4742c1f-b3e7-4763-bb03-9a5185c52ab8
📒 Files selected for processing (14)
.changeset/tarball-url-binding.mdinstalling/deps-installer/src/install/verifyLockfileResolutions.tspacquet/crates/cli/tests/lockfile_resolution_reuse.rspacquet/crates/package-manager/src/build_resolution_verifiers.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/resolving-npm-resolver/src/violation_codes.rsresolving/default-resolver/src/index.tsresolving/npm-resolver/src/createNpmResolutionVerifier.tsresolving/npm-resolver/src/violationCodes.tsresolving/npm-resolver/test/createNpmResolutionVerifier.test.tsresolving/resolver-base/src/index.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/tarball-url-binding.md
🚧 Files skipped from review as they are similar to previous changes (9)
- resolving/default-resolver/src/index.ts
- pacquet/crates/resolving-npm-resolver/src/violation_codes.rs
- resolving/npm-resolver/src/violationCodes.ts
- pacquet/crates/package-manager/src/build_resolution_verifiers.rs
- resolving/resolver-base/src/index.ts
- resolving/npm-resolver/src/createNpmResolutionVerifier.ts
- resolving/npm-resolver/test/createNpmResolutionVerifier.test.ts
- installing/deps-installer/src/install/verifyLockfileResolutions.ts
- pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/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). (7)
- GitHub Check: Code Coverage
- 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: Lint and Test (macos-latest)
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (2)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: Log emissions are part of matching pnpm — when porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/cli/tests/lockfile_resolution_reuse.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/package-manager/src/install/tests.rs
pacquet/**/tests/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/tests/**/*.rs: Prefer real fixtures over dependency-injection seams — usetempfile::TempDir, the mocked registry, or integration tests spawning the actual binary for happy paths and most error paths; use the DI seam only for filesystem error kinds, deterministic time, shared process-global state, or external-service happy paths
Follow test-logging guidance — log before non-assert_eq!assertions,dbg!complex structures, skip logging for simple scalarassert_eq!
Tests must not be tolerant of missing build/runtime environment by silently returning early when a tool isn't found — let the test panic when a required tool is absent
Prefer#[cfg_attr(target_os = "windows", ignore = "...")](or matching#[cfg(unix)]gates) over runtime probe-and-skip helpers for platform-locked tools
Port relevant pnpm tests to Rust tests whenever they translate when porting behavior from pnpm
Use snapshot tests withinstaand carefully review diffs when intentional changes alter snapshots; accept withcargo insta reviewonly after careful review
Tests that need the mocked registry should startpnprthroughpacquet-testing-utils;cargo test/cargo nextest runshould not require a separatejust registry-mock launchstep
Files:
pacquet/crates/cli/tests/lockfile_resolution_reuse.rs
🧠 Learnings (3)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/cli/tests/lockfile_resolution_reuse.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.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/cli/tests/lockfile_resolution_reuse.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.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/tests/lockfile_resolution_reuse.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/package-manager/src/install/tests.rs
🔇 Additional comments (10)
pacquet/crates/package-manager/src/install/tests.rs (2)
1791-1796: Update this rationale to the new verifier behavior.It still says the verifier is active “via
minimumReleaseAge”, but this path now runs whenevertrust_lockfileisfalse. Reword the comment sotrust_lockfileis the opt-out andTARBALL_URL_MISMATCHstays the preempting failure it is here.
1370-1370: LGTM!Also applies to: 1471-1471, 1728-1728, 1833-1833, 2961-2967, 4800-4800
pacquet/crates/cli/tests/lockfile_resolution_reuse.rs (1)
50-58: LGTM!pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs (6)
158-168: Past review comment remains applicable.The policy snapshot still lacks a marker for the tarball URL binding rule. The existing comment at this location correctly identifies that old cache entries could bypass the new
TARBALL_URL_MISMATCHenforcement.
304-318: Past review comment remains applicable.The
ctx.non_semver_versionescape hatch mentioned in the PR objectives is still not checked before the tarball URL binding logic. The existing comment correctly identifies that URL-keyed tarballs with valid semver versions could be incorrectly treated as registry-pinned entries.
7-13: LGTM!Also applies to: 44-47
372-408: Clarify intended behavior: fail-closed vs. fail-open.The doc comment and code implement fail-closed (reject when metadata unavailable), but the PR objectives state: "it fails only on a confirmed mismatch; if metadata is unavailable or the version is not listed, the check passes to avoid transient/regression/offline failures."
The fail-closed implementation has security merit (per the doc comment's reasoning), but this contradicts the documented PR behavior. Either:
- Update PR description to reflect fail-closed if this is the intended final design, or
- Change lines 402-406 to return
None(pass) when registry metadata is unavailable, matching the stated objectives.
587-590: LGTM!
980-997: LGTM!pacquet/crates/resolving-npm-resolver/src/lookup_context.rs (1)
30-47: LGTM!
2a532bf to
687de3b
Compare
|
Thanks for the thorough review. Addressed in Fixed
Not changed (with rationale)
All suites green locally ( Written by an agent (Claude Code, claude-opus-4-8). |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/verifyLockfileResolutions.ts`:
- Around line 252-258: The dedupe key for candidates currently uses name,
version, and JSON.stringify(snapshot.resolution) but omits nonSemverVersion,
causing different snapshot types to collide; update the key generation (where
key is defined) to incorporate nonSemverVersion (or a boolean flag indicating
its presence) so entries with different nonSemverVersion values produce distinct
keys—look for nameVerFromPkgSnapshot, the candidates.set call, and the key
variable near DepPath/snapshot usage and include nonSemverVersion in that key
construction.
In `@resolving/npm-resolver/src/createNpmResolutionVerifier.ts`:
- Around line 189-202: The prefix-matching used to choose a named registry
before the tarball check needs to apply the same canonicalization as the final
tarball comparison; normalize the registry prefixes (or the tarballUrl) with the
same logic used by sameTarballUrl() before calling pickRegistryForVersion so an
http/https-only difference doesn't cause pickRegistryForVersion to pick the
wrong packument. Update the call site around pickRegistryForVersion (and/or the
namedRegistryPrefixes passed into it) so you canonicalize the prefixes/URL the
same way as the final comparison, ensuring runTarballUrlCheck receives the
registry selected using canonicalized routing.
🪄 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: d2e76b57-e5f4-461b-9b10-bf93087453d8
📒 Files selected for processing (14)
.changeset/tarball-url-binding.mdinstalling/deps-installer/src/install/verifyLockfileResolutions.tspacquet/crates/cli/tests/lockfile_resolution_reuse.rspacquet/crates/package-manager/src/build_resolution_verifiers.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/resolving-npm-resolver/src/violation_codes.rsresolving/default-resolver/src/index.tsresolving/npm-resolver/src/createNpmResolutionVerifier.tsresolving/npm-resolver/src/violationCodes.tsresolving/npm-resolver/test/createNpmResolutionVerifier.test.tsresolving/resolver-base/src/index.ts
✅ Files skipped from review due to trivial changes (2)
- resolving/npm-resolver/src/violationCodes.ts
- .changeset/tarball-url-binding.md
🚧 Files skipped from review as they are similar to previous changes (8)
- resolving/default-resolver/src/index.ts
- resolving/resolver-base/src/index.ts
- pacquet/crates/package-manager/src/build_resolution_verifiers.rs
- pacquet/crates/resolving-npm-resolver/src/violation_codes.rs
- pacquet/crates/cli/tests/lockfile_resolution_reuse.rs
- pacquet/crates/resolving-npm-resolver/src/lookup_context.rs
- pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs
- pacquet/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). (7)
- GitHub Check: Agent
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:
resolving/npm-resolver/test/createNpmResolutionVerifier.test.tsresolving/npm-resolver/src/createNpmResolutionVerifier.tsinstalling/deps-installer/src/install/verifyLockfileResolutions.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use util.types.isNativeError() instead of instanceof Error for error type checking in Jest tests
Files:
resolving/npm-resolver/test/createNpmResolutionVerifier.test.ts
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: Log emissions are part of matching pnpm — when porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs
🧠 Learnings (5)
📚 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:
resolving/npm-resolver/test/createNpmResolutionVerifier.test.tsresolving/npm-resolver/src/createNpmResolutionVerifier.tsinstalling/deps-installer/src/install/verifyLockfileResolutions.ts
📚 Learning: 2026-05-23T17:29:56.247Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11878
File: resolving/npm-resolver/src/createNpmResolutionVerifier.ts:381-418
Timestamp: 2026-05-23T17:29:56.247Z
Learning: When reviewing the npm resolver code, note that `PackageMetaCache` is intentionally keyed only by `name` and `name:full` (no registry component). As a result, code that shares this cache (e.g., `createNpmResolutionVerifier.ts` via the shared `validateSharedMeta` guard) can prevent cross-package contamination by matching names, but it cannot distinguish two different registries that serve packages with the same name within a single install. Don’t flag (or attempt to fix) this as a local issue in the verifier alone—correctly distinguishing registries would require a coordinated change to the resolver cache key shape. (The Pacquet/Rust cache is already registry-qualified, unlike the npm-resolver cache.)
Applied to files:
resolving/npm-resolver/src/createNpmResolutionVerifier.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/resolving-npm-resolver/src/create_npm_resolution_verifier/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/resolving-npm-resolver/src/create_npm_resolution_verifier/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/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs
🔇 Additional comments (2)
pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs (1)
159-205: LGTM!Also applies to: 207-232, 282-382, 630-676, 735-752
resolving/npm-resolver/test/createNpmResolutionVerifier.test.ts (1)
48-76: LGTM!Also applies to: 201-208, 268-389, 398-407
The lockfile verifier now checks that a registry entry pinning an explicit `tarball` URL points at the artifact the registry's own metadata lists for that `name@version`. Previously a tampered lockfile could pair a trusted `name@version` with an attacker-chosen tarball URL (and a matching integrity for those bytes), so the install fetched the attacker's bytes. A mismatch — or any entry that can't be confirmed against the registry — is rejected with `ERR_PNPM_TARBALL_URL_MISMATCH`. Non-standard registry tarball URLs (npm Enterprise, GitHub Packages) still pass because they match the metadata; differences that are pure normalization (http/https, default ports, `%2f` scope encoding) are canonicalized away rather than flagged, on both the final comparison and the named-registry routing. The binding is unconditional: it runs regardless of `minimumReleaseAge` / `trustPolicy` and is not narrowed by their exclude lists. Entries without an explicit tarball URL reconstruct it from name+version+registry and are inherently bound; `file:`/git-hosted resolutions are out of scope. It is fail-closed: an entry passes only when the registry metadata affirmatively lists the version with a matching tarball URL; otherwise it is rejected. Consequently an install that re-verifies a lockfile now requires the configured registry to be reachable; `trustLockfile` is the opt-out. The verification-cache policy snapshot records a `tarballUrlBinding` marker and `canTrustPastCheck` requires it, so a cache record written before this rule existed is re-verified rather than trusted. The candidate dedupe key includes `nonSemverVersion` so a registry snapshot and a URL-keyed tarball snapshot that share a `name@version` and serialized resolution stay distinct. Threading `nonSemverVersion` to the verifier also stops release-age/trust verification from being applied to URL-keyed tarball dependencies that carry a semver `version` copied from their manifest. Mirrored in pacquet. A few pacquet install-dispatch / resolution-reuse tests that pin a deliberately bogus tarball URL (or run against an unreachable registry) now set `trustLockfile` so the always-on, fail-closed tarball-URL check doesn't flag the fixture before the path under test runs.
687de3b to
9b2c453
Compare
|
Second round addressed in
All suites green locally ( Written by an agent (Claude Code, claude-opus-4-8). |
What
The lockfile resolution verifier now confirms that a registry entry pinning an explicit
tarballURL points at the artifact the registry's own metadata lists for thatname@version. A mismatch — or any entry that can't be confirmed against the registry — is rejected withERR_PNPM_TARBALL_URL_MISMATCH.Why
Follow-up to the design discussion on #12122. The verifier checked the age/trust of
name@versionagainst the registry packument but never bound the lockfile'starballURL to it. For the non-standard entries pnpm preserves a tarball URL for (npm Enterprise, GitHub Packages — seetoLockfileResolution), pnpm fetches straight from that URL. So a tampered lockfile could pair a trustedname@versionwith an attacker-chosen tarball URL (plus a matching integrity for the attacker's bytes); verification passed against the legitimate version while the install fetched the attacker's bytes. Defending a checked-in lockfile is explicitly in this feature's threat model.How
tarball, fetch the packument and assert the URL equalsversions[v].dist.tarball. The comparison canonicalizes away benign differences — http/https scheme, default ports (:443/:80), and%2fscope-separator encoding (case-insensitive) — so only real mismatches are flagged. The packument is fetched from the user's configured registry (the lockfile's tarball host can't redirect it), and named-registry routing uses the same canonicalization so a scheme/%2f-only difference doesn't route to the wrong packument.minimumReleaseAge/trustPolicyand is not narrowed by their exclude lists, because it guards integrity, not maturity/trust. Disabling the age/trust policies must not silently disable anti-tamper. (createNpmResolutionVerifiertherefore always returns a verifier.)dist.tarball, the entry is rejected — otherwise a tampered lockfile could smuggle a malicious URL past the check by pointing it at aname@versionthe registry can't vouch for.trustLockfileis the opt-out for environments that treat the on-disk lockfile as already trusted.tarballUrlBindingmarker andcanTrustPastCheckrequires it, so a cache record written before this rule existed is re-verified rather than trusted (closing an upgrade-time bypass).tarballreconstruct the URL from name+version+registry and are inherently bound (no check).file:/git-hosted resolutions stay out of scope (fix: skip lockfileminimumReleaseAge/trustPOlicyverification for non-registry tarball #12122).nonSemverVersionto the verifier so URL-keyed tarball deps (a remotehttps:tarball that carries a semverversioncopied from its manifest) are recognized as deliberate non-registry deps and skipped — also fixing a latent release-age over-match on them. The candidate dedupe key includesnonSemverVersionso a registry snapshot and a URL-keyed snapshot sharing aname@versionand serialized resolution stay distinct.Mirrored in pacquet (
create_npm_resolution_verifier). The dedupe-key change is TS-only: pacquet's candidateversioncomes from the lockfile key suffix, so the two shapes never share a key there.Tests
canTrustPastCheckrejects a cache record lacking the binding marker. Regression-verified (the mismatch test fails when the check is disabled).minimumReleaseAge: 0/trustPolicy: offcases, default-port/scheme equivalence, and the missing-tarballUrlBindingcache rejection. A few install-dispatch / resolution-reuse tests that pin a deliberately bogus tarball URL (or run against an unreachable registry to prove resolution reuse) now settrustLockfile, since the always-on fail-closed tarball-URL check would otherwise flag the fixture before the path under test runs.clippy --deny warnings,fmt, anddylintclean.Closes the gap discussed in #12122.
Written by an agent (Claude Code, claude-opus-4-8).