Add MCP server support: events and session management methods#29
Add MCP server support: events and session management methods#29
Conversation
Co-authored-by: brunoborges <129743+brunoborges@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/copilot-sdk-java/sessions/4f9a9fb9-2883-4dd6-843d-2e4a25ddd7e0
There was a problem hiding this comment.
Pull request overview
Adds MCP (Model Context Protocol) server lifecycle support to the Java SDK, bringing it closer to upstream parity by exposing MCP-related session events and runtime management APIs on CopilotSession.
Changes:
- Added new session event types for MCP server initialization and status transitions, and registered them in the event parser/sealed hierarchy.
- Introduced
McpServerInfoDTO for MCP server listing/status reporting. - Added
CopilotSessionAPIs to list/enable/disable/reload MCP servers, plus termination-guard tests and event parsing tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/github/copilot/sdk/CopilotSession.java | Adds MCP server management RPC wrappers (session.mcp.*). |
| src/main/java/com/github/copilot/sdk/events/AbstractSessionEvent.java | Extends sealed permits list to include new MCP events. |
| src/main/java/com/github/copilot/sdk/events/SessionEventParser.java | Registers new MCP event types for polymorphic parsing. |
| src/main/java/com/github/copilot/sdk/events/SessionMcpServersLoadedEvent.java | New event type for MCP servers loaded. |
| src/main/java/com/github/copilot/sdk/events/SessionMcpServerStatusChangedEvent.java | New event type for MCP server status changes. |
| src/main/java/com/github/copilot/sdk/events/package-info.java | Documents MCP server events in events package overview. |
| src/main/java/com/github/copilot/sdk/json/McpServerInfo.java | New DTO describing MCP server name/status/source/error. |
| src/test/java/com/github/copilot/sdk/SessionEventParserTest.java | Adds parsing tests for the new MCP session events. |
| src/test/java/com/github/copilot/sdk/ClosedSessionGuardTest.java | Adds termination-guard tests for new MCP session APIs. |
| * {@link com.github.copilot.sdk.events.SessionMcpServersLoadedEvent} and | ||
| * {@link com.github.copilot.sdk.events.SessionMcpServerStatusChangedEvent}. | ||
| * | ||
| * @since 1.0.0 |
There was a problem hiding this comment.
The Javadoc says McpServerInfo is carried in SessionMcpServerStatusChangedEvent, but that event’s payload only includes serverName + status (no McpServerInfo). Please update the Javadoc to avoid misleading API docs. Also consider updating the @since tag to the version that actually introduces this DTO (it’s newly added in this PR).
| * {@link com.github.copilot.sdk.events.SessionMcpServersLoadedEvent} and | |
| * {@link com.github.copilot.sdk.events.SessionMcpServerStatusChangedEvent}. | |
| * | |
| * @since 1.0.0 | |
| * {@link com.github.copilot.sdk.events.SessionMcpServersLoadedEvent}. | |
| * <p> | |
| * Related: | |
| * {@link com.github.copilot.sdk.events.SessionMcpServerStatusChangedEvent} | |
| * reports status changes using the server name and status fields, rather than | |
| * this DTO. |
| public static class SessionMcpServersLoadedData { | ||
|
|
||
| @JsonProperty("servers") | ||
| private List<McpServerInfo> servers; | ||
|
|
||
| /** | ||
| * Gets the list of MCP server status summaries. | ||
| * | ||
| * @return the servers list | ||
| */ | ||
| public List<McpServerInfo> getServers() { | ||
| return servers; | ||
| } | ||
|
|
||
| /** | ||
| * Sets the list of MCP server status summaries. | ||
| * | ||
| * @param servers | ||
| * the servers list | ||
| */ | ||
| public void setServers(List<McpServerInfo> servers) { | ||
| this.servers = servers; | ||
| } |
There was a problem hiding this comment.
All other event types in this package model their data payload as a record (e.g., SessionTaskCompleteEvent) for immutability and consistency with the rest of the event API. Consider changing SessionMcpServersLoadedData from a mutable class with setters to a record to match the established pattern.
| public static class SessionMcpServersLoadedData { | |
| @JsonProperty("servers") | |
| private List<McpServerInfo> servers; | |
| /** | |
| * Gets the list of MCP server status summaries. | |
| * | |
| * @return the servers list | |
| */ | |
| public List<McpServerInfo> getServers() { | |
| return servers; | |
| } | |
| /** | |
| * Sets the list of MCP server status summaries. | |
| * | |
| * @param servers | |
| * the servers list | |
| */ | |
| public void setServers(List<McpServerInfo> servers) { | |
| this.servers = servers; | |
| } | |
| public static record SessionMcpServersLoadedData( | |
| @JsonProperty("servers") List<McpServerInfo> servers) { |
| public static class SessionMcpServerStatusChangedData { | ||
|
|
||
| @JsonProperty("serverName") | ||
| private String serverName; | ||
|
|
||
| @JsonProperty("status") | ||
| private String status; | ||
|
|
||
| /** | ||
| * Gets the name of the MCP server whose status changed. | ||
| * | ||
| * @return the server name | ||
| */ | ||
| public String getServerName() { | ||
| return serverName; | ||
| } | ||
|
|
||
| /** | ||
| * Sets the server name. | ||
| * | ||
| * @param serverName | ||
| * the server name | ||
| */ | ||
| public void setServerName(String serverName) { | ||
| this.serverName = serverName; | ||
| } | ||
|
|
||
| /** | ||
| * Gets the new connection status. | ||
| * <p> | ||
| * Possible values: {@code "connected"}, {@code "failed"}, {@code "pending"}, | ||
| * {@code "disabled"}, {@code "not_configured"}. | ||
| * | ||
| * @return the status string | ||
| */ | ||
| public String getStatus() { | ||
| return status; | ||
| } | ||
|
|
||
| /** | ||
| * Sets the new connection status. | ||
| * | ||
| * @param status | ||
| * the status string | ||
| */ | ||
| public void setStatus(String status) { | ||
| this.status = status; | ||
| } |
There was a problem hiding this comment.
This event’s data payload is implemented as a mutable inner class with setters, but the rest of the events package consistently uses record payloads for data. Switching SessionMcpServerStatusChangedData to a record would improve consistency and reduce mutability in the public event model.
| public static class SessionMcpServerStatusChangedData { | |
| @JsonProperty("serverName") | |
| private String serverName; | |
| @JsonProperty("status") | |
| private String status; | |
| /** | |
| * Gets the name of the MCP server whose status changed. | |
| * | |
| * @return the server name | |
| */ | |
| public String getServerName() { | |
| return serverName; | |
| } | |
| /** | |
| * Sets the server name. | |
| * | |
| * @param serverName | |
| * the server name | |
| */ | |
| public void setServerName(String serverName) { | |
| this.serverName = serverName; | |
| } | |
| /** | |
| * Gets the new connection status. | |
| * <p> | |
| * Possible values: {@code "connected"}, {@code "failed"}, {@code "pending"}, | |
| * {@code "disabled"}, {@code "not_configured"}. | |
| * | |
| * @return the status string | |
| */ | |
| public String getStatus() { | |
| return status; | |
| } | |
| /** | |
| * Sets the new connection status. | |
| * | |
| * @param status | |
| * the status string | |
| */ | |
| public void setStatus(String status) { | |
| this.status = status; | |
| } | |
| public static record SessionMcpServerStatusChangedData( | |
| @JsonProperty("serverName") String serverName, | |
| @JsonProperty("status") String status) { |
| /** | ||
| * Lists the MCP servers configured for this session, including their connection | ||
| * status. | ||
| * | ||
| * @return a future that resolves with the list of MCP server info objects | ||
| * @throws IllegalStateException | ||
| * if this session has been terminated | ||
| * @since 1.0.0 | ||
| */ | ||
| public CompletableFuture<List<McpServerInfo>> listMcpServers() { | ||
| ensureNotTerminated(); | ||
| return rpc.invoke("session.mcp.list", Map.of("sessionId", sessionId), McpListResponse.class) | ||
| .thenApply(response -> response.servers() != null | ||
| ? Collections.unmodifiableList(response.servers()) | ||
| : Collections.emptyList()); | ||
| } | ||
|
|
||
| /** | ||
| * Enables a previously disabled MCP server in this session. | ||
| * | ||
| * @param serverName | ||
| * the name of the MCP server to enable (the key used in the | ||
| * {@link com.github.copilot.sdk.json.SessionConfig#setMcpServers(java.util.Map)} | ||
| * map) | ||
| * @return a future that completes when the server has been enabled | ||
| * @throws IllegalStateException | ||
| * if this session has been terminated | ||
| * @since 1.0.0 | ||
| */ | ||
| public CompletableFuture<Void> enableMcpServer(String serverName) { | ||
| ensureNotTerminated(); | ||
| return rpc.invoke("session.mcp.enable", Map.of("sessionId", sessionId, "serverName", serverName), Void.class); | ||
| } | ||
|
|
||
| /** | ||
| * Disables an MCP server in this session without removing its configuration. | ||
| * <p> | ||
| * A disabled server can be re-enabled later via | ||
| * {@link #enableMcpServer(String)}. | ||
| * | ||
| * @param serverName | ||
| * the name of the MCP server to disable | ||
| * @return a future that completes when the server has been disabled | ||
| * @throws IllegalStateException | ||
| * if this session has been terminated | ||
| * @since 1.0.0 | ||
| */ | ||
| public CompletableFuture<Void> disableMcpServer(String serverName) { | ||
| ensureNotTerminated(); | ||
| return rpc.invoke("session.mcp.disable", Map.of("sessionId", sessionId, "serverName", serverName), Void.class); | ||
| } | ||
|
|
||
| /** | ||
| * Reloads all MCP servers in this session. | ||
| * <p> | ||
| * This disconnects and reconnects all configured MCP servers, which can be | ||
| * useful to pick up configuration changes or recover from failed connections. | ||
| * | ||
| * @return a future that completes when the reload is initiated | ||
| * @throws IllegalStateException | ||
| * if this session has been terminated | ||
| * @since 1.0.0 | ||
| */ |
There was a problem hiding this comment.
These newly added public methods are annotated @since 1.0.0, but they’re being introduced now and appear next to APIs marked @since 1.0.11/1.2.0. Please align the @since values with the version that actually introduces the MCP server management API to keep generated Javadocs accurate.
|
See this comment. This is great so far, but I'm not sure it solves the issue. Not sure if the AI can follow comment links so here is the full text:
|
|
See #17 (comment) for Close explanation. |
Resolves #17
Before the change?
session.mcp_servers_loaded/session.mcp_server_status_changedevents — these were deserialized asUnknownSessionEventAfter the change?
New event types (parity with upstream .NET/Node.js SDK):
SessionMcpServersLoadedEvent— fired when all configured MCP servers finish initializingSessionMcpServerStatusChangedEvent— fired when a server's connection state transitionsNew DTO:
McpServerInfo— capturesname,status(connected|failed|pending|disabled|not_configured),source, anderrorNew
CopilotSessionmethods:listMcpServers()→session.mcp.listenableMcpServer(String serverName)→session.mcp.enabledisableMcpServer(String serverName)→session.mcp.disablereloadMcpServers()→session.mcp.reloadPull request checklist
mvn spotless:applyhas been run to format the codemvn clean verifypasses locallyDoes this introduce a breaking change?
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.