diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index c76e3c3c08..d7f67f90ca 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -370,6 +370,13 @@ impl McpConnectionManager { !self.clients.is_empty() } + /// Returns immediately with whether every configured client has finished starting. + pub fn startup_is_complete(&self) -> bool { + self.clients + .values() + .all(|client| client.startup_complete.load(Ordering::Acquire)) + } + pub(crate) fn contains_server(&self, server_name: &str) -> bool { self.clients.contains_key(server_name) } diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index a671f0dd74..b2214155f9 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -51,6 +51,7 @@ use crate::session::CodexSpawnOk; use crate::session::SUBMISSION_CHANNEL_CAPACITY; use crate::session::emit_subagent_session_started; use crate::session::session::Session; +use crate::session::turn_context::McpAccess; use crate::session::turn_context::TurnContext; use codex_login::AuthManager; use codex_models_manager::manager::SharedModelsManager; @@ -81,6 +82,7 @@ pub(crate) async fn run_codex_thread_interactive( parent_ctx: Arc, cancel_token: CancellationToken, subagent_source: SubAgentSource, + mcp_access: McpAccess, initial_history: Option, ) -> Result { let (tx_sub, rx_sub) = async_channel::bounded(SUBMISSION_CHANNEL_CAPACITY); @@ -110,6 +112,7 @@ pub(crate) async fn run_codex_thread_interactive( conversation_history, requested_history_mode: None, session_source: SessionSource::SubAgent(subagent_source.clone()), + mcp_access, forked_from_thread_id, parent_thread_id: Some(parent_session.thread_id), thread_source: Some(ThreadSource::Subagent), @@ -202,6 +205,7 @@ pub(crate) async fn run_codex_thread_one_shot( parent_ctx: Arc, cancel_token: CancellationToken, subagent_source: SubAgentSource, + mcp_access: McpAccess, final_output_json_schema: Option, initial_history: Option, ) -> Result { @@ -216,6 +220,7 @@ pub(crate) async fn run_codex_thread_one_shot( parent_ctx, child_cancel.clone(), subagent_source, + mcp_access, initial_history, )) .await?; diff --git a/codex-rs/core/src/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index 9513dcc56c..831f591465 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -174,6 +174,7 @@ async fn run_codex_thread_interactive_respects_pre_cancelled_spawn() { parent_ctx, cancel_token, SubAgentSource::Review, + crate::session::turn_context::McpAccess::Enabled, /*initial_history*/ None, ), ) diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index de56de41a6..32b23c7ed5 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -670,6 +670,7 @@ async fn spawn_guardian_review_session( Arc::clone(parent_turn), cancel_token.clone(), SubAgentSource::Other(GUARDIAN_REVIEWER_NAME.to_string()), + crate::session::turn_context::McpAccess::Enabled, initial_history, )) .await?; diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 4426fa462a..ad08ffb565 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -423,6 +423,7 @@ pub(crate) struct CodexSpawnArgs { pub(crate) conversation_history: InitialHistory, pub(crate) requested_history_mode: Option, pub(crate) session_source: SessionSource, + pub(crate) mcp_access: McpAccess, pub(crate) forked_from_thread_id: Option, pub(crate) parent_thread_id: Option, pub(crate) thread_source: Option, @@ -515,6 +516,7 @@ impl Codex { conversation_history, requested_history_mode, session_source, + mcp_access, forked_from_thread_id, parent_thread_id, thread_source, @@ -687,6 +689,7 @@ impl Codex { agent_status_tx.clone(), conversation_history, session_source_clone, + mcp_access, skills_service, plugins_manager, mcp_manager.clone(), diff --git a/codex-rs/core/src/session/review.rs b/codex-rs/core/src/session/review.rs index 5a0e294b6d..2de2e0431e 100644 --- a/codex-rs/core/src/session/review.rs +++ b/codex-rs/core/src/session/review.rs @@ -117,6 +117,7 @@ pub(super) async fn spawn_review_thread( reasoning_effort, reasoning_summary, session_source, + mcp_access: parent_turn_context.mcp_access, parent_thread_id: parent_turn_context.parent_thread_id, originator: parent_turn_context.originator.clone(), environments: parent_turn_context.environments.clone(), diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 826b1fd616..1dd12c7caa 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -38,6 +38,7 @@ pub(crate) struct Session { /// The set of enabled features should be invariant for the lifetime of the /// session. pub(super) features: ManagedFeatures, + pub(super) mcp_access: McpAccess, pub(super) multi_agent_version: OnceLock, pub(super) pending_mcp_server_refresh_config: Mutex>, pub(crate) conversation: Arc, @@ -490,6 +491,7 @@ impl Session { agent_status: watch::Sender, initial_history: InitialHistory, session_source: SessionSource, + mcp_access: McpAccess, skills_service: Arc, plugins_manager: Arc, mcp_manager: Arc, @@ -677,7 +679,6 @@ impl Session { let mcp_runtime_context = McpRuntimeContext::new(Arc::clone(&environment_manager), mcp_runtime_cwd); let mcp_runtime_context_for_auth = mcp_runtime_context.clone(); - let mcp_access = McpAccess::for_session_source(&session_configuration.session_source); let auth_and_mcp_fut = async move { let auth = auth_manager_clone.auth().await; let mcp_config = mcp_manager_for_mcp @@ -1142,6 +1143,7 @@ impl Session { state: Mutex::new(state), managed_network_proxy_refresh_lock: Semaphore::new(/*permits*/ 1), features: config.features.clone(), + mcp_access, multi_agent_version, pending_mcp_server_refresh_config: Mutex::new(None), conversation: Arc::new(RealtimeConversationManager::new()), diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index d0973ef05e..113983c3b8 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -5229,6 +5229,7 @@ async fn session_new_fails_when_zsh_fork_enabled_without_packaged_zsh() { agent_status_tx, InitialHistory::New, SessionSource::Exec, + McpAccess::Enabled, skills_service, plugins_manager, mcp_manager, @@ -5478,6 +5479,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { &session_telemetry, session_configuration.provider.clone(), &session_configuration, + McpAccess::Enabled, config.multi_agent_version_from_features(), services.user_shell.as_ref(), services.shell_zsh_path.as_ref(), @@ -5500,6 +5502,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { state: Mutex::new(state), managed_network_proxy_refresh_lock: Semaphore::new(/*permits*/ 1), features: config.features.clone(), + mcp_access: McpAccess::Enabled, multi_agent_version: OnceLock::from(config.multi_agent_version_from_features()), pending_mcp_server_refresh_config: Mutex::new(None), conversation: Arc::new(RealtimeConversationManager::new()), @@ -5611,6 +5614,7 @@ async fn make_session_with_config_and_rx( agent_status_tx, InitialHistory::New, SessionSource::Exec, + McpAccess::Enabled, skills_service, plugins_manager, mcp_manager, @@ -5719,6 +5723,7 @@ async fn make_session_with_history_source_and_agent_control_and_rx( agent_status_tx, initial_history, session_source, + McpAccess::Enabled, skills_service, plugins_manager, mcp_manager, @@ -7604,6 +7609,7 @@ where &session_telemetry, session_configuration.provider.clone(), &session_configuration, + McpAccess::Enabled, config.multi_agent_version_from_features(), services.user_shell.as_ref(), services.shell_zsh_path.as_ref(), @@ -7626,6 +7632,7 @@ where state: Mutex::new(state), managed_network_proxy_refresh_lock: Semaphore::new(/*permits*/ 1), features: config.features.clone(), + mcp_access: McpAccess::Enabled, multi_agent_version: OnceLock::from(config.multi_agent_version_from_features()), pending_mcp_server_refresh_config: Mutex::new(None), conversation: Arc::new(RealtimeConversationManager::new()), @@ -7790,7 +7797,7 @@ async fn built_tools_uses_the_step_mcp_runtime() -> anyhow::Result<()> { } #[tokio::test] -async fn review_step_uses_disabled_mcp_runtime_without_publishing_it() { +async fn disabled_mcp_step_uses_isolated_runtime_without_publishing_it() { let (mut session, mut turn_context) = make_session_and_context().await; session.services.selected_capability_roots = vec![SelectedCapabilityRoot { id: "review-root".to_string(), @@ -7799,7 +7806,8 @@ async fn review_step_uses_disabled_mcp_runtime_without_publishing_it() { path: PathUri::from_abs_path(&turn_context.config.cwd), }, }]; - turn_context.session_source = SessionSource::SubAgent(SubAgentSource::Review); + session.mcp_access = McpAccess::Disabled; + turn_context.mcp_access = McpAccess::Disabled; let session = Arc::new(session); let turn_context = Arc::new(turn_context); let published_before = session.services.latest_mcp_runtime(); diff --git a/codex-rs/core/src/session/tests/guardian_tests.rs b/codex-rs/core/src/session/tests/guardian_tests.rs index d13239d810..f929e50080 100644 --- a/codex-rs/core/src/session/tests/guardian_tests.rs +++ b/codex-rs/core/src/session/tests/guardian_tests.rs @@ -734,6 +734,7 @@ async fn guardian_subagent_does_not_inherit_parent_exec_policy_rules() { session_source: SessionSource::SubAgent(SubAgentSource::Other( GUARDIAN_REVIEWER_NAME.to_string(), )), + mcp_access: crate::session::turn_context::McpAccess::Enabled, forked_from_thread_id: None, parent_thread_id: None, thread_source: None, diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index eecfc9e0a3..72506812e7 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -10,7 +10,6 @@ use codex_protocol::ThreadId; use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::openai_models::ModelInfo; use codex_protocol::protocol::MultiAgentVersion; -use codex_protocol::protocol::SubAgentSource; use codex_protocol::protocol::TurnEnvironmentSelection; use codex_sandboxing::compatibility_sandbox_policy_for_permission_profile; use codex_sandboxing::policy_transforms::effective_file_system_sandbox_policy; @@ -112,6 +111,7 @@ pub struct TurnContext { pub(crate) reasoning_effort: Option, pub(crate) reasoning_summary: ReasoningSummaryConfig, pub(crate) session_source: SessionSource, + pub(crate) mcp_access: McpAccess, pub(crate) parent_thread_id: Option, pub(crate) originator: String, pub(crate) environments: TurnEnvironmentSnapshot, @@ -155,30 +155,9 @@ pub(crate) enum McpAccess { Disabled, } -impl McpAccess { - pub(crate) fn for_session_source(session_source: &SessionSource) -> Self { - match session_source { - SessionSource::SubAgent(SubAgentSource::Review) => Self::Disabled, - SessionSource::Cli - | SessionSource::VSCode - | SessionSource::Exec - | SessionSource::Mcp - | SessionSource::Custom(_) - | SessionSource::Internal(_) - | SessionSource::Unknown - | SessionSource::SubAgent( - SubAgentSource::Compact - | SubAgentSource::MemoryConsolidation - | SubAgentSource::ThreadSpawn { .. } - | SubAgentSource::Other(_), - ) => Self::Enabled, - } - } -} - impl TurnContext { pub(crate) fn mcp_access(&self) -> McpAccess { - McpAccess::for_session_source(&self.session_source) + self.mcp_access } pub(crate) fn permission_profile(&self) -> PermissionProfile { @@ -296,6 +275,7 @@ impl TurnContext { reasoning_effort, reasoning_summary: self.reasoning_summary, session_source: self.session_source.clone(), + mcp_access: self.mcp_access, parent_thread_id: self.parent_thread_id, originator: self.originator.clone(), environments: self.environments.clone(), @@ -506,6 +486,7 @@ impl Session { session_telemetry: &SessionTelemetry, provider: ModelProviderInfo, session_configuration: &SessionConfiguration, + mcp_access: McpAccess, multi_agent_version: MultiAgentVersion, user_shell: &shell::Shell, shell_zsh_path: Option<&PathBuf>, @@ -574,6 +555,7 @@ impl Session { reasoning_effort, reasoning_summary, session_source, + mcp_access, parent_thread_id: session_configuration.parent_thread_id, originator: session_configuration.originator.clone(), environments, @@ -775,6 +757,7 @@ impl Session { &self.services.session_telemetry, session_configuration.provider.clone(), &session_configuration, + self.mcp_access, multi_agent_version, self.services.user_shell.as_ref(), self.services.shell_zsh_path.as_ref(), diff --git a/codex-rs/core/src/tasks/review.rs b/codex-rs/core/src/tasks/review.rs index 19ac0fdc35..c4113b31b0 100644 --- a/codex-rs/core/src/tasks/review.rs +++ b/codex-rs/core/src/tasks/review.rs @@ -22,6 +22,7 @@ use crate::review_format::format_review_findings_block; use crate::review_format::render_review_output_text; use crate::session::TurnInput; use crate::session::session::Session; +use crate::session::turn_context::McpAccess; use crate::session::turn_context::TurnContext; use crate::state::TaskKind; use codex_features::Feature; @@ -122,6 +123,17 @@ async fn start_review_conversation( .clone() .unwrap_or_else(|| ctx.model_info.slug.clone()); sub_agent_config.model = Some(model); + let mcp_access = if session + .clone_session() + .services + .latest_mcp_runtime() + .manager() + .startup_is_complete() + { + McpAccess::Enabled + } else { + McpAccess::Disabled + }; (run_codex_thread_one_shot( sub_agent_config, session.auth_manager(), @@ -131,6 +143,7 @@ async fn start_review_conversation( ctx.clone(), cancellation_token, SubAgentSource::Review, + mcp_access, /*final_output_json_schema*/ None, /*initial_history*/ None, ) diff --git a/codex-rs/core/src/thread_manager.rs b/codex-rs/core/src/thread_manager.rs index 4ea86cb33d..c943bcffb6 100644 --- a/codex-rs/core/src/thread_manager.rs +++ b/codex-rs/core/src/thread_manager.rs @@ -1588,6 +1588,7 @@ impl ThreadManagerState { conversation_history: initial_history, requested_history_mode: history_mode, session_source, + mcp_access: crate::session::turn_context::McpAccess::Enabled, forked_from_thread_id, parent_thread_id, thread_source, diff --git a/codex-rs/core/tests/suite/review.rs b/codex-rs/core/tests/suite/review.rs index 77cae51aea..65b6d11148 100644 --- a/codex-rs/core/tests/suite/review.rs +++ b/codex-rs/core/tests/suite/review.rs @@ -37,8 +37,10 @@ use core_test_support::wait_for_event; use pretty_assertions::assert_eq; use std::path::PathBuf; use std::sync::Arc; +use std::time::Duration; use tempfile::TempDir; use tokio::io::AsyncWriteExt as _; +use tokio::time::timeout; use uuid::Uuid; use wiremock::Mock; use wiremock::MockServer; @@ -291,6 +293,42 @@ apps = true .with_config(move |config| config.chatgpt_base_url = apps_base_url) } +fn pinned_apps_with_tool_suggest_builder( + codex_home: Arc, + apps_base_url: String, +) -> TestCodexBuilder { + pinned_apps_builder(codex_home, apps_base_url).with_config(|config| { + for feature in [ + Feature::Plugins, + Feature::RemotePlugin, + Feature::ToolSuggest, + ] { + config + .features + .enable(feature) + .expect("test config should allow feature update"); + } + }) +} + +async fn mount_suggested_plugin(server: &MockServer) { + Mock::given(method("GET")) + .and(path("/ps/plugins/suggested")) + .and(query_param("scope", "GLOBAL")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "enabled": true, + "plugins": [{ + "id": "plugin_github", + "name": "github", + "status": "ENABLED", + "installation_policy": "AVAILABLE", + "release": {"display_name": "GitHub"} + }] + }))) + .mount(server) + .await; +} + fn write_enabled_test_plugin(codex_home: &TempDir) -> anyhow::Result { let plugin_root = codex_home.path().join("plugins/cache/test/sample/local"); std::fs::create_dir_all(plugin_root.join(".codex-plugin"))?; @@ -310,19 +348,60 @@ enabled = true Ok(plugin_root) } +fn write_test_plugin_mcp(plugin_root: &std::path::Path, server_url: &str) -> anyhow::Result<()> { + std::fs::write( + plugin_root.join(".mcp.json"), + format!(r#"{{"mcpServers":{{"sample":{{"type":"http","url":"{server_url}/mcp"}}}}}}"#), + )?; + Ok(()) +} + +async fn mount_pending_mcp_server(server: &MockServer) { + Mock::given(method("POST")) + .and(path("/mcp")) + .respond_with(ResponseTemplate::new(200).set_delay(Duration::from_secs(60))) + .mount(server) + .await; +} + +async fn wait_for_pending_mcp_request(server: &MockServer) -> anyhow::Result { + timeout(Duration::from_secs(5), async { + loop { + let request_count = server + .received_requests() + .await + .unwrap_or_default() + .into_iter() + .filter(|request| request.method.as_str() == "POST" && request.url.path() == "/mcp") + .count(); + if request_count > 0 { + return request_count; + } + tokio::task::yield_now().await; + } + }) + .await + .map_err(|_| anyhow::anyhow!("MCP server did not receive a startup request")) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn review_does_not_expose_mcp_tools() -> anyhow::Result<()> { +async fn review_exposes_mcp_tools_when_parent_mcp_is_ready() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; let apps_server = AppsTestServer::mount(&server).await?; + let unavailable_mcp_server = MockServer::start().await; let codex_home = Arc::new(TempDir::new()?); let plugin_root = write_enabled_test_plugin(&codex_home)?; std::fs::write( plugin_root.join(".mcp.json"), format!( - r#"{{"mcpServers":{{"sample":{{"type":"http","url":"{}/api/codex/apps"}}}}}}"#, - apps_server.chatgpt_base_url + r#"{{"mcpServers":{{ + "sample":{{"type":"http","url":"{}/api/codex/apps"}}, + "unavailable":{{"type":"http","url":"{}/mcp"}} + }}}}"#, + apps_server.chatgpt_base_url, + unavailable_mcp_server.uri() ), )?; let mut builder = apps_enabled_builder(apps_server.chatgpt_base_url.clone()) @@ -346,7 +425,7 @@ async fn review_does_not_expose_mcp_tools() -> anyhow::Result<()> { test.submit_turn("Inspect available Apps tools").await?; - submit_custom_review(&test.codex, "Review without MCP tools").await?; + submit_custom_review(&test.codex, "Review with ready MCP tools").await?; let entered = wait_for_event(&test.codex, |event| { matches!( event, @@ -373,26 +452,14 @@ async fn review_does_not_expose_mcp_tools() -> anyhow::Result<()> { requests[0].body_json()["tools"] ); let request = &requests[1]; + assert!(request.body_contains_text("list_mcp_resources")); assert!( request .tool_by_name("mcp__sample", "calendar_create_event") - .is_none(), - "review request unexpectedly exposed the configured MCP tool: {:?}", + .is_some(), + "review request should expose the ready MCP tool: {:?}", request.body_json()["tools"] ); - for tool_name in [ - "mcp__codex_apps", - "mcp__sample", - "list_mcp_resources", - "list_mcp_resource_templates", - "read_mcp_resource", - ] { - assert!( - !request.body_contains_text(tool_name), - "review request unexpectedly exposed {tool_name}: {:?}", - request.body_json()["tools"] - ); - } Ok(()) } @@ -505,12 +572,16 @@ print(json.dumps({ } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn review_does_not_start_mcp_when_apps_feature_is_pinned_on() -> anyhow::Result<()> { +async fn review_skips_pending_mcp_even_if_apps_is_pinned() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; let apps_server = AppsTestServer::mount(&server).await?; let codex_home = Arc::new(TempDir::new()?); + let plugin_mcp_server = MockServer::start().await; + mount_pending_mcp_server(&plugin_mcp_server).await; + let plugin_root = write_enabled_test_plugin(&codex_home)?; + write_test_plugin_mcp(&plugin_root, &plugin_mcp_server.uri())?; let mut builder = pinned_apps_builder(codex_home, apps_server.chatgpt_base_url.clone()); let test = builder.build_with_auto_env(&server).await?; @@ -519,10 +590,7 @@ async fn review_does_not_start_mcp_when_apps_feature_is_pinned_on() -> anyhow::R .disable(Feature::Apps) .expect("managed feature normalization should succeed"); assert!(pinned_features.enabled(Feature::Apps)); - wait_for_event(&test.codex, |event| { - matches!(event, EventMsg::McpStartupComplete(_)) - }) - .await; + let mcp_requests_before_review = wait_for_pending_mcp_request(&plugin_mcp_server).await?; let responses = mount_sse_sequence( &server, @@ -531,11 +599,16 @@ async fn review_does_not_start_mcp_when_apps_feature_is_pinned_on() -> anyhow::R .await; submit_custom_review(&test.codex, "Review without starting pinned Apps MCP").await?; + let mut review_started = false; wait_for_event(&test.codex, |event| match event { - EventMsg::McpStartupUpdate(update) => { + EventMsg::EnteredReviewMode(_) => { + review_started = true; + false + } + EventMsg::McpStartupUpdate(update) if review_started => { panic!("review emitted an MCP startup update: {update:?}") } - EventMsg::McpStartupComplete(complete) => { + EventMsg::McpStartupComplete(complete) if review_started => { panic!("review emitted an MCP startup completion: {complete:?}") } EventMsg::TurnComplete(_) => true, @@ -544,45 +617,44 @@ async fn review_does_not_start_mcp_when_apps_feature_is_pinned_on() -> anyhow::R .await; assert_eq!(responses.requests().len(), 1); + let request = responses.single_request(); + for tool_name in [ + "mcp__codex_apps", + "mcp__sample", + "list_mcp_resources", + "list_mcp_resource_templates", + "read_mcp_resource", + ] { + assert!( + !request.body_contains_text(tool_name), + "pending review unexpectedly exposed {tool_name}: {:?}", + request.body_json()["tools"] + ); + } + assert_eq!( + wait_for_pending_mcp_request(&plugin_mcp_server).await?, + mcp_requests_before_review, + "review should not start a second MCP client while the parent MCP is pending" + ); Ok(()) } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn review_does_not_suggest_tools_when_apps_feature_is_pinned_on() -> anyhow::Result<()> { +async fn review_skips_tool_suggestions_with_pending_mcp_and_pinned_apps() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; let apps_server = AppsTestServer::mount(&server).await?; - Mock::given(method("GET")) - .and(path("/ps/plugins/suggested")) - .and(query_param("scope", "GLOBAL")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "enabled": true, - "plugins": [{ - "id": "plugin_github", - "name": "github", - "status": "ENABLED", - "installation_policy": "AVAILABLE", - "release": {"display_name": "GitHub"} - }] - }))) - .mount(&server) - .await; + mount_suggested_plugin(&server).await; let codex_home = Arc::new(TempDir::new()?); - let mut builder = pinned_apps_builder(codex_home, apps_server.chatgpt_base_url.clone()) - .with_config(move |config| { - for feature in [ - Feature::Plugins, - Feature::RemotePlugin, - Feature::ToolSuggest, - ] { - config - .features - .enable(feature) - .expect("test config should allow feature update"); - } - }); + let plugin_mcp_server = MockServer::start().await; + mount_pending_mcp_server(&plugin_mcp_server).await; + let plugin_root = write_enabled_test_plugin(&codex_home)?; + write_test_plugin_mcp(&plugin_root, &plugin_mcp_server.uri())?; + let mut builder = + pinned_apps_with_tool_suggest_builder(codex_home, apps_server.chatgpt_base_url.clone()); let test = builder.build_with_auto_env(&server).await?; + wait_for_pending_mcp_request(&plugin_mcp_server).await?; let suggested_requests_before_review = server .received_requests() @@ -624,45 +696,86 @@ async fn review_does_not_suggest_tools_when_apps_feature_is_pinned_on() -> anyho } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn review_does_not_discover_auth_for_plugin_mcp_servers() -> anyhow::Result<()> { +async fn review_keeps_tool_suggestions_with_ready_mcp_and_pinned_apps() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let apps_server = AppsTestServer::mount(&server).await?; + mount_suggested_plugin(&server).await; + let codex_home = Arc::new(TempDir::new()?); + let mut builder = + pinned_apps_with_tool_suggest_builder(codex_home, apps_server.chatgpt_base_url.clone()); + let test = builder.build_with_auto_env(&server).await?; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::McpStartupComplete(_)) + }) + .await; + + let suggested_requests_before_review = server + .received_requests() + .await + .unwrap_or_default() + .into_iter() + .filter(|request| request.url.path() == "/ps/plugins/suggested") + .count(); + let responses = mount_sse_sequence( + &server, + vec![responses::sse(vec![responses::ev_completed("resp-1")])], + ) + .await; + submit_custom_review(&test.codex, "Review with tool suggestions").await?; + + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + let request = responses.single_request(); + assert!( + request.body_json()["tools"] + .as_array() + .is_some_and(|tools| tools.iter().any(|tool| { + tool.get("name").and_then(serde_json::Value::as_str) + == Some("request_plugin_install") + })), + "ready review should expose request_plugin_install: {:?}", + request.body_json()["tools"] + ); + let suggested_requests_after_review = server + .received_requests() + .await + .unwrap_or_default() + .into_iter() + .filter(|request| request.url.path() == "/ps/plugins/suggested") + .count(); + assert!( + suggested_requests_after_review > suggested_requests_before_review, + "ready review should fetch recommended plugins" + ); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn review_skips_auth_discovery_when_parent_mcp_is_pending() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; let plugin_mcp_server = MockServer::start().await; + mount_pending_mcp_server(&plugin_mcp_server).await; let codex_home = Arc::new(TempDir::new()?); let plugin_root = write_enabled_test_plugin(&codex_home)?; - std::fs::write( - plugin_root.join(".mcp.json"), - format!( - r#"{{ - "mcpServers": {{ - "sample": {{ - "type": "http", - "url": "{}/mcp" - }} - }} -}}"#, - plugin_mcp_server.uri() - ), - )?; + write_test_plugin_mcp(&plugin_root, &plugin_mcp_server.uri())?; let test = test_codex() .with_home(codex_home) .build_with_auto_env(&server) .await?; - wait_for_event(&test.codex, |event| { - matches!(event, EventMsg::McpStartupComplete(_)) - }) - .await; + wait_for_pending_mcp_request(&plugin_mcp_server).await?; let plugin_mcp_requests_before_review = plugin_mcp_server .received_requests() .await .unwrap_or_default() .len(); - assert!( - plugin_mcp_requests_before_review > 0, - "parent session should exercise the plugin MCP endpoint" - ); let responses = mount_sse_sequence( &server,