diff --git a/codex-rs/core/src/guardian/decision.rs b/codex-rs/core/src/guardian/decision.rs index 38477da6f2..ab11c061f4 100644 --- a/codex-rs/core/src/guardian/decision.rs +++ b/codex-rs/core/src/guardian/decision.rs @@ -129,6 +129,19 @@ pub(crate) fn decide_approval( &assessment.authorization.permissions, ) .await; + // MCP prepares the call after the response is scored. The score must + // also match the rules of the server that will handle the call. + if !require_synchronous_review + && let Ok(GuardianApprovalRequest::McpToolCall { + prompt_permissions: Some((environment_id, sandbox)), + .. + }) = &request.request + { + let owner_permissions = + super::permissions::for_mcp_environment(environment_id, sandbox).ok(); + require_synchronous_review = + owner_permissions.as_ref() != Some(&assessment.authorization.permissions); + } } let retried = reasons.retry.is_some(); let decision = codex_guardian_reviewer::ReviewRequest { diff --git a/codex-rs/core/src/guardian/permissions.rs b/codex-rs/core/src/guardian/permissions.rs index b87b7ff570..2aef89bc82 100644 --- a/codex-rs/core/src/guardian/permissions.rs +++ b/codex-rs/core/src/guardian/permissions.rs @@ -1,5 +1,5 @@ -//! Fresh reviews of environment-owned MCP calls use the prepared environment's rules. -//! Other reviews and early scoring use the issuing context. +//! Guardian reviews and score reuse use the prepared MCP owner's rules when available. +//! Early scoring uses the issuing context, before the MCP call is prepared. use codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID; use codex_file_system::FileSystemSandboxContext; @@ -7,6 +7,7 @@ use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemSandboxPolicyContext; use codex_utils_absolute_path::AbsolutePathBuf; +use super::GuardianApprovalRequest; use super::GuardianReviewContext; use crate::context::GuardianPermissionContext; use crate::session::turn_context::TurnEnvironment; @@ -25,6 +26,21 @@ pub(super) fn check_environment_id(id: Option<&str>) -> anyhow::Result<()> { Ok(()) } +pub(super) fn for_request( + request: &GuardianApprovalRequest, + context: Option<&GuardianReviewContext>, +) -> anyhow::Result> { + match request { + GuardianApprovalRequest::McpToolCall { + prompt_permissions: Some((environment_id, sandbox)), + .. + } => for_mcp_environment(environment_id, sandbox).map(Some), + _ => context + .map(|context| for_environment(context, request.target_environment_id())) + .transpose(), + } +} + pub(super) fn for_mcp_environment( environment_id: &str, sandbox: &FileSystemSandboxContext, diff --git a/codex-rs/core/src/guardian/prompt.rs b/codex-rs/core/src/guardian/prompt.rs index 5057fb7cc7..a4afaae26a 100644 --- a/codex-rs/core/src/guardian/prompt.rs +++ b/codex-rs/core/src/guardian/prompt.rs @@ -122,20 +122,7 @@ pub(super) async fn collect_guardian_prompt_items_with_parent_turn( &request, reasons.retry.or(reasons.approval), )?; - let permissions = match request { - GuardianApprovalRequest::McpToolCall { - prompt_permissions: Some((environment_id, sandbox)), - .. - } => Some(super::permissions::for_mcp_environment( - &environment_id, - &sandbox, - )?), - _ => parent_context - .map(|context| { - super::permissions::for_environment(context, request.target_environment_id()) - }) - .transpose()?, - }; + let permissions = super::permissions::for_request(&request, parent_context)?; let node_repl_snapshot = if node_repl_transcripts_enabled { session .services diff --git a/codex-rs/core/src/guardian/review_request.rs b/codex-rs/core/src/guardian/review_request.rs index 443ffab7ba..3e03eb042f 100644 --- a/codex-rs/core/src/guardian/review_request.rs +++ b/codex-rs/core/src/guardian/review_request.rs @@ -100,11 +100,9 @@ impl ReviewHost for super::super::runtime::ReviewRuntime { fn permissions(&self) -> Option { let request = self.request.request.as_ref().ok()?; - crate::guardian::permissions::for_environment( - &self.context, - request.target_environment_id(), - ) - .ok() + crate::guardian::permissions::for_request(request, Some(&self.context)) + .ok() + .flatten() } async fn servicing_turn( diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index a66716b262..27a5c425d2 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -37,6 +37,7 @@ use codex_connectors::AppToolPolicyEvaluator; use codex_connectors::AppToolPolicyInput; use codex_extension_api::McpToolContext; use codex_features::Feature; +use codex_file_system::FileSystemSandboxContext; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_mcp::MCP_TOOL_CODEX_APPS_META_KEY; use codex_mcp::McpPermissionPromptAutoApproveContext; @@ -257,6 +258,7 @@ pub(crate) async fn handle_mcp_tool_call( &invocation, metadata.clone(), GuardianReviewContext::from(step_context), + capture_environment_permissions(prepared_call.permission_source()), ); notify_mcp_tool_call_started( sess.as_ref(), @@ -1312,6 +1314,20 @@ pub(crate) struct McpToolApprovalContext { pub(crate) invocation: Option, pub(crate) metadata: McpToolApprovalMetadata, pub(crate) review_context: GuardianReviewContext, + /// Rules captured when MCP prepared this call, if the server belongs to an environment. + pub(crate) environment_permissions: Option<(String, Arc)>, +} + +fn capture_environment_permissions( + source: McpPermissionSource<'_>, +) -> Option<(String, Arc)> { + match source { + McpPermissionSource::Environment { + environment_id, + sandbox, + } => Some((environment_id.to_owned(), Arc::clone(sandbox))), + McpPermissionSource::Thread { .. } => None, + } } impl Session { @@ -1321,6 +1337,7 @@ impl Session { invocation: &McpInvocation, metadata: McpToolApprovalMetadata, review_context: GuardianReviewContext, + environment_permissions: Option<(String, Arc)>, ) -> Arc { let key = (invocation.server.clone(), call_id.to_string()); let metadata = Arc::new(McpToolApprovalContext { @@ -1329,6 +1346,7 @@ impl Session { .then(|| invocation.clone()), metadata, review_context, + environment_permissions, }); let mut registry = self .mcp_tool_approval_metadata @@ -1695,13 +1713,7 @@ async fn prepare_mcp_tool_approval( approval_mode: policy.mode, allow_session_remember: session_approval_key.is_some(), allow_persistent_approval: persistent_approval_key.is_some(), - prompt_permissions: match permission_source { - McpPermissionSource::Environment { - environment_id, - sandbox, - } => Some((environment_id.to_owned(), Arc::clone(sandbox))), - McpPermissionSource::Thread { .. } => None, - }, + prompt_permissions: capture_environment_permissions(permission_source), }; McpToolApprovalRequirement::Required { action: Box::new(action), diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 23888ae7f1..0792cd279e 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -3723,6 +3723,7 @@ async fn approval_metadata_is_released_when_the_invocation_future_is_dropped() { &invocation, metadata.clone(), review_context.clone(), + /*environment_permissions*/ None, ); std::future::pending::<()>().await; }); @@ -3732,6 +3733,7 @@ async fn approval_metadata_is_released_when_the_invocation_future_is_dropped() { &invocation, metadata.clone(), review_context.clone(), + /*environment_permissions*/ None, ); assert_eq!( session @@ -3755,6 +3757,7 @@ async fn approval_metadata_is_released_when_the_invocation_future_is_dropped() { &invocation, metadata, review_context, + /*environment_permissions*/ None, ); let registry = session.mcp_tool_approval_metadata.lock().unwrap(); let mut keys = registry.keys().cloned().collect::>(); diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 1575de9244..c1e6f2ba17 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -1187,6 +1187,15 @@ async fn review_guardian_mcp_elicitation( } }; guardian_request.category = guardian_scope; + // Keep the action the server asked about, but use its owner's rules from the original call. + if let Ok(crate::guardian::GuardianApprovalRequest::McpToolCall { + prompt_permissions, .. + }) = &mut guardian_request.request + && let Some(permissions) = + approval_context.and_then(|context| context.environment_permissions) + { + *prompt_permissions = Some(permissions); + } let declined_reason = guardian_request.request.as_ref().err().cloned(); let decision = crate::guardian::decide_approval( session, diff --git a/codex-rs/core/tests/suite/guardian_environments_tests.rs b/codex-rs/core/tests/suite/guardian_environments_tests.rs index 79bbc02f16..12128ceafc 100644 --- a/codex-rs/core/tests/suite/guardian_environments_tests.rs +++ b/codex-rs/core/tests/suite/guardian_environments_tests.rs @@ -142,6 +142,7 @@ async fn direct_mcp_approval_uses_the_server_owners_denied_paths() -> Result<()> #[test_case("exec_command")] #[test_case("apply_patch")] #[test_case("request_permissions")] +#[test_case("mcp_tool_call"; "mcp_owner_does_not_reuse_primary_score")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn guardian_reviews_target_environment_and_reuses_prefix(tool: &str) -> Result<()> { skip_if_no_network!(Ok(())); @@ -155,7 +156,7 @@ async fn guardian_reviews_target_environment_and_reuses_prefix(tool: &str) -> Re attach.send(()).expect("attach secondary executor"); let server = start_mock_server().await; - let test = test_codex() + let mut builder = test_codex() .with_pre_build_hook(|home| { std::fs::write( home.join("config.toml"), @@ -183,9 +184,23 @@ async fn guardian_reviews_target_environment_and_reuses_prefix(tool: &str) -> Re .features .disable(Feature::DeferredExecutor) .expect("disable deferred executor"); - }) - .build_with_auto_env(&server) - .await?; + }); + let test = if tool == "mcp_tool_call" { + // The MCP server runs on the host, not on the mock executor. + let servers = serde_json::from_value(json!({ + "files": { + "command": cargo_bin("test_stdio_server")?, + "environment_id": LOCAL_ENVIRONMENT_ID, + "default_tools_approval_mode": "prompt", + } + }))?; + builder + .with_config(move |config| config.mcp_servers.set(servers).expect("set MCP server")) + .build(&server) + .await? + } else { + builder.build_with_auto_env(&server).await? + }; let manager = test.thread_manager.environment_manager(); let secondary_id = "guardian-secondary"; manager.upsert_environment( @@ -220,8 +235,13 @@ async fn guardian_reviews_target_environment_and_reuses_prefix(tool: &str) -> Re secondary.config = EnvironmentConfigState::Ready(secondary_config); let targets = [secondary_id, primary.environment_id.as_str()]; + let tools = if tool == "mcp_tool_call" { + ["exec_command", tool] + } else { + [tool; 2] + }; let mut events = Vec::new(); - for (index, environment_id) in targets.iter().enumerate() { + for (index, (environment_id, tool)) in targets.iter().zip(tools).enumerate() { let call_id = format!("action-{index}"); let action = match tool { "exec_command" => ev_function_call( @@ -251,6 +271,12 @@ async fn guardian_reviews_target_environment_and_reuses_prefix(tool: &str) -> Re }) .to_string(), ), + "mcp_tool_call" => ev_function_call_with_namespace( + &call_id, + "mcp__files", + "echo", + r#"{"message":"hi"}"#, + ), _ => unreachable!(), }; events.push(sse(vec![action, ev_completed(&call_id)])); @@ -318,6 +344,11 @@ async fn guardian_reviews_target_environment_and_reuses_prefix(tool: &str) -> Re } let responses = mount_sse_sequence(&server, events.into_iter().skip(1).step_by(2).collect()).await; + let mut environments = vec![primary.clone().into_request(), secondary.into_request()]; + if tool == "mcp_tool_call" { + // The primary produced the low score; the MCP server belongs to the other computer. + environments.reverse(); + } test.codex.ensure_rollout_materialized().await; test.codex .start_or_steer_turn( @@ -328,7 +359,7 @@ async fn guardian_reviews_target_environment_and_reuses_prefix(tool: &str) -> Re .with_thread_settings(ThreadSettingsOverrides { environments: Some(TurnEnvironmentRequests::new( test.config.cwd.clone(), - vec![primary.clone().into_request(), secondary.into_request()], + environments, )), ..Default::default() }), @@ -350,7 +381,7 @@ async fn guardian_reviews_target_environment_and_reuses_prefix(tool: &str) -> Re .await??; advance .send(()) - .expect("start action on the primary computer"); + .expect("start action on the other computer"); wait_for_event(&test.codex, |event| { matches!(event, EventMsg::TurnComplete(_)) }) @@ -362,7 +393,7 @@ async fn guardian_reviews_target_environment_and_reuses_prefix(tool: &str) -> Re .filter(|request| request.body_json()["client_metadata"]["x-openai-subagent"] == "guardian") .collect::>(); assert_eq!(reviews.len(), targets.len()); - for (review, environment_id) in reviews.iter().zip(targets) { + for ((review, environment_id), tool) in reviews.iter().zip(targets).zip(tools) { let groups = review.message_input_text_groups("user"); let latest = groups.last().context("current review input")?; assert_eq!( @@ -389,10 +420,12 @@ async fn guardian_reviews_target_environment_and_reuses_prefix(tool: &str) -> Re .iter() .find_map(|text| serde_json::from_str::(text).ok()) .context("planned action JSON")?; - assert_eq!( - (&action["tool"], &action["environment_id"]), - (&json!(tool), &json!(environment_id)) - ); + assert_eq!(action["tool"], json!(tool)); + if tool == "mcp_tool_call" { + assert_eq!(action["server"], json!("files")); + } else { + assert_eq!(action["environment_id"], json!(environment_id)); + } } assert_eq!( reviews[0].body_json()["client_metadata"]["thread_id"], diff --git a/codex-rs/core/tests/suite/remote_env.rs b/codex-rs/core/tests/suite/remote_env.rs index 4b7b479ccd..04748e8d17 100644 --- a/codex-rs/core/tests/suite/remote_env.rs +++ b/codex-rs/core/tests/suite/remote_env.rs @@ -51,6 +51,8 @@ use codex_history::RolloutItem; use codex_http_client::HttpClientFactory; use codex_http_client::OutboundProxyPolicy; use codex_network_proxy::NetworkProxyConfig; +use codex_protocol::approvals::GuardianAssessmentStatus; +use codex_protocol::approvals::GuardianReviewReason; use codex_protocol::capabilities::CapabilityRootLocation; use codex_protocol::capabilities::SelectedCapabilityRoot; use codex_protocol::config_types::CollaborationMode; @@ -64,6 +66,8 @@ use codex_protocol::models::PermissionProfileSnapshot; use codex_protocol::models::ResponseItem; use codex_protocol::models::SandboxPermissions; use codex_protocol::models::input_text; +use codex_protocol::openai_models::GuardianModelPolicy; +use codex_protocol::openai_models::GuardianReviewMode; use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry; @@ -3232,11 +3236,22 @@ async fn deferred_executor_guardian_uses_newly_ready_step_environment( &json!({ "environment_id": REMOTE_ENVIRONMENT_ID }).to_string(), ), ), - completed_response( - "resp-guardian-command", - if tool == "mcp_elicitation" { - ev_function_call_with_namespace(EXEC_CALL_ID, "mcp__cua_repl", "js", "{}") - } else { + if tool == "mcp_elicitation" { + let mut call = + ev_function_call_with_namespace(EXEC_CALL_ID, "mcp__cua_repl", "js", "{}"); + call["item"]["id"] = json!("cua-item"); + sse(vec![ + ev_response_created("resp-guardian-command"), + json!({ + "type": "response.metadata", "response_id": "resp-guardian-command", + "metadata": {"guardian": {"item_id": "cua-item", "requires_review": false}} + }), + call, + ev_completed("resp-guardian-command"), + ]) + } else { + completed_response( + "resp-guardian-command", ev_function_call( EXEC_CALL_ID, "exec_command", @@ -3247,9 +3262,9 @@ async fn deferred_executor_guardian_uses_newly_ready_step_environment( "justification": "Review the newly ready remote environment.", }) .to_string(), - ) - }, - ), + ), + ) + }, completed_response( "resp-guardian-review", ev_assistant_message( @@ -3272,16 +3287,28 @@ async fn deferred_executor_guardian_uses_newly_ready_step_environment( config.approvals_reviewer = ApprovalsReviewer::AutoReview; assert!(config.features.enable(Feature::DeferredExecutor).is_ok()); if tool == "mcp_elicitation" { + config + .features + .enable(Feature::GuardianV2) + .expect("enable Guardian v2"); config.mcp_servers.set(serde_json::from_value(json!({ "cua_repl": { "command": if cfg!(windows) { "python" } else { "python3" }, "args": ["-u", "-c", super::guardian_mcp_elicitation::ELICITATION_SERVER, - json!([{"codex_sensitive_action": true}]).to_string(), "forward"], + "[{}]", "forward"], "default_tools_approval_mode": "approve", } })).expect("MCP server config")).expect("set MCP fixture"); } }); + if tool == "mcp_elicitation" { + builder = builder.with_model_info_override("gpt-5.5", |model| { + model.guardian = Some(GuardianModelPolicy { + computer_use: Some(GuardianReviewMode::Adaptive), + ..Default::default() + }); + }); + } let (attach_tx, attach_rx) = tokio::sync::oneshot::channel(); let (shutdown_tx, shutdown_rx) = tokio::sync::oneshot::channel(); let exec_server = tokio::spawn(serve_environment_with_agents_md( @@ -3349,7 +3376,14 @@ async fn deferred_executor_guardian_uses_newly_ready_step_environment( .await?; wait_for_response_request_count(&responses, /*expected_count*/ 1).await; attach_tx.send(()).expect("attach remote environment"); + let mut denial_reason = None; wait_for_event(&test.codex, |event| { + if let EventMsg::GuardianAssessment(assessment) = event + && assessment.status == GuardianAssessmentStatus::Denied + && assessment.target_item_id.as_deref() == Some(EXEC_CALL_ID) + { + denial_reason = assessment.review_reason; + } matches!(event, EventMsg::TurnComplete(_)) }) .await; @@ -3368,10 +3402,18 @@ async fn deferred_executor_guardian_uses_newly_ready_step_environment( }) .context("expected Guardian review request")?; let guardian_context = guardian_request.message_input_texts("user").join("\n"); + // Both actions start in the remote-primary step, but the MCP server runs locally. + let (owner_denied_path, other_denied_path) = if tool == "mcp_elicitation" { + // The low score is labeled local, but the model saw the remote primary's rules. + assert_eq!(denial_reason, Some(GuardianReviewReason::FreshRequired)); + (&local_denied_path, &remote_denied_path) + } else { + (&remote_denied_path, &local_denied_path) + }; for expected in [ "".to_string(), format!("{}", remote_cwd.display()), - format!("- path `{}`", remote_denied_path.display()), + format!("- path `{}`", owner_denied_path.display()), ] { assert!( guardian_context.contains(&expected), @@ -3379,8 +3421,8 @@ async fn deferred_executor_guardian_uses_newly_ready_step_environment( ); } assert!( - !guardian_context.contains(&format!("- path `{}`", local_denied_path.display())), - "Guardian used the stale local environment's denied-read policy: {guardian_context}" + !guardian_context.contains(&format!("- path `{}`", other_denied_path.display())), + "Guardian used the other environment's denied-read policy: {guardian_context}" ); let rejection = requests .last() diff --git a/codex-rs/ext/guardian-reviewer/src/approval_request.rs b/codex-rs/ext/guardian-reviewer/src/approval_request.rs index 85e09cad57..69c9d41190 100644 --- a/codex-rs/ext/guardian-reviewer/src/approval_request.rs +++ b/codex-rs/ext/guardian-reviewer/src/approval_request.rs @@ -75,8 +75,8 @@ pub enum GuardianApprovalRequest { tool_title: Option, tool_description: Option, annotations: Option, - /// Only direct calls to environment-owned servers supply their prepared rules. - /// Thread-owned calls and server-initiated approvals use the issuing context. + /// Rules captured for an environment-owned call. Used for direct approval and + /// for a server's follow-up when its call ID matches that still-running call. prompt_permissions: Option<(String, Arc)>, }, RequestPermissions {