feat(pacquet): port lockfile verification (minimumReleaseAge + trustPolicy)#11729
Conversation
📝 WalkthroughWalkthroughAdds a lockfile verification gate: resolver-base trait, npm resolution verifier (attestation-first publish-time, trust no-downgrade), verification runner with JSONL cache and compaction, on-disk packument mirror, config/workspace policy fields, registry model updates, reporter events, and Install/CLI integration with tests. ChangesLockfile Verification Flow
Estimated code review effort Possibly related issues
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Mirrors upstream's lockfileVerificationLogger so pacquet can emit the started / done / failed events the lockfile-verification gate produces during install. The runner that fires these events lands in a follow-up phase; this change adds only the wire shape and its tests, leaving emit-call-sites for later. Phase 1 of #11722.
Adds pacquet-resolving-resolver-base, a small crate holding the ResolutionVerifier trait, ResolutionVerification enum, and ResolutionPolicyViolation struct ported from @pnpm/resolving.resolver-base. The trait + violation type live in their own crate (not in the verification runner) so future verifier crates - npm today, custom resolvers tomorrow - can depend on the trait without pulling in the runner, matching upstream's layering. verify() returns a hand-rolled Pin<Box<dyn Future + Send + '_>> so the trait stays dyn-compatible without an async-trait dep; the runner stores its verifier list as &[Box<dyn ResolutionVerifier>] to fan out across a heterogeneous set. Phase 1 of #11722.
The lockfile-verification gate (#11722) needs RFC3339 date parsing for the npm verifier's release-age and trust-policy checks. chrono is already in the workspace as a dev-dep; this moves the same entry into [workspace.dependencies] so the forthcoming pacquet-resolving-npm-resolver crate can depend on it without a new third-party crate selection. No production code consumes chrono yet; the runtime dep slot is added under the rule that new third-party deps need explicit approval (which the user gave in the planning round for #11722).
Ports the seven config keys the upstream lockfile-verification gate reads from pnpm-workspace.yaml plus the cacheDir field they share: - cacheDir (default resolved per upstream getCacheDir's XDG / platform / LOCALAPPDATA chain) - minimumReleaseAge, minimumReleaseAgeExclude, minimumReleaseAgeIgnoreMissingTime, minimumReleaseAgeStrict - trustPolicy (new TrustPolicy enum: off | no-downgrade) - trustPolicyExclude, trustPolicyIgnoreAfter Also moves the version-policy parser into pacquet-config and adds the matcher-based createPackageVersionPolicy companion to the already-ported expandPackageVersionSpecs. The matcher variant is what minimumReleaseAgeExclude and trustPolicyExclude evaluate against: wildcard name patterns (is-*, @scope/*) plus exact-version unions (lodash@4.17.21 || 4.17.22). expandPackageVersionSpecs keeps its callers via a re-export shim in pacquet-package-manager. Phase 2 of #11722.
…ions Extends Package, PackageVersion, and PackageDistribution with the fields the lockfile-verification verifier consults: - Package gains `time` (per-version publish timestamps, HashMap<String, serde_json::Value> so the reserved `unpublished` object key round-trips alongside the version-keyed strings), `modified` (abbreviated-metadata last-modified shortcut), and `etag` (kept for Phase 5's conditional-GET fetcher). - PackageVersion gains `_npmUser.trustedPublisher` via a new NpmUser + TrustedPublisher type pair. - PackageDistribution gains `attestations.provenance` via new AttestationsDist + ProvenanceMeta types. A small Package::published_at helper resolves the per-version lookup while filtering the reserved `unpublished` object value that lives in the same map. Four new fixture-based deserialize tests pin the wire shapes: full-provenance packument, no-_npmUser / no-attestations fallback, missing time field, and the `unpublished` reserved-key skip. Existing PackageVersion / Package construction sites in tests fill the new fields explicitly so the breaking field additions don't ripple into runtime callers (which don't build these structs by hand). Phase 3 of #11722.
…registry Creates the new pacquet-resolving-npm-resolver crate and lands its network-side helpers ahead of the verifier itself: - violation_codes: MINIMUM_RELEASE_AGE_VIOLATION + TRUST_DOWNGRADE consts (verbatim from upstream). - named_registry: BUILTIN_NAMED_REGISTRIES (`gh:` builtin) + build_named_registry_prefixes (longest-first sorted, trailing-slash normalized) + pick_registry_for_version (tarball-URL match first, scope-routing fallback). - fetch_full_metadata: no-cache `Accept: application/json` fetch targeting the full-metadata endpoint so `time`, `_npmUser`, and `dist.attestations` come back populated. Cached variant lands in Phase 5. - fetch_attestation_published_at: port of upstream's Sigstore-bundle parser hitting `/-/npm/v1/attestations/<name>@<version>`; earliest Rekor `integratedTime` wins; non-2xx, malformed body, network errors all collapse to `Ok(None)` so the verifier falls through to the next layer. - errors: FetchMetadataError::Network for the underlying reqwest failures the verifier folds into violation reasons. 15 mockito-driven unit tests pin the wire contracts: auth-header attachment, full-vs-abbreviated endpoint targeting, 404/5xx/ malformed-body fall-through, earliest-bundle selection, the `integratedTime` string-or-number defensive parse, trailing-slash trim on the registry root, longest-first prefix ordering, and the near-miss URL rejection on the named-registry router. The verifier (create_npm_resolution_verifier) and trust_checks land in follow-up commits this phase. Phase 4 (slice 1 of 2) of #11722.
Adds trust_checks.rs — the rank-based supply-chain-evidence walk that decides whether a lockfile version's `_npmUser.trustedPublisher` or `dist.attestations.provenance` is weaker than the strongest evidence seen on any earlier-published version of the same package. - TrustEvidence enum (Provenance < TrustedPublisher). - TrustViolation with TrustDowngrade + TrustCheckFailed variants mirroring upstream's PnpmError codes. - TrustCheckOptions threads trustPolicyExclude (a PackageVersionPolicy built in Phase 2), trustPolicyIgnoreAfter (minutes cutoff), and an injectable `now` for deterministic ignore-after tests. - fail_if_trust_downgraded ports the upstream control flow one-for-one: exclude-policy short-circuit, time + manifest lookups, the prerelease guard, history walk, rank compare. - get_trust_evidence ports upstream's two-line accessor for reuse by future consumers. 14 fixture-based tests cover: first-version pass, trusted-publisher → provenance downgrade, provenance → unsigned downgrade, equal-rank pass, rank upgrade, later-publish-doesn't-affect- earlier, stable-target-ignores-prerelease-history, prerelease- target-sees-prerelease-history, ignore-after-skips, ignore-after- still-checks-fresh, exclude-any-version, exclude-exact-version, missing-time TrustCheckFailed, unparseable-timestamp TrustCheckFailed. Phase 4 (slice 2 of 2 — main verifier next) of #11722.
Adds the `NpmResolutionVerifier` that re-applies `minimumReleaseAge` and `trustPolicy='no-downgrade'` to every npm-registry-resolved lockfile entry, plus the factory that returns `None` when no policy is active. Implements the `ResolutionVerifier` trait from `pacquet-resolving-resolver-base`: `verify` short-circuits on non-registry resolutions and non-semver versions, dispatches the age / trust checks, and surfaces violations through `ResolutionVerification::Err` with the `MINIMUM_RELEASE_AGE_VIOLATION` or `TRUST_DOWNGRADE` code. `policy()` returns the JSON snapshot the verification cache hashes; `can_trust_past_check` accepts a looser maturity cutoff but demands byte-exact match on every exclude list and the trust policy. The publish-timestamp lookup walks the attestation endpoint, then the full packument. The abbreviated-modified shortcut and the on-disk-mirror layer land in Phase 5 alongside the cached fetcher they depend on; their slots in `PublishedAtLookupContext` will be added at the same time. Ports `createNpmResolutionVerifier.ts` from upstream pnpm at `2a9bd897bf`.
Adds the on-disk packument mirror that pacquet shares with pnpm. The verifier now consults a JSONL mirror at `<cache_dir>/v11/metadata-full/<registry-slug>/<encoded-name>.jsonl`, issues a conditional GET (If-None-Match / If-Modified-Since), and either serves the cached body on 304 or writes the new body back on 200. Without `cache_dir` the call falls through to an unconditional GET — same behavior as the no-cache fetcher. The path / IO helpers (`encode_pkg_name`, `get_registry_name`, `get_pkg_mirror_path`, `prepare_json_for_disk`, `load_meta_headers`, `load_meta`, `save_meta`) and the `FULL_META_DIR` / `ABBREVIATED_META_DIR` constants live in a new `mirror` module so the abbreviated-cache fast path (and any future `pickPackage` work) can reuse them. Constants and directory layout are byte-identical to pnpm's so a mirror populated by one stack works for the other. Two new error variants on `FetchMetadataError` mirror upstream's codes: `META_NOT_MODIFIED_WITHOUT_CACHE` and `META_CACHE_MISSING_AFTER_304`. Wires `create_npm_resolution_verifier` to the cached fetcher with a new `cache_dir: Option<PathBuf>` field on the options bag. Ports `fetchFullMetadataCached.ts` and the cache helpers from `pickPackage.ts` at upstream commit `2a9bd897bf`.
Adds the new `pacquet-lockfile-verification` crate that hosts the fan-out gate pnpm runs immediately after loading a lockfile, before any resolution decision or fetch. Walks every entry in `lockfile.packages`, dedupes by `(name, version, resolution)`, and asks every active `ResolutionVerifier` to evaluate each candidate. Verifiers handle their own protocol short-circuit so the runner stays policy-neutral. This slice ports the runner-side surface only: - `verify_lockfile_resolutions::<R: Reporter>` — throw-mode entry point. Concurrency-capped fan-out via tokio Semaphore + futures-util FuturesUnordered, sort + cap violations at 20, escalate to `LOCKFILE_RESOLUTION_VERIFICATION` on mixed-code batches. `Started` / `Done` / `Failed` reporter events pair via a drop guard so a panic can never leave a hanging "Verifying…" frame. - `collect_resolution_policy_violations` — data-returning sibling for auto-collect / strict-prompt callers. - `hash_lockfile` — sha256-hex content hash of the in-memory Lockfile. Normalises HashMap key order through a sorted serde_json value so two lockfiles that deserialise to the same shape hash the same. Hex output (not base64) — pacquet's cache reads its own records out of the shared JSONL; cross-stack hash compat isn't required for the cache contract. - `VerifyError` — three diagnostic variants (`MINIMUM_RELEASE_AGE_VIOLATION`, `TRUST_DOWNGRADE`, `LOCKFILE_RESOLUTION_VERIFICATION`) carrying the rendered breakdown. Help text matches pnpm. The on-disk JSONL cache (`tryLockfileVerificationCache` / `recordVerification`) lands in a follow-up slice. Ports `verifyLockfileResolutions.ts` at upstream commit `2a9bd897bf`.
Adds the on-disk cache that lets repeat installs against an
unchanged lockfile (under the same or stricter policy) skip the
per-package registry round-trips entirely.
Cache layout matches pnpm at `<cache_dir>/lockfile-verified.jsonl`,
one JSON record per line:
{ "lockfile": { "hash", "path", "size", "mtimeNs", "inode" },
"verifiedAt": "...", "policy": { ... } }
Two indexes share the same records:
- **by content hash** — primary lookup. Catches the worktree /
CI-checkout case where the same lockfile content lives at a
different path.
- **by absolute path** — same-machine stat shortcut. When the
cached entry's `(size, mtime_ns, inode)` matches the current
stat, the cached hash is still correct and the gate skips
reading the lockfile entirely.
Policy snapshot trust is verifier-driven: every active
`ResolutionVerifier::can_trust_past_check` must agree the cached
snapshot still covers what it currently demands; any rejection
falls through to the full gate.
Compaction kicks in when the file passes ~1.5 MB:
reverse-walk → dedupe by (path, hash) → keep newest →
trim to MAX_CACHE_ENTRIES (1000) → write to temp + rename.
Single-line appends are atomic on POSIX and NTFS so parallel pnpm
/ pacquet processes (monorepo installs, CI matrices sharing a
cache) can write without coordination.
`record_lockfile_verified` provides a thin wrapper for callers
that hold a freshly-written lockfile and want to mark it as
already-verified for the next install.
Wires the cache into `verify_lockfile_resolutions`:
- `cache_dir` + `lockfile_path` on the options bag activates it.
- Lookup runs before the verifier fan-out; a hit returns Ok
without emitting Started/Done events.
- A miss reuses the precomputed stat + hash on the matching
recordVerification call.
- Empty-candidate runs still record a success so the next install
can stat-only the lockfile.
Ports `verifyLockfileResolutionsCache.ts` and
`recordLockfileVerified.ts` at upstream commit `2a9bd897bf`.
Adds the install-side wiring upstream pnpm has at [`installing/deps-installer/src/install/index.ts:355-383`](https://github.com/pnpm/pnpm/blob/2a9bd897bf/installing/deps-installer/src/install/index.ts#L355-L383): the gate runs immediately after `Lockfile::load_current_from_virtual_store_dir` and before the `pnpm:context` emit, only when `lockfile.is_some()`. Pipeline: Install::run └─ build_resolution_verifiers(&Config, Arc<ThrottledClient>) └─ create_npm_resolution_verifier(opts) -> Option<NpmResolutionVerifier> └─ verify_lockfile_resolutions::<Reporter>(lockfile, &verifiers, &opts) When the user hasn't set `minimumReleaseAge` or `trustPolicy`, the verifier list is empty and the runner short-circuits — zero overhead on installs without policy. New surface: - `pacquet_package_manager::build_resolution_verifiers` — assembles the verifier list from `Config`. Maps `Config::registry` into the single-default `{"default": url}` map the npm verifier expects; `named_registries` stays empty pending the npmrc wiring for it. - `InstallError::LockfileVerification(VerifyError)` — transparent miette diagnostic so the inner `MINIMUM_RELEASE_AGE_VIOLATION` / `TRUST_DOWNGRADE` / `LOCKFILE_RESOLUTION_VERIFICATION` code is what the user sees, matching pnpm byte-for-byte. - `InstallError::BuildVerifiers(BuildVerifiersError)` — surfaces an invalid `minimumReleaseAgeExclude` / `trustPolicyExclude` pattern before the gate runs. Plumbing: - `Install` and `Add` gain `lockfile_path: Option<&'a Path>` and `http_client_arc: Arc<ThrottledClient>` fields. The CLI derives both from existing state — `lockfile_path` from `manifest.path()`'s parent (becomes `workspace_root` once workspace support lands), `http_client_arc` from a `State::http_client` field that's now held behind `Arc` so the verifier can own a clone.
Two new tests pin the user-visible contract for the gate wired in the previous slice: **install layer** (`pacquet-package-manager`): - `install_rejects_invalid_minimum_release_age_exclude_pattern` — proves `build_resolution_verifiers` runs from inside `Install::run` by feeding it an invalid exclude pattern and asserting `InstallError::BuildVerifiers`. Sub-second; no registry round-trip. - `frozen_lockfile_gate_rejects_under_huge_minimum_release_age` — positive-path proof that the verifier actually runs end-to-end. With `minimumReleaseAge` set to 100 years against the mocked registry, every lockfile entry trips the cutoff and the install fails with `InstallError::LockfileVerification(MinimumReleaseAgeViolation)` before any tarball is fetched. **CLI layer** (`pacquet-cli`): - New `tests/lockfile_verification.rs` spawning the actual binary via `CommandTempCwd`. Writes `pnpm-workspace.yaml` with the upstream-canonical `minimumReleaseAge` key, a pinning `pnpm-lock.yaml`, and a matching `package.json`. Asserts non-zero exit and that stderr contains the literal `MINIMUM_RELEASE_AGE_VIOLATION` so `pnpm errors` URL routing and log consumers both work. Both layers verify the gate short-circuits before any virtual-store materialization.
…shots **TEST_PORTING.md.** New "Lockfile Verification Gate" section enumerates every upstream test the gate work mirrored, with a [x]/[ ] checkbox per item and the Rust-side test name. Covers: - `trustChecks.test.ts` — 14 of 16 tests pinned in `pacquet-resolving-npm-resolver`'s `trust_checks::tests`. - `fetchAttestationPublishedAt.test.ts` — 7 tests pinned in `fetch_attestation_published_at::tests`. - `createNpmResolutionVerifier.test.ts` — 5 of 6 tests pinned; the abbreviated-shortcut case stays unchecked until that layer is ported. - `fetchFullMetadataCached` — the four Phase 5 mockito scenarios. - `verifyLockfileResolutions.ts` runner — 11 of 14 pinned; three dedupe / cache-write-on-failure cases still need explicit Rust-side tests. - `verifyLockfileResolutionsCache.ts` — 7 of 14 pinned; the stat-shortcut size/mtime-drift variants and the multi-verifier merge happy-path are gaps. - `recordLockfileVerified.ts` — wrapper guards covered; the load-equivalent-hash and git-branch lockfile cases are gaps. - `minimumReleaseAge.ts` install-side — 3 of 8 pinned, plus Rust port notes flagging the resolver-dependent fall-back branches as out of scope until pacquet has a resolver. - `config/version-policy/test/index.ts` — covered via the upstream version-policy port + the install-side `BuildVerifiersError` wrapper. **Diagnostic snapshots.** Three new insta snapshots in `errors::tests` pin the rendered Display text for the three violation shapes a user actually sees: - `single_entry_single_code` — one immature pin, per-policy code on the envelope. - `three_entries_single_code` — three immature pins, single-column breakdown (no per-line code prefix). - `three_entries_mixed_codes` — mixed batch, escalated envelope code, `[CODE]` prefix per line so each entry's policy is visible. Mirrors upstream `PnpmError` Display shape byte-for-byte, including the "1 lockfile entries" pluralization quirk in `verifyLockfileResolutions.ts:195`.
96b80d7 to
ac95e79
Compare
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11729 +/- ##
==========================================
+ Coverage 89.73% 89.88% +0.14%
==========================================
Files 129 144 +15
Lines 14973 16416 +1443
==========================================
+ Hits 13436 14755 +1319
- Misses 1537 1661 +124 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.46295833142,
"stddev": 0.09601062476765562,
"median": 2.4363146483199998,
"user": 2.6587926199999994,
"system": 3.7840780800000005,
"min": 2.38120435782,
"max": 2.71451939582,
"times": [
2.71451939582,
2.43553283382,
2.42002761082,
2.50277656682,
2.38120435782,
2.43709646282,
2.48078918282,
2.4533655368200002,
2.3891472948200003,
2.41512407182
]
},
{
"command": "pacquet@main",
"mean": 2.38530443632,
"stddev": 0.06000941863966295,
"median": 2.38561646632,
"user": 2.63072742,
"system": 3.71921018,
"min": 2.28888112582,
"max": 2.4775389538200003,
"times": [
2.4330095978200004,
2.31879538682,
2.35545183482,
2.4426053428200003,
2.34144331182,
2.42408587682,
2.4775389538200003,
2.3979394328200003,
2.3732934998200004,
2.28888112582
]
},
{
"command": "pnpm",
"mean": 4.85386434842,
"stddev": 0.0441834257254969,
"median": 4.847314497819999,
"user": 8.27087462,
"system": 4.185005479999999,
"min": 4.78663152682,
"max": 4.9156721038199995,
"times": [
4.81727505482,
4.84123816382,
4.9156721038199995,
4.82565632582,
4.91412524982,
4.88842112782,
4.88008051182,
4.8161525878199996,
4.78663152682,
4.85339083182
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6873426244,
"stddev": 0.05630086491385367,
"median": 0.6711006559000001,
"user": 0.38705597999999997,
"system": 1.53871672,
"min": 0.6589826084000001,
"max": 0.8464335334,
"times": [
0.8464335334,
0.6744955214,
0.6827645774000001,
0.6645330834000001,
0.6629854884,
0.6715735384000001,
0.6589826084000001,
0.6736728044000001,
0.6706277734,
0.6673573154000001
]
},
{
"command": "pacquet@main",
"mean": 0.6840045072000001,
"stddev": 0.03066679918064518,
"median": 0.6752058159000001,
"user": 0.38376738000000005,
"system": 1.5390222199999999,
"min": 0.6510623824,
"max": 0.7479076094000001,
"times": [
0.7479076094000001,
0.6990104164000001,
0.6771936174000001,
0.6600421374000001,
0.6552537834000001,
0.6900236094000001,
0.7186858304000001,
0.6676476714,
0.6732180144000001,
0.6510623824
]
},
{
"command": "pnpm",
"mean": 2.6448112404,
"stddev": 0.07293240762346315,
"median": 2.6307673604,
"user": 3.22260268,
"system": 2.19792502,
"min": 2.5671053474,
"max": 2.8329690494,
"times": [
2.6201962934,
2.6513602684,
2.6567810443999997,
2.5942258244,
2.5671053474,
2.6661762054,
2.6335344304,
2.6280002904,
2.5977636504,
2.8329690494
]
}
]
} |
- typos: `unparseable` → `unparsable` (4 sites; `trust_checks`
tests, the cached fetcher, the trust-violation help text).
- rustdoc: tighten ambiguous intra-doc links — disambiguate
function-vs-module references with `()` where the function form
was intended, downgrade `[ssri::Integrity]` and `[load_meta*]`
links that pointed at items the consumer crate doesn't depend on
or that aren't part of the public surface, and fix
`cli::State::http_client`'s `Arc` link.
- dylint (perfectionist): rename single-letter closure / generic
params (`R` → `Reporter` matching the rest of the codebase;
`|v|`, `|p|`, `|t|`, `|d|`, `|l|`, `|r|`, `|s|`, `|m|` →
descriptive identifiers); reorder the `Debug, Clone, Default`
derives to the workspace's `prefix_then_alphabetical` style; add
trailing commas on multi-line macro invocations and remove
stray ones from single-line ones; switch escaped-quote strings
(`"W/\"v1\""`) to raw strings (`r#"W/"v1""#`).
- Windows test fix: `cache::tests::malformed_lines_are_tolerated_on_read`
+ `compaction_dedupes_by_path_and_hash` previously interpolated
the lockfile path directly into JSON, which produced invalid
JSON on Windows (literal backslashes). Build the record via
`serde_json::to_string(&CacheRecord { .. })` so the path is
escaped at the JSON layer.
No behavior changes — every tweak is lint / portability hygiene.
There was a problem hiding this comment.
Pull request overview
Ports pnpm's lockfile-verification gate to pacquet. After loading pnpm-lock.yaml, every entry is re-checked against the resolver's policies (minimumReleaseAge, trustPolicy='no-downgrade') before any resolution or fetch occurs, so a lockfile resolved under a weaker policy cannot reach the filesystem. Successful runs are recorded in a JSONL stat-and-skip cache.
Changes:
- Introduces three new crates:
pacquet-resolving-resolver-base(trait + violation types),pacquet-resolving-npm-resolver(npm verifier, trust-rank, attestation/full-metadata fetchers, violation codes), andpacquet-lockfile-verification(runner, JSONL cache, in-memory lockfile hasher). - Extends
pacquet-config(new fields +TrustPolicyenum +createPackageVersionPolicy),pacquet-reporter(newLockfileVerificationchannel), andpacquet-registry(enrichedPackage/PackageVersionwithtime,_npmUser.trustedPublisher,dist.attestations,modified). - Wires the gate into
pacquet-package-manager::installbetween lockfile load and the frozen-lockfile dispatch.
Reviewed changes
Copilot reviewed 67 out of 68 changed files in this pull request and generated no comments.
Show a summary per file
Only the snippets below were surfaced in the diff context for this turn; the full PR is much larger (per the description: 3 new crates and 4 extended crates).
| File | Description |
|---|---|
pacquet/crates/resolving-resolver-base/Cargo.toml |
Declares the new resolver-base crate and its pacquet-lockfile + serde_json deps. |
pacquet/crates/resolving-resolver-base/src/lib.rs |
Defines ResolutionVerification, ResolutionPolicyViolation, VerifyCtx, VerifyFuture, and the ResolutionVerifier trait that both the npm verifier and the runner key off. |
pacquet/crates/resolving-resolver-base/src/tests.rs |
Unit tests covering the err round-trip, violation construction, and &dyn ResolutionVerifier dispatch via a stub. |
pacquet/crates/resolving-npm-resolver/src/violation_codes.rs |
Verbatim port of upstream violation-code constants (MINIMUM_RELEASE_AGE_VIOLATION, TRUST_DOWNGRADE). |
pacquet/crates/resolving-npm-resolver/src/trust_checks/tests.rs |
Trust-downgrade unit tests (provenance vs. trustedPublisher transitions, first-version baseline, etc.). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
pacquet/crates/cli/tests/lockfile_verification.rs (1)
14-15: ⚡ Quick winReplace wildcard re-export from
_utilswith explicit imports (or remove it if unused).Using
pub use _utils::*;makes symbol flow opaque and violates the module import rule.Suggested cleanup
-pub mod _utils; -pub use _utils::*; +mod _utils; +// use _utils::{specific_item_1, specific_item_2}; // if neededAs per coding guidelines, "Do not use star imports inside module bodies; use explicit imports like
use super::{Foo, bar}instead ofuse super::*;for modules you control".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pacquet/crates/cli/tests/lockfile_verification.rs` around lines 14 - 15, The module currently does a wildcard re-export with "pub use _utils::*;" which hides which symbols are exposed; replace it by explicitly re-exporting only the needed items from _utils (e.g., "pub use _utils::{ItemA, helper_fn}" listing the actual types/functions used) or remove the re-export entirely if nothing from _utils is required; locate the "pub mod _utils;" / "pub use _utils::*;" lines and update them to explicit "pub use _utils::{...}" entries matching the symbols referenced by the tests in this file.pacquet/crates/resolving-npm-resolver/src/trust_checks/tests.rs (1)
257-264: ⚡ Quick winThis assertion doesn’t prove non-excluded versions are actually checked.
Line 258 uses
1.0.0, which passes even if exclusion were mistakenly applied package-wide. Use a non-excluded version that should downgrade, then assert it fails.✅ Stronger assertion pattern
- // A different version of the same package is still checked. - let err = fail_if_trust_downgraded(&meta, "1.0.0", &opts).err(); - // 1.0.0 has trusted-publisher itself, so the check still passes - // even though it's not excluded — the exclude policy only - // matters when there'd otherwise be a downgrade. This test pins - // that the exclude is targeted, not blanket. - assert!(err.is_none(), "1.0.0 has its own trusted-publisher → passes"); + // A different (non-excluded) version should still be checked and fail on downgrade. + let meta = make_package( + "acme", + &[ + ("1.0.0", "2025-01-01T00:00:00.000Z", Evidence::TrustedPublisher), + ("1.1.0", "2025-02-01T00:00:00.000Z", Evidence::None), // excluded + ("1.2.0", "2025-03-01T00:00:00.000Z", Evidence::None), // not excluded + ], + ); + let err = fail_if_trust_downgraded(&meta, "1.2.0", &opts) + .expect_err("non-excluded version should still be checked"); + assert!(matches!(err, TrustViolation::TrustDowngrade { .. }), "got {err:?}");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pacquet/crates/resolving-npm-resolver/src/trust_checks/tests.rs` around lines 257 - 264, The test currently calls fail_if_trust_downgraded(&meta, "1.0.0", &opts) which passes even if the exclude policy were applied globally; change the test to call fail_if_trust_downgraded with a non-excluded version that should be considered a downgrade (e.g., "0.9.0" or another lower-version string that is not in the exclusion set) and assert that err.is_some() (or assert!(err.is_err())) to prove the resolver actually checks non-excluded versions; update the assertion and message accordingly and keep references to the same meta and opts variables and the fail_if_trust_downgraded function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pacquet/crates/config/src/lib.rs`:
- Around line 664-697: The config fields for the minimumReleaseAge policy are
inconsistent with pnpm: update the doc on minimum_release_age (and/or convert
stored value) to indicate minutes instead of milliseconds and ensure any parsing
converts minutes->milliseconds if you keep internal ms; change the default for
minimum_release_age_ignore_missing_time to true to match pnpm; and implement
conditional defaulting for minimum_release_age_strict so it becomes true when
minimum_release_age is explicitly configured (e.g., add a custom deserializer or
a post-construction normalizer like Config::normalize_defaults that checks
Self::minimum_release_age.is_some() and sets minimum_release_age_strict = true
unless the user explicitly set it). Ensure you reference the struct fields
minimum_release_age, minimum_release_age_ignore_missing_time, and
minimum_release_age_strict when making these changes and update the doc comments
accordingly.
In `@pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs`:
- Around line 218-224: The code silently skips a candidate when
serde_json::to_string(&metadata.resolution) fails (the Err(_) => continue),
which can let entries bypass verification; instead, fail fast by propagating or
returning an error with context. Replace the continue with an early return or
propagated error from the current function (e.g., return Err(...)) that includes
the serialization error and identifying information about metadata.resolution
(and/or the enclosing candidate id) so callers/logs can diagnose the failure
rather than skipping verification.
- Around line 317-357: The guard always emits elapsed_ms: 0 because Drop no
longer has started_at; update TerminalEmitGuard to store started_at (e.g., add
started_at: Option<Instant> or Instant) when constructed in
TerminalEmitGuard::failed so Drop can recompute elapsed_ms; change
TerminalEmitGuard::failed to save the Instant used to measure, adjust cancel()
to clear or preserve started_at appropriately, and in the Drop impl compute
elapsed_ms = started_at.elapsed().as_millis() as u64 for the
LockfileVerificationMessage::Failed branch before calling emit::<Reporter>,
ensuring the emitted payload matches pnpm's expected elapsed_ms semantics.
In `@pacquet/crates/resolving-npm-resolver/src/fetch_attestation_published_at.rs`:
- Around line 55-57: The URL for the attestation endpoint embeds pkg_name and
version raw, which breaks for scoped names like "`@scope/pkg`"; update the code
that builds the URL (the lines creating registry, url, and the request via
opts.http_client.acquire_for_url) to percent-encode pkg_name and version (encode
'@' -> %40 and '/' -> %2F) before interpolation, using either
url::percent_encoding::percent_encode with an appropriate NON_ALPHANUMERIC set
or adding a small dependency like urlencoding::encode; ensure the encoded values
are used in
format!("{registry}/-/npm/v1/attestations/{encoded_pkg}@{encoded_version}") and
keep the subsequent opts.http_client.acquire_for_url(&url).await.get(&url) call
unchanged, and add a unit/integration test that requests an attestation for a
scoped package (e.g., "`@acme/mylib`@1.0.0") to prevent regressions.
In `@pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs`:
- Around line 60-63: The JSON deserialization error from the
.json::<Package>().await call is being mapped to FetchMetadataError::Network;
update that mapping to use FetchMetadataError::Decode so decode failures are
classified correctly. Locate the await chain where .json::<Package>() is
followed by .map_err(|error| FetchMetadataError::Network { url: url.clone(),
error }) and change the variant to FetchMetadataError::Decode (preserving the
url.clone() and error fields), ensuring downstream diagnostics and pnpm error
codes reflect a decode/parse failure; this likely lives inside the
fetch_full_metadata function handling package metadata.
- Around line 48-50: The registry URL is built by concatenating raw pkg_name
into the path (let url = format!("{registry}{name}", registry = opts.registry,
name = pkg_name);), which breaks for scoped names like "`@scope/pkg`"; change it
to percent-encode pkg_name as a single path segment before formatting the URL
(use a percent-encoding utility to encode "/" as "%2F"), update the creation of
url and subsequent request acquisition (where opts.registry and pkg_name are
used) to use the encoded name, and add a unit/integration test that fetches
metadata for a scoped package (e.g., "`@myorg/tool`") to verify end-to-end
behavior.
---
Nitpick comments:
In `@pacquet/crates/cli/tests/lockfile_verification.rs`:
- Around line 14-15: The module currently does a wildcard re-export with "pub
use _utils::*;" which hides which symbols are exposed; replace it by explicitly
re-exporting only the needed items from _utils (e.g., "pub use _utils::{ItemA,
helper_fn}" listing the actual types/functions used) or remove the re-export
entirely if nothing from _utils is required; locate the "pub mod _utils;" / "pub
use _utils::*;" lines and update them to explicit "pub use _utils::{...}"
entries matching the symbols referenced by the tests in this file.
In `@pacquet/crates/resolving-npm-resolver/src/trust_checks/tests.rs`:
- Around line 257-264: The test currently calls fail_if_trust_downgraded(&meta,
"1.0.0", &opts) which passes even if the exclude policy were applied globally;
change the test to call fail_if_trust_downgraded with a non-excluded version
that should be considered a downgrade (e.g., "0.9.0" or another lower-version
string that is not in the exclusion set) and assert that err.is_some() (or
assert!(err.is_err())) to prove the resolver actually checks non-excluded
versions; update the assertion and message accordingly and keep references to
the same meta and opts variables and the fail_if_trust_downgraded function.
🪄 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: 3523a4c2-49c7-4ac7-b2fb-0caf982a1849
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockpacquet/crates/lockfile-verification/src/errors/snapshots/pacquet_lockfile_verification__errors__tests__single_entry_single_code.snapis excluded by!**/*.snappacquet/crates/lockfile-verification/src/errors/snapshots/pacquet_lockfile_verification__errors__tests__three_entries_mixed_codes.snapis excluded by!**/*.snappacquet/crates/lockfile-verification/src/errors/snapshots/pacquet_lockfile_verification__errors__tests__three_entries_single_code.snapis excluded by!**/*.snap
📒 Files selected for processing (64)
Cargo.tomlpacquet/crates/cli/src/cli_args/add.rspacquet/crates/cli/src/cli_args/install.rspacquet/crates/cli/src/state.rspacquet/crates/cli/tests/lockfile_verification.rspacquet/crates/config/Cargo.tomlpacquet/crates/config/src/defaults.rspacquet/crates/config/src/defaults/tests.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/version_policy.rspacquet/crates/config/src/version_policy/tests.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/lockfile-verification/Cargo.tomlpacquet/crates/lockfile-verification/src/cache.rspacquet/crates/lockfile-verification/src/cache/tests.rspacquet/crates/lockfile-verification/src/errors.rspacquet/crates/lockfile-verification/src/errors/tests.rspacquet/crates/lockfile-verification/src/hash_lockfile.rspacquet/crates/lockfile-verification/src/hash_lockfile/tests.rspacquet/crates/lockfile-verification/src/lib.rspacquet/crates/lockfile-verification/src/record_lockfile_verified.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rspacquet/crates/package-manager/Cargo.tomlpacquet/crates/package-manager/src/add.rspacquet/crates/package-manager/src/build_resolution_verifiers.rspacquet/crates/package-manager/src/build_snapshot/tests.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/lib.rspacquet/crates/package-manager/src/version_policy.rspacquet/crates/package-manager/src/version_policy/tests.rspacquet/crates/registry/src/lib.rspacquet/crates/registry/src/package.rspacquet/crates/registry/src/package/tests.rspacquet/crates/registry/src/package_distribution.rspacquet/crates/registry/src/package_version.rspacquet/crates/reporter/src/lib.rspacquet/crates/reporter/src/tests.rspacquet/crates/resolving-npm-resolver/Cargo.tomlpacquet/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/errors.rspacquet/crates/resolving-npm-resolver/src/fetch_attestation_published_at.rspacquet/crates/resolving-npm-resolver/src/fetch_attestation_published_at/tests.rspacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rspacquet/crates/resolving-npm-resolver/src/fetch_full_metadata/tests.rspacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached.rspacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached/tests.rspacquet/crates/resolving-npm-resolver/src/lib.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/resolving-npm-resolver/src/mirror.rspacquet/crates/resolving-npm-resolver/src/mirror/tests.rspacquet/crates/resolving-npm-resolver/src/named_registry.rspacquet/crates/resolving-npm-resolver/src/named_registry/tests.rspacquet/crates/resolving-npm-resolver/src/trust_checks.rspacquet/crates/resolving-npm-resolver/src/trust_checks/tests.rspacquet/crates/resolving-npm-resolver/src/violation_codes.rspacquet/crates/resolving-resolver-base/Cargo.tomlpacquet/crates/resolving-resolver-base/src/lib.rspacquet/crates/resolving-resolver-base/src/tests.rspacquet/plans/TEST_PORTING.md
💤 Files with no reviewable changes (1)
- pacquet/crates/package-manager/src/version_policy/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🧰 Additional context used
📓 Path-based instructions (2)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: Log emissions must mirror pnpm'spnpm:<channel>events throughglobalLogger/logger.debug(...)/streamParser.write(...)calls with matching payload and ordering for@pnpm/cli.default-reportercompatibility
Prefer real fixtures (tempfile::TempDir, mocked registry, integration tests) over dependency-injection seams for testing, reserving DI only for branches real fixtures cannot cover (filesystem error kinds, deterministic time, process-global state, external-service paths)
Declare a newtype wrapper (struct) for branded string types instead of collapsing the brand into plain String or &str
If upstream pnpm always validates before construction of a branded string type, validate too by constructing only via TryFrom and/or FromStr
If upstream pnpm never validates a branded string type, brand only for type-safety by exposing an infallible From and From<&str> constructor
If upstream pnpm occasionally constructs a branded string type without validation, expose from_str_unchecked constructor alongside the validating constructor
Match upstream pnpm serde behavior for branded string types by using #[serde(try_from = "String")] for deserialization and #[serde(into = "String")] for serialization
Use #[derive(derive_more::From)] and #[derive(derive_more::Into)] for mechanical branded string type conversions instead of handwriting impl blocks
Model upstream string literal unions or string literal types as Rust enums instead of newtype wrappers
Treat string template literal types (e.g.,${string}@${string}) as branded string types with the same validation discipline as other branded strings
Choose owned vs. borrowed parameters to minimize copies; widen to the most encompassing type (&Path over &PathBuf, &str over &String) when it doesn't force extra copies
Prefer Arc::clone(&x) / Rc::clone(&x) over x.clone() for reference-counted types to make the cost visible at the call site
Follow test-logging guidance in the style guide: log before...
Files:
pacquet/crates/package-manager/src/lib.rspacquet/crates/cli/src/cli_args/add.rspacquet/crates/lockfile-verification/src/hash_lockfile/tests.rspacquet/crates/registry/src/package_version.rspacquet/crates/resolving-npm-resolver/src/errors.rspacquet/crates/registry/src/lib.rspacquet/crates/cli/src/state.rspacquet/crates/cli/tests/lockfile_verification.rspacquet/crates/lockfile-verification/src/lib.rspacquet/crates/package-manager/src/version_policy.rspacquet/crates/package-manager/src/build_resolution_verifiers.rspacquet/crates/resolving-npm-resolver/src/lib.rspacquet/crates/resolving-npm-resolver/src/violation_codes.rspacquet/crates/resolving-npm-resolver/src/fetch_attestation_published_at.rspacquet/crates/reporter/src/lib.rspacquet/crates/config/src/defaults.rspacquet/crates/resolving-npm-resolver/src/mirror/tests.rspacquet/crates/package-manager/src/build_snapshot/tests.rspacquet/crates/lockfile-verification/src/record_lockfile_verified.rspacquet/crates/resolving-npm-resolver/src/fetch_full_metadata/tests.rspacquet/crates/resolving-resolver-base/src/tests.rspacquet/crates/cli/src/cli_args/install.rspacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached/tests.rspacquet/crates/config/src/lib.rspacquet/crates/resolving-resolver-base/src/lib.rspacquet/crates/resolving-npm-resolver/src/named_registry.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/config/src/defaults/tests.rspacquet/crates/package-manager/src/add.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/registry/src/package_distribution.rspacquet/crates/reporter/src/tests.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/lockfile-verification/src/cache.rspacquet/crates/package-manager/src/install.rspacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rspacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached.rspacquet/crates/registry/src/package.rspacquet/crates/lockfile-verification/src/hash_lockfile.rspacquet/crates/resolving-npm-resolver/src/named_registry/tests.rspacquet/crates/lockfile-verification/src/errors/tests.rspacquet/crates/lockfile-verification/src/errors.rspacquet/crates/resolving-npm-resolver/src/trust_checks.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/config/src/version_policy/tests.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rspacquet/crates/registry/src/package/tests.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rspacquet/crates/resolving-npm-resolver/src/mirror.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rspacquet/crates/config/src/version_policy.rspacquet/crates/resolving-npm-resolver/src/fetch_attestation_published_at/tests.rspacquet/crates/lockfile-verification/src/cache/tests.rspacquet/crates/resolving-npm-resolver/src/trust_checks/tests.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs
pacquet/**/Cargo.toml
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/Cargo.toml: Check workspace.dependencies in the root Cargo.toml before adding a new dependency to see if it's already declared
Add new dependencies to the specific crate that needs them, not to the workspace root or shared crates, unless multiple crates actually depend on it
Do not add a dependency not already declared in workspace.dependencies without explicit human request and approval to add it to the workspace root Cargo.toml
Files:
pacquet/crates/config/Cargo.tomlpacquet/crates/resolving-npm-resolver/Cargo.tomlpacquet/crates/resolving-resolver-base/Cargo.tomlpacquet/crates/lockfile-verification/Cargo.tomlpacquet/crates/package-manager/Cargo.toml
🔇 Additional comments (44)
Cargo.toml (1)
16-42: LGTM!Also applies to: 52-52
pacquet/crates/cli/src/cli_args/add.rs (1)
107-110: LGTM!Also applies to: 114-114, 118-118
pacquet/crates/cli/src/cli_args/install.rs (1)
168-175: LGTM!Also applies to: 179-179, 183-183
pacquet/crates/cli/src/state.rs (1)
16-20: LGTM!Also applies to: 68-71
pacquet/crates/cli/tests/lockfile_verification.rs (1)
1-13: LGTM!Also applies to: 17-95
pacquet/crates/config/Cargo.toml (1)
23-23: LGTM!pacquet/crates/config/src/defaults.rs (1)
126-159: LGTM!pacquet/crates/config/src/defaults/tests.rs (1)
2-4: LGTM!Also applies to: 103-145
pacquet/crates/config/src/version_policy.rs (1)
1-255: LGTM!pacquet/crates/config/src/version_policy/tests.rs (1)
1-219: LGTM!pacquet/crates/config/src/workspace_yaml/tests.rs (1)
1-782: LGTM!pacquet/crates/lockfile-verification/Cargo.toml (1)
1-38: LGTM!pacquet/crates/lockfile-verification/src/cache.rs (1)
1-438: LGTM!pacquet/crates/lockfile-verification/src/cache/tests.rs (1)
1-333: LGTM!pacquet/crates/config/src/workspace_yaml.rs (1)
444-455: ⚡ Quick winThe
minimumReleaseAgeStricthandling is correct; no derivation logic is needed.When
minimumReleaseAgeis set andminimumReleaseAgeStrictis omitted, pnpm defaults tofalsefor the strict setting—there is no special derivation based on the presence ofminimumReleaseAge. The code correctly leavesconfig.minimum_release_age_strictat its default value (false) when the workspace YAML field is absent, matching pnpm's behavior.pacquet/crates/package-manager/Cargo.toml (1)
14-36: LGTM!pacquet/crates/package-manager/src/add.rs (1)
23-27: LGTM!Also applies to: 55-63, 87-95
pacquet/crates/package-manager/src/build_resolution_verifiers.rs (1)
1-132: LGTM!pacquet/crates/package-manager/src/build_snapshot/tests.rs (1)
13-31: LGTM!pacquet/crates/package-manager/src/install.rs (1)
1-1: LGTM!Also applies to: 4-7, 14-16, 42-61, 169-185, 207-211, 284-309
pacquet/crates/package-manager/src/install/tests.rs (1)
24-2954: LGTM!pacquet/crates/package-manager/src/install_package_from_registry/tests.rs (1)
62-69: LGTM!pacquet/crates/package-manager/src/lib.rs (1)
3-3: LGTM!Also applies to: 33-33
pacquet/crates/package-manager/src/version_policy.rs (1)
1-12: LGTM!pacquet/crates/registry/src/lib.rs (1)
7-9: LGTM!pacquet/crates/registry/src/package.rs (1)
19-57: LGTM!Also applies to: 62-70
pacquet/crates/registry/src/package/tests.rs (1)
22-23: LGTM!Also applies to: 42-43, 96-111, 156-285
pacquet/crates/registry/src/package_distribution.rs (1)
13-52: LGTM!pacquet/crates/registry/src/package_version.rs (1)
19-62: LGTM!pacquet/crates/reporter/src/lib.rs (1)
179-190: LGTM!Also applies to: 660-707
pacquet/crates/reporter/src/tests.rs (1)
10-15: LGTM!Also applies to: 803-903
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached.rs (1)
33-46: LGTM!Also applies to: 80-187
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached/tests.rs (1)
25-60: LGTM!Also applies to: 62-105, 107-151, 153-181, 183-221
pacquet/crates/resolving-npm-resolver/src/lib.rs (1)
14-40: LGTM!pacquet/crates/resolving-npm-resolver/src/lookup_context.rs (1)
26-65: LGTM!pacquet/crates/resolving-npm-resolver/src/mirror.rs (1)
41-50: LGTM!Also applies to: 57-63, 68-93, 98-107, 112-128, 138-150, 157-164, 179-191, 202-216, 226-233, 241-279
pacquet/crates/resolving-npm-resolver/src/mirror/tests.rs (1)
12-193: LGTM!pacquet/crates/resolving-npm-resolver/src/named_registry.rs (1)
20-23: LGTM!Also applies to: 37-61, 76-107, 109-114
pacquet/crates/resolving-npm-resolver/src/named_registry/tests.rs (1)
7-95: LGTM!pacquet/crates/resolving-npm-resolver/src/violation_codes.rs (1)
10-11: LGTM!pacquet/crates/resolving-resolver-base/Cargo.toml (1)
1-23: LGTM!pacquet/crates/resolving-resolver-base/src/lib.rs (1)
20-127: LGTM!pacquet/crates/resolving-resolver-base/src/tests.rs (1)
18-102: LGTM!pacquet/plans/TEST_PORTING.md (1)
621-740: LGTM!
- **config**: fix three `minimumReleaseAge*` defaults to match pnpm. `minimumReleaseAge` docs now say "minutes" (was "milliseconds"). `minimumReleaseAgeIgnoreMissingTime` defaults to `true` (was `false`), matching pnpm's permissive stance for registries that strip per-version `time`. `minimumReleaseAgeStrict` becomes `Option<bool>` with a `resolved_minimum_release_age_strict()` helper that defaults to `true` when `minimumReleaseAge` is set and `false` otherwise — the same conditional default pnpm's settings page documents. - **verify_lockfile_resolutions**: stop silently skipping candidates when a `LockfileResolution` fails to serialize. Every variant derives `Serialize` and the wire shape never carries non-string keys, so a serialization failure is a logic bug and should panic loudly rather than under-verify the lockfile. - **verify_lockfile_resolutions**: capture `started_at` on the `TerminalEmitGuard` so the `Failed` reporter payload emits an accurate `elapsed_ms` at drop time. The previous version baked the elapsed value at construction (~0ms) and overwrote it with literal `0` on drop, so failure telemetry was always 0. - **fetch_full_metadata / fetch_full_metadata_cached**: percent-encode the package name path segment, mirroring upstream's `toUri` at `resolving/npm-resolver/src/fetch.ts`. Scoped names like `@scope/pkg` are now routed to the registry as a single path segment via the new `registry_url::to_registry_url` helper. - **fetch_full_metadata**: refactor the response read so a JSON decode failure surfaces as `FetchMetadataError::Decode`, not `Network`. Routes the upstream `decode_error` miette code correctly instead of conflating transport and parse failures. - **trust_checks**: don't abort the trust-history scan when an individual prior version lacks a publish timestamp. The previous `?` early-returned, masking the entire ancestry — a downgrade from `1.0.0 (TrustedPublisher)` to `1.1.0 (none)` with `1.0.1` missing `time` would silently pass. Now skips the offending version and continues the walk. New tests: - `registry_url::tests` — scope encoding, trailing-slash normalisation, unscoped passthrough. - `fetch_full_metadata::tests::fetch_full_metadata_encodes_scoped_name` — round-trip a scoped name through mockito at the encoded path. - `fetch_full_metadata::tests::fetch_full_metadata_surfaces_decode_failure_distinctly` — malformed-body response surfaces as `Decode`, not `Network`. - `trust_checks::tests::prior_version_missing_time_does_not_mask_trust_history` — regression test pinning the trust-history scan against the early-return bug. Addresses CodeRabbit review feedback on PR #11729.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs (1)
247-251: ⚡ Quick winAvoid rebuilding the verifier list for every candidate.
verifiers.iter().map(Arc::clone).collect()runs once per candidate, adding avoidable allocation/churn in the hot fan-out path.Proposed refactor
async fn run_fan_out( candidates: Vec<Candidate>, verifiers: &[Arc<dyn ResolutionVerifier>], concurrency: Option<usize>, ) -> Vec<ResolutionPolicyViolation> { let limit = concurrency.unwrap_or(DEFAULT_CONCURRENCY).max(1); let semaphore = Arc::new(Semaphore::new(limit)); + let shared_verifiers: Arc<Vec<Arc<dyn ResolutionVerifier>>> = + Arc::new(verifiers.iter().map(Arc::clone).collect()); let mut futures = FuturesUnordered::new(); for candidate in candidates { let semaphore = Arc::clone(&semaphore); - let verifiers: Vec<Arc<dyn ResolutionVerifier>> = - verifiers.iter().map(Arc::clone).collect(); + let verifiers = Arc::clone(&shared_verifiers); futures.push(async move { // Holding the permit across every verifier .await keeps // the effective in-flight count bounded by the semaphore. // Releasing per-verifier would let N candidates × M // verifiers race past the cap. let _permit = semaphore.acquire().await.expect("semaphore not closed during fan-out"); - evaluate_candidate(candidate, &verifiers).await + evaluate_candidate(candidate, verifiers.as_slice()).await }); }As per coding guidelines, "Choose owned vs. borrowed parameters to minimize copies; widen to the most encompassing type (&Path over &PathBuf, &str over &String) when it doesn't force extra copies".
🤖 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/lockfile-verification/src/verify_lockfile_resolutions.rs` around lines 247 - 251, The loop rebuilds the verifier list per candidate causing unnecessary allocations; instead, prepare a shared cloned collection once and cheaply clone a handle into each async task: create a single shared container (e.g., let verifiers_shared = Arc::new(verifiers.iter().map(Arc::clone).collect::<Vec<_>>()); ) outside the for candidate in candidates loop, and inside the loop clone Arc::clone(&verifiers_shared) (and Arc::clone(&semaphore) as already done) to move into the async move closure so the code no longer calls verifiers.iter().map(Arc::clone).collect() per candidate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pacquet/crates/config/src/workspace_yaml/tests.rs`:
- Around line 753-762: The test is missing assertions that settings.apply_to
actually copies the exclusion lists into the Config; after calling
settings.apply_to(&mut config, Path::new("/proj")) add assertions that the
Config fields holding the parsed excludes (e.g.,
config.minimum_release_age_exclude and config.trust_policy_exclude) equal the
expected Vecs/sets of patterns/identifiers from the workspace YAML (use the same
literal values the parser test used), and assert their lengths/contents (or
equality) to ensure both exclusion lists are propagated by Config::new/apply_to.
In `@pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata/tests.rs`:
- Around line 89-92: The test currently only inspects formatted debug text of
the error from fetch_full_metadata("acme", &opts) — tighten it to assert the
specific error variant (e.g., the Network/FetchError::Network variant emitted by
fetch_full_metadata) before or alongside the existing text check; locate the err
binding returned by fetch_full_metadata in this test and add an assertion using
matches! or direct pattern matching against the resolver's error enum (the
Network variant), then keep the assert!(text.contains("acme")) check to ensure
the URL remains in the message.
---
Nitpick comments:
In `@pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs`:
- Around line 247-251: The loop rebuilds the verifier list per candidate causing
unnecessary allocations; instead, prepare a shared cloned collection once and
cheaply clone a handle into each async task: create a single shared container
(e.g., let verifiers_shared =
Arc::new(verifiers.iter().map(Arc::clone).collect::<Vec<_>>()); ) outside the
for candidate in candidates loop, and inside the loop clone
Arc::clone(&verifiers_shared) (and Arc::clone(&semaphore) as already done) to
move into the async move closure so the code no longer calls
verifiers.iter().map(Arc::clone).collect() per candidate.
🪄 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: 780a2318-8bd3-4d06-9ba5-0348fcfe554d
📒 Files selected for processing (13)
pacquet/crates/config/src/lib.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rspacquet/crates/resolving-npm-resolver/src/fetch_full_metadata/tests.rspacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached.rspacquet/crates/resolving-npm-resolver/src/lib.rspacquet/crates/resolving-npm-resolver/src/registry_url.rspacquet/crates/resolving-npm-resolver/src/registry_url/tests.rspacquet/crates/resolving-npm-resolver/src/trust_checks.rspacquet/crates/resolving-npm-resolver/src/trust_checks/tests.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
- pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs
- pacquet/crates/resolving-npm-resolver/src/trust_checks/tests.rs
- pacquet/crates/config/src/workspace_yaml.rs
- pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached.rs
- pacquet/crates/resolving-npm-resolver/src/lib.rs
- pacquet/crates/resolving-npm-resolver/src/trust_checks.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: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: Log emissions must mirror pnpm'spnpm:<channel>events throughglobalLogger/logger.debug(...)/streamParser.write(...)calls with matching payload and ordering for@pnpm/cli.default-reportercompatibility
Prefer real fixtures (tempfile::TempDir, mocked registry, integration tests) over dependency-injection seams for testing, reserving DI only for branches real fixtures cannot cover (filesystem error kinds, deterministic time, process-global state, external-service paths)
Declare a newtype wrapper (struct) for branded string types instead of collapsing the brand into plain String or &str
If upstream pnpm always validates before construction of a branded string type, validate too by constructing only via TryFrom and/or FromStr
If upstream pnpm never validates a branded string type, brand only for type-safety by exposing an infallible From and From<&str> constructor
If upstream pnpm occasionally constructs a branded string type without validation, expose from_str_unchecked constructor alongside the validating constructor
Match upstream pnpm serde behavior for branded string types by using #[serde(try_from = "String")] for deserialization and #[serde(into = "String")] for serialization
Use #[derive(derive_more::From)] and #[derive(derive_more::Into)] for mechanical branded string type conversions instead of handwriting impl blocks
Model upstream string literal unions or string literal types as Rust enums instead of newtype wrappers
Treat string template literal types (e.g.,${string}@${string}) as branded string types with the same validation discipline as other branded strings
Choose owned vs. borrowed parameters to minimize copies; widen to the most encompassing type (&Path over &PathBuf, &str over &String) when it doesn't force extra copies
Prefer Arc::clone(&x) / Rc::clone(&x) over x.clone() for reference-counted types to make the cost visible at the call site
Follow test-logging guidance in the style guide: log before...
Files:
pacquet/crates/resolving-npm-resolver/src/registry_url.rspacquet/crates/resolving-npm-resolver/src/fetch_full_metadata/tests.rspacquet/crates/resolving-npm-resolver/src/registry_url/tests.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/workspace_yaml/tests.rs
- `workspace_yaml::tests` now asserts the exclusion-list fields (`minimumReleaseAgeExclude`, `trustPolicyExclude`) propagate through `apply_to` into `Config`, not just into the parsed `WorkspaceSettings` shape — catches regressions in the yaml-to-config bridge. - `fetch_full_metadata::tests::fetch_full_metadata_surfaces_5xx_as_network_error` now asserts the `FetchMetadataError::Network` variant via `matches!`, in addition to the existing debug-string check, so a future error remapping (e.g. promoting a decode mistake into the network arm) doesn't slip through. Addresses CodeRabbit follow-up comments on PR #11729.
Resolves the dylint-flagged trailing comma on
\`workspace_yaml/tests.rs\` (single-line \`assert_eq!\` invocation
must not end with a trailing comma) plus three findings from
Codex's review:
- **High**: pacquet now defaults \`minimum_release_age\` to
\`Some(24 * 60)\` (1440 minutes / 24 hours), matching upstream
pnpm's default at \`config/reader/src/index.ts:176\`. Pacquet
previously left it at \`None\`, so a normal install with no
yaml-level policy didn't enforce the upstream 24-hour gate.
\`Config::resolved_minimum_release_age_strict()\` is simplified
to \`unwrap_or(false)\`: pnpm's conditional default ("\`strict\`
defaults to \`true\` when \`minimumReleaseAge\` is explicitly
set") needs the \`explicitlySetKeys\` tracker pnpm's reader
maintains, which pacquet doesn't have yet — the function's doc
comment now flags this divergence so a future
resolver/reader landing can tighten it.
- **Medium**: every \`VerifyError\` variant's miette diagnostic
code gains the \`ERR_PNPM_\` prefix, matching pnpm's
\`PnpmError(...)\` user-visible output:
\`MINIMUM_RELEASE_AGE_VIOLATION\` → \`ERR_PNPM_MINIMUM_RELEASE_AGE_VIOLATION\`,
\`TRUST_DOWNGRADE\` → \`ERR_PNPM_TRUST_DOWNGRADE\`,
\`LOCKFILE_RESOLUTION_VERIFICATION\` → \`ERR_PNPM_LOCKFILE_RESOLUTION_VERIFICATION\`.
The CLI integration test now asserts the prefixed form, so a
future regression that drops the prefix trips the test.
- (The scoped-URL-encoding finding was already addressed in
\`269b1c913f\` — both \`fetch_full_metadata\` and
\`fetch_full_metadata_cached\` route through
\`registry_url::to_registry_url\`.)
\`install::tests::frozen_install_silently_swallows_unreachable_optional_tarball\`
now sets \`config.minimum_release_age = None\` explicitly — the
test exercises the optional-tarball-swallow path and isn't about
the release-age gate; without the opt-out, the new default would
make the verifier try to fetch \`broken-pkg\`'s metadata from the
real \`registry.npmjs.org\` (where it 404s) and trip a violation
before the optional-snapshot dispatch runs.
Pulls in: - cddf522 feat(pacquet): port lockfile verification (minimumReleaseAge + trustPolicy) (#11729) — adds `default_cache_dir`, which was written against the pre-DI-refactor shape and uses the closure-based `HomeDir: FnOnce(...)` parameter plus a module-level `NoEnv` struct this branch's refactor retired. Followup commit ports it to the trait-based DI seam the PR introduces. - c8d8fde feat(config-deps): support optionalDependencies with platform filtering (#11725) — does not touch files this branch edits. https://claude.ai/code/session_01Btd6rp2MgfXb3ArHwSJpqA
…I seam `default_cache_dir` was added on main (cddf522, #11729) in parallel with this PR's DI refactor, using the pre-refactor closure-based shape (`HomeDir: FnOnce() -> Option<PathBuf>` plus a module-level `NoEnv` test fake). This branch's earlier commit `5d14def` retired that module-level `NoEnv` in favour of per-test unit-struct fakes; after merging main the resulting code referenced `NoEnv` at `defaults/tests.rs:155` with no definition in scope (E0425). Port `default_cache_dir` to match the same trait-based shape now used by `default_store_dir`: - Drop the `HomeDir: FnOnce(...)` parameter; bound `Sys: EnvVar + GetHomeDir` and call `Sys::home_dir()` instead. - Collapse the `SmartDefault` expression on `Config::cache_dir` from `default_cache_dir::<Host, _>(home::home_dir)` to `default_cache_dir::<Host>()`. - Rewrite the two `default_cache_dir` tests to implement [`EnvVar`] and [`GetHomeDir`] on per-test unit struct fakes (no module-level `NoEnv` reference). https://claude.ai/code/session_01Btd6rp2MgfXb3ArHwSJpqA
Summary
Ports pnpm's lockfile-verification gate to pacquet, tracking #11722. The gate re-applies the resolver's policy checks (
minimumReleaseAge,trustPolicy='no-downgrade') to every entry inpnpm-lock.yamlimmediately after the lockfile is loaded, before any resolution or fetch happens — so a lockfile resolved elsewhere (committed to the repo, restored from CI cache, or generated by a tool that bypassed local policy) can't reach the filesystem under a weaker policy.Three new crates and four extended ones:
pacquet-resolving-resolver-base—ResolutionVerifiertrait + types.pacquet-resolving-npm-resolver—NpmResolutionVerifier, trust-rank logic, attestation fetcher, cached full-metadata fetcher, mirror helpers (path / IO).pacquet-lockfile-verification— fan-out runner, in-memory lockfile hasher, JSONL stat-and-skip cache +recordLockfileVerifiedwrapper.pacquet-config— newcacheDir,minimumReleaseAge*,trustPolicy*fields +TrustPolicyenum +createPackageVersionPolicy. Defaults match upstream byte-for-byte:minimumReleaseAgedefaults to1440minutes (24 h) andminimumReleaseAgeIgnoreMissingTimedefaults totrue.pacquet-reporter—pnpm:lockfile-verificationchannel.pacquet-registry— enrichesPackage/PackageVersionwithtime,_npmUser.trustedPublisher,dist.attestations,modified.pacquet-package-manager::install— wires the gate between lockfile load and the frozen-lockfile dispatch.Phase-by-phase landing order matches the plan in #11722. Ports against upstream pnpm at commit
2a9bd897bf.Test plan
cargo check --workspace --lockedclean.cargo clippy --workspace --locked -- --deny warningsclean (matchesjust lint).cargo nextest run -p pacquet-resolving-resolver-base -p pacquet-resolving-npm-resolver -p pacquet-lockfile-verification— 110 new unit tests, all passing.cargo nextest run -p pacquet-package-manager— 263 install-layer tests pass (including the new e2e tests for the gate).cargo nextest run -p pacquet-cli --test lockfile_verification— CLI integration test: spawns thepacquetbinary against apnpm-workspace.yamlwithminimumReleaseAgeand asserts non-zero exit + stderr containsERR_PNPM_MINIMUM_RELEASE_AGE_VIOLATION(theERR_PNPM_prefix matches upstreamPnpmErrorrendering sopnpm errorsURL routing works on pacquet output).errors::testspin the rendered diagnostic text (single-entry, three-entry single-code, three-entry mixed-code) byte-for-byte against the upstreamPnpmErrorshape.pacquet/plans/TEST_PORTING.mdupdated with a "Lockfile Verification Gate" section listing ~50 upstream tests this work mirrored, with checkboxes and Rust-side test names.Manual smoke:
pacquet install --frozen-lockfileagainst a lockfile pinning a real registry package withminimumReleaseAge: 100_years_in_minexits non-zero withERR_PNPM_MINIMUM_RELEASE_AGE_VIOLATIONin stderr.minimumReleaseAge: 0(gate disabled) succeeds with nopnpm:lockfile-verificationevents fired.<cache_dir>/lockfile-verified.jsonl) and skip the verifier fan-out via the stat shortcut.Known gaps
Tracked in the new TEST_PORTING.md section:
local_metalayer are stubbed (nofetchAbbreviatedMetadataCachedport yet). The 4-layer publish-time lookup falls back to attestation → full-meta, which is functionally sufficient.minimumReleaseAgeStrictships asOption<bool>with aresolved_minimum_release_age_strict()accessor that defaults tofalse. Upstream's default is conditional on whether the user explicitly setminimumReleaseAge(viaexplicitlySetKeys); pacquet has noexplicitlySetKeystracker yet, so the conditional fall-back is the one upstream surface this port doesn't match. Comment on the accessor flags the divergence.minimumReleaseAgenon-strict-mode resolver fall-back is out of scope until pacquet has a real resolver.Written by an agent (Claude Code, claude-opus-4-7).