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) })); }