fix add normalization for git describe compiler versions#46030
Conversation
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #46030 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (99 additions detected, threshold is 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Normalizes local git describe compiler versions for package manifest compatibility checks while preserving true prerelease semantics.
Changes:
- Added
git describesemantic-version normalization. - Applied normalization to manifest
min-versionchecks. - Added unit and package-resolution regression tests.
Show a summary per file
| File | Description |
|---|---|
pkg/semverutil/semverutil.go |
Adds version normalization helper. |
pkg/semverutil/semverutil_test.go |
Tests normalization behavior. |
pkg/cli/add_package_manifest.go |
Normalizes compiler versions before comparison. |
pkg/cli/add_package_manifest_test.go |
Tests manifest resolution with local versions. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Medium
There was a problem hiding this comment.
Clean fix. The regex correctly targets git-describe prerelease patterns, normalization is safely gated behind the existing IsValid check, and the test coverage is thorough. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 13.8 AIC · ⌖ 4.25 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics (2 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — this is a clean, well-reasoned fix with good regression coverage.
📋 Key Themes & Highlights
Key Themes
- Root cause properly addressed: The bug is fixed at the right level — normalizing before comparison rather than hacking the comparison itself.
- Edge case handling: The regex correctly distinguishes git-describe noise (
27-gabc1234[-dirty]) from real semver prereleases (beta.1), and thedirty-only case is handled separately. - One minor concern: The prerelease regex
^[0-9]+-g[0-9a-f]+(?:-dirty)?$could silently strip a manually created tag likev1.2.3-1-gdeadbeefthat happens to match the git-describe pattern. In practice this is very unlikely, but worth a brief comment in the code noting the tradeoff.
Positive Highlights
- ✅ Normalization applied after
IsValidso invalid inputs are already rejected before reachingNormalizeGitDescribeSemver - ✅ Unit tests cover all meaningful cases: clean tag, post-tag, dirty, post-tag+dirty, and real prerelease
- ✅ Integration test validates the full manifest resolution path, not just the helper in isolation
- ✅ Minimal call-site change (one line) with clear intent
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 22.5 AIC · ⌖ 5.28 AIC · ⊞ 6.7K
Comment /matt to run again
There was a problem hiding this comment.
Non-blocking observations
The core fix is correct: properly collapses git-describe prerelease suffixes before the semver comparison, and the regex correctly distinguishes real git-describe patterns from legitimate semver prereleases like beta.1.
Two minor issues flagged inline:
Findings summary
- Regex hash length (
pkg/semverutil/semverutil.go):g[0-9a-f]+matches single-char hashes; should be{7,}to reflect real git abbreviated SHA minimums. - Fragile test cleanup (
add_package_manifest_test.go):t.Cleanuprestores to hardcoded"v1.2.3"instead of capturing and restoring the pre-subtest value.
Neither is blocking merge.
🔎 Code quality review by PR Code Quality Reviewer · 56.1 AIC · ⌖ 4.54 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/add_package_manifest_test.go:388
Fragile cleanup: restores to hardcoded "v1.2.3" instead of the pre-test value — if test ordering or a prior sibling subtest's cleanup changes the baseline, this silently sets the wrong version for subsequent subtests.
<details>
<summary>💡 Suggested fix</summary>
Capture the current version at subtest entry and restore it:
t.Run("accepts git describe compiler version for matching min-version", func(t *testing.T) {
originalVersion := currentVersionForTest() // or however you r…
</details>
<details><summary>pkg/semverutil/semverutil.go:62</summary>
**Regex `g[0-9a-f]+` matches single-char hashes — real git abbreviated SHAs are at least 7 chars.** A prerelease like `1-ga` would be incorrectly treated as a git-describe suffix and silently stripped.
<details>
<summary>💡 Suggested fix</summary>
Require at least 7 hex characters after `g` to match real abbreviated SHAs:
```go
var gitDescribePrereleaseRegex = regexp.MustCompile(`^\[0-9\]+-g\[0-9a-f\]{7,}(?:-dirty)?$`)This prevents false-positive matches on short or contrived prereleas…
|
🎉 This pull request is included in a new release. Release: |
Summary
Fixes a false
min-versionfailure whengh-awis built from a non-tagged local commit. In that case,git describeproduces a version likev1.0.0-27-g117acb7f9c, which is semantically older thanv1.0.0under standard semver prerelease ordering, causing the min-version check to incorrectly reject a compatible compiler.Changes
pkg/semverutil/semverutil.gogitDescribePrereleaseRegexto match git-describe prerelease patterns (<N>-g<hash>and<N>-g<hash>-dirty).NormalizeGitDescribeSemver(v string) stringwhich strips git-describe-style anddirtyprerelease suffixes, collapsing the version to its base release tag.v1.2.3-27-gabc1234→v1.2.3v1.2.3-27-gabc1234-dirty→v1.2.3v1.2.3-dirty→v1.2.3v1.2.3-beta.1→v1.2.3-beta.1(real prerelease preserved)pkg/cli/add_package_manifest.gocurrentVersionis a valid semver, normalizes it withsemverutil.NormalizeGitDescribeSemverbefore comparing againstmanifest.MinVersion.Tests
pkg/semverutil/semverutil_test.go:TestNormalizeGitDescribeSemver— 5 table-driven cases covering clean tag, git-describe output, dirty, git-describe+dirty, and real prerelease.pkg/cli/add_package_manifest_test.go: new case verifying that a compiler version ofv1.0.0-27-g117acb7f9csatisfiesmin-version: v1.0.0.Risk
Low. Normalisation applies only to the in-memory compiler version string during manifest min-version checks. No stored data, API surface, or other comparisons are affected. Real prerelease labels are preserved.