Refresh update-time container digests and enforce commit-age cooldowns - #47990
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds cooldown enforcement for moving workflow refs and refreshes container digest pins during updates.
Changes:
- Resolves branch commit metadata and applies a three-day cooldown.
- Normalizes and refreshes container image digest pins.
- Adds focused unit and integration coverage.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/update_workflows.go |
Adds commit-aware ref resolution and cooldown checks. |
pkg/cli/update_workflows_test.go |
Tests branch and SHA cooldown behavior. |
pkg/cli/update_integration_test.go |
Verifies commit metadata retrieval. |
pkg/cli/update_cooldown.go |
Defines commit cooldown logic. |
pkg/cli/update_cooldown_test.go |
Tests commit cooldown calculations. |
pkg/cli/update_container_pins.go |
Normalizes and optionally refreshes container pins. |
pkg/cli/update_container_pins_test.go |
Tests normalization and digest refresh. |
pkg/cli/update_command.go |
Enables pin refresh during updates. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/cli/update_workflows.go:341
- The default-branch path also omits the documented
actions/*andgithub/*cooldown exemption, so SHA-tracked workflows from those namespaces no longer update immediately as promised by the shared flag contract. Guard this check with!isExemptFromCoolDown(repo)as the release paths do.
if result := checkCommitCoolDownWithDate(repo, defaultBranch, latestCommit.CommittedAt, coolDown); result.InCoolDown {
cooldownLog.Printf("Workflow source %s default branch %s: %s", repo, defaultBranch, result.Message)
fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Skipping commit candidate %s@%s: %s", repo, defaultBranch, result.Message)))
return currentSHA, nil
}
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Medium
| if result := checkCommitCoolDownWithDate(repo, currentRef, latestCommit.CommittedAt, commitCoolDown); result.InCoolDown { | ||
| cooldownLog.Printf("Workflow source %s branch %s: %s", repo, currentRef, result.Message) | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Skipping commit candidate %s@%s: %s", repo, currentRef, result.Message))) | ||
| return currentRef, nil |
There was a problem hiding this comment.
Addressed in d9d7bb3: cooldown-blocked refs now return an explicit blocked result, and redirect resolution exits before any workflow download/redirect-following when blocked.
| commitCoolDown := effectiveCommitCoolDown(coolDown) | ||
| if result := checkCommitCoolDownWithDate(repo, currentRef, latestCommit.CommittedAt, commitCoolDown); result.InCoolDown { | ||
| cooldownLog.Printf("Workflow source %s branch %s: %s", repo, currentRef, result.Message) | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Skipping commit candidate %s@%s: %s", repo, currentRef, result.Message))) | ||
| return currentRef, nil | ||
| } |
There was a problem hiding this comment.
Addressed in d9d7bb3: commit cooldown checks now apply the existing actions/* and github/* exemption before evaluating branch/default-branch commit age.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Refresh update-time container digests and enforce commit-age cooldowns
The overall design is solid and well-tested. Two pre-existing inline comments already flag the two blocking issues:
1. Stale branch ref returned instead of current SHA on cooldown (update_workflows.go ~line 305)
When the current ref is a commit SHA and the latest default-branch commit is in cooldown, resolveLatestCommitFromDefaultBranchWithDeps correctly returns currentSHA. But in the branch path of resolveLatestRefWithDeps, the function returns the branch name string (e.g. main) on cooldown rather than the callers original SHA — the mutable branch ref is written back to the source, which effectively adopts the branch as the new ref despite the cooldown.
2. Commit cooldown skips the actions/* / github/* exemption (update_workflows.go ~line 306)
effectiveCommitCoolDown applies the 3-day window unconditionally. isExemptFromCoolDown, which exempts actions/* and github/* repos per the --cool-down flag documentation, is never consulted — this breaks the existing contract for those namespaces.
Both issues need to be resolved before merge.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 27.2 AIC · ⌖ 7.4 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (25 tests)
✅ HighlightsBehavioral contracts enforced:
Strengths:
Edge cases covered:
Verdict
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (296 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 Matter
ADRs 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
|
There was a problem hiding this comment.
REQUEST_CHANGES — Two issues need addressing before merge.
Blocking findings
1. Hard error on missing commit date (resilience regression)
parseLatestBranchCommitInfo returns a hard error when the GitHub API returns a commit with no date. The old code only needed the SHA and always succeeded. This can break gh aw update for any workflow source whose repo returns a dateless commit. The fix is to treat a missing date as CommittedAt = zero (which the cooldown check handles correctly — zero time produces a very large age and passes immediately). See inline comment on line 513.
2. effectiveCommitCoolDown silently ignores user's --cooldown value
Any positive --cooldown value (even 1h) is silently overridden to the hardcoded 3-day window for branch/commit sources. --cooldown 0 is the only way to reduce it. This violates the documented contract of the flag and surprises users who set shorter cooldowns expecting them to apply everywhere. See inline comment on line 116.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 69.9 AIC · ⌖ 4.85 AIC · ⊞ 5.7K
Comment /review to run again
| dateStr = result.Commit.Author.Date | ||
| } | ||
| if dateStr == "" { | ||
| return latestBranchCommitInfo{}, fmt.Errorf("empty commit date returned for branch %s", branch) |
There was a problem hiding this comment.
Hard error on missing commit date will break workflow updates for any commit lacking date fields.
💡 Suggested fix
If both committer and author dates are absent, treat it as CommittedAt = zero and skip the cooldown check rather than returning an error. A zero CommittedAt will produce a very large age in checkCommitCoolDownWithDate, so the commit will always pass cooldown — safe and graceful.
if dateStr == "" {
// No date available: skip cooldown, return just the SHA.
return latestBranchCommitInfo{SHA: result.SHA}, nil
}Returning a hard error here means that any workflow sourced from a repo where GitHub returns a commit with no date (e.g., certain merge-queue commits, squash commits from some forges, or API edge cases) will cause the entire gh aw update to fail for that source. The old getLatestBranchCommitSHA only required the SHA and always succeeded — this is a clear resilience regression.
There was a problem hiding this comment.
Addressed in d9d7bb3: missing committer date no longer hard-fails update resolution. We now return SHA-only metadata and allow update flow to continue without applying commit cooldown for that commit date.
| if coolDown <= 0 { | ||
| return 0 | ||
| } | ||
| return commitRefCoolDown |
There was a problem hiding this comment.
effectiveCommitCoolDown silently overrides the user-configured cooldown, always applying the hardcoded 3-day window regardless of what --cooldown was set to.
💡 Details
A user who sets --cooldown 1h expecting hourly updates everywhere will get 1-hour release cooldowns but 3-day commit cooldowns with no warning or documentation at the call site. The coolDownFlagUsage string says the flag controls the cooldown period, and actions/*/github/* repos are documented as the only exemptions — the user has no way to know their value is silently ignored for branch/commit sources.
This is especially surprising when --cooldown 0 disables cooldowns (handled correctly) but any nonzero value, including values shorter than 3 days, still locks to 3 days.
Consider either:
- Using
min(coolDown, commitRefCoolDown)so the user can shorten but not eliminate the minimum safety window, or - Documenting the override prominently in
coolDownFlagUsageso users know commit sources always use the 3-day floor.
// Option 1: respect user intent but enforce a floor
func effectiveCommitCoolDown(coolDown time.Duration) time.Duration {
if coolDown <= 0 {
return 0
}
return max(coolDown, commitRefCoolDown)
}There was a problem hiding this comment.
Addressed in d9d7bb3: kept the fixed 3-day commit cooldown behavior and documented it directly in coolDownFlagUsage so the override is explicit when cool-down is enabled.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design — requesting changes on two correctness issues.
📋 Key Themes & Findings
Issues
-
Author-date fallback bypasses cooldown (
update_workflows.go:528) — cherry-picked commits carry an old author date that can be years behind the committer date, silently defeating the 3-day window. Prefer committer date or error when absent. -
imageSetnormalization mismatch (update_container_pins.go:113) —imageSetbuilt from rawimages(which may contain@sha256:suffixes) is passed toPruneStaleContainerPins, whose keys are base-tag-only. A digest-pinned lock file will cause valid pins to be pruned incorrectly. -
effectiveCommitCoolDowninterface surprise (update_cooldown.go:116) — the function accepts any duration but only<= 0is meaningful; every positive value maps to the fixed 3-day constant, which is unintuitive. The signature implies the caller's value is respected.
Positive Highlights
- ✅ Clean
workflowUpdateDepsinjection for testability — makes branch/default-branch paths fully unit-testable without network calls. - ✅ Solid test coverage for the new cooldown paths (
TestResolveLatestRef_BranchCommitCooldown,TestResolveLatestRef_CommitCooldown). - ✅ Correct API endpoint switch from
/branches/{branch}to/commits/{branch}to retrieve commit metadata. - ✅ Lockfile normalization in
collectImagesFromLockFilesis the right place to strip digest suffixes before collecting image names.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 54.4 AIC · ⌖ 4.93 AIC · ⊞ 6.7K
Comment /matt to run again
| getRepoDefaultBranch func(ctx context.Context, repo string) (string, error) | ||
| getLatestBranchCommit func(ctx context.Context, repo, branch string) (latestBranchCommitInfo, error) | ||
| } | ||
|
|
There was a problem hiding this comment.
[/codebase-design] Silently falling back to author date can bypass the cooldown for cherry-picks — the author date may be years older than the committer date.
💡 Suggested fix
Return an error when committer.date is absent, or document the fallback contract with a test case where author date ≪ committer date:
dateStr := result.Commit.Committer.Date
if dateStr == "" {
return latestBranchCommitInfo{}, fmt.Errorf("empty committer date returned for branch %s", branch)
}@copilot please address this.
There was a problem hiding this comment.
Addressed in d9d7bb3: removed author-date fallback for commit cooldown. Resolution now uses committer.date only; when it is absent we return SHA-only info (no hard failure) and added test coverage for the missing-committer-date case.
| base, _, _ := strings.Cut(img, "@sha256:") | ||
| imageSet[base] = struct { | ||
| }{} | ||
| imageSet[img] = struct{}{} |
There was a problem hiding this comment.
[/diagnosing-bugs] imageSet is built from raw (un-normalized) images entries but PruneStaleContainerPins uses it to match against cache keys that are stored without the @sha256: suffix — if any image in collectImagesFromLockFiles still returns a digest-pinned ref this prune step will never match and stale pins will accumulate.
💡 Suggested fix
Normalize here exactly like collectImagesFromLockFiles does:
for _, img := range images {
base, _, _ := strings.Cut(img, "`@sha256`:")
imageSet[base] = struct{}{}
}Add a unit test with a pre-seeded cache entry whose key matches the base tag, then call updateContainerPins with a lock file that only references the digest-pinned form, and assert the entry is retained (not pruned).
@copilot please address this.
| if coolDown <= 0 { | ||
| return 0 | ||
| } | ||
| return commitRefCoolDown |
There was a problem hiding this comment.
[/codebase-design] effectiveCommitCoolDown silently ignores the user-supplied --cool-down value and always returns the hard-coded 3-day constant — this is a surprising interface: the function signature implies the input duration affects the result, but only <= 0 is significant.
💡 Suggestion
Either rename to commitCoolDownPeriod(disabled bool) to make the binary nature explicit, or document in coolDownFlagUsage that commit cooldowns are always fixed at 3 days regardless of the flag value. A test asserting the user passes 24h but gets 72h would make this contract visible.
@copilot please address this.
There was a problem hiding this comment.
Addressed in d9d7bb3: documented the fixed 3-day commit cooldown contract in coolDownFlagUsage and kept the existing test that asserts nonzero user cooldown values map to the fixed commit cooldown.
|
@copilot run pr-finisher skill |
…ted repos Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
gh aw updatecould leave container digest pins stale after refs moved, and it would immediately adopt fresh branch/head commits even when the resolved version and commit were too new. This change makes update resolution keep container pins aligned with updated refs and applies a 3-day cooldown to moving commit-based workflow sources.Commit-based workflow source cooldown
Container digest refresh during update
gh aw updatenow re-resolves container digests for images already present inactions-lock.json, instead of treating existing pins as always current.pinned_imageentries synchronized when workflow/action updates move a container tag to a new manifest digest.Lockfile image normalization
image:tag@sha256:...and cache keys stored asimage:tag.Test coverage
Example of the normalized pin flow: