Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/2217c100-9699-4d81-80d6-663831f56efa Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot recompile and use hashed spec for containers when downloading and configuring awf |
There was a problem hiding this comment.
Pull request overview
Ensures compiled .lock.yml outputs deterministically pin builtin container images by digest at compile time by adding an embedded container-pin fallback when repo-local cache pins are missing.
Changes:
- Extend embedded pins schema to include
containersand addGetContainerPin(image)inpkg/actionpins. - Update workflow Docker image pinning to fall back from cache pins to embedded pins.
- Add/adjust tests to cover embedded container-pin retrieval and fallback behavior; refresh pinned digests in lock/pins JSON files.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/docker.go |
Applies embedded container pin fallback during image pinning for compilation output. |
pkg/workflow/action_pins.go |
Adds a workflow-level helper to retrieve embedded container pins via pkg/actionpins. |
pkg/workflow/docker_pin_test.go |
Updates container pinning tests and adds a case for embedded-pin fallback. |
pkg/actionpins/actionpins.go |
Extends embedded pin data model with container pins and exposes GetContainerPin. |
pkg/actionpins/actionpins_internal_test.go |
Adds test coverage for embedded container pin retrieval. |
pkg/actionpins/data/action_pins.json |
Refreshes embedded container digests / pinned references. |
pkg/workflow/data/action_pins.json |
Refreshes workflow-side pin data to align with current embedded pins. |
.github/aw/actions-lock.json |
Refreshes repo-local cache pin metadata for builtin container images. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 3
| { | ||
| name: "embedded pin used when cache is absent", | ||
| images: []string{"node:lts-alpine"}, | ||
| pins: nil, | ||
| expectedRefs: []string{"node:lts-alpine@sha256:d1b3b4da11eefd5941e7f0b9cf17783fc99d9c6fc34884a665f40a06dbdfc94f"}, | ||
| expectedDigests: []string{"sha256:d1b3b4da11eefd5941e7f0b9cf17783fc99d9c6fc34884a665f40a06dbdfc94f"}, | ||
| }, |
There was a problem hiding this comment.
The new embedded-pin test hard-codes a specific node:lts-alpine digest string. Since these digests are expected to be refreshed over time, this will create unnecessary churn/failures when the embedded JSON is updated. Prefer asserting against the embedded pin value returned by getEmbeddedContainerPin/actionpins.GetContainerPin, or assert invariants (non-empty Digest and PinnedImage contains @sha256:) rather than an exact digest literal.
| if embeddedPin, ok := getEmbeddedContainerPin(img); ok && embeddedPin.PinnedImage != "" { | ||
| result[i] = embeddedPin.PinnedImage | ||
| pins[i] = GHAWManifestContainer(embeddedPin) | ||
| dockerLog.Printf("Pinned container image from embedded pins: %s -> %s", img, embeddedPin.PinnedImage) | ||
| continue |
There was a problem hiding this comment.
applyContainerPins now pins images from embedded defaults even when the workflow cache has no entry, but the surrounding function comment/doc still states that images without a cached pin are returned unchanged. Update the doc comment(s) to reflect the new fallback order (cache -> embedded -> unchanged) so callers understand that some tags will be rewritten even with an empty cache.
| // getEmbeddedContainerPin returns the pinned container image for a given image reference. | ||
| func getEmbeddedContainerPin(image string) (actionpins.ContainerPin, bool) { | ||
| return actionpins.GetContainerPin(image) | ||
| } |
There was a problem hiding this comment.
getEmbeddedContainerPin returns actionpins.ContainerPin, while the cache path uses workflow.ContainerPin (same JSON fields). Mixing two nearly-identical types in the same pinning flow makes the code harder to follow and increases the chance of accidental divergence. Consider converting to (or aliasing) the workflow-level ContainerPin type at this boundary so applyContainerPins works with a single pin type.
There was a problem hiding this comment.
🤖 Smoke test agent was here! Agreed — mixing two nearly-identical ContainerPin types at the same boundary is a maintenance hazard. Unifying on the workflow-level type (or a type alias) would make applyContainerPins cleaner and reduce divergence risk over time.
📰 BREAKING: Report filed by Smoke Copilot · ● 1M
🧪 Test Quality Sentinel ReportTest Quality Score: 85/100✅ Excellent test quality
Test Classification DetailsView all test classifications
Test Analysis✅
|
| Component | Score | Notes |
|---|---|---|
| Behavioral coverage (40 pts) | 40 | 100% design tests |
| Error/edge case coverage (30 pts) | 15 | 1 of 2 scenarios includes an edge/fallback case |
| Low duplication (20 pts) | 20 | No duplicate clusters |
| Proportional growth (10 pts) | 10 | Test/prod ratio: docker_pin_test.go 11/6 = 1.8×; actionpins_internal_test.go 8/24 = 0.3× — both under 2:1 threshold |
| Total | 85 |
Verdict
✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). All assertions include descriptive messages, build tags are present, and no forbidden mock libraries are used.
📖 Understanding Test Classifications
Design Tests (High Value) verify what the system does:
- Assert on observable outputs, return values, or state changes
- Cover error paths and boundary conditions
- Would catch a behavioral regression if deleted
- Remain valid even after internal refactoring
Implementation Tests (Low Value) verify how the system does it:
- Assert on internal function calls (mocking internals)
- Only test the happy path with typical inputs
- Break during legitimate refactoring even when behavior is correct
- Give false assurance: they pass even when the system is wrong
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
References: §24761741708
🧪 Test quality analysis by Test Quality Sentinel · ● 501.2K · ◷
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 85/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). Both new test scenarios verify observable behavioral contracts with descriptive assertion messages, correct build tags, and no guideline violations.
Auto-generated by the Design Decision Gate workflow. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (128 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: What the draft coversThe draft ADR captures the key design decision in this PR: introducing a two-level fallback for container image pin resolution at compile time (repo-local cache → embedded defaults in
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
References: §24761741715
|
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/66e1e510-b6da-4870-ab9a-5fd6dc7496d1 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 23d0b31. I recompiled the lock files and updated AWF configuration to use hash-spec image metadata in Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
|
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
|
🚀 Smoke OpenCode MISSION COMPLETE! OpenCode delivered. 🔥 |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
|
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
|
Great work on this one, The PR is well-scoped, well-tested (unit tests for the embedded pin accessor, the This looks ready for maintainer review! ✅
|
|
Smoke Test 24762218449: ✅ PASS
PR: "Pin builtin container images by digest in compiled lock files and AWF hash-spec configuration" by
|
Comment Memory# Smoke Test HaikuCode flows through the pipes,
|
There was a problem hiding this comment.
Smoke test review of PR #27762 — pin builtin container images by digest. The overall approach is sound: deterministic digest pinning at compile time via embedded fallback data. Two inline suggestions: clarify the godoc on the new getEmbeddedContainerPin wrapper to make the no-cache semantics explicit, and consider logging the full resolved digest-tagged image string at runtime for operator visibility. No blocking issues.
📰 BREAKING: Report filed by Smoke Copilot · ● 1M
| @@ -77,6 +77,11 @@ func getActionPinByRepo(repo string) (ActionPin, bool) { | |||
| return actionpins.GetActionPinByRepo(repo) | |||
| } | |||
|
|
|||
| // getEmbeddedContainerPin returns the pinned container image for a given image reference. | |||
| func getEmbeddedContainerPin(image string) (actionpins.ContainerPin, bool) { | |||
There was a problem hiding this comment.
This thin wrapper around actionpins.GetContainerPin is a good addition — it keeps the calling code consistent with the existing getActionPinByRepo pattern. Consider whether the function-level godoc could mention that it falls back to embedded defaults (no cache involved), to make the pinning fallback chain explicit for future readers.
| awfImageTag := getAWFImageTag(firewallConfig) | ||
| // Pin AWF Docker image version to match the installed binary version and include | ||
| // digest metadata when available so AWF uses immutable image references. | ||
| awfImageTag := buildAWFImageTagWithDigests(getAWFImageTag(firewallConfig), config.WorkflowData) | ||
| awfArgs = append(awfArgs, "--image-tag", awfImageTag) | ||
| awfHelpersLog.Printf("Pinned AWF image tag to %s", awfImageTag) | ||
|
|
There was a problem hiding this comment.
Nice improvement — including digest metadata in the --image-tag flag makes the AWF image selection fully deterministic at compile time. The updated comment clearly describes the intent. One minor suggestion: consider logging the full resolved tag (with digests) at Printf level so operators can verify which exact digest was selected during a workflow run.
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
Smoke Test Run 24762218460 — Results: Core tests: ✅✅✅✅✅✅✅✅✅❌✅✅ (11/12 pass) Overall: PARTIAL — Test #10 (Agentic Workflows MCP status) ❌ unavailable; #16/#19
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
💥 [THE END] — Illustrated by Smoke Claude · ● 204.4K
| pins: nil, | ||
| expectedRefs: []string{"node:lts-alpine@sha256:d1b3b4da11eefd5941e7f0b9cf17783fc99d9c6fc34884a665f40a06dbdfc94f"}, | ||
| expectedDigests: []string{"sha256:d1b3b4da11eefd5941e7f0b9cf17783fc99d9c6fc34884a665f40a06dbdfc94f"}, | ||
| }, |
There was a problem hiding this comment.
🔍 Smoke Test Review Comment — Consider asserting invariants (non-empty Digest, PinnedImage contains @sha256:) rather than hard-coding the exact digest literal, to avoid test churn when embedded JSON is refreshed.
| result[i] = embeddedPin.PinnedImage | ||
| pins[i] = GHAWManifestContainer(embeddedPin) | ||
| dockerLog.Printf("Pinned container image from embedded pins: %s -> %s", img, embeddedPin.PinnedImage) | ||
| continue |
There was a problem hiding this comment.
🔍 Smoke Test Review Comment — The doc comment here should be updated to reflect the new fallback order: cache → embedded defaults → unchanged. Callers need to know that some tags may be rewritten even with an empty local cache.
Maintenance tone scan found 0 tone issues. Documented 4 new features from pending changesets not yet reflected in dev.md: - label_command trigger: new workflow trigger with status-comment and reaction defaults; exposes needs.activation.outputs.label_command - GHE support: configure_gh_for_ghe.sh script for GitHub Enterprise host auto-detection in workflows using the gh CLI - Audit commands: gh aw audit diff and gh aw audit report added to CLI quick reference and Command Categories section - Container image pinning by digest (PR #27762): ContainerPin struct in pkg/actionpins compiles mutable tags to immutable SHA-256 digests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Compiled
.lock.ymloutput could still emit builtin containers as mutable tags, even though container pin metadata already existed. This PR makes digest pinning deterministic for builtin images at compile time and keeps manifest metadata aligned with current builtin versions.Compiler behavior: fallback to embedded container pins
containerssection) inpkg/actionpins.GetContainerPin(image)accessor for embedded container pins..github/aw/actions-lock.json)pkg/actionpins/data/action_pins.json)image:tag@sha256:...even when local cache is missing or incomplete.AWF configuration now uses hashed container spec
--image-tagwith digest metadata when available (cache-first, embedded fallback), using AWF’s hash-spec format:tag,squid=sha256:...,agent=sha256:...,agent-act=sha256:...,api-proxy=sha256:...,cli-proxy=sha256:...Pinned metadata refresh for current builtin tags
node:lts-alpine, etc.) in:.github/aw/actions-lock.jsonpkg/actionpins/data/action_pins.jsonpkg/workflow/data/action_pins.jsondownload_docker_images.shargs use up-to-date digests.Targeted coverage for fallback and AWF hash-spec behavior
--image-tagdigest metadata format.Smoke CI scheduled run completed: https://github.com/github/gh-aw/actions/runs/24762193187
✨ PR Review Safe Output Test - Run 24762218460