Refresh container pins before daily image scans - #51083
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| compileCmd.Flags().BoolP("force", "f", false, "Force overwrite of existing dependency files (only applies when --dependabot is set; e.g., dependabot.yml)") | ||
| compileCmd.Flags().Bool("refresh-stop-time", false, "Force regeneration of stop-after times instead of preserving existing values from lock files") | ||
| compileCmd.Flags().Bool("force-refresh-action-pins", false, "Force refresh of action pins by clearing the cache and resolving all action SHAs from GitHub API") | ||
| compileCmd.Flags().Bool("refresh-container-pins", false, "Refresh existing container image digest pins before compiling") |
There was a problem hiding this comment.
@copilot align name with force refresh action pins, force prefix
There was a problem hiding this comment.
Addressed in 654bc81 by renaming the compile flag to --force-refresh-container-pins and updating the workflow, tests, docs, and changeset references.
|
No ADR enforcement needed: PR #51083 does not have the 'implementation' label and has only 12 new lines of code in business logic directories (threshold: 100). |
|
|
|
test |
|
|
There was a problem hiding this comment.
Pull request overview
Adds container-pin refresh support so daily security scans target current mutable image tags rather than stale cached digests.
Changes:
- Adds and propagates
compile --refresh-container-pins. - Refreshes pins before daily Syft, Grype, and Grant scans.
- Updates generated pin caches and workflow lock files.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/compile_config.go |
Adds refresh configuration. |
pkg/cli/compile_orchestrator.go |
Runs pin refresh before compilation. |
cmd/gh-aw/main.go |
Registers and propagates the CLI flag. |
cmd/gh-aw/compile_flags_test.go |
Tests flag registration and propagation. |
.github/workflows/daily-squid-image-scan.md |
Enables refresh for daily scans. |
.github/workflows/daily-squid-image-scan.lock.yml |
Recompiles the scan workflow. |
.github/workflows/mcp-inspector.lock.yml |
Embeds the refreshed Python digest. |
.github/aw/actions-lock.json |
Refreshes and prunes cached pins. |
pkg/actionpins/data/action_pins.json |
Synchronizes embedded action-pin data. |
pkg/workflow/data/action_pins.json |
Synchronizes workflow pin data. |
.changeset/patch-refresh-container-pins-before-scans.md |
Records the patch release change. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Balanced
| if config.RefreshContainerPins && !config.NoEmit { | ||
| compileOrchestratorLog.Print("Refreshing container image digest pins before compilation") | ||
| if _, err := updateContainerPins(ctx, defaultContainerPinUpdateDeps(), workflowDir, config.Verbose, containerPinUpdateOptions{refreshExisting: true}); err != nil { | ||
| return nil, fmt.Errorf("failed to refresh container pins: %w", err) |
| compileCmd.Flags().BoolP("force", "f", false, "Force overwrite of existing dependency files (only applies when --dependabot is set; e.g., dependabot.yml)") | ||
| compileCmd.Flags().Bool("refresh-stop-time", false, "Force regeneration of stop-after times instead of preserving existing values from lock files") | ||
| compileCmd.Flags().Bool("force-refresh-action-pins", false, "Force refresh of action pins by clearing the cache and resolving all action SHAs from GitHub API") | ||
| compileCmd.Flags().Bool("refresh-container-pins", false, "Refresh existing container image digest pins before compiling") |
There was a problem hiding this comment.
The --refresh-container-pins flag is correctly wired end-to-end (CLI flag → compileCmdOptions → CompileConfig.RefreshContainerPins → orchestrator). The !config.NoEmit guard correctly skips the refresh in dry-run mode. Tests cover flag existence, default value, and propagation. Lock files and action-pin JSON files are consistently updated. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14.2 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — two targeted observations; no blocking issues found.
📋 Key Themes
Issues
- Naming inconsistency (
cmd/gh-aw/main.goline 771):--refresh-container-pinsbreaks theforce-prefix pattern established by--force-refresh-action-pins. @pelikhan already flagged this inline. - Orchestrator error path untested (
pkg/cli/compile_orchestrator.goline 115): theRefreshContainerPinsbranch callsupdateContainerPinsbut there is no test that exercises the error-return path. Propagation relies solely on the struct-field tests.
Positive Highlights
- ✅ Clean end-to-end wiring: config struct → flag registration → orchestrator guard, all consistent
- ✅
!config.NoEmitguard correctly skips the network call in dry-run mode - ✅ Two focused unit tests added covering both flag presence and config propagation
- ✅ Changeset entry is accurate and concise
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 36.5 AIC · ⊞ 7.1K
Comment /matt to run again
| compileCmd.Flags().BoolP("force", "f", false, "Force overwrite of existing dependency files (only applies when --dependabot is set; e.g., dependabot.yml)") | ||
| compileCmd.Flags().Bool("refresh-stop-time", false, "Force regeneration of stop-after times instead of preserving existing values from lock files") | ||
| compileCmd.Flags().Bool("force-refresh-action-pins", false, "Force refresh of action pins by clearing the cache and resolving all action SHAs from GitHub API") | ||
| compileCmd.Flags().Bool("refresh-container-pins", false, "Refresh existing container image digest pins before compiling") |
There was a problem hiding this comment.
[/codebase-design] The flag name --refresh-container-pins does not follow the force- prefix convention used by the adjacent --force-refresh-action-pins flag — users who know --force-refresh-action-pins will expect --force-refresh-container-pins.
💡 Suggested rename
compileCmd.Flags().Bool("force-refresh-container-pins", false, "Force refresh of container image digest pins before compiling")Also rename struct fields: forceRefreshContainerPins in compileCmdOptions and ForceRefreshContainerPins in CompileConfig, mirroring ForceRefreshActionPins.
@copilot please address this.
| compileOrchestratorLog.Print("Refreshing container image digest pins before compilation") | ||
| if _, err := updateContainerPins(ctx, defaultContainerPinUpdateDeps(), workflowDir, config.Verbose, containerPinUpdateOptions{refreshExisting: true}); err != nil { | ||
| return nil, fmt.Errorf("failed to refresh container pins: %w", err) | ||
| } |
There was a problem hiding this comment.
[/tdd] The refresh path in the orchestrator has no integration test — only flag existence and config propagation are tested. If updateContainerPins returns an error (e.g. network failure on the runner), compile silently fails with no regression guard.
💡 What to add
Add a test in pkg/cli/ that sets RefreshContainerPins: true in CompileConfig, stubs or injects the updateContainerPins dependency to return an error, and asserts that CompileWorkflows surfaces the error rather than silently swallowing it. This would have caught any future breakage in the error-propagation path.
@copilot please address this.
|
test body
|
There was a problem hiding this comment.
🧪 Test Quality Sentinel Report
✅ Test Quality Score: 90/100 — Excellent
Analyzed 2 test(s): 2 design, 0 implementation, 0 violation(s).
📊 Metrics (2 tests)
| Metric | Value |
|---|---|
| Analyzed | 2 (Go: 2, JS: 0) |
| ✅ Design | 2 (100%) |
| 0 (0%) | |
| Edge/error coverage | 2 (100%) |
| Duplicate clusters | 0 |
| Inflation | Yes (~3:1; expected for multi-flag coverage file) |
| 🚨 Violations | 0 |
| Test | File | Classification | Issues |
|---|---|---|---|
TestCompileCommandShortFlags |
cmd/gh-aw/compile_flags_test.go:7 |
design_test / behavioral_contract / high_value |
None |
TestCompileOptionsPropagateRefreshContainerPins |
cmd/gh-aw/compile_flags_test.go:42 |
design_test / behavioral_contract / high_value |
None |
Test Analysis
TestCompileCommandShortFlags: Verifies CLI flag contract — --force/-f, --logical-repo/-l, --grant default, and the new --refresh-container-pins flag with its default value (false). Each check guards against accidental flag renames or default changes. Error paths covered via t.Fatal on nil flag lookup. Design contract: high-value.
TestCompileOptionsPropagateRefreshContainerPins: Enforces that compileCmdOptions.refreshContainerPins propagates to CompileConfig.RefreshContainerPins. Prevents silent feature disablement of the new refresh behaviour. Design contract: high-value.
Inflation note: 15 test additions vs ~5 production additions in main.go gives ~3:1. Not a hard violation — the test covers multiple pre-existing flags alongside the new one.
Score Breakdown
design_ratio = 2/2 → 40/40
edge_coverage = 2/2 → 30/30
duplicate_score = 20/20
inflation_score = 0/10 (ratio > 2:1)
──────────────────────────────
Total = 90/100
Verdict
✅ passed. 0% implementation tests (threshold: 30%). No violations. Build tag
(go/redacted):build !integrationpresent. No mock libraries used.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: Request changes — the refresh feature has real correctness/reliability gaps that undercut its stated purpose.
The intent (re-resolve container digests before daily scans so mutable tags don't hide patched CVEs) is sound, but the implementation has an unscoped, expensive refresh, a failure path that gets masked by || true in the very workflow this PR targets, and a silent no-op interaction with --no-emit. Test coverage for the new orchestrator logic itself is effectively absent.
Themes
- Unscoped refresh cost:
updateContainerPinswithrefreshExisting: truere-resolves every cached image in every lock file underworkflowDir, not just images relevant to the current compile target — turning every--refresh-container-pinscompile into a full-repo network refresh. - Masked failure in the primary consumer: The daily-scan workflow's
compile ... || trueswallows the new hard-fail error path (failed to refresh container pins: %w), so exactly the workflow this feature was built for can silently proceed on stale pins if the refresh itself fails. - Silent NoEmit interaction:
RefreshContainerPins && !NoEmithas no logging when skipped, making the flag's actual effect undiscoverable in dry-run/validate contexts. - Test gap: New tests only assert CLI flag → config field wiring; the actual conditional-refresh/error-propagation logic added to
compile_orchestrator.gois untested.
🔎 Code quality review by PR Code Quality Reviewer · auto · 114.2 AIC · ⊞ 7.8K
Comment /review to run again
| // Create and configure compiler | ||
| if config.RefreshContainerPins && !config.NoEmit { | ||
| compileOrchestratorLog.Print("Refreshing container image digest pins before compilation") | ||
| if _, err := updateContainerPins(ctx, defaultContainerPinUpdateDeps(), workflowDir, config.Verbose, containerPinUpdateOptions{refreshExisting: true}); err != nil { |
There was a problem hiding this comment.
This refresh resolves digests for every container image referenced in workflowDir's lock files, not just the ones relevant to the workflow(s) being compiled — every compile --refresh-container-pins invocation now does a full network round-trip (docker buildx/crane/pull) per cached image.
💡 Unscoped network refresh on every compile call
updateContainerPins calls collectImagesFromLockFiles(workflowDir), which scans all *.lock.yml files in the directory, and passes refreshExisting: true, which bypasses the cache-skip check in the loop:
if hasExistingPin && existingPin.Digest != "" && !opts.refreshExisting {
// skip — never reached when refreshExisting is true
}So the daily image-scan workflow (and any future caller passing this flag) triggers digest resolution attempts for every image in every compiled workflow in the repo, regardless of whether config.MarkdownFiles targets a subset. That is a large, unbounded amount of external network calls per compile invocation, with no scoping to the files actually being (re)compiled, and no way to opt into a narrower refresh.
Consider scoping the refresh to only the images referenced by config.MarkdownFiles (or documenting/accepting the full-repo cost explicitly), and/or adding a timeout/parallelism cap given the sequential loop with three fallback strategies each per image.
| output_dir="/tmp/gh-aw/agent/image-scan" | ||
| mkdir -p "$output_dir" | ||
| "$GITHUB_WORKSPACE/gh-aw" compile --syft --grype --grant 2>&1 | tee "$output_dir/compile-output.txt" || true | ||
| "$GITHUB_WORKSPACE/gh-aw" compile --refresh-container-pins --syft --grype --grant 2>&1 | tee "$output_dir/compile-output.txt" || true |
There was a problem hiding this comment.
The || true at the end of this pipeline swallows the new hard failure path in compile_orchestrator.go (return nil, fmt.Errorf("failed to refresh container pins: %w", err)), so a broken pin refresh is silently ignored by the scan workflow.
💡 Refresh failures are masked by `|| true`
With --refresh-container-pins, if updateContainerPins returns an error (e.g. collectImagesFromLockFiles failing, or actionCache.Load()/Save() failing), CompileWorkflows now aborts the whole compile with an error. But this step still ends in || true, so the workflow proceeds as if compilation succeeded — with the security scan then running against a stale compile-output.txt/lock files, defeating the entire purpose of this PR (ensuring pins are current before scanning).
Consider checking the tee'd output or the underlying command's exit status for the container-pin-refresh failure explicitly before falling back to || true, or fail the job when the refresh step specifically errors.
| } | ||
|
|
||
| // Create and configure compiler | ||
| if config.RefreshContainerPins && !config.NoEmit { |
There was a problem hiding this comment.
Requesting --refresh-container-pins together with --no-emit silently skips the refresh with zero feedback to the user.
💡 Silent no-op when NoEmit is set
if config.RefreshContainerPins && !config.NoEmit {When a user runs gh aw compile --refresh-container-pins --no-emit (e.g. in a validate/dry-run CI job), this condition is false, so the refresh never runs — but there is no warning or log message explaining that the flag was ignored. The user has no way to tell whether pins were actually refreshed or the flag was silently dropped, which is confusing given the flag exists specifically to be explicit and predictable for the daily scan workflow.
Add a verbose/warning log (similar to the shellcheckEnabled skip-with-NoEmit handling a few lines above) so the behavior is discoverable.
| t.Fatalf("expected --grant default to be false, got %s", grantFlag.DefValue) | ||
| } | ||
|
|
||
| refreshContainerPinsFlag := compileCmd.Flags().Lookup("refresh-container-pins") |
There was a problem hiding this comment.
The new test only verifies flag-to-config wiring; it does not cover the actual orchestrator behavior this PR introduces (the conditional refresh call, its interaction with NoEmit, or error propagation).
💡 No test coverage for the core new behavior
TestCompileOptionsPropagateRefreshContainerPins confirms compileCmdOptions{refreshContainerPins: true} maps to CompileConfig.RefreshContainerPins, which is trivial plumbing. The actual feature — the if config.RefreshContainerPins && !config.NoEmit { updateContainerPins(...) } branch added in pkg/cli/compile_orchestrator.go — has no direct test:
- No test that
updateContainerPinsis invoked withrefreshExisting: truewhen the flag is set. - No test that the branch is skipped when
NoEmitis true. - No test that a
updateContainerPinserror surfaces as aCompileWorkflowserror (failed to refresh container pins: %w).
Given compile_orchestrator_test.go/update_container_pins_test.go already have patterns for injecting fake containerPinUpdateDeps, this should be straightforward to add and would catch regressions in the new conditional logic.
|
@copilot Please address the current review feedback, refresh the branch if GitHub can update it cleanly, and run the Open review items to address (newest first):
Branch refresh was requested. Run context: https://github.com/github/gh-aw/actions/runs/31188101705
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot Please address the current review feedback, refresh the branch if GitHub can update it cleanly, and run the Open review items to address (newest first):
Branch refresh was requested. Run context: https://github.com/github/gh-aw/actions/runs/31200043159
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. I ran the pr-finisher workflow and pushed follow-up fixes in |
|
🎉 This pull request is included in a new release. Release: |
The daily container image security scan could keep scanning stale digest-pinned images even after mutable upstream tags were rebuilt with patched packages. This left real critical CVEs visible in the gate until the local container pin cache was refreshed.
Compile support
gh aw compile --refresh-container-pins.Daily scan workflow
python:alpinedigest and synced embedded action-pin data.Run context: https://github.com/github/gh-aw/actions/runs/31188101705> Generated by 👨🍳 PR Sous Chef · gpt54 · 14.3 AIC · ⊞ 5.9K · ◷