fix(compiler): normalize gateway policy server IDs - #58023
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1ad74329-b8a6-4ad4-b378-805e3fbfc0ce
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1ad74329-b8a6-4ad4-b378-805e3fbfc0ce
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused normalization fix is consistent with MCP rendering and has targeted unit and compilation coverage.
Review tier: Balanced
Findings: None
What changed in this PR
Fixes shared-gateway policies to reference normalized MCP server IDs.
Changes:
- Derives primary policy servers from normalized manifest entries.
- Adds regression coverage for
safeoutputsand policy references.
| File | Description |
|---|---|
pkg/workflow/mcp_gateway_config.go |
Uses normalized generated server names in policies. |
pkg/workflow/enclave_github_proxy_test.go |
Verifies policy resolution and compiled safe-output naming. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1ad74329-b8a6-4ad4-b378-805e3fbfc0ce
|
@copilot Please review the current PR state, refresh the branch if needed, and run the
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.
|
|
/review |
|
/matt |
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This patch fixes the original gateway wiring regressions, but it also makes the enclave backend explicitly optional for every generated config. That trades a deterministic startup contract for a silent fallback: if the AWF-hosted enclave never comes up, the gateway can still initialize and the primary agent will only fail later when it first touches an enclave tool.
Blocking theme
The new "required": false on awf-enclave removes the only hard startup check for the deferred enclave path. The deferred-server handoff already prevents eager connection attempts; marking the backend optional as well means a broken readiness export, wrong capability header, or dead enclave process no longer fails fast during setup. The workflow will instead proceed into agent execution and blow up at tool-use time with a much less diagnosable error.
That is a correctness regression in the startup contract, not just a logging nuance. Keep the deferred connection behavior, but preserve a positive readiness assertion before handing control to the agent.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 36 AIC · ⌖ 7.34 AIC · ⊞ 23.5K
Comment /review to run again
| fmt.Fprintf(yaml, " %q: {\n", enclaveMCPServerName) | ||
| yaml.WriteString(" \"type\": \"http\",\n") | ||
| fmt.Fprintf(yaml, " \"url\": %q,\n", enclaveMCPUpstreamURL) | ||
| yaml.WriteString(" \"required\": false,\n") |
There was a problem hiding this comment.
Making awf-enclave globally optional removes the fail-fast check for the deferred enclave path, so a broken enclave startup now escapes setup and only crashes later when the first enclave tool is invoked.
💡 Why this blocks
The deferred-server export already solves the eager-connect problem; adding "required": false on top changes behavior again by telling the gateway that missing enclave connectivity is acceptable. That hides real startup defects such as a dead enclave process, bad capability header, or missing readiness handoff, and turns them into harder-to-debug runtime failures during agent execution.
A safer fix is to preserve deferred startup while keeping an explicit readiness assertion before the agent step begins. If the gateway truly must tolerate a deferred backend here, the generated workflow needs a separate check that proves the enclave endpoint became reachable before any enclave tool can be used.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /diagnosing-bugs to this multi-file gateway/enclave handoff fix.
📋 Key Themes & Highlights
Key Themes
- Engine parity gap:
writeEnclaveMCPJSONgainedrequired: falsefor the deferredawf-enclavebackend, but the parallel Codex TOML writer (writeEnclaveMCPTOML) did not, leaving the same eager-connection failure this PR fixes unaddressed for Codex-engine workflows using enclaves. No test exercises this TOML path. - Naming clarity:
gateway-api-keyoutput is set to the same value asgateway-agent-id(agentId) rather than a distinct credential — worth a comment or rename to avoid confusion about whether a real API key is being minted.
Positive Highlights
- ✅ Good root-cause fix for the
agentPoliciesreferencing unknown server IDs — now driven fromcollectMCPServersForManifestinstead of a separate tool-name list, closing the drift between manifest and policy. - ✅ Solid, targeted regression tests added for the shared-gateway compile path (
TestCompileEnclaveGitHubSharedGateway,TestToolsWithEnclaveGitHubIssuesUnionsTypedToolsets) that assert both the fixed behavior and non-regression (NotContainson the old broken values). - ✅
MCP_GATEWAY_API_KEYcorrectly added to both the AWF exclude-env list and the copilot execution step's filtered env, keeping the primary agent's visible environment minimal per the stated security goal.
See inline comments for the two actionable items.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 111 AIC · ⌖ 15.1 AIC · ⊞ 10.3K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/enclaves.go:366
[/codebase-design] writeEnclaveMCPTOML (Codex engine path) doesn't get the required = false field that writeEnclaveMCPJSON gained in this PR — the deferred/optional-backend fix is engine-specific and silently absent for Codex.
<details>
<summary>💡 Details</summary>
The PR's goal is to mark the delayed awf-enclave HTTP backend optional so eager connection failures (seen in run 33676040801) don't abort startup. writeEnclaveMCPJSON was updated with `yaml.WriteString(" …
actions/setup/js/start_mcp_gateway.cjs:1359
[/diagnosing-bugs] gateway-api-key is set to the same value as gateway-agent-id (agentId) — this isn't really an "API key", it's the existing agent ID reused under a new name. If that's intentional (agent ID doubles as bearer credential), a short comment would help; if a distinct secret was meant to be minted here, this silently reuses a non-secret identifier as the credential handed to the AWF enclave container.
<details>
<summary>💡 Context</summary>
The PR's stated goal is: "hand…
Ran the PR finisher pass. The branch is current and clean at 706f86a; no formatter or source changes were needed. |
|
Hey @lpcox 👋 — thanks for fixing the gateway policy server ID normalization! This PR looks ready for review: ✅ Well-focused — all changes stay within the MCP gateway initialization and policy configuration boundary, touching only the compiler and setup action infrastructure. ✅ Strong test coverage — includes regression tests for startup scripts, enclave GitHub proxy configuration, and compiled workflow ordering. ✅ Clear context — references specific firewall smoke run failures with links and explains the isolation-boundary approach (primary gets ✅ Comprehensive validation — detailed test commands with run results demonstrate the fixes address the exposed defects. This looks good for maintainer review. The changes are mechanically sound and the test additions show confidence in the fix. 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.
|
|
@copilot Please address the current blocking review feedback, refresh the branch if needed, and run the
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.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in commit |
|
@copilot Please verify the current head against the remaining open review state and close out any still-applicable follow-up before handing back to maintainers.
Please run the
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.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Latest head is reconciled. The startup-contract review is superseded by |
|
@lpcox shouldn't it be optional?
|
not required if there are no enclaves, but required network isolation when enclaves are enabled. |
|
@copilot fix this failing ci check https://github.com/github/gh-aw/actions/runs/33692826660/job/100456290392?pr=58023 |
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Fixed in commit |
|
🎉 This pull request is included in a new release. Release: |
Summary
safeoutputsinstead of the frontmatter descriptorsafe-outputsawf-enclaveHTTP backend optional while preserving deferred-server readiness handoffissues-read-v1GitHub toolset requirement into the shared GitHub MCP server without broadening primary identity policyContext
This fixes failures exposed by the gh-aw-firewall shared-enclave smoke runs after #57973:
agentPoliciesreferenced unknown serversafe-outputscontext-only serverMCP_GATEWAY_API_KEYwas absentThe generated shared gateway now uses actual MCP server IDs, treats the AWF-hosted enclave backend as deferred/optional during gateway initialization, and registers the
issuestoolset needed by the enclave. Identity policy remains the isolation boundary: the primary gets onlyget_me, while the enclave gets onlylist_issuesandissue_read. The startup action exports agateway-api-keyalias, and enclave execution passes it to the AWF wrapper while--exclude-env MCP_GATEWAY_API_KEYkeeps it out of the primary agent.Validation
go test ./pkg/workflow -run 'Test(GenerateEnclaveGatewayContract|CompileEnclaveStartupOrdering|ToolsWithEnclaveGitHubIssuesUnionsTypedToolsets|EnclaveGitHubMCPGatewayConfiguration|CompileEnclaveGitHubSharedGateway)$' -count=1go test ./pkg/workflow -run 'Test(CompileEnclaveGitHubSharedGateway|ComputeAWFExcludeEnvVarNames)$' -count=1npx vitest run actions/setup/js/start_mcp_gateway.test.cjsbash actions/setup/sh/start_mcp_gateway_test.shmake agent-report-progressRun: https://github.com/github/gh-aw/actions/runs/33679827993
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
PR Sous Chef branch refresh requested. Run: https://github.com/github/gh-aw/actions/runs/33682167508
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.