fix(remediation): stop parent-update targets merging into install targets

[osfv]

What changed and why

suggestedFixCommands could emit a version that does not exist for the package it names, e.g. npm install axios@4.0.6 where 4.0.6 is form-data's version.

The reported diagnosis (index-aligned lookup) pointed at the right area but the mechanism is slightly different. In buildSuggestedFixCommandPlan the accumulator is keyed by package name only. A transitive finding whose remediation is a within-range refresh produces a parent-update target with package = the parent (axios) and targetVersion = the child's version (4.0.6), because that target means "refresh form-data within axios's range", not "install axios@4.0.6". When a direct finding for axios also exists, the two land on the same key, and upsertTarget's "higher version wins" merge compares versions of two different packages, so form-data's 4.0.6 replaces axios's 1.19.0 and the merged target is emitted as a direct install.

Fix: key parent-update targets in their own keyspace, per parent and per child, so they only merge with a refresh of the same child through the same parent. Install targets (direct / parent-upgrade) keep merging per package exactly as before. Each finding keeps resolving to its own target through findFixTargetForFinding, and coverage counts are unchanged.

For the repro in the issue the plan now emits npm update axios && npm install axios@1.19.0 js-yaml@4.3.1. The refresh is kept rather than dropped because npm install axios@1.19.0 alone does not guarantee npm re-resolves an already-locked form-data@4.0.5 that still satisfies the range.

Tests: tests/remediation/fix-commands-parent-update-merge.test.ts covers the issue repro, two refreshes of different children through the same parent, and the unchanged merge of repeated install targets.

