gemini: use topology hostname for MCP_GATEWAY_HOST_DOMAIN under network isolation - #48775
Conversation
…der network isolation When network isolation is active and the engine is Gemini, set MCP_GATEWAY_HOST_DOMAIN to the topology hostname (awmg-mcpg) instead of localhost. The Gemini CLI honors HTTP_PROXY but ignores NO_PROXY, causing localhost:8080 to be tunneled through the squid egress proxy which denies it (60+ TCP_DENIED per run). The awmg-mcpg topology hostname is already in the firewall allowlist. Closes #48751 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes Gemini MCP gateway routing under network isolation by using the allowlisted topology hostname instead of proxied loopback traffic.
Changes:
- Selects
awmg-mcpgfor isolated Gemini workflows. - Adds integration coverage for the routing behavior.
- Recompiles the Gemini smoke workflow.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/mcp_setup_gateway.go |
Adds Gemini-specific isolated routing. |
pkg/workflow/mcp_setup_generator_test.go |
Verifies topology-hostname generation. |
.github/workflows/smoke-gemini.lock.yml |
Applies the generated hostname change. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Medium
| } else if engine.GetID() == "gemini" && isAWFNetworkIsolationEnabled(workflowData) { | ||
| // domain is "awmg-mcpg" when network isolation is active; preserve it. | ||
| hostDomain = domain |
|
/smoke-gemini |
|
🚀 Smoke Gemini MISSION COMPLETE! Gemini has spoken. ✨ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #48775 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (51 additions detected, threshold is 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and well-targeted.
- The new
gemini+ network-isolation branch inwriteMCPGatewayExportscorrectly intercepts before the genericlocalhostoverride, preservingawmg-mcpgasMCP_GATEWAY_HOST_DOMAIN. - The shell script already dereferences
$MCP_GATEWAY_HOST_DOMAINdynamically, so no shell-side changes are needed. - New test covers the regression path clearly.
- Lock file recompile is consistent.
The existing Copilot review comment about stale inline docs in the shell script is a minor nit — not blocking.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.1 AIC · ⌖ 5.68 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting with one improvement suggestion.
📋 Key Themes & Highlights
Key Themes
- Root cause addressed correctly: The fix targets the real issue (Gemini honoring
HTTP_PROXYbut ignoringNO_PROXY) rather than a workaround. Using the topology hostname that is already in the squid allowlist is the right approach. - Test coverage gap: The new test covers the network-isolation case but leaves the non-isolated Gemini path untested — a counterpart test would complete the specification.
Positive Highlights
- ✅ Conditional is narrow and precise: only triggers for
engine == "gemini" && networkIsolationEnabled; no other paths affected - ✅ Excellent inline comments explaining the
HTTP_PROXY/NO_PROXYasymmetry - ✅ Integration test uses
require.NotContainsas well asrequire.Contains— both sides of the assertion are covered for the isolation case - ✅ Lock file recompiled and included in the PR
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 32.5 AIC · ⌖ 4.81 AIC · ⊞ 6.7K
Comment /matt to run again
| require.Contains(t, yamlStr, `export MCP_GATEWAY_HOST_DOMAIN="awmg-mcpg"`, | ||
| "Gemini MCP_GATEWAY_HOST_DOMAIN must use awmg-mcpg under network isolation so the Gemini CLI does not tunnel localhost through the squid egress proxy") | ||
| require.NotContains(t, yamlStr, `export MCP_GATEWAY_HOST_DOMAIN="localhost"`, | ||
| "Gemini MCP_GATEWAY_HOST_DOMAIN must not be localhost under network isolation") |
There was a problem hiding this comment.
[/tdd] Missing counterpart test: Gemini without network isolation should still resolve MCP_GATEWAY_HOST_DOMAIN to localhost — there is no test asserting this.
💡 Suggested test
Adding a test for the non-isolated Gemini path guards against a future refactor accidentally routing non-isolated runs through the topology hostname.
func TestMCPGatewayDockerCommandGeminiWithoutNetworkIsolationUsesLocalhost(t *testing.T) {
frontmatter := `---
on: workflow_dispatch
engine: gemini
sandbox:
agent:
sudo: false
tools:
github:
mode: remote
toolsets: [repos]
---
# Test Gemini Without Network Isolation
`
compiler := NewCompiler()
tmpDir := t.TempDir()
inputFile := filepath.Join(tmpDir, "test.md")
err := os.WriteFile(inputFile, []byte(frontmatter), 0644)
require.NoError(t, err)
err = compiler.CompileWorkflow(inputFile)
require.NoError(t, err)
outputFile := stringutil.MarkdownToLockFile(inputFile)
content, err := os.ReadFile(outputFile)
require.NoError(t, err)
yamlStr := string(content)
require.Contains(t, yamlStr, `export MCP_GATEWAY_HOST_DOMAIN="localhost"`,
"Gemini MCP_GATEWAY_HOST_DOMAIN must be localhost when network isolation is NOT active")
}@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (1 test)
Verdict
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
REQUEST_CHANGES — the core fix direction is reasonable, but leaves a stale companion shell script that contradicts the new behavior and lacks test coverage for adjacent branches (docker-sbx+gemini, non-isolated gemini, other engines under isolation).
Themes
- Blocking:
actions/setup/sh/convert_gateway_config_gemini.shstill documents/defaultsMCP_GATEWAY_HOST_DOMAINtolocalhostfor Gemini, directly contradicting the newawmg-mcpgexception added in Go. This is a known, previously-flagged (unresolved) inconsistency (Copilot review comment id 3671570844) that risks silently reintroducing the exactTCP_DENIEDbug this PR aims to fix if the env var handling ever diverges. - Test coverage is one-sided: only the new "happy path" (Gemini + isolation →
awmg-mcpg) is asserted. No test proves the branch ordering againstisDockerSbxRuntime, that non-isolated Gemini still getslocalhost, or that other engines are unaffected. - Minor: the new condition uses a bare
"gemini"string literal rather than theconstants.GeminiEngineconstant already used elsewhere in the codebase for the same purpose.
🔎 Code quality review by PR Code Quality Reviewer · sonnet50 · 94.2 AIC · ⌖ 4.68 AIC · ⊞ 7.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/mcp_setup_gateway.go:163
This exception fixes the Go side but leaves convert_gateway_config_gemini.sh untouched, whose comments and fallback default still assume localhost — silently reintroducing the exact TCP_DENIED bug if the env var is ever unset.
<details>
<summary>💡 Stale companion script contradicts this fix</summary>
actions/setup/sh/convert_gateway_config_gemini.sh (not part of this diff) still says:
# Optional environment variables:
# - MCP_GATEWAY_HOST_DOMAIN: Host-side domain for Gemini MCP …
</details>
<details><summary>pkg/workflow/mcp_setup_generator_test.go:84</summary>
Missing regression tests leave the branch ordering (docker-sbx > gemini-isolation > generic-isolation) and non-Gemini engines unverified.
<details>
<summary>💡 Gaps in test coverage for the new branch</summary>
The new `else if engine.GetID() == "gemini" && isAWFNetworkIsolationEnabled(...)` branch sits between the `isDockerSbxRuntime` check and the generic `isAWFNetworkIsolationEnabled` fallback, but no test in this PR covers:
1. Gemini + docker-sbx combined (does docker-sbx correctly still…
</details>
<details><summary>pkg/workflow/mcp_setup_gateway.go:163</summary>
`engine.GetID() == "gemini"` uses a bare string literal instead of the existing `constants.GeminiEngine` constant already used elsewhere in this same package.
<details>
<summary>💡 Fragile magic-string engine comparison</summary>
`pkg/workflow/gemini_engine.go` (in the very call chain that reaches this function) already uses `constants.GeminiEngine` for engine-name comparisons, e.g. `GetAllowedDomainsForEngine(constants.GeminiEngine, ...)`. Comparing against the bare string `"gemini"` here in…
</details>
|
@copilot |
TriageCategory: bug | Risk: medium | Score: 72/100
Recommended action:
|
…ork isolation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit
Local validation: |
|
/smoke-gemini |
|
🚀 Smoke Gemini MISSION COMPLETE! Gemini has spoken. ✨ |
Smoke Test Results
|
|
🎉 This pull request is included in a new release. Release: |
Under network isolation, the Gemini CLI was routing MCP gateway connections (
localhost:8080) through the squid egress proxy — generating 60–72TCP_DENIEDfirewall entries per run. Root cause: Gemini honorsHTTP_PROXYbut ignoresNO_PROXY, so loopback traffic is still proxied.Changes
pkg/workflow/mcp_setup_gateway.go— InwriteMCPGatewayExports, add a Gemini-specific branch: whenisAWFNetworkIsolationEnabledandengine.GetID() == "gemini", setMCP_GATEWAY_HOST_DOMAINto the resolveddomain(awmg-mcpg) rather thanlocalhost. Theawmg-mcpgtopology hostname is already in the squid allowlist via the auto-allow-topology-hostnames ACL.pkg/workflow/mcp_setup_generator_test.go— New integration testTestMCPGatewayDockerCommandGeminiNetworkIsolationUsesTopologyHostnameassertingMCP_GATEWAY_HOST_DOMAIN="awmg-mcpg"(notlocalhost) for Gemini under network isolation..github/workflows/smoke-gemini.lock.yml— Recompiled; now emitsawmg-mcpgforMCP_GATEWAY_HOST_DOMAIN.