-
Notifications
You must be signed in to change notification settings - Fork 7.2k
feat: hot reload mcp servers #8957
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
602c7ae
5029060
49a122f
0b60f56
81772af
0f4d841
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,6 +60,7 @@ use codex_app_server_protocol::LogoutChatGptResponse; | |
| use codex_app_server_protocol::McpServerOauthLoginCompletedNotification; | ||
| use codex_app_server_protocol::McpServerOauthLoginParams; | ||
| use codex_app_server_protocol::McpServerOauthLoginResponse; | ||
| use codex_app_server_protocol::McpServerRefreshResponse; | ||
| use codex_app_server_protocol::McpServerStatus; | ||
| use codex_app_server_protocol::ModelListParams; | ||
| use codex_app_server_protocol::ModelListResponse; | ||
|
|
@@ -157,6 +158,7 @@ use codex_protocol::items::TurnItem; | |
| use codex_protocol::models::ResponseItem; | ||
| use codex_protocol::protocol::GitInfo as CoreGitInfo; | ||
| use codex_protocol::protocol::McpAuthStatus as CoreMcpAuthStatus; | ||
| use codex_protocol::protocol::McpServerRefreshConfig; | ||
| use codex_protocol::protocol::RateLimitSnapshot as CoreRateLimitSnapshot; | ||
| use codex_protocol::protocol::RolloutItem; | ||
| use codex_protocol::protocol::SessionMetaLine; | ||
|
|
@@ -425,6 +427,9 @@ impl CodexMessageProcessor { | |
| ClientRequest::McpServerOauthLogin { request_id, params } => { | ||
| self.mcp_server_oauth_login(request_id, params).await; | ||
| } | ||
| ClientRequest::McpServerRefresh { request_id, params } => { | ||
| self.mcp_server_refresh(request_id, params).await; | ||
| } | ||
| ClientRequest::McpServerStatusList { request_id, params } => { | ||
| self.list_mcp_server_status(request_id, params).await; | ||
| } | ||
|
|
@@ -2302,6 +2307,57 @@ impl CodexMessageProcessor { | |
| outgoing.send_response(request_id, response).await; | ||
| } | ||
|
|
||
| async fn mcp_server_refresh(&self, request_id: RequestId, _params: Option<()>) { | ||
| let config = match self.load_latest_config().await { | ||
| Ok(config) => config, | ||
|
Comment on lines
+2311
to
+2312
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This new Useful? React with 👍 / 👎. |
||
| Err(error) => { | ||
| self.outgoing.send_error(request_id, error).await; | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| let mcp_servers = match serde_json::to_value(&config.mcp_servers) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why serialize with i think |
||
| Ok(value) => value, | ||
| Err(err) => { | ||
| let error = JSONRPCErrorError { | ||
| code: INTERNAL_ERROR_CODE, | ||
| message: format!("failed to serialize MCP servers: {err}"), | ||
| data: None, | ||
| }; | ||
| self.outgoing.send_error(request_id, error).await; | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| let mcp_oauth_credentials_store_mode = | ||
| match serde_json::to_value(config.mcp_oauth_credentials_store_mode) { | ||
| Ok(value) => value, | ||
| Err(err) => { | ||
| let error = JSONRPCErrorError { | ||
| code: INTERNAL_ERROR_CODE, | ||
| message: format!( | ||
| "failed to serialize MCP OAuth credentials store mode: {err}" | ||
| ), | ||
| data: None, | ||
| }; | ||
| self.outgoing.send_error(request_id, error).await; | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| let refresh_config = McpServerRefreshConfig { | ||
| mcp_servers, | ||
| mcp_oauth_credentials_store_mode, | ||
| }; | ||
|
|
||
| // Refresh requests are queued per thread; each thread rebuilds MCP connections on its next | ||
| // active turn to avoid work for threads that never resume. | ||
| let thread_manager = Arc::clone(&self.thread_manager); | ||
| thread_manager.refresh_mcp_servers(refresh_config).await; | ||
| let response = McpServerRefreshResponse {}; | ||
| self.outgoing.send_response(request_id, response).await; | ||
| } | ||
|
|
||
| async fn mcp_server_oauth_login( | ||
| &self, | ||
| request_id: RequestId, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not refresh the mcp connection manager for all active threads for inform them there is an update with the latest mcp config. It is up to the thread to refresh the connection on next active turn. This should help with performance and avoid excessive refresh if there is a lot of MCP changes at once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe put this in a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"It is up to the thread to refresh the connection on next active turn."
What does this mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All active threads in the thread manager would be aware of a new mcp config but we are not triggering an actual reinitialization yet. On a future turn (i.e. user submit a new prompt), if the thread detects a pending_mcp_config, then it would first reinitialize and then move onto the actual turn. The benefit here is that we are not doing any extra leg work - for example you started a thread and then we change the mcp servers but never reengage with that thread, then that thread would not refresh mcp server until called upon.