Verified against current main (#1084 / #1085 moved findFirstFixedVersion but do not touch this path).

Note: #1007 is assigned to @alamb-hex. The maintainer's check-in on Sept 6 had no reply, so I went ahead; happy to close this in favour of theirs if they are still working on it.

Closes #1007

[sonukapoor]

Thanks for this, and for going back to first principles on it rather than
taking the issue at its word.

Worth saying plainly: your diagnosis is right and mine was wrong. I endorsed the
index-alignment theory back in August and it does not hold up. The actual cause
is exactly what you describe, that an npm parent-update target stores the
parent in package but the child's version in targetVersion, so it collided
with a direct finding on the same parent and the higher-version-wins merge ended
up comparing versions of two different packages. Splitting the keyspace is the
right shape of fix.

I also checked the tests do real work: reverting targetKey back to
install:${target.package} fails two of the three, and the no-regression case
correctly keeps passing. That is the kind of test I want to see on a merge bug.

Where I do not think it is finished is that it fixes plan.command but leaves
plan.targets alone, and the targets are what the JSON, the terminal table and
the HTML report all render from. Running the reporter's own repro on this branch
still gives me this:

High severity parent updates within range
| Package | Current | Recommended target | Context                                        |
| axios   | 1.16.1  | 4.0.6              | axios@1.16.1 already allows form-data@4.0.6    |

and in the JSON:

{ "package": "axios", "currentVersion": "1.16.1", "targetVersion": "4.0.6", "kind": "parent-update" }

and in the HTML report, isBreakingUpgrade compares axios 1.16.1 against
form-data's 4.0.6, sees the major jump and stamps it with a breaking badge. That
is the second half of what @alamb-hex reported, that the output implies axios
needs a three-major upgrade. If we close #1007 on this, his next scan reopens it.

The other thing I would want sorted before merge is that keying per child means
several findings behind one parent now produce several targets that all carry
package: "axios". On a scan touching one package I get "2 command groups ready
across 3 packages", two axios rows in the table where the first reads as a
downgrade, npm update axios spawned twice by --fix, and two
cve.fix.applied audit records with versions that belong to other packages.

Neither of those is a reason to abandon the approach. The keyspace split is
correct. It just needs to stop putting a foreign package's version in a field
that every renderer treats as this package's version, and it needs to not emit
one target per child when the command is the same. Left some specific notes
inline.

One process note so nothing is unfair here: #1007 is assigned to @alamb-hex, but
we moved to first-working-PR-wins on 2026-09-07 precisely because assignment was
not reserving anything, so this PR stands on its own. He reported the bug and he
will get reporter credit in the release notes either way.

[sonukapoor]

The keyspace split is the right call and I agree with the diagnosis.

What it does not fix is that targetVersion on a parent-update target is still the CHILD's version while package is the parent, and every renderer treats those two fields as belonging together.

So on the reporter's own repro this branch still produces { "package": "axios", "targetVersion": "4.0.6" } in the JSON, an axios | 1.16.1 | 4.0.6 row under a column headed "Recommended target", and a breaking badge in the HTML report because isBreakingUpgrade compares axios 1.16.1 against form-data's 4.0.6 and sees a major jump. That is the second half of what @alamb-hex reported.

Smallest fix I can see is to carry the child separately, something like refreshedChild: { name, targetVersion }, and stop putting a foreign package's version in targetVersion. If that is too invasive, suppressing the version column and the breaking badge when kind === "parent-update" would at least stop the output being wrong.

Second thing on the same change: keying per child means N findings behind one parent now produce N targets that all carry package: "axios" and the same command. On a scan touching one package I get "2 command groups ready across 3 packages", two axios rows in the table where the first reads as a downgrade, npm update axios spawned twice by --fix, and two cve.fix.applied audit records carrying other packages' versions. Worth deduping on the emitted command before render and apply.

[sonukapoor]

These two lines together assert that targets labelled axios carry form-data's and follow-redirects' versions, which is the exact pairing #1007 was filed about.

As written this locks the bug in, so it should change alongside the fix rather than encoding the current behaviour.

[sonukapoor]

The comment on targetKey says targetVersion is the child's version rather than <package>'s, but that is not true on this branch. Here package is finding.pkg.name, so the target package IS the child and the old merge was sound.

The key degenerates to parent-update:form-data>form-data and splits it anyway, which changes the emitted command on pnpm, Yarn and Bun:

	before	after
pnpm	pnpm add form-data@4.0.6	pnpm update --recursive --no-save form-data && pnpm add form-data@4.0.6
yarn	yarn add form-data@4.0.6	yarn upgrade form-data && yarn add form-data@4.0.6
bun	bun add form-data@4.0.6	bun update form-data && bun add form-data@4.0.6

Might well be an improvement, but it is undisclosed and untested on three of the four package managers we support. Worth a test and a corrected comment.

[sonukapoor]

Checking in on both of these rather than letting them sit.

#1113 has not had a push since the ninth, and #1114 since the eleventh. No pressure at all, I just want to know whether you are still planning to pick them up, because both have people waiting behind them.

On #1113 specifically: someone else opened a PR fixing the half your change leaves open, the plan.targets cross-wiring. Yours came first and has the claim, so the plan is to land yours and have theirs rebase on top. Nothing needed from you on that beyond the change request already on the PR.

On #1114, the regex handling in stripComments is still the only thing blocking it. Worth knowing that someone independently wrote a second implementation of the same comment-stripping this weekend and hit exactly the same bug, so it is a genuinely easy trap rather than anything you missed.

If you would rather I took that part, say the word and I will, and you keep the PR.

[sonukapoor]

Picking this up in-house rather than leaving it with you, and I want to be straight about why, because it isn't that you were slow.

Scanning a real repo this morning I hit the live version of this: nx validated at 22.7.7 after 18 versions above current were scanned and 17 found still vulnerable, while the emitted command read pnpm add nx@22.5.1. So the tool was handing out a version it had itself just determined was still vulnerable. With a release due tomorrow I did not want that sitting in the headline feature.

That is the plan.targets half I flagged in the review. runnableFixCommand reads from targets through findSuggestedCommandForFinding, so fixing plan.command alone left the per-finding command, the JSON, and the HTML report still wrong. #1142 does that half.

Your diagnosis is the part that mattered and it stands. The index-alignment theory I endorsed back in August was wrong, and you found the actual cause: a parent-update target storing the parent in package while the version belongs to something else. I could not have gone looking in upsertTarget without that. You are credited for it in #1142 and you will be in the v1.35.0 release notes.

If you would still like to finish this one yourself, say so and I will close #1142 instead. Otherwise no action needed here, and I would genuinely like to see you take another.

[sonukapoor]

Approving and merging this.

First, a correction I owe you. When I picked up the plan.targets half as #1142 on the fifteenth I said it covered that half, and it does not. #1142 fixed an adjacent thing, a stale command surviving upsertTarget when targetVersion is raised. It never touched the keyspace collision, and main still has no targetKey. I checked this morning by running the plan for the #1007 repro against current main:

main today:      npm install axios@4.0.6
this branch:     npm update axios && npm install axios@1.19.0 js-yaml@4.3.1

So the bug you diagnosed is still live in v1.35.0, and yours is the change that fixes it. I was wrong twice on this one, first on the index-alignment theory and then on thinking I had already covered your half.

Rebased on current main it is 144 suites and 1888 tests green, build clean.

On the two things I asked for in the review: both still stand, and I am taking them out of your way rather than holding the fix hostage to them. The parent-update target still carries the child's version under the parent's name, so the table shows axios 1.16.1 to 4.0.6 with a breaking badge, and two children behind one parent still produce two axios rows and npm update axios twice. That is an output-contract change touching JSON, terminal, HTML and the SARIF metadata that landed last week, so it wants its own issue and its own review rather than being bolted onto a fix that is already correct. Filed as #1152.

Thanks for the diagnosis. The keyspace split is the part I could not have found without you.

[sonukapoor]

Merged - thank you @osfv!

The diagnosis was the hard part and it was yours. The keyspace split is in, and #1152 tracks the target-shape half so it gets its own review rather than riding along on a fix that was already correct.

Happy to see you take another whenever you have time.