fix: support npm semver ranges in devEngines#9061
Conversation
Greptile SummaryThis PR adds support for npm-compatible semver ranges (e.g. Confidence Score: 5/5Safe to merge — core logic is correct, well-scoped to package.json sources, and backed by solid unit and e2e tests; remaining findings are minor style/coverage gaps. Both P2 findings are non-blocking: the tilde-range test gap is a coverage nicety and the prefer_offline concern only manifests in an edge case (range not installed + prefer_offline mode) that doesn't produce wrong data. No P0/P1 issues found. src/semver.rs — minor unit-test coverage gap for tilde ranges; src/toolset/tool_version.rs — prefer_offline guard inconsistency in the npm range remote-fetch block. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[resolve_version called\nv = '>=22.0.0 <23.0.0'] --> B{v == 'latest'?}
B -- No --> C[list_installed_versions_matching v\nfuzzy_match_filter → empty for range]
C --> D{source is package.json\n&& is_npm_semver_range_query?}
D -- No --> G[normal fuzzy resolution path]
D -- Yes --> E{latest_versions disabled?}
E -- Yes --> F[list_installed_versions\nnpm_semver_range_filter]
F --> H{match found?}
H -- Yes --> I[return latest match\nmatches.last]
H -- No --> J{offline?}
E -- No --> J
J -- No --> K[list_remote_versions\nnpm_semver_range_filter]
K --> L{match found?}
L -- Yes --> I
L -- No --> G
J -- Yes --> M[return range string as-is]
G --> N[list_versions_matching_with_opts\nfuzzy empty → resolve_prefix]
Reviews (4): Last reviewed commit: "refactor: inline package json range sour..." | Re-trigger Greptile |
| if query.split_whitespace().count() > 1 { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
split_whitespace().count() > 1 may intercept unusual non-range inputs
This catch-all check fires for any multi-word string (e.g. "long term", "lts carbon"), routing it through Range::parse. If parsing fails the function returns None and the regex fallback runs normally — so correctness is preserved — but the heuristic silently swallows those inputs rather than letting them reach fuzzy_match_filter directly. Narrowing the check to strings that contain at least one semver operator character alongside the space would make the intent clearer and reduce ambiguity for non-standard version aliases.
// Suggested: replace the broad whitespace check with a more targeted guard
if query.split_whitespace().count() > 1
&& query.bytes().any(|b| matches!(b, b'<' | b'>' | b'=' | b'^' | b'~'))
{
return true;
}|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Code Review
This pull request introduces full npm-compatible semver range support by integrating the nodejs-semver crate. It replaces the previous heuristic-based simplify_semver logic with a robust npm_semver_range_filter used during version resolution. Feedback suggests optimizing the version parsing in src/semver.rs by stripping 'v' prefixes once to avoid redundant parsing operations.
| let version = v.as_str(); | ||
| NodeVersion::parse(version) | ||
| .or_else(|_| { | ||
| NodeVersion::parse(version.trim_start_matches(['v', 'V'])) | ||
| }) | ||
| .is_ok_and(|version| range.satisfies(&version)) |
There was a problem hiding this comment.
The current implementation performs a double parse for any version string starting with 'v' or 'V'. Since these prefixes are extremely common in tool versions, it's more efficient to strip the prefix once and perform a single parse. This optimization is safe as semver parsing logic remains consistent whether the prefix is stripped beforehand or as a fallback.
| let version = v.as_str(); | |
| NodeVersion::parse(version) | |
| .or_else(|_| { | |
| NodeVersion::parse(version.trim_start_matches(['v', 'V'])) | |
| }) | |
| .is_ok_and(|version| range.satisfies(&version)) | |
| let version = v.trim_start_matches(|c| c == 'v' || c == 'V'); | |
| NodeVersion::parse(version).is_ok_and(|version| range.satisfies(&version)) |
Refs jdx#8935 ## Summary - preserve package.json devEngines semver ranges instead of simplifying them to exact-looking versions - resolve npm-compatible ranges with nodejs-semver when filtering backend versions - cover lower-bound, compound, caret, wildcard, and package manager range behavior ## Tests - cargo test config::config_file::idiomatic_version::package_json - cargo test semver - ./e2e/run_test e2e/core/test_package_json --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
### 🐛 Bug Fixes - **(docs)** typo in Go Backend by @dolmen in [#9065](#9065) - **(task)** render dependency templates even when no args are passed by @MatthiasGrandl in [#9062](#9062) - support npm semver ranges in devEngines by @risu729 in [#9061](#9061) ### New Contributors - @dolmen made their first contribution in [#9065](#9065) - @MatthiasGrandl made their first contribution in [#9062](#9062)
Refs #8935
Summary
Tests