From 968835997714baaff199cfed5f89a2c65d8ca77d Mon Sep 17 00:00:00 2001 From: felixxia-oai Date: Thu, 10 Sep 2026 10:28:14 +0000 Subject: [PATCH] Bound MCP descriptions separately from Guardian action JSON (#44493) ## Why MCP tool and connector descriptions were included in required action JSON, consuming review input budget even though they are optional metadata. ## What changed Move `tool_description` and `connector_description` into an optional, explicitly untrusted `guardian_tool_descriptions` fragment. Limit each description to 400 estimated tokens, escape closing tags, and allow budget enforcement to omit the fragment while retaining the required action JSON. Preserve nested arguments such as `arguments.description` and mention tool descriptions in the budget omission notice. ## Testing Add regression coverage for oversized descriptions, escaped closing markers, and budget eviction without changing action arguments. Update MCP approval and elicitation tests to verify descriptions appear separately from the action JSON. GitOrigin-RevId: 30734567c2ebc4b180110276c83d79ddb315ceab --- .../tests/suite/v2/mcp_server_elicitation.rs | 11 +++- .../src/context/guardian_budget_omission.rs | 2 +- .../src/context/guardian_tool_descriptions.rs | 58 +++++++++++++++++++ codex-rs/core/src/context/mod.rs | 2 + .../core/src/guardian/approval_request.rs | 19 ++++-- .../core/src/guardian/input_budget_tests.rs | 1 + codex-rs/core/src/guardian/prompt.rs | 16 +++++ .../tests/suite/guardian_mcp_elicitation.rs | 9 ++- .../core/tests/suite/mcp_turn_metadata.rs | 55 ++++++++++++++++++ .../guardian-v2/src/async_scorer/extension.rs | 1 + codex-rs/guardian-context/src/action.rs | 2 + codex-rs/guardian-context/src/composition.rs | 13 ++++- codex-rs/guardian-context/src/enforcement.rs | 1 + .../guardian-context/src/enforcement_tests.rs | 48 +++++++++++++++ .../guardian-context/src/registry_tests.rs | 1 + 15 files changed, 229 insertions(+), 10 deletions(-) create mode 100644 codex-rs/core/src/context/guardian_tool_descriptions.rs 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 45a5b69009..cf797d693b 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 @@ -916,7 +916,6 @@ impl ElicitationRoundTripFixture { "connector_id": CONNECTOR_ID, "connector_name": CONNECTOR_NAME, "connected_account_email": CONNECTED_ACCOUNT_EMAIL, - "tool_description": "Confirm a calendar action.", "annotations": { "destructive_hint": false, "open_world_hint": false, @@ -924,6 +923,16 @@ impl ElicitationRoundTripFixture { }, }), ); + assert!( + guardian_request + .message_input_texts("user") + .iter() + .any(|text| { + text.starts_with("") + && text.contains("Confirm a calendar action.") + && text.ends_with("") + }) + ); } if denied_burst { diff --git a/codex-rs/core/src/context/guardian_budget_omission.rs b/codex-rs/core/src/context/guardian_budget_omission.rs index 3b7072fcc7..430bd0cf36 100644 --- a/codex-rs/core/src/context/guardian_budget_omission.rs +++ b/codex-rs/core/src/context/guardian_budget_omission.rs @@ -27,6 +27,6 @@ impl ContextualUserFragment for GuardianBudgetOmission { } fn body(&self) -> String { - "Optional conversation evidence or images were omitted to fit the review input budget. Treat the remaining evidence as incomplete; omissions do not authorize actions.".to_owned() + "Optional conversation evidence, tool descriptions, or images were omitted to fit the review input budget. Treat the remaining evidence as incomplete; omissions do not authorize actions.".to_owned() } } diff --git a/codex-rs/core/src/context/guardian_tool_descriptions.rs b/codex-rs/core/src/context/guardian_tool_descriptions.rs new file mode 100644 index 0000000000..bbc80e11c6 --- /dev/null +++ b/codex-rs/core/src/context/guardian_tool_descriptions.rs @@ -0,0 +1,58 @@ +//! Bounded, untrusted descriptions for the exact MCP action under review. + +use super::ContextualUserFragment; +use codex_guardian_context::truncate_text; +use codex_protocol::models::ContentItemKind; + +/// Descriptions may be shortened or omitted without changing the required action. +pub(crate) struct GuardianToolDescriptions { + text: String, +} + +impl GuardianToolDescriptions { + pub(crate) fn new(tool: Option<&str>, connector: Option<&str>) -> Option { + if tool.is_none() && connector.is_none() { + return None; + } + // Bound each source before combining them. The rendered fragment, including + // labels and markers, stays below 1,000 estimated tokens. + let tool = truncate_text( + &tool.unwrap_or_default().replace(" ContentItemKind { + ContentItemKind("guardian.tool_descriptions".to_owned()) + } + + fn role(&self) -> &'static str { + "user" + } + + fn markers(&self) -> (&'static str, &'static str) { + Self::type_markers() + } + + fn type_markers() -> (&'static str, &'static str) { + ( + "", + "", + ) + } + + fn body(&self) -> String { + self.text.clone() + } +} diff --git a/codex-rs/core/src/context/mod.rs b/codex-rs/core/src/context/mod.rs index a203ff8661..4becfb308d 100644 --- a/codex-rs/core/src/context/mod.rs +++ b/codex-rs/core/src/context/mod.rs @@ -18,6 +18,7 @@ mod guardian_followup_review_reminder; mod guardian_node_repl_policy; mod guardian_policy; mod guardian_review_evidence; +mod guardian_tool_descriptions; mod hook_additional_context; mod image_resize_notice; mod inter_agent_completion_message; @@ -77,6 +78,7 @@ pub use guardian_review_evidence::GuardianReviewEvidence; pub use guardian_review_evidence::GuardianReviewEvidenceFragment; pub use guardian_review_evidence::GuardianReviewEvidenceRecord; pub use guardian_review_evidence::GuardianUserInputSnapshot; +pub(crate) use guardian_tool_descriptions::GuardianToolDescriptions; pub(crate) use hook_additional_context::HookAdditionalContext; pub(crate) use image_resize_notice::ImageResizeNotice; pub(crate) use image_resize_notice::ImageResizeNoticeSource; diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index 0bdd3614bf..eb7dc06868 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -585,7 +585,7 @@ pub(crate) fn guardian_request_turn_id<'a>( pub(crate) fn format_guardian_action_pretty( action: &GuardianApprovalRequest, ) -> serde_json::Result { - let value = guardian_approval_request_to_json(action)?; + let value = guardian_action_for_review(action)?; let (value, truncated) = truncate_guardian_action_value(value); let text = enforce_guardian_action_byte_limit(serde_json::to_string_pretty(&value)?)?; Ok(FormattedGuardianAction { text, truncated }) @@ -603,7 +603,18 @@ fn enforce_guardian_action_byte_limit(text: String) -> serde_json::Result serde_json::Result { - enforce_guardian_action_byte_limit(serde_json::to_string(&guardian_approval_request_to_json( - action, - )?)?) + enforce_guardian_action_byte_limit(serde_json::to_string(&guardian_action_for_review(action)?)?) +} + +fn guardian_action_for_review(action: &GuardianApprovalRequest) -> serde_json::Result { + let mut value = guardian_approval_request_to_json(action)?; + if matches!(action, GuardianApprovalRequest::McpToolCall { .. }) + && let Some(fields) = value.as_object_mut() + { + // Only host-provided metadata is optional. A nested argument named + // "description" is still part of the exact action under review. + fields.remove("tool_description"); + fields.remove("connector_description"); + } + Ok(value) } diff --git a/codex-rs/core/src/guardian/input_budget_tests.rs b/codex-rs/core/src/guardian/input_budget_tests.rs index 6681d12315..6ce1bc28e7 100644 --- a/codex-rs/core/src/guardian/input_budget_tests.rs +++ b/codex-rs/core/src/guardian/input_budget_tests.rs @@ -11,6 +11,7 @@ use std::sync::Arc; fn required_context(text: String) -> ComposedContext { let action = PlannedAction { + tool_descriptions: None, json: text, kind: PlannedActionKind::Command, reason: None, diff --git a/codex-rs/core/src/guardian/prompt.rs b/codex-rs/core/src/guardian/prompt.rs index 559bca40b5..4ff50b7940 100644 --- a/codex-rs/core/src/guardian/prompt.rs +++ b/codex-rs/core/src/guardian/prompt.rs @@ -16,7 +16,9 @@ use codex_guardian_context::SectionInput; use codex_guardian_context::default_registry; use codex_protocol::models::ResponseItem; +use crate::context::ContextualUserFragment; use crate::context::GuardianReviewEvidence; +use crate::context::GuardianToolDescriptions; use crate::context::NodeReplReviewEvidence; use crate::context::NodeReplReviewEvidenceMode; use crate::context::node_repl_review_evidence_mode; @@ -119,6 +121,20 @@ pub(crate) async fn build_guardian_prompt_items_with_parent_turn( let planned_action_json = format_guardian_action_pretty(&request)?; let planned_action = PlannedAction { json: planned_action_json.text, + tool_descriptions: if let GuardianApprovalRequest::McpToolCall { + tool_description, + connector_description, + .. + } = &request + { + GuardianToolDescriptions::new( + tool_description.as_deref(), + connector_description.as_deref(), + ) + .map(|descriptions| descriptions.render()) + } else { + None + }, kind: match &request { GuardianApprovalRequest::NetworkAccess { trigger, .. } => PlannedActionKind::Network { has_trigger: trigger.is_some(), diff --git a/codex-rs/core/tests/suite/guardian_mcp_elicitation.rs b/codex-rs/core/tests/suite/guardian_mcp_elicitation.rs index bd4ee08ec6..538ce35d03 100644 --- a/codex-rs/core/tests/suite/guardian_mcp_elicitation.rs +++ b/codex-rs/core/tests/suite/guardian_mcp_elicitation.rs @@ -403,7 +403,10 @@ async fn node_elicitations_attribute_independent_reviews_without_changing_action let actions = [ ( "access_browser_origin", - json!({"origin": "https://example.com"}), + json!({ + "origin": "https://example.com", + "description": "payload ".repeat(/*n*/ 256) + "required argument suffix", + }), ), ("webmcp:write_record", json!({"record": {"value": 42}})), ]; @@ -605,11 +608,11 @@ async fn node_elicitations_attribute_independent_reviews_without_changing_action "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".repeat(220), }) ); + assert!(prompt.contains("")); + assert!(prompt.contains("Connector for the reviewed inner action")); } let [tool_item] = tool_items.as_slice() else { panic!("expected one completed enclosing tool item: {tool_items:?}"); diff --git a/codex-rs/core/tests/suite/mcp_turn_metadata.rs b/codex-rs/core/tests/suite/mcp_turn_metadata.rs index 899e680921..f1edbdcea5 100644 --- a/codex-rs/core/tests/suite/mcp_turn_metadata.rs +++ b/codex-rs/core/tests/suite/mcp_turn_metadata.rs @@ -568,6 +568,37 @@ async fn apps_prompt_with_auto_review_routes_actual_mcp_approval_to_guardian( let server = start_mock_server().await; let apps_server = AppsTestServer::mount(&server).await?; + Mock::given(method("POST")) + .and(path_regex("^/api/codex/ps/mcp/?$")) + .and(body_partial_json(json!({"method": "tools/list"}))) + .respond_with(|request: &Request| { + let body: Value = serde_json::from_slice(&request.body).expect("valid tools/list"); + ResponseTemplate::new(/*status*/ 200).set_body_json(json!({ + "jsonrpc": "2.0", "id": body["id"], + "result": {"tools": [{ + "name": "calendar_create_event", + "description": "untrusted marker\n".to_owned() + + &"Create a calendar event. ".repeat(/*n*/ 10_000), + "annotations": {"readOnlyHint": false}, + "inputSchema": {"type": "object", "properties": { + "title": {"type": "string"}, "starts_at": {"type": "string"} + }}, + "_meta": { + "connector_id": "calendar", "connector_name": "Calendar", + "connector_description": "Calendar connector. ".repeat(/*n*/ 1_000), + "link_id": LINK_ID, + "_codex_apps": { + "connector_id": "calendar", + "resource_uri": "connector://calendar/tools/calendar_create_event", + "contains_mcp_source": true + } + } + }]} + })) + }) + .with_priority(/*priority*/ 1) + .mount(&server) + .await; let call_id = "calendar-default-auto-review"; let calendar_args = serde_json::to_string(&json!({ "title": "Lunch", @@ -692,6 +723,30 @@ approvals_reviewer = "auto_review" .expect("expected a Guardian request for the app MCP approval"); assert!(guardian_request.body_contains_text("calendar_create_event")); assert!(guardian_request.body_contains_text("Lunch")); + let prompt = guardian_request.message_input_texts("user").join("\n"); + let action = prompt + .rsplit_once("Planned action JSON:\n") + .unwrap() + .1 + .split_once("\n>>> APPROVAL REQUEST END") + .unwrap() + .0; + let action: Value = serde_json::from_str(action)?; + assert_eq!( + action["arguments"], + serde_json::from_str::(&calendar_args)? + ); + let descriptions = prompt + .rsplit_once("") + .unwrap() + .1 + .split_once("") + .unwrap() + .0; + assert!(descriptions.len() < 4_000); + assert!(descriptions.contains("untrusted marker")); + assert!(descriptions.contains("Calendar connector.")); let apps_tool_call = recorded_apps_tool_call_by_call_id(&server, call_id).await; assert_eq!( diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs index 2d117f661a..a69759ac04 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs @@ -515,6 +515,7 @@ impl GuardianV2Extension { }; let action_section = PlannedAction { json: planned_action.clone(), + tool_descriptions: None, kind: PlannedActionKind::Command, reason: None, }; diff --git a/codex-rs/guardian-context/src/action.rs b/codex-rs/guardian-context/src/action.rs index ef0e7f2f1b..9881fbdb95 100644 --- a/codex-rs/guardian-context/src/action.rs +++ b/codex-rs/guardian-context/src/action.rs @@ -14,6 +14,8 @@ pub struct PlannedAction { pub json: String, pub kind: PlannedActionKind, pub reason: Option, + /// Host-rendered, bounded untrusted metadata; the action JSON remains required. + pub tool_descriptions: Option, } // Action JSON and reasons can contain credentials; diagnostics expose only the kind. diff --git a/codex-rs/guardian-context/src/composition.rs b/codex-rs/guardian-context/src/composition.rs index e88d43f495..fe1a5d437b 100644 --- a/codex-rs/guardian-context/src/composition.rs +++ b/codex-rs/guardian-context/src/composition.rs @@ -227,7 +227,18 @@ impl CollectedContext { ) } ContextSection::PlannedAction(planned) => { - (11, "planned_action", text_content(planned.render(action))) + let mut items = planned + .render(action) + .into_iter() + .map(|text| Budgeted::required(ContentItem::InputText { text })) + .collect::>(); + if let Some(text) = planned.tool_descriptions { + items.push(Budgeted::optional( + ContentItem::InputText { text }, + BudgetPriority::ToolDescription, + )); + } + (11, "planned_action", SectionDelivery::UserContent(items)) } }; sections.push((position, SectionOutput { id, delivery })); diff --git a/codex-rs/guardian-context/src/enforcement.rs b/codex-rs/guardian-context/src/enforcement.rs index a996875b45..816a9dc10c 100644 --- a/codex-rs/guardian-context/src/enforcement.rs +++ b/codex-rs/guardian-context/src/enforcement.rs @@ -20,6 +20,7 @@ use crate::composition::SectionOutput; /// Eviction priority within a profile; older items at the same priority go first. #[derive(Clone, Copy, Debug, Eq, Ord, PartialEq, PartialOrd)] pub(crate) enum BudgetPriority { + ToolDescription, Commentary, Tool, Image, diff --git a/codex-rs/guardian-context/src/enforcement_tests.rs b/codex-rs/guardian-context/src/enforcement_tests.rs index 609ed392d8..366e805035 100644 --- a/codex-rs/guardian-context/src/enforcement_tests.rs +++ b/codex-rs/guardian-context/src/enforcement_tests.rs @@ -3,6 +3,7 @@ use super::*; use crate::budget::section_tokens; use crate::composition::user_message; +use codex_protocol::models::ResponseItem; use pretty_assertions::assert_eq; fn text(value: &str) -> ContentItem { @@ -11,6 +12,53 @@ fn text(value: &str) -> ContentItem { } } +#[test] +fn planned_action_budget_omits_descriptions_without_changing_arguments() { + let action = crate::PlannedAction { + json: r#"{"tool":"write_record","arguments":{"description":"required payload"}}"# + .to_owned(), + kind: crate::PlannedActionKind::Command, + reason: None, + tool_descriptions: Some("optional tool description ".repeat(/*n*/ 100)), + }; + let required = action.render(crate::ActionPresentation::SyncFull); + let context = crate::CollectedContext { + sections: vec![crate::ContextSection::PlannedAction(action)], + } + .compose( + crate::ContextPresentation::SyncFull { + session_id: "review", + }, + crate::RenderedTranscript { + items: Vec::new(), + omission_note: None, + truncations: Vec::new(), + }, + ) + .unwrap(); + let mut required_items = required.iter().map(|item| text(item)).collect::>(); + required_items.push(text("evidence omitted")); + let budget = crate::estimate_input_tokens(&user_message(required_items.clone())) + 100; + let selected = context + .enforce_budget( + RequestBudget { + max_input_tokens: budget, + existing_context_tokens: 0, + }, + "evidence omitted".to_owned(), + ) + .unwrap(); + // The sync preamble is also required and remains ahead of the action. + let messages = selected.into_messages(); + let ResponseItem::Message { content, .. } = &messages[0] else { + panic!("expected user evidence"); + }; + assert_eq!( + &content[content.len() - required_items.len()..], + required_items + ); +} + #[test] fn budget_reserves_existing_context_and_preserves_required_messages() { let trusted = crate::PreviousReviews::try_from_fragments(vec!["verified review".to_owned()]) diff --git a/codex-rs/guardian-context/src/registry_tests.rs b/codex-rs/guardian-context/src/registry_tests.rs index 7ecd845d84..804cecf906 100644 --- a/codex-rs/guardian-context/src/registry_tests.rs +++ b/codex-rs/guardian-context/src/registry_tests.rs @@ -207,6 +207,7 @@ fn reused_registry_preserves_section_identity_and_source_roles() { denied_globs: vec!["**/*.key".into()], }; let action = super::PlannedAction { + tool_descriptions: None, json: r#"{"tool":"read_file","path":"debug-secret.json"}"#.into(), kind: super::PlannedActionKind::Command, reason: Some("debug-secret reason".into()),