fix: issue-5850 - Settle override conflicts between edges and propagate changes

[AlonNavon]

A first step in fixing the overrides feature. In this PR I'm aiming to fix 3 bugs:

1. When we add an edge going into a node we update the node's overrides, but we don't update the overrides of that node's outgoing edges, and so forth. We need the up-to-date overrides to filter through.
2. When we remove an edge going into a node we don't update the overrides at all (and we don't update the outgoing edges like in the previous bug).
3. When we add an edge going in, and we already have a different override set for the node, we just ignore the existing override set and overwrite it with that of the new edge. Instead, this PR chooses the most specific override set. This still isn't the absolutely correct logic, since different override sets can have implications down the line of the dependency chain, but it has the advantage of being consistent (instead of just going with the last edge in). Also, it raises an error if it encounters a real conflict, meaning two incoming edges with override sets that aren't just a subset of one another.
   So if we have dependency chains A->B->C and A->C, and we override C under B, then C will be overridden.

References

Fixes some of the override issues.

[wraithgar]

This is going to need tests

[wraithgar]

The edge's reload seems to me where this kind of thing should be happening. Is this a more subtle error perhaps where we're not reloading the overrides where we should?

[AlonNavon]

The edge's reload seems to me where this kind of thing should be happening. Is this a more subtle error perhaps where we're not reloading the overrides where we should?

There are several bugs in the mechanism, and this is just the first fix.
Here I handle the case where a node has two incoming edges with different override sets, and I percolate it downwards in the dependency tree. So I'm not sure why it should be in the edge's reload.
If you @wraithgar have time I can hop on a Google meet to discuss (and explain the other bugs).

[AlonNavon]

@wraithgar
The linter and the tests are successful (except a random failure in macOS 18.0 which is unrelated to the PR).

[AlonNavon]

@wraithgar Anything else we need to do to merge this?

[sgarfinkel]

@AlonNavon This looks awesome, really looking forward to having overrides work more the way you'd expect. With this change, will running npm install re-apply any npm overrides as expected, or will running npm u still be required? Hopefully this gets approved soon 👍

[AlonNavon]

@sgarfinkel
Hey, this is just the first fix. Concretely, there are at least 5 identifiable bugs. It fixes (1) and (2) and gives a reasonable solution for (3). I have other short PRs planned for (4) and (5), but first I want to merge this one. With those merged the behavior should be stable and correct (up to some freaky edge cases regarding the conflict resolution).
But I haven't been able to get this one merged yet. Hopefully after the holidays it will get renewed attention. Every upvote counts.

The major bugs:

1. No percolation to the out-edges.
2. Not updating when deleting in-edges.
3. No conflict resolution of override sets (though ultimately, edges with conflicting overrides shouldn't be valid and be deduplicated IMHO).
4. A certain flow that updates to the parent's overrides which doesn't make sense.
5. Mishandling version ranges on edges.

[wraithgar]

Anything else we need to do to merge this?

Just patience. Holidays are over and we got a lot of PRs. This PR is on the work board it's not been forgotten.

[sgarfinkel]

Why was this closed?

[ljharb]

The PR wasn't made from the OP's fork, but presumably from their company's (which means that maintainers can't push to the PR branch, btw - always only make PRs from your own personal forks), and presumably someone at their company deleted the fork.

[AlonNavon]

Yeah, this was a mistake on our end. Our devops deleted the repo by accident.
We restored the repo, but we need some maintainer with permissions to reopen the PR.
@sgarfinkel @ljharb @wraithgar

[jbjhjm]

Can we have an update on this please? The PR seems to be on pause for months, but it adds important bugfixes.
All lights are green, are there any problems remaining to be solved before merging it @wraithgar ?

[jakubmazanec]

@AlonNavon @ljharb What is the status on this PR? Can we please finally merge it? It's been 2,5 years since the original issue (#4232) , and we still have to perform complete clean npm install every time when we need to have some overrides (not mentioning the obvious security issues). Come on 🙏

[jdalton]

