Conversation
There was a problem hiding this comment.
Copilot review overview
馃煛 Changes recommended
The npm SemVer logic can still be bypassed by the earlier Version guard.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Updates npm cooldown version handling to use SemVer precedence and correct prerelease detection.
Changes:
- Adds
Vulns::Semver.prerelease?. - Applies SemVer comparisons to npm cooldown candidates.
- Adds SemVer and npm cooldown regression tests.
A moderate issue remains: the earlier Version guard can bypass the npm-specific SemVer logic.
| File | Summary |
|---|---|
Library/鈥婬omebrew/鈥媣ulns/鈥媠emver.rb |
Adds SemVer prerelease detection. |
Library/鈥婬omebrew/鈥媡est/鈥媣ulns/鈥媠emver_spec.rb |
Tests prerelease and build metadata handling. |
Library/鈥婬omebrew/鈥媡est/鈥媎ev-cmd/鈥媌ump_spec.rb |
Tests npm prerelease cooldown ordering. |
Library/鈥婬omebrew/鈥媎ev-cmd/鈥媌ump.rb |
Updates npm cooldown filtering to use SemVer. |
馃挕 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| current_str = current.to_s | ||
| current_is_prerelease = current_str.include?("-") | ||
| latest_str = latest.to_s |
| # @param right the version to compare against | ||
| sig { params(left: String, right: String).returns(Integer) } | ||
| def semver_compare(left, right) | ||
| Vulns::Semver.compare(left, right) || (Version.new(left) <=> Version.new(right)) || 0 |
There was a problem hiding this comment.
Let's not use Vulns for this; Homebrew's Version class is better here I think but CC @p-linnane @andrew for thoughts
There was a problem hiding this comment.
The bug reproduces against Version on main:
1.2.3-next.1 <=> 1.2.3 = 1
1.2.4-next.2 <=> 1.2.4 = 1
1.2.3+build-1 <=> 1.2.3 = 1
1.2.3-alpha.1 <=> 1.2.3 = -1
Version orders the four prerelease keywords it has tokens for (alpha, beta, pre, rc) correctly and ranks every other npm dist-tag identifier such as next or canary, plus all build metadata, above the release. Adding semver precedence to Version itself would reorder every formula, so the npm path needs a separate comparator.
@MikeMcQuaid on the namespace: agreed that bump should take its comparator from core rather than from Vulns. vulns/repology.rb requires utils/repology.rb, so the dependency direction through vulns/ is vulns to core, and this would be the first dependency going the other way. Instead of reverting to Version, move Library/Homebrew/vulns/semver.rb to Library/Homebrew/semver.rb as Homebrew::Semver, update the call sites in vulns/vulnerability.rb and move the spec across. Behaviour stays the same and bump depends only on core.
Two places either side of the npm loop still compare with Version:
return if latest <= currentinversion_with_cooldown. Withcurrentat1.2.3-next.1and npmlatestat1.2.3this returns before the loop runs and the caller falls back tolatest, bypassing the cooldown this PR fixes. The guard requires the semver comparison for theNpmstrategy, so it moves below thecase strategydispatch or compares per strategy.cooldown_skipped = (latest if cooldown_version && cooldown_version < latest)inlivecheck_result. In the scenario the new test covers (current1.2.3-next.1,latest1.2.4, returning1.2.4-next.2)Versionranks1.2.4-next.2above1.2.4, socooldown_skippedisnilandbrew bumpprints1.2.4-next.2as the candidate while omitting the cooldown hold on1.2.4. The previous code skipped1.2.4-next.2and returnedcurrent, so the report was correct. Either that comparison takes the same comparator or the npm branch returns the skipped version alongside the chosen one, with a test entering throughretrieve_versions_by_arch.
One smaller point is that semver_compare ends || 0, so when both the semver parse and Version#<=> fail the two versions compare equal, both next guards fall through and the unparseable version is returned as a bump candidate. The previous code skipped it, so returning nil and skipping the candidate restores that.
Agreed, makes sense to me. |

brew bumpskips npm releases published within the last day. Its cooldown check usesVersion, which incorrectly ranks1.2.3-next.1above1.2.3. This can leave no eligible version;brew bumpthen falls back tolatestand silently bypasses the cooldown. The npm code also mistakes build metadata such as1.2.3+build-1for a prerelease because it checks for a hyphen.Use
Vulns::Semverfor npm version precedence and prerelease detection. Invalid semver versions still fall back toVersion; PyPI and RubyGems behavior is unchanged.brew benchmarkresults.brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?AI (Claude Code, Opus 5) assisted in tracking down the cause of the
TODOand in writing the change and the tests. I reviewed the change and ranbrew lgtmlocally.