Skip to content

Add frontmatter policy for local-only Docker image validation - #66284

Merged
pelikhan merged 4 commits into
mainfrom
copilot/support-local-image-validation
Oct 7, 2026
Merged

pelikhan merged 4 commits into
mainfrom
copilot/support-local-image-validation

Conversation

Copilot AI commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

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.

  • Frontmatter: Add docker-image-pull-policy: never to apply local-only validation to generated agent and detection download steps. Unset or always retains the existing pull behavior.
  • Fail-closed helper: Require digest-pinned, registry-qualified references; verify exact local RepoDigests for the entire set before creating requested aliases. Reject missing metadata, conflicting aliases, and failures without pulling or retrying.
  • Coverage and guidance: Add mock-Docker and compiler regression tests, and document both consumer hooks and the limits of local metadata validation.
docker-image-pull-policy: never

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Add opt-in local-image mode to download_docker_images.sh Add frontmatter policy for local-only Docker image validation Oct 6, 2026
Copilot AI requested a review from pelikhan October 6, 2026 21:38
@pelikhan
pelikhan marked this pull request as ready for review October 6, 2026 23:34
Copilot AI balanced review requested due to automatic review settings October 6, 2026 23:34

Copilot AI left a comment

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.

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 Medium severity

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

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.

Added download_docker_images_local_test.sh to make test-scripts; the target passes. Fixed in 61a19e3.

Comment thread pkg/workflow/docker.go Outdated

