Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions docs/adr/47990-refresh-container-digests-and-commit-cooldown.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
# ADR-47990: Refresh Container Digests on Update and Enforce Commit-Age Cooldown

**Date**: 2026-07-25
**Status**: Draft
**Deciders**: Unknown

---

### Context

The `gh aw update` command manages two categories of pinned references: container image digest pins stored in `actions-lock.json`, and workflow source refs (branch, commit SHA, or semantic-version tag). Two independent problems were discovered:

1. **Stale container pins**: Container image tags (e.g., `image:tag`) can move to a new manifest digest after the initial pin is written. The previous implementation treated existing `pinned_image` entries as always current and skipped re-resolving them, leaving lock files referencing outdated digests when upstream images were republished.

2. **Unsafe moving-ref adoption**: Branch-tracked and commit-SHA-tracked workflow sources had no commit-age gate. A freshly pushed commit (seconds or minutes old) could be adopted immediately during an update run, creating operational risk analogous to the release cooldown problem already solved for tag-based sources.

Additionally, lock files compiled with digest-pinned references (`image:tag@sha256:...`) were not being normalized back to their base image ref before cache key lookups, causing reconciliation mismatches.

### Decision

We decided to (a) always re-resolve container digests for images already present in `actions-lock.json` during `gh aw update` runs (controlled by a new `refreshExisting` option on the internal `updateContainerPins` function), and (b) enforce a fixed 3-day commit-age cooldown on branch-tracked and commit-SHA-tracked workflow sources, gated by the same cooldown flag that controls release cooldowns. The commit cooldown is non-configurable (always 3 days) except when the user explicitly disables cooldowns entirely. Digest-pinned lock file image refs are normalized to their base tag before cache reconciliation to eliminate key mismatches.

### Alternatives Considered

#### Alternative 1: Manual Digest Refresh (opt-in flag)

Expose a `--refresh-pins` flag so users can explicitly request container digest re-resolution rather than doing it on every update run. This avoids extra `docker manifest inspect` (or equivalent) calls in normal update flows and gives users explicit control.

Not chosen because the goal of `gh aw update` is to keep the lock file fully synchronized. Silent staleness — where a container digest is out of date but nothing prompts the user to notice — is the worse failure mode. The Docker call is already non-fatal (fails gracefully when Docker is unavailable), so the cost of always refreshing is low.

#### Alternative 2: Time-Based Cache Invalidation for Existing Pins

Instead of always re-resolving, introduce a TTL (e.g., 24 hours) on cached container pins: refresh a pin only if it was last resolved more than TTL ago, storing a timestamp alongside the digest.

Not chosen because it adds persistent timestamp state to `actions-lock.json`, complicating the lock file schema and migration. It also creates a window of guaranteed staleness equal to the TTL, whereas always re-resolving on update provides the freshest state at no additional schema cost.

#### Alternative 3: Per-Ref Commit Cooldown Configured by the User

Allow the commit cooldown to be configured separately from the release cooldown (e.g., `--commit-cooldown 2d`), giving fine-grained control over branch vs. release cadence.

Not chosen because the immediate need is to close a safety gap (too-fresh commits being adopted), and a fixed 3-day constant is already well-understood from the existing release cooldown precedent. Adding a second configurable flag increases UX complexity without clear current demand; this can be revisited if operators need different thresholds.

### Consequences

#### Positive
- Container pins in `actions-lock.json` stay synchronized with upstream image manifest changes across update runs, removing a category of silent staleness.
- Branch-tracked and commit-SHA-tracked workflow sources are subject to the same stability guarantee as tag-based releases: at least 3 days must elapse before a moving ref is adopted.
- Digest-pinned image refs in lock files are normalized before cache key lookup, eliminating reconciliation mismatches between `image:tag@sha256:...` and `image:tag` keys.
- Dependency-injected `containerPinUpdateDeps` and extended `workflowUpdateDeps` structs make both behaviors unit-testable without Docker or live GitHub API calls.

