fix: install dependencies added on the other lane during lane merge - #10579
Open
davidfirst wants to merge 2 commits into
Open
fix: install dependencies added on the other lane during lane merge#10579davidfirst wants to merge 2 commits into
davidfirst wants to merge 2 commits into
Conversation
PR Summary by QodoFix lane-merge installs for dependencies added on the other lane
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1. Unverified test precondition
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+337
to
+340
| // create a lockfile that does not include is-positive | ||
| helper.command.install(); | ||
| const laneWs = helper.scopeHelper.cloneWorkspace(); | ||
| helper.command.switchLocalLane('main', '-x'); |
There was a problem hiding this comment.
1. Unverified test precondition 🐞 Bug ☼ Reliability
The new e2e test claims the lane lockfile “does not include is-positive” but never asserts that is-positive is absent before the merge, so the test can pass without actually exercising the regression it’s meant to catch.
Agent Prompt
### Issue description
The new e2e scenario documents that the lane workspace’s lockfile/node_modules should not contain `is-positive` before merging `main`, but it does not verify that assumption. If `is-positive` becomes present (e.g., fixture changes, transitive deps, or state leakage), the test may pass even if the merge/install behavior regresses.
### Issue Context
The setup runs `helper.command.install()` and immediately snapshots the workspace, but there is no assertion that `node_modules/is-positive` (and/or the lockfile) is actually absent at that moment.
### Fix Focus Areas
- e2e/harmony/lanes/merge-lanes-main.e2e.ts[337-347]
### Suggested fix
Add a precondition assertion right after the initial `helper.command.install()` (and before cloning/switching lanes), e.g.:
- Assert `node_modules/is-positive` does **not** exist.
- Optionally assert the lockfile does **not** mention `is-positive` (depending on the package-manager configured for these e2e runs).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 6568919 |
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
1 participant
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.
Problem
When you merge a lane, and the incoming side added a new package dependency, the install step does not install the new package. The package manager shows "Lockfile is up to date, resolution step is skipped". The auto-snap that follows fails with a "missing packages" error.
Cause
The merge flow loads the components before it writes the merged files and the merged config. The install step that runs after the merge uses this stale cache. As a result, the install manifest does not contain the new dependency, and the package manager skips the resolution step.
The config-merger records the new dependency correctly in the unmerged-components store. Only the stale cache prevents the install from using it.
Fix
Clear the workspace cache before the install step of the merge. The install then loads the components with the merged state, and the new dependency enters the install manifest.
Added an e2e test that reproduces the failure and verifies the fix.