Conversation
|
@copilot if upgrade occurred, relaunch cli with same flags (and a special flag to prevent recursion hidden) |
…run with new version Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
gh aw upgradegh aw upgrade to prevent stale version in lock files
…e version in lock files Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in be37b31. After the extension is upgraded, the CLI now re-launches the freshly-installed binary automatically with the same flags plus a hidden Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where gh aw upgrade would detect an outdated extension version but only warn, then continue compiling lock files with the old version string baked into the running process. The fix introduces automatic self-upgrade via gh extension upgrade and re-launching the freshly-installed binary with a hidden --skip-extension-upgrade flag to prevent recursion, ensuring all subsequent steps use the correct version string. Additionally, it replaces string-based version comparison with proper semver comparison via golang.org/x/mod/semver.
Changes:
- Added
upgradeExtensionIfOutdated()inupdate_extension_check.gothat auto-upgrades the extension and uses proper semver comparison - Modified
upgrade_command.goto relaunch the new binary after upgrade viarelaunchWithSameArgs(), with a hidden--skip-extension-upgradeflag to prevent infinite recursion - Added tests for dev build skip and silent API failure scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
pkg/cli/update_extension_check.go |
New upgradeExtensionIfOutdated function with semver-based comparison and automatic gh extension upgrade |
pkg/cli/upgrade_command.go |
Replaced warn-only check with auto-upgrade + relaunch mechanism; added relaunchWithSameArgs helper and --skip-extension-upgrade flag |
pkg/cli/update_extension_check_test.go |
Tests for dev build skip and silent API failure in the new function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
pkg/cli/upgrade_command.go
Outdated
| if upgraded { | ||
| upgradeLog.Print("Extension was upgraded; re-launching with new binary") | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Continuing upgrade with newly installed version...")) | ||
| return relaunchWithSameArgs("--skip-extension-upgrade") |
There was a problem hiding this comment.
Bug: When --create-pull-request (or --pr) is passed and the extension is upgraded, the child process relaunched via relaunchWithSameArgs receives all original arguments including --create-pull-request. The child process performs the full upgrade and creates the PR. After the child exits successfully, relaunchWithSameArgs returns nil, which propagates back to RunE. The code at line 100 then checks createPR again and attempts to create a second PR.
To fix this, runUpgradeCommand should signal to the caller that a relaunch occurred (e.g., by returning a sentinel error or a boolean), so that RunE can skip the PR creation step. Alternatively, the relaunch path could call os.Exit(0) after the child succeeds (similar to how it handles child failures), since the child has already done all the work.
| return relaunchWithSameArgs("--skip-extension-upgrade") | |
| if err := relaunchWithSameArgs("--skip-extension-upgrade"); err != nil { | |
| // If the child process failed, propagate the error to the caller. | |
| return err | |
| } | |
| // The child process completed successfully and has already performed the upgrade | |
| // (including any side effects such as creating pull requests). Exit the parent | |
| // process so we do not repeat the remaining steps. | |
| os.Exit(0) | |
| return nil |
pkg/cli/update_extension_check.go
Outdated
| // Fall back to normalised string comparison when versions are not valid semver. | ||
| currentNorm := strings.TrimPrefix(currentVersion, "v") | ||
| latestNorm := strings.TrimPrefix(latestVersion, "v") | ||
| if currentNorm >= latestNorm { | ||
| updateExtensionCheckLog.Print("Extension is already up to date (string comparison fallback)") | ||
| if verbose { | ||
| fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("✓ gh-aw extension is up to date")) | ||
| } | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
Bug: The string comparison fallback (currentNorm >= latestNorm) suffers from the exact same lexicographic ordering problem the PR claims to fix. For example, "0.9.0" >= "0.10.0" evaluates to true lexicographically (since "9" > "1"), which would incorrectly report the extension as up-to-date when it's actually outdated. This would prevent the upgrade from happening, which is the opposite of what the PR intends.
While this fallback is only reached for non-semver version strings, it would be safer to log a warning and proceed with the upgrade attempt rather than relying on broken string comparison. Or simply remove this fallback and always require valid semver.
| // Fall back to normalised string comparison when versions are not valid semver. | |
| currentNorm := strings.TrimPrefix(currentVersion, "v") | |
| latestNorm := strings.TrimPrefix(latestVersion, "v") | |
| if currentNorm >= latestNorm { | |
| updateExtensionCheckLog.Print("Extension is already up to date (string comparison fallback)") | |
| if verbose { | |
| fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("✓ gh-aw extension is up to date")) | |
| } | |
| return false, nil | |
| } | |
| // Versions are not valid semver; skip unreliable string comparison and | |
| // proceed with upgrade to avoid incorrectly treating an outdated version | |
| // as up to date. | |
| updateExtensionCheckLog.Printf("Non-semver versions detected (current=%q, latest=%q); proceeding with upgrade", currentVersion, latestVersion) |
|
@coplot review and apply comments |
|
@copilot review and apply comments |
…ove broken string-comparison fallback Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Both review comments addressed in 011edea:
|
|
@copilot review and simplify updates |
… dead ensureLatestExtensionVersion Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 518c7e4. Simplifications applied:
|
Adds the 🌍 Community Contributions table with 5 confirmed attributions linking community-labeled issues to their resolving PRs: - #21630 (@Mossaka) → #21655 sparse-checkout support docs - #21243 (@holwerda) → #21286 github-app auth in dependencies - #20299 (@grahame-white) → #20300 fix gh aw upgrade lock files - #20249 (@mvdbos) → #20301 cross-repo workflow_call validation - #18480 (@JoshGreenslade) → #21993 GHE Cloud data residency docs Also flags 241 community-labeled issues as attribution candidates needing manual review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
gh aw upgradedetected outdated extension versions but only warned, then continued compiling lock files with the old version string baked into the running process. Lock file headers (GH_AW_INFO_CLI_VERSION) and generated YAML ended up referencing the previous version.Changes
update_extension_check.go— NewupgradeExtensionIfOutdated(verbose bool) (bool, error)that:gh extension upgrade github/gh-awwhen a newer release is detectedgolang.org/x/mod/semver.Compareinstead of string comparison (fixes incorrect ordering for versions like0.9.0vs0.10.0)(true, nil)on successful upgrade; silently ignores API/network errors; skips dev buildsupgrade_command.go— Replaces the warn-onlyensureLatestExtensionVersioncall withupgradeExtensionIfOutdated. When an upgrade is performed, the command automatically re-launches the freshly-installed binary with the same flags plus a hidden--skip-extension-upgradeflag that prevents infinite recursion. This ensures all subsequent steps (codemods, lock-file compilation) run under the new binary and embed the correct version string. TherelaunchWithSameArgshelper re-execs the binary with forwarded stdin/stdout/stderr and preserves the child exit code.update_extension_check_test.go— Tests forupgradeExtensionIfOutdatedcovering dev build skip and silent API failure.Behaviour
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.