fix: handle lockfile conflicts in optimistic install#11605
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
📜 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). (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{js,ts,tsx,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📚 Learning: 2026-05-14T09:04:00.133ZApplied to files:
🔇 Additional comments (6)
📝 WalkthroughWalkthroughAdds a synchronous helper to detect merge markers in wanted lockfiles, wires conflict detection into deps.status (early-failing when conflicts exist), adds filesystem-backed tests for conflict scenarios, and includes a changeset release note. ChangesLockfile Merge Conflict Detection and Gating
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
deps/status/test/checkDepsStatus.test.ts (1)
399-447: ⚡ Quick winAdd a regression case for
sharedWorkspaceLockfile: falsewith conflict markers in a project lockfile.Current coverage validates only one lockfile path. A per-project lockfile conflict case would protect against false
upToDate: truewhen manifests are unchanged.🤖 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 `@deps/status/test/checkDepsStatus.test.ts` around lines 399 - 447, Add a regression test mirroring the existing "wanted lockfile has merge conflict markers" case but for per-project lockfiles by setting sharedWorkspaceLockfile: false in the CheckDepsStatusOptions and ensuring the mock workspace state includes a project entry; create a project dir with a pnpm-lock.yaml containing conflict markers, call checkDepsStatus(opts) where opts includes sharedWorkspaceLockfile: false and rootProjectManifestDir pointing to the workspace root and pnpmfile/paths as needed, then assert result.upToDate is false and result.issue mentions the specific project lockfile path; reference the existing test block in checkDepsStatus.test.ts, the checkDepsStatus function, and the CheckDepsStatusOptions/sharedWorkspaceLockfile flag to locate where to add the new it(...) test.
🤖 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.
Inline comments:
In `@deps/status/src/checkDepsStatus.ts`:
- Around line 171-178: The current early-return only checks a single
wantedLockfileDir for merge conflicts (computed as lockfileDir ?? workspaceDir
?? rootProjectManifestDir) which misses per-project lockfiles when
sharedWorkspaceLockfile is false; update the logic in checkDepsStatus (where
wantedLockfileDir and lockfileHasMergeConflicts are used) to build and iterate
over all relevant lockfile directories: if sharedWorkspaceLockfile is true keep
the existing single check, otherwise gather each project's lockfile directory
(e.g., per-project manifest dir or project.lockfileDir) and call
lockfileHasMergeConflicts for each one, returning upToDate: false and the
appropriate issue as soon as any conflicted lockfile is found.
---
Nitpick comments:
In `@deps/status/test/checkDepsStatus.test.ts`:
- Around line 399-447: Add a regression test mirroring the existing "wanted
lockfile has merge conflict markers" case but for per-project lockfiles by
setting sharedWorkspaceLockfile: false in the CheckDepsStatusOptions and
ensuring the mock workspace state includes a project entry; create a project dir
with a pnpm-lock.yaml containing conflict markers, call checkDepsStatus(opts)
where opts includes sharedWorkspaceLockfile: false and rootProjectManifestDir
pointing to the workspace root and pnpmfile/paths as needed, then assert
result.upToDate is false and result.issue mentions the specific project lockfile
path; reference the existing test block in checkDepsStatus.test.ts, the
checkDepsStatus function, and the CheckDepsStatusOptions/sharedWorkspaceLockfile
flag to locate where to add the new it(...) test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c51ce8e7-1926-475f-bc45-9ccdfc936565
📒 Files selected for processing (4)
.changeset/fix-optimistic-lockfile-conflict.mddeps/status/src/checkDepsStatus.tsdeps/status/test/checkDepsStatus.test.tsinstalling/commands/src/installDeps.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx,jsx}: Use trailing commas in code following Standard Style with modifications
Prefer functions over classes
Declare functions after they are used, relying on hoisting
Functions should have no more than two or three arguments; use a single options object for functions needing more parameters
Organize imports in order: standard libraries, external dependencies (sorted alphabetically), then relative imports
Files:
installing/commands/src/installDeps.tsdeps/status/src/checkDepsStatus.tsdeps/status/test/checkDepsStatus.test.ts
**/*.test.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
instanceof Errorfor error type checking in Jest tests; useutil.types.isNativeError()instead to ensure compatibility across realms
Files:
deps/status/test/checkDepsStatus.test.ts
🔇 Additional comments (2)
.changeset/fix-optimistic-lockfile-conflict.md (1)
2-7: Changeset entry looks accurate and aligned with the fix scope.installing/commands/src/installDeps.ts (1)
169-169: Good guard: optimistic repeat-install is now correctly blocked when lockfile conflict markers are present.Also applies to: 472-483
There was a problem hiding this comment.
Pull request overview
This PR improves optimisticRepeatInstall correctness by preventing “Already up to date” short-circuits when the wanted lockfile is conflicted, and by ensuring patch/pnpmfile modifications are considered even when workspace manifests haven’t changed (fixing #11604).
Changes:
- Detect merge-conflict markers in
pnpm-lock.yamland treat the dependency state as not up to date. - Reorder
checkDepsStatusso patch/pnpmfile changes are evaluated before the “no manifests changed” early exit. - Add regression tests and a changeset for the affected packages.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| installing/commands/src/installDeps.ts | Skips optimistic fast-path when the lockfile appears conflicted by adding a merge-conflict marker check. |
| deps/status/src/checkDepsStatus.ts | Treats conflicted wanted lockfiles as not up to date; moves patch/pnpmfile checks before manifest early-exit; adds lockfileDir support. |
| deps/status/test/checkDepsStatus.test.ts | Adds test coverage for patch-only changes and for conflicted lockfiles returning upToDate: false. |
| .changeset/fix-optimistic-lockfile-conflict.md | Declares patch releases for the impacted packages and documents the fix. |
Comments suppressed due to low confidence (1)
installing/commands/src/installDeps.ts:173
installDeps()now checkslockfileHasMergeConflicts()before callingcheckDepsStatus(), butcheckDepsStatus()also reads the wanted lockfile to detect merge conflicts. In the common conflict-free case this will readpnpm-lock.yamltwice, which undermines the performance benefit ofoptimisticRepeatInstall. Consider removing the pre-check here and relying on thecheckDepsStatusresult (or otherwise sharing the read result) so the lockfile is only read once.
if (!opts.update && !opts.dedupe && params.length === 0 && opts.optimisticRepeatInstall && !await lockfileHasMergeConflicts(opts.lockfileDir ?? opts.workspaceDir ?? opts.dir)) {
const { upToDate } = await checkDepsStatus({
...opts,
ignoreFilteredInstallCache: true,
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8f26b4c to
265f7ff
Compare
|
compilation fails |
`sharedWorkspaceLockfile` is not in `WORKSPACE_STATE_SETTING_KEYS`, so placing it inside `WorkspaceState.settings` broke type checking. Pass it directly on the `CheckDepsStatusOptions` instead.
`@pnpm/lockfile.fs` was added as a runtime dependency but the tsconfig project references were not updated. meta-updater enforces this in CI.
265f7ff to
38c0e52
Compare
|
Compilation fix pushed:
Local Written by an agent (Claude Code, claude-opus-4-7). |
…tate The first iteration of the conflict-detection fix unconditionally read pnpm-lock.yaml on every install - once in installDeps and again inside checkDepsStatus - defeating the point of optimisticRepeatInstall, which was to skip reading the lockfile entirely when nothing changed. Restore the fast path by: - Dropping the redundant lockfile read from installDeps. checkDepsStatus already returns upToDate: false when the lockfile is conflicted, so the pre-check was dead weight. - Gating the conflict check inside checkDepsStatus on the lockfile's mtime: if it hasn't been touched since the last successful install, it cannot have grown conflict markers, so the read is skipped. Conflict markers introduced after a successful install (e.g. via git pull/merge) still update the lockfile mtime, so the correctness fix is preserved.
|
Restored the Changes:
Conflict markers introduced after a successful install (e.g. via
Written by an agent (Claude Code, claude-opus-4-7). |
findConflictedLockfileDir awaits its work serially with no concurrent operations to interleave, so the async overhead (Promise.all microtasks, event-loop hops) buys nothing. Convert to a plain for-loop with fs.statSync and a new wantedLockfileHasMergeConflictsSync export. Also resolves a test-mock issue: the previous version called safeStat from deps/status, which is jest-mocked across the test file. The mocked safeStat returned undefined (or stale stats from earlier tests), causing the conflict check to silently no-op. Switching to fs.statSync bypasses the mock and gets the real mtime of the temp lockfile the regression tests write.
|
Two improvements in 44bdc98: Sync conversion. The conflict check awaits its work serially with no concurrent operations to interleave, so async buys nothing. Converted Fixes the CI test failure. The failing tests in run 25851101531 were caused by the mtime gate calling Written by an agent (Claude Code, claude-opus-4-7). |
|
Congrats on merging your first pull request! 🎉🎉🎉 |
|
@zkochan thanks for finishing this! |
|
no problem, thanks for the contribution🎉 |
Summary
optimisticRepeatInstallfast path when the wanted lockfile contains merge conflict markersFixes #11604.
Testing
corepack pnpm --filter @pnpm/deps.status exec tsgo --buildcorepack pnpm --filter @pnpm/deps.status exec eslint "src/**/*.ts" "test/**/*.ts"corepack pnpm --filter @pnpm/installing.commands exec tsgo --buildcorepack pnpm --filter @pnpm/installing.commands exec eslint "src/**/*.ts"pnpm-lock.yamlconflict markers instead of printingAlready up to dateNotes:
corepack pnpm --filter @pnpm/deps.status test -- checkDepsStatus.test.tscurrently fails before running tests in this checkout withmodule is already linked; the same failure also occurs for an existing deps/status test, so I validated with direct compile/lint plus the end-to-end repro.pn -F=pnpm compilebecause that script removedpnpm/distand then invokedpnpm/bin/pnpm.mjs, which imports the removed bundle.Summary by CodeRabbit
Bug Fixes
Releases
Tests