Adding my voice to the "this would be great to land".
If it needs more work Socket Security can put their dev team on it.
(I'm on the Socket dev team and it would be me 😸)

Update:

@AlonNavon thank you for digging into this! This is not a simple thing 😵

[AlonNavon]

@jdalton @jakubmazanec @jbjhjm
The whole overrides mechanism is broken and in my opinion non-functional - it's inconsistent between two consecutive runs of npm install, it silently reverts overrides without so much as showing a warning, sometimes it just ignores the overrides completely, sometimes it only applies for some instances but not all of the overridden library.
The underlying issue was identified at least going back to 2022. When I realized that it silently reverts overrides (which are often done due to security issues within transitive dependencies) - I contacted the npm team and convinced them to upgrade it to a P0 bug.

Several months later, after the issue still went unresolved, I dove deep into the code and found exactly what's broken in it (the description at the header is the tip of the iceberg). I've done numerous fixes and got it to work properly on numerous test cases.
So I opened this PR as the first step in fixing the mechanism. Several more PRs are necessary, but I wanted to add them incrementally to make sure we don't create accidental breakages (some people might be implicitly relying on the random erroneous behavior).
However almost a year later, despite being aimed at solving a P0 bug, and even though npm major version 10 has been released, this PR still hasn't gone through any kind of code review (and at least insofar as to how correctly raise errors or write warnings in the CLI - it needs the maintainers' attention). I've mapped out the bugs, and I'll be happy to engage with the maintainers regarding the code fixes if they prefer to write them themselves.

But right now except forking npm I'm not sure what to do...

[wraithgar]

Thanks @AlonNavon for making this PR, it takes quite a bit of time and effort to fix these kinds of bugs. Arborist is a very complicated piece of npm.

It's unfortunate that this PR has sat so long, but this has proven to require more focused attention than we were able to give it. It also of course fell through the cracks as time went on, and as other priorities demanded attention. External PRs are the lifeblood of npm, and having them sit like this is does nobody any good.

So, again, thank you for your time on this and for continuing to bring it back to our attention.

Yes, overrides needs work. That you are willing not only to fix an immediate problem but are willing to work on this in phases to fix things more holistically is, frankly, wonderful. Doubly so given that you hadn't received much feedback in the interim. Thank you for sticking with it.

Thank you also @jdalton for offering to help. It would be most welcome.

Currently we are working on getting npm 11 started, so that we don't fall behind Node.js's release cycle. It is a very conservative release, mostly cleanup of old code whose removal constitutes a breaking change. And of course there is the engines update which allows us to keep our dependencies up to date. There is also a growing list of external PRs that we need to review. Any PR that is beyond a simple bug fix does take some dedicated time and focus, and that is a precious commodity.

This bug fix is important, fixing overrides is important. So let's get in to what will help this land.

First off, the code at a glance looks fine. What gives us the most pause is the large chunk of net new code in Arborist. This is by necessity something that we have to take more care with than a typical PR. The tests for Arborist aren't the strongest: they are mostly unit tests with not a lot of them interacting w/ the module as a whole. Side effects usually are a problem with these PRs. Given that y'all are willing to stick with this makes it easier to accept, because the thing we are mostly trying to avoid is tech debt that nobody is going to have capacity to go address.

Second off, having another party willing to help review helps. I think if we get to a point where the PR author, @jdalton, and myself think things look good we can land it.

Finally, I would appreciate a more specific callout of what you need from me. I know it's probably frustrating to be given MORE work here, especially since you've put so much into this already. Unfortunately that's the reality we're dealing with. This isn't something I'm going to be able to focus on deeply enough to know what you need from me. So let's start here:

and at least insofar as to how correctly raise errors or write warnings in the CLI - it needs the maintainers' attention

Please @ mention me to point out where in the code you're looking for this feedback.

Thanks to all who have interacted with this issue, even if it was a simple upvote.

[wraithgar]

We would of course prefer that the majority of our work happen in GitHub issues and PRs, but if you feel you need to talk in a context that is more high bandwidth (but also more time constrained) you can find us in the #npm channel in the OpenJS slack.

[AlonNavon]

@wraithgar This case means that the same node has two edges with completely different override sets pointing to it. This log is useful for debugging, not sure if just to remove the line or there's some other convention used in the arborist.

[AlonNavon]

@wraithgar This case means that the same node has two edges with completely different override sets pointing to it, which means that this node can't really decide which one is correct.
Our options are:

1. Go with the flow - in the current logic the out edges continue with the first overrides set they got, so this isn't breaking comparing to the existing logic. But it's obviously the wrong behavior.
2. Raise an error - just fail the command. Not sure how you do that in the arborist.
3. Duplicate the node - this is the same package with different override requirements. The proper logic is to duplicate the node, and then dedup it later if there's no conflict.

Currently I chose (1), but maybe it should at least show some kind of warning.

[AlonNavon]

@jdalton
It's been a year since I wrote the fix, and I could use going over the fix with someone and explain what's going on, and what are the remaining issues. If you're interested you can email me at alon@sealsecurity.io.

[jdalton]

@AlonNavon Emailed. Yes, lets sync up 😸

Update::

• Synced with @AlonNavon yesterday
• Trying the fix out on the Socket side of things today
  (finding things to tweak so will give it some time for the dust to settle and report back)
• Optimistic 🎉 🕺