fix: correctly assign targetVersion for parent-update targets

[RohithPariki]

What changed and why

The targetVersion for parent-update was being incorrectly populated with the child dependency's safe version (targetChildVersion). During map insertion, if the parent was also flagged as a direct finding, its correct targetVersion could be overwritten by this incorrect child version due to version comparison sorting, resulting in cross-wired suggested commands.

As per PR review feedback, this updates upsertTarget to key the targets map using a combination of package, kind, and child package name (for parent-update). This completely prevents parent-update targets from incorrectly merging with direct install targets for the same package, fixing #1007 at the source.

Additionally, this retains the change to assign currentVersion to targetVersion for parent-update targets, preventing the HTML output table from erroneously displaying the child's version in the Fixed column for the parent.

Closes #1007

[sonukapoor]

Thanks for this, and apologies for the slow response.

This overlaps with #1113, which @osfv opened two days earlier and which fixes the same issue from a different angle. Under the first-working-PR-wins rule that one has the claim, so I want to be straight that this PR will not merge as it stands.

But it is not wasted work, and I would rather explain why than just close it.

#1113 fixes the structure. The root cause is that a parent-update target and a direct finding on the same parent collide in a map keyed by package name, and it rekeys that map. What it does not fix is plan.targets, so the JSON, the terminal table and the HTML report still pair the parent with the child's version. You found that half, and you are the only one who touched the output surfaces and the docs that describe them.

So what I would like, if you are up for it: once #1113 lands, rebase this on top with just the targetVersion change and the docs. Both of you get credit for the part you actually fixed.

One thing to settle first, and it is my call rather than yours. For update-parent-within-range the fix is npm update <parent>, so the parent's version in package.json does not change. Setting targetVersion to currentVersion makes the table read "axios 1.2.0 to 1.2.0", which is not wrong so much as meaningless. I think the honest answer is null with a dash in the Fixed column, but I want to look at how that renders before asking you to build it. I will follow up here.

Also, body.md looks like it was committed by accident. Worth dropping when you rebase.

[sonukapoor]

Thanks for sticking with this one, and for cutting it down - the three-line version
is much easier to reason about than the earlier one.

You have definitely found the real bug. I reproduced the exact scenario from #1007
and the command does go from npm install axios@4.0.6 js-yaml@4.3.1 on main to
npm install axios@1.19.0 js-yaml@4.3.1 with your change, which is the "Expected"
in the issue verbatim. So the diagnosis is right.

I do not think we can take the fix in this shape though, and the reason is
something I had wrong myself until I went looking: NpmTransitiveRemediation.package
does not consistently mean the parent. The resolver builds it two different ways.

At npm-transitive-resolution.ts:93 and :173 it is the parent:

package: directParentName,
currentVersion: directParent.version,

but at :454 and :501, the deep-chain path, it is the child:

package: args.vulnerableName,
currentVersion: args.installedVersion,

The type at types.ts:147 has no comment either way, so nothing in the code tells
you which one you are holding.

That second shape is the common one in real projects, and on it your change turns a
useful target into a no-op. Scanning examples/wrong-parent:

main:      js-cookie  3.0.6 -> 3.0.7
this PR:   js-cookie  3.0.6 -> 3.0.6

Every row in the "parent updates within range" table reads X -> X after the
change. Same in the HTML report, which renders the same target through
html-reporter.ts:919. For the direct-parent shape your change is an improvement
(mocha 10.0.0 -> 10.0.0 is better than main's cross-wired mocha 10.0.0 -> 5.1.0),
but one assignment cannot be correct for both meanings.

Two more things I ran into while checking it.

The widened matcher on line 564 is load-bearing but has no test. If I revert just
that clause to the single condition from main, the full suite still passes
1849/1849 - yet the real CLI drops coveredFindingCount to zero and nulls
runnableFixCommand for every npm within-range finding, which also empties those
fields in the JSON, SARIF and SBOM output. A branch that would break the output
contract if deleted, and that no test notices, needs a test.

And matching on currentVersion is much less specific than matching on a child's
fix version, which bites in lockstep-versioned monorepos where everything ships at
one version. I built an nx-shaped case where @nx/devkit@21.2.23 is vulnerable via
@nx/webpack: main picks npm update @nx/webpack (correct), this PR picks
npm update @nx/devkit, which cannot fix it.

Where I would go instead: the actual cross-wiring happens in upsertTarget
(fix-commands.ts:677). axios enters the plan twice, once as a direct fix at
1.19.0 and once as an in-range parent at 4.0.6, and the version-max merge picks the
higher one. Keying that map by (package, kind), or skipping the version-max merge
when either side is parent-update, fixes #1007 at the source and leaves
targetVersion alone - so none of the three problems above arise. That feels like
the smaller and safer change to me.

If you would rather keep this approach, then the prerequisite is making
package/currentVersion mean one thing across all four construction sites and
documenting it on the type.

One more to flag: the test at tests/output.test.ts:624 is the #989 regression
test, and it asserts toContain("5.1.0"). It still passes, but only because 5.1.0
appears in the free-text reason rendered in the Context column - the column it was
written to guard now shows 10.0.0. Worth resolving deliberately rather than leaving
it passing by accident.

Also minor: line 564 is 183 characters where its neighbours are 40-115, and line
565 right below it already shows the house style for an (a || b) clause.

[sonukapoor]

targetVersion: finding.recommendedNpmTransitiveRemediation.currentVersion,

This is correct when package is the parent, but the resolver also builds this
object with the child in package (npm-transitive-resolution.ts:454 and :501),
and on that path this makes Target equal to the installed version. Scanning
examples/wrong-parent goes from js-cookie 3.0.6 -> 3.0.7 on main to
3.0.6 -> 3.0.6 here, and every row in that table does the same. Note the
pnpm/yarn/bun sibling at line 375 still uses targetChildVersion, so the two would
disagree about what targetVersion means for the same kind.

[sonukapoor]

The || item.targetVersion === ...currentVersion clause.

Two things. It has no coverage: reverting it to main's single condition leaves the
suite at 1849/1849 green, while the real CLI silently drops coveredFindingCount
to zero and nulls runnableFixCommand for every npm within-range finding, which
also empties those fields in JSON, SARIF and the SBOMs. And matching on
currentVersion is far less discriminating than on a child's fix version - in a
lockstep-versioned monorepo (nx, Angular, Babel all ship at one version) I get
npm update @nx/devkit where main correctly gives npm update @nx/webpack.

Also, at 183 characters this is well past the surrounding style; line 565 below
already shows how we wrap an (a || b) clause here.

[sonukapoor]

The assertion flipped from 5.1.0 to 10.0.0.

Mutating line 250 back to targetChildVersion fails exactly this one assertion, so
this is the only coverage the behaviour change has. Worth adding a case that pins
the intended semantics rather than the single value.

[sonukapoor]

Heads up on an overlap before you spend more time here.

The commit you pushed on the sixteenth, fix(remediation): stop parent-update targets merging into install targets, is the same change as #1113 by @osfv. I compared the diffs rather than going by the title: all 23 of that PR's added lines in fix-commands.ts appear verbatim in yours, down to the same new test file name.

#1113 was opened on 2026-09-09, a week before that commit, so it has the claim. We go oldest-first here, which is what I told @osfv on the fourteenth would happen. #1113 is approved and merging shortly.

Nothing wrong with what you wrote, and your own change in this PR, the targetVersion assignment for parent-update targets, is separate and still wanted. What would help is dropping that last commit and rebasing on current main, which already carries the keyspace split. Then this PR is just your fix again and I can review it on its own terms.

Shout if the rebase throws anything odd and I will take a look.

[sonukapoor]

Closing this, and the reason is not your work.

You pushed the keyspace fix on the 16th, within an hour of the review asking for it. #1113 landed the same approach on the 17th and closed #1007 with it, which left this PR fixing an issue that had already closed underneath it. Nobody told you for five days. That part is on us, not you.

What is left in your diff once #1113 is accounted for is a single line:

-  targetVersion: ...recommendedNpmTransitiveRemediation.targetChildVersion,
+  targetVersion: ...recommendedNpmTransitiveRemediation.currentVersion,

That is aimed at a real and still-open problem. A parent-update target stores the parent in package but the child's version in targetVersion, so every renderer prints a version belonging to a different package. #1152 tracks exactly that.

One thing worth knowing before you pick it up, because it is what the earlier review was circling: NpmTransitiveRemediation.package does not consistently mean the parent. npm-transitive-resolution.ts:93 and :173 put the parent in it; :454 and :501, the deep-chain path and the more common one, put the child. So currentVersion is sometimes the child's own current version, and swapping to it turns js-cookie 3.0.6 -> 3.0.7 into 3.0.6 -> 3.0.6.

Fixing #1152 properly most likely means resolving that dual meaning first. It is undocumented on the type at types.ts:147, which is why it reads as correct to everyone who looks at it, including me on a first pass.

#1152 is open and unclaimed. First working pull request is the one that lands, so there is nothing to wait for.

[sonukapoor]

Correction, and it is mine. I pointed you at #1152 without checking whether anything was already open against it. #1174 has been open since the 21st and closes it.

So that was a bad pointer an hour after closing your PR, which is twice in one morning, and both times because I checked the issue and not the pull requests. Sorry.

I am not going to name a third thing right now, because doing it blind is exactly what caused the first two. If you want something to pick up, say so and I will look properly first and come back with something genuinely unclaimed.