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

feat(package-manager): write .modules.yaml after install#407

Merged
zkochan merged 2 commits into
mainfrom
claude/pacquet-modules-yaml-PHmOI
May 9, 2026
Merged

feat(package-manager): write .modules.yaml after install#407
zkochan merged 2 commits into
mainfrom
claude/pacquet-modules-yaml-PHmOI

Conversation

@KSXGitHub
Copy link
Copy Markdown
Contributor

@KSXGitHub KSXGitHub commented May 9, 2026

Mirror upstream pnpm's writeModulesManifest call at the end of a successful install (deps-installer/src/install/index.ts L1608-L1630 on commit 086c5e91e8) so node_modules/.modules.yaml is persisted with the resolved layout: layoutVersion, nodeLinker, the included set derived from the dispatched dependency groups, the store and virtual-store directories, the default registry, and the pacquet@<version> package-manager identifier. This is the contract a follow-up install (or another tool) keys off to detect a layout change and prune accordingly.

Adds a focused test that runs an empty-snapshot frozen install and asserts the on-disk manifest fields, mirroring upstream's installing/modules-yaml/test/index.ts style.

Summary by CodeRabbit

Release Notes

  • New Features

    • The package installer now generates a module manifest file after installation, containing dependency configuration, store paths, and registry information for improved package tracking.
  • Bug Fixes

    • Enhanced error reporting for manifest generation failures during installation.

Review Change Stack

Mirror upstream pnpm's `writeModulesManifest` call at the end of a
successful install (deps-installer/src/install/index.ts L1608-L1630
on commit 086c5e91e8) so `node_modules/.modules.yaml` is persisted
with the resolved layout: `layoutVersion`, `nodeLinker`, the
`included` set derived from the dispatched dependency groups, the
store and virtual-store directories, the `default` registry, and
the `pacquet@<version>` package-manager identifier. This is the
contract a follow-up install (or another tool) keys off to detect
a layout change and prune accordingly.

Adds a focused test that runs an empty-snapshot frozen install and
asserts the on-disk manifest fields, mirroring upstream's
`installing/modules-yaml/test/index.ts` style.
@KSXGitHub KSXGitHub requested a review from zkochan May 9, 2026 21:13
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 9, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9be483de-3ed1-4341-b0ed-460665d80639

📥 Commits