#### Negative
- Every `gh aw update` run now makes one additional `docker manifest inspect` (or equivalent) call per pinned container image, even when the digest has not changed. In environments where Docker is unavailable this is already non-fatal, but it does add latency and noise in verbose output.
- The commit-age cooldown is not user-configurable independently of the release cooldown. Users who want aggressive commit tracking (e.g., CI environments where they own both sides of the workflow repo) cannot lower the 3-day window without disabling all cooldowns.
- The `getLatestBranchCommitSHA` API call was changed to the `/repos/{repo}/commits/{ref}` endpoint (which returns richer JSON) rather than `/repos/{repo}/branches/{branch}` with a `--jq .commit.sha` filter. This increases response payload size and may surface different error shapes for non-existent refs.

#### Neutral
- The public `UpdateContainerPins` function retains its existing signature and defaults (`refreshExisting: false`) for callers outside of `RunUpdateWorkflows`, preserving backward compatibility for any other call sites.
- Branch-commit cache (`branchCommitCache`) now stores `latestBranchCommitInfo` structs (SHA + CommittedAt) instead of plain strings, which is a breaking change to the in-memory cache type — harmless since the cache is process-scoped and cleared at the start of each update run.

---

*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*
2 changes: 1 addition & 1 deletion pkg/cli/update_command.go
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ func RunUpdateWorkflows(ctx context.Context, opts UpdateWorkflowsOptions) error
// already reflect the current AWF version; stale pins from superseded versions are pruned
// and new versions are resolved in a single pass.
updateLog.Print("Updating container image digest pins")
newContainerPins, err := UpdateContainerPins(ctx, opts.WorkflowsDir, opts.Verbose)
newContainerPins, err := updateContainerPins(ctx, defaultContainerPinUpdateDeps(), opts.WorkflowsDir, opts.Verbose, containerPinUpdateOptions{refreshExisting: true})
if err != nil {
// Non-fatal: Docker may not be available in all environments.
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Warning: Failed to update container pins: %v", err)))
Expand Down
57 changes: 32 additions & 25 deletions pkg/cli/update_container_pins.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,18 @@ var dockerImagesArgPattern = regexp.MustCompile(`download_docker_images\.sh"?\s+
// in practice and are not expected here.
var buildxDigestPattern = regexp.MustCompile(`(?m)^Digest:\s+(sha256:[a-f0-9]{64})`)

type containerPinUpdateDeps struct {
fetchDigest func(ctx context.Context, image string, verbose bool) (string, error)
}

type containerPinUpdateOptions struct {
refreshExisting bool
}

func defaultContainerPinUpdateDeps() containerPinUpdateDeps {
return containerPinUpdateDeps{fetchDigest: fetchContainerDigest}
}

// UpdateContainerPins resolves SHA-256 digests for all container images referenced in
// the compiled lock files under workflowDir and stores the pins in
// .github/aw/actions-lock.json.
Expand All @@ -57,6 +69,10 @@ var buildxDigestPattern = regexp.MustCompile(`(?m)^Digest:\s+(sha256:[a-f0-9]{64
// Returns true when new container pins were added and lock files should be
// recompiled to embed the digest references.
func UpdateContainerPins(ctx context.Context, workflowDir string, verbose bool) (bool, error) {
return updateContainerPins(ctx, defaultContainerPinUpdateDeps(), workflowDir, verbose, containerPinUpdateOptions{})
}

func updateContainerPins(ctx context.Context, deps containerPinUpdateDeps, workflowDir string, verbose bool, opts containerPinUpdateOptions) (bool, error) {
containerPinsLog.Print("Starting container pin update")

if verbose {
Expand Down Expand Up @@ -92,18 +108,10 @@ func UpdateContainerPins(ctx context.Context, workflowDir string, verbose bool)
}
}

// Build a set of base image tags (without @sha256: digest suffix) currently
// referenced in the compiled lock files so that stale entries (e.g. superseded
// AWF versions) can be pruned. Lock files that were previously compiled may
// already embed pinned references (image:tag@sha256:...), so we strip the
// digest before comparing against container pin keys, which always use the
// base tag as the key.
imageSet := make(map[string]struct {
}, len(images))
imageSet := make(map[string]struct{}, len(images))
for _, img := range images {
base, _, _ := strings.Cut(img, "@sha256:")
imageSet[base] = struct {
}{}
imageSet[base] = struct{}{}
}

// Remove any container pin entries that are no longer referenced by the
Expand All @@ -124,30 +132,30 @@ func UpdateContainerPins(ctx context.Context, workflowDir string, verbose bool)
var skippedImages []string

for _, image := range images {
// Images already containing @sha256: are immutably pinned — skip them.
if strings.Contains(image, "@sha256:") {
skippedImages = append(skippedImages, image)
continue
}

// Check if we already have a valid pin for this image in the cache.
if pin, ok := actionCache.GetContainerPin(image); ok && pin.Digest != "" {
existingPin, hasExistingPin := actionCache.GetContainerPin(image)
if hasExistingPin && existingPin.Digest != "" && !opts.refreshExisting {
if verbose {
fmt.Fprintln(os.Stderr, console.FormatVerboseMessage(fmt.Sprintf("%s already pinned: %s", image, pin.Digest)))
fmt.Fprintln(os.Stderr, console.FormatVerboseMessage(fmt.Sprintf("%s already pinned: %s", image, existingPin.Digest)))
}
skippedImages = append(skippedImages, image)
continue
}

// Attempt to resolve the digest without pulling.
digest, resolveErr := fetchContainerDigest(ctx, image, verbose)
digest, resolveErr := deps.fetchDigest(ctx, image, verbose)
if resolveErr != nil {
containerPinsLog.Printf("Failed to resolve digest for %s: %v", image, resolveErr)
failedImages = append(failedImages, imageFailure{image: image, reason: resolveErr.Error()})
continue
}

pinnedImage := image + "@" + digest
if hasExistingPin && existingPin.Digest == digest && existingPin.PinnedImage == pinnedImage {
if verbose {
fmt.Fprintln(os.Stderr, console.FormatVerboseMessage(fmt.Sprintf("%s already pinned: %s", image, digest)))
}
skippedImages = append(skippedImages, image)
continue
}
actionCache.SetContainerPin(image, digest, pinnedImage)
updatedImages = append(updatedImages, pinnedEntry{image: image, pinnedImage: pinnedImage})

Expand All @@ -160,7 +168,7 @@ func UpdateContainerPins(ctx context.Context, workflowDir string, verbose bool)
fmt.Fprintln(os.Stderr, "")

if len(updatedImages) > 0 {
fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Pinned %d container image(s):", len(updatedImages))))
fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Updated %d container image pin(s):", len(updatedImages))))
for _, entry := range updatedImages {
fmt.Fprintln(os.Stderr, console.FormatListItem(entry.pinnedImage))
}
Expand Down Expand Up @@ -231,9 +239,8 @@ func collectImagesFromLockFiles(workflowDir string) ([]string, error) {
continue
}
for img := range strings.FieldsSeq(matches[1]) {
if img != "" {
imageSet[img] = struct {
}{}
if base, _, _ := strings.Cut(img, "@sha256:"); base != "" {
imageSet[base] = struct{}{}
}
}
}
Expand Down
66 changes: 66 additions & 0 deletions pkg/cli/update_container_pins_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,20 @@ jobs:
"node:lts-alpine",
},
},
{
name: "digest pinned images normalized to base ref",
lockFileContent: `name: test
jobs:
setup:
steps:
- name: Download container images
run: bash "${RUNNER_TEMP}/gh-aw/actions/download_docker_images.sh" ghcr.io/github/github-mcp-server:v0.32.0@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa node:lts-alpine@sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
`,
expectedImages: []string{
"ghcr.io/github/github-mcp-server:v0.32.0",
"node:lts-alpine",
},
},
{
name: "no docker images in lock file",
lockFileContent: `name: test
Expand Down Expand Up @@ -312,3 +326,55 @@ jobs:
assert.NotContains(t, string(data), "0.27.0", "stale version should not appear in saved file")
assert.Contains(t, string(data), "0.27.2", "current version should be in saved file")
}

func TestUpdateContainerPins_RefreshExistingPin(t *testing.T) {
tmpDir := t.TempDir()

actionsLockDir := filepath.Join(tmpDir, ".github", "aw")
require.NoError(t, os.MkdirAll(actionsLockDir, 0755))
actionsLockContent := `{
"entries": {},
"containers": {
"ghcr.io/github/github-mcp-server:v0.32.0": {
"image": "ghcr.io/github/github-mcp-server:v0.32.0",
"digest": "sha256:1111111111111111111111111111111111111111111111111111111111111111",
"pinned_image": "ghcr.io/github/github-mcp-server:v0.32.0@sha256:1111111111111111111111111111111111111111111111111111111111111111"
}
}
}
`
require.NoError(t, os.WriteFile(filepath.Join(actionsLockDir, "actions-lock.json"), []byte(actionsLockContent), 0644))

workflowsDir := filepath.Join(tmpDir, ".github", "workflows")
require.NoError(t, os.MkdirAll(workflowsDir, 0755))
lockFileContent := `name: test
jobs:
setup:
steps:
- name: Download container images
run: bash "${RUNNER_TEMP}/gh-aw/actions/download_docker_images.sh" ghcr.io/github/github-mcp-server:v0.32.0@sha256:1111111111111111111111111111111111111111111111111111111111111111
`
require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "test.lock.yml"), []byte(lockFileContent), 0644))

originalDir, _ := os.Getwd()
defer os.Chdir(originalDir) //nolint:errcheck
require.NoError(t, os.Chdir(tmpDir))

deps := defaultContainerPinUpdateDeps()
const refreshedDigest = "sha256:2222222222222222222222222222222222222222222222222222222222222222"
deps.fetchDigest = func(_ context.Context, image string, verbose bool) (string, error) {
require.Equal(t, "ghcr.io/github/github-mcp-server:v0.32.0", image)
return refreshedDigest, nil
}

changed, err := updateContainerPins(context.Background(), deps, workflowsDir, false, containerPinUpdateOptions{refreshExisting: true})
require.NoError(t, err)
assert.True(t, changed, "refreshing an existing pin should report a change")

cache := workflow.NewActionCache(tmpDir)
require.NoError(t, cache.Load())
pin, ok := cache.GetContainerPin("ghcr.io/github/github-mcp-server:v0.32.0")
require.True(t, ok)
assert.Equal(t, refreshedDigest, pin.Digest)
assert.Equal(t, "ghcr.io/github/github-mcp-server:v0.32.0@"+refreshedDigest, pin.PinnedImage)
}
32 changes: 31 additions & 1 deletion pkg/cli/update_cooldown.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ import (

var cooldownLog = logger.New("cli:update_cooldown")

const coolDownFlagUsage = "Cool-down period before applying a new release (e.g., 7d, 24h, 0 to disable). Does not apply to actions/* or github/* repositories"
const coolDownFlagUsage = "Cool-down period before applying a new release (e.g., 7d, 24h, 0 to disable). Does not apply to actions/* or github/* repositories. Commit-tracked sources use a fixed 3d cooldown when cool-down is enabled."
const commitRefCoolDown = 3 * 24 * time.Hour

// parseCoolDownFlag parses a cooldown duration string.
// Accepts day-suffix notation ("7d") or Go duration format ("168h", "0").
Expand Down Expand Up @@ -105,6 +106,35 @@ func checkReleaseCoolDownWithDate(repo, tag string, publishedAt time.Time, coolD
return coolDownCheckResult{InCoolDown: true, Message: msg}
}

// effectiveCommitCoolDown returns the commit-age cooldown used for branch- and
// commit-tracked workflow sources. Commit cooldown defaults to 3 days and can
// be disabled only when the user explicitly disables cooldowns entirely.
func effectiveCommitCoolDown(coolDown time.Duration) time.Duration {
if coolDown <= 0 {
return 0
}
return commitRefCoolDown

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Using min(coolDown, commitRefCoolDown) so the user can shorten but not eliminate the minimum safety window, or
  2. Documenting the override prominently in coolDownFlagUsage so 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)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// checkCommitCoolDownWithDate checks whether a moving branch/default-branch
// commit is old enough to adopt during update resolution.
func checkCommitCoolDownWithDate(repo, ref string, committedAt time.Time, coolDown time.Duration) coolDownCheckResult {
if coolDown <= 0 {
return coolDownCheckResult{}
}
age := max(time.Since(committedAt), 0)
if age >= coolDown {
return coolDownCheckResult{}
}
remaining := coolDown - age
ageStr := formatCoolDownDuration(age)
remainingStr := formatCoolDownDuration(remaining)
periodStr := formatCoolDownDuration(coolDown)
msg := fmt.Sprintf("%s@%s latest commit is %s old and needs to cool down (%s remaining out of %s cooldown period)",
repo, ref, ageStr, remainingStr, periodStr)
return coolDownCheckResult{InCoolDown: true, Message: msg}
}

// checkReleaseCoolDown returns a result indicating whether a release tag is
// within the cooldown window and should be skipped.
// The result always includes the fetched PublishedAt date (when available) so that
Expand Down
35 changes: 35 additions & 0 deletions pkg/cli/update_cooldown_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -230,3 +230,38 @@ func TestFormatCoolDownDuration(t *testing.T) {
})
}
}

func TestEffectiveCommitCoolDown(t *testing.T) {
tests := []struct {
name string
input time.Duration
expected time.Duration
}{
{name: "disabled when zero", input: 0, expected: 0},
{name: "disabled when negative", input: -1 * time.Hour, expected: 0},
{name: "fixed three day cooldown", input: 7 * 24 * time.Hour, expected: 3 * 24 * time.Hour},
{name: "explicit shorter release cooldown still uses three days", input: 24 * time.Hour, expected: 3 * 24 * time.Hour},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.Equal(t, tt.expected, effectiveCommitCoolDown(tt.input))
})
}
}

func TestCheckCommitCoolDownWithDate(t *testing.T) {
committedAt := time.Now().Add(-2 * 24 * time.Hour)
result := checkCommitCoolDownWithDate("owner/repo", "main", committedAt, 3*24*time.Hour)
assert.True(t, result.InCoolDown, "commit 2d old should be in cooldown with 3d window")
assert.Contains(t, result.Message, "owner/repo")
assert.Contains(t, result.Message, "main")
assert.Contains(t, result.Message, "cool down")
}

func TestCheckCommitCoolDownWithDate_NotInCoolDown(t *testing.T) {
committedAt := time.Now().Add(-4 * 24 * time.Hour)
result := checkCommitCoolDownWithDate("owner/repo", "main", committedAt, 3*24*time.Hour)
assert.False(t, result.InCoolDown, "commit 4d old should not be in cooldown with 3d window")
assert.Empty(t, result.Message)
}
11 changes: 6 additions & 5 deletions pkg/cli/update_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -440,13 +440,14 @@ func TestGetRepoDefaultBranch_Integration(t *testing.T) {
assert.Equal(t, "main", branch, "actions/checkout default branch should be 'main'")
}

// TestGetLatestBranchCommitSHA_Integration verifies fetching the latest commit SHA for a branch.
func TestGetLatestBranchCommitSHA_Integration(t *testing.T) {
// TestGetLatestBranchCommitInfo_Integration verifies fetching the latest commit info for a branch.
func TestGetLatestBranchCommitInfo_Integration(t *testing.T) {
skipWithoutGitHubAuth(t)

sha, err := getLatestBranchCommitSHA(context.Background(), "actions/checkout", "main")
require.NoError(t, err, "Should fetch latest commit SHA for actions/checkout main branch")
assert.True(t, IsCommitSHA(sha), "Result should be a 40-char commit SHA, got: %s", sha)
info, err := getLatestBranchCommitInfo(context.Background(), "actions/checkout", "main")
require.NoError(t, err, "Should fetch latest commit info for actions/checkout main branch")
assert.True(t, IsCommitSHA(info.SHA), "Result should be a 40-char commit SHA, got: %s", info.SHA)
assert.False(t, info.CommittedAt.IsZero(), "CommittedAt should be populated")
}

// --- Org Dry-Run Integration Tests ---
Expand Down
Loading
Loading