Update: I tried running this on real-world setups, and we sometimes get an error like:
expected hoisted manifest for <some-package-name>
I'm investigating although my knowledge around this area is limited :)

I'm working on a different implementation, the original one is too intrusive and changes too much of how yarn works.

@VincentBailly thank you for helping pushing this forward :)
I just pushed a hopefully working version. It is much less intrusive, doesn't touch any logic but the missing packages integrity check, which fixed the hoisting errors I had.

Is it possible to run this PR in CI? I don't see any automatic status checks.

@VincentBailly sorry for bothering, if you could just let me know when you can review so I could adjust my plans accordingly, thanks!

@ohana54 My apology, I missed your previous message, I will look into it tomorrow.

No problem, really appreciate your help 🙂

I had a try and I observed that the change suggested addresses only one of the various scenarios that would result in a yarn.lock update:

• adding a new dependency in a package.json file: this PR fixes this scenario
• remove a dependency in a package.json file: this scenario is still broken, no error is reported
• manually modifying the yarn.lock file so we have either too many or too few entries: this scenario is still broken (for example, if we manually remove a dependency of our dependencies then yarn will not complain).

From what I see, it seems like the right place to fix this issue is at the yarn.lock generation step. We could compare the generated yarn.lock and the yarn.lock on disk and then fail if they differ, that would prevent us to need to think of all the possible scenarios leading to a modification of the yarn.lock file. This would also be way easier to code and to maintain.

Does it make sense to you @ohana54, or am I missing something? Would you be willing to do this change?

Thanks for the review, I think it does make sense. I was so focused on the integrity checker being wrong that I haven't thought of that simple solution.

It will require to generate a lockfile inside the integrity checker, meaning we still need to have all the patterns there in order to generate it (similar to the current fix).
Then replace the current missingPatterns check with a lockfile comparison.

Is that direction ok?

@VincentBailly pushed a new version that does the comparison.
Note that although yarn loads the original lockfile at the start of the install flow, it is being mutated (by calls to getLocked), therefore I had to load it again.

Testing the following scenarios:

• removing a dependency from a package.json
• adding a new dependency to a package.json
• manually removing a transitive dependency from the lock file
• lock file in a merge conflict state

This code correctly failed in all of the tested scenario.

The code looks good to me, thanks for the contribution.

Great work, the version 1.23.21 has been release with this fix.

Thank you so much! We're so happy this is behind us, we're trying to have reproducible CI builds and this olwas a major blocker.