Fix pseudo version hash handling#181
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughMigrate now prefers the commit SHA returned by GitHub for pseudo-version short component (falls back to input hash); migrate's target-version guard is relaxed under --force (allows non-greater targets); go.mod/fiber regexes and migration semver lower bounds were adjusted; tests updated to use short input hashes and mocked full Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User (CLI)
participant M as migrate cmd
participant GH as GitHub API
participant GOM as go.mod
U->>M: run migrate --hash <short_hash> [--force]
M->>GH: GET /repos/.../commits/<short_hash>
GH-->>M: { "sha":"<full_sha>", "commit":{ "author":{ "date": "..." } } }
note right of M: chooseSHA = GH.sha if present\nelse chooseSHA = inputHash
M->>M: shortComponent := chooseSHA[:12]
alt target not greater && not --force
M-->>U: reject migration
else
M->>GOM: update pseudo-version using date + shortComponent
M-->>U: success / updated go.mod
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ReneWerner87, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses an issue with how pseudo-versions are handled, specifically ensuring that commit pseudo-versions correctly utilize the full SHA hash. It also updates migration tests to properly account for short hashes, improving the robustness of version handling.
Highlights
- Pseudo-version hash resolution: The "pseudoVersionFromHash" function now extracts the full SHA from the GitHub API response, ensuring pseudo-versions are generated using the complete commit hash rather than potentially truncated ones.
- Migration test updates: The "Test_Migrate_WithHash" test has been refactored to simulate scenarios where a short hash is provided, validating that the system correctly resolves and uses the full SHA for pseudo-version generation.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request aims to improve pseudo-version generation by using the full commit SHA from the GitHub API, even when a short hash is provided. The changes correctly fetch the full SHA and update the test to cover this scenario.
My review identifies one significant issue in the error handling. If the GitHub API returns an error (e.g., for an invalid hash), the code doesn't detect this and proceeds to generate a pseudo-version with incorrect data. I've provided a suggestion to address this by ensuring a valid SHA is received from the API before proceeding.
| short := data.SHA | ||
| if short == "" { | ||
| short = hash | ||
| } |
There was a problem hiding this comment.
This fallback logic can mask an underlying problem. If the GitHub API call for a commit hash results in an error (like a 404 Not Found), data.SHA will be empty. The code currently falls back to using the input hash, but data.Commit.Committer.Date will also be its zero value. This leads to the creation of an invalid pseudo-version with a timestamp from year 1.
Instead of falling back, it would be more robust to treat an empty data.SHA as an error, as it indicates the API did not return the expected commit information.
| short := data.SHA | |
| if short == "" { | |
| short = hash | |
| } | |
| if data.SHA == "" { | |
| return "", fmt.Errorf("commit hash %q not found via GitHub API", hash) | |
| } | |
| short := data.SHA |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cmd/migrate.go (3)
149-156: Fix golangci-lint fieldalignment failure by reordering fields in the decode structGovet flags the anonymous struct at Line 149: "struct with 40 pointer bytes could be 32". Reordering fields removes padding and unblocks the pipeline without changing JSON decoding behavior.
Apply this diff:
var data struct { - SHA string `json:"sha"` - Commit struct { - Committer struct { - Date time.Time `json:"date"` - } `json:"committer"` - } `json:"commit"` + Commit struct { + Committer struct { + Date time.Time `json:"date"` + } `json:"committer"` + } `json:"commit"` + SHA string `json:"sha"` }
134-147: Handle non-200 GitHub responses to avoid invalid pseudo-versionsIf GitHub returns a non-200 (e.g., 404 for a short/unknown hash), the code still attempts to decode JSON and proceeds with zero time, yielding an invalid pseudo-version. Check StatusCode and fail early with context.
Apply this diff to add status validation and an informative error:
res, err := client.Do(req) if err != nil { return "", fmt.Errorf("http request failed: %w", err) } +if res.StatusCode != http.StatusOK { + // read a small body snippet for diagnostics; ignore read errors + b, _ := io.ReadAll(io.LimitReader(res.Body, 4<<10)) + return "", fmt.Errorf("github commits API %s: %s: %s", url, res.Status, strings.TrimSpace(string(b))) +}And add the missing import:
import ( "context" "encoding/json" "fmt" + "io" "net/http" "os" "strconv" "strings" "time" )
161-169: Validate date and enforce 12-char hash when API SHA is unavailable
- If the API payload lacks a commit date, generating a pseudo-version is unsafe—fail fast.
- When the API doesn’t return SHA and the user provided a short hash (<12), the resulting pseudo-version may be rejected by the Go toolchain. Require at least 12 hex chars when falling back to the user input.
Apply this diff:
-short := data.SHA -if short == "" { - short = hash -} +fromAPI := data.SHA != "" +short := data.SHA +if short == "" { + short = hash +} +if !fromAPI && len(short) < 12 { + return "", fmt.Errorf("commit hash must be at least 12 hex characters when GitHub did not return a SHA") +} if len(short) > 12 { short = short[:12] } -pv := module.PseudoVersion("v"+strconv.FormatUint(base.Major(), 10), "v"+base.String(), data.Commit.Committer.Date, short) +if data.Commit.Committer.Date.IsZero() { + return "", fmt.Errorf("commit date missing in API response for %q", url) +} +pv := module.PseudoVersion("v"+strconv.FormatUint(base.Major(), 10), "v"+base.String(), data.Commit.Committer.Date, short) return strings.TrimPrefix(pv, "v"), nil
🧹 Nitpick comments (2)
cmd/migrate_test.go (2)
321-332: Strengthen the test by asserting the GitHub commit endpoint was called exactly onceMakes the test more deterministic and guarantees the code path actually queried the short-hash endpoint.
Apply this diff:
httpmock.Activate() defer httpmock.DeactivateAndReset() commitURL := "https://api.github.com/repos/gofiber/fiber/commits/" + short httpmock.RegisterResponder(http.MethodGet, commitURL, httpmock.NewBytesResponder(200, []byte(`{"sha":"`+hash+`","commit":{"committer":{"date":"2020-01-02T03:04:05Z"}}}`))) @@ _, err = runCobraCmd(cmd, "-t=3.0.0", "--hash="+short) require.NoError(t, err) +// Ensure our short-hash lookup was actually performed +calls := httpmock.GetCallCountInfo() +assert.Equal(t, 1, calls[http.MethodGet+" "+commitURL])
321-332: Optional: Add a fallback-path test when API returns noshaConsider a companion test where the API returns 200 without a top-level
sha, and the provided hash is already 12+ chars, verifying we still produce the correct pseudo-version from the user input.If helpful, I can draft this test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cmd/migrate.go(2 hunks)cmd/migrate_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: golangci-lint
cmd/migrate.go
[error] 149-149: golangci-lint: fieldalignment: struct with 40 pointer bytes could be 32 (govet)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build (1.25.x, windows-latest)
🔇 Additional comments (1)
cmd/migrate_test.go (1)
321-332: LGTM: Test now exercises short-hash input and asserts pseudo-version uses API-provided SHAThis validates the intended behavior shift and protects against regressions where a 7-char hash would leak into the pseudo-version. Nice.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
cmd/internal/migrations/lists.go (1)
33-33: Semver lower-bound widened correctly to include pre-releases (-0).Using ">=1.0.0-0" ensures v1 pre-releases are included. The "To: >=0.0.0-0" effectively makes the upper constraint always true; that’s fine if the intent is to always run MigrateGoPkgs whenever curr >= 1.0.0-0. If you want to make that intent clearer, consider a wildcard like "*" (Masterminds/semver supports it) or a brief comment explaining why "To" is intentionally unconstrained.
cmd/internal/migrations/lists_test.go (2)
41-42: Tests aligned with new -0 boundaries.Assertions now reflect the updated constraints in lists.go. Consider adding one pre-release case (e.g., target = 3.0.0-rc.1) to prove the -0 gating behaves as intended for v3 prereleases.
Here’s a minimal additional test you could add (outside this hunk) to validate pre-release handling:
func Test_DoMigration_Verbose_PreReleaseTarget(t *testing.T) { t.Parallel() dir := t.TempDir() curr := semver.MustParse("2.0.0") target := semver.MustParse("3.0.0-rc.1") var buf bytes.Buffer cmd := &cobra.Command{} cmd.SetOut(&buf) require.NoError(t, migrations.DoMigration(cmd, dir, curr, target, true, true)) out := buf.String() assert.Contains(t, out, "MigrateGoPkgs") // should consider Go package migration // v3 migrations should be considered since <4.0.0-0 matches 3.0.0-rc.1 assert.NotContains(t, out, "Skipping migration from >=2.0.0-0 to <4.0.0-0") }
79-79: Duplicate check is fine but slightly brittle.Repeatedly asserting the exact "Skipping migration ..." string ties the test to formatting. If you want to reduce brittleness, consider checking for substrings like ">=2.0.0-0" and "<4.0.0-0" independently, or use a regex.
cmd/version.go (1)
43-43: Broadened whitespace handling is good; tighten capture to avoid trailing comments and anchor to go.mod grammar.Current
(.*)will still capture things like "// indirect". Anchoring to the require line and capturing only the version token makes this more robust, while still accepting tabs/spaces.Apply this diff:
-var ( - currentVersionRegexp = regexp.MustCompile(`github\.com/gofiber/fiber[^\n]*?\s+(.*)\n`) +var ( + // Match a require line or a line inside a require (...) block, capture only the version token. + // Examples matched: + // require github.com/gofiber/fiber/v2 v2.52.0 + // require github.com/gofiber/fiber/v2 v2.52.0 + // require ( + // github.com/gofiber/fiber/v2 v2.52.0 // indirect + // ) + currentVersionRegexp = regexp.MustCompile(`(?m)^\s*(?:require\s+)?\s*github\.com/gofiber/fiber/v\d+\s+(v[^\s]+)\b`)Optionally, add a unit test covering both single-line
requireand blockrequire (...)with// indirect.cmd/internal/migrations/common.go (1)
17-17: Whitespace relaxation is correct; address CodeQL “missing anchor” by anchoring to go.mod lines without changing capture groups.Current pattern can match anywhere. You can keep groups 1–4 intact for the replacement while anchoring to begin/end of go.mod lines and supporting both
requiresingle-line and block forms.Apply this diff:
-var ( - pkgRegex = regexp.MustCompile(`(github\.com/gofiber/fiber/)(v\d+)(\s+)(v[\w.-]+)`) +var ( + // Anchor to lines in go.mod (either "require <mod> <ver>" or entries in a require (...) block). + // Keeps capture groups compatible with existing replacement: + // 1: module path prefix "github.com/gofiber/fiber/" + // 2: major "v<digits>" + // 3: whitespace between major and full version + // 4: full version (e.g., v2.52.0 or pseudo-versions) + pkgRegex = regexp.MustCompile(`(?m)^(?:\s*(?:require\s+)?)\s*(github\.com/gofiber/fiber/)(v\d+)(\s+)(v[\w\.-]+)\s*$`)This should appease the CodeQL warning while preserving behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
cmd/internal/migrations/common.go(1 hunks)cmd/internal/migrations/lists.go(1 hunks)cmd/internal/migrations/lists_test.go(3 hunks)cmd/migrate_test.go(1 hunks)cmd/version.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/migrate_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
cmd/internal/migrations/lists_test.go
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
cmd/version.go
🧬 Code graph analysis (1)
cmd/internal/migrations/lists.go (1)
cmd/internal/migrations/common.go (1)
MigrateGoPkgs(21-58)
🪛 GitHub Check: CodeQL
cmd/internal/migrations/common.go
[failure] 17-17: Missing regular expression anchor
When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build (1.25.x, windows-latest)
🔇 Additional comments (2)
cmd/internal/migrations/lists.go (1)
35-36: Good use of -0 to gate v3 migrations and exclude any v4 pre-releases."From: >=2.0.0-0" and "To: <4.0.0-0" correctly include v3 pre-releases (e.g., 3.0.0-rc.1) while excluding anything at or beyond v4 pre-release. Nice.
cmd/internal/migrations/lists_test.go (1)
62-62: Expectation updated consistently.This mirrors the lists.go change and keeps the verbose output stable across versions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cmd/internal/migrations/common.go (1)
17-17: Anchoring addresses the prior GHAS finding.The new anchored regex removes the “matches anywhere” issue previously flagged by code scanning. Good move.
cmd/migrate.go (1)
161-165: Don’t fall back to a short input hash; treat missingsha/timestamp as an error and enforce a 12-char short.If GitHub responds with an error or a non-commit payload,
data.SHAmay be empty anddata.Commit.Committer.Datezero. Falling back tohashcan yield an invalid pseudo-version (shorter than 12 chars, zero timestamp). Prefer failing fast.Apply this diff within
pseudoVersionFromHash:- short := data.SHA - if short == "" { - short = hash - } - if len(short) > 12 { - short = short[:12] - } + // Validate API result + if data.SHA == "" || data.Commit.Committer.Date.IsZero() { + return "", fmt.Errorf("commit %q not found or missing date in GitHub API response", hash) + } + // Use a 12-char short from the full SHA as per Go pseudo-version convention + short := data.SHA + if len(short) < 12 { + return "", fmt.Errorf("resolved commit SHA too short (%d): %q", len(short), short) + } + short = short[:12]Additionally (recommended even though outside the changed hunk), explicitly fail on non-200 responses:
res, err := client.Do(req) if err != nil { return "", fmt.Errorf("http request failed: %w", err) } + if res.StatusCode != http.StatusOK { + return "", fmt.Errorf("github api GET %s: status %d", url, res.StatusCode) + }This prevents generating pseudo-versions that
go modmay reject and aligns with the goal of using the full commit SHA for derivation.
🧹 Nitpick comments (1)
cmd/migrate.go (1)
134-141: Harden GitHub API calls with headers and optional auth to avoid 403/rate limits.Set a User-Agent and honor
GITHUB_TOKENif present. This improves reliability without changing behavior.Apply this small addition:
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { return "", fmt.Errorf("create http request: %w", err) } - client := http.Client{} + req.Header.Set("User-Agent", "gofiber-cli (+https://github.com/gofiber/cli)") + if tok := os.Getenv("GITHUB_TOKEN"); tok != "" { + req.Header.Set("Authorization", "Bearer "+tok) + req.Header.Set("Accept", "application/vnd.github+json") + } + client := http.Client{}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
cmd/internal/migrations/common.go(2 hunks)cmd/migrate.go(2 hunks)cmd/migrate_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/migrate_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build (1.25.x, windows-latest)
🔇 Additional comments (1)
cmd/migrate.go (1)
97-99: Force-mode guard relaxation looks good; verify downgrade flow semantics.Allowing non-greater targets under
--forceis reasonable. Please confirmmigrations.DoMigrationinterpretsmigrateFromandtargetVersioncorrectly for downgrades/equal versions to avoid reapplying irreversible steps.Consider running focused tests:
- downgrading within same major with
--force- same-version “re-run” with
--force
|
|
||
| var ( | ||
| pkgRegex = regexp.MustCompile(`(github\.com/gofiber/fiber/)(v\d+)( *?)(v[\w.-]+)`) | ||
| pkgRegex = regexp.MustCompile(`(?m)^(\s*require\s+)?(github\.com/gofiber/fiber/)(v\d+)(\s+)(v[\w.-]+)$`) |
There was a problem hiding this comment.
Fix anchored go.mod regex: it misses indentation and “// indirect” comments, so replacements won’t happen inside require blocks.
Your pattern anchors to start/end but only accepts leading whitespace when the line also contains "require ". Lines inside a require (...) block are indented and often end with // indirect, so they won’t match and Fiber won’t be updated in go.mod.
Adjust the regex to always allow indentation and to preserve any trailing comment. Update the replacement to keep the indentation and trailing comment.
Apply this diff:
- pkgRegex = regexp.MustCompile(`(?m)^(\s*require\s+)?(github\.com/gofiber/fiber/)(v\d+)(\s+)(v[\w.-]+)$`)
+ pkgRegex = regexp.MustCompile(`(?m)^(\s*)(?:require\s+)?(github\.com/gofiber/fiber/)(v\d+)(\s+)(v[\w\.-]+)(\s*(?:\/\/.*)?)$`)
@@
- "${1}${2}v"+strconv.FormatUint(target.Major(), 10)+"${4}v"+target.String(),
+ "${1}${2}v"+strconv.FormatUint(target.Major(), 10)+"${4}v"+target.String()+"${6}",Follow-up:
- Add tests covering:
- single-line require,
- block require with indentation,
- lines ending with
// indirect.
Also applies to: 40-41
🤖 Prompt for AI Agents
In cmd/internal/migrations/common.go around lines 17 (and also update the
related patterns/logic at lines 40-41), the current pkgRegex is too strict: it
only allows leading whitespace when the line contains "require " and it doesn't
permit trailing comments like "// indirect", so matches inside indented
require(...) blocks fail and replacements lose indentation/comments; change the
regex to always allow optional leading indentation, capture an optional
"require" token, capture the module path and version groups, and capture any
trailing whitespace and optional trailing comment so the replacement can
reinsert the original indentation and comment; update the replacement string to
reuse the captured leading indentation and trailing comment groups (preserving
indentation and any "// indirect" or other comments); run/update tests to cover
single-line require, block require with indentation, and lines ending with "//
indirect".
2 participants
Summary
Testing
make lint(fails: terminated)make test(fails: terminated)go test ./cmd -run Test_Migrate_WithHash -count=1https://chatgpt.com/codex/tasks/task_e_68aa1f1ddc6883269f8c5b40a136fa4e
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests