[prx-my]

What changed and why

Follow-up to #1257, which fixed positionAgainstRange throwing above Number.MAX_SAFE_INTEGER as a side effect of tidying up OA008 range handling, and pinned it with a regression test.

This PR adds the two pieces that were not already covered:

• A comment on the existing guard in positionAgainstRange explaining the asymmetry that makes it necessary: satisfies guards the SemVer constructor inside Range.test(), while ltr/gtr route through outside() which does not. Without that note the guard reads as removable.
• A boundary test for positionAgainstRange("9007199254740991.0.0", "*"), which returns "satisfies". The wildcard takes a different path than the existing ^1.0.0 case, so it was uncovered.

No src behaviour changes.

Related

#1265, already closed by #1257

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[prx-my]

Update: this branch has been refreshed with the latest main. It is no longer behind and now only awaits the required review/status checks before it can merge.

[sonukapoor]

Your analysis was right, and then the ground moved under you. Worth spelling out because the diff no longer matches the title.

When you opened this on the 2nd it genuinely threw. I checked out the commit before #1257 and ran it: positionAgainstRange("9007199254740992.0.0", "*") gives Invalid major version, and so does ^1.2.0, while the boundary at 9007199254740991.0.0 returns satisfies. Exactly the boundary you described, and your root cause is right too: satisfies routes through Range.test() which swallows the constructor error, ltr and gtr go through outside() which does not.

#1257 then landed on the 5th, tidying up OA008 range handling, and fixed it as a side effect. It also shipped a regression test for it, returns null instead of throwing above semver's safe integer limit, in the file you are adding to, a few lines above your additions. So on current main nothing throws and the behaviour is already pinned.

That leaves three things in your PR and they are not equal.

The comment is worth keeping. It records the asymmetry between satisfies and ltr/gtr, which is the thing that would let someone "simplify" the guard away later. The existing test pins the behaviour; nothing explains the mechanism.

positionAgainstRange("9007199254740991.0.0", "*") is worth keeping too, and it is the one assertion here that is genuinely new. At the boundary * returns satisfies while the existing test's ^1.0.0 returns above, so the wildcard takes a different path and nothing currently covers it.

The other two, oversized against * and against ^1.2.0, both return null, which the existing test already asserts with ^1.0.0. Three ranges proving the same branch is the kind of thing I would normally ask you to trim.

So: keep the comment, keep the boundary-with-* assertion, drop the two oversized duplicates, and retitle from fix(version) to test(version) since nothing in src changes behaviour any more. Then it is a clean addition and I will take it.

One thing on us rather than you: #1265 is still open even though #1257 fixed and pinned it on the 5th, which is why this looked like live work when you picked it up. I am closing it with a note pointing at #1257.

[prx-my]

Done. Trimmed to what you asked for.

• Kept the comment, and expanded it so it records the asymmetry rather than just the symptom: satisfies guards the constructor inside Range.test(), while ltr/gtr route through outside() which does not, so only those two throw and only above MAX_SAFE_INTEGER. It now says outright that the guard is load-bearing, which is the bit someone would delete while "simplifying".
• Kept the boundary test. positionAgainstRange("9007199254740991.0.0", "*") returns "satisfies". You were right that * takes a different path than the existing ^1.0.0 assertion.
• Dropped the two oversized duplicates (* and ^1.2.0), since the existing ^1.0.0 case already proves that branch.
• Retitled to test(version).

Branch is on current main (the rebase was a no-op, it was already there) and green locally: version-extensions 50/50, lint:tests, and build.

Thanks for catching that #1257 had already fixed and pinned this, and for the breakdown of which parts were still worth keeping. That made the trim straightforward. The note about #1265 having been left open is appreciated too.

[sonukapoor]

All four, thank you. The comment is the part I wanted most, and I checked the src change is comment-only the hard way rather than by reading it: compiling both sides, the emitted version.js differs by exactly your four comment lines and nothing else.

The boundary assertion earns its place too. Replacing the try/catch with an off-by-one bounds guard, which is the simplification your comment exists to prevent, fails both the existing test and yours, and yours is the one that covers satisfies returning at the boundary rather than gtr classifying it as above.

Sorry this took a second round over something #1257 had already fixed underneath you. Approved.

[sonukapoor]

This and #1285 are both approved and sitting in the merge queue. The only thing holding them is that self-scan and self-scan-action are red on each, and that is not anything in your code.

Both branch from 300c5aa, which predates #1307 taking handlebars to 4.7.10 on main. The self-scan therefore reads a lockfile still pinning 4.7.9 and trips on the one critical that fix cleared: Failing: 1 vulnerability finding at or above high (--fail-on high), handlebars 4.7.9 arriving transitively through ts-jest. main is green on the same check.

A rebase onto current main picks up the fixed lockfile and should take both green with no other change. ts-jest already allows 4.7.10 within its declared range, so there is nothing to resolve beyond the lockfile itself.

I merge these one at a time rather than in a batch, so they will land shortly once CI is green.

[sonukapoor]

Re-approving on the current head after you brought main in.

Re-verified against main at c5d2272: merges clean, build clean, full suite green at 168 suites and 2370 tests on the merge result, all six checks green on the head. Your own diff is unchanged from what I approved, so this is the same change against newer main.

Confirmed the two pieces are the ones #1257 left behind. The existing boundary test only exercises ^1.0.0, so the wildcard path really was uncovered, and the comment is worth having: the guard reads as dead code without the note that satisfies catches inside Range.test() while ltr and gtr do not.

[sonukapoor]

Merged - thank you @prx-my! The comment on the guard is the part I expect to pay off; it was exactly the kind of thing a later tidy-up removes without noticing.