Conversation
Add checkDockerHost() to src/cli.ts that inspects DOCKER_HOST on startup. If it points at a non-default socket (e.g. tcp://localhost:2375 for a DinD sidecar), AWF exits immediately with a clear error explaining why it is incompatible and pointing at the new docs section. Also add a "Workflow-Scope DinD Incompatibility" section to docs/usage.md documenting the root cause, the error message users will see, and the --enable-dind workaround for agents that genuinely need Docker access. Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/d99ee10d-b3d6-4811-a197-9eb8bb15da2a
There was a problem hiding this comment.
Pull request overview
Adds an early-startup guard to prevent AWF from running when DOCKER_HOST is configured to use workflow-scope DinD (or any non-default Docker socket), since that breaks AWF’s network/iptables isolation assumptions.
Changes:
- Add
checkDockerHost()and invoke it in the main CLI action to fail fast on unsupportedDOCKER_HOSTvalues. - Add unit tests covering allowed/blocked
DOCKER_HOSTcases. - Document the workflow-scope DinD incompatibility and the
--enable-dindworkaround.
Show a summary per file
| File | Description |
|---|---|
| src/cli.ts | Introduces checkDockerHost() and a startup fail-fast check before running the main workflow. |
| src/cli.test.ts | Adds targeted unit tests for the new checkDockerHost() validation behavior. |
| docs/usage.md | Documents why workflow-scope DinD breaks AWF and provides guidance/workarounds. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
| return { | ||
| valid: false, | ||
| error: | ||
| `DOCKER_HOST is set to an external daemon (${dockerHost}). ` + | ||
| 'AWF requires the local Docker daemon (default socket). ' + | ||
| 'Workflow-scope DinD is incompatible with AWF\'s network isolation model. ' + | ||
| 'See the "Workflow-Scope DinD Incompatibility" section in docs/usage.md for details and workarounds.', | ||
| }; |
There was a problem hiding this comment.
checkDockerHost() rejects non-default unix:// sockets (e.g. unix:///tmp/custom-docker.sock) but the returned error always says "external daemon", which is inaccurate/misleading for local unix sockets. Either (a) change the wording to something like "unsupported DOCKER_HOST value" and explicitly list the only supported socket values, or (b) loosen the check to allow any unix:// socket and only block remote schemes (tcp://, ssh://, etc.).
| } | ||
| }); | ||
|
|
||
| it('should return invalid for a TLS TCP daemon', () => { |
There was a problem hiding this comment.
This test name implies TLS based only on port 2376, but TLS is controlled by DOCKER_TLS_VERIFY/cert env vars, not the port number. Consider renaming to avoid encoding an assumption (e.g., "should return invalid for a TCP daemon on a non-default port").
| it('should return invalid for a TLS TCP daemon', () => { | |
| it('should return invalid for a TCP daemon on a non-default port', () => { |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 85.33% | 85.35% | 📈 +0.02% |
| Statements | 85.18% | 85.20% | 📈 +0.02% |
| Functions | 87.45% | 87.50% | 📈 +0.05% |
| Branches | 77.69% | 77.67% | 📉 -0.02% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cli.ts |
61.0% → 61.1% (+0.10%) | 61.4% → 61.5% (+0.09%) |
src/docker-manager.ts |
85.9% → 86.2% (+0.32%) | 85.5% → 85.8% (+0.31%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Smoke Test Results — run 24287094163 ✅ GitHub MCP: "fix: remove duplicate paragraph..." / "fix: loosen checkDockerHost..." Overall: PASS
|
🤖 Smoke Test Results
Overall: PASS · PR by
|
|
Smoke Test Results
|
Chroot Version Comparison Results
Overall: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot environment.
|
Smoke Test: GitHub Actions Services Connectivity ✅All checks passed:
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS Notes
|
Workflow-scope DinD (
DOCKER_HOST=tcp://localhost:2375) silently breaks AWF because Docker Compose routes container creation through the DinD daemon's network namespace, where AWF's fixed subnet (172.30.0.0/24) and iptables DNAT rules are unreachable from the runner.Changes
Fail-fast check (
src/cli.ts):checkDockerHost()inspectsDOCKER_HOSTat startup. Any value other than the two canonical local sockets (unix:///var/run/docker.sock,unix:///run/docker.sock) exits immediately with a clear error:Documentation (
docs/usage.md): New Workflow-Scope DinD Incompatibility subsection under Limitations — explains root cause, the error message, and the--enable-dindworkaround for agents that genuinely need Docker access.Tests (
src/cli.test.ts): 7 unit tests forcheckDockerHost()covering absent/undefined env var, both valid local socket paths, TCP, TLS TCP, and non-standard unix socket.