Fix MCP gateway mount policy rejection for safeoutputs backend server - #51870
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Thanks for addressing the mount-policy regression. I think this needs one compiler-side change before merge: Requested change (blocking): include every compiler-supported per-server host mount in This belongs in the gh-aw compiler rather than mcpg. Once While updating this, please preserve Docker semantics for mode-less mounts: they are writable, so they must not be added to the allowlist as Security tightening: |
|
@copilot apply the recommended change in this comment #51870 (comment) |
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Addressed in 17d1465:
Done in 17d1465 — see my reply on the review comment above for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ Ponytail Reviewer completed successfully! Pre-fetched pr-diff.patch contains only compiled .lock.yml workflow files (generated artifacts), which are explicitly out of scope for ponytail over-engineering review. The actual source change (pkg/workflow/mcp_setup_gateway.go) is not present in the fetched diff, so there is no reviewable code to assess for complexity/over-engineering.
|
There was a problem hiding this comment.
Pull request overview
Fixes MCP gateway startup failures by explicitly allowing compiler-supported host mounts.
Changes:
- Builds and forwards
MCP_GATEWAY_ALLOWED_MOUNT_ROOTS. - Merges built-in and user-configured mounts with deterministic permissions.
- Adds tests and regenerates compiled artifacts.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/mcp_setup_gateway.go |
Implements and forwards the mount allowlist. |
pkg/workflow/mcp_gateway_mount_policy_test.go |
Tests mount collection, merging, and forwarding. |
.github/workflows/*.lock.yml (284 files) |
Regenerates compiled workflows with the new environment variable. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden |
Updates Claude golden output. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden |
Updates Codex golden output. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden |
Updates Copilot golden output. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden |
Updates Gemini golden output. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/basic-copilot.golden |
Updates basic compilation output. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden |
Updates Playwright compilation output. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden |
Updates smoke-test compilation output. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/with-imports.golden |
Updates imported-workflow compilation output. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 228/294 changed files
- Comments generated: 1
- Review effort level: Balanced
| entries = append(entries, root+":"+rootModes[root]) | ||
| } | ||
| return strings.Join(entries, ",") |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes are consistent and correct across all lock files. Each file receives two coordinated changes: (1) a new MCP_GATEWAY_ALLOWED_MOUNT_ROOTS env export with a correctly structured path:mode list matching existing volume mounts, and (2) -e MCP_GATEWAY_ALLOWED_MOUNT_ROOTS passed to the docker run command so the gateway container receives the policy. Mount roots match the existing -v volume mounts. No correctness, security, or reliability concerns found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 45.9 AIC · ⌖ 9.04 AIC · ⊞ 5.4K
🧪 Test Quality Sentinel Report — PR #51870Repository: github/gh-aw Score: 100/100 ✅ ExcellentImplementation Test Ratio: 0% (threshold: 30%) ✅ SummaryThis PR adds 13 new/modified behavioral tests to Key strengths:
Test Classification Table (13 tests, 5 new)
All 13 tests are design tests verifying behavioral contracts. Quality SignalsEdge-case & error coverage:
Code quality:
Verdict: ✅ APPROVED — Excellent test quality with comprehensive behavioral coverage and no violations.
|
Documents the decision to introduce buildMCPGatewayAllowedMountRoots in the compiler to resolve gh-aw-mcpg v0.4.9 mount policy violations. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (366 new lines in 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 MatterADRs 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
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — two minor suggestions on test robustness; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Hardcoded ordered assertion (
test line 158): theTestWriteMCPGatewayExportsIncludesAllowedMountRootsassertion embeds the full sorted comma-separated value; this will produce an opaque failure if a new built-in root is added. IndividualContainschecks (as used elsewhere in the same file) are more resilient. - Untested scalar-tool-entry path (
collectMCPServerConfiguredMounts): the early-exitcontinuefor non-map[string]anyvalues is correct Go, but a small table test would document and lock in the no-panic guarantee.
Positive Highlights
- ✅ Root cause addressed, not just the symptom: exports the allowlist before gateway startup so mcpg never sees an unlisted mount.
- ✅
addRootdedup logic correctly prevents rw → ro downgrade while allowing ro → rw upgrade. - ✅ Backslash-escape normalization of
\${VAR}inparseMCPGatewayAllowlistMountcloses a subtle injection/mismatch path from imported partials. - ✅ Determinism guaranteed via
sliceutil.SortedKeys, with an explicit determinism test. - ✅ Both
argsandentrypointArgsvolume-flag patterns (-v,--volume,--volume=,-v=) are covered, including a test for mode-less mounts defaulting torw. - ✅ Golden fixture updates included — reviewers can see the exported env var in compiled output.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 70.6 AIC · ⌖ 6.82 AIC · ⊞ 7.1K
Comment /matt to run again
| payloadDir: "/tmp/payloads", | ||
| }) | ||
|
|
||
| assert.Contains(t, runScript.String(), `export MCP_GATEWAY_ALLOWED_MOUNT_ROOTS="${GITHUB_WORKSPACE}:rw,${RUNNER_TEMP}/gh-aw:ro,${RUNNER_TEMP}/gh-aw/safeoutputs:rw,/tmp:rw,/usr/bin/gh:ro"`) |
There was a problem hiding this comment.
[/tdd] This assertion hard-codes the full sorted comma-separated string, coupling the test to insertion order and the exact set of built-in roots.
If a new built-in root is added to buildMCPGatewayAllowedMountRoots, this test fails with an opaque string mismatch instead of pointing to the new entry. The determinism test on line 68 already covers ordering stability.
💡 Suggested approach
Replace with individual Contains assertions (consistent with how TestBuildMCPGatewayAllowedMountRoots already tests individual entries):
assert.Contains(t, runScript.String(), `export MCP_GATEWAY_ALLOWED_MOUNT_ROOTS=`)
assert.Contains(t, runScript.String(), `${GITHUB_WORKSPACE}:rw`)
assert.Contains(t, runScript.String(), `${RUNNER_TEMP}/gh-aw:ro`)
assert.Contains(t, runScript.String(), `${RUNNER_TEMP}/gh-aw/safeoutputs:rw`)
assert.Contains(t, runScript.String(), `/tmp:rw`)
assert.Contains(t, runScript.String(), `/usr/bin/gh:ro`)@copilot please address this.
| // can be reflected in the gateway's trusted mount allowlist. This covers | ||
| // explicit "mounts" arrays as well as "-v"/"--volume" flags embedded in | ||
| // "args"/"entrypointArgs" for containerized servers. | ||
| func collectMCPServerConfiguredMounts(tools map[string]any) []string { |
There was a problem hiding this comment.
[/diagnosing-bugs] collectMCPServerConfiguredMounts iterates a tools map[string]any with no test for the nil path, and silently skips non-map[string]any tool entries. Both are safe in Go, but there is no test that verifies mount entries from a tool whose value is not a map are skipped without panic.
This is a minor gap — worth a short table-driven test case to confirm that a scalar tool entry ("my-tool": "somestring") does not cause a nil-pointer dereference.
💡 Suggested test case
t.Run("scalar tool value is silently skipped", func(t *testing.T) {
tools := map[string]any{
"not-a-map": "just-a-string",
"my-server": map[string]any{
"mounts": []any{"/srv/(redacted)
},
}
roots := buildMCPGatewayAllowedMountRoots(tools, nil)
assert.Contains(t, roots, "/srv/(redacted)
// scalar entry should be silently ignored, not panic
})@copilot please address this.
Add two new terms based on recent commits (#51842, #51870): - Host Service Ports (services:) — AWF sandbox access to GitHub Actions services containers via --allow-host-service-ports - MCP Gateway Mount-Roots Allowlist (MCP_GATEWAY_ALLOWED_MOUNT_ROOTS) — compiler-computed mount policy allowlist for MCP backend containers Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
The
agentjob in run 31410242707 failed becausegh-aw-mcpgv0.4.9 introduced a trusted host-path mount policy that, by default, only permits read-only access to$GITHUB_WORKSPACEand read-write access to the system temp dir. Our compiler mounts${GITHUB_WORKSPACE}read-write into the built-insafeoutputsMCP server, so the gateway now rejects it at startup:This breaks
safeoutputstool registration entirely, failing every workflow run that uses it.Gateway configuration
buildMCPGatewayAllowedMountRoots()(pkg/workflow/mcp_setup_gateway.go) which computes an explicit allowlist granting read-write access to the paths our built-in servers mount (${GITHUB_WORKSPACE},${RUNNER_TEMP}/gh-aw,/tmp) and read-only access to/usr/bin/gh, merged with any user-configuredsandbox.mcp.mountsMCP_GATEWAY_ALLOWED_MOUNT_ROOTSbefore gateway startup, and forward it into the gateway container via a new-e MCP_GATEWAY_ALLOWED_MOUNT_ROOTSflagTests & generated artifacts
.lock.ymlworkflows and regenerated wasm golden fixtures to reflect the new exported env var