fix(pacquet): byte-identical lockfile — generation-once owner records + cycle-closing edges#12365
Merged
Conversation
…nership generation The owner-keyed first-walk record was overwritten on every owner pass, so once the owning importer hoisted a peer into its own scope (e.g. typanion under @yarnpkg/core, owned by hooks/read-package-hook), every other importer's hoist of that peer was suppressed — upstream's once-per-generation missingPeersOfChildren promise keeps the owner walk's initial misses visible instead, and each importer hoists its own copy. The record now stores the recording owner: the owning importer writes once per ownership generation (its later post-hoist passes never refresh it), an ownership change records afresh, and an owner's report replaces a non-owner's provisional one. Restores the (typanion@3.14.0) / (@babel/types@7.29.7) suffixes that regressed with the deterministic-owner port; whole-monorepo lockfile diff vs fresh pnpm 11.6.0: 11 -> 3 changed lines. Part of #12266.
…entry pacquet's cycle break dropped the edge at the first re-entry of an ancestor package, so a snapshot like arraybuffer.prototype.slice's lost its cycle-closing es-abstract dependency line. pnpm's buildTree gate (parentIdsContainSequence) only drops a direct self-edge and the second lap of the full parent…child sequence: the first re-entry's node exists with its own back-edge pruned, and the previously-resolved-children merge in the peer pass — already ported, but dead code until now — restores the pruned children on the repeated node's graph entry. Ported the gate to the seed walk, the lockfile-reuse walk, and the lazy realization in the peer pass. Whole-monorepo lockfile diff vs fresh pnpm 11.6.0: 3 → 0 — the real lockfile document is now byte-identical, deterministic across runs. Part of #12266.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📜 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). (7)
🧰 Additional context used📓 Path-based instructions (1)pacquet/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
🧠 Learnings (8)📚 Learning: 2026-05-20T19:40:55.051ZApplied to files:
📚 Learning: 2026-05-22T00:08:44.646ZApplied to files:
📚 Learning: 2026-05-20T23:07:58.444ZApplied to files:
📚 Learning: 2026-05-24T21:10:50.292ZApplied to files:
📚 Learning: 2026-06-12T14:51:20.984ZApplied to files:
📚 Learning: 2026-05-24T21:10:50.292ZApplied to files:
📚 Learning: 2026-06-06T18:58:37.156ZApplied to files:
📚 Learning: 2026-06-12T20:41:57.558ZApplied to files:
🔇 Additional comments (4)
📝 WalkthroughWalkthroughThe PR refactors missing-peer tracking to record ownership metadata and changes cycle-edge detection to preserve first re-entries while dropping only full-sequence repeats. This enables cycle-closing edges to reach the lockfile snapshot and makes peer-record writes deterministic by generation. ChangesMissing-peer ownership tracking
Cycle-closing edge preservation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review by Qodo
Context used 1. Cycle comment mismatch
|
PR Summary by QodoFix lockfile parity: stable missing-peer recording and cycle-closing edges WalkthroughsDescription• Persist owner missing-peer reports once per ownership generation to match pnpm behavior. • Keep cycle-closing edges on first cycle re-entry across eager, reuse, and lazy walks. • Add regression tests covering missing-peer generation semantics and cycle edge retention. Diagramgraph TD
A["resolve_dependency_tree"] --> B[("WorkspaceTreeCtx")] --> C[("Missing-peer cache")]
A --> D{"Cycle gate"} --> E["Tree edges"] --> F["resolve_peers"] --> G["Lockfile graph"]
B --> F
subgraph Legend
direction LR
_p["Process"] ~~~ _d{"Decision"} ~~~ _s[("State")]
end
High-Level AssessmentThe following are alternative approaches to this PR: 1. Use explicit ownership-generation IDs
2. Adopt full upstream cycle detection verbatim (closer port)
Recommendation: Current approach (recording File ChangesBug fix (2)
Tests (2)
|
Contributor
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12365 +/- ##
==========================================
+ Coverage 88.15% 88.17% +0.01%
==========================================
Files 295 295
Lines 37733 37769 +36
==========================================
+ Hits 33265 33302 +37
+ Misses 4468 4467 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
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
2 participants
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.
Summary
The final two resolution-level parity fixes for #12266. With both applied,
pacquet install --lockfile-onlyon this monorepo produces a real-lockfile document that is byte-identical to fresh pnpm 11.6.0 output (back-to-back against the live registry, deterministic across runs) — down from ~3,200 changed lines when the umbrella issue was opened.1. Owner missing-peer records are written once per ownership generation
The pacquet side of #12362 overwrote the owner-keyed first-walk record on every owner pass. Once the owning importer hoisted a peer into its own scope (e.g.
typanionunder@yarnpkg/core, owned byhooks/read-package-hook), the record went empty and every other importer's hoist of that peer was suppressed — pnpm's once-per-generationmissingPeersOfChildrenpromise keeps the owner walk's initial misses visible instead, and each importer hoists its own copy. The record now stores the recording owner: the owning importer writes once per ownership generation (later post-hoist passes never refresh it), an ownership change records afresh, and an owner's report replaces a non-owner's provisional one. Restores the(typanion@3.14.0)/(@babel/types@7.29.7)suffixes (11 → 3 lines).2. A cycle's closing edge survives the first re-entry
pacquet's cycle break dropped the edge at the first re-entry of an ancestor package, losing snapshot lines like
es-abstract: 1.24.2underarraybuffer.prototype.slice@1.0.4. pnpm'sbuildTreegate (parentIdsContainSequence) only drops a direct self-edge and the second lap of the fullparent … childsequence; the repeated node's pruned back-edge is restored on its graph entry by the previously-resolved-children merge — already ported in pacquet's peer pass, but dead code until this gate. Ported to the seed walk, the lockfile-reuse walk, and the lazy realization (3 → 0 lines).Verification
owner_missing_record_is_written_once_per_generationandcycle_closing_edge_reaches_the_graph(both verified to fail without their fixes); fullresolving-deps-resolver(127),package-manager+cli(781) suites pass; clippy--deny warnings, rustfmt clean.What remains on the umbrella
Only the env-lockfile document (doc 1), which the issue originally scoped out: pacquet's first document lacks the
packageManagerDependenciesblock and thelibc:fields on the@pacquet/linux-*platform packages.Written by an agent (Claude Code, claude-fable-5).
Summary by CodeRabbit
Bug Fixes
Tests