Repository navigation
Add frontmatter policy for local-only Docker image validation - #66284
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Explicit always can incorrectly inherit local-only mode, and the new shell regression suite is not registered with the standard test target.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Adds an opt-in local-only Docker image policy for generated agent and detection jobs.
Changes:
- Adds frontmatter schema and compiler propagation.
- Implements fail-closed local image validation and aliasing.
- Adds tests and usage documentation.
| File | Description |
|---|---|
pkg/workflow/threat_detection_job_runtime_test.go |
Tests detection policy propagation. |
pkg/workflow/threat_detection_external.go |
Propagates policy to detection jobs. |
pkg/workflow/mcp_setup_generator.go |
Applies policy to agent downloads. |
pkg/workflow/docker.go |
Emits the download-step policy environment. |
pkg/workflow/docker_predownload_test.go |
Tests compiled agent download steps. |
pkg/parser/schemas/main_workflow_schema.json |
Defines the frontmatter field. |
pkg/parser/schema_test.go |
Tests policy validation. |
docs/src/content/docs/reference/frontmatter.md |
Documents behavior and limitations. |
docs/src/content/docs/reference/frontmatter-full.md |
Adds the field to the full reference. |
actions/setup/sh/download_docker_images.sh |
Implements local-only validation. |
actions/setup/sh/download_docker_images_local_test.sh |
Adds mock-Docker regression cases. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| @@ -0,0 +1,105 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
Added download_docker_images_local_test.sh to make test-scripts; the target passes. Fixed in 61a19e3.
|
|
||
| // generateDownloadDockerImagesStep generates the step to download Docker images | ||
| func generateDownloadDockerImagesStep(yaml *strings.Builder, dockerImages []string) { | ||
| func generateDownloadDockerImagesStep(yaml *strings.Builder, dockerImages []string, localOnly bool) { |
There was a problem hiding this comment.
The generator now preserves unset, always, and never; explicit values are emitted as step-level overrides. Added compiler/runtime coverage for always. Fixed in 61a19e3.
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR #66284 does not have the implementation label (has_implementation_label=false) and has 85 new lines in business logic directories, below the 100-line threshold (requires_adr_by_default_volume=false). No custom .design-gate.yml present, so defaults applied.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on test wiring and policy semantics.
📋 Key Themes & Highlights
Key Themes
- Untested CI gap: the new
download_docker_images_local_test.shmock-Docker suite is never invoked bymake test-scripts, so the fail-closed validation logic it covers has no actual CI safety net (reinforces an existing unresolved review comment). - Doc/behavior mismatch: an untagged digest reference (
repo@sha256:...) defaults to a:latestalias, contradicting the new docs' claim that versioned references never create:latest; no test covers this path. - Ambiguous policy boolean: collapsing "unset" and "always" into one
localOnlybool means the interaction with a user-setenv: GH_AW_DOCKER_IMAGE_PULL_POLICYis unspecified and untested (reinforces an existing unresolved review comment).
Positive Highlights
- ✅ The fail-closed validation logic (reject non-digest-pinned, validate all images before tagging any, no retry/fallback to pull) is a solid, conservative design matching the stated security goal.
- ✅ Good coverage breadth in the new test script for conflicting digests, inspect failures, and tag failures — it just needs to be wired into CI.
- ✅ Documentation clearly calls out the limits of local metadata validation (no byte/provenance verification).
Fallback note: not applicable — pr-triage agent responded successfully with change_type: new_feature.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 49.2 AIC · ⌖ 14.7 AIC · ⊞ 10.1K
Comment /matt to run again
| exit 1 | ||
| fi | ||
| repository="${BASH_REMATCH[1]}" | ||
| tag="${BASH_REMATCH[7]:-latest}" |
There was a problem hiding this comment.
[/tdd] When a digest reference omits a tag (e.g. repo@sha256:...), tag defaults to latest, so the script tags it as repository:latest. This contradicts the new docs' claim that "a versioned reference creates only its versioned alias, not :latest" and silently produces a floating-tag alias in a mode whose entire purpose is pinned, non-floating references.
💡 Suggested fix
Either reject untagged digest references in never mode (require an explicit tag) or document/test the :latest fallback explicitly so behavior matches the stated guarantee. The current test suite (download_docker_images_local_test.sh) only exercises explicitly-tagged references, so this gap isn't caught.
@copilot please address this.
There was a problem hiding this comment.
Local-only mode now rejects digest references without an explicit tag, preventing a :latest alias; added a regression case. Fixed in 61a19e3.
| @@ -0,0 +1,105 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
[/tdd] This mock-Docker regression suite isn't invoked anywhere — make test-scripts enumerates shell tests explicitly (lines 495-505) and doesn't include this file, so CI can pass without ever exercising the fail-closed helper's validation logic.
💡 Suggested fix
Add a line to the test-scripts target in Makefile:
bash actions/setup/sh/download_docker_images_local_test.shWithout this, the comprehensive edge-case coverage (conflicting digests, inspect failures, tag failures, uppercase digest rejection, etc.) written in this PR provides no actual safety net.
@copilot please address this.
There was a problem hiding this comment.
Registered the mock-Docker suite in make test-scripts; the target passes. Fixed in 61a19e3.
|
|
||
| // generateDownloadDockerImagesStep generates the step to download Docker images | ||
| func generateDownloadDockerImagesStep(yaml *strings.Builder, dockerImages []string) { | ||
| func generateDownloadDockerImagesStep(yaml *strings.Builder, dockerImages []string, localOnly bool) { |
There was a problem hiding this comment.
[/codebase-design] localOnly collapses two semantically distinct states — "unset" and explicit always" — into the same boolean, and the generated step emits no override env var for either. This means a user who sets a workflow-level env: GH_AW_DOCKER_IMAGE_PULL_POLICY: never(expecting the compiler-generated step to inherit it) gets silently overridden back toalwaysbehavior only whendocker-image-pull-policyis explicitlynever` — the interface doesn't make the precedence/interaction between the new frontmatter key and the existing env var observable.
💡 Suggested improvement
Consider threading a tri-state policy (unset/always/never) through instead of a boolean, and document/test what happens if a user sets the env var directly via env: frontmatter alongside (or instead of) docker-image-pull-policy. Right now there's no test covering that interaction, so behavior there is undefined-by-implementation rather than by design.
@copilot please address this.
There was a problem hiding this comment.
The generator now preserves all three policy states and explicitly emits always/never, leaving unset absent so inherited env remains observable. Added coverage for both agent and detection steps. Fixed in 61a19e3.
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
There was a problem hiding this comment.
Request changes
Two merge blockers remain.
Blocking themes
docker-image-pull-policy: alwaysis not a real override today because the generated step only exportsGH_AW_DOCKER_IMAGE_PULL_POLICYfornever, so a workflow/job-level env value can still force local-only behavior even when the frontmatter is unset or explicitlyalways.- The new mock-Docker regression suite is orphaned from the repository's scripted test entrypoints, so CI can go green while the new fail-closed path rots untested.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 92.9 AIC · ⌖ 5.8 AIC · ⊞ 19.6K
Comment /review to run again
There was a problem hiding this comment.
Applied harden + audit review focus given this PR is primarily a fail-closed validation/bug-fix change (new local-only Docker pull policy, regex/digest validation, shell helper).
Found 3 blocking issues, all tied to the new docker-image-pull-policy: never local-only validation path:
- Untagged digest references silently fall back to a
:latestalias (actions/setup/sh/download_docker_images.sh:45), contradicting the PR's own docs claim that "a versioned reference creates only its versioned alias, not:latest" — undermining the floating-tag guarantee this mode exists to provide. - The new mock-Docker regression test (
download_docker_images_local_test.sh) isn't wired intomake test-scripts/CI, so none of its fail-closed edge-case coverage (conflicting digests, inspect/tag failures, uppercase-digest rejection) actually runs in CI. generateDownloadDockerImagesStep'slocalOnly boolcollapses "policy unset" and "policy explicitlyalways" into the same no-override behavior (pkg/workflow/docker.go:290), which can silently defeat a directly-setGH_AW_DOCKER_IMAGE_PULL_POLICY=neverenv var whenever frontmatter doesn't also saynever.
Details and suggested fixes are in the inline comments.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 141.9 AIC · ⌖ 13.6 AIC · ⊞ 8.1K
| exit 1 | ||
| fi | ||
| repository="${BASH_REMATCH[1]}" | ||
| tag="${BASH_REMATCH[7]:-latest}" |
There was a problem hiding this comment.
Untagged digest reference silently gets a :latest alias, contradicting the docs
When a never-mode image reference omits an explicit tag (e.g. repo@sha256:...), tag defaults to latest (line 45: tag="${BASH_REMATCH[7]:-latest}"), so the alias created is repository:latest.
This directly contradicts the new docs added in this PR (docs/src/content/docs/reference/frontmatter.md): "A versioned reference creates only its versioned alias, not :latest". In local-only mode — whose entire purpose is avoiding floating/mutable tags — an untagged digest silently produces a floating :latest alias.
The new test suite (download_docker_images_local_test.sh) only exercises explicitly-tagged references, so this gap isn't caught by the added coverage.
Suggested fix: either reject untagged digest references in never mode (require an explicit tag, matching the stated guarantee), or explicitly document/test that untagged references fall back to :latest so behavior matches what's promised.
@copilot please address this.
There was a problem hiding this comment.
Local-only mode now rejects digest references without an explicit tag, preventing a :latest alias; added a regression case. Fixed in 61a19e3.
| @@ -0,0 +1,105 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
New mock-Docker regression suite is never run in CI
download_docker_images_local_test.sh (covering conflicting digests, inspect failures, tag failures, uppercase-digest rejection, etc.) is not wired into any Makefile target. make test-scripts (Makefile lines 495–505) lists shell tests explicitly and does not include this file, and it's not referenced from test-all or any CI workflow either.
As a result, CI can pass without ever exercising the new fail-closed local-only validation logic — the comprehensive edge-case coverage in this file currently provides no safety net.
Suggested fix: add this script to the test-scripts target in the Makefile:
bash actions/setup/sh/download_docker_images_local_test.sh@copilot please address this.
There was a problem hiding this comment.
Registered the mock-Docker suite in make test-scripts; the target passes. Fixed in 61a19e3.
|
|
||
| // generateDownloadDockerImagesStep generates the step to download Docker images | ||
| func generateDownloadDockerImagesStep(yaml *strings.Builder, dockerImages []string) { | ||
| func generateDownloadDockerImagesStep(yaml *strings.Builder, dockerImages []string, localOnly bool) { |
There was a problem hiding this comment.
localOnly boolean collapses "unset" and explicit always" into one state, hiding a precedence bug with the env var
generateDownloadDockerImagesStep only emits env: GH_AW_DOCKER_IMAGE_PULL_POLICY: never when docker-image-pull-policy == "never"; otherwise it emits no override at all (both when the frontmatter key is absent and when it's explicitly "always").
Since the shell script reads GH_AW_DOCKER_IMAGE_PULL_POLICY from the step env: (falling back to always only when unset), a user who sets a job/workflow-level env: GH_AW_DOCKER_IMAGE_PULL_POLICY: never (e.g. via jobs.agent.env or top-level env:) expecting the generated step to inherit it will have that value silently overridden back to the default always pull behavior whenever docker-image-pull-policy isn't explicitly "never" in frontmatter — defeating the local-only guarantee without any error.
Suggested fix: thread a tri-state policy (unset / "always" / "never") instead of a boolean, and explicitly emit GH_AW_DOCKER_IMAGE_PULL_POLICY: always when docker-image-pull-policy: "always" is configured, leaving the env line absent only when the frontmatter key is truly unset. Add a test for the interaction between frontmatter docker-image-pull-policy and a directly-set GH_AW_DOCKER_IMAGE_PULL_POLICY env var.
@copilot please address this.
There was a problem hiding this comment.
The generator now preserves all three policy states and explicitly emits always/never, leaving unset absent so inherited env remains observable. Added coverage for both agent and detection steps. Fixed in 61a19e3.
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 5f69c8a
|
…-image-validation Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |

Preloading images does not prevent the generated download step from pulling them before agent or detection startup. Credential-free consumer jobs need an explicit way to require local images without a registry fallback.
docker-image-pull-policy: neverto apply local-only validation to generated agent and detection download steps. Unset oralwaysretains the existing pull behavior.RepoDigestsfor the entire set before creating requested aliases. Reject missing metadata, conflicting aliases, and failures without pulling or retrying.