From fd346b8dbaa24573a0244bc917811849d27c4cf4 Mon Sep 17 00:00:00 2001 From: felixxia-oai Date: Wed, 16 Sep 2026 16:45:27 +0000 Subject: [PATCH] Centralize Guardian action preparation for review (#45987) ## What changed Move duplicated action preparation into `codex_guardian_context::action_for_review`, shared by approval rendering and cached-evidence size checks. Preserve the existing behavior: omit top-level `tool_description` and `connector_description` metadata from MCP tool calls while keeping tool arguments intact. ## Testing Add unit tests verifying that MCP arguments retain description fields, including nested fields, and that other action types remain unchanged. GitOrigin-RevId: 8e890fb263556596f695177f917750377c04d43c --- .../core/src/guardian/approval_request.rs | 16 +------- .../guardian-v2/src/async_scorer/approval.rs | 9 +--- codex-rs/guardian-context/src/action.rs | 16 ++++++++ codex-rs/guardian-context/src/action_tests.rs | 41 +++++++++++++++++++ codex-rs/guardian-context/src/lib.rs | 1 + 5 files changed, 61 insertions(+), 22 deletions(-) create mode 100644 codex-rs/guardian-context/src/action_tests.rs diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index ffed746fc8..e1c67e52b0 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -537,20 +537,8 @@ pub(crate) fn guardian_request_turn_id<'a>( pub(crate) fn format_guardian_action_pretty( action: &GuardianApprovalRequest, ) -> serde_json::Result { - let mut value = guardian_action_for_review(action)?; + let mut value = + codex_guardian_context::action_for_review(guardian_approval_request_to_json(action)?); value.sort_all_objects(); serde_json::to_string_pretty(&value) } - -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/ext/guardian-v2/src/async_scorer/approval.rs b/codex-rs/ext/guardian-v2/src/async_scorer/approval.rs index 2e4c2d24e2..a557ce1845 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/approval.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/approval.rs @@ -140,14 +140,7 @@ async fn cached_evidence( return Err(GuardianReviewReason::MissingScore); }; // Elicitations and intercepted execs can expand beyond the original scored action. - let mut action = input.action.clone(); - if action.get("tool").and_then(serde_json::Value::as_str) == Some("mcp_tool_call") - && let Some(fields) = action.as_object_mut() - { - // Match the action renderer: host descriptions are optional, arguments are not. - fields.remove("tool_description"); - fields.remove("connector_description"); - } + let action = codex_guardian_context::action_for_review(input.action.clone()); let max_action_bytes = TruncationPolicy::Tokens(config.max_action_tokens).byte_budget(); let action_fits = serde_json::to_string_pretty(&action) .is_ok_and(|action| action.len().saturating_add(1) <= max_action_bytes); diff --git a/codex-rs/guardian-context/src/action.rs b/codex-rs/guardian-context/src/action.rs index 8b2022cc28..3883de031e 100644 --- a/codex-rs/guardian-context/src/action.rs +++ b/codex-rs/guardian-context/src/action.rs @@ -8,6 +8,18 @@ use crate::SectionError; use crate::SectionInput; use crate::SectionScope; +/// Projects an approval action onto the exact JSON reviewed and measured by Guardian. +/// Host descriptions are optional; nested tool arguments must remain complete. +pub fn action_for_review(mut action: serde_json::Value) -> serde_json::Value { + if action.get("tool").and_then(serde_json::Value::as_str) == Some("mcp_tool_call") + && let Some(fields) = action.as_object_mut() + { + fields.remove("tool_description"); + fields.remove("connector_description"); + } + action +} + /// Host-prepared action evidence. JSON stays complete through request admission. #[derive(Clone, PartialEq)] pub struct PlannedAction { @@ -111,6 +123,10 @@ impl PlannedAction { pub(crate) struct PlannedActionSection; +#[cfg(test)] +#[path = "action_tests.rs"] +mod tests; + impl SectionContributor for PlannedActionSection { fn scope(&self) -> SectionScope { SectionScope::Shared diff --git a/codex-rs/guardian-context/src/action_tests.rs b/codex-rs/guardian-context/src/action_tests.rs new file mode 100644 index 0000000000..c1efc4331a --- /dev/null +++ b/codex-rs/guardian-context/src/action_tests.rs @@ -0,0 +1,41 @@ +//! Approval projections omit optional host metadata without changing the actual action. + +use super::action_for_review; +use pretty_assertions::assert_eq; +use serde_json::json; + +#[test] +fn mcp_projection_preserves_arguments_with_description_fields() { + let arguments = json!({ + "description": "required argument", + "tool_description": "also an argument", + "nested": {"connector_description": "required nested argument"}, + }); + let action = json!({ + "tool": "mcp_tool_call", + "server": "example", + "tool_name": "publish", + "tool_description": "optional host metadata", + "connector_description": "optional connector metadata", + "arguments": arguments, + }); + assert_eq!( + action_for_review(action), + json!({ + "tool": "mcp_tool_call", + "server": "example", + "tool_name": "publish", + "arguments": arguments, + }), + ); +} + +#[test] +fn other_actions_preserve_description_fields() { + let action = json!({ + "tool": "custom_action", + "tool_description": "required argument", + "connector_description": "also required", + }); + assert_eq!(action_for_review(action.clone()), action); +} diff --git a/codex-rs/guardian-context/src/lib.rs b/codex-rs/guardian-context/src/lib.rs index f12d9b1baa..36bce55ae1 100644 --- a/codex-rs/guardian-context/src/lib.rs +++ b/codex-rs/guardian-context/src/lib.rs @@ -23,6 +23,7 @@ use transcript::ConversationTranscriptSection; pub use action::ActionPresentation; pub use action::PlannedAction; pub use action::PlannedActionKind; +pub use action::action_for_review; pub use authorization::GuardianRootMessage; pub use section::ContextSection;