From 970b7f2ff4f612b8e8cd340eb6b6d789d7141dd2 Mon Sep 17 00:00:00 2001 From: Ankush Gupta Date: Fri, 21 Aug 2026 23:08:37 +0000 Subject: [PATCH] Preserve strict MCP auto-review outcomes (#40031) ## What changed Propagate canonical denial, timeout, and abort responses from strict MCP auto-review instead of replacing them with a generic decline. This preserves the reviewer's action and metadata, including denial rationale, while still failing closed when no canonical decision is available. Update the fallback message to direct the agent to request explicit user approval before proceeding. ## Testing Add unit and app-server integration coverage for strict auto-review denials and cancellations. GitOrigin-RevId: d8126ab223ab546b95e2d984f6e4dd540a324a59 --- .../tests/suite/v2/mcp_server_elicitation.rs | 40 +++++++++++++------ codex-rs/codex-mcp/src/elicitation.rs | 9 +++-- codex-rs/codex-mcp/src/elicitation_tests.rs | 29 ++++++++++++++ codex-rs/core/src/session/mcp.rs | 9 ++++- 4 files changed, 71 insertions(+), 16 deletions(-) diff --git a/codex-rs/app-server/tests/suite/v2/mcp_server_elicitation.rs b/codex-rs/app-server/tests/suite/v2/mcp_server_elicitation.rs index 51821100fb..bd39b7755f 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_server_elicitation.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_server_elicitation.rs @@ -101,8 +101,8 @@ const TOOL_NAME: &str = "calendar_confirm_action"; const TOOL_CALL_ID: &str = "call-calendar-confirm"; const NEXT_TURN_TOOL_CALL_ID: &str = "call-calendar-next-turn"; const ELICITATION_MESSAGE: &str = "Allow this request?"; -const STRICT_DECLINE_MESSAGE: &str = - "Strict automated review failed. Do not proceed or ask the user for approval."; +const STRICT_DECLINE_MESSAGE: &str = "Automated review of this operation failed. Do not proceed without asking the user for explicit approval."; +const GUARDIAN_DENIAL_RATIONALE: &str = "The calendar action exceeds the user's authorization."; const OPENAI_FORM_MESSAGE: &str = "Select a template"; const IMAGE_DATA_URL: &str = "data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciLz4="; @@ -529,12 +529,14 @@ async fn start_elicitation_services( if let ElicitationScenario::Strict(strict) = scenario { let completion = streams.pop().expect("parent model completion"); for approved in strict.review_outcomes() { + let assessment = if *approved { + json!({ "outcome": "allow" }) + } else { + json!({ "outcome": "deny", "rationale": GUARDIAN_DENIAL_RATIONALE }) + }; streams.push(responses::sse(vec![ responses::ev_response_created("resp-guardian"), - responses::ev_assistant_message( - "msg-guardian", - &json!({ "outcome": if *approved { "allow" } else { "deny" } }).to_string(), - ), + responses::ev_assistant_message("msg-guardian", &assessment.to_string()), responses::ev_completed("resp-guardian"), ])); } @@ -1094,17 +1096,31 @@ impl ServerHandler for ElicitationAppsMcpServer { .map_err(|err| { rmcp::ErrorData::internal_error(err.to_string(), None) })?; - let expected = if strict.review_outcomes().get(index) == Some(&true) { - json!({ + let expected = match strict.review_outcomes().get(index) { + Some(true) => json!({ "action": "accept", "content": {}, "_meta": { "approvals_reviewer": "auto_review" }, - }) - } else { - json!({ + }), + Some(false) => json!({ + "action": "decline", + "_meta": { + "approvals_reviewer": "auto_review", + "message": format!( + "This action was rejected due to unacceptable risk.\n\ + Reason: {GUARDIAN_DENIAL_RATIONALE}\n\ + The agent must not attempt to achieve the same outcome via workaround, \ + indirect execution, or policy circumvention. \ + Proceed only with a materially safer alternative, \ + or if the user explicitly approves the action after being informed of the risk. \ + Otherwise, stop and request user input." + ), + }, + }), + None => json!({ "action": "decline", "_meta": { "message": STRICT_DECLINE_MESSAGE }, - }) + }), }; assert_eq!( serde_json::to_value(result) diff --git a/codex-rs/codex-mcp/src/elicitation.rs b/codex-rs/codex-mcp/src/elicitation.rs index 08257d8fbe..6218419449 100644 --- a/codex-rs/codex-mcp/src/elicitation.rs +++ b/codex-rs/codex-mcp/src/elicitation.rs @@ -41,8 +41,7 @@ use tokio::sync::oneshot; static NEXT_ELICITATION_REQUEST_ID: AtomicU64 = AtomicU64::new(0); -const STRICT_AUTO_REVIEW_DECLINE_MESSAGE: &str = - "Strict automated review failed. Do not proceed or ask the user for approval."; +const STRICT_AUTO_REVIEW_DECLINE_MESSAGE: &str = "Automated review of this operation failed. Do not proceed without asking the user for explicit approval."; #[derive(Debug, Clone)] pub struct ElicitationReviewRequest { @@ -278,8 +277,12 @@ impl ElicitationRequestManager { .await { Ok(Some(response)) - if response.action == ElicitationAction::Accept + if (response.action == ElicitationAction::Accept && response.content == Some(serde_json::json!({})) + || matches!( + response.action, + ElicitationAction::Decline | ElicitationAction::Cancel + ) && response.content.is_none()) && response .meta .as_ref() diff --git a/codex-rs/codex-mcp/src/elicitation_tests.rs b/codex-rs/codex-mcp/src/elicitation_tests.rs index de16aa007a..1c206a0a4e 100644 --- a/codex-rs/codex-mcp/src/elicitation_tests.rs +++ b/codex-rs/codex-mcp/src/elicitation_tests.rs @@ -227,6 +227,35 @@ async fn strict_auto_review_respects_explicit_elicitation_denials() { } } +#[tokio::test] +async fn strict_auto_review_preserves_guardian_denials_and_cancellations() { + for response in [ + ElicitationResponse { + action: ElicitationAction::Decline, + content: None, + meta: Some(json!({ + "approvals_reviewer": "auto_review", + "message": "The user has not authorized sending this data. Ask the user for approval.", + })), + }, + ElicitationResponse { + action: ElicitationAction::Cancel, + content: None, + meta: Some(json!({ "approvals_reviewer": "auto_review" })), + }, + ] { + let reviewer = RecordingReviewer::new(Ok(Some(response.clone()))); + let (_, events, sender) = elicitation_fixture( + AskForApproval::Never, + PermissionProfile::Disabled, + Some(reviewer.clone()), + ); + assert_eq!(send_elicitation(&sender, Some(json!(true))).await, response); + assert_eq!(reviewer.calls.load(Relaxed), 1); + assert!(events.is_empty(), "strict review must not emit an event"); + } +} + #[tokio::test] async fn strict_auto_review_fails_closed_without_a_canonical_decision() { for marker in ["null", "\"true\"", "1", "{}", "[true]"] { diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 9ae36d2536..afec74ffe8 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -807,7 +807,14 @@ async fn review_guardian_mcp_elicitation( ) .await; - return Ok(matches!(decision, ReviewDecision::Approved).then(|| { + return Ok(matches!( + decision, + ReviewDecision::Approved + | ReviewDecision::Denied { .. } + | ReviewDecision::TimedOut + | ReviewDecision::Abort + ) + .then(|| { mcp_elicitation_response_from_guardian_decision(decision, &turn_context.model_info) })); }