Repository navigation
Generate DIFC policies for GitHub App workflows - #58302
Conversation
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The critical TOML/Codex source-policy omission must be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Generates matching DIFC source and sink policies for GitHub App-authenticated workflows.
Changes:
- Enables automatic GitHub
allow-onlyandsafeoutputswrite-sink policies. - Adds compiler regression tests.
- Expands DIFC troubleshooting guidance.
File summaries
| File | Review |
|---|---|
pkg/workflow/non_github_mcp_guard_policy_test.go |
Updates policy derivation expectations. |
pkg/workflow/mcp_renderer_github.go |
Critical: TOML/Codex workflows still omit the automatic GitHub source guard policy. Add TOML rendering and Codex + GitHub App coverage. |
pkg/workflow/mcp_github_config.go |
Nits: Documentation incorrectly states automatic lockdown always sets repos=all; it is visibility-dependent. |
pkg/workflow/github_mcp_app_token_test.go |
Verifies generated source and sink policies. |
.github/aw/github-mcp-server.md |
Nit: Guidance presents mcp_servers as universal, although JSON configs use mcpServers. |
.github/aw/debug-agentic-workflow.md |
Adds DIFC troubleshooting guidance. |
Review details
Suppressed comments (2)
pkg/workflow/mcp_github_config.go:542
- The auto-lockdown step does not always set
repos=all: it emitsrepos=publicfor public repositories unlessprivate-to-public-flows: allowis enabled, andrepos=allfor private/internal repositories (actions/setup/js/determine_automatic_lockdown.cjs:69-72). The function documentation should describe the visibility-dependent output rather than an invariant that is false.
// 2. Auto-lockdown — when the GitHub tool is present without explicit guard policies,
// auto-lockdown detection will set repos=all at runtime, so a
// write-sink policy with accept=["*"] is returned to match that runtime behaviour.
pkg/workflow/mcp_github_config.go:581
- This repeats the incorrect
repos=allinvariant. Automatic lockdown resolvesrepostopublicfor a public repository by default, so keeping this explanation risks future changes deriving sink behavior from the wrong source-policy scope.
// When no explicit guard policy is configured but automatic lockdown detection would run
// (GitHub tool present and not disabled), return accept=["*"] because automatic lockdown
// always sets repos=all at runtime. GitHub App token scope is authentication, not a
// substitute for the DIFC sink labels enforced by the MCP gateway.
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
| } | ||
| } | ||
| shouldUseStepOutputForGuardPolicy := len(explicitGuardPolicies) == 0 && !hasGitHubApp(githubTool) | ||
| shouldUseStepOutputForGuardPolicy := len(explicitGuardPolicies) == 0 |
|
|
||
| > ⚠️ **Do NOT use `mode: remote`** in GitHub Actions workflows. Remote mode does not work with the GitHub Actions token (`GITHUB_TOKEN`) — it requires a special PAT or GitHub App token with MCP access. The default `mode: local` (Docker-based) works with `GITHUB_TOKEN` and should always be used. | ||
| > | ||
| > GitHub App authentication only scopes the token. It does not replace DIFC guard policy labels. When safe outputs are enabled, compile the workflow and confirm the lock file includes both `mcp_servers.github.guard-policies.allow-only` and `mcp_servers.safeoutputs.guard-policies.write-sink`. |
|
✅ PR Code Quality Reviewer completed the code quality review. 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
- "github.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has <=100 new lines of code in business logic directories.
|
|
✅ Test Quality Sentinel completed test quality analysis. 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.
|
|
✅ Ponytail Reviewer completed successfully! 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
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
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 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes
This fixes the source/sink policy gap for GitHub App workflows, but one of the user-facing validation instructions is still wrong for the default compiled lock format. The guidance needs to cover both JSON mcpServers locks and TOML mcp_servers locks, otherwise the exact mismatch behind this bug remains easy to miss.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 30.9 AIC · ⌖ 7.39 AIC · ⊞ 21.8K
Comment /review to run again
|
|
||
| > ⚠️ **Do NOT use `mode: remote`** in GitHub Actions workflows. Remote mode does not work with the GitHub Actions token (`GITHUB_TOKEN`) — it requires a special PAT or GitHub App token with MCP access. The default `mode: local` (Docker-based) works with `GITHUB_TOKEN` and should always be used. | ||
| > | ||
| > GitHub App authentication only scopes the token. It does not replace DIFC guard policy labels. When safe outputs are enabled, compile the workflow and confirm the lock file includes both `mcp_servers.github.guard-policies.allow-only` and `mcp_servers.safeoutputs.guard-policies.write-sink`. |
There was a problem hiding this comment.
The new doc check only mentions the TOML path (mcp_servers...), but this compiler also emits JSON lock configs under mcpServers. That leaves the primary generated format without the guidance you are adding here, so anyone validating a GitHub Actions workflow can still miss the exact source/sink policy pair this change is about.
💡 Why this matters and how to fix it
The JSON renderer is the default path for compiled lock files, and the existing review history already caught the same mismatch in this file. Update the wording so it tells authors to inspect both lock shapes, or describe the shared semantic check instead of a format-specific path. For example:
...confirm the generated lock file includes a GitHub `allow-only` guard policy and a safeoutputs `write-sink` guard policy (under `mcpServers` in JSON locks or `mcp_servers` in TOML locks).
That keeps the debugging guidance aligned with what the compiler actually emits across engines.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /grill-with-docs — requesting changes: the fix is incomplete for Codex/TOML workflows, and the doc guidance points at the wrong lock-file key for most engines.
📋 Key Themes & Highlights
Key Themes
- Incomplete root-cause fix:
deriveWriteSinkGuardPolicyFromWorkflowandRenderGitHubMCPcorrectly stop skipping guard policies for GitHub App workflows, butrenderGitHubTOML(Codex engine) never rendersexplicitGuardPoliciesor the automaticallow-only/write-sinkpolicies at all — so Codex + GitHub App workflows still ship without DIFC guard policies, reproducing the bug this PR is meant to fix. - Doc inaccuracy: the new guidance in
github-mcp-server.mdtells readers to inspectmcp_servers.github.guard-policies..., but that key only applies to Codex TOML output; JSON-based engines (the majority) usemcpServers.github.guard-policies.... - Both issues were already flagged by an existing (apparently unaddressed) reviewer pass on this PR — re-flagging here since they remain unresolved in the current diff.
Positive Highlights
- ✅ Good, targeted removal of the
hasGitHubAppshort-circuit in bothmcp_github_config.goandmcp_renderer_github.gofor the JSON path, with clear updated comments explaining the reasoning (auth scope ≠ DIFC label). - ✅ Solid new test coverage in
github_mcp_app_token_test.goasserting both source and sink policy shapes appear together in the JSON lock file. - ✅
non_github_mcp_guard_policy_test.goupdate correctly flips the old "no guard policy with GitHub App" expectation to the new intended behavior.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 28.1 AIC · ⌖ 14.9 AIC · ⊞ 10.3K
Comment /matt to run again
| } | ||
| } | ||
| shouldUseStepOutputForGuardPolicy := len(explicitGuardPolicies) == 0 && !hasGitHubApp(githubTool) | ||
| shouldUseStepOutputForGuardPolicy := len(explicitGuardPolicies) == 0 |
There was a problem hiding this comment.
[/diagnosing-bugs] The fix only reaches the JSON rendering path. renderGitHubTOML (Codex engine) never calls renderGitHubMCPGuardPolicies or receives GuardPolicies/GuardPoliciesFromStep at all — so for Codex-engine GitHub App workflows, neither the source nor sink guard policy is emitted, silently reproducing the exact bug this PR fixes for JSON engines.
💡 Why this matters
This mirrors an existing unresolved review comment (id 3928852995) on this same line from a previous pass — it appears not yet addressed. Since the PR title is "Generate DIFC policies for GitHub App workflows" and the new test (github_mcp_app_token_test.go) only asserts against the JSON lock content, the root cause (guard policy derivation being conditioned on hasGitHubApp) was fixed at the data layer but the TOML renderer was never wired to consume deriveWriteSinkGuardPolicyFromWorkflow/explicit guard policies at all — a pre-existing gap now more visible because GitHub App + Codex is a supported combination this PR claims to fix.
Suggested fix: thread guard-policy fields into renderGitHubTOML (and its TOML rendering helpers) the same way RenderGitHubMCP does for JSON, or explicitly document/test that Codex/TOML workflows are out of scope for this PR and file a follow-up issue.
@copilot please address this.
|
|
||
| > ⚠️ **Do NOT use `mode: remote`** in GitHub Actions workflows. Remote mode does not work with the GitHub Actions token (`GITHUB_TOKEN`) — it requires a special PAT or GitHub App token with MCP access. The default `mode: local` (Docker-based) works with `GITHUB_TOKEN` and should always be used. | ||
| > | ||
| > GitHub App authentication only scopes the token. It does not replace DIFC guard policy labels. When safe outputs are enabled, compile the workflow and confirm the lock file includes both `mcp_servers.github.guard-policies.allow-only` and `mcp_servers.safeoutputs.guard-policies.write-sink`. |
There was a problem hiding this comment.
[/grill-with-docs] The doc instructs readers to check mcp_servers.github.guard-policies... in the lock file, but per the existing (still-unresolved) reviewer comment (id 3928853038), JSON-based engine configs use the mcpServers key (see pkg/workflow/mcp_renderer.go:140) — only Codex TOML output uses mcp_servers. Since the majority of engines render JSON, this instruction will send most readers looking for the wrong path.
💡 Suggested fix
Clarify the two shapes explicitly, e.g.:
Inspect the generated lock file for
mcpServers.github.guard-policies(JSON-based engines) ormcp_servers.githubguard fields (Codex TOML).
@copilot please address this.
There was a problem hiding this comment.
A small bit of test-specific boilerplate can be folded into the existing guard-policy coverage, which keeps the suite easier to scan. net: -15 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #58302 · codex · mai10 · 6.02 AIC · ⌖ 10.7 AIC · ⊞ 13.5K
Comment /ponytail to run again
| // TestNonGitHubMCPServersGetGuardPoliciesWithGitHubApp verifies that non-GitHub MCP servers | ||
| // still get write-sink guard policies when a GitHub App is configured. GitHub App token scope | ||
| // is authentication, not a substitute for DIFC sink policy labels. | ||
| func TestNonGitHubMCPServersGetGuardPoliciesWithGitHubApp(t *testing.T) { |
There was a problem hiding this comment.
pkg/workflow/non_github_mcp_guard_policy_test.go:290: yagni: dedicated GitHub App test with a single case and duplicated expectation. Fold it into the existing table-driven guard-policy tests and keep one assertion path.
|
🎉 This pull request is included in a new release. Release: |
GitHub App authentication could suppress automatic DIFC policy generation, leaving guarded GitHub MCP sources paired with an unguarded/noop
safeoutputssink. That caused safe-output writes such as dispatch, noop, and incomplete reports to be denied while the workflow appeared successful.Policy generation
allow-onlyguard policies whenever no explicit policy is configured, including GitHub App workflows.safeoutputswrite-sinkpolicy for the same automatic-lockdown path.Behavior preserved
Coverage and guidance
Expected generated policy shape: