fix: robust Docker socket GID resolution for ARC/DinD and split-daemon topologies#44446
Conversation
…topologies - Add GH_AW_DOCKER_SOCK_PATH override env var: lets operators on ARC-DinD (DOCKER_HOST=tcp://) or other split-daemon setups point the MCP gateway at the real socket path without host-side symlink/chown hacks. - Add GH_AW_DOCKER_SOCK_GID override env var: lets operators supply the socket group directly when stat cannot resolve it from the path. - Change stat -c to stat -Lc (follow symlinks): a socket symlinked from a non-standard path now resolves to the real socket's group without requiring chown -h on the symlink. - Fail loudly instead of falling back to group 0: when the socket group cannot be determined and no override is set, exit 1 with an actionable error message naming the path tried and the override env vars. Passing --group-add 0 to a non-root container guaranteed a confusing downstream 'Docker daemon is not accessible' with no indication of the true cause. - Update tests to assert the new override check snippets, stat -L call, and loud failure message are present in compiled YAML. - Update wasm golden files and recompile all .lock.yml files. Closes #44251 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the generated “Start MCP Gateway” shell to resolve the Docker socket path and socket group more reliably on runners where the Docker daemon is not exposed via the default /var/run/docker.sock path (e.g., ARC + DinD sidecar / split-daemon topologies), and regenerates all impacted golden/lock artifacts.
Changes:
- Add
GH_AW_DOCKER_SOCK_PATHandGH_AW_DOCKER_SOCK_GIDoverrides for explicit socket path and group configuration. - Switch socket-group detection to
stat -Lc '%g' …to follow symlinks and remove the silent--group-add 0fallback in favor of an explicit failure. - Regenerate workflow
.lock.ymlfiles and wasm golden fixtures to reflect the new emitted shell.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/mcp_setup_generator.go | Updates MCP gateway setup generation to support socket path/GID overrides, symlink-following stat -L, and loud failure behavior. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/basic-copilot.golden | Regenerated golden fixture reflecting new socket path/GID resolution shell. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden | Regenerated golden fixture reflecting new socket path/GID resolution shell. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/with-imports.golden | Regenerated golden fixture reflecting new socket path/GID resolution shell. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden | Regenerated golden fixture reflecting new socket path/GID resolution shell. |
| .github/workflows/ace-editor.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/agentic-token-audit.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/agentic-token-optimizer.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/agentic-token-trend-audit.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/archie.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/architecture-guardian.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/artifacts-summary.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/bot-detection.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/brave.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/ci-coach.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/cli-consistency-checker.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/code-simplifier.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/contribution-check.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/copilot-centralization-drilldown.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/copilot-centralization-optimizer.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/copilot-opt.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/craft.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/daily-byok-ollama-test.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/daily-credit-limit-test.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/daily-max-ai-credits-test.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/daily-team-status.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/daily-windows-terminal-integration-builder.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/delight.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/dependabot-repair.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/designer-drift-audit.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/dev-hawk.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/eslint-miner.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/example-failure-category-filter.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/example-permissions-warning.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/firewall-escape.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/firewall.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/gpclean.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/jsweep.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/layout-spec-maintainer.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/linter-miner.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/mergefest.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/objective-impact-report.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/org-health-report.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/outcome-collector.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/pdf-summary.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/plan.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/pr-description-caveman.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/pr-triage-agent.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/python-data-charts.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/q.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/refactoring-cadence.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/refiner.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/release.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/repo-tree-map.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/research.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/ruflo-backed-task.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/security-compliance.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/security-review.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/skillet.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/smoke-ci.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/smoke-copilot-sdk.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/smoke-copilot-small.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/smoke-copilot-sub-agents.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/smoke-service-ports.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/smoke-test-tools.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/smoke-workflow-call.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/stale-pr-cleanup.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/super-linter.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/technical-doc-writer.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/test-dispatcher.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/test-project-url-default.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/test-quality-sentinel.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/test-workflow.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/tidy.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/ubuntu-image-analyzer.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/update-astro.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/video-analyzer.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/weekly-issue-summary.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/workflow-generator.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/workflow-health-manager.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
| .github/workflows/workflow-normalizer.lock.yml | Regenerated workflow lock with updated MCP gateway docker-socket detection shell. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 273/273 changed files
- Comments generated: 1
- Review effort level: Low
| yaml.WriteString(" if [ -n \"${GH_AW_DOCKER_SOCK_GID:-}\" ]; then\n") | ||
| yaml.WriteString(" DOCKER_SOCK_GID=\"$GH_AW_DOCKER_SOCK_GID\"\n") | ||
| yaml.WriteString(" else\n") | ||
| yaml.WriteString(" DOCKER_SOCK_GID=$(stat -Lc '%g' \"$DOCKER_SOCK_PATH\" 2>/dev/null)\n") |
There was a problem hiding this comment.
Addressed in earlier commit. Line 592 of mcp_setup_generator.go now includes || true:
yaml.WriteString(" DOCKER_SOCK_GID=$(stat -Lc '%g' \"$DOCKER_SOCK_PATH\" 2>/dev/null || true)\n")This ensures the empty-variable check and error path at lines 593-597 are always reachable under set -eo pipefail.
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
Documents the decision to add GH_AW_DOCKER_SOCK_PATH/GH_AW_DOCKER_SOCK_GID overrides and replace the silent --group-add 0 fallback with a loud exit 1 for ARC/DinD and split-daemon runner topologies.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (740 new lines in business-logic directories) but does not have a linked Architecture Decision Record (ADR). 📄 Draft ADR committed:
📋 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
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
Verdict
Test scope: The only changed test is
All assertions carry descriptive failure messages. Build tag References: §28990059189 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Skills-Based Review Summary 🧠Applied 📋 Findings (4)
@copilot please address the review comments above.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /grill-with-docs — requesting changes on three areas.
📋 Key Themes & Highlights
Issues Found
- Missing ordering assertion (
/tdd):require.Less(sockGIDOverrideSnippet, socketGIDComputeSnippet)is missing — the path-override ordering is tested but the GID-override ordering is not, so a block transposition would go undetected. - Missing branch-order coverage (
/tdd): tests only doContains; no assertion verifies the GID override appears beforestatin generated output. - Diagnostic gap on failure (
/diagnosing-bugs): adding a[ -e "$DOCKER_SOCK_PATH" ]check beforeexit 1would help operators distinguish "path not found" from "stat permission denied" without any additional tooling. - Stale docs (
/grill-with-docs):self-hosted-runners.mdlines 168 and 245 still describe the old silent-fallback behaviour and omitGH_AW_DOCKER_SOCK_PATH/GH_AW_DOCKER_SOCK_GID; ARC/DinD operators reading those pages won't know the new overrides exist.
Positive Highlights
- ✅ All three root-cause bugs (non-unix scheme fallback, missing symlink follow, silent group-0) addressed cleanly.
- ✅
stat -Lcis the right fix — compact, portable, and the Go comment explains why-Lis needed. - ✅ Fail-loud with
::error::+exit 1is the correct pattern; the message names both override variables. - ✅ Ordering assertions for the path-override side are a great TDD pattern.
- ✅ 273 lock files regenerated consistently — expected and correct churn.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 128 AIC · ⌖ 7 AIC · ⊞ 6.6K
Comment /matt to run again
| "MCP_GATEWAY_GID should be computed before it is used in the docker command") | ||
| require.Less(t, strings.Index(yamlStr, userSnippet), strings.Index(yamlStr, groupAddSnippet), | ||
| "Docker command should include user mapping before supplementary group mapping") | ||
| require.Less(t, strings.Index(yamlStr, sockPathOverrideSnippet), strings.Index(yamlStr, socketPathSnippet), |
There was a problem hiding this comment.
[/tdd] Missing ordering assertion: there is no require.Less verifying that sockGIDOverrideSnippet appears before socketGIDComputeSnippet. The path-override ordering is checked (line 582), but the GID-override ordering is not — a future refactor could swap those blocks undetected.
💡 Suggested assertion
require.Less(t, strings.Index(yamlStr, sockGIDOverrideSnippet), strings.Index(yamlStr, socketGIDComputeSnippet),
"GH_AW_DOCKER_SOCK_GID override check should appear before stat-based GID computation")This mirrors the existing sockPathOverrideSnippet < socketPathSnippet assertion and closes the symmetry gap.
@copilot please address this.
There was a problem hiding this comment.
Addressed in commit 20dfd54. The ordering assertion is present at lines 604-605 of mcp_setup_generator_test.go:
require.Less(t, strings.Index(yamlStr, sockGIDOverrideSnippet), strings.Index(yamlStr, socketGIDComputeSnippet),
"GH_AW_DOCKER_SOCK_GID override check should appear before stat call")| require.Contains(t, yamlStr, socketGIDComputeSnippet, | ||
| "Shell should compute DOCKER_SOCK_GID from resolved socket path") | ||
| "Shell should compute DOCKER_SOCK_GID from resolved socket path following symlinks") | ||
| require.Contains(t, yamlStr, sockGIDFailSnippet, |
There was a problem hiding this comment.
[/tdd] No test covers the success path when GH_AW_DOCKER_SOCK_GID is set — the test only verifies the override snippet is present in the YAML, not that the generated shell skips stat when the override is supplied. A test that exercises both the override-present and override-absent branches at runtime would catch regressions where the branch order is correct but the conditional logic is inverted.
💡 Suggested test
Add a separate test (or table-driven sub-test) that compiles a workflow with GH_AW_DOCKER_SOCK_GID set and asserts that the generated shell takes the short-circuit branch:
// Verify that when GH_AW_DOCKER_SOCK_GID is set the stat call is inside the else branch,
// not reachable before the override check.
require.Less(t,
strings.Index(yamlStr, sockGIDOverrideSnippet),
strings.Index(yamlStr, socketGIDComputeSnippet),
"GID override branch must appear before stat-based fallback")Current tests only do Contains, so a transposition of the two blocks would pass undetected.
@copilot please address this.
There was a problem hiding this comment.
The ordering assertion at lines 604-605 addresses this concern. It verifies that sockGIDOverrideSnippet appears before socketGIDComputeSnippet in the generated YAML, which ensures the override check precedes the stat call. A transposition of the blocks would fail this assertion.
| yaml.WriteString(" else\n") | ||
| yaml.WriteString(" DOCKER_SOCK_GID=$(stat -Lc '%g' \"$DOCKER_SOCK_PATH\" 2>/dev/null)\n") | ||
| yaml.WriteString(" if [ -z \"$DOCKER_SOCK_GID\" ]; then\n") | ||
| yaml.WriteString(" echo \"::error::Cannot determine Docker socket group for '$DOCKER_SOCK_PATH'. Set GH_AW_DOCKER_SOCK_PATH and GH_AW_DOCKER_SOCK_GID to configure the socket path and group explicitly.\" >&2\n") |
There was a problem hiding this comment.
[/diagnosing-bugs] The error message is emitted to stderr but then exit 1 terminates the whole runner step — any downstream log lines (including the gateway startup logs) are lost. Consider also printing the resolved DOCKER_SOCK_PATH value alongside the error message so operators can quickly see exactly which path was tried.
💡 Suggested improvement
Current:
echo "::error::Cannot determine Docker socket group for '$DOCKER_SOCK_PATH'. Set GH_AW_DOCKER_SOCK_PATH and GH_AW_DOCKER_SOCK_GID to configure the socket path and group explicitly." >&2
exit 1The path is already embedded in the message, which is good. A small additional hint — listing whether the path exists at all — would reduce the "no such file vs wrong permissions" ambiguity for operators:
echo "::error::Cannot determine Docker socket group for '$DOCKER_SOCK_PATH'. Set GH_AW_DOCKER_SOCK_PATH and GH_AW_DOCKER_SOCK_GID to configure the socket path and group explicitly." >&2
[ -e "$DOCKER_SOCK_PATH" ] || echo "::warning::'$DOCKER_SOCK_PATH' does not exist on this runner." >&2
exit 1This is optional but significantly speeds up diagnosis when the socket path itself is wrong.
@copilot please address this.
There was a problem hiding this comment.
Addressed in commit 20dfd54. Socket existence warning added at line 595 of mcp_setup_generator.go:
yaml.WriteString(" [ -e \"$DOCKER_SOCK_PATH\" ] || echo \"::warning::'$DOCKER_SOCK_PATH' does not exist on this runner.\" >&2\n")Corresponding test assertion at line 582-583 of mcp_setup_generator_test.go.
There was a problem hiding this comment.
Review: fix: robust Docker socket GID resolution for ARC/DinD and split-daemon topologies
Overall: The fix is well-scoped and addresses the three root causes correctly. The approach — operator overrides, symlink-following stat, and loud failure — is the right pattern for this class of infrastructure portability issue.
Findings:
One non-blocking but recommended improvement: the GH_AW_DOCKER_SOCK_GID override is used without numeric validation. If an operator sets this to a non-numeric value, docker run --group-add <value> will fail with an opaque Docker engine error rather than the actionable message already established in the stat-failure path. Adding a grep -qE '^[0-9]+$' check before use would maintain consistency.
Everything else looks correct:
stat -Lccorrectly follows symlinks on Linux runners.- Lock file regeneration is mechanically uniform across all 260+ workflows.
- Tests assert override order, symlink-following stat, and loud failure.
- The
exit 1+::error::message naming the override vars is actionable.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 89.3 AIC · ⌖ 6.13 AIC · ⊞ 4.8K
| // to a non-root container gives no Docker-socket access and produces a confusing | ||
| // downstream "Docker daemon is not accessible" error. | ||
| yaml.WriteString(" if [ -n \"${GH_AW_DOCKER_SOCK_GID:-}\" ]; then\n") | ||
| yaml.WriteString(" DOCKER_SOCK_GID=\"$GH_AW_DOCKER_SOCK_GID\"\n") |
There was a problem hiding this comment.
The GH_AW_DOCKER_SOCK_GID override is accepted without validating that it contains a numeric value. Docker requires --group-add to receive a numeric GID or group name; a non-numeric user-supplied string will be passed through to the docker run command and fail with a cryptic Docker engine error rather than an early actionable message.
Consider adding a validation step:
if ! printf '%s' "$GH_AW_DOCKER_SOCK_GID" | grep -qE '^[0-9]+$'; then
echo "::error::GH_AW_DOCKER_SOCK_GID must be a numeric GID, got: ${GH_AW_DOCKER_SOCK_GID}" >&2
exit 1
fiThis would surface misconfiguration early with the same clear error-message pattern already used for the missing-GID case.
@copilot please address this.
There was a problem hiding this comment.
Addressed in commit 8c22b9f. Numeric validation implemented at lines 601-606 of mcp_setup_generator.go:
yaml.WriteString(" case \"$DOCKER_SOCK_GID\" in\n")
yaml.WriteString(" ''|*[!0-9]*)\n")
yaml.WriteString(" echo \"::error::DOCKER_SOCK_GID='$DOCKER_SOCK_GID' is not a valid numeric group ID. Set GH_AW_DOCKER_SOCK_GID to a numeric value.\" >&2\n")
yaml.WriteString(" exit 1\n")
yaml.WriteString(" ;;\n")
yaml.WriteString(" esac\n")Corresponding test assertions at lines 584-587, 608-610.
|
@copilot please run the
|
There was a problem hiding this comment.
REQUEST_CHANGES — The core logic is a clear improvement over the silent --group-add 0 fallback, but three issues should be addressed before merge.
### Blocking findings (3)
-
stat -Lcis GNU coreutils only (line 584) — On macOS self-hosted runnersstat -Lcsilently returns empty, triggering the loudexit 1with a misleading message. Either add a BSD fallback or document the Linux-only assumption and update the error message to mentionGH_AW_DOCKER_SOCK_GIDas the macOS path. (Comment on line 584) -
GH_AW_DOCKER_SOCK_GIDis not validated as numeric (line 582) — The override value is passed verbatim todocker --group-add. A non-numeric string will fail at container startup with an opaque Docker error rather than an actionable one at the point of override consumption. Acase *[!0-9]*)guard should be added after resolvingDOCKER_SOCK_GID. (Comment on line 582) -
exit 1presence is untested (line 534) —sockGIDFailSnippettests the error echo but notexit 1. Removal ofexit 1would pass tests while silently allowing the step to continue with an empty GID. (Comment on line 534)
### Non-blocking finding (1)
- **
(sockname/redacted) (2-slash authority form) yields a relative path** (line 570) —${DOCKER_HOST#(redacted) works correctly for the standard three-slash form but producessockname(relative) for `(sockname/redacted) Low likelihood in practice; acceptable to handle with a comment rather than a fix. (Comment on line 570)
🔎 Code quality review by PR Code Quality Reviewer · 194 AIC · ⌖ 5.92 AIC · ⊞ 5.4K
Comment /review to run again
| yaml.WriteString(" if [ -n \"${GH_AW_DOCKER_SOCK_GID:-}\" ]; then\n") | ||
| yaml.WriteString(" DOCKER_SOCK_GID=\"$GH_AW_DOCKER_SOCK_GID\"\n") | ||
| yaml.WriteString(" else\n") | ||
| yaml.WriteString(" DOCKER_SOCK_GID=$(stat -Lc '%g' \"$DOCKER_SOCK_PATH\" 2>/dev/null)\n") |
There was a problem hiding this comment.
stat -Lc is GNU coreutils only and will silently break on macOS self-hosted runners.
💡 Details & suggested fix
stat -Lc is Linux/GNU coreutils syntax. On macOS (and other BSD-based self-hosted runners) the equivalent flag is stat -Lf. Running this generated script on a macOS runner will produce empty DOCKER_SOCK_GID output (stat exits non-zero, swallowed by 2>/dev/null), which triggers the new exit 1 with a misleading message claiming the socket group cannot be determined — when the real problem is the wrong stat flag.
Since ARC/DinD is Linux this may be acceptable, but the assumption is implicit and the error message will confuse macOS operators who try the GH_AW_DOCKER_SOCK_PATH override and still get a failure.
Portable fallback:
DOCKER_SOCK_GID=$(stat -Lc '%g' "$DOCKER_SOCK_PATH" 2>/dev/null || \
stat -Lf '%g' "$DOCKER_SOCK_PATH" 2>/dev/null)Or at minimum add a comment in the generator code:
// stat -Lc is GNU coreutils (Linux only). macOS self-hosted runners must
// set GH_AW_DOCKER_SOCK_GID explicitly to bypass stat.There was a problem hiding this comment.
Addressed in commit 20dfd54. Portability comment added at lines 584-585 of mcp_setup_generator.go:
// Note: stat -Lc is GNU coreutils (Linux only). macOS self-hosted runners must set
// GH_AW_DOCKER_SOCK_GID explicitly to bypass stat.| // to a non-root container gives no Docker-socket access and produces a confusing | ||
| // downstream "Docker daemon is not accessible" error. | ||
| yaml.WriteString(" if [ -n \"${GH_AW_DOCKER_SOCK_GID:-}\" ]; then\n") | ||
| yaml.WriteString(" DOCKER_SOCK_GID=\"$GH_AW_DOCKER_SOCK_GID\"\n") |
There was a problem hiding this comment.
GH_AW_DOCKER_SOCK_GID is passed to docker --group-add without validating it is numeric, so a misconfigured override will cause a cryptic container startup failure instead of a clear error.
💡 Details & suggested fix
Line 582 assigns DOCKER_SOCK_GID="$GH_AW_DOCKER_SOCK_GID" verbatim. Docker's --group-add requires a numeric GID or a group name that exists inside the container's /etc/group. A non-numeric value (e.g., docker as a group name string, or a typo like 1000 --privileged) will not be shell-injected (the quoting in buildDockerCommandWithExpandableVars prevents that), but it will cause docker run to fail with an opaque error rather than a clear one at the point the override is consumed.
The same issue applies to the stat-derived path: stat -Lc '%g' should always produce a number, but on unusual filesystems it can return a group name string.
Suggested guard after resolving DOCKER_SOCK_GID:
case "$DOCKER_SOCK_GID" in
*[!0-9]*)
echo "::error::DOCKER_SOCK_GID must be a numeric GID, got: '$DOCKER_SOCK_GID'" >&2
exit 1 ;;
esacThere was a problem hiding this comment.
Addressed in commit 8c22b9f. Numeric validation implemented at lines 601-606 of mcp_setup_generator.go using the case pattern ''|*[!0-9]* to reject empty and non-numeric values. Test coverage at lines 584-587, 608-610 of mcp_setup_generator_test.go.
| sockGIDOverrideSnippet := `if [ -n "${GH_AW_DOCKER_SOCK_GID:-}" ]; then` | ||
| // Loud failure: exit 1 + error message when socket group can't be resolved, instead of | ||
| // silently passing --group-add 0 which guarantees a confusing downstream failure. | ||
| sockGIDFailSnippet := `echo "::error::Cannot determine Docker socket group for '$DOCKER_SOCK_PATH'. Set GH_AW_DOCKER_SOCK_PATH and GH_AW_DOCKER_SOCK_GID to configure the socket path and group explicitly." >&2` |
There was a problem hiding this comment.
The loud-failure test verifies the echo message but never asserts exit 1 is present, leaving a silent regression path.
💡 Details & suggested fix
sockGIDFailSnippet checks only that the ::error:: echo line exists. If the exit 1 on the next line is accidentally removed from the generator, the step will emit the error annotation and then continue executing — passing DOCKER_SOCK_GID='' (empty string) to docker --group-add '' which fails with a different, unrelated error.
Add an assertion:
sockGIDExitSnippet := `exit 1`
// ... inside the require block:
require.Less(t, strings.Index(yamlStr, sockGIDFailSnippet), strings.Index(yamlStr, sockGIDExitSnippet),
"exit 1 must immediately follow the loud error message in the socket-group failure block")Or at minimum:
require.Contains(t, yamlStr, sockGIDExitSnippet,
"Shell must exit 1 when socket group cannot be resolved")There was a problem hiding this comment.
Addressed in commit 20dfd54. Exit assertion added at lines 606-607 of mcp_setup_generator_test.go:
require.Less(t, strings.Index(yamlStr, sockGIDFailSnippet), strings.Index(yamlStr, sockGIDExitSnippet),
"exit 1 must immediately follow the loud error message in the socket-group failure block")sockGIDExitSnippet is defined at line 537.
| yaml.WriteString(" DOCKER_SOCK_PATH=\"${GH_AW_DOCKER_SOCK_PATH:-}\"\n") | ||
| yaml.WriteString(" if [ -z \"$DOCKER_SOCK_PATH\" ]; then\n") | ||
| yaml.WriteString(" case \"${DOCKER_HOST:-}\" in\n") | ||
| yaml.WriteString(" unix://* ) DOCKER_SOCK_PATH=\"${DOCKER_HOST#unix://}\" ;;\n") |
There was a problem hiding this comment.
${DOCKER_HOST#(redacted) strips only two slashes, yielding a relative path for the two-slash authority form (sockname/redacted)
💡 Details & suggested fix
The standard Docker unix socket URI uses three slashes: (redacted) The strip ${DOCKER_HOST#(redacted) removes the (redacted) prefix, leaving /var/run/docker.sock` (correct for the three-slash form).
However, Docker Engine also accepts the two-slash authority form (docker.sock/redacted) (socket in the current directory), and some orchestrators emit (redacted) In these cases the strip produces a relative path (docker.sock or ./docker.sock), which:
- Makes
stat -Lc '%g'resolve relative to the step working directory (not the socket's actual location) - Makes
docker -v relative-path:/var/run/docker.sockfail withinvalid volume specification
The fix is to strip the authority component explicitly:
(redacted) ) DOCKER_SOCK_PATH="${DOCKER_HOST#(redacted)
# Strip optional authority (e.g. (hostname/redacted) -> /path)
case "$DOCKER_SOCK_PATH" in
/*) ;; # already absolute
*) DOCKER_SOCK_PATH="/${DOCKER_SOCK_PATH#*/}" ;;
esac ;;This is a narrow edge case in practice, but the /* fallback branch below this line (line 571) does not cover it.
There was a problem hiding this comment.
Addressed in commit 20dfd54. Authority-stripping logic added at lines 572-576 of mcp_setup_generator.go:
yaml.WriteString(" # Strip optional authority component (e.g., unix://hostname/path -> /path)\n")
yaml.WriteString(" case \"$DOCKER_SOCK_PATH\" in\n")
yaml.WriteString(" /* ) ;; # already absolute\n")
yaml.WriteString(" * ) DOCKER_SOCK_PATH=\"/${DOCKER_SOCK_PATH#*/}\" ;;\n")
yaml.WriteString(" esac ;;\n")This handles both unix:///path (three-slash) and unix://hostname/path (two-slash authority) forms.
|
@copilot run pr-finisher skill |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
✅ All tools validated successfully! Agent Container Smoke Test confirms agent container is ready. |
|
🚀 Smoke Antigravity MISSION COMPLETE! Antigravity has spoken. ✨ |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
🚀 Smoke Gemini MISSION COMPLETE! Gemini has spoken. ✨ |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
🚀 Smoke Pi MISSION COMPLETE! Pi delivered. 🥧 |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
📰 BREAKING: Smoke Copilot - AOAI (apikey) is now investigating this pull request. Sources say the story is developing... |
|
📰 BREAKING: Smoke Copilot - AOAI (Entra) is now investigating this pull request. Sources say the story is developing... |
|
❌ Smoke Copilot Small failed. Please review the logs for details. |
Agent Container Tool Check\n\n| Tool | Status | Version |\n|------|--------|--------|\n| bash | ✅ | 5.2.21 |\n| sh | ✅ | available |\n| git | ✅ | 2.54.0 |\n| jq | ✅ | 1.7 |\n| yq | ✅ | 4.53.3 |\n| curl | ✅ | 8.5.0 |\n| gh | ✅ | 2.96.0 |\n| node | ✅ | 22.23.1 |\n| python3 | ✅ | 3.11.15 (PyPy) |\n| go | ✅ | 1.24.13 |\n| java | ✅ | 10.0.301 |\n| dotnet | ✅ | available |\n\nResult: 12/12 tools available ✅\n\nOverall Status: PASSWarning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Smoke test
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
|
Smoke Test: Gemini - 29100329385\n- Test 1: ✅\n- Test 2: ✅\n- Test 3: ✅\n- Test 4: ✅\n- Test 5: ❌\n\nOverall Status: FAILWarning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
|
Smoke Test: Codex - 29100329324 Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
|
Smoke Test 29100726052 Results:
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
💥 [THE END] — Illustrated by Smoke Claude · 121.8 AIC · ⌖ 31.1 AIC · ⊞ 8.4K
Comment /smoke-claude to run again
| // The script handles override variables (GH_AW_DOCKER_SOCK_PATH, GH_AW_DOCKER_SOCK_GID), | ||
| // DOCKER_HOST parsing, stat -Lc symlink following, and numeric validation. | ||
| // See actions/setup/sh/resolve_docker_socket_gid.sh for implementation details. | ||
| yaml.WriteString(" source \"${RUNNER_TEMP}/gh-aw/actions/resolve_docker_socket_gid.sh\"\n") |
There was a problem hiding this comment.
Nice — delegating socket GID resolution to a dedicated script improves testability and readability. 👍
| yaml.WriteString(" * ) DOCKER_SOCK_PATH=/var/run/docker.sock ;;\n") | ||
| yaml.WriteString(" esac\n") | ||
| yaml.WriteString(" DOCKER_SOCK_GID=$(stat -c '%g' \"$DOCKER_SOCK_PATH\" 2>/dev/null || echo '0')\n") | ||
| // Resolve Docker socket path and GID using the dedicated shell script. |
There was a problem hiding this comment.
Good call documenting the override vars and stat -Lc behavior inline; helps future maintainers understand the ARC/DinD edge cases.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Smoke bot leave two notes. Build good. Some smoke checks fail from missing tools and perms.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · 93.9 AIC · ⌖ 5.88 AIC · ⊞ 19.1K
Comment /smoke-copilot to run again
Add label smoke to run again
|
Smoke test results: ✅1,2,5,6,8,10,15; ❌3,4,7,9,11,12,14 Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
|
🎉 This pull request is included in a new release. Release: |
On runners where
DOCKER_HOST=tcp://(ARC + DinD sidecar), the MCP gateway's socket-group detection silently falls back to--group-add 0, producing a non-root container with no Docker socket access and a confusing downstreamDocker daemon is not accessibleerror.Root cause
Three compounding bugs in the shell emitted by
generateMCPGatewaySetup:DOCKER_HOSTschemes fall back to/var/run/docker.sock, which doesn't exist on the runner when the daemon is in a sidecarstat -c '%g'(no-L) reads the symlink's own group, not the resolved socket's — so even a symlinked workaround requireschown -hstat … 2>/dev/null || echo '0'silently emits group 0, guaranteeing failure with zero signalChanges
GH_AW_DOCKER_SOCK_PATHoverride — operators set this to the bind-mounted socket path (e.g./dind-sock/docker.sock) instead of carrying a host-side symlink hack at/var/run/docker.sockGH_AW_DOCKER_SOCK_GIDoverride — operators can supply the GID directly whenstatcan't resolve itstat -Lc '%g'— follows symlinks so a symlinked socket resolves to the real socket's group withoutchown -hon the linkexit 1s with an actionable::error::message naming the path tried and the two override vars; no more silent--group-add 0Generated shell (simplified):
Tests in
mcp_setup_generator_test.goupdated to assert the override check order,stat -Lcall, and failure message are present in compiled YAML. All.lock.ymlfiles and wasm golden files regenerated.✨ PR Review Safe Output Test - Run 29100329226
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.comSee Network Configuration for more information.