feat: add configurable agent base image for GitHub Actions runner parity#214
feat: add configurable agent base image for GitHub Actions runner parity#214
Conversation
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 77.19% | 76.95% | 📉 -0.24% |
| Statements | 77.27% | 77.05% | 📉 -0.22% |
| Functions | 77.17% | 77.41% | 📈 +0.24% |
| Branches | 69.76% | 69.21% | 📉 -0.55% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/docker-manager.ts |
75.9% → 76.1% (+0.23%) | 75.2% → 75.5% (+0.23%) |
src/cli.ts |
35.5% → 35.9% (+0.38%) | 35.5% → 36.2% (+0.62%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Security Review: Custom Base Image SupportI've reviewed PR #214 and identified security considerations regarding the custom base image feature. SummaryThis PR allows users to specify arbitrary Docker base images via Security Concerns1. Third-Party Image Trust (Medium Risk)Location: ARG BASE_IMAGE=ubuntu:22.04
FROM ${BASE_IMAGE}Issue: The PR documentation recommends third-party images (
Why this matters:
Mitigating factors (existing controls):
2. Documentation GuidanceLocation: Recommendation: The documentation should include stronger security warnings about custom base images: ### Security Considerations
**⚠️ IMPORTANT:** Custom base images introduce supply chain risk. When using third-party images:
1. **Verify image sources** - Only use images from trusted publishers
2. **Review image contents** - Understand what tools and configurations are included
3. **Pin specific versions** - Use image digests (e.g., `@sha256:...`) instead of tags to prevent tag manipulation
4. **Monitor for vulnerabilities** - Third-party images may not receive timely security updates
For maximum security, use the default `ubuntu:22.04` image. Custom base images are recommended only when:
- You trust the image publisher
- You've reviewed the image build process
- The benefits outweigh the supply chain risksSuggested Actions
VerdictThe PR does not introduce new security vulnerabilities in the firewall implementation itself - all existing security controls remain intact. However, it does introduce supply chain risk by recommending third-party images. This is an acceptable trade-off if:
Recommendation: Approve with request for enhanced security documentation.
|
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Addressed in 39594c9. Security review feedback: Added "Security Considerations" section to CI failures (both unrelated to this PR):
|
The previous implementation only checked /usr/bin/docker specifically, which could fail when docker is installed in other locations like /usr/local/bin/docker (common in runner images). This caused the wrapper to create a symlink at /usr/bin/docker but fail to create docker-real, breaking the wrapper functionality. Changes: - Use 'command -v docker' to find docker regardless of installation path - Copy docker binary to docker-real from any location - Only remove /usr/bin/docker if that's where docker was found - Add explicit error handling for broken symlinks This ensures the wrapper works correctly with both minimal ubuntu:22.04 and GitHub Actions runner images that may have docker in different paths. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Resolved conflicts in containers/agent/Dockerfile: - Kept BASE_IMAGE argument for custom base image support - Combined Node.js installation approaches: check if v22 exists first (for runner images), otherwise install with verification from main - Adopted docker-stub.sh from main (Docker-in-Docker support removed in v0.9.1) - Removed Docker CLI installation and docker wrapper logic (no longer needed) Docker-in-Docker support was removed in main via PR #205, so the docker wrapper improvements from this branch are now obsolete. The BASE_IMAGE feature and Node.js handling improvements remain relevant for runner parity. All tests pass (551 tests).
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
💫 TO BE CONTINUED... Smoke Claude failed! Our hero faces unexpected challenges... |
Smoke Test ResultsLast 2 Merged PRs:
Test Results:
Overall Status: PASS
|
Security Review: PR #214I've completed a security review of this PR. Overall, the changes are acceptable from a security perspective, but I have one recommendation for improvement. ✅ Security Controls MaintainedAll critical security hardening remains intact:
✅ Good Security Practices
|
|
@copilot please take the suggestion from the security review and implement it |
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Implemented in 7b856e7. Added
If an invalid image is specified, the CLI exits with an error message explaining the approved options. |
Added more edge case tests for the validation function in 8976ec4 (5 additional test cases). The per-file coverage for both modified files ( The |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 77.19% | 76.95% | 📉 -0.24% |
| Statements | 77.27% | 77.05% | 📉 -0.22% |
| Functions | 77.17% | 77.41% | 📈 +0.24% |
| Branches | 69.76% | 69.21% | 📉 -0.55% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/docker-manager.ts |
75.9% → 76.1% (+0.23%) | 75.2% → 75.5% (+0.23%) |
src/cli.ts |
35.5% → 35.9% (+0.38%) | 35.5% → 36.2% (+0.62%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
|
@copilot please bump up the test coverage: https://github.com/githubnext/gh-aw-firewall/actions/runs/21092298632/job/60683405279?pr=214 |
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Added more tests in c87c634:
Per-file coverage for modified files has increased (+0.38% for cli.ts, +0.23% for docker-manager.ts). The small overall regression (-0.24%) is due to the CLI integration code (lines 685-699) which uses |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 77.88% | 77.63% | 📉 -0.25% |
| Statements | 77.94% | 77.71% | 📉 -0.23% |
| Functions | 77.29% | 77.54% | 📈 +0.25% |
| Branches | 71.00% | 70.44% | 📉 -0.56% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/docker-manager.ts |
79.1% → 79.3% (+0.19%) | 78.4% → 78.6% (+0.19%) |
src/cli.ts |
35.5% → 35.9% (+0.38%) | 35.5% → 36.2% (+0.62%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Security Review: Base Image Validation ScopeI've reviewed this PR for security implications and found one concern regarding the base image validation allowlist:
|
- Add gh (GitHub CLI) package to agent container - Add PATH manipulation to prefer system binaries over toolcache - Make user/group creation idempotent (check if exists first) - Add catthehacker act-XX.XX images to allowlist Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
Smoke Test Results (Run ID: 21269309104) Last 2 Merged PRs:
✅ GitHub MCP: Retrieved recent PRs Status: PASS cc: @Mossaka
|
Smoke Test Results (Claude)Last 2 merged PRs:
Test Results:
Overall: PASS
|
Cover the act image type in validation tests to fix coverage regression. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 77.88% | 77.63% | 📉 -0.25% |
| Statements | 77.94% | 77.71% | 📉 -0.23% |
| Functions | 77.29% | 77.54% | 📈 +0.25% |
| Branches | 71.00% | 70.44% | 📉 -0.56% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/docker-manager.ts |
79.1% → 79.3% (+0.19%) | 78.4% → 78.6% (+0.19%) |
src/cli.ts |
35.5% → 35.9% (+0.38%) | 35.5% → 36.2% (+0.62%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Smoke Test Results ✅GitHub MCP: ✅
Playwright: ✅ Page title: "GitHub · Change is constant. GitHub keeps you ahead. · GitHub" Overall Status: PASS cc @Mossaka
|
|
Smoke Test Results - Claude Engine Last 2 Merged PRs:
Test Results:
Overall Status: PASS
|
|
🧪 Smoke Test Results (Copilot) Last 2 Merged PRs:
✅ GitHub MCP - Retrieved PR data Status: PASS 🎉 cc: @Mossaka
|
BASE_IMAGEbuild argument to the agent Dockerfile--agent-base-imageCLI option to allow specifying custom base image when using--build-local^v22\.)validateAgentBaseImage()function with allowlist of approved base image patternsCI Notes:
Security Summary:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.