Conversation
- Replace PROXY_URL with command-line arguments for MCP target specification - Add backend package to handle both HTTP proxying and stdio MCP server execution - Integrate mcp-go library for MCP protocol support over stdio - Update Dockerfile to include Node.js, npm, Python, and uv for MCP server execution - Add graceful shutdown handling for all server components - Update documentation with new usage examples for stdio MCP servers
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for stdio MCP (Model Context Protocol) servers alongside the existing HTTP proxy functionality. The change enables the proxy to launch and communicate with MCP servers via stdin/stdout instead of only proxying to existing HTTP endpoints.
Key changes include:
- Replaced
PROXY_URLenvironment variable/flag with command-line arguments for flexible target specification - Added new backend package to handle both HTTP proxying and stdio MCP server execution
- Integrated graceful shutdown handling for all server components
Reviewed Changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/proxy/main.go | Updated ProxyRouter to accept generic http.Handler instead of URL string |
| pkg/mcp-proxy/main.go | Added backend support, graceful shutdown, and command-line argument handling |
| pkg/backend/main.go | New package implementing stdio MCP server execution and HTTP proxy bridge |
| main.go | Removed PROXY_URL flag in favor of command-line arguments |
| go.mod | Added mcp-go dependency and updated related packages |
| README.md | Updated documentation with new usage examples for both HTTP and stdio targets |
| Dockerfile | Added Node.js, npm, Python, and uv for MCP server execution |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| handler = _handler | ||
| done <- struct{}{} | ||
| }() | ||
| select { |
There was a problem hiding this comment.
The select statement doesn't handle a timeout case. If the setupProxy goroutine hangs indefinitely, this could block forever. Consider adding a timeout using MCPClientInitTimeout or context.WithTimeout.
| go func() { | ||
| defer wg.Done() | ||
| if be != nil { | ||
| if err := be.Wait(); err != nil && ctx.Err() != context.Canceled { |
There was a problem hiding this comment.
The condition ctx.Err() != context.Canceled should check against context.Canceled directly or use errors.Is(ctx.Err(), context.Canceled) for proper error comparison.
| if err := be.Wait(); err != nil && ctx.Err() != context.Canceled { | |
| if err := be.Wait(); err != nil && !errors.Is(ctx.Err(), context.Canceled) { |
| exit <- struct{}{} | ||
| } | ||
| }() | ||
|
|
There was a problem hiding this comment.
This goroutine only executes when be != nil, but it's always added to the WaitGroup count of 3. This could cause the WaitGroup to wait indefinitely if be is nil. Consider conditionally adding to WaitGroup or restructuring the logic.
| if be != nil { | |
| wg.Add(1) | |
| go func() { | |
| defer wg.Done() | |
| if err := be.Wait(); err != nil && ctx.Err() != context.Canceled { | |
| lock.Lock() | |
| errs = append(errs, err) | |
| lock.Unlock() | |
| } | |
| exit <- struct{}{} | |
| }() | |
| } |
Summary
Type of Change
Related Issues