test(modules-yaml): pin hoistedLocations round-trip (#438 slice 2)#473
Conversation
`hoistedLocations` has been part of the schema since #332 but had no behavior tests — the field was deserialized and serialized by serde without anything pinning the on-disk shape. Add two round-trip tests covering the two states pacquet can encounter today: - `hoisted_locations_round_trips` — a manifest with `hoistedLocations` populated round-trips through write+read with values intact; the raw on-disk JSON keeps the per-depPath array shape. - `absent_hoisted_locations_is_omitted_on_write` — `None` (the state every pacquet install writes today) serializes as `hoistedLocations` *absent* from the file, matching upstream's `JSON.stringify(undefined)` behavior at installing/modules-yaml/src/index.ts. Also add a doc-comment on the field itself explaining what it represents (per-depPath list of lockfile-relative directory paths used by `linkHoistedModules` and rebuild) and noting that pacquet's install pipeline does not populate it yet. Refs <https://github.com/pnpm/pnpm/blob/94240bc046/installing/modules-yaml/src/index.ts#L43>.
📝 WalkthroughWalkthroughThis PR adds documentation and tests for the Changeshoisted_locations field documentation and testing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR pins existing .modules.yaml schema support for hoistedLocations, documenting its purpose and adding regression coverage for its read/write behavior.
Changes:
- Added documentation for
Modules::hoisted_locations. - Added tests covering populated
hoistedLocationsround-trip behavior and omission when absent.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
crates/modules-yaml/src/lib.rs |
Documents the existing hoisted_locations manifest field and its upstream shape. |
crates/modules-yaml/tests/real_fs.rs |
Adds real filesystem tests for hoistedLocations serialization/deserialization behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/modules-yaml/tests/real_fs.rs (2)
212-221: ⚡ Quick winAlign non-
assert_eq!assertions with the test-logging convention.Lines 212 and 221 use non-
assert_eq!assertions without prior debug logging. Please either add logging/dbg!before these checks or convert them toassert_eq!(..., None)-style assertions.As per coding guidelines, "
**/tests/**/*.rs: ... log before non-assert_eq!assertions, usedbg!for complex structures ..."🤖 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/modules-yaml/tests/real_fs.rs` around lines 212 - 221, The two non-assert_eq! checks should follow the test-logging convention: replace the bare assert! checks for manifest.hoisted_locations and raw.get("hoistedLocations") with assert_eq! comparisons (e.g., assert_eq!(manifest.hoisted_locations, None) and assert_eq!(raw.get("hoistedLocations"), None)) so they both produce standardized diff-friendly output, or alternatively insert dbg!(...) calls immediately before each assert! (using dbg!(manifest.hoisted_locations) and dbg!(&raw)) if you prefer logging; update the assertions around manifest.hoisted_locations and the raw variable accordingly.
163-164: ⚡ Quick winReplace SHA permalink with
blob/mainin the upstream test reference.Line 164 should point to
https://github.com/pnpm/pnpm/blob/main/...instead of a pinned SHA URL to follow the repo’s porting-reference 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/modules-yaml/tests/real_fs.rs` around lines 163 - 164, Update the upstream test reference URL string that currently uses a pinned SHA to instead use the blob/main path; locate the comment containing "https://github.com/pnpm/pnpm/blob/94240bc046/installing/modules-yaml/src/index.ts#L43" (the URL literal near the "Mirrors the optional `Record<string, string[]>` shape" comment) and change the permalinked SHA segment to "blob/main" so it reads "https://github.com/pnpm/pnpm/blob/main/installing/modules-yaml/src/index.ts#L43".crates/modules-yaml/src/lib.rs (1)
217-218: ⚡ Quick winUse
blob/mainfor upstream reference links in new porting docs.Line 218 uses a SHA-pinned permalink; please switch it to
https://github.com/pnpm/pnpm/blob/main/...to match the repo’s upstream-reference policy.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/modules-yaml/src/lib.rs` around lines 217 - 218, Update the SHA-pinned GitHub URL in the doc comment that references "Record<string, string[]>" so it uses blob/main instead of the commit SHA; locate the doc comment containing the link "https://github.com/pnpm/pnpm/blob/94240bc046/installing/modules-yaml/src/index.ts#L43" and change it to "https://github.com/pnpm/pnpm/blob/main/installing/modules-yaml/src/index.ts#L43" to comply with the upstream-reference policy.
🤖 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/modules-yaml/src/lib.rs`:
- Around line 217-218: Update the SHA-pinned GitHub URL in the doc comment that
references "Record<string, string[]>" so it uses blob/main instead of the commit
SHA; locate the doc comment containing the link
"https://github.com/pnpm/pnpm/blob/94240bc046/installing/modules-yaml/src/index.ts#L43"
and change it to
"https://github.com/pnpm/pnpm/blob/main/installing/modules-yaml/src/index.ts#L43"
to comply with the upstream-reference policy.
In `@crates/modules-yaml/tests/real_fs.rs`:
- Around line 212-221: The two non-assert_eq! checks should follow the
test-logging convention: replace the bare assert! checks for
manifest.hoisted_locations and raw.get("hoistedLocations") with assert_eq!
comparisons (e.g., assert_eq!(manifest.hoisted_locations, None) and
assert_eq!(raw.get("hoistedLocations"), None)) so they both produce standardized
diff-friendly output, or alternatively insert dbg!(...) calls immediately before
each assert! (using dbg!(manifest.hoisted_locations) and dbg!(&raw)) if you
prefer logging; update the assertions around manifest.hoisted_locations and the
raw variable accordingly.
- Around line 163-164: Update the upstream test reference URL string that
currently uses a pinned SHA to instead use the blob/main path; locate the
comment containing
"https://github.com/pnpm/pnpm/blob/94240bc046/installing/modules-yaml/src/index.ts#L43"
(the URL literal near the "Mirrors the optional `Record<string, string[]>`
shape" comment) and change the permalinked SHA segment to "blob/main" so it
reads
"https://github.com/pnpm/pnpm/blob/main/installing/modules-yaml/src/index.ts#L43".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a9ad6b7f-e9f9-4240-a44e-07f6e05264d6
📒 Files selected for processing (2)
crates/modules-yaml/src/lib.rscrates/modules-yaml/tests/real_fs.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). (9)
- GitHub Check: Agent
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Dylint
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Doc
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (windows-latest)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Preserve existing method chains andpipe-traitchains; do not break them into intermediateletbindings 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 (&Pathover&PathBuf,&strover&String) when it does not force extra copies.
PreferArc::clone(&x)andRc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse 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::*;inlib.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 plainStringor&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 viaTryFrom<String>and/orFromStr; 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 infallibleFrom<String>andFrom<&str>constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bareasassertion), add afrom_str_unchecked(or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...
Files:
crates/modules-yaml/src/lib.rscrates/modules-yaml/tests/real_fs.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.rs: When porting behavior from pnpm, port the relevant pnpm tests to Rust tests whenever they translate. Matching test coverage is the easiest way to prove behavioral parity.
Consult the test-porting plan inplans/TEST_PORTING.mdbefore adding ported tests. Follow the conventions expected for ports: useknown_failuresmodules, usepacquet_testing_utils::allow_known_failure!at the not-yet-implemented boundary, and temporarily break the subject under test to verify the ported test actually catches the regression. Update TEST_PORTING.md checkboxes as items land.
Follow the test-logging guidance in CODE_STYLE_GUIDE.md: log before non-assert_eq!assertions, usedbg!for complex structures, skip logging for simple scalarassert_eq!assertions.
Files:
crates/modules-yaml/tests/real_fs.rs
🧠 Learnings (1)
📚 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/modules-yaml/src/lib.rscrates/modules-yaml/tests/real_fs.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #473 +/- ##
==========================================
- Coverage 89.18% 89.17% -0.01%
==========================================
Files 116 116
Lines 10472 10472
==========================================
- Hits 9339 9338 -1
- Misses 1133 1134 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Micro-Benchmark ResultsLinux |
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.59134159684,
"stddev": 0.0836458781183094,
"median": 2.5911752857400003,
"user": 2.61777212,
"system": 3.58973528,
"min": 2.4510618397400004,
"max": 2.70920823474,
"times": [
2.6755091067400003,
2.63207949474,
2.5704805867400005,
2.67827890674,
2.51902404074,
2.5231601047400005,
2.70920823474,
2.4510618397400004,
2.54274366874,
2.61186998474
]
},
{
"command": "pacquet@main",
"mean": 2.5443089156400007,
"stddev": 0.07198495971113027,
"median": 2.52400633574,
"user": 2.6332149199999995,
"system": 3.5441085800000005,
"min": 2.43971203274,
"max": 2.6616668167400004,
"times": [
2.6616668167400004,
2.50677356074,
2.43971203274,
2.57386312274,
2.5158533387400004,
2.6591064017400003,
2.48343593374,
2.50666153574,
2.53215933274,
2.56385708074
]
},
{
"command": "pnpm",
"mean": 5.982697187340001,
"stddev": 0.08412187788573089,
"median": 5.96664880524,
"user": 8.73356862,
"system": 4.364365179999999,
"min": 5.87231352774,
"max": 6.14446690774,
"times": [
5.91819096874,
5.87231352774,
6.01929176274,
5.97738459674,
5.94169199074,
6.09063706674,
5.908507811740001,
6.14446690774,
5.95591301374,
5.998574226740001
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.74446345534,
"stddev": 0.04417683846389534,
"median": 0.73353599314,
"user": 0.3792123799999999,
"system": 1.61424154,
"min": 0.71090577314,
"max": 0.86344027914,
"times": [
0.86344027914,
0.75157403614,
0.72395968014,
0.7327054951399999,
0.73436649114,
0.72105970714,
0.75120014114,
0.74215438614,
0.71326856414,
0.71090577314
]
},
{
"command": "pacquet@main",
"mean": 0.75442093094,
"stddev": 0.025489927082225458,
"median": 0.74768326564,
"user": 0.37869198,
"system": 1.6364304399999998,
"min": 0.72576906214,
"max": 0.8026625281399999,
"times": [
0.76488882214,
0.7390847511399999,
0.7550102091399999,
0.72576906214,
0.77112438514,
0.72710985614,
0.8026625281399999,
0.73578011514,
0.78242325814,
0.74035632214
]
},
{
"command": "pnpm",
"mean": 2.54787054774,
"stddev": 0.13851187808441887,
"median": 2.54212733064,
"user": 2.90365048,
"system": 2.19310134,
"min": 2.39881021814,
"max": 2.87130400714,
"times": [
2.52309097814,
2.40670511014,
2.87130400714,
2.56116368314,
2.41890319114,
2.5739389261400003,
2.39881021814,
2.51505766114,
2.61576331314,
2.59396838914
]
}
]
} |
Summary
Slice 2 of #438 (the
nodeLinker: 'hoisted'umbrella). Smaller than expected — when I looked, the schema field was already in place from #332's original.modules.yamlport, so the work reduced to pinning behavior rather than building plumbing.Two round-trip tests + a doc-comment on the field.
What was actually missing
The umbrella's §"Pacquet's current state" table claimed
hoistedLocationswas "Missing entirely" — that was wrong. The field is atcrates/modules-yaml/src/lib.rs:212and serde-handles read/write correctly. What was missing:Record<string, string[]>shape upstream relies on.hoisted_dependencies,hoisted_aliases) have full docstrings; this one was silent.Tests added
Both live in
crates/modules-yaml/tests/real_fs.rs(pacquet-side, no upstream counterpart — upstream'sinstalling/modules-yaml/test/index.tsdoesn't exercisehoistedLocationsdirectly):hoisted_locations_round_trips— a manifest withhoistedLocationspopulated survives write+read; the raw on-disk JSON keeps the per-depPath array shape (multi-entry arrays included, to pin the nested-conflict layout).absent_hoisted_locations_is_omitted_on_write—None(the state every pacquet install writes today) serializes as the field absent from the file, matching upstream'sJSON.stringify(undefined)behavior. Guards against accidentalOption::is_someregressions on theskip_serializing_if.Regression catch verified
Per
plans/TEST_PORTING.md's "break the subject to verify the test catches it" convention, I temporarily added#[serde(rename = "wrongName")]to the field.hoisted_locations_round_tripsfailed at theexpect("present")on the deserialized value (the camelCase key no longer mapped). Reverted before commit.Type-shape decision
Upstream's actual
ModulesRaw.hoistedLocationsisRecord<string, string[]> | undefined— notRecord<DepPath, string[]>despite the values being populated from depPaths internally. The umbrella's scope item ("Option<BTreeMap<DepPath, Vec<String>>>") was inconsistent with the upstream schema; I kept the existingBTreeMap<String, Vec<String>>because:hoisted_dependenciesfield below, which deliberately keepsStringkeys (per its doc-comment at lines 148-153) because pnpm'sDepPath | ProjectIdunion can't be statically disambiguated.Test plan
hoisted_locations_round_trips— populated caseabsent_hoisted_locations_is_omitted_on_write—NonecasewrongNamecauseshoisted_locations_round_tripsto fail at the deserialize assertionjust ready(857 → 859 tests, all green)cargo doc --document-private-items,taplo format --check,just dylintall cleanWritten by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Documentation
Tests