fix: loosen checkDockerHost to accept any unix:// socket; fix misleading test name#1912
fix: loosen checkDockerHost to accept any unix:// socket; fix misleading test name#1912
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
…b.com/github/gh-aw-firewall into copilot/fix-review-comments Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Loosens checkDockerHost() to treat any unix:// socket as local (and therefore compatible), while rejecting remote Docker daemon schemes and documenting the workflow-scope DinD incompatibility.
Changes:
- Add
checkDockerHost()to validateDOCKER_HOSTand fail fast on remote daemon schemes. - Add unit tests covering unset/undefined, default unix sockets, custom unix sockets, and TCP daemons.
- Document why workflow-scope DinD (via
DOCKER_HOST=tcp://...) is incompatible and provide a workaround.
Show a summary per file
| File | Description |
|---|---|
| src/cli.ts | Adds checkDockerHost() and enforces it during CLI startup to reject remote daemons. |
| src/cli.test.ts | Adds/updates tests to match the relaxed unix-socket acceptance and TCP rejection. |
| docs/usage.md | Adds a new section explaining workflow-scope DinD incompatibility and the --enable-dind workaround. |
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
| valid: false, | ||
| error: | ||
| `DOCKER_HOST is set to an external daemon (${dockerHost}). ` + | ||
| 'AWF requires the local Docker daemon (default socket). ' + |
There was a problem hiding this comment.
The error text says “AWF requires the local Docker daemon (default socket)”, but checkDockerHost() explicitly accepts any unix:// socket (including non-default paths). This message is misleading for users with a custom local socket; consider changing it to refer to a local unix socket (or “local Docker socket”) rather than the default socket specifically.
| 'AWF requires the local Docker daemon (default socket). ' + | |
| 'AWF requires a local Docker unix socket. ' + |
|
|
||
| ``` | ||
| ❌ DOCKER_HOST is set to an external daemon (tcp://localhost:2375). AWF requires the | ||
| local Docker daemon (default socket). Workflow-scope DinD is incompatible with AWF's |
There was a problem hiding this comment.
This documentation example repeats “local Docker daemon (default socket)”, but the new behavior accepts non-default unix:// sockets too. Updating this wording to “local Docker daemon via a unix socket” (or similar) would keep the docs consistent with the relaxed check.
| local Docker daemon (default socket). Workflow-scope DinD is incompatible with AWF's | |
| local Docker daemon via a unix socket. Workflow-scope DinD is incompatible with AWF's |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 85.85% | 85.87% | 📈 +0.02% |
| Statements | 85.76% | 85.77% | ➡️ +0.01% |
| Functions | 87.54% | 87.58% | 📈 +0.04% |
| Branches | 78.56% | 78.53% | 📉 -0.03% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cli.ts |
62.2% → 62.3% (+0.03%) | 62.8% → 62.8% (+0.01%) |
src/docker-manager.ts |
86.3% → 86.6% (+0.36%) | 85.9% → 86.2% (+0.35%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
|
| 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.0% (+0.04%) | 61.4% → 61.5% (+0.04%) |
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
Smoke Test Results
Overall: PASS
|
🔥 Smoke Test ResultsPR: fix: loosen checkDockerHost to accept any unix:// socket; fix misleading test name
Overall: PASS
|
|
Smoke test results:
|
Chroot Version Comparison Results
Overall: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot.
|
Smoke Test: GitHub Actions Services Connectivity ✅
All checks passed. (Note:
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
checkDockerHost()rejected non-default unix sockets (e.g.unix:///tmp/custom-docker.sock) with an "external daemon" error — inaccurate, since those still point to a local daemon.Changes
src/cli.ts: Replace hardcodedLOCAL_DOCKER_HOST_VALUESset with astartsWith('unix://')check. Only remote schemes (tcp://,ssh://) are now rejected. The "external daemon" error message is now accurate.src/cli.test.ts:'should return invalid for a TLS TCP daemon'→'should return invalid for a TCP daemon on a non-default port'(TLS is controlled by env vars, not port number)valid: true, consistent with the loosened check