Conversation
…ated UpgradeTransitiveDependencyVersion writes the override into package.json but leaves package-lock.json untouched and reports success, producing a commit that npm ci rejects. An override changes no declared dependency, so patchEditedDependencies throws RESOLUTION_REQUIRED, which RESOLVABLE_REASONS downgrades into a retry against whole-closure resolution. That resolver never reads overrides, so it rebuilds an identical graph and returns success. Adds an engine-level test pinning the mechanism and a recipe-level test pinning the consumer contract. Both fail by design; no production change. overridesOutsideDeclaredDependenciesFailsLoud appeared to cover this but passes only because it stubs no registry, so the fallback 404s and the original deferral is rethrown. Comment added recording that.
An override changes no declared dependency, so regeneration falls through to whole-closure resolution, which seeds from dependencies/devDependencies/ optionalDependencies only and never reads overrides. It rebuilt an identical graph and reported success over a lock that still pinned the old version. Extract the declared overrides per package manager and refuse in NpmGraphBuilder.select, the single funnel every direct and transitive requirement passes through. A package nothing depends on is never selected, so an override naming something outside the closure stays the no-op it is. Unsupported key forms are refused at extraction instead of dropped, because a key that is not a package name would never reach select and would resolve to a closure that silently ignores it. That includes the path-scoped keys this recipe writes for a dependencyPath run: pnpm "express>accepts" and yarn "express/accepts". parsePath already separates those from a scoped name like "@types/node".
Rewrite the requested range in NpmGraphBuilder.select before the dedupe, so a locked version that no longer satisfies the overridden range is re-resolved rather than kept. The substitution has to precede the dedupe or an already-chosen version wins and the override silently loses. Only npm applies. Resolution is package-manager agnostic, but each manager renders its own lock format through its own patcher and only npm has a fixture covering an applied override, so the others keep failing loud until each has a test.
Each refusal branch asserts its own message, so a test that passes proves the branch was reached rather than that the run failed earlier for another reason. Includes the scoped-name case: @types/node also contains a slash but is a plain package, not a path, and must resolve as a no-op rather than be refused.
A manifest carrying an override its lock contradicts is the broken state this recipe exists to prevent: npm ci rejects it outright. Returning the edited manifest with a warning still emitted that state, just annotated. On a failed regeneration, return the original manifest with the warning and keep the edit out of the shared live tree, so a later dependency recipe does not read an override that was never applied.
Refuse for the unsupported package managers before inspecting entries, so the reason reported is the same one whatever the entries contain, and wrap the whole extraction: parsePath rejects a malformed key by throwing, which would otherwise escape as a recipe crash rather than the warning the caller turns a failure into.
A dependencyPath run writes {"parent": {"child": range}}. Apply it to the whole
closure, then prove that was the same answer: requireScopeHolds refuses unless
the parent is the child's only requirer, since any other requirer would have
kept its own version and needed a second copy this engine does not place.
The nested write path reparses the document, so guard it the way setFlatEntry
already guards the un-nested one, or the entry is rewritten every cycle and the
recipe never stabilises.
Deeper nesting stays refused.
The nested write path re-serialised the whole document, so a dependencyPath run reformatted every line of package.json. Rebuild only the overrides value and splice it back in; every other member keeps its original whitespace. The test now asserts the manifest's own formatting survives, which is what caught this: the expected output had spaces before colons throughout.
requireScopeHolds decides whether a scoped override could be applied globally, and nothing exercised it. Both refusals are now covered: another package requiring the child, and the root declaring it directly. The global form of the same two-requirer shape resolves, which is the contrast that makes the refusal meaningful rather than blanket caution.
Downgrading and introducing a new package both resolve. Orphaning one does not: the engine has no prune edit for that shape and refuses, leaving the lock untouched and naming the package. Asserted as it behaves, so the gap is visible rather than assumed working.
Import the Jackson and json.tree types rather than naming them inline, reuse detectIndentUnit and makeMember from PackageJsonHelper instead of copying them, and configure the pretty printer for the separator and indent unit rather than patching its output with a string replace and a hand-rolled re-indent. Dropping the placeholder member also removes the risk of __placeholder__ being written into a manifest if the follow-up replace ever missed.
selectDep routes an npm: alias to selectAlias, which keys the slot by the alias name and resolves the real package itself, so it never reaches select. An override naming that dependency was skipped without trace and the run reported success over a lock that still held the old version. select is not the single funnel every requirement passes through, which is what made this easy to miss. Refusing keeps the do-no-harm contract; applying the override there needs a decision about which name npm matches on.
npm auto-installs an unmet non-optional peer through installMissingPeers, which resolves the version straight off the registry and never reaches select. An override naming that peer was skipped without trace and the run reported success over a lock that still held the old version. Second path found that goes around select, after the aliased dependency. Listed in the PR as needing follow-up because no reproducing case had been built; the earlier probe put the peer on a version nothing resolved to, so installMissingPeers never ran.
A manifest edited without its lock does not install. npm ci reports EUSAGE for the pair either way: "Invalid: lock file's is-number@6.0.0 does not satisfy is-number@7.0.0" for an override, "Missing: is-even@1.0.0 from lock file" for an add. So a failed regeneration that keeps the edit emits the same broken artifact the recipe was asked to avoid, with a warning beside it. Applies the rule already used by UpgradeTransitiveDependencyVersion to AddDependency, ChangeDependency, RemoveDependency and UpgradeDependencyVersion: return the original manifest with the warning, and keep the edit out of the shared live tree so a later recipe does not read an edit that was never applied. This reverses the contract closureAddFailsLoudWarnsAndRecordsDataTableRow pins, which asserted the edit is retained. That test is updated, and the reversal is deliberate and worth discussing: the failure is still loud, still warns on both files and still records a NodeLockRegenerationFailures row. What changes is that the artifact left behind installs.
An override keyed on the real package does not reach a slot that installs it under an alias: npm 11 leaves the aliased copy at its own resolution and applies the override only to the non-aliased instances. Refusing that case was over-strict, and it would have failed regeneration for any project that aliases a package it also overrides globally. An override keyed on the alias name itself is still refused, because selectAlias resolves the real package on its own and never reaches select, so that one would be skipped without trace. Keying an override on a direct dependency's alias is rejected by npm itself with EOVERRIDE when it disagrees with the declared spec, which is the case that made the right matching rule hard to guess before measuring it.
Reverts the do-no-harm change in all five Node recipes, restoring the original contract: the edit is returned with a warning and a NodeLockRegenerationFailures row. The change was argued on "the artifact does not install", which measurement does not support. npm ci rejects the pair, but npm install succeeds and reconciles it, producing exactly the resolution the recipe wanted. A kept edit is therefore one ordinary command from correct, and carries more information for a reader than no diff at all. Leaves the recipes byte-identical to their previous state, so this PR is only about overrides in the resolver. Keeps the new UpgradeDependencyVersion failure test, which covers a path that had none.
Three forms reached select with a key matching no package, so the closure
resolved as if the override were absent and the engine reported success over an
unchanged lock: {"tslib@^2": "1.0.0"}, {"tslib": {".": "1.0.0"}} and
{"lodash": {"tslib@^2": "1.0.0"}}. A fourth, a name declared both globally and
under a parent, resolved by key order. All four are the defect this engine exists
to avoid, in forms the denylist did not enumerate.
A key is now accepted only when it is a plain package name and a value only when
node-semver parses it as a range, so an unrecognised selector is refused by
construction rather than by having been listed. Declaring the same name twice is
refused rather than resolved by iteration order.
Removes the *, ".", $ and parsePath checks, which the name pattern subsumes.
npm rejects this outright: "EOVERRIDE - Override for is-number@^7.0.0 conflicts with direct dependency" (verified against npm 11). Resolving it here picked the override while the importer kept its declared range, so the diff found nothing to change and the run reported success over an unchanged lock. The check already existed for scoped overrides. It now runs for every override, which is what npm does, and requireScopeHolds becomes requireOverridesHold since it now answers two questions about the resolved graph rather than one.
Refusing on the mere presence of resolutions or pnpm.overrides stripped lock regeneration from every project carrying such a block, including runs of the sibling recipes that never touch overrides, and many real Yarn projects carry one. The builder now records which override names actually reached a package in the closure, so the check runs after resolution rather than at extraction. An override naming something nothing depends on stays the no-op it is; one that would move a resolution still refuses, so no untested lock is emitted. Paired tests: an out-of-closure override resolves, an applied one refuses.
The allowlist already rejects a name@version selector, which removes the misleading refusal that named the parent as its own other requirer. Test pins it. Jackson imports sorted ahead of the openrewrite ones, IOException and Json imported rather than named inline, and the stray blank lines collapsed.
setNestedOverride returned the document unchanged on any exception, which turns a failure into "nothing changed, no warning" -- the silent no-op this change set removes everywhere else. It now surfaces. The javadoc claimed the write does not reformat. Measured: a global override is appended in place and the block keeps its layout, but the nested dependencyPath write re-renders the overrides value, so an existing one-line block comes back expanded. Everything outside overrides is preserved either way. The javadoc now says which path does what, and a test pins the global case.
declaredOverrides took a map the caller passed in only so it could be populated as a side effect, and the four non-npm callers passed an empty one they never read. It now returns both maps in a small holder.
UpgradeTransitiveDependencyVersion never stabilised under pnpm: RewriteTest reported "Expected recipe to complete in 1 cycle, but took 2 cycles", and raising the allowed cycles just moved the number. setPnpmOverridesEntry had no idempotence check. Both of its branches fell through to a Jackson re-serialise and a reparse, and reparseJson returns a fresh document while editAndRegenerate decides changed-ness by reference identity, so every cycle reported a change no matter what the content was. The text itself was stable, so preserving formatting alone would not have fixed this; the load-bearing part is returning the document unchanged when the entry already holds the requested value, which is what setFlatEntry already did for npm `overrides` and Yarn `resolutions`. The new PackageJsonHelper.setNestedEntry does both: it builds or updates pnpm.overrides in place, so a one-line override no longer reprints the whole manifest in Jackson's pretty-printer style, and it returns the document unchanged when there is nothing to do.
The per-dependency scope resolves an added dependency's own closure without reading overrides. Adding a dependency to a project that already declares one locked the new transitive at the registry version and reported success, so the lock disagreed with the manifest and npm ci would reject it. Measured: an add pulling in shared@^1.0.0 under an override of ^2.0.0 locked shared@1.5.0. That is a wrong lock rather than an unapplied override, and it is reachable from any of the sibling recipes, which makes it worse than the gaps this change set was already refusing. The check sits after the empty-diff deferral so the reason reported for an override-only edit is unchanged. Also: the scoped equivalence check now counts peer requirers, which are not in resolvedEdges and were invisible to it; the name pattern accepts ~, which npm's own charset allows; and the javadoc orphaned above OVERRIDE_NAME is gone.
pstreef
changed the base branch from
fix/transitive-override-lock-regeneration
to
main
September 24, 2026 10:43
A dependencyPath segment may carry a version (express@4.0.0>accepts). The writer already emitted name@version parent keys, but the allowlist matched the whole key and refused every one of them, so the capability was unreachable. Match a parent key on its name part only, and check the selected version against the resolved parent in requireOverridesHold. A leaf key still may not carry a range: that selects which copies to override, and the engine cannot apply an override to only some of them.
pstreef
marked this pull request as ready for review
September 24, 2026 11:37
Comment-only. Drops a claim in select() that every requirement funnels through it, which the two refusals directly below it contradict, and three comments restating their own signatures. What is left is the ordering hazard, npm behaviour that is not derivable from any code here, the scoped-override equivalence argument, and the reason each refusal exists.
The per-dependency scope does not apply overrides, so it was handed any manifest that declared one. That refused edits the override had nothing to do with: on pnpm, Yarn and Bun the whole-closure scope cannot apply an override either, so adding an unrelated dependency to a project carrying a resolutions block failed where it had produced a correct patch before. Route on whether a resolved edit reaches an overridden package instead, and read the keys leniently so a selector this engine refuses to apply still does not sink an unrelated edit. A key that bounds no name at all could be selecting anything, so nothing can be ruled out against it.
The top-level case was covered; a key bounding no name one level in was not. Dropping the recursive call's result leaves this red and the top-level test green, which is the gap it closes.
Jackson deprecated fields() in favour of properties(); iteration order is unchanged, both walking the node's backing map. Scoped to the two call sites this branch adds. The fourteen elsewhere in the module are older and left for a cleanup of their own, as is the fieldNames() call further down this file.
…de-closure-resolution # Conflicts: # rewrite-javascript/src/main/java/org/openrewrite/javascript/internal/lock/NativeLockEngine.java # rewrite-javascript/src/main/java/org/openrewrite/javascript/internal/lock/NpmGraphBuilder.java
Over half of them restated a signature, a throw message, or something the pull request describes. What is left is what the code cannot say: an ordering that breaks silently if moved, npm behaviour not derivable here, two regexes, and the argument for applying a scoped override globally.
Each branch of the scoped-override proof is a property of the graph alone, so build one directly. Three end-to-end refusals go: they needed a registry route and a lock fixture each, and the two direct-dependency branches both asserted "direct dependency", so neither identified which fired. A requirer that only peer-depends on the child now has coverage; deleting the getPeerDependencies() half of that condition reddens exactly one test. One end-to-end refusal stays, since a unit test cannot catch nobody calling the method.
- A `$name` override value resolves to the root's own spec for that
direct dependency, as npm documents; a bump of a dependency overridden
as `"$name"` regenerates instead of refusing. A reference to a package
the root does not declare still refuses.
- A version selector on a parent key is a range, so `lodash@^4` holds
for lodash 4.17.20.
- A nested override keeps a pin already standing on its path or leaf by
moving it under ".", and an entry already pinned under "." is left
alone so the recipe stays single-cycle.
- An existing empty `"pnpm": {}` gains `overrides` as valid JSON.
- Remove the reparse helpers that no longer have callers.
knutwannheden
force-pushed
the
fix/transitive-override-closure-resolution
branch
from
September 28, 2026 12:57
f862449 to
3712153
Compare
Contributor
|
I pushed 3712153 on top of this branch with fixes for the clear-cut findings: A few design questions I left for you:
|
Contributor
Author
|
Thanks for the fixes. On the questions:
|
Comparing the lock's recorded overrides with the edited manifest refused every regeneration whenever the two were derived differently: a workspace member edited against the root's lock, a catalog-resolved value, a Bun key recorded in its normalised form. Refuse instead when the edit changes an override field or a spec a `$name` refers to, since no patcher writes that section; a section already out of date before the edit is left to the install. The names the lock records still bound the edit, so a member edit that reaches a root override, or one pnpm reads from pnpm-workspace.yaml, is refused rather than patched. That section is read once per regeneration, and only that section of the lock is parsed. Also: - Bun takes `overrides` whenever the field is present, even empty. - A selector that bounds no name refuses before the closure is resolved.
Contributor
|
I pushed b1a629d on top of your five commits. The recorded-overrides check from 7cb1158 refused regenerations it shouldn't: it compared the lock's The same comparison also refuses whenever pnpm or Bun derive the section differently from the manifest's raw values: a What the commit does instead
Tests. New:
pnpm 11: a follow-up PR. pnpm 11 moved overrides out of
The pnpm goldens in this module say they were recorded with pnpm 11.2.2. On pnpm 11:
The follow-up would capture Two more things worth knowing
|
Contributor
Author
|
Thanks, b1a629d makes sense. The lock's section can't be compared value by value once pnpm and Bun derive it differently from the manifest, and whether the edit changes it is the only thing that matters while no patcher writes it. Taking it as is. The pnpm 11 support, the alias slots and parsing the lock once are now listed as follow-ups in the description. I also updated the Scope section for the new refusal and the Bun fallback. |
knutwannheden
approved these changes
Sep 29, 2026
…failures
- `{"express": {".": "4.18.2", "accepts": ...}}`, the shape the nested
writer produces to keep an existing pin, is read as a global override of
express beside the nested one instead of refused.
- The direct-dependency conflict check and `$name` both use npm's one root
edge per name (dev over optional over prod over peer), so a library's
dev range next to a wider peer range is no longer a conflict.
- The nested override write prints the manifest without markers, so an
earlier recipe's warning cannot break it, and a failed write throws
rather than returning the manifest unchanged.
- A one-line `overrides: {...}` section in pnpm-lock.yaml is read.
- The per-dependency override check runs before Yarn Berry's checksum
downloads.
Contributor
|
One more round: I pushed 7d154a3 after another review pass over the whole PR.
Tests: Worth a follow-up, not blocking:
From my side this is ready to merge. |
knutwannheden
approved these changes
Sep 30, 2026
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.
Problem
UpgradeTransitiveDependencyVersionwrote the override intopackage.json, left the lock alone,and reported success.
npm cirejects that:An override changes no declared dependency, so regeneration falls through to whole-closure
resolution, which seeds from
dependencies/devDependencies/optionalDependenciesand never readsoverrides. It rebuilt the same graph and called that success.Solution
Apply the declared overrides in
NpmGraphBuilder.select, before the dedupe. AdependencyPathrunwrites
{"parent": {"child": range}}; that is applied to the whole closure andrequireOverridesHoldthen proves the parent was the child's only requirer, refusing when it wasn't.
Selectors are accepted by allowlist, a plain package name and a node-semver range. A denylist came
first and missed four forms, each reporting success over an unchanged lock.
Three paths resolve without reaching
select:npm:aliases, auto-installed peers, and theper-dependency patch scope. The first two refuse. The third locked a newly pulled-in transitive at
its registry version, so an edit reaching an overridden package now goes to the closure scope. That
routing reads keys leniently, so a selector this engine will not apply cannot sink an unrelated edit.
No recipe changed. Every refusal is an
EngineFailure, so the recipe keeps the edit, warns andrecords a
NodeLockRegenerationFailuresrow as it does today. The engine throwsEngineFailure383times already; this adds nineteen, each replacing a case that reported success over a lock
contradicting its manifest.
Scope
Only npm applies overrides. On pnpm, Yarn and Bun an override is refused only when it actually reaches
the closure, so a project merely carrying a
resolutionsblock keeps its lock regeneration. A selectorthe engine cannot read, such as a glob or a path, is bounded to the package names it could select and
refused only when one of those is in the closure. One that bounds no name, such as
*, refuses.The engine reads overrides from the same fields the manager does: pnpm merges a top-level
resolutionsunder
pnpm.overrides, and Bun falls back toresolutionswhenoverridesis absent.pnpm and Bun also record the overrides in the lock, and neither patcher writes that section yet. On those
two managers an edit that changes the overrides, or a spec a
$nameoverride refers to, is thereforerefused, even when the override moves nothing: adding or changing an override keeps warning rather than
regenerating. A section already stale before the edit is left to the install. The names that section
records also count as overridden, so an edit reaching an override the engine does not read from the
edited manifest (a workspace root's, or one from
pnpm-workspace.yaml) is refused rather than patched.Two pre-existing guards an override can now reach: workspaces defer before resolution, and one that
orphans a locked package hits the unreachable-entry check and refuses, lock untouched.
Verified end to end through the CLI against the real registry.
Follow-ups:
overridessection ofpnpm-lock.yamlandbun.lock, so pnpm and Bun can regenerateafter an override edit.
pnpmfield ofpackage.json, so the recipe'spnpm.overridesedit doesnothing there (predates this PR). Capture
pnpm-workspace.yamlin the scanning recipe, readpackage.jsonoverrides only for pnpm 10 and older, and write the recipe's override to
pnpm-workspace.yamlon 11+.npm:alias slots once non-npm alias entries are patched. Unreachabletoday, since every such lock diff refuses alias entries.