Skip to content

feat(pacquet): port lockfile verification (minimumReleaseAge + trustPolicy)#11729

Merged
zkochan merged 19 commits into
mainfrom
pacquet-lockfile-verification
May 18, 2026
Merged

feat(pacquet): port lockfile verification (minimumReleaseAge + trustPolicy)#11729
zkochan merged 19 commits into
mainfrom
pacquet-lockfile-verification

Conversation

@zkochan

@zkochan zkochan commented May 18, 2026

Copy link
Copy Markdown
Member

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 in pnpm-lock.yaml immediately 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-baseResolutionVerifier trait + types.
  • pacquet-resolving-npm-resolverNpmResolutionVerifier, 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 + recordLockfileVerified wrapper.
  • pacquet-config — new cacheDir, minimumReleaseAge*, trustPolicy* fields + TrustPolicy enum + createPackageVersionPolicy. Defaults match upstream byte-for-byte: minimumReleaseAge defaults to 1440 minutes (24 h) and minimumReleaseAgeIgnoreMissingTime defaults to true.
  • pacquet-reporterpnpm:lockfile-verification channel.
  • pacquet-registry — enriches Package / PackageVersion with time, _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 --locked clean.
  • cargo clippy --workspace --locked -- --deny warnings clean (matches just 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 the pacquet binary against a pnpm-workspace.yaml with minimumReleaseAge and asserts non-zero exit + stderr contains ERR_PNPM_MINIMUM_RELEASE_AGE_VIOLATION (the ERR_PNPM_ prefix matches upstream PnpmError rendering so pnpm errors URL routing works on pacquet output).
  • Three insta snapshots in errors::tests pin the rendered diagnostic text (single-entry, three-entry single-code, three-entry mixed-code) byte-for-byte against the upstream PnpmError shape.
  • pacquet/plans/TEST_PORTING.md updated 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-lockfile against a lockfile pinning a real registry package with minimumReleaseAge: 100_years_in_min exits non-zero with ERR_PNPM_MINIMUM_RELEASE_AGE_VIOLATION in stderr.
  • Same install with minimumReleaseAge: 0 (gate disabled) succeeds with no pnpm:lockfile-verification events fired.
  • Repeated runs hit the on-disk JSONL cache (<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:

  • Abbreviated-modified shortcut + on-disk local_meta layer are stubbed (no fetchAbbreviatedMetadataCached port yet). The 4-layer publish-time lookup falls back to attestation → full-meta, which is functionally sufficient.
  • minimumReleaseAgeStrict ships as Option<bool> with a resolved_minimum_release_age_strict() accessor that defaults to false. Upstream's default is conditional on whether the user explicitly set minimumReleaseAge (via explicitlySetKeys); pacquet has no explicitlySetKeys tracker yet, so the conditional fall-back is the one upstream surface this port doesn't match. Comment on the accessor flags the divergence.
  • A handful of upstream tests still need explicit Rust-side mirrors (cache stat-shortcut size/mtime drift variants, the multi-verifier merge happy-path, the recordLockfileVerified load-equivalent-hash case).
  • The minimumReleaseAge non-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).

@coderabbitai

coderabbitai Bot commented May 18, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Adds 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.

Changes

Lockfile Verification Flow

Layer / File(s) Summary
Workspace and crate manifests
Cargo.toml, pacquet/crates/*/Cargo.toml
Workspace dependency wiring updated; new crate manifests for lockfile-verification, resolving-npm-resolver, resolving-resolver-base; package-manager Cargo deps adjusted.
CLI / State threading
pacquet/crates/cli/src/*, pacquet/crates/cli/tests/*
State.http_client now Arc; CLI add/install compute lockfile_path and pass http_client_arc and lockfile_path into package-manager entrypoints; CLI E2E test added for minimumReleaseAge gating.
Config and version policy
pacquet/crates/config/src/*, pacquet/crates/config/Cargo.toml
Adds default_cache_dir, version_policy module (parser/matcher), TrustPolicy enum and multiple Config fields for cache_dir, minimum_release_age*, trust_policy*, and workspace YAML application/tests.
Registry model
pacquet/crates/registry/src/*
Extends Package/PackageVersion/PackageDistribution with time, etag, modified, npm_user, attestations/provenance and associated types plus tests and re-exports.
Reporter events
pacquet/crates/reporter/src/*
Adds LogEvent::LockfileVerification payload and serialization tests for started/done/failed messages and optional lockfilePath field.
Resolver base contract
pacquet/crates/resolving-resolver-base/src/*
New crate defining ResolutionVerifier trait, Verification result types, VerifyCtx, and tests.
NPM resolution verifier & helpers
pacquet/crates/resolving-npm-resolver/src/*
New crate implementing create_npm_resolution_verifier, attestation and full-metadata fetchers (cached and mirror), mirror helpers, lookup context, named-registry routing, trust_checks, violation codes, and extensive tests.
Lockfile verification runner & cache
pacquet/crates/lockfile-verification/src/*
New crate with verify_lockfile_resolutions runner, JSONL on-disk cache (try/record/compact), hash_lockfile, record_lockfile_verified wrapper, VerifyError rendering, and test coverage for runner and cache semantics.
Package-manager integration
pacquet/crates/package-manager/src/*
Adds build_resolution_verifiers, threads verifier list into Install, adds http_client_arc and lockfile_path to Install/Add, maps verification/build errors into InstallError, and updates tests including gate integration tests.

Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Possibly related PRs

"I nibble bytes beneath the moonlit tree,
I fetch attestations then hop with glee,
Cache me a carrot, verify the rest —
A rabbit's seal on the lockfile quest." 🐇🔐

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pacquet-lockfile-verification

zkochan added 14 commits May 18, 2026 21:00
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`.
@zkochan zkochan force-pushed the pacquet-lockfile-verification branch from 96b80d7 to ac95e79 Compare May 18, 2026 19:06
@github-actions

github-actions Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.02      7.7±0.34ms   561.7 KB/sec    1.00      7.6±0.14ms   572.9 KB/sec

@codecov-commenter

codecov-commenter commented May 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.66667% with 124 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.88%. Comparing base (e01d2bc) to head (ed04349).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...npm-resolver/src/create_npm_resolution_verifier.rs 91.64% 31 Missing ⚠️
...kfile-verification/src/record_lockfile_verified.rs 0.00% 20 Missing ⚠️
pacquet/crates/lockfile-verification/src/cache.rs 91.28% 17 Missing ⚠️
...ing-npm-resolver/src/fetch_full_metadata_cached.rs 81.42% 13 Missing ⚠️
...npm-resolver/src/fetch_attestation_published_at.rs 82.45% 10 Missing ⚠️
.../package-manager/src/build_resolution_verifiers.rs 89.28% 6 Missing ⚠️
.../crates/resolving-npm-resolver/src/trust_checks.rs 94.11% 6 Missing ⚠️
pacquet/crates/config/src/version_policy.rs 92.64% 5 Missing ⚠️
...acquet/crates/resolving-npm-resolver/src/mirror.rs 94.38% 5 Missing ⚠️
.../crates/lockfile-verification/src/hash_lockfile.rs 87.09% 4 Missing ⚠️
... and 3 more
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.463 ± 0.096 2.381 2.715 1.03 ± 0.05
pacquet@main 2.385 ± 0.060 2.289 2.478 1.00
pnpm 4.854 ± 0.044 4.787 4.916 2.03 ± 0.05
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)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 687.3 ± 56.3 659.0 846.4 1.00 ± 0.09
pacquet@main 684.0 ± 30.7 651.1 747.9 1.00
pnpm 2644.8 ± 72.9 2567.1 2833.0 3.87 ± 0.20
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
      ]
    }
  ]
}

zkochan added 2 commits May 18, 2026 21:46
- 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.
@zkochan zkochan marked this pull request as ready for review May 18, 2026 20:04
Copilot AI review requested due to automatic review settings May 18, 2026 20:04
@coderabbitai coderabbitai Bot added area: lockfile area: supply chain security Issues related to minimumReleaseAge, blockExoticSubdeps, build script safety, and trust policies. labels May 18, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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), and pacquet-lockfile-verification (runner, JSONL cache, in-memory lockfile hasher).
  • Extends pacquet-config (new fields + TrustPolicy enum + createPackageVersionPolicy), pacquet-reporter (new LockfileVerification channel), and pacquet-registry (enriched Package/PackageVersion with time, _npmUser.trustedPublisher, dist.attestations, modified).
  • Wires the gate into pacquet-package-manager::install between 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Nitpick comments (2)
pacquet/crates/cli/tests/lockfile_verification.rs (1)

14-15: ⚡ Quick win

Replace wildcard re-export from _utils with 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 needed

As per coding guidelines, "Do not use star imports inside module bodies; use explicit imports like use super::{Foo, bar} instead of use 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 win

This 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

📥 Commits

Reviewing files that changed from the base of the PR and between e01d2bc and 78db367.

⛔ Files ignored due to path filters (4)
  • Cargo.lock is excluded by !**/*.lock
  • pacquet/crates/lockfile-verification/src/errors/snapshots/pacquet_lockfile_verification__errors__tests__single_entry_single_code.snap is excluded by !**/*.snap
  • pacquet/crates/lockfile-verification/src/errors/snapshots/pacquet_lockfile_verification__errors__tests__three_entries_mixed_codes.snap is excluded by !**/*.snap
  • pacquet/crates/lockfile-verification/src/errors/snapshots/pacquet_lockfile_verification__errors__tests__three_entries_single_code.snap is excluded by !**/*.snap
📒 Files selected for processing (64)
  • Cargo.toml
  • pacquet/crates/cli/src/cli_args/add.rs
  • pacquet/crates/cli/src/cli_args/install.rs
  • pacquet/crates/cli/src/state.rs
  • pacquet/crates/cli/tests/lockfile_verification.rs
  • pacquet/crates/config/Cargo.toml
  • pacquet/crates/config/src/defaults.rs
  • pacquet/crates/config/src/defaults/tests.rs
  • pacquet/crates/config/src/lib.rs
  • pacquet/crates/config/src/version_policy.rs
  • pacquet/crates/config/src/version_policy/tests.rs
  • pacquet/crates/config/src/workspace_yaml.rs
  • pacquet/crates/config/src/workspace_yaml/tests.rs
  • pacquet/crates/lockfile-verification/Cargo.toml
  • pacquet/crates/lockfile-verification/src/cache.rs
  • pacquet/crates/lockfile-verification/src/cache/tests.rs
  • pacquet/crates/lockfile-verification/src/errors.rs
  • pacquet/crates/lockfile-verification/src/errors/tests.rs
  • pacquet/crates/lockfile-verification/src/hash_lockfile.rs
  • pacquet/crates/lockfile-verification/src/hash_lockfile/tests.rs
  • pacquet/crates/lockfile-verification/src/lib.rs
  • pacquet/crates/lockfile-verification/src/record_lockfile_verified.rs
  • pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs
  • pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rs
  • pacquet/crates/package-manager/Cargo.toml
  • pacquet/crates/package-manager/src/add.rs
  • pacquet/crates/package-manager/src/build_resolution_verifiers.rs
  • pacquet/crates/package-manager/src/build_snapshot/tests.rs
  • pacquet/crates/package-manager/src/install.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
  • pacquet/crates/package-manager/src/lib.rs
  • pacquet/crates/package-manager/src/version_policy.rs
  • pacquet/crates/package-manager/src/version_policy/tests.rs
  • pacquet/crates/registry/src/lib.rs
  • pacquet/crates/registry/src/package.rs
  • pacquet/crates/registry/src/package/tests.rs
  • pacquet/crates/registry/src/package_distribution.rs
  • pacquet/crates/registry/src/package_version.rs
  • pacquet/crates/reporter/src/lib.rs
  • pacquet/crates/reporter/src/tests.rs
  • pacquet/crates/resolving-npm-resolver/Cargo.toml
  • pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs
  • pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/errors.rs
  • pacquet/crates/resolving-npm-resolver/src/fetch_attestation_published_at.rs
  • pacquet/crates/resolving-npm-resolver/src/fetch_attestation_published_at/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs
  • pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached.rs
  • pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/lib.rs
  • pacquet/crates/resolving-npm-resolver/src/lookup_context.rs
  • pacquet/crates/resolving-npm-resolver/src/mirror.rs
  • pacquet/crates/resolving-npm-resolver/src/mirror/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/named_registry.rs
  • pacquet/crates/resolving-npm-resolver/src/named_registry/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/trust_checks.rs
  • pacquet/crates/resolving-npm-resolver/src/trust_checks/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/violation_codes.rs
  • pacquet/crates/resolving-resolver-base/Cargo.toml
  • pacquet/crates/resolving-resolver-base/src/lib.rs
  • pacquet/crates/resolving-resolver-base/src/tests.rs
  • pacquet/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's pnpm:<channel> events through globalLogger / logger.debug(...) / streamParser.write(...) calls with matching payload and ordering for @pnpm/cli.default-reporter compatibility
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.rs
  • pacquet/crates/cli/src/cli_args/add.rs
  • pacquet/crates/lockfile-verification/src/hash_lockfile/tests.rs
  • pacquet/crates/registry/src/package_version.rs
  • pacquet/crates/resolving-npm-resolver/src/errors.rs
  • pacquet/crates/registry/src/lib.rs
  • pacquet/crates/cli/src/state.rs
  • pacquet/crates/cli/tests/lockfile_verification.rs
  • pacquet/crates/lockfile-verification/src/lib.rs
  • pacquet/crates/package-manager/src/version_policy.rs
  • pacquet/crates/package-manager/src/build_resolution_verifiers.rs
  • pacquet/crates/resolving-npm-resolver/src/lib.rs
  • pacquet/crates/resolving-npm-resolver/src/violation_codes.rs
  • pacquet/crates/resolving-npm-resolver/src/fetch_attestation_published_at.rs
  • pacquet/crates/reporter/src/lib.rs
  • pacquet/crates/config/src/defaults.rs
  • pacquet/crates/resolving-npm-resolver/src/mirror/tests.rs
  • pacquet/crates/package-manager/src/build_snapshot/tests.rs
  • pacquet/crates/lockfile-verification/src/record_lockfile_verified.rs
  • pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata/tests.rs
  • pacquet/crates/resolving-resolver-base/src/tests.rs
  • pacquet/crates/cli/src/cli_args/install.rs
  • pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached/tests.rs
  • pacquet/crates/config/src/lib.rs
  • pacquet/crates/resolving-resolver-base/src/lib.rs
  • pacquet/crates/resolving-npm-resolver/src/named_registry.rs
  • pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
  • pacquet/crates/config/src/defaults/tests.rs
  • pacquet/crates/package-manager/src/add.rs
  • pacquet/crates/config/src/workspace_yaml/tests.rs
  • pacquet/crates/registry/src/package_distribution.rs
  • pacquet/crates/reporter/src/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/lookup_context.rs
  • pacquet/crates/lockfile-verification/src/cache.rs
  • pacquet/crates/package-manager/src/install.rs
  • pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs
  • pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached.rs
  • pacquet/crates/registry/src/package.rs
  • pacquet/crates/lockfile-verification/src/hash_lockfile.rs
  • pacquet/crates/resolving-npm-resolver/src/named_registry/tests.rs
  • pacquet/crates/lockfile-verification/src/errors/tests.rs
  • pacquet/crates/lockfile-verification/src/errors.rs
  • pacquet/crates/resolving-npm-resolver/src/trust_checks.rs
  • pacquet/crates/config/src/workspace_yaml.rs
  • pacquet/crates/config/src/version_policy/tests.rs
  • pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs
  • pacquet/crates/registry/src/package/tests.rs
  • pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/mirror.rs
  • pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs
  • pacquet/crates/config/src/version_policy.rs
  • pacquet/crates/resolving-npm-resolver/src/fetch_attestation_published_at/tests.rs
  • pacquet/crates/lockfile-verification/src/cache/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/trust_checks/tests.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/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.toml
  • pacquet/crates/resolving-npm-resolver/Cargo.toml
  • pacquet/crates/resolving-resolver-base/Cargo.toml
  • pacquet/crates/lockfile-verification/Cargo.toml
  • pacquet/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 win

The minimumReleaseAgeStrict handling is correct; no derivation logic is needed.

When minimumReleaseAge is set and minimumReleaseAgeStrict is omitted, pnpm defaults to false for the strict setting—there is no special derivation based on the presence of minimumReleaseAge. The code correctly leaves config.minimum_release_age_strict at 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!

Comment thread pacquet/crates/config/src/lib.rs Outdated
Comment thread pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs Outdated
Comment thread pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs Outdated
Comment thread pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs Outdated
Comment thread pacquet/crates/resolving-npm-resolver/src/trust_checks.rs
- **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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs (1)

247-251: ⚡ Quick win

Avoid 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

📥 Commits

Reviewing files that changed from the base of the PR and between 78db367 and 269b1c9.

📒 Files selected for processing (13)
  • pacquet/crates/config/src/lib.rs
  • pacquet/crates/config/src/workspace_yaml.rs
  • pacquet/crates/config/src/workspace_yaml/tests.rs
  • pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs
  • 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/fetch_full_metadata/tests.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/registry_url.rs
  • pacquet/crates/resolving-npm-resolver/src/registry_url/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/trust_checks.rs
  • pacquet/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's pnpm:<channel> events through globalLogger / logger.debug(...) / streamParser.write(...) calls with matching payload and ordering for @pnpm/cli.default-reporter compatibility
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.rs
  • pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/registry_url/tests.rs
  • pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs
  • pacquet/crates/config/src/lib.rs
  • pacquet/crates/config/src/workspace_yaml/tests.rs

Comment thread pacquet/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.
Copilot AI review requested due to automatic review settings May 18, 2026 20:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 69 out of 70 changed files in this pull request and generated no new comments.

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.
@zkochan zkochan merged commit cddf522 into main May 18, 2026
27 of 28 checks passed
@zkochan zkochan deleted the pacquet-lockfile-verification branch May 18, 2026 21:32
KSXGitHub pushed a commit that referenced this pull request May 19, 2026
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
KSXGitHub pushed a commit that referenced this pull request May 19, 2026
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: lockfile area: supply chain security Issues related to minimumReleaseAge, blockExoticSubdeps, build script safety, and trust policies.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants