Read the two-document pnpm-lock.yaml written by pnpm 12 - #210
Merged
Conversation
pnpm 12 splits pnpm-lock.yaml into an env document holding configDependencies and packageManagerDependencies, followed by the project document. @pnpm/lockfile-file@9 parses with js-yaml.load(), which rejects multi-document streams, so isolate failed with "expected a single document in the stream, but found more" (#205). Point the pnpm_lockfile_file_v9 alias at @pnpm/lockfile.fs, the maintained successor of @pnpm/lockfile-file. It reads both shapes and still writes lockfile format v9 — the isolated lockfile for a pnpm 11 workspace is byte-identical to what the old reader produced. Also copy the env document into the isolated lockfile. Without it, running pnpm install --frozen-lockfile in the isolated directory fails with ERR_PNPM_FROZEN_LOCKFILE_WITH_OUTDATED_LOCKFILE, because the output manifest keeps the root packageManager field but the lockfile no longer pins it. The document describes the environment rather than the workspace graph, so it is copied verbatim; it is skipped when omitPackageManager drops the field.
Keep the lockfile env document when the output drops packageManager but the document still pins configDependencies. The env document records resolutions and integrity for specifiers that live in pnpm-workspace.yaml, which is copied to the isolate verbatim, so dropping the whole document under --omit-package-manager left those specifiers unresolvable. Only packageManagerDependencies is stripped now. Add an integration test that runs the real @pnpm/lockfile.fs reader against a two-document pnpm-lock.yaml written by pnpm 12. Every existing test mocks that module, so nothing exercised the parsing behavior the dependency swap was made for. Two fixtures cover the plain case and one with config dependencies. Also reset the isRushWorkspace mock in the outer beforeEach rather than a nested one, type the env fixture with satisfies instead of as never, and correct three stale comments: the lockfile module description in CLAUDE.md, a "copied verbatim" JSDoc, and a test title saying the env document is appended when it is prepended.
Extract getPnpmLockfileDir into src/lib/utils. The Rush-aware lockfile directory was computed identically in generate-pnpm-lockfile.ts, copy-patches.ts and collect-installed-names-pnpm.ts, and this branch already touched two of the three. The v8/v9 reader dispatch beside it is left alone: the three call sites differ in error handling, so folding them together would change behavior in the patch paths. Log which case produced a missing env document. readEnvLockfile returns null both for an ordinary single-document lockfile and for a two-document one whose env document it could not parse; only the second is a problem, and it used to return silently. Also rename hasEnvConfigDependencies to pinsConfigDependencies, note at the call site that the env write must follow the project write, record that the v9 reader migrates patchedDependencies to bare hashes, and correct the claim that pruning the unreferenced env entries would need a re-resolution — it would be a reachability walk, and it is skipped because pnpm never reads those entries.
Do not let an unreadable env document abort the isolate. readEnvLockfile special-cases only ENOENT and passes the document through yaml.load, so a lockfile whose project document read fine could still throw here and fail the whole run after that document was already written. It is now caught and warned about, which is also what closes the window where the isolate directory was left holding a project-only lockfile beside a manifest that keeps packageManager. Recognize the CRLF spelling in the env-document probe, since the reader normalizes line endings before it looks for the marker. Cover what the earlier fix rounds added: a unit test for getPnpmLockfileDir against real directories, and integration cases for a source lockfile with no env document and one whose env document cannot be parsed. The two patch test files no longer re-implement the Rush path in their mock factory, so the mock cannot drift from the helper. Also correct the integration test's claim that both fixtures were captured from a real pnpm 12 install: the config-dependencies one is hand-edited and its integrity hashes are placeholders.
The pnpm 12 lockfile fix shipped with every test mocking the package whose parsing behavior it was made for, so nothing exercised the change. Write the rule down where the next one will read it.
The repo sets minimumReleaseAge to 7 days, and 1100.2.4 was published five days ago, so CI refused the lockfile with ERR_PNPM_MINIMUM_RELEASE_AGE_VIOLATION across all four jobs. 1100.2.2 is two weeks old, exports the same readEnvLockfile, writeEnvLockfile and extractMainDocument, and pins older transitive @pnpm packages that clear the cutoff as well. Local installs missed this because pnpm skips the policy check when the tree is already up to date; a clean install reproduces it.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new “partial mocks” in two unit tests keep the real getPnpmLockfileDir, which consults the real filesystem via ./is-rush-workspace and makes the tests environment-dependent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates isolate-package’s pnpm v9 lockfile handling to support pnpm 12’s two-document pnpm-lock.yaml format (env document + project document), including propagating the env document into isolated output when needed to keep pnpm install --frozen-lockfile working.
Changes:
- Swap the pnpm v9 lockfile reader/writer alias from
@pnpm/lockfile-fileto@pnpm/lockfile.fs(maintained successor that understands multi-document YAML streams). - Copy the pnpm 12 “env lockfile” document into the isolated lockfile (and strip
packageManagerDependencieswhen--omit-package-manageris used, while preservingconfigDependenciessupport). - Centralize Rush vs non-Rush pnpm lockfile directory selection via
getPnpmLockfileDir, and add unit + integration coverage with real fixtures.
File summaries
| File | Description |
|---|---|
| src/lib/utils/index.ts | Exports the new getPnpmLockfileDir helper. |
| src/lib/utils/get-pnpm-lockfile-dir.ts | Adds a shared helper to locate the workspace pnpm-lock.yaml, including Rush layout. |
| src/lib/utils/get-pnpm-lockfile-dir.test.ts | Adds direct unit coverage for Rush vs non-Rush lockfile directory detection. |
| src/lib/patches/copy-patches.ts | Uses getPnpmLockfileDir for consistent pnpm lockfile location (Rush-aware). |
| src/lib/patches/copy-patches.test.ts | Switches to partial mocking approach intended to keep getPnpmLockfileDir “real”. |
| src/lib/patches/collect-installed-names-pnpm.ts | Uses getPnpmLockfileDir and widens importer map typing for v9 branded keys. |
| src/lib/patches/collect-installed-names-pnpm.test.ts | Switches to partial mocking approach intended to keep getPnpmLockfileDir “real”. |
| src/lib/lockfile/helpers/generate-pnpm-lockfile.ts | Uses @pnpm/lockfile.fs v9 API, prunes with casts, and propagates pnpm 12 env document into isolated output. |
| src/lib/lockfile/helpers/generate-pnpm-lockfile.test.ts | Extends unit tests for env document behavior and Rush lockfile directory reads. |
| src/lib/lockfile/helpers/generate-pnpm-lockfile.integration.test.ts | Adds integration tests with real fixtures to ensure multi-document read/write shape works end-to-end. |
| src/lib/lockfile/helpers/fixtures/pnpm-two-document/workspace/pnpm-workspace.yaml | Fixture workspace config for pnpm 12 multi-doc lockfile case. |
| src/lib/lockfile/helpers/fixtures/pnpm-two-document/workspace/pnpm-lock.yaml | Fixture pnpm 12 two-document pnpm-lock.yaml (env + project). |
| src/lib/lockfile/helpers/fixtures/pnpm-two-document/workspace/package.json | Fixture root manifest with packageManager pin. |
| src/lib/lockfile/helpers/fixtures/pnpm-two-document/workspace/apps/svc/package.json | Fixture app package manifest used by integration test isolation. |
| src/lib/lockfile/helpers/fixtures/pnpm-config-dependencies/workspace/pnpm-workspace.yaml | Fixture adding configDependencies to validate env doc retention when omitting packageManager. |
| src/lib/lockfile/helpers/fixtures/pnpm-config-dependencies/workspace/pnpm-lock.yaml | Fixture multi-doc lockfile including config dependency resolutions. |
| src/lib/lockfile/helpers/fixtures/pnpm-config-dependencies/workspace/package.json | Fixture root manifest with packageManager pin for config-deps case. |
| src/lib/lockfile/helpers/fixtures/pnpm-config-dependencies/workspace/apps/svc/package.json | Fixture app package manifest used by integration tests. |
| pnpm-lock.yaml | Updates repo lockfile to reflect @pnpm/lockfile.fs@1100.2.4 and its dependency graph. |
| package.json | Updates pnpm_lockfile_file_v9 alias to npm:@pnpm/lockfile.fs@1100.2.4. |
| .claude/CLAUDE.md | Updates repository notes to reflect the new pnpm v9 reader/writer package and adds integration-test guidance. |
Review details
Files not reviewed (3)
- pnpm-lock.yaml: Generated file
- src/lib/lockfile/helpers/fixtures/pnpm-config-dependencies/workspace/pnpm-lock.yaml: Generated file
- src/lib/lockfile/helpers/fixtures/pnpm-two-document/workspace/pnpm-lock.yaml: Generated file
- Files reviewed: 18/21 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
@pnpm/lockfile.fs reaches execa through network.git-utils and safe-execa, and execa's ^9.2.0 range resolves to pretty-ms@9.3.1, published a day ago and so inside the repo's 7-day minimumReleaseAge window. 9.3.0 satisfies the same range and is a year old. The override is temporary: once 9.3.1 has aged past the cutoff it can go.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The pinned @pnpm/lockfile.fs version in package.json does not match the version stated in the PR description and should be aligned to avoid confusion.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (3)
- pnpm-lock.yaml: Generated file
- src/lib/lockfile/helpers/fixtures/pnpm-config-dependencies/workspace/pnpm-lock.yaml: Generated file
- src/lib/lockfile/helpers/fixtures/pnpm-two-document/workspace/pnpm-lock.yaml: Generated file
- Files reviewed: 19/22 changed files
- Comments generated: 1
- Review effort level: Lite
The partial mock left the real helper in place, but the helper reads rush.json off disk through its own isRushWorkspace import, which a `#/lib/utils` factory cannot intercept. So those tests consulted the filesystem and ignored their own isRushWorkspace mock. Neither suite exercises a Rush workspace, and get-pnpm-lockfile-dir.test.ts covers both branches directly, so mock it to the workspace root and say why.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
There are a few correctness/consistency issues to address (misleading new error message, PR description vs pinned dependency version mismatch, and unit-test determinism concerns in new partial mocks).
Review details
Files not reviewed (3)
- pnpm-lock.yaml: Generated file
- src/lib/lockfile/helpers/fixtures/pnpm-config-dependencies/workspace/pnpm-lock.yaml: Generated file
- src/lib/lockfile/helpers/fixtures/pnpm-two-document/workspace/pnpm-lock.yaml: Generated file
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
src/lib/patches/copy-patches.test.ts:19
- This partial mock keeps getPnpmLockfileDir "real", but the real implementation uses fs.existsSync via isRushWorkspace in a different module, so it can make the unit test environment-dependent (and it is not controlled by the isRushWorkspace mock below). To keep tests deterministic, explicitly mock getPnpmLockfileDir based on the mocked isRushWorkspace result.
vi.mock("#/lib/utils", () => ({
filterPatchedDependencies: vi.fn(),
getIsolateRelativeLogPath: vi.fn((p: string) => p),
getPackageName: vi.fn((spec: string) => {
if (spec.startsWith("@")) {
src/lib/patches/collect-installed-names-pnpm.test.ts:22
- This partial mock leaves getPnpmLockfileDir real, which makes the unit test depend on the real filesystem (via isRushWorkspace's existsSync). To keep the test deterministic, mock getPnpmLockfileDir explicitly based on the mocked isRushWorkspace function.
vi.mock("#/lib/utils", () => ({
getPackageName: vi.fn((spec: string) => {
if (spec.startsWith("@")) {
const parts = spec.split("@");
return `@${parts[1] ?? ""}`;
src/lib/lockfile/helpers/generate-pnpm-lockfile.ts:92
- The assertion error message still says the lockfile is missing at workspaceRootDir, but the read now happens from lockfileRootDir (which differs for Rush workspaces). This can mislead users when the lockfile is expected under common/config/rush.
assert(lockfile, `No input lockfile found at ${workspaceRootDir}`);
package.json:72
- PR description and linked issue discussion refer to pnpm_lockfile_file_v9 resolving to @pnpm/lockfile.fs@1100.2.4, but the code pins 1100.2.2 here (and the lockfile matches). Please reconcile by updating the description or bumping the dependency (and regenerating pnpm-lock.yaml) so the implemented version matches what was verified.
"meow": "^14.1.0",
"outdent": "^0.8.0",
"pnpm_lockfile_file_v8": "npm:@pnpm/lockfile-file@8",
"pnpm_lockfile_file_v9": "npm:@pnpm/lockfile.fs@1100.2.2",
"pnpm_prune_lockfile_v8": "npm:@pnpm/prune-lockfile@5",
"pnpm_prune_lockfile_v9": "npm:@pnpm/prune-lockfile@6.0.0",
- Files reviewed: 19/22 changed files
- Comments generated: 0 new
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
src/lib/lockfile/helpers/generate-pnpm-lockfile.ts introduces an implicitly-any local (let envLockfile;) that is likely to fail type-checking under the repo’s TypeScript settings.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (3)
- pnpm-lock.yaml: Generated file
- src/lib/lockfile/helpers/fixtures/pnpm-config-dependencies/workspace/pnpm-lock.yaml: Generated file
- src/lib/lockfile/helpers/fixtures/pnpm-two-document/workspace/pnpm-lock.yaml: Generated file
- Files reviewed: 19/22 changed files
- Comments generated: 1
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The compatibility fix is coherent and covered by focused unit and unmocked integration tests.
Review details
Files not reviewed (3)
- pnpm-lock.yaml: Generated file
- src/lib/lockfile/helpers/fixtures/pnpm-config-dependencies/workspace/pnpm-lock.yaml: Generated file
- src/lib/lockfile/helpers/fixtures/pnpm-two-document/workspace/pnpm-lock.yaml: Generated file
- Files reviewed: 19/22 changed files
- Comments generated: 0 new
- Review effort level: Balanced
`let envLockfile;` typechecks fine, since TypeScript widens an unannotated `let` from its assignments, but spelling out `EnvLockfile | null` saves the reader working that out.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The dependency migration and environment-document handling are focused and covered by unit and unmocked integration tests.
Review details
Files not reviewed (3)
- pnpm-lock.yaml: Generated file
- src/lib/lockfile/helpers/fixtures/pnpm-config-dependencies/workspace/pnpm-lock.yaml: Generated file
- src/lib/lockfile/helpers/fixtures/pnpm-two-document/workspace/pnpm-lock.yaml: Generated file
- Files reviewed: 19/22 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
isolate-package@1.36.0fails on any workspace installed with pnpm 12. pnpm now writespnpm-lock.yamlas a stream of two YAML documents, an env document holdingconfigDependenciesandpackageManagerDependenciesfollowed by the project document. The pinned reader@pnpm/lockfile-file@9.0.0parses withjs-yaml.load(), which rejects multi-document streams, so the run dies with "The lockfile at ... is broken: expected a single document in the stream, but found more".The
pnpm_lockfile_file_v9alias now resolves to@pnpm/lockfile.fs@1100.2.2, the maintained successor of@pnpm/lockfile-file, whose last release predates the format. It reads both shapes and still writes lockfile format v9. For a pnpm 11 workspace the isolated lockfile is byte-identical to what the old reader produced, so nothing changes for anyone already on a working version. The newer types needed a few casts where they meet@pnpm/prune-lockfile, which carries its own copy of the lockfile types and brands importer keys.Reading the lockfile is only half of it. The issue reports a second failure behind the first: the isolated manifest keeps the root
packageManagerfield, but the isolated lockfile had no env document, sopnpm install --frozen-lockfileinside the isolate fails withERR_PNPM_FROZEN_LOCKFILE_WITH_OUTDATED_LOCKFILE. The env document is now carried into the isolated lockfile. When--omit-package-managerdrops the field, onlypackageManagerDependenciesis stripped and the document still goes out if it pinsconfigDependencies, becausepnpm-workspace.yamlis copied verbatim and keeps declaring them.Verified by hand against real pnpm 12 workspaces, since the automated tests assert the shape of the emitted lockfile rather than that pnpm accepts it: a plain workspace, one with an internal workspace dependency and a catalog, and the
--omit-package-managerpath. Each isolate installs withpnpm install --frozen-lockfile --prodand resolves at runtime.Closes #205
Follow-ups: #207 (the isolated lockfile still writes
patchedDependenciesin the pre-pnpm-11 shape), #208 (the v8/v9 reader fork is written out in three modules), #209 (env document edge cases left unhandled, including Rush with config dependencies)Decisions:
@pnpm/lockfile.fsto 1100.2.2 rather than the current 1100.2.4. CI rejected 1100.2.4 under the repo's 7-dayminimumReleaseAgepolicy, since it was published five days ago. 1100.2.2 is two weeks old and exports the samereadEnvLockfile,writeEnvLockfileandextractMainDocument.pretty-msat 9.3.0 through apnpm-workspace.yamloverride, for the same policy. It arrives transitively throughnetwork.git-utilsandsafe-execa, and its newest patch was published a day before this branch. The override is temporary and can go once that version has aged past the cutoff. Pinning to an older release was the conservative option; excluding the package from the policy would have weakened it.pnpm_lockfile_file_v9even though it now resolves to a differently named package. It names the lockfile format rather than the package, which matchespnpm_prune_lockfile_v9, and renaming it would touch four files for no behavior change..claude/CLAUDE.md: a change made to get different behavior out of a dependency needs at least one test that exercises that dependency unmocked. This branch shipped its first round with every test mocking the package the whole change was about.