Rust: Make crate fallback logic more conservative in path resolution library - #1
Conversation
…library Source PR: github#22495 Source head: df7d49d
⛔ Shipwright · BlockedRecommendation: do not merge PR #1 · Tier
Findings (14)
Fireworks usage: 13,727 input · 1,826 output · 15,553 total tokens · $0.0042 · 23s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| module; | ||
|
|
||
| bindingset[str] | ||
| private string leftPad(string str) { result = ("0000" + str).suffix(str.length()) } |
There was a problem hiding this comment.
Shipwright · CRITICAL
The shared leftPad changed from 3-digit to 4-digit padding.
Impact: The shared leftPad changed from 3-digit to 4-digit padding. This alters lexicographic ordering for version components with 4+ digits. For example, 1.2.1000 vs 1.2.999 now compares differently than the previous Go/Ruby implementations, producing incorrect maybeBefore/maybeAfter/maybeBetween and before/equal/after results.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| */ | ||
| bindingset[s] | ||
| string getMajor(string s) { result = s.regexpCapture("v?(\\d+).*", 1) } | ||
|
|
There was a problem hiding this comment.
Shipwright · CRITICAL
The new padSemVer regexes use '.*' after numeric components, so getMajor can match strings that are not valid SemVer, such as '123abc'.
Impact: The new padSemVer regexes use '.*' after numeric components, so getMajor can match strings that are not valid SemVer, such as '123abc'. The removed Go and JavaScript implementations required a full SemVer pattern. This loosening can cause non-SemVer strings to be normalized and compared, producing incorrect version ordering results.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| module; | ||
|
|
||
| bindingset[str] | ||
| private string leftPad(string str) { result = ("0000" + str).suffix(str.length()) } |
There was a problem hiding this comment.
Shipwright · HIGH
The new shared 'leftPad' uses '("0000" + str).suffix(str.length())', which pads to 4 digits, while the removed Go/Ruby implementations used '("000" + str).suffix(str.length())', pa
Impact: The new shared 'leftPad' uses '("0000" + str).suffix(str.length())', which pads to 4 digits, while the removed Go/Ruby implementations used '("000" + str).suffix(str.length())', padding to 3 digits. This changes normalized version ordering for versions with 4+ digit components. For example, '1.2.1000' and '1.2.999' now compare differently than before, potentially producing incorrect 'maybeBefore'/'maybeAfter'/'maybe…
Suggested fix: Fix the review finding before release.
| module; | ||
|
|
||
| bindingset[str] | ||
| private string leftPad(string str) { result = ("0000" + str).suffix(str.length()) } |
There was a problem hiding this comment.
Shipwright · HIGH
The new 'padSemVer' uses 'leftPad' with 4-digit padding, but the removed Ruby 'normalizeSemver' used 3-digit padding.
Impact: The new 'padSemVer' uses 'leftPad' with 4-digit padding, but the removed Ruby 'normalizeSemver' used 3-digit padding. This changes the normalized representation for Ruby Gemfile version comparisons, potentially altering 'before', 'equal', and 'after' results for versions with 4+ digit components.
Suggested fix: Fix the review finding before release.
| module; | ||
|
|
||
| bindingset[str] | ||
| private string leftPad(string str) { result = ("0000" + str).suffix(str.length()) } |
There was a problem hiding this comment.
Shipwright · HIGH
The shared SemVer utility silently changes behavior across Go, JavaScript, and Ruby by mixing previously different regex strictness and padding widths.
Impact: The shared SemVer utility silently changes behavior across Go, JavaScript, and Ruby by mixing previously different regex strictness and padding widths. There is no comment or test evidence documenting that 4-digit padding and optional 'v' are intentional, making it hard for a newcomer to understand whether the behavioral changes are deliberate or accidental.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| */ | ||
| bindingset[s] | ||
| string getMajor(string s) { result = s.regexpCapture("v?(\\d+).*", 1) } | ||
|
|
There was a problem hiding this comment.
Shipwright · HIGH
The new shared padSemVer accepts an optional leading 'v' in getMajor/getMinor/getPatch.
Impact: The new shared padSemVer accepts an optional leading 'v' in getMajor/getMinor/getPatch. The removed JavaScript and Ruby implementations did not accept a leading 'v'. JavaScript dependency versions such as 'v1.2.3' that previously did not normalize now normalize and participate in version comparisons, changing query results for JavaScript and Ruby dependencies.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| */ | ||
| bindingset[s] | ||
| string getMajor(string s) { result = s.regexpCapture("v?(\\d+).*", 1) } | ||
|
|
There was a problem hiding this comment.
Shipwright · MEDIUM
The new 'padSemVer' regexes in 'getMajor', 'getMinor', and 'getPatch' accept an optional leading 'v' ('v?(\d+)...').
Impact: The new 'padSemVer' regexes in 'getMajor', 'getMinor', and 'getPatch' accept an optional leading 'v' ('v?(\d+)...'). The removed Go 'normalizeSemver' also accepted 'v?', but the removed JavaScript 'normalizeSemver' did not. JavaScript dependency versions such as 'v1.2.3' that previously did not normalize now normalize and participate in version comparisons, changing query results for JavaScript dependencies.
Suggested fix: Fix the review finding before release.
| */ | ||
| bindingset[s] | ||
| string getMajor(string s) { result = s.regexpCapture("v?(\\d+).*", 1) } | ||
|
|
There was a problem hiding this comment.
Shipwright · MEDIUM
The new 'padSemVer' regexes use '.*' after the numeric component, so 'getMajor' can match a string that starts with digits but is not a valid SemVer (e.g., '123abc').
Impact: The new 'padSemVer' regexes use '.' after the numeric component, so 'getMajor' can match a string that starts with digits but is not a valid SemVer (e.g., '123abc'). The removed Go implementation required a full SemVer pattern 'v?(\d+)\.(\d+)\.(\d+)(\D.)?', and the removed JavaScript implementation required '(\d+)\.(\d+)\.(\d+)'. This loosening can cause non-SemVer strings to be normalized and compared,…
Suggested fix: Fix the review finding before release.
| */ | ||
| bindingset[s] | ||
| string getMajor(string s) { result = s.regexpCapture("v?(\\d+).*", 1) } | ||
|
|
There was a problem hiding this comment.
Shipwright · MEDIUM
The new 'padSemVer' regexes accept an optional leading 'v', but the removed Ruby 'getMajor', 'getMinor', and 'getPatch' did not.
Impact: The new 'padSemVer' regexes accept an optional leading 'v', but the removed Ruby 'getMajor', 'getMinor', and 'getPatch' did not. Ruby Gemfile version strings such as 'v1.2.3' that previously did not normalize now normalize and participate in version comparisons, changing query results for Ruby dependencies.
Suggested fix: Fix the review finding before release.
| */ | ||
| bindingset[s] | ||
| string getMajor(string s) { result = s.regexpCapture("v?(\\d+).*", 1) } | ||
|
|
There was a problem hiding this comment.
Shipwright · MEDIUM
The new 'padSemVer' regexes use '.*' after the numeric component, so 'getMajor' can match a string that starts with digits but is not a valid SemVer.
Impact: The new 'padSemVer' regexes use '.' after the numeric component, so 'getMajor' can match a string that starts with digits but is not a valid SemVer. The removed Go 'normalizeSemver' required a full SemVer pattern 'v?(\d+)\.(\d+)\.(\d+)(\D.)?'. This loosening can cause non-SemVer strings to be normalized and compared, producing incorrect version ordering results.
Suggested fix: Fix the review finding before release.
| */ | ||
| bindingset[s] | ||
| string getMajor(string s) { result = s.regexpCapture("v?(\\d+).*", 1) } | ||
|
|
There was a problem hiding this comment.
Shipwright · MEDIUM
The new 'padSemVer' regexes accept an optional leading 'v', but the removed JavaScript 'normalizeSemver' did not.
Impact: The new 'padSemVer' regexes accept an optional leading 'v', but the removed JavaScript 'normalizeSemver' did not. JavaScript dependency versions such as 'v1.2.3' that previously did not normalize now normalize and participate in version comparisons, changing query results for JavaScript dependencies.
Suggested fix: Fix the review finding before release.
| */ | ||
| bindingset[s] | ||
| string getMajor(string s) { result = s.regexpCapture("v?(\\d+).*", 1) } | ||
|
|
There was a problem hiding this comment.
Shipwright · MEDIUM
The new 'padSemVer' regexes accept an optional leading 'v', but the removed Go 'normalizeSemver' also accepted 'v?', so this is not a new defect for Go.
Impact: The new 'padSemVer' regexes accept an optional leading 'v', but the removed Go 'normalizeSemver' also accepted 'v?', so this is not a new defect for Go. However, the shared implementation now applies this looser matching to JavaScript and Ruby, which previously did not accept a leading 'v'. This can cause non-SemVer strings to be normalized and compared, producing incorrect version ordering results.
Suggested fix: Fix the review finding before release.
1 participant
When doing a QA run for github#21795, I noticed that our fallback logic for crate dependencies can sometimes lead to combinatorial explosions. This PR alleviates this by only applying the fallback logic to crates that have a unique latest version. DCA confirms that this PR resolves the performance issue, most notably on the
reflaxe-rustproject, where thehxrtcrate exists in many test copies (e.g. https://github.com/fullofcaffeine/reflaxe.rust/blob/main/test/snapshot/abstracts_conversions/intended/hxrt/Cargo.toml).Source merge-base:
b0fa3e770386f18fd4d63225301ea4c41e7a5721Source head:
df7d49d1e0aff059ee3e93bd72434ca7813a0c3b