feat(mcp): support host-local MCP servers#183
Conversation
📦 Previous Review (superseded by newer review)Claude finished @dpup's task in 2m 5s —— View job Code Review
Overall this is a clean, well-tested implementation. The proxy relay pattern for bridging host-local MCP servers is sound, the validation is thorough, and the test coverage is extensive. A few things worth addressing: Issues1. var mcpRelayClient = &http.Client{
Transport: &http.Transport{
Proxy: nil,
},
}There's no 2. Test duplication between
They test the same code paths with slightly different names. Not a correctness issue, but the duplication will grow harder to maintain as the relay evolves. 3. Partial overlap in
4. if !strings.HasPrefix(spec.URL, "https://") {
if !isHostLocalURL(spec.URL) {
return fmt.Errorf("... http:// is only allowed for localhost and 127.0.0.1")
}
}The error message says "localhost and 127.0.0.1" but the function also accepts Observations (no action required)Manager change is correct. Removing the
Security model is sound. Host-local URLs are validated at config load time, not at request time. The relay uses a Docs are accurate and match the implementation. The guide correctly describes the three server types and the proxy relay mechanism. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
📦 Previous Review (superseded by newer review)Claude finished @dpup's task in 3m 8s —— View job Code Review
The follow-up commit ( Issues1. // Strip port
host := hostPart
if idx := strings.LastIndexByte(host, ':'); idx >= 0 {
host = host[:idx]
}
return host == "localhost" || host == "127.0.0.1" || host == "[::1]"For This case isn't in the test suite: 2. Code comments still say "localhost or 127.0.0.1" after the The error message was updated but the doc comments weren't: // MCPServerConfig:
// Host-local servers (http://localhost or http://127.0.0.1) are reached
// isHostLocalURL:
// (localhost or 127.0.0.1). These are MCP servers running on the host machineBoth should mention Observations (no action required)Test duplication is still present.
Security model holds. The relay client uses Docs are accurate. The guide and reference correctly describe all three server types, the |
📦 Previous Review (superseded by newer review)Claude finished @dpup's task in 3m 3s —— View job Code Review
Previous reviews flagged three issues. Here's the status of each, plus anything new. Previously Flagged — Now FixedIPv6 loopback without port ( Error message and comments missing
Still Outstanding1. Test duplication across Three reviews have flagged this and it remains unaddressed. The following tests are functionally identical:
Not a correctness issue but the duplication will compound as the relay grows. The regression file was presumably created to lock in behavior from a bug fix — the new file duplicates it without adding coverage. 2. No timeout on First flagged in the initial review and not changed. It's the same as the existing behavior for remote MCP servers, so not a regression, but a completely unresponsive host-local server will block a relay goroutine indefinitely. The context propagation via Observations (no action required)
Security model is sound. URL validation happens at config load time. The relay uses Docs are accurate. The guide correctly describes all three server types, the proxy relay mechanism, and troubleshooting steps for host-local servers specifically. Overall the implementation is solid. The IPv6 and comment issues from the last review are resolved. The remaining items (test duplication, relay timeout) are pre-existing concerns rather than new problems introduced by this PR. |
Enable MCP servers running on the host machine (localhost, 127.0.0.1, [::1]) to be reached from inside containers via the proxy relay. - Add isHostLocalURL() using net/url.Parse for loopback detection - Relax HTTPS validation to allow HTTP for host-local URLs - Remove Auth==nil skip so unauthenticated servers get relay URLs - Add proxy relay tests for host-local with/without auth - Update MCP guide and moat.yaml reference docs Closes #69
dpup
force-pushed
the
feat/69-host-local-mcp
branch
from
c80707c to
08f1550
Compare
📦 Previous Review (superseded by newer review)Claude finished @dpup's task in 3m 43s —— View job Code Review
Three previous reviews flagged issues in earlier iterations. Here's the status of outstanding items and a fresh look at the current state of the code. Previously Flagged — Now ResolvedIPv6 without port ( Error message and doc comments missing Test duplication ( Still OutstandingNo timeout on var mcpRelayClient = &http.Client{
Transport: &http.Transport{
Proxy: nil,
},
}A host-local MCP server that accepts a connection but never responds will block the relay goroutine indefinitely. The New Issues
Docs observability section implies host-local traffic isn't traced ( Host-local traffic also flows through the proxy relay — the relay is the only path from container to host-local server. "All remote MCP traffic" is true but implies host-local is excluded. A reader who just set up a host-local server might wonder why they're seeing proxy traces for it. Fix this → Observations (no action required)
Security model is correct. URL validation happens at config parse time. The relay uses
Overall this is solid. The previous blocking issues are all resolved. The two new items above are minor — the missing timeout is a pre-existing limitation, and the docs/test issues are clarity improvements rather than correctness problems. |
- Add comment clarifying TestMCPCredentialInjection tests the proxy-interception path, not the relay path - Update observability docs to note both remote and host-local MCP traffic flows through the proxy relay
|
Claude finished @dpup's task in 3m 17s —— View job Code Review
All issues from the three prior rounds of review have been addressed. Here's a final accounting. Previously Flagged — Now FixedIPv6 loopback without port — Fixed. Error message and doc comments missing Test duplication — Resolved.
Docs observability section implied host-local traffic isn't traced — Fixed. Line 297 now reads: "All MCP traffic (both remote and host-local) flows through the proxy relay, so it appears in network traces." Still OutstandingNo timeout on var mcpRelayClient = &http.Client{
Transport: &http.Transport{
Proxy: nil,
},
}This has been flagged in every review. A host-local server that accepts a TCP connection but never sends a response will hold a relay goroutine open indefinitely. This is a pre-existing condition for remote MCP servers too. The practical difference here is that a local server in a bad state (crash loop, blocked goroutine, zombie process) is a common scenario, whereas an unreachable remote endpoint typically fails fast with a connection refused or timeout. Worth noting since host-local is now a first-class feature. Fix this → Observations (no action required)
Security model is sound. URL validation happens at config parse time. The relay uses The implementation is solid. The one remaining item (relay timeout) is a pre-existing limitation. Everything else from prior reviews is resolved. |
1 participant
Summary
Closes #69 — enables agents in containers to communicate with MCP servers running on the host machine.
localhost,127.0.0.1, or[::1]are automatically detected as host-localurl.Parse+Hostname()with support for IPv4/IPv6 loopback addressesUsage in moat.yaml
Test plan
TestLoad_MCP_HostLocal— host-local config parsingTestLoad_MCP_HostLocal_Validation— validation rulesTestIsHostLocalURL— 12 subtests (localhost, 127.0.0.1, [::1], remote hosts)TestLoad_MCP_MixedHostAndRemote— mixed host + remote configsTestLoad_MCP_HostLocalIPv6— IPv6 loopback supportTestIsHostLocalURL_EdgeCases— 11 edge casesTestMCPRelay_HostLocal— proxy relay for host-localTestMCPRelay_HostLocalWithPath/WithAuth— path and auth forwardinggo test -race— no races,make lint— 0 issuesReview notes
agent.yaml→moat.yamlrename in feat(config): rename agent.yaml to moat.yaml #204)isHostLocalURLusesurl.Parse+Hostname()instead of manual string parsinghandleMCPRelaypathAuth == nilskip in manager.go)