fix: add write-sink guard policies for non-GitHub MCP servers on auto-lockdown#21342
fix: add write-sink guard policies for non-GitHub MCP servers on auto-lockdown#21342
Conversation
…to-lockdown is used When a workflow has the github tool configured without explicit repos/min-integrity guard policies, the auto-lockdown detection step runs at runtime and always sets repos=all. Non-GitHub MCP servers (playwright, serena, mcpscripts, safeoutputs, agentic-workflows, web-fetch, custom) were missing the corresponding write-sink guard policies needed for the MCP gateway to allow writes to these servers. - Modify deriveWriteSinkGuardPolicyFromWorkflow to return accept=["*"] write-sink policy when GitHub tool is present, no explicit guard policies configured, and no GitHub App configured (same conditions that trigger auto-lockdown) - Update renderSafeOutputsMCPConfigWithOptions and renderSafeOutputsTOML to use deriveWriteSinkGuardPolicyFromWorkflow instead of deriveSafeOutputsGuardPolicyFromGitHub - Update tests to reflect new behavior and add GitHub App exclusion test - Recompile all 172 workflow lock files with the fix applied Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
✅ All tools validated successfully! Agent Container Smoke Test confirms agent container is ready. |
|
@copilot also add new tests to ensure that if the github-mcp server has a configured allow-only guard-policy that all non-github mcp servers have a write-sink guard-policy. |
There was a problem hiding this comment.
Pull request overview
Adds a missing “write-sink” guard policy for non-GitHub MCP servers when GitHub auto-lockdown is inferred (GitHub tool present but repos/min-integrity not explicitly configured), preventing the MCP gateway from blocking writes in that runtime scenario.
Changes:
- Extend
deriveWriteSinkGuardPolicyFromWorkflowto emitwrite-sink: { accept: ["*"] }in the auto-lockdown detection case (with a GitHub App exclusion). - Route safeoutputs guard-policy rendering through
deriveWriteSinkGuardPolicyFromWorkflowso it matches the same auto-lockdown behavior. - Update/extend tests and regenerate workflow lockfiles / golden fixtures to reflect the newly compiled guard-policies.
Reviewed changes
Copilot reviewed 165 out of 165 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/mcp_github_config.go | Derives non-GitHub write-sink guard policies from GitHub config, including new auto-lockdown fallback. |
| pkg/workflow/mcp_config_builtin.go | Updates safeoutputs JSON config rendering to use the new workflow-level guard policy derivation. |
| pkg/workflow/mcp_renderer_builtin.go | Updates safeoutputs TOML rendering to use the new workflow-level guard policy derivation. |
| pkg/workflow/non_github_mcp_guard_policy_test.go | Updates/adds tests for auto-lockdown guard-policy propagation and GitHub App exclusion. |
| pkg/workflow/mcp_config_compilation_test.go | Tightens compilation assertions to avoid false positives from accept: ["*"] in guard-policies. |
| pkg/workflow/testdata/wasm_golden/TestWasmGolden_CompileFixtures/smoke-copilot.golden | Updates wasm golden fixture with new compiled guard-policies blocks. |
| .github/workflows/*.lock.yml (regenerated set) | Recompiled workflow lockfiles now include guard-policies.write-sink.accept=["*"] for non-GitHub MCP servers when auto-lockdown applies. |
Comments suppressed due to low confidence (1)
pkg/workflow/mcp_config_builtin.go:153
- The log message says guard-policies are derived from the GitHub guard-policy, but deriveWriteSinkGuardPolicyFromWorkflow can now also return a default policy from auto-lockdown detection (no explicit repos/min-integrity). Consider adjusting the message to avoid implying explicit guard policies were set.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
pkg/workflow/mcp_github_config.go
Outdated
| // When no explicit guard policy is configured but automatic lockdown detection would run | ||
| // (GitHub tool present and not disabled, no GitHub App configured), a write-sink policy with | ||
| // accept=["*"] is returned because automatic lockdown always sets repos=all at runtime. | ||
| // | ||
| // Returns nil when no GitHub guard policies are configured or when workflowData is nil. | ||
| func deriveWriteSinkGuardPolicyFromWorkflow(workflowData *WorkflowData) map[string]any { |
There was a problem hiding this comment.
Fixed in 78f43de. The docstring now enumerates both non-nil cases (explicit guard policy and auto-lockdown) and clarifies the two nil-return conditions (no GitHub tool, or GitHub App configured). The log messages in mcp_config_builtin.go and mcp_renderer_builtin.go were also updated to mention auto-lockdown alongside guard-policy derivation.
| serverName: `"my-api"`, | ||
| expectedContent: []string{`"get_data"`, `"list_items"`}, | ||
| // Check that the tools array specifically does not have the wildcard | ||
| // (guard-policies accept may still contain "*" at deeper indentation) | ||
| unexpectedInServer: []string{"\"tools\": [\n \"*\""}, |
There was a problem hiding this comment.
Fixed in 2543fd9. The unexpectedInServer loop now uses regexp.MatchString instead of strings.Contains, and the three brittle patterns have been changed from whitespace-sensitive literals like "\"tools\": [\n \"*\"" to a whitespace-agnostic regex: "tools"\s*:\s*\[\s*"\*". This matches "tools": ["*"] in any formatting without false-positives from guard-policies's "accept": ["*"] (different key name).
| guardPolicies := deriveWriteSinkGuardPolicyFromWorkflow(workflowData) | ||
| if len(guardPolicies) > 0 { | ||
| mcpRendererLog.Print("Adding guard-policies to safeoutputs TOML (derived from GitHub guard-policy)") | ||
| // Render guard-policies in TOML format | ||
| renderGuardPoliciesToml(yaml, guardPolicies, constants.SafeOutputsMCPServerID.String()) |
There was a problem hiding this comment.
✅ Agreed — the log message could be more precise to distinguish the auto-lockdown-derived case from the explicit guard-policy case.
📰 BREAKING: Report filed by Smoke Copilot
There was a problem hiding this comment.
Already fixed in 78f43de — the log message was updated to "derived from GitHub guard-policy or auto-lockdown detection" in both mcp_config_builtin.go and mcp_renderer_builtin.go.
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
Smoke Test (Codex)PRs: #21338 [smoke-multi-pr] PR 1: Documentation smoke test; #21311 [docs] docs: condense MultiRepoOps best practices and remove redundant sections
Warning
|
…y for explicit allow-only guard policies Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Added |
|
Smoke Test Results — §23180654468
Overall: cc
|
There was a problem hiding this comment.
Smoke test review on PR #21342. This fix correctly extends write-sink guard policies to all non-GitHub MCP servers during auto-lockdown. The refactoring is clean and the mass-recompilation of 172 lock files ensures the fix is applied consistently across all workflows.
📰 BREAKING: Report filed by Smoke Copilot
| renderGuardPoliciesToml(yaml, guardPolicies, constants.SafeOutputsMCPServerID.String()) | ||
| } | ||
| } | ||
| guardPolicies := deriveWriteSinkGuardPolicyFromWorkflow(workflowData) |
There was a problem hiding this comment.
Consistent with the change in mcp_config_builtin.go. The simplification from nested if-blocks to a single function call improves readability. Consider adding a test case that verifies this TOML path also gets guard policies when auto-lockdown conditions are met.
| guardPolicies = deriveSafeOutputsGuardPolicyFromGitHub(githubTool) | ||
| } | ||
| } | ||
| guardPolicies := deriveWriteSinkGuardPolicyFromWorkflow(workflowData) |
There was a problem hiding this comment.
Good refactor — centralizing the guard policy derivation through deriveWriteSinkGuardPolicyFromWorkflow ensures non-GitHub MCP servers get consistent write-sink policies when auto-lockdown triggers. This is cleaner than the previous per-server checks.
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
Smoke Test Results — Run §23180654460Overall: PARTIAL (2 skipped, rest passed)
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
💥 [THE END] — Illustrated by Smoke Claude
| "GITHUB_REPOSITORY": "$GITHUB_REPOSITORY" | ||
| }, | ||
| "guard-policies": { | ||
| "write-sink": { |
There was a problem hiding this comment.
The write-sink guard policy with accept: ["*"] here correctly applies the write-sink restriction for non-GitHub MCP servers on auto-lockdown. This ensures that only trusted write destinations are permitted.
| "mounts": ["/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs:rw"] | ||
| "mounts": ["/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs:rw"], | ||
| "guard-policies": { | ||
| "write-sink": { |
There was a problem hiding this comment.
Good addition of write-sink guard policy for the Playwright MCP server. Consistent pattern with other non-GitHub MCP servers in this workflow.
…FromWorkflow auto-lockdown case Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…x patterns Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Already added |
When the
githubtool is configured without explicitrepos/min-integrity, the auto-lockdown step always setsrepos=allat runtime — but non-GitHub MCP servers (playwright, serena, mcp-scripts, safeoutputs, agentic-workflows, web-fetch, custom) had no correspondingwrite-sink: { accept: ["*"] }guard policies compiled in, causing the MCP gateway to block writes.Changes
mcp_github_config.go—deriveWriteSinkGuardPolicyFromWorkflownow returnswrite-sink: { accept: ["*"] }when the GitHub tool is present without explicit guard policies and without a GitHub App configured (the exact conditions that generate the auto-lockdown step)mcp_config_builtin.go/mcp_renderer_builtin.go— safeoutputs rendering updated to usederiveWriteSinkGuardPolicyFromWorkflowinstead of callingderiveSafeOutputsGuardPolicyFromGitHubdirectly, so it benefits from the same auto-lockdown detectionTests — updated to expect guard policies in the auto-lockdown case; added
TestNonGitHubMCPServersNoGuardPoliciesWithGitHubAppto cover the GitHub App exclusion; addedTestAllNonGitHubMCPServersGetWriteSinkWhenGitHubHasAllowOnlyto verify that when GitHub has an explicitallow-onlyguard policy configured (allreposvariants and allmin-integritylevels), every non-GitHub MCP server type (playwright, serena, agentic-workflows, mcp-scripts, safe-outputs, web-fetch) receives a correctly derivedwrite-sinkguard-policy; tightenedunexpectedInServerchecks in compilation tests to target thetoolsarray wildcard specifically rather than any"*"in the server blockLock files — all 172 workflow files recompiled; non-GitHub MCP servers now include
write-sink: { accept: ["*"] }in the generated JSON config when auto-lockdown appliesBefore (smoke-copilot, no explicit
min-integrity):After:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
✨ PR Review Safe Output Test - Run 23180654460