feat: fall back to older cooled-down release instead of skipping update - #47730
Conversation
…Cache.Set Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 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
Adds cooldown fallback so updates select the newest cooled-down release instead of skipping entirely.
Changes:
- Adds cooldown-aware fallback for actions and workflows.
- Injects cooldown checks for testing.
- Rejects empty-SHA action cache entries and adds tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/action_cache.go |
Rejects empty SHA entries. |
pkg/workflow/action_cache_test.go |
Tests empty-SHA rejection. |
pkg/cli/update_workflows.go |
Adds workflow release fallback. |
pkg/cli/update_workflows_test.go |
Tests workflow cooldown behavior. |
pkg/cli/update_actions.go |
Adds action release fallback. |
pkg/cli/update_actions_test.go |
Tests action fallback paths. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/cli/update_actions.go:1006
- The primary cooldown lookup on line 996 still bypasses
deps.checkCoolDown, so this fallback is only reached according to a live GitHub API result rather than the injected result used by the new test. Route that lookup through the dependency to make the behavior deterministic and honor the new injection point.
olderVersion, olderSHA, findErr := findCooledDownActionVersion(ctx, deps, repo, currentVersion, effectiveAllowMajor, verbose, coolDown)
pkg/cli/update_actions.go:1006
- This message claims the whole workflow-file update is skipped, but the newly added fallback may immediately update it to an older release. Change the message to say that only
latestVersionis being skipped so successful runs do not emit contradictory status.
// Try to find an older release that has passed the cooldown period.
olderVersion, olderSHA, findErr := findCooledDownActionVersion(ctx, deps, repo, currentVersion, effectiveAllowMajor, verbose, coolDown)
- Files reviewed: 6/6 changed files
- Comments generated: 8
- Review effort level: Medium
| if sha == "" { | ||
| actionCacheLog.Printf("refusing to store action pin entry with empty SHA for %s@%s; entry skipped", repo, version) | ||
| return |
| continue | ||
|
|
||
| // Try to find an older release that has passed the cooldown period. | ||
| olderVersion, olderSHA, findErr := findCooledDownActionVersion(ctx, deps, entry.Repo, entry.Version, effectiveAllowMajor, verbose, coolDown) |
| ) (string, string, error) { | ||
| baseRepo := gitutil.ExtractBaseRepo(repo) | ||
|
|
||
| output, err := deps.runGHReleasesAPI(ctx, baseRepo) |
| // Iterate from newest to oldest upgrade candidate, stopping at the first release | ||
| // that has passed the cooldown period. This lets users receive the latest cooled | ||
| // release rather than being blocked on a single too-recent version. | ||
| for _, c := range upgradeCandidates { |
| @@ -198,8 +200,16 @@ func updateActions(ctx context.Context, deps actionUpdateDeps, allowMajor, verbo | |||
| if coolDownResult.InCoolDown { | |||
| cooldownLog.Printf("Action %s: %s", entry.Repo, coolDownResult.Message) | |||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Skipping update for %s: %s", entry.Repo, coolDownResult.Message))) | |||
| if result.InCoolDown { | ||
| cooldownLog.Printf("Action fallback %s@%s: %s", repo, c.tag, result.Message) | ||
| if verbose { | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Skipping update for %s: %s", repo, result.Message))) |
| return c.tag, nil | ||
| } | ||
| cooldownLog.Printf("Workflow source %s: %s", repo, result.Message) | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Skipping update for %s: %s", repo, result.Message))) |
| for _, c := range candidates { | ||
| result := deps.checkCoolDown(ctx, repo, c.tag, coolDown) |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (8 tests)
Flagged Tests (1)TestActionCache_Set_EmptySHARejected (action_cache_test.go) — design_test, high_value. Adds 28 lines for 4 production lines (7:1 ratio). Not a quality concern: covers both negative (empty SHA rejected) and positive (valid SHA accepted) paths in one test, which is good practice for this guard condition. Verdict
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (446 new lines in Draft ADR committed:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Cooldown fallback logic
The implementation is clean and well-structured. The core logic correctly walks candidates newest-first, stopping at the first release that has passed the cooldown window. The empty-SHA guard in ActionCache.Set is a good defensive addition. Test coverage is thorough across unit and integration scenarios.
Non-blocking observation: In updateActionRefsInContentWithDeps, when a repo's primary candidate is in cooldown, findCooledDownActionVersion makes an additional runGHReleasesAPI call. However, the fallback result is correctly written back to the per-invocation cache[cacheKey] map, so subsequent occurrences of the same repo+currentVersion pair in the same file are served from cache — no correctness issue and the extra API call is bounded to one per unique repo+version.
Overall the change achieves its goal safely. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 32.8 AIC · ⌖ 4.52 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on two correctness issues and one design concern.
📋 Key Themes & Highlights
Issues Found
-
Misleading "Skipping update" message (
update_actions.golines ~203 and ~1001): The stderr skip message fires unconditionally before the fallback attempt, so users see "Skipping update for ruby/setup-ruby" even when the action is actually updated to a fallback version. Same pattern in bothupdateActionsandupdateActionRefsInContentWithDeps. See inline comments. -
Missing test: SHA error falls to next candidate (
findCooledDownActionVersion): The code correctlycontinues whengetActionSHAForTagreturns an error, but no test covers the case where the first candidate fails SHA resolution and the second succeeds. See inline comment on line 575. -
Duplicated filter/sort logic (
findCooledDownActionVersion): The release-filtering, parsing, and descending-sort pattern is now in at least three places. Worth extracting as a shared helper to prevent drift.
Positive Highlights
- ✅
checkCoolDownis now injectable on both deps structs — excellent for deterministic testing - ✅
ActionCache.Setempty-SHA guard is a clean, defensive fix - ✅ Good test coverage overall: 5 new integration-level tests covering the key fallback paths
- ✅
resolveLatestReleaseWithDepsrefactor is clean and easy to follow
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 44.1 AIC · ⌖ 5.13 AIC · ⊞ 6.7K
Comment /matt to run again
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Skipping update for %s: %s", entry.Repo, coolDownResult.Message))) | ||
| skippedActions = append(skippedActions, entry.Repo) | ||
| continue | ||
|
|
There was a problem hiding this comment.
[/diagnosing-bugs] The "Skipping update for %s" message is printed to stderr unconditionally before attempting the fallback — so users see "Skipping update for ruby/setup-ruby" in the output even when the update actually proceeds to an older fallback version.
💡 Suggested fix
Move the stderr user-facing message inside the if findErr != nil || olderVersion == "" ... branch, and add an informational message on the fallback path:
if findErr != nil || olderVersion == "" || olderSHA == "" {
fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Skipping update for %s: %s", entry.Repo, coolDownResult.Message)))
skippedActions = append(skippedActions, entry.Repo)
continue
}
fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Falling back to %s for %s (latest release still in cooldown)", olderVersion, entry.Repo)))@copilot please address this.
| @@ -907,7 +1001,17 @@ func updateActionRefsInContentWithDeps(ctx context.Context, deps actionUpdateDep | |||
| if verbose { | |||
There was a problem hiding this comment.
[/diagnosing-bugs] Same misleading stderr message issue exists in updateActionRefsInContentWithDeps: the "Skipping update for %s" message is emitted before calling findCooledDownActionVersion, so the user sees a skip message even when the fallback succeeds.
💡 Suggested fix
Restructure to only show the skip message when no fallback is found, matching the same fix as the updateActions path for consistent UX:
// Only log skip when fallback also fails
olderVersion, olderSHA, findErr := findCooledDownActionVersion(...)
if findErr != nil || olderVersion == "" || olderSHA == "" {
cooldownLog.Printf(...)
fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Skipping update for %s: %s", repo, coolDownResult.Message)))
continue
}@copilot please address this.
| releases := strings.Split(strings.TrimSpace(string(output)), "\n") | ||
|
|
||
| currentVer := parseVersion(currentVersion) | ||
|
|
There was a problem hiding this comment.
[/tdd] findCooledDownActionVersion makes two network calls per fallback candidate (cooldown check + SHA fetch), but there is no test covering the case where getActionSHAForTag fails for one candidate and succeeds for the next — i.e., SHA fetch failure should silently continue to the next candidate.
💡 Suggested test
func TestFindCooledDownActionVersion_SHAErrorFallsToNext(t *testing.T) {
deps := newTestActionUpdateDeps()
deps.runGHReleasesAPI = func(_ context.Context, _ string) ([]byte, error) {
return []byte("v1.322.0
v1.321.0
v1.320.0"), nil
}
deps.checkCoolDown = func(_ context.Context, _, tag string, cd time.Duration) coolDownCheckResult {
return coolDownCheckResult{} // all cooled down
}
deps.getActionSHAForTag = func(_ context.Context, _, tag string) (string, error) {
if tag == "v1.322.0" {
return "", errors.New("not found")
}
if tag == "v1.321.0" {
return "sha321...", nil
}
return "", errors.New("unexpected")
}
version, _, err := findCooledDownActionVersion(context.Background(), deps, "repo/action", "v1.320.0", true, false, 7*24*time.Hour)
require.NoError(t, err)
assert.Equal(t, "v1.321.0", version)
}@copilot please address this.
| // findCooledDownActionVersion searches for the newest release that is strictly | ||
| // newer than currentVersion but has passed the cooldown period. It is used as | ||
| // a fallback when the highest candidate is still in cooldown: rather than | ||
| // skipping the update entirely, we walk down the release list toward older |
There was a problem hiding this comment.
[/codebase-design] findCooledDownActionVersion duplicates the release-list fetch, version parsing, filtering, and sorting logic already in resolveLatestReleaseWithDeps (in update_workflows.go) and the inner loop in getLatestActionReleaseViaGit. This is the third place this pattern appears, which will make future changes (e.g. adding pre-release opt-in) require triple updates.
Consider extracting a shared filteredUpgradeCandidates(releases []string, currentVer, allowMajor) helper that both the workflow and action paths can call, leaving the cooldown iteration to the call sites. This keeps the sorting/filtering logic as a single deep module.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — one confirmed correctness bug in the test suite; two medium UX/observability regressions.
Findings summary
High — test correctness
TestFindCooledDownActionVersion_EmptySHASkipped: thecurrentVersion(v4.4.0) is already newer than all mocked releases, so the function returns before reachinggetActionSHAForTag. The guard it claims to test is never exercised. Fix thecurrentVersionto something likev1.0.0.
Medium — UX regressions
resolveLatestReleaseWithDeps(workflows): the new fallback loop emits a "Skipping update"stderrline for every in-cooldown candidate, not just the first. Repos with many recent releases will spam the terminal.isExemptFromCoolDownearly-return: the--verbose"Found newer release" message is lost for exempt repos — regression from the old code path.
Low — design concern (non-blocking)
findCooledDownActionVersion(actions): agetActionSHAForTagerror silently advances to the next-oldest release instead of aborting. The implicit retry-on-error is surprising and undocumented.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 62.5 AIC · ⌖ 5.1 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/update_actions_test.go:245
False-positive test — the empty-SHA guard is never exercised: currentVersion is "v4.4.0" but the mocked release list only contains v1.321.0 and v1.320.0. Neither is newer than v4.4.0, so candidates is empty and the function returns ("", "", nil) before ever calling getActionSHAForTag. The test passes for the wrong reason.
<details>
<summary>💡 Fix</summary>
Use a currentVersion that is older than the mocked releases so the candidates list is non-empty, e.g.:
version…
</details>
<details><summary>pkg/cli/update_workflows.go:488</summary>
**Cooldown log spam for every skipped candidate**: the loop prints a cooldown message (`fmt.Fprintln(os.Stderr, ...)`) for every in-cooldown candidate, not just the first. For a repo with 10 recent releases all in cooldown, users will see 10 "Skipping update" lines for a single action — noisy and confusing.
<details>
<summary>💡 Fix</summary>
Log once (at entry to the loop or only for the first skipped candidate), then continue silently:
```go
firstSkip := true
for _, c := range upgradeCandi…
</details>
<details><summary>pkg/cli/update_workflows.go:473</summary>
**`verbose` log silently dropped for cooldown-exempt repos**: when `isExemptFromCoolDown(repo)` is true the function returns the best candidate immediately, but never emits the `"Found newer release"` log that the old code did (and that the in-loop path still does). Users running with `--verbose` will see no output for exempt repos even when they are being updated.
<details>
<summary>💡 Fix</summary>
```go
if isExemptFromCoolDown(repo) {
if verbose {
fmt.Fprintln(os.Stderr, consol…
</details>
<details><summary>pkg/cli/update_actions.go:113</summary>
**`findCooledDownActionVersion` makes a SHA call even when it will be discarded on cooldown error**: the function calls `deps.getActionSHAForTag` after confirming a candidate is not in cooldown — this is correct — but if `getActionSHAForTag` fails it logs and continues trying the _next_ candidate. That means a transient SHA resolution failure silently falls through to an older release rather than bubbling the error. Callers will get a quietly downgraded version with no warning to the user.
<de…
</details>
🤖 PR Triage
Score breakdown: Impact 28 · Urgency 16 · Quality 8 Summary: Adds cooldown-fallback logic to Reviews: 2× CHANGES_REQUESTED (github-actions[bot]) · 1× APPROVED
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in I refactored release-candidate filtering/sorting into shared helpers and reused them in both action/workflow cooldown fallback paths, then ran local validation ( |
|
@copilot please run the Unresolved review feedback:
|
Confirmed on current HEAD ( |
🤖 PR Triage
Rationale: Core update logic change — walks back through release candidates when newest is in cooldown. Good description, CI green, 8 files +683/-57. Needs human review of release-selection logic.
|
Confirmed on current HEAD ( Current failures remain in |
🤖 PR Triage
Rationale: Adds cooldown fallback logic so Blockers: 3 CI failures —
|
|
@copilot please run the Failed checks on the latest head: Run: https://github.com/github/gh-aw/actions/runs/30120416243
|
|
Triage | Category: Recommended action: Score breakdown: impact=28 (UX improvement), urgency=15 (open 19+ hrs), quality=12 (CI 3 failures, has ADR+tests)
|
|
@copilot please run the Unresolved review feedback:
Failed checks on the latest head: Run: https://github.com/github/gh-aw/actions/runs/30139125642
|
|
🎉 This pull request is included in a new release. Release: |
When the newest available release is still within the cooldown window,
gh aw updatewas silently skipping the action/workflow entirely. It should instead walk older candidates (newer than current) until it finds one that has cooled down.Cooldown fallback logic
resolveLatestReleaseWithDeps— sorts compatible upgrade candidates newest-first, iterates checking cooldown, returns first cooled-down version; falls back tocurrentRefif all are still hotupdateActions/updateActionRefsInContentWithDeps— same fallback via newfindCooledDownActionVersionhelper, which fetches the full release list and walks it when the primary check triggers cooldowncheckCoolDownis now injectable on bothactionUpdateDepsandworkflowUpdateDepsfor deterministic testingEmpty-SHA guard
ActionCache.Setnow rejects entries withsha == ""(logs and returns early).findCooledDownActionVersionalso validates SHA before returning a candidate, preventing a cooldown failure from silently writing an unresolvable pin toactions-lock.json.Run: https://github.com/github/gh-aw/actions/runs/30074181699
Run: https://github.com/github/gh-aw/actions/runs/30087480557
Run: https://github.com/github/gh-aw/actions/runs/30091871574