// generateDownloadDockerImagesStep generates the step to download Docker images
func generateDownloadDockerImagesStep(yaml *strings.Builder, dockerImages []string) {
func generateDownloadDockerImagesStep(yaml *strings.Builder, dockerImages []string, localOnly bool) {

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.

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.

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

✅ 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.

🏗️ ADR gate enforced by Design Decision Gate 🏗️

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

✅ Test Quality Sentinel completed test quality analysis.

Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff

🧪 Test quality analysis by Test Quality Sentinel

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

✅ Ponytail Reviewer completed successfully!

Lean already. Ship.

Generated by Ponytail Reviewer for #66284

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

✅ PR Code Quality Reviewer completed the code quality review.

🔎 Code quality review by PR Code Quality Reviewer

@github-actions github-actions Bot left a comment

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.

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.sh mock-Docker suite is never invoked by make 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 :latest alias, 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 localOnly bool means the interaction with a user-set env: GH_AW_DOCKER_IMAGE_PULL_POLICY is 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}"

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.

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

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.

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

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.

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

Without 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.

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.

Registered the mock-Docker suite in make test-scripts; the target passes. Fixed in 61a19e3.

Comment thread pkg/workflow/docker.go Outdated

// generateDownloadDockerImagesStep generates the step to download Docker images
func generateDownloadDockerImagesStep(yaml *strings.Builder, dockerImages []string) {
func generateDownloadDockerImagesStep(yaml *strings.Builder, dockerImages []string, localOnly bool) {

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] 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.

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.

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.

@github-actions github-actions Bot mentioned this pull request Oct 6, 2026
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Comment Memory

Peek at saved memory (pr-code-quality-reviewer)
reviewed_at: 2026-10-06T23:45:13.862+00:00
review_event: REQUEST_CHANGES
top_themes:
  - explicit always policy does not override inherited GH_AW_DOCKER_IMAGE_PULL_POLICY
  - new local-only Docker regression test is not wired into scripted CI entrypoints
files_reviewed:
  - actions/setup/sh/download_docker_images.sh
  - actions/setup/sh/download_docker_images_local_test.sh
  - docs/src/content/docs/reference/frontmatter-full.md
  - docs/src/content/docs/reference/frontmatter.md
  - pkg/parser/schema_test.go
  - pkg/parser/schemas/main_workflow_schema.json
  - pkg/workflow/docker.go
  - pkg/workflow/docker_predownload_test.go
  - pkg/workflow/mcp_setup_generator.go
  - pkg/workflow/threat_detection_external.go
  - pkg/workflow/threat_detection_job_runtime_test.go
comment_count: 0

Note

This comment is managed by comment memory.

Expand the saved memory block to view or edit the persistent context for this thread.
Edit only the text inside the backtick fences; workflow metadata and the footer are regenerated automatically.

Learn more about comment memory

🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 92.9 AIC · ⌖ 5.8 AIC · ⊞ 19.6K · ◷
Comment /review to run again

@github-actions github-actions Bot left a comment

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.

Request changes

Two merge blockers remain.

Blocking themes
  • docker-image-pull-policy: always is not a real override today because the generated step only exports GH_AW_DOCKER_IMAGE_PULL_POLICY for never, so a workflow/job-level env value can still force local-only behavior even when the frontmatter is unset or explicitly always.
  • 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

@github-actions github-actions Bot left a comment

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.

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:

  1. Untagged digest references silently fall back to a :latest alias (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.
  2. The new mock-Docker regression test (download_docker_images_local_test.sh) isn't wired into make test-scripts/CI, so none of its fail-closed edge-case coverage (conflicting digests, inspect/tag failures, uppercase-digest rejection) actually runs in CI.
  3. generateDownloadDockerImagesStep's localOnly bool collapses "policy unset" and "policy explicitly always" into the same no-override behavior (pkg/workflow/docker.go:290), which can silently defeat a directly-set GH_AW_DOCKER_IMAGE_PULL_POLICY=never env var whenever frontmatter doesn't also say never.

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}"

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.

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.

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.

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

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.

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.

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.

Registered the mock-Docker suite in make test-scripts; the target passes. Fixed in 61a19e3.

Comment thread pkg/workflow/docker.go Outdated

// generateDownloadDockerImagesStep generates the step to download Docker images
func generateDownloadDockerImagesStep(yaml *strings.Builder, dockerImages []string) {
func generateDownloadDockerImagesStep(yaml *strings.Builder, dockerImages []string, localOnly bool) {

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.

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.

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.

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.

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot address the following outstanding work in one pass:

  1. Update this branch with the latest main using make merge-main, resolving any conflicts and preserving the intended changes.
  2. Review (actions/setup/sh/download_docker_images_local_test.sh:1): This mock-Docker regression suite is not invoked by the repository's test-scripts target, which enumerates shell tests explicitly, so CI can pass without exercising any of the new fail-closed helper behavior. Add this script to that target so these cases run in the normal test suite. - Add frontmatter policy for local-only Docker image validation #66284 (comment)
  3. Review (pkg/workflow/docker.go:290): The localOnly boolean collapses an unset policy and an explicit always policy into the same value. Because the generated step emits no override in both cases, a valid workflow-level env: GH_AW_DOCKER_IMAGE_PULL_POLICY: never causes docker-image-pull-policy: always to run the local-only branch instead of pulling. Preserve the three states and emit GH_AW_DOCKER_IMAGE_PULL_POLICY: always when always is explicitly configured, while leaving it absent only when the frontmatter field is unset. - Add frontmatter policy for local-only Docker image validation #66284 (comment)
  4. Review (actions/setup/sh/download_docker_images.sh:45): [/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. - Add frontmatter policy for local-only Docker image validation #66284 (comment)
  5. Review (actions/setup/sh/download_docker_images_local_test.sh:1): [/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. - Add frontmatter policy for local-only Docker image validation #66284 (comment)
  6. Review (pkg/workflow/docker.go:290): [/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. - Add frontmatter policy for local-only Docker image validation #66284 (comment)
  7. Review (actions/setup/sh/download_docker_images.sh:45): Untagged digest reference silently gets a :latest alias, contradicting the docs - Add frontmatter policy for local-only Docker image validation #66284 (comment)
  8. Review (actions/setup/sh/download_docker_images_local_test.sh:1): New mock-Docker regression suite is never run in CI - Add frontmatter policy for local-only Docker image validation #66284 (comment)
  9. Review (pkg/workflow/docker.go:290): localOnly boolean collapses "unset" and explicit always" into one state, hiding a precedence bug with the env var - Add frontmatter policy for local-only Docker image validation #66284 (comment)

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
Sous-chef work: 173086efd80c4ea53ba2c44eccaba6f6f62d90225c10de4e9dca251acc937e96 47389ad7b3754c94615f79c65b9a6723bd7004ef52687b10bdca5a70338f4483 4bce3335440f3331655694ef4ad228c9aa1464be62f7d3edd5680f34eebfc3ea 4e2673aab169f5ddce775ba63a872f40da51432a2d8a54bf74aefcc0b29e2155 65bc21da3c1c662f5debd05c54269e2d1b8169cb47d9cdef97777080e22c9eb2 75fb76279049bf76f475475c9f9bd7bcfbe3e56d8256876afcdc4f6bc1924585 ee78bd106acf98f4d0fcf2362785f160cb46a9d9c6b9477fc1164e56996474d3 ef46c2fbd3e508abdefae125ab7973f60c1ae60b4b839edfe1fc5ce4cd4e67de
Sous-chef state: 16394ebec7926f9c7d5ddba3a3502c3177f61a4869dab2bd6f0ef916a3975184

Generated by 👨‍🍳 PR Sous Chef · pi · haiku45 · 10.6 AIC · ⌖ 6.06 AIC · ⊞ 1K · ◷
Comment /souschef to run again

Copilot AI and others added 2 commits October 7, 2026 01:03
…-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>
Copilot AI requested a review from gh-aw-bot October 7, 2026 01:23
@pelikhan
pelikhan merged commit 5651c8d into main Oct 7, 2026
2 checks passed
@pelikhan
pelikhan deleted the copilot/support-local-image-validation branch October 7, 2026 10:43
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

🎉 This pull request is included in a new release.

Release: v0.91.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support explicit local-only image validation without pre-start Docker pulls

4 participants