diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 2d2badebd8..4c6e9f1a04 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -1154,7 +1154,9 @@ impl Session { }; turn_state.lock().await.insert_mcp_tool_approval_metadata( call_id.to_string(), - (invocation.server == CODEX_APPS_MCP_SERVER_NAME).then(|| invocation.clone()), + (invocation.server == CODEX_APPS_MCP_SERVER_NAME + || is_node_repl_backed_server(&invocation.server)) + .then(|| invocation.clone()), metadata, ); } diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 0dffbcdb23..8920588d47 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -13,6 +13,7 @@ use codex_mcp::MCP_TOOL_CODEX_APPS_META_KEY; use codex_protocol::capabilities::CapabilityRootLocation; use codex_protocol::capabilities::SelectedCapabilityRoot; use codex_protocol::config_types::ApprovalsReviewer; +use codex_protocol::mcp::is_node_repl_backed_server; use codex_protocol::mcp_approval_meta::APPROVAL_KIND_KEY as MCP_ELICITATION_APPROVAL_KIND_KEY; use codex_protocol::mcp_approval_meta::APPROVAL_KIND_MCP_TOOL_CALL as MCP_ELICITATION_APPROVAL_KIND_MCP_TOOL_CALL; use codex_protocol::mcp_approval_meta::APPROVAL_KIND_TOOL_SUGGESTION as MCP_ELICITATION_APPROVAL_KIND_TOOL_SUGGESTION; @@ -724,6 +725,24 @@ async fn review_guardian_mcp_elicitation( return Ok(None); }; + // The invocation identifies the tool event, but a nested elicitation can + // review a different action and connector than the enclosing JavaScript. + let originating_call_id = if is_node_repl_backed_server(&request.server_name) + && let Some(call_id) = request + .elicitation + .meta() + .and_then(|meta| meta.get("callId")) + .and_then(Value::as_str) + && let Some((Some(invocation), _)) = session + .mcp_tool_approval_metadata(&turn_context.sub_id, call_id) + .await + && invocation.server == request.server_name + { + Some(call_id) + } else { + None + }; + let require_synchronous_review = matches!( request .elicitation @@ -804,7 +823,7 @@ async fn review_guardian_mcp_elicitation( } let GuardianElicitationReview::ApprovalRequest(guardian_request) = - guardian_elicitation_review_request(&request) + guardian_elicitation_review_request(&request, originating_call_id) else { return Ok(None); }; @@ -884,7 +903,8 @@ async fn review_guardian_mcp_elicitation( return Ok(None); } - let guardian_request = match guardian_elicitation_review_request(&request) { + let guardian_request = match guardian_elicitation_review_request(&request, originating_call_id) + { GuardianElicitationReview::NotRequested => return Ok(None), GuardianElicitationReview::Decline(reason) => { warn!( @@ -921,6 +941,7 @@ async fn review_guardian_mcp_elicitation( fn guardian_elicitation_review_request( request: &ElicitationReviewRequest, + originating_call_id: Option<&str>, ) -> GuardianElicitationReview { let (meta, requested_schema) = match &request.elicitation { Elicitation::Mcp(rmcp::model::ElicitRequestParams::FormElicitationParams { @@ -987,11 +1008,13 @@ fn guardian_elicitation_review_request( GuardianElicitationReview::ApprovalRequest(Box::new( crate::guardian::GuardianApprovalRequest::McpToolCall { - id: format!( - "mcp_elicitation:{}:{}", - request.server_name, - mcp_elicitation_request_id(&request.request_id) - ), + id: originating_call_id.map(str::to_owned).unwrap_or_else(|| { + format!( + "mcp_elicitation:{}:{}", + request.server_name, + mcp_elicitation_request_id(&request.request_id) + ) + }), server: request.server_name.clone(), tool_name, arguments, diff --git a/codex-rs/core/src/session/mcp_tests.rs b/codex-rs/core/src/session/mcp_tests.rs index 781dc301d7..120789bc35 100644 --- a/codex-rs/core/src/session/mcp_tests.rs +++ b/codex-rs/core/src/session/mcp_tests.rs @@ -47,7 +47,7 @@ fn guardian_elicitation_review_request_builds_mcp_tool_call() { })))); let GuardianElicitationReview::ApprovalRequest(guardian_request) = - guardian_elicitation_review_request(&request) + guardian_elicitation_review_request(&request, /*originating_call_id*/ None) else { panic!("expected Guardian MCP tool call request"); }; @@ -86,7 +86,7 @@ fn guardian_elicitation_review_request_defaults_missing_tool_params() { let request = form_request(guardian_meta(/*tool_params*/ None)); let GuardianElicitationReview::ApprovalRequest(guardian_request) = - guardian_elicitation_review_request(&request) + guardian_elicitation_review_request(&request, /*originating_call_id*/ None) else { panic!("expected Guardian MCP tool call request"); }; @@ -163,7 +163,7 @@ fn guardian_elicitation_review_request_requires_opt_in() { }))); assert_eq!( - guardian_elicitation_review_request(&request), + guardian_elicitation_review_request(&request, /*originating_call_id*/ None), GuardianElicitationReview::NotRequested ); } @@ -181,7 +181,7 @@ fn guardian_elicitation_review_request_declines_unsupported_opt_in_shapes() { }), }; assert!(matches!( - guardian_elicitation_review_request(&url_request), + guardian_elicitation_review_request(&url_request, /*originating_call_id*/ None), GuardianElicitationReview::Decline(_) )); @@ -201,7 +201,10 @@ fn guardian_elicitation_review_request_declines_unsupported_opt_in_shapes() { }), }; assert!(matches!( - guardian_elicitation_review_request(&non_empty_schema_request), + guardian_elicitation_review_request( + &non_empty_schema_request, + /*originating_call_id*/ None + ), GuardianElicitationReview::Decline(_) )); @@ -210,7 +213,10 @@ fn guardian_elicitation_review_request_declines_unsupported_opt_in_shapes() { "codex_request_type": "approval_request", }))); assert!(matches!( - guardian_elicitation_review_request(&missing_tool_name_request), + guardian_elicitation_review_request( + &missing_tool_name_request, + /*originating_call_id*/ None + ), GuardianElicitationReview::Decline(_) )); } diff --git a/codex-rs/core/tests/suite/guardian_mcp_elicitation.rs b/codex-rs/core/tests/suite/guardian_mcp_elicitation.rs index daa5b54ad3..b385e31e63 100644 --- a/codex-rs/core/tests/suite/guardian_mcp_elicitation.rs +++ b/codex-rs/core/tests/suite/guardian_mcp_elicitation.rs @@ -3,9 +3,12 @@ use anyhow::Result; use codex_core::TurnInputRequest; use codex_core::config::Constrained; use codex_protocol::config_types::ApprovalsReviewer; +use codex_protocol::items::McpToolCallStatus; +use codex_protocol::items::TurnItem; use codex_protocol::openai_models::AutoReviewMessages; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::GuardianAssessmentAction; use codex_protocol::protocol::GuardianAssessmentStatus; use codex_protocol::protocol::Op; use codex_protocol::protocol::ReviewDecision; @@ -36,6 +39,10 @@ def send(message): pending_call = None approval_meta = json.loads(sys.argv[1]) if len(sys.argv) > 1 else {} +multiple = isinstance(approval_meta, list) +approval_metas = approval_meta if multiple else [approval_meta] +pending_elicitations = [] +approval_results = {} for line in sys.stdin: request = json.loads(line) method = request.get("method") @@ -44,21 +51,37 @@ for line in sys.stdin: "capabilities": {"tools": {}}, "serverInfo": {"name": "guardian-elicitation-test", "version": "1"}} elif method == "tools/list": - result = {"tools": [{"name": "request_approval", + result = {"tools": [{"name": "js" if multiple else "request_approval", "inputSchema": {"type": "object", "properties": {}}, "annotations": {"readOnlyHint": True}}]} elif method == "tools/call": + if request["params"].get("arguments", {}).get("seed"): + send({"id": request["id"], "result": {"content": [{"type": "text", "text": "seeded"}]}}) + continue pending_call = request["id"] - send({"id": "server-approval", "method": "elicitation/create", "params": { - "message": "Approve the server-side action?", - "requestedSchema": {"type": "object", "properties": {}}, - "_meta": {"codex_request_type": "approval_request", - "codex_approval_kind": "mcp_tool_call", "tool_name": "write_record", - **approval_meta}}}) + approval_results = {} + for index, meta in enumerate(approval_metas): + invocation_meta = {} + if len(sys.argv) > 2 and sys.argv[2] == "forward": + invocation_meta["callId"] = request["params"]["_meta"]["callId"] + pending_elicitations.append({ + "id": f"server-approval-{index}" if multiple else "server-approval", + "method": "elicitation/create", "params": { + "message": "Approve the server-side action?", + "requestedSchema": {"type": "object", "properties": {}}, + "_meta": {"codex_request_type": "approval_request", + "codex_approval_kind": "mcp_tool_call", "tool_name": "write_record", + **meta, **invocation_meta}}}) + send(pending_elicitations.pop(0)) continue - elif method is None and request.get("id") == "server-approval": - send({"id": pending_call, "result": {"content": [ - {"type": "text", "text": json.dumps(request.get("result"))}]}}) + elif method is None and str(request.get("id", "")).startswith("server-approval"): + approval_results[request["id"]] = request.get("result") + if pending_elicitations: + send(pending_elicitations.pop(0)) + else: + result = approval_results if multiple else request.get("result") + send({"id": pending_call, "result": {"content": [ + {"type": "text", "text": json.dumps(result)}]}}) continue elif method == "resources/list": result = {"resources": []} @@ -342,3 +365,291 @@ async fn server_initiated_mcp_elicitation_can_require_synchronous_auto_review( test.codex.shutdown_and_wait().await?; Ok(()) } + +#[derive(Clone, Copy, PartialEq, Eq)] +enum CallIdSource { + Host, + Missing, + NonString, + Unknown, + WrongServer, + PreviousTurn, +} + +#[test_case("node_repl", false, CallIdSource::Host; "ordinary_node_repl")] +#[test_case("node_repl", true, CallIdSource::Host; "strict_node_repl")] +#[test_case("cua_repl", false, CallIdSource::Host; "ordinary_cua_repl")] +#[test_case("cua_repl", true, CallIdSource::Host; "strict_cua_repl")] +#[test_case("node_repl", false, CallIdSource::Missing; "missing_call_id")] +#[test_case("node_repl", true, CallIdSource::Missing; "strict_missing_call_id")] +#[test_case("node_repl", false, CallIdSource::NonString; "non_string_call_id")] +#[test_case("node_repl", false, CallIdSource::Unknown; "unknown_call_id")] +#[test_case("node_repl", false, CallIdSource::WrongServer; "wrong_server_call_id")] +#[test_case("node_repl", false, CallIdSource::PreviousTurn; "previous_turn_call_id")] +#[test_case("elicitation", false, CallIdSource::Host; "unrelated_server")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn node_elicitations_attribute_independent_reviews_without_changing_actions( + server_name: &str, + strict_auto_review: bool, + call_id_source: CallIdSource, +) -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_wine_exec!(Ok(()), "the MCP fixture requires a host Python interpreter"); + + let actions = [ + ( + "access_browser_origin", + json!({"origin": "https://example.com"}), + ), + ("webmcp:write_record", json!({"record": {"value": 42}})), + ]; + let meta = actions.map(|(tool_name, arguments)| { + let mut meta = json!({ + "codex_strict_auto_review": strict_auto_review, + "codex_sensitive_action": true, + "tool_name": tool_name, + "tool_params": arguments, + "connector_id": "inner-connector", + "connector_name": "Inner Connector", + "connector_description": "Connector for the reviewed inner action", + "tool_title": "Inner action", + "tool_description": "Review this action independently from JavaScript", + }); + match call_id_source { + CallIdSource::Host | CallIdSource::Missing => {} + CallIdSource::NonString => meta["callId"] = json!(42), + CallIdSource::Unknown => meta["callId"] = json!("unknown-invocation"), + CallIdSource::WrongServer | CallIdSource::PreviousTurn => { + meta["callId"] = json!("seed-invocation"); + } + } + meta + }); + let server = responses::start_mock_server().await; + let fixture_config = json!({ + "command": if cfg!(windows) { "python" } else { "python3" }, + "args": ["-u", "-c", ELICITATION_SERVER, serde_json::to_string(&meta)?, + if call_id_source == CallIdSource::Host { "forward" } else { "omit" }], + "default_tools_approval_mode": "approve", + }); + let mut mcp_servers = json!({(server_name): fixture_config.clone()}); + if call_id_source == CallIdSource::WrongServer { + mcp_servers["cua_repl"] = fixture_config; + } + let mcp_servers = serde_json::from_value(mcp_servers)?; + let mut builder = test_codex().with_config(move |config| { + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + config.approvals_reviewer = if strict_auto_review { + ApprovalsReviewer::User + } else { + ApprovalsReviewer::AutoReview + }; + config + .mcp_servers + .set(mcp_servers) + .expect("set MCP fixture"); + }); + let test = builder.build_with_auto_env(&server).await?; + wait_for_mcp_server(&test.codex, server_name).await?; + if matches!( + call_id_source, + CallIdSource::WrongServer | CallIdSource::PreviousTurn + ) { + let seed_server = if call_id_source == CallIdSource::WrongServer { + "cua_repl" + } else { + server_name + }; + responses::mount_sse_once( + &server, + responses::sse(vec![ + responses::ev_function_call_with_namespace( + "seed-invocation", + &format!("mcp__{seed_server}"), + "js", + r#"{"seed":true}"#, + ), + responses::ev_completed("seed-call"), + ]), + ) + .await; + if call_id_source == CallIdSource::PreviousTurn { + responses::mount_sse_once( + &server, + responses::sse(vec![responses::ev_completed("seed-complete")]), + ) + .await; + test.submit_text_turn("Prepare the earlier invocation.") + .await?; + } + } + let parent_arguments = json!({"code": "await browser.doTwoActions()"}); + responses::mount_sse_once( + &server, + responses::sse(vec![ + responses::ev_function_call_with_namespace( + "eliciting-tool", + &format!("mcp__{server_name}"), + "js", + &parent_arguments.to_string(), + ), + responses::ev_completed("parent-tool"), + ]), + ) + .await; + let mut guardian = Vec::new(); + for outcome in ["allow", "deny"] { + guardian.push( + responses::mount_sse_once_match( + &server, + body_partial_json(json!({"client_metadata": {"x-openai-subagent": "guardian"}})), + responses::sse(vec![ + responses::ev_assistant_message( + "review-result", + &json!({ + "risk_level": "low", "user_authorization": "high", + "outcome": outcome, "rationale": "Independent inner action decision.", + }) + .to_string(), + ), + responses::ev_completed(outcome), + ]), + ) + .await, + ); + } + let parent = responses::mount_sse_once( + &server, + responses::sse(vec![responses::ev_completed("parent-complete")]), + ) + .await; + test.codex + .start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text { + text: "Run the JavaScript invocation with two reviewed inner actions.".into(), + text_elements: Vec::new(), + }])) + .await?; + let mut assessments = Vec::new(); + let mut tool_items = Vec::new(); + loop { + let event = tokio::time::timeout(Duration::from_secs(30), test.codex.next_event()) + .await + .context("timed out waiting for independent MCP Guardian reviews")??; + match event.msg { + EventMsg::GuardianAssessment(assessment) + if assessment.status != GuardianAssessmentStatus::InProgress => + { + assessments.push(assessment) + } + EventMsg::ItemCompleted(event) => { + if let TurnItem::McpToolCall(item) = event.item + && item.id == "eliciting-tool" + { + tool_items.push(item); + } + } + EventMsg::ElicitationRequest(_) => panic!("Guardian approval must not prompt the user"), + EventMsg::TurnComplete(_) => break, + _ => {} + } + } + assert_eq!(assessments.len(), 2); + assert_ne!(assessments[0].id, assessments[1].id); + let attributed = call_id_source == CallIdSource::Host && server_name != "elicitation"; + for (index, (assessment, status)) in assessments + .iter() + .zip([ + GuardianAssessmentStatus::Approved, + GuardianAssessmentStatus::Denied, + ]) + .enumerate() + { + let target = if attributed { + "eliciting-tool".to_string() + } else { + format!("mcp_elicitation:{server_name}:server-approval-{index}") + }; + assert_eq!( + (&assessment.target_item_id, &assessment.status), + (&Some(target), &status) + ); + assert_eq!( + assessment.action, + GuardianAssessmentAction::McpToolCall { + server: server_name.to_string(), + tool_name: meta[index]["tool_name"] + .as_str() + .expect("inner tool name") + .to_string(), + connector_id: Some("inner-connector".to_string()), + connector_name: Some("Inner Connector".to_string()), + tool_title: Some("Inner action".to_string()), + } + ); + let request = guardian[index].single_request(); + let prompt = request.message_input_texts("user").join("\n"); + let action_text = prompt + .rsplit_once("Planned action JSON:\n") + .context("Guardian planned action")? + .1 + .split_once("\n>>> APPROVAL REQUEST END") + .context("Guardian action end")? + .0; + assert_eq!( + serde_json::from_str::(action_text)?, + json!({ + "tool": "mcp_tool_call", "server": server_name, + "tool_name": meta[index]["tool_name"], "arguments": meta[index]["tool_params"], + "connector_id": "inner-connector", "connector_name": "Inner Connector", + "connector_description": "Connector for the reviewed inner action", + "tool_title": "Inner action", + "tool_description": "Review this action independently from JavaScript", + }) + ); + } + let [tool_item] = tool_items.as_slice() else { + panic!("expected one completed enclosing tool item: {tool_items:?}"); + }; + assert_eq!( + ( + tool_item.server.as_str(), + tool_item.tool.as_str(), + &tool_item.arguments, + tool_item.status + ), + ( + server_name, + "js", + &parent_arguments, + McpToolCallStatus::Completed + ) + ); + let output = parent + .single_request() + .function_call_output("eliciting-tool"); + let replies: Value = serde_json::from_str( + output["output"][1]["text"] + .as_str() + .context("MCP fixture responses")?, + )?; + assert_eq!(replies.as_object().map(serde_json::Map::len), Some(2)); + assert_eq!( + replies["server-approval-0"], + json!({ + "action": "accept", "content": {}, "_meta": {"approvals_reviewer": "auto_review"}, + }) + ); + assert_eq!(replies["server-approval-1"]["action"], "decline"); + assert_eq!( + replies["server-approval-1"]["_meta"]["approvals_reviewer"], + "auto_review" + ); + assert!( + replies["server-approval-1"]["_meta"]["message"] + .as_str() + .expect("Guardian rejection reason") + .contains("Independent inner action decision.") + ); + test.codex.shutdown_and_wait().await?; + Ok(()) +}