feat: add MCP file support for ACP server configuration#214
feat: add MCP file support for ACP server configuration#214
Conversation
|
✅ Preview binaries are ready! To test with modules: |
There was a problem hiding this comment.
Pull request overview
Adds support for supplying MCP server configuration via a JSON file when running the server in experimental ACP transport mode, and passes the filtered MCP server list into the ACP session initialization.
Changes:
- Add
--mcp-fileserver flag (and env var support via Viper) gated behind--experimental-acp. - Load and filter MCP servers from a JSON file based on agent MCP capabilities, and send them in
NewSession. - Add unit/integration tests for MCP config loading/filtering and flag validation behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| x/acpio/acpio.go | Loads MCP config from file, filters unsupported server types, and passes supported MCPs to ACP NewSession. |
| x/acpio/mcp_internal_test.go | Adds internal unit tests for MCP config loading and capability-based filtering. |
| x/acpio/acpio_test.go | Updates NewWithPipes test helper call for new MCP file path parameter. |
| lib/httpapi/setup.go | Plumbs MCP file path through SetupACPConfig into acpio.NewWithPipes. |
| cmd/server/server.go | Introduces --mcp-file flag, validates it requires --experimental-acp, and passes it into ACP setup. |
| cmd/server/server_test.go | Adds tests asserting the new flag can be set via default/CLI/env var and precedence rules. |
| e2e/echo_test.go | Adds integration coverage ensuring --mcp-file fails without --experimental-acp. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| defer func() { | ||
| if closeErr := mcpFile.Close(); closeErr != nil { | ||
| logger.Error("Failed to close mcp file", "error", err) |
There was a problem hiding this comment.
In the deferred Close error path, the logger uses the outer err variable instead of closeErr, so the logged error will be wrong (often nil) when Close() fails. Log closeErr and include the file path for easier debugging.
| logger.Error("Failed to close mcp file", "error", err) | |
| logger.Error("Failed to close mcp file", "path", mcpFilePath, "error", closeErr) |
| mcpFile, err := os.Open(mcpFilePath) | ||
| if err != nil { | ||
| return nil, xerrors.Errorf("Failed to open mcp file: %v", err) | ||
| } |
There was a problem hiding this comment.
The open error is formatted with %v, which loses error unwrapping. Prefer %w (e.g., xerrors.Errorf("Failed to open mcp file: %w", err)) so callers can inspect the underlying cause via errors.Is/As.
| if err = decoder.Decode(&allMcpList); err != nil { | ||
| return nil, xerrors.Errorf("Failed to decode mcp file: %v", err) | ||
| } |
There was a problem hiding this comment.
The decode error is formatted with %v, which loses error unwrapping. Prefer %w (e.g., xerrors.Errorf("Failed to decode mcp file: %w", err)) so callers can inspect the underlying cause via errors.Is/As.
| acpMCPFile := viper.GetString(FlagMCPFile) | ||
|
|
||
| if acpMCPFile != "" && !experimentalACP { | ||
| return xerrors.Errorf("--mcp-file requires --experimental-acp requires to be set") |
There was a problem hiding this comment.
The error message returned when --mcp-file is used without --experimental-acp has duplicated wording ("requires" twice / "requires to be set"). Consider simplifying it to a single clear sentence (e.g., "--mcp-file requires --experimental-acp") to improve UX and keep messages consistent with other flag validations.
| return xerrors.Errorf("--mcp-file requires --experimental-acp requires to be set") | |
| return xerrors.Errorf("--mcp-file requires --experimental-acp") |
No description provided.