Skip to content
This repository was archived by the owner on May 14, 2026. It is now read-only.

feat: frozen-lockfile freshness check (#447)#450

Merged
zkochan merged 4 commits into
mainfrom
feat/447
May 13, 2026
Merged

feat: frozen-lockfile freshness check (#447)#450
zkochan merged 4 commits into
mainfrom
feat/447

Conversation

@zkochan

@zkochan zkochan commented May 13, 2026

Copy link
Copy Markdown
Member

Summary

Closes #447. Adds a satisfies_package_manifest check to pacquet's frozen-lockfile dispatcher so a stale pnpm-lock.yaml no longer silently installs the wrong shape of node_modules.

Before this, pacquet install --frozen-lockfile would happily materialize whatever the lockfile said, even when a dev had edited package.json (added, removed, or bumped a dep) without re-running the resolver. Upstream pnpm catches this at the dispatch site with ERR_PNPM_OUTDATED_LOCKFILE (pkg-manager/core/src/install/index.ts:808-832); this PR ports the same gate.

What's checked

Ported from upstream's satisfiesPackageManifest. Four phases, short-circuiting on the first failure:

  1. Flat-record specifier diff over dependencies ∪ devDependencies ∪ optionalDependencies. Catches added/removed/modified deps in one bucket; rendered as a SpecDiff with per-bucket lists.
  2. publishDirectory parity between the importer entry and the manifest's publishConfig.directory.
  3. dependenciesMeta JSON equality between the importer and the manifest (with absent ≡ empty-object equivalence to match upstream's ?? {} coercion).
  4. Per-field name-set + specifier match. Catches same-name-same-specifier moves between fields the flat-record diff doesn't see (e.g. react moved from dependencies to devDependencies). Applies upstream's precedence rule (optional > prod > dev): a dep that appears in a higher-precedence manifest field is filtered out of the lower-precedence field's check, so a manifest listing the same dep in both prod and dev still satisfies a lockfile that records it under prod only.

Reasons are surfaced as typed StalenessReason variants (SpecifiersDiffer, DepSpecifierMismatch, PublishDirectoryMismatch, DependenciesMetaMismatch, NoImporter) so callers match on the discriminant rather than parsing format strings, and tests assert on shape rather than wording. The SpecDiff::Display impl handles singular/plural ("1 dependency was added" vs "2 dependencies were added") so user-facing CI output reads cleanly.

New errors

  • InstallError::OutdatedLockfile { reason: StalenessReason } — surfaced as ERR_PNPM_OUTDATED_LOCKFILE (miette code pacquet_package_manager::outdated_lockfile). Hint points at pnpm install --lockfile-only to regenerate.
  • InstallError::NoImporter { importer_id } — distinguishes "lockfile file is missing" (NoLockfile) from "lockfile is present but has no importer entry for this project." Renders as importers["{id}"] for readability.

Performance

Confirmed by hyperfine on a 1352-package fixture with 110-dep manifest (warm reinstall, --warmup 2 --runs 10):

mean range
main (no check) 573.5 ms 554-603
PR (with check) 574.7 ms 549-602

Ratio 1.00 ± 0.05 — within noise. The check is pure-CPU map/set operations on string keys with no syscalls or async; ~50-200 μs for the alot7 manifest.

Out of scope (matching the issue)

Test plan

  • just ready — 722 tests pass, lint clean.
  • just dylint — perfectionist clean.
  • taplo format --check — clean.
  • RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace --locked — clean.
  • Unit tests in crates/lockfile/src/freshness/tests.rs (13 tests) covering:
    • Happy paths: matching baseline, all-three-fields, dependenciesMeta: {} ≡ absent, publishDirectory match, importer empty-vs-absent dev-deps equivalence, same dep in prod + dev (precedence), same dep in prod + optional (precedence).
    • Failure paths: manifest adds dep, manifest drops dep, manifest bumps specifier, dep moves between fields, cross-field swap with same cardinalities, publishDirectory mismatch, dependenciesMeta mismatch.
    • Message-shape tests: SpecDiff::Display plural and singular wording, NoImporter bracket-quoted id formatting.
  • Integration tests in crates/package-manager/src/install/tests.rs (2 new):
    • frozen_lockfile_errors_when_manifest_drifts_from_lockfile — drifted manifest fails with OutdatedLockfile before any fetch attempt. Fixture uses a bogus tarball URL so a fetch attempt would also fail — only the early OutdatedLockfile makes the test pass cleanly.
    • frozen_lockfile_errors_when_lockfile_has_no_root_importer — well-formed lockfile with empty importers map surfaces as NoImporter for ..
  • Existing tests using PARTIAL_INSTALL_LOCKFILE updated to populate the manifest with the matching placeholder dep so they don't trip the new check.

Upstream test coverage audit

Cross-checked against upstream's satisfiesPackageManifest.test.ts (21 assertions). All applicable cases ported; cases left out are the ones in "Out of scope" above (link:, autoInstallPeers, semver-range satisfies). Caught one behavioral gap in the process: my port was missing the precedence filter on the per-field check, which would have failed a manifest listing the same dep in both dependencies and devDependencies. Fixed before merge.


Written by an agent (Claude Code, claude-opus-4-7).

Add a `satisfies_package_manifest` check to pacquet's frozen-lockfile
dispatcher so a stale `pnpm-lock.yaml` no longer silently installs
the wrong shape of `node_modules`. Mirrors upstream's gate at
[pkg-manager/core/src/install/index.ts:808-832](https://github.com/pnpm/pnpm/blob/94240bc046/pkg-manager/core/src/install/index.ts#L808-L832):
before linking, compare the lockfile importer entry against the
on-disk `package.json` and fail with `ERR_PNPM_OUTDATED_LOCKFILE`
when they diverge.

The check (ported from upstream's
[`satisfiesPackageManifest`](https://github.com/pnpm/pnpm/blob/94240bc046/lockfile/verification/src/satisfiesPackageManifest.ts))
runs two passes:

1. Flat-record specifier diff over `dependencies ∪ devDependencies ∪
   optionalDependencies`. Catches added / removed / modified deps in
   one bucket; renders as a `SpecDiff` listing each entry per
   category so the user sees exactly what drifted.
2. Per-field name-set + specifier match. Catches the
   same-name-same-specifier-moved-between-buckets case the flat-
   record diff doesn't see (e.g. `react` moved from
   `dependencies` to `devDependencies`).

Reasons are surfaced as typed `StalenessReason` variants so
`pacquet-package-manager`'s `InstallError::OutdatedLockfile`
forwards a structured detail rather than a parsed string. The new
`InstallError::NoImporter` distinguishes "lockfile file is missing"
(`NoLockfile`) from "lockfile is present but has no importer entry
for this project."

Scoped to what pacquet supports today — no catalogs, no auto-
install-peers pre-pass, no `excludeLinksFromLockfile`, no semver-
range satisfies check (covered in pnpm's `localTarballDepsAreUpToDate`
which lives outside this gate). Multi-importer support comes when
workspace install (#431) lands.

Closes #447.
Copilot AI review requested due to automatic review settings May 13, 2026 12:24
@coderabbitai

coderabbitai Bot commented May 13, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Adds a lockfile freshness verifier (StalenessReason, SpecDiff, satisfies_package_manifest), exports it from the lockfile crate, gates frozen-lockfile installs with an early freshness check returning new InstallError variants, and adds unit + integration tests covering mismatches and formatting.

Changes

Lockfile Freshness Verification

Layer / File(s) Summary
Freshness types and core verifier
crates/lockfile/src/freshness.rs
Defines StalenessReason and SpecDiff, implements satisfies_package_manifest with flattened-union specifier diff, publishDirectory and dependenciesMeta checks, per-field specifier validations, and helper flatten/diff functions.
Freshness unit tests
crates/lockfile/src/freshness/tests.rs
Unit tests for matching, added/removed/modified specifiers, cross-field moves, publishDirectory and dependenciesMeta mismatches, NoImporter formatting, and SpecDiff display/grammar.
Lockfile crate exports
crates/lockfile/src/lib.rs
Declares mod freshness; and pub use freshness::*; to expose the verifier and types to consumers.
Install frozen-lockfile freshness gate
crates/package-manager/src/install.rs
Adds InstallError::OutdatedLockfile { reason: StalenessReason } and InstallError::NoImporter { importer_id: String }; imports freshness utilities and validates the root importer against on-disk package.json before performing a frozen install, returning early on mismatch.
Install test fixture alignment & regressions
crates/package-manager/src/install/tests.rs
Updates several warm-reinstall tests to include the placeholder dependency so fixtures match; adds two frozen-lockfile tests asserting early OutdatedLockfile and NoImporter errors.

Sequence Diagram(s)

sequenceDiagram
  participant Manifest as PackageManifest
  participant Importer as Lockfile.importers["."]
  participant Checker as satisfies_package_manifest
  participant Install as Install::run (frozen)
  participant Result as InstallError / Ok
  Install->>Checker: validate importer vs manifest
  Checker->>Importer: read dependency specifiers, publishDirectory, dependenciesMeta
  Checker->>Manifest: read dependencies/dev/optional and publishConfig, dependenciesMeta
  Checker->>Checker: compute flat-union diff via diff_flat_records
  alt diff non-empty
    Checker-->>Install: Err(OutdatedLockfile { SpecifiersDiffer })
  else
    Checker->>Checker: publishDirectory / dependenciesMeta / per-field checks
    alt mismatch
      Checker-->>Install: Err(OutdatedLockfile { reason })
    else
      Checker-->>Install: Ok(())
      Install->>Install: proceed with frozen install
    end
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers

  • KSXGitHub

Poem

🐇 I checked the lockfile, sniffed the manifest tree,

If specs have wandered, I cry "Not for me!"
A hop, a nudge, a rabbitly cheer,
Freshness enforced — your installs are clear!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'feat: frozen-lockfile freshness check (#447)' is concise, clearly identifies the main feature being added, and directly corresponds to the primary change in the changeset (implementing a freshness check for frozen-lockfile installs).
Linked Issues check ✅ Passed The code changes fully implement the objectives from #447: a typed StalenessReason enum with variants, a SpecDiff struct for user-readable diffs, the satisfies_package_manifest check with two-pass verification, integration into Install::run's frozen-lockfile path, and comprehensive unit and integration tests.
Out of Scope Changes check ✅ Passed All changes are directly scoped to implementing the frozen-lockfile freshness check as specified in #447; no extraneous modifications to catalogs, workspace support, link: filtering, or other out-of-scope features are present.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description check ✅ Passed The PR description comprehensively documents the changes, including a clear summary, technical implementation details, test coverage, and explicit out-of-scope items that align with the repository's contribution guidelines.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 feat/447

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


Comment @coderabbitai help to get the list of available commands and usage tips.

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

Adds a lockfile “freshness” gate to the --frozen-lockfile install path so pacquet rejects stale pnpm-lock.yaml when package.json drifts, mirroring pnpm’s ERR_PNPM_OUTDATED_LOCKFILE behavior to prevent silently installing the wrong node_modules shape.

Changes:

  • Introduces pacquet_lockfile::satisfies_package_manifest + StalenessReason to detect manifest/lockfile drift.
  • Wires the freshness check into Install::run for --frozen-lockfile, adding OutdatedLockfile and NoImporter install errors.
  • Updates/extends package-manager integration tests and adds lockfile-crate unit tests for the new verification behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
crates/package-manager/src/install.rs Adds frozen-lockfile freshness dispatch check and new error variants/diagnostics.
crates/package-manager/src/install/tests.rs Updates existing fixtures for the new check and adds integration coverage for outdated lockfile / missing importer.
crates/lockfile/src/lib.rs Exposes the new freshness module from the lockfile crate.
crates/lockfile/src/freshness.rs Implements the manifest-vs-importer verification logic and user-facing diff formatting.
crates/lockfile/src/freshness/tests.rs Adds unit tests covering specifier diffs, field-move detection, and diff display formatting.
Comments suppressed due to low confidence (3)

crates/package-manager/src/install/tests.rs:855

  • This test mutates the manifest in-memory via add_dependency(...) but never calls manifest.save(). Other tests in this file save after mutation, and the comment mentions the on-disk manifest matching the lockfile; consider saving to keep the fixture accurate and avoid future regressions if code starts re-reading package.json from disk.
    let manifest_path = dir.path().join("package.json");
    let mut manifest = PackageManifest::create_if_needed(manifest_path).unwrap();
    // Manifest must match `PARTIAL_INSTALL_LOCKFILE` — the freshness
    // check (#447) rejects any drift between the on-disk manifest and
    // the lockfile importer entry.
    manifest.add_dependency("placeholder", "1.0.0", DependencyGroup::Prod).unwrap();

crates/package-manager/src/install/tests.rs:946

  • This test mutates the manifest in-memory via add_dependency(...) but never calls manifest.save(). Other tests in this file save after mutation, and the comment mentions the on-disk manifest matching the lockfile; consider saving to keep the fixture accurate and avoid future regressions if code starts re-reading package.json from disk.
    let manifest_path = dir.path().join("package.json");
    let mut manifest = PackageManifest::create_if_needed(manifest_path).unwrap();
    // Manifest must match the fixture lockfile below — the freshness
    // check (#447) rejects any drift between the on-disk manifest and
    // the lockfile importer entry.
    manifest.add_dependency("placeholder", "1.0.0", DependencyGroup::Prod).unwrap();

crates/package-manager/src/install/tests.rs:1081

  • This test mutates the manifest in-memory via add_dependency(...) but never calls manifest.save(). Other tests in this file save after mutation, and the comment mentions the on-disk manifest matching the lockfile; consider saving to keep the fixture accurate and avoid future regressions if code starts re-reading package.json from disk.
    let manifest_path = dir.path().join("package.json");
    let mut manifest = PackageManifest::create_if_needed(manifest_path).unwrap();
    // Manifest must match `PARTIAL_INSTALL_LOCKFILE` — the freshness
    // check (#447) rejects any drift between the on-disk manifest and
    // the lockfile importer entry.
    manifest.add_dependency("placeholder", "1.0.0", DependencyGroup::Prod).unwrap();


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/lockfile/src/freshness.rs Outdated
Comment thread crates/lockfile/src/freshness.rs
Comment thread crates/lockfile/src/freshness.rs
Comment thread crates/package-manager/src/install.rs Outdated
Comment thread crates/package-manager/src/install/tests.rs
Comment thread crates/package-manager/src/install.rs Outdated
Comment thread crates/lockfile/src/freshness.rs Outdated
@codecov

codecov Bot commented May 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.50276% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.29%. Comparing base (1fa70a4) to head (6a1ec27).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
crates/lockfile/src/freshness.rs 89.20% 19 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #450      +/-   ##
==========================================
- Coverage   87.45%   87.29%   -0.16%     
==========================================
  Files         100      106       +6     
  Lines        7865     8674     +809     
==========================================
+ Hits         6878     7572     +694     
- Misses        987     1102     +115     

☔ 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 13, 2026

Copy link
Copy Markdown

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.01     16.3±0.57ms   265.6 KB/sec    1.00     16.2±0.19ms   268.0 KB/sec

@github-actions

github-actions Bot commented May 13, 2026

Copy link
Copy Markdown

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.533 ± 0.075 2.444 2.679 1.02 ± 0.04
pacquet@main 2.475 ± 0.049 2.411 2.565 1.00
pnpm 6.110 ± 0.051 6.025 6.172 2.47 ± 0.05
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.5327158177399998,
      "stddev": 0.07497244829459591,
      "median": 2.5149506419399996,
      "user": 2.55801202,
      "system": 3.53160684,
      "min": 2.44438707394,
      "max": 2.6794006069400003,
      "times": [
        2.55301756394,
        2.6098870019400002,
        2.44438707394,
        2.58478866494,
        2.5083779969399997,
        2.45699999294,
        2.6794006069400003,
        2.51353692994,
        2.46039799194,
        2.51636435394
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.47458971224,
      "stddev": 0.049289145065392274,
      "median": 2.4643615524399998,
      "user": 2.5250587199999996,
      "system": 3.53162684,
      "min": 2.41128639294,
      "max": 2.56478246994,
      "times": [
        2.41708554794,
        2.51161734394,
        2.43764689494,
        2.56478246994,
        2.46919996094,
        2.45952314394,
        2.41128639294,
        2.49028027494,
        2.52798003994,
        2.45649505294
      ]
    },
    {
      "command": "pnpm",
      "mean": 6.10968167944,
      "stddev": 0.0505406393246956,
      "median": 6.128259635940001,
      "user": 8.96442782,
      "system": 4.47755274,
      "min": 6.025029534940001,
      "max": 6.17159623894,
      "times": [
        6.17159623894,
        6.1664798129400005,
        6.133540940940001,
        6.136848956940001,
        6.13362521094,
        6.03521384494,
        6.08854835594,
        6.082955566940001,
        6.1229783309400005,
        6.025029534940001
      ]
    }
  ]
}

Scenario: Frozen Lockfile (Hot Cache)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 690.0 ± 57.9 642.8 838.5 1.00
pacquet@main 696.7 ± 75.6 641.2 885.4 1.01 ± 0.14
pnpm 2588.5 ± 100.3 2510.9 2828.4 3.75 ± 0.35
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.6900262016200001,
      "stddev": 0.05793580520588494,
      "median": 0.67194470112,
      "user": 0.35179118000000004,
      "system": 1.4698748399999997,
      "min": 0.6427830146200001,
      "max": 0.83853711362,
      "times": [
        0.83853711362,
        0.71770209762,
        0.6808156816200001,
        0.66307372062,
        0.6427830146200001,
        0.7063021556200001,
        0.6552067876200001,
        0.65191260862,
        0.65244541962,
        0.69148341662
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.6967051121200001,
      "stddev": 0.07556155265138503,
      "median": 0.6683400421200001,
      "user": 0.35486848000000004,
      "system": 1.4752084399999998,
      "min": 0.6412476796200001,
      "max": 0.8854265506200001,
      "times": [
        0.76425755762,
        0.6412476796200001,
        0.70637077962,
        0.65407070162,
        0.67776452862,
        0.65062533562,
        0.8854265506200001,
        0.66975009062,
        0.6506079036200001,
        0.66692999362
      ]
    },
    {
      "command": "pnpm",
      "mean": 2.5884886469199997,
      "stddev": 0.10032379400323758,
      "median": 2.55955194762,
      "user": 3.10201578,
      "system": 2.2463415400000004,
      "min": 2.51085352562,
      "max": 2.82839096762,
      "times": [
        2.55168971662,
        2.57568341662,
        2.66029580962,
        2.64705971962,
        2.5124757396199997,
        2.51196928762,
        2.56741417862,
        2.51085352562,
        2.51905410762,
        2.82839096762
      ]
    }
  ]
}

Seven fixes from Copilot review on #450:

- Per-field check now runs unconditionally so it catches same-
  cardinality cross-field swaps (e.g. lockfile prod={a}, dev={b} vs
  manifest prod={b}, dev={a}). The flat-union diff matches in this
  case because the union stays identical — only the per-field
  comparison sees the field-level mismatch.
- Implemented `publishDirectory` and `dependenciesMeta` checks the
  module had declared in `StalenessReason` but never emitted.
  Mirrors upstream's `satisfiesPackageManifest.ts:51-58`.
  `dependenciesMeta` treats absent and empty-object as equivalent
  (matching upstream's `?? {}` coercion).
- `SpecDiff::Display` now uses singular wording when n==1
  ("1 dependency was added" instead of "1 dependencies were added").
- `OutdatedLockfile`'s help text no longer suggests
  `pacquet install --no-frozen-lockfile` (which doesn't exist);
  points users at `pnpm install --lockfile-only` to regenerate.
- `NoImporter` messages now use `importers["."]` formatting
  instead of `{:?}` debug-format quoting that rendered short keys
  awkwardly as `importers."."`. Applied to both the
  `StalenessReason::NoImporter` and `InstallError::NoImporter`
  display impls.
- Test sites that mutate the manifest in-memory via
  `add_dependency` now also call `manifest.save()`, in case future
  code paths start re-reading `package.json` from disk.

Added three new tests pinning the new behaviors:
`cross_field_swap_with_same_cardinalities_caught_by_per_field_check`,
`publish_directory_mismatch_returns_publish_directory_mismatch`,
`dependencies_meta_mismatch_returns_dependencies_meta_mismatch`,
`no_importer_message_uses_bracket_quoted_id`,
`spec_diff_display_uses_singular_for_count_of_one`. Updated the
existing `spec_diff_display_lists_added_removed_modified` test to
use n=2 for the plural arm so the pluralization wording is also
pinned.

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

🧹 Nitpick comments (3)
crates/lockfile/src/freshness/tests.rs (2)

203-209: ⚡ Quick win

Update pnpm reference to blob/main

The upstream link in this block is SHA-pinned. Please point it to https://github.com/pnpm/pnpm/blob/main/... to keep references aligned with the repo’s pnpm-porting convention.

As per coding guidelines: "If reading on GitHub, open files from https://github.com/pnpm/pnpm/blob/main/... ... rather than from a permalinked SHA."

🤖 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 `@crates/lockfile/src/freshness/tests.rs` around lines 203 - 209, Update the
upstream pnpm link in the test comment (the block describing per-field follow-up
loop and referencing satisfiesPackageManifest) to use the main branch URL by
replacing
"https://github.com/pnpm/pnpm/blob/94240bc046/lockfile/verification/src/satisfiesPackageManifest.ts#L67-L100"
with
"https://github.com/pnpm/pnpm/blob/main/lockfile/verification/src/satisfiesPackageManifest.ts#L67-L100"
so the reference follows the repo convention; ensure only the URL is changed in
that comment near the test declaration.

192-201: ⚡ Quick win

Align this test’s name with what it actually verifies

missing_importer_returns_no_importer currently only asserts root_project().is_none() and does not validate a NoImporter reason mapping path. Either rename the test to match behavior or extend it to assert the caller-level NoImporter outcome.

🤖 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 `@crates/lockfile/src/freshness/tests.rs` around lines 192 - 201, The test name
missing_importer_returns_no_importer is misleading because it only checks
lockfile.root_project().is_none(); rename the test function to
missing_importer_returns_none (or another name that reflects it simply asserts
None) by updating the function identifier in the test, so the name matches the
actual assertion; alternatively, if you prefer to keep the original name, extend
the test to simulate the caller-level behavior (callers that convert None into a
NoImporter reason) and assert that the resulting reason equals NoImporter.
crates/lockfile/src/freshness.rs (1)

11-176: ⚡ Quick win

Use pnpm blob/main links in Rustdoc references

Several upstream references (for example at Line 12 and Line 169) are pinned to a specific SHA. Please switch them to https://github.com/pnpm/pnpm/blob/main/... so future ports always point to the current source-of-truth.

As per coding guidelines: "When porting features, bug fixes, or behavior changes from pnpm... open files from https://github.com/pnpm/pnpm/blob/main/... rather than from a permalinked SHA."

🤖 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 `@crates/lockfile/src/freshness.rs` around lines 11 - 176, Update the hardcoded
SHA links in the module-level Rustdoc to use the canonical blob/main paths
(replace occurrences like the `satisfiesPackageManifest` upstream link and any
other `https://github.com/pnpm/pnpm/blob/<SHA>/...` references with
`https://github.com/pnpm/pnpm/blob/main/...`); search for the doc comments that
reference `satisfiesPackageManifest` and the lines around the `StalenessReason`
and function comments to update every permalinked URL to the `blob/main` form so
future ports reference the current upstream source.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@crates/lockfile/src/freshness.rs`:
- Around line 11-176: Update the hardcoded SHA links in the module-level Rustdoc
to use the canonical blob/main paths (replace occurrences like the
`satisfiesPackageManifest` upstream link and any other
`https://github.com/pnpm/pnpm/blob/<SHA>/...` references with
`https://github.com/pnpm/pnpm/blob/main/...`); search for the doc comments that
reference `satisfiesPackageManifest` and the lines around the `StalenessReason`
and function comments to update every permalinked URL to the `blob/main` form so
future ports reference the current upstream source.

In `@crates/lockfile/src/freshness/tests.rs`:
- Around line 203-209: Update the upstream pnpm link in the test comment (the
block describing per-field follow-up loop and referencing
satisfiesPackageManifest) to use the main branch URL by replacing
"https://github.com/pnpm/pnpm/blob/94240bc046/lockfile/verification/src/satisfiesPackageManifest.ts#L67-L100"
with
"https://github.com/pnpm/pnpm/blob/main/lockfile/verification/src/satisfiesPackageManifest.ts#L67-L100"
so the reference follows the repo convention; ensure only the URL is changed in
that comment near the test declaration.
- Around line 192-201: The test name missing_importer_returns_no_importer is
misleading because it only checks lockfile.root_project().is_none(); rename the
test function to missing_importer_returns_none (or another name that reflects it
simply asserts None) by updating the function identifier in the test, so the
name matches the actual assertion; alternatively, if you prefer to keep the
original name, extend the test to simulate the caller-level behavior (callers
that convert None into a NoImporter reason) and assert that the resulting reason
equals NoImporter.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 50b49b07-65c9-4a5d-b9ab-6ebeefa9d164

📥 Commits

Reviewing files that changed from the base of the PR and between e25717b and 542306f.

📒 Files selected for processing (4)
  • crates/lockfile/src/freshness.rs
  • crates/lockfile/src/freshness/tests.rs
  • crates/package-manager/src/install.rs
  • crates/package-manager/src/install/tests.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • crates/package-manager/src/install.rs
  • crates/package-manager/src/install/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: Lint and Test (macos-latest)
  • GitHub Check: Lint and Test (windows-latest)
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Code Coverage
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Preserve existing method chains and pipe-trait chains; do not break them into intermediate let bindings unless there is a concrete justification such as a compilation failure, borrow checker rejection, meaningful performance improvement, or other technical necessity. Refactoring for style alone is not sufficient justification.
Choose owned vs. borrowed parameters to minimize copies; prefer borrowed types (&Path over &PathBuf, &str over &String) when it does not force extra copies.
Prefer Arc::clone(&x) and Rc::clone(&x) over x.clone() for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Write use super::{Foo, bar} instead of use super::*; for any glob whose target is a module you control. External-crate preludes (e.g., use rayon::prelude::*;) and root-of-module re-exports (e.g., pub use submodule::*; in lib.rs) are exceptions.
Follow Rust API Guidelines for naming, as documented in https://rust-lang.github.io/api-guidelines/naming.html.
Declare a newtype wrapper for any branded string type being ported from TypeScript pnpm. Do not collapse the brand into a plain String or &str; give the type its own struct so misuse is a type error.
When porting branded string types where upstream TypeScript always validates before construction, validate in the Rust port too. Construct the wrapper only via TryFrom<String> and/or FromStr; do not provide an infallible public constructor that takes an arbitrary string.
For branded string types where upstream TypeScript never validates (used purely for type-safety to prevent confusion between string slots), expose an infallible From<String> and From<&str> constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bare as assertion), add a from_str_unchecked (or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...

Files:

  • crates/lockfile/src/freshness.rs
  • crates/lockfile/src/freshness/tests.rs
🧠 Learnings (2)
📚 Learning: 2026-05-07T23:19:08.272Z
Learnt from: KSXGitHub
Repo: pnpm/pacquet PR: 401
File: tasks/integrated-benchmark/src/work_env.rs:343-344
Timestamp: 2026-05-07T23:19:08.272Z
Learning: When reviewing Rust code in pnpm/pacquet for deprecated API usage, do not automatically treat `serde_saphyr::to_string` as deprecated. In `serde-saphyr` v0.0.25, `serde_saphyr::to_string` has no `#[deprecated]` attribute (the `#[deprecated]` later in `serde-saphyr-0.0.25/src/lib.rs` applies to a different function). Only flag `serde_saphyr::to_string` as deprecated if the resolved dependency version’s source shows `#[deprecated]` on that specific function.

Applied to files:

  • crates/lockfile/src/freshness.rs
  • crates/lockfile/src/freshness/tests.rs
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In Rust test code, follow the repo’s CODE_STYLE_GUIDE test-logging rule: add logging (e.g., `eprintln!`/`eprintln!(...)`) so that useful diagnostic values are printed when a test fails, unless the assertion is `assert_eq!` (where the differing values are already included). Concretely, if you use assertions like `assert!`, `assert_ne!`, etc., ensure the test logs the relevant actual/expected values (or context) before/around the assertion so failures can be diagnosed without rerunning.

Applied to files:

  • crates/lockfile/src/freshness/tests.rs
🔇 Additional comments (2)
crates/lockfile/src/freshness.rs (1)

177-377: LGTM!

crates/lockfile/src/freshness/tests.rs (1)

1-191: LGTM!

Also applies to: 210-392

`[SpecDiff::Display]` doesn't resolve — `Display` is a trait impl, not
an associated item — so rustdoc fails with `unresolved link` under
`-D rustdoc::broken-intra-doc-links` (caught by the Doc CI job, not
`just ready` locally). Refer to it as "[SpecDiff]'s Display impl"
instead.
Copilot AI review requested due to automatic review settings May 13, 2026 12:50

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 5 out of 5 changed files in this pull request and generated no new comments.

Surveyed upstream's `satisfiesPackageManifest` test file
(<https://github.com/pnpm/pnpm/blob/94240bc046/lockfile/verification/test/satisfiesPackageManifest.ts>)
and found one behavioral gap and three uncovered cases.

**Behavioral gap.** Upstream's per-field check filters a dep out of
the `devDependencies` check when it also appears in `dependencies`
or `optionalDependencies` (precedence: optional > prod > dev). My
port did not, so a manifest listing the same dep in both prod and
dev fields would incorrectly fail the dev-field check against a
lockfile that records it under prod only. Add the filter at the
per-field loop in `freshness.rs`.

**Three previously-uncovered upstream test cases ported:**

- `dependencies_meta_empty_object_equivalent_to_absent` —
  `dependenciesMeta: {}` on manifest with no `dependenciesMeta` on
  importer satisfies (the absent/empty equivalence already worked,
  just wasn't pinned by a test).
- `publish_directory_match_satisfies` — happy path for
  publishDirectory (previously only the mismatch case was tested).
- `same_dep_in_prod_and_dev_counts_under_prod` and
  `same_dep_in_prod_and_optional_counts_under_optional` — the
  precedence-filter behavior fixed above.
- `importer_empty_dev_dependencies_equivalent_to_absent` — the
  importer side's `Some(empty)` vs `None` equivalence.

Tests upstream covers that remain out of scope for pacquet:
`link:` deps (#431), `autoInstallPeers` peer-folding (pacquet has
no separate auto-install-peers mode), `excludeLinksFromLockfile`,
semver-range satisfies check (lives in upstream's
`localTarballDepsAreUpToDate`, out of scope per #447).

722 tests now run (+5 from the porting). All checks green
(`just ready`, `just dylint`, `cargo doc -D warnings`, `taplo`).
@zkochan zkochan merged commit cf125b1 into main May 13, 2026
15 of 16 checks passed
@zkochan zkochan deleted the feat/447 branch May 13, 2026 13:35
KSXGitHub pushed a commit that referenced this pull request May 13, 2026
Pull in 28 commits from upstream main, including the
`pacquet-npmrc` → `pacquet-config` rename (#420) plus features:
- supportedArchitectures + --cpu/--os/--libc (#456)
- frozen-lockfile (#442, #443, #447, #450)
- git-fetcher (#436 / #446, #451, #454)
- side-effects cache (#421 / #422, #423, #424)
- real-hoist + global-virtual-store (#432 / #438 / #444, #449, #452)
- patchedDependencies + allow-builds (#425, #427, #428)
- engine/platform installability (#434 / #439)

Conflicts resolved:
- `crates/npmrc/` files migrated under the renamed
  `crates/config/` directory; `Npmrc` → `Config` everywhere
  except `NpmrcAuth` (which keeps the `.npmrc`-domain name).
- `Config::current` reads the env-var DI generic `Api: EnvVar`
  for ${VAR}-substitution in `.npmrc`. Production turbofish in
  `cli_args.rs` is `Config::current::<RealApi, _, _, _, _>(...)`.
- Two-phase `NpmrcAuth::apply_*` retained so default-registry
  creds key at the yaml-resolved registry URL.
- New `Config::auth_headers` field plumbed through
  `install_package_by_snapshot`'s `DownloadTarballToStore`.
- Tests under `crates/config/src/workspace_yaml/tests.rs`
  pick up the new ParseYaml unit test added on this branch.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Frozen-lockfile freshness check: error when package.json drifts from pnpm-lock.yaml

2 participants