Reviewing files that changed from the base of the PR and between b788562 and 7fe3209.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (3)
  • crates/package-manager/Cargo.toml
  • crates/package-manager/src/install.rs
  • crates/package-manager/src/install/tests.rs
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Agent
  • GitHub Check: Run benchmark on ubuntu-latest
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Log emissions are part of 'match pnpm'—when porting a function that fires pnpm:<channel> events through globalLogger, logger.debug(...), or streamParser.write(...), mirror the call site, payload, and ordering so @pnpm/cli.default-reporter parses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing into plain String or &str to preserve the distinction between validating and non-validating brands
If upstream always validates before construction of a branded string type, implement validation via TryFrom<String> and/or FromStr without providing an infallible public constructor
If upstream never validates a branded string type, expose an infallible From<String> constructor to provide type-safety without runtime validation
When upstream occasionally constructs a branded string type without validation via bare as assertion, expose a from_str_unchecked constructor so callers can opt into the same unchecked path explicitly
Match upstream serde behavior for branded types—use #[serde(try_from = "String")] for deserialization to go through the validator and #[serde(into = "String")] for serialization
Use #[derive(derive_more::From)] and #[derive(derive_more::Into)] for mechanical conversion impls on branded types rather than handwriting impl blocks
Model string-literal unions (like 'auto' | 'always' | 'never') as Rust enums instead of newtype wrappers
Template literal types (like ${string}@${string}) should be treated as branded string types with the same validation discipline applied
Follow the code style guide in CODE_STYLE_GUIDE.md covering code-level conventions not enforced by tooling: imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching, pipe-trait, error handling, test layout, and cloning of Arc and Rc
Warnings are errors in cargo clippy --locked -- --deny warnings, and should not be silenced with `#[allow(....

Files:

  • crates/package-manager/src/install/tests.rs
  • crates/package-manager/src/install.rs
🧠 Learnings (2)
📚 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/package-manager/src/install/tests.rs
📚 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/package-manager/src/install/tests.rs
  • crates/package-manager/src/install.rs
🔇 Additional comments (8)
crates/package-manager/Cargo.toml (1)

16-16: LGTM!

New workspace dependencies for .modules.yaml support are correctly placed in alphabetical order within their respective sections.

Also applies to: 29-29

crates/package-manager/src/install.rs (5)

1-15: LGTM!

Imports are well-organized with appropriate aliasing to disambiguate NodeLinker from both pacquet_npmrc and pacquet_modules_yaml.


59-62: LGTM!

The new WriteModules error variant correctly uses #[diagnostic(transparent)] to propagate the underlying error's diagnostic information.


81-91: LGTM!

Collecting the dependency groups into a Vec once is justified since it's consumed twice—for the install dispatch and for deriving the included field. The O(n) contains calls are negligible for a 3-element vector.


213-225: LGTM!

The manifest write is correctly placed after ImportingDone and before pnpm:summary, matching upstream's writeModulesManifest call ordering.


237-277: LGTM!

The helper functions are well-implemented:

  • map_node_linker uses exhaustive matching, ensuring compile-time safety if either enum gains variants.
  • build_modules_manifest correctly assembles the manifest with concat! for compile-time version string construction and ..Default::default() for fields not yet supported.
crates/package-manager/src/install/tests.rs (2)

3-6: LGTM!

Imports are correctly specified for the new test, with no star imports.

Also applies to: 15-15


549-640: LGTM!

The new test comprehensively validates the .modules.yaml manifest fields. Good choice to use a non-default dependency_groups selection (Prod + Optional, excluding Dev) to specifically verify the included mapping. The pipe_as_ref usage correctly threads the &Path to read_modules_manifest.


📝 Walkthrough

Walkthrough

This PR adds .modules.yaml manifest writing to the pacquet install command. The install flow collects dependency groups into a single vector, derives included dependencies from that set, and after the import stage completes, writes the manifest file with layout metadata, registry mappings, and store configuration.

Changes

Modules Manifest Persistence

Layer / File(s) Summary
Dependencies
crates/package-manager/Cargo.toml
Workspace dependencies pacquet-modules-yaml and httpdate added.
Imports and Error Type
crates/package-manager/src/install.rs
Module imports for manifest types and new InstallError::WriteModules variant to wrap manifest write failures.
Core Install Flow
crates/package-manager/src/install.rs
Dependency groups collected into Vec<DependencyGroup>, IncludedDependencies derived for lockfile paths and manifest; manifest written after ImportingDone stage.
Manifest Builders
crates/package-manager/src/install.rs
Helpers map_node_linker and build_modules_manifest construct on-disk Modules payload with layout version, node linker config, registry mapping, store paths, and metadata.
Manifest Persistence Test
crates/package-manager/src/install/tests.rs
Test install_writes_modules_yaml runs frozen install and reads .modules.yaml to assert persisted layout, included dependencies, store paths, default registry, and package manager version.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • pnpm/pacquet#332: Introduces pacquet-modules-yaml crate with manifest types and serialization that this PR directly depends on and consumes.

Suggested reviewers

  • zkochan
  • anthonyshew

Poem

🐰 A manifest is born when installs complete,
Recording every node and path so neat—
From registry maps to stores held high,
The .modules.yaml watches packages fly! 📦✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding .modules.yaml file writing functionality to the install process in the package-manager.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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 claude/pacquet-modules-yaml-PHmOI

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

Comment thread crates/package-manager/src/install/tests.rs Outdated
Address KSXGitHub's review on #407: thread the `modules_dir` path
through `pipe_as_ref` instead of the bare function call, matching
the pipe-trait style used in `crates/modules-yaml/tests/index.rs`.
@codecov
Copy link
Copy Markdown

codecov Bot commented May 9, 2026

Codecov Report

❌ Patch coverage is 94.44444% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.49%. Comparing base (b788562) to head (7fe3209).

Files with missing lines Patch % Lines
crates/package-manager/src/install.rs 94.44% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #407      +/-   ##
==========================================
+ Coverage   82.34%   82.49%   +0.14%     
==========================================
  Files          65       65              
  Lines        3705     3741      +36     
==========================================
+ Hits         3051     3086      +35     
- Misses        654      655       +1     

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

@KSXGitHub KSXGitHub marked this pull request as ready for review May 9, 2026 21:20
Copilot AI review requested due to automatic review settings May 9, 2026 21:20
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 9, 2026

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.01     16.4±0.67ms   264.0 KB/sec    1.00     16.2±0.41ms   267.4 KB/sec

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR mirrors pnpm’s post-install behavior by persisting node_modules/.modules.yaml at the end of a successful install, so subsequent installs/tools can detect layout changes and prune accordingly.

Changes:

  • Write node_modules/.modules.yaml after importing_done using pacquet-modules-yaml::write_modules_manifest.
  • Populate the manifest with layout/linker metadata, included dependency-group flags, registry/store paths, and pacquet@<version>.
  • Add an install test asserting key .modules.yaml fields are persisted on disk.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

File Description
crates/package-manager/src/install.rs Writes .modules.yaml on successful install; adds manifest assembly helpers and error plumbing.
crates/package-manager/src/install/tests.rs Adds a focused integration test verifying .modules.yaml contents after a frozen install.
crates/package-manager/Cargo.toml Adds pacquet-modules-yaml and httpdate dependencies needed for manifest writing.
Cargo.lock Updates lockfile for the added dependencies.

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

hoist_pattern: Some(config.hoist_pattern.clone()),
included,
layout_version: Some(LayoutVersion),
node_linker: Some(map_node_linker(&config.node_linker)),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it is ok as it is. No need to change.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 9, 2026

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.489 ± 0.038 2.428 2.558 1.03 ± 0.03
pacquet@main 2.411 ± 0.064 2.312 2.480 1.00
pnpm 6.223 ± 0.098 6.101 6.439 2.58 ± 0.08
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.4893071277199996,
      "stddev": 0.03778976077576028,
      "median": 2.48801144742,
      "user": 2.3753783200000003,
      "system": 3.36489756,
      "min": 2.42810304392,
      "max": 2.5575729479200002,
      "times": [
        2.52316059892,
        2.46451462392,
        2.47858697992,
        2.42810304392,
        2.49743591492,
        2.51086128192,
        2.5575729479200002,
        2.5102452679200002,
        2.45727061492,
        2.46532000292
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.4111535662200003,
      "stddev": 0.06379447916434569,
      "median": 2.42247320542,
      "user": 2.39112862,
      "system": 3.38241746,
      "min": 2.31181046692,
      "max": 2.47961142292,
      "times": [
        2.46517937092,
        2.3309840799200003,
        2.47673802092,
        2.4233362459200003,
        2.34722215092,
        2.47961142292,
        2.38542668492,
        2.42161016492,
        2.46961705392,
        2.31181046692
      ]
    },
    {
      "command": "pnpm",
      "mean": 6.22328802892,
      "stddev": 0.09818946555315908,
      "median": 6.18564593392,
      "user": 9.01279572,
      "system": 4.48764126,
      "min": 6.10132038692,
      "max": 6.43915123092,
      "times": [
        6.19051080792,
        6.24078210892,
        6.16611539092,
        6.32172352492,
        6.17129887592,
        6.10132038692,
        6.43915123092,
        6.26594780092,
        6.15524910192,
        6.18078105992
      ]
    }
  ]
}

Scenario: Frozen Lockfile (Hot Cache)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 711.5 ± 39.7 684.7 819.5 1.00
pacquet@main 774.4 ± 85.7 677.6 945.9 1.09 ± 0.13
pnpm 2597.7 ± 82.6 2477.2 2805.3 3.65 ± 0.23
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.71147247968,
      "stddev": 0.03973354536960486,
      "median": 0.69915582528,
      "user": 0.24434402,
      "system": 1.38205258,
      "min": 0.68469674478,
      "max": 0.81950639578,
      "times": [
        0.81950639578,
        0.70084386078,
        0.72952994278,
        0.70095215778,
        0.68469674478,
        0.69641861878,
        0.69270433278,
        0.69176109278,
        0.69909597378,
        0.69921567678
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.77440743028,
      "stddev": 0.08566570037403819,
      "median": 0.76706276728,
      "user": 0.24646102,
      "system": 1.3750902800000002,
      "min": 0.67764001278,
      "max": 0.94593922578,
      "times": [
        0.94593922578,
        0.69876728778,
        0.79311861478,
        0.82704566378,
        0.71563719278,
        0.67764001278,
        0.83814049978,
        0.74100691978,
        0.82010955278,
        0.68666933278
      ]
    },
    {
      "command": "pnpm",
      "mean": 2.59766507688,
      "stddev": 0.08264555105272708,
      "median": 2.58873502628,
      "user": 3.1241152199999997,
      "system": 2.26331228,
      "min": 2.47722099178,
      "max": 2.80528396978,
      "times": [
        2.61349385178,
        2.60148889078,
        2.55241293378,
        2.60199523578,
        2.47722099178,
        2.58363174378,
        2.59383830878,
        2.56462109378,
        2.58266374878,
        2.80528396978
      ]
    }
  ]
}

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.

4 participants