mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
fix: change codex/sandbox-state/update from a notification to a request (#8142)
Historically, `accept_elicitation_for_prompt_rule()` was flaky because we were using a notification to update the sandbox followed by a `shell` tool request that we expected to be subject to the new sandbox config, but because [rmcp](https://crates.io/crates/rmcp) MCP servers delegate each incoming message to a new Tokio task, messages are not guaranteed to be processed in order, so sometimes the `shell` tool call would run before the notification was processed. Prior to this PR, we relied on a generous `sleep()` between the notification and the request to reduce the change of the test flaking out. This PR implements a proper fix, which is to use a _request_ instead of a notification for the sandbox update so that we can wait for the response to the sandbox request before sending the request to the `shell` tool call. Previously, `rmcp` did not support custom requests, but I fixed that in https://github.com/modelcontextprotocol/rust-sdk/pull/590, which made it into the `0.12.0` release (see #8288). This PR updates `shell-tool-mcp` to expect `"codex/sandbox-state/update"` as a _request_ instead of a notification and sends the appropriate ack. Note this behavior is tied to our custom `codex/sandbox-state` capability, which Codex honors as an MCP client, which is why `core/src/mcp_connection_manager.rs` had to be updated as part of this PR, as well. This PR also updates the docs at `shell-tool-mcp/README.md`.
This commit is contained in:
@@ -35,7 +35,7 @@ pub mod mcp;
|
||||
mod mcp_connection_manager;
|
||||
pub mod openai_models;
|
||||
pub use mcp_connection_manager::MCP_SANDBOX_STATE_CAPABILITY;
|
||||
pub use mcp_connection_manager::MCP_SANDBOX_STATE_NOTIFICATION;
|
||||
pub use mcp_connection_manager::MCP_SANDBOX_STATE_METHOD;
|
||||
pub use mcp_connection_manager::SandboxState;
|
||||
mod mcp_tool_call;
|
||||
mod message_history;
|
||||
|
||||
@@ -184,17 +184,20 @@ struct ManagedClient {
|
||||
}
|
||||
|
||||
impl ManagedClient {
|
||||
/// Returns once the server has ack'd the sandbox state update.
|
||||
async fn notify_sandbox_state_change(&self, sandbox_state: &SandboxState) -> Result<()> {
|
||||
if !self.server_supports_sandbox_state_capability {
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
self.client
|
||||
.send_custom_notification(
|
||||
MCP_SANDBOX_STATE_NOTIFICATION,
|
||||
let _response = self
|
||||
.client
|
||||
.send_custom_request(
|
||||
MCP_SANDBOX_STATE_METHOD,
|
||||
Some(serde_json::to_value(sandbox_state)?),
|
||||
)
|
||||
.await
|
||||
.await?;
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
@@ -253,9 +256,9 @@ impl AsyncManagedClient {
|
||||
|
||||
pub const MCP_SANDBOX_STATE_CAPABILITY: &str = "codex/sandbox-state";
|
||||
|
||||
/// Custom MCP notification for sandbox state updates.
|
||||
/// Custom MCP request to push sandbox state updates.
|
||||
/// When used, the `params` field of the notification is [`SandboxState`].
|
||||
pub const MCP_SANDBOX_STATE_NOTIFICATION: &str = "codex/sandbox-state/update";
|
||||
pub const MCP_SANDBOX_STATE_METHOD: &str = "codex/sandbox-state/update";
|
||||
|
||||
#[derive(Debug, Clone, Serialize, Deserialize)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
|
||||
Reference in New Issue
Block a user