fix(remediation): exclude pre-release fix candidates - #1096
Conversation
sonukapoor
left a comment
There was a problem hiding this comment.
This is good, and it fixes more than the issue asked for. One test to add and then it goes in.
I had downgraded #1082 on the reasoning that #1084's comparator fix made this unreproducible. That was wrong, and your PR is what made me check properly. It holds only for a pre-release of the same release; a pre-release of a higher release still satisfies >= hint. On current main:
published ["1.2.2", "1.2.4-beta.1"], hint 1.2.3 -> 1.2.4-beta.1
which reaches the user as npm install <pkg>@1.2.4-beta.1 presented as a verified high-severity fix. So this closes a live defect, not a latent one. I have corrected my comment on the issue.
Two things I liked. Handling a pre-release hint by resolving upward to the stable release is the right call and semver-sound, rather than just returning null. And the note rewording is a genuine accuracy fix, not decoration: the old string claimed the hint "is not published on npm", which was false. It was published, it was just ineligible.
The one ask. The variant that still reproduces on main is the one variant without a test. Both new tests use a pre-release hint; neither covers a stable hint with a higher pre-release published. Revert your filter today and nothing catches it:
it("does not recommend a higher pre-release when the stable hint is unpublished", async () => {
mockPackument(["1.2.2", "1.2.4-beta.1"]);
const result = await resolvePublishedFixVersion("tar", "1.2.3");
expect(result.resolvedVersion).toBeNull();
});Two things I am deliberately NOT asking you to fix here, both filed separately so this stays scoped. The "no published version >= hint was found" note is now inaccurate when candidates existed but were all filtered out, and it needs a third branch rather than a reword because that exact sentence is asserted in tests/output.test.ts and printed in the docs. And getRecommendedAction can now say "Upgrade to 1.2.3-rc.1+" while the fix plan correctly says there is nothing to run.
One small thing for next time: branch naming here is feature/issue-NNN-... or bugfix/issue-NNN-.... Nothing leaks into history on a squash merge, so no action needed.
44ac58a to
8337a3a
Compare
|
Added the requested stable-hint regression in 8337a3a. With published versions Rebased onto current main as CONTRIBUTING.md requests. Validation passed: 8 focused tests; 137 suites / 1,759 tests overall; dependency install, test hygiene, build, advisory sync, and diff checks. The PR description now covers the stable-hint case too. The two separate output follow-ups are untouched, and I noted the branch convention for future contributions. |
|
Addressed the requested review item in commit
The PR is ready for re-review. |
sonukapoor
left a comment
There was a problem hiding this comment.
The test is in and it does the job. I reverted your filter and all three tests die, including the new stable-hint one, so the case that was still live on main is now pinned.
Verified on top of current main: 1773 tests pass, build clean, and the case resolves:
published ["1.2.2", "1.2.4-beta.1"], hint 1.2.3
main: 1.2.4-beta.1 <- emitted as a verified high-severity fix
yours: null <- correctly skipped
Thanks for pushing on this one. Your PR is also what made me recheck my own severity call on #1082, which was wrong: I had downgraded it on the assumption that #1084 made this unreproducible, and it only covered half the cases. The issue is corrected.
The two follow-ups I mentioned are filed separately, so nothing outstanding here.
|
Merged, thank you @rksharma-owg. |
What changed and why
resolvePublishedFixVersioncan recommend a pre-release as a verified remediation target even when the advisory hint is a stable version: with published versions1.2.2and1.2.4-beta.1, a hint of1.2.3selects the beta. An exact published pre-release hint is also returned unchanged.Filter out pre-release candidates before exact-match and nearest-version selection, using the existing
isPreReleaseVersionpolicy. The resolver now selects the next published non-pre-release version or returns no target when none qualifies. Notes for pre-release hints explain their ineligibility instead of incorrectly claiming they are unpublished.Closes #1082
Regression coverage
Removing the filter makes the requested stable-hint regression fail with
1.2.4-beta.1instead ofnull.Validation
On current main
936cf81plus this branch, using macOS arm64, Node.js 22.23.1 and npm 10.9.8:npm ci,npm run lint:tests, andnpm run buildpassed.node dist/index.js advisories synccompleted with 228,081 records.npm test -- --runInBand tests/lowest-safe-version.test.ts: 8 tests passed.npm test -- --runInBand: 137 suites, 1,759 tests passed.git diff --checkpassed.Compatibility
Public types and existing eligible release targets are unchanged. Advisory hints and registry version keys remain external inputs; this change only tightens which versions can be recommended. The separate output-note and recommended-action follow-ups identified in review remain outside this PR.