openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-08 20:40:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

Move Guardian MCP elicitation helpers into the reviewer crate (#57743)

Browse Source 
## What changed

Extract MCP approval elicitation parsing and review-decision response conversion from `core/src/session/mcp.rs` into `codex-guardian-reviewer`. Export the helpers and `GuardianElicitationReview` for the session code to use, while keeping session lookup, provenance checks, and approval enforcement in the host.

## Testing

Move the existing elicitation tests into the reviewer crate, retaining coverage for request construction, opt-in requirements, unsupported forms, decision mapping, and model-specific timeout instructions.

GitOrigin-RevId: fb745fa0e49a29f015bbda16c14b5fc2b12278b9
This commit is contained in:
jif
2026-10-28 13:56:17 +00:00
committed by copyberry
parent 29e3c61b48
commit ce2d47bdc4
7 changed files with 461 additions and 418 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/session/mcp.rs
+5 -187
View File
@@ -11,13 +11,16 @@ use codex_exec_server::ExecutorCapabilityDiscoveryV2Snapshot;
use codex_exec_server::MAX_SELECTED_CAPABILITY_ROOTS;
use codex_exec_server::ResolvedSelectedCapabilityRoot;
use codex_exec_server::supports_capability_discovery_v2;
use codex_guardian_reviewer::GuardianElicitationReview;
use codex_guardian_reviewer::guardian_elicitation_review_request;
use codex_guardian_reviewer::mcp_elicitation_decline_without_message;
use codex_guardian_reviewer::mcp_elicitation_response_from_guardian_decision;
use codex_mcp::CODEX_APPS_MCP_SERVER_NAME;
use codex_mcp::ElicitationReviewRequest;
use codex_mcp::ElicitationReviewer;
use codex_mcp::ElicitationReviewerHandle;
use codex_mcp::MCP_TOOL_CODEX_APPS_META_KEY;
use codex_otel::auth_storage::AuthStorageOriginator;
use codex_prompts::ResolvedModelMessages;
use codex_protocol::capabilities::CapabilityRootLocation;
use codex_protocol::capabilities::SelectedCapabilityRoot;
use codex_protocol::config_types::ApprovalsReviewer;
@@ -26,42 +29,21 @@ 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;
use codex_protocol::mcp_approval_meta::APPROVALS_REVIEWER_KEY as MCP_ELICITATION_APPROVALS_REVIEWER_KEY;
use codex_protocol::mcp_approval_meta::CONNECTOR_DESCRIPTION_KEY as MCP_ELICITATION_CONNECTOR_DESCRIPTION_KEY;
use codex_protocol::mcp_approval_meta::CONNECTOR_ID_KEY as MCP_ELICITATION_CONNECTOR_ID_KEY;
use codex_protocol::mcp_approval_meta::CONNECTOR_NAME_KEY as MCP_ELICITATION_CONNECTOR_NAME_KEY;
use codex_protocol::mcp_approval_meta::PERSIST_KEY as MCP_ELICITATION_PERSIST_KEY;
use codex_protocol::mcp_approval_meta::REQUEST_TYPE_APPROVAL_REQUEST as MCP_ELICITATION_REQUEST_TYPE_APPROVAL_REQUEST;
use codex_protocol::mcp_approval_meta::REQUEST_TYPE_KEY as MCP_ELICITATION_REQUEST_TYPE_KEY;
use codex_protocol::mcp_approval_meta::SENSITIVE_ACTION_KEY as MCP_ELICITATION_SENSITIVE_ACTION_KEY;
use codex_protocol::mcp_approval_meta::STRICT_AUTO_REVIEW_KEY as MCP_ELICITATION_STRICT_AUTO_REVIEW_KEY;
use codex_protocol::mcp_approval_meta::TOOL_DESCRIPTION_KEY as MCP_ELICITATION_TOOL_DESCRIPTION_KEY;
use codex_protocol::mcp_approval_meta::TOOL_NAME_KEY as MCP_ELICITATION_TOOL_NAME_KEY;
use codex_protocol::mcp_approval_meta::TOOL_PARAMS_KEY as MCP_ELICITATION_TOOL_PARAMS_KEY;
use codex_protocol::mcp_approval_meta::TOOL_TITLE_KEY as MCP_ELICITATION_TOOL_TITLE_KEY;
use codex_protocol::openai_models::GuardianScope;
use codex_protocol::openai_models::ModelInfo;
use codex_rmcp_client::Elicitation;
use rmcp::model::ElicitationAction;
use rmcp::model::RequestMetaObject;
use serde_json::Map;
const MCP_ELICITATION_DECLINE_MESSAGE_KEY: &str = "message";
const TOOL_SUGGESTION_ACTION_INSTALL: &str = "install";
const TOOL_SUGGESTION_ACTION_KEY: &str = "suggest_type";
const TOOL_SUGGESTION_TOOL_ID_KEY: &str = "tool_id";
const TOOL_SUGGESTION_TOOL_TYPE_KEY: &str = "tool_type";
#[derive(Debug, PartialEq)]
enum GuardianElicitationReview {
NotRequested,
Decline(&'static str),
ApprovalRequest {
request: Box<crate::guardian::GuardianApprovalRequest>,
tool_call_id: Option<String>,
},
}
struct GuardianMcpElicitationReviewer {
session: std::sync::Weak<Session>,
}
@@ -1223,7 +1205,7 @@ async fn review_guardian_mcp_elicitation(
Ok(decision.map(|decision| {
if let Some(reason) = declined_reason {
warn!(server_name = %request.server_name,
request_id = %mcp_elicitation_request_id(&request.request_id), reason,
request_id = %request.request_id, reason,
"declining Guardian MCP elicitation before review");
return mcp_elicitation_decline_without_message();
}
@@ -1231,115 +1213,12 @@ 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 {
meta,
requested_schema,
..
}) => (meta, Some(requested_schema)),
Elicitation::Mcp(rmcp::model::ElicitRequestParams::UrlElicitationParams {
meta, ..
}) => {
return if meta_requests_approval_request(meta) {
GuardianElicitationReview::Decline(
"guardian MCP elicitation review only supports form elicitations",
)
} else {
GuardianElicitationReview::NotRequested
};
}
Elicitation::Mcp(_) => {
return GuardianElicitationReview::Decline(
"guardian MCP elicitation review does not support this elicitation mode",
);
}
Elicitation::OpenAiForm { .. }
| Elicitation::OpenAiElicitationForm { .. }
| Elicitation::UserVerification { .. } => {
return GuardianElicitationReview::NotRequested;
}
};
let Some(meta) = meta.as_ref().map(|meta| &meta.0.0) else {
return GuardianElicitationReview::NotRequested;
};
if metadata_str(meta, MCP_ELICITATION_REQUEST_TYPE_KEY)
!= Some(MCP_ELICITATION_REQUEST_TYPE_APPROVAL_REQUEST)
{
return GuardianElicitationReview::NotRequested;
}
if metadata_str(meta, MCP_ELICITATION_APPROVAL_KIND_KEY)
!= Some(MCP_ELICITATION_APPROVAL_KIND_MCP_TOOL_CALL)
{
return GuardianElicitationReview::Decline(
"guardian MCP elicitation metadata must declare mcp_tool_call approval kind",
);
}
if requested_schema.is_some_and(|schema| !schema.properties.is_empty()) {
return GuardianElicitationReview::Decline(
"guardian MCP elicitation review only supports empty form schemas",
);
}
let Some(tool_name) = metadata_owned_string(meta, MCP_ELICITATION_TOOL_NAME_KEY) else {
return GuardianElicitationReview::Decline(
"guardian MCP elicitation metadata must include a non-empty tool_name",
);
};
let arguments = match meta.get(MCP_ELICITATION_TOOL_PARAMS_KEY) {
Some(value @ Value::Object(_)) => Some(value.clone()),
Some(_) => {
return GuardianElicitationReview::Decline(
"guardian MCP elicitation tool_params must be an object",
);
}
None => Some(Value::Object(Map::new())),
};
GuardianElicitationReview::ApprovalRequest {
// Synthetic display IDs must not participate in tool observation ordering.
tool_call_id: originating_call_id.map(str::to_owned),
request: Box::new(crate::guardian::GuardianApprovalRequest::McpToolCall {
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,
connector_id: metadata_owned_string(meta, MCP_ELICITATION_CONNECTOR_ID_KEY),
connector_name: metadata_owned_string(meta, MCP_ELICITATION_CONNECTOR_NAME_KEY),
connector_description: metadata_owned_string(
meta,
MCP_ELICITATION_CONNECTOR_DESCRIPTION_KEY,
),
connected_account_email: None,
tool_title: metadata_owned_string(meta, MCP_ELICITATION_TOOL_TITLE_KEY),
tool_description: metadata_owned_string(meta, MCP_ELICITATION_TOOL_DESCRIPTION_KEY),
annotations: None,
}),
}
}
fn elicitation_connector_id(elicitation: &Elicitation) -> Option<&str> {
elicitation
.meta()
.and_then(|meta| metadata_str(meta, MCP_ELICITATION_CONNECTOR_ID_KEY))
}
fn meta_requests_approval_request(meta: &Option<RequestMetaObject>) -> bool {
meta.as_ref()
.and_then(|meta| metadata_str(&meta.0, MCP_ELICITATION_REQUEST_TYPE_KEY))
== Some(MCP_ELICITATION_REQUEST_TYPE_APPROVAL_REQUEST)
}
fn metadata_str<'a>(meta: &'a Map<String, Value>, key: &str) -> Option<&'a str> {
meta.get(key).and_then(Value::as_str)
}
@@ -1378,67 +1257,6 @@ fn plugin_install_elicitation_telemetry_metadata(
})
}
fn mcp_elicitation_request_id(id: &RequestId) -> String {
match id {
rmcp::model::NumberOrString::String(value) => value.to_string(),
rmcp::model::NumberOrString::Number(value) => value.to_string(),
}
}
fn mcp_elicitation_response_from_guardian_decision(
decision: ReviewDecision,
model_info: &ModelInfo,
) -> ElicitationResponse {
match decision {
ReviewDecision::Approved
| ReviewDecision::ApprovedForSession
| ReviewDecision::ApprovedMcpPolicyAmendment
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
| ReviewDecision::NetworkPolicyAmendment { .. } => ElicitationResponse {
action: ElicitationAction::Accept,
content: Some(serde_json::json!({})),
meta: Some(mcp_elicitation_auto_meta()),
},
ReviewDecision::Denied { rejection } => mcp_elicitation_decline_with_message(rejection),
ReviewDecision::TimedOut => mcp_elicitation_decline_with_message(
ResolvedModelMessages::from_model(model_info)
.auto_review()
.timeout_instructions
.to_string(),
),
ReviewDecision::Abort => ElicitationResponse {
action: ElicitationAction::Cancel,
content: None,
meta: Some(mcp_elicitation_auto_meta()),
},
}
}
fn mcp_elicitation_decline_with_message(message: String) -> ElicitationResponse {
ElicitationResponse {
action: ElicitationAction::Decline,
content: None,
meta: Some(serde_json::json!({
MCP_ELICITATION_DECLINE_MESSAGE_KEY: message,
MCP_ELICITATION_APPROVALS_REVIEWER_KEY: ApprovalsReviewer::AutoReview,
})),
}
}
fn mcp_elicitation_decline_without_message() -> ElicitationResponse {
ElicitationResponse {
action: ElicitationAction::Decline,
content: None,
meta: Some(mcp_elicitation_auto_meta()),
}
}
fn mcp_elicitation_auto_meta() -> serde_json::Value {
serde_json::json!({
MCP_ELICITATION_APPROVALS_REVIEWER_KEY: ApprovalsReviewer::AutoReview,
})
}
#[cfg(test)]
#[path = "mcp_tests.rs"]
mod tests;
codex-rs/core/src/session/mcp_tests.rs
-231
View File
@@ -1,96 +1,7 @@
use super::*;
use pretty_assertions::assert_eq;
use rmcp::model::BooleanSchema;
use rmcp::model::ElicitationSchema;
use rmcp::model::PrimitiveSchemaDefinition;
use serde_json::json;
fn meta(value: Value) -> Option<RequestMetaObject> {
let Value::Object(map) = value else {
panic!("metadata must be an object");
};
Some(RequestMetaObject::from(map))
}
fn guardian_meta(tool_params: Option<Value>) -> Option<RequestMetaObject> {
let mut value = json!({
"codex_approval_kind": "mcp_tool_call",
"codex_request_type": "approval_request",
"connector_id": "browser-use",
"connector_name": "Browser Use",
"tool_name": "access_browser_origin",
"tool_title": "Access browser origin",
});
if let Some(tool_params) = tool_params {
value["tool_params"] = tool_params;
}
meta(value)
}
fn form_request(meta: Option<RequestMetaObject>) -> ElicitationReviewRequest {
ElicitationReviewRequest {
server_name: "browser-use".to_string(),
request_id: rmcp::model::NumberOrString::Number(7),
elicitation: Elicitation::Mcp(rmcp::model::ElicitRequestParams::FormElicitationParams {
meta,
message: "Allow origin?".to_string(),
requested_schema: ElicitationSchema::builder()
.build()
.expect("schema should build"),
}),
}
}
#[test_case::test_case(None, "mcp_elicitation:browser-use:7"; "unlinked elicitation")]
#[test_case::test_case(Some("browser-call"), "browser-call"; "linked elicitation")]
fn guardian_elicitation_review_request_builds_mcp_tool_call(
originating_call_id: Option<&str>,
expected_request_id: &str,
) {
let request = form_request(guardian_meta(Some(json!({
"origin": "https://example.com",
}))));
assert_eq!(
guardian_elicitation_review_request(&request, originating_call_id),
GuardianElicitationReview::ApprovalRequest {
tool_call_id: originating_call_id.map(str::to_owned),
request: Box::new(crate::guardian::GuardianApprovalRequest::McpToolCall {
id: expected_request_id.to_owned(),
server: "browser-use".to_owned(),
tool_name: "access_browser_origin".to_owned(),
arguments: Some(json!({ "origin": "https://example.com" })),
connector_id: Some("browser-use".to_owned()),
connector_name: Some("Browser Use".to_owned()),
connector_description: None,
connected_account_email: None,
tool_title: Some("Access browser origin".to_owned()),
tool_description: None,
annotations: None,
}),
}
);
}
#[test]
fn guardian_elicitation_review_request_defaults_missing_tool_params() {
let request = form_request(guardian_meta(/*tool_params*/ None));
let GuardianElicitationReview::ApprovalRequest {
request: guardian_request,
..
} = guardian_elicitation_review_request(&request, /*originating_call_id*/ None)
else {
panic!("expected Guardian MCP tool call request");
};
let crate::guardian::GuardianApprovalRequest::McpToolCall { arguments, .. } = *guardian_request
else {
panic!("expected Guardian MCP tool call request");
};
assert_eq!(arguments, Some(json!({})));
}
#[test]
fn plugin_install_elicitation_telemetry_metadata_requires_install_tool_suggestion() {
let event = EventMsg::ElicitationRequest(ElicitationRequestEvent {
@@ -147,145 +58,3 @@ fn plugin_install_elicitation_telemetry_metadata_requires_install_tool_suggestio
None
);
}
#[test]
fn guardian_elicitation_review_request_requires_opt_in() {
let request = form_request(meta(json!({
"codex_approval_kind": "mcp_tool_call",
"tool_name": "access_browser_origin",
})));
assert_eq!(
guardian_elicitation_review_request(&request, /*originating_call_id*/ None),
GuardianElicitationReview::NotRequested
);
}
#[test]
fn guardian_elicitation_review_request_declines_unsupported_opt_in_shapes() {
let url_request = ElicitationReviewRequest {
server_name: "browser-use".to_string(),
request_id: rmcp::model::NumberOrString::Number(8),
elicitation: Elicitation::Mcp(rmcp::model::ElicitRequestParams::UrlElicitationParams {
meta: guardian_meta(Some(json!({}))),
message: "Open URL".to_string(),
url: "https://example.com".to_string(),
elicitation_id: "elicit-1".to_string(),
}),
};
assert!(matches!(
guardian_elicitation_review_request(&url_request, /*originating_call_id*/ None),
GuardianElicitationReview::Decline(_)
));
let non_empty_schema_request = ElicitationReviewRequest {
server_name: "browser-use".to_string(),
request_id: rmcp::model::NumberOrString::Number(9),
elicitation: Elicitation::Mcp(rmcp::model::ElicitRequestParams::FormElicitationParams {
meta: guardian_meta(Some(json!({}))),
message: "Allow origin?".to_string(),
requested_schema: ElicitationSchema::builder()
.required_property(
"confirmed",
PrimitiveSchemaDefinition::Boolean(BooleanSchema::new()),
)
.build()
.expect("schema should build"),
}),
};
assert!(matches!(
guardian_elicitation_review_request(
&non_empty_schema_request,
/*originating_call_id*/ None
),
GuardianElicitationReview::Decline(_)
));
let missing_tool_name_request = form_request(meta(json!({
"codex_approval_kind": "mcp_tool_call",
"codex_request_type": "approval_request",
})));
assert!(matches!(
guardian_elicitation_review_request(
&missing_tool_name_request,
/*originating_call_id*/ None
),
GuardianElicitationReview::Decline(_)
));
}
#[test]
fn guardian_decisions_map_to_elicitation_responses_without_session_state() {
let model = codex_models_manager::model_info::model_info_from_slug("acting-model");
assert_eq!(
mcp_elicitation_response_from_guardian_decision(ReviewDecision::Approved, &model),
ElicitationResponse {
action: ElicitationAction::Accept,
content: Some(json!({})),
meta: Some(json!({
"approvals_reviewer": ApprovalsReviewer::AutoReview,
})),
}
);
assert_eq!(
mcp_elicitation_response_from_guardian_decision(
ReviewDecision::denied("Denied by Guardian"),
&model,
),
ElicitationResponse {
action: ElicitationAction::Decline,
content: None,
meta: Some(json!({
"approvals_reviewer": ApprovalsReviewer::AutoReview,
"message": "Denied by Guardian",
})),
}
);
assert_eq!(
mcp_elicitation_response_from_guardian_decision(ReviewDecision::TimedOut, &model),
ElicitationResponse {
action: ElicitationAction::Decline,
content: None,
meta: Some(json!({
"approvals_reviewer": ApprovalsReviewer::AutoReview,
"message": codex_prompts::ResolvedModelMessages::from_model(&model).auto_review().timeout_instructions,
})),
}
);
assert_eq!(
mcp_elicitation_response_from_guardian_decision(ReviewDecision::Abort, &model),
ElicitationResponse {
action: ElicitationAction::Cancel,
content: None,
meta: Some(json!({
"approvals_reviewer": ApprovalsReviewer::AutoReview,
})),
}
);
}
#[test]
fn guardian_elicitation_timeout_uses_acting_model_instructions() {
let mut model = codex_models_manager::model_info::model_info_from_slug("acting-model");
for timeout_instructions in ["Catalog timeout instructions.", ""] {
model.model_messages = Some(
serde_json::from_value(json!({
"auto_review": {
"timeout_instructions": timeout_instructions,
},
}))
.expect("model messages should deserialize"),
);
assert_eq!(
mcp_elicitation_response_from_guardian_decision(ReviewDecision::TimedOut, &model),
ElicitationResponse {
action: ElicitationAction::Decline,
content: None,
meta: Some(json!({
"approvals_reviewer": ApprovalsReviewer::AutoReview,
"message": timeout_instructions,
})),
}
);
}
}