From 2a5d70bece1aea998e52855a44425a22805f34f3 Mon Sep 17 00:00:00 2001 From: Roy Han Date: Wed, 18 Mar 2026 16:49:25 -0700 Subject: [PATCH] guardian patch and test update --- codex-rs/core/src/codex.rs | 18 +++++- codex-rs/core/src/codex_tests.rs | 58 ++++++++++++------- codex-rs/core/src/mcp_tool_call.rs | 8 +-- .../core/src/tools/runtimes/apply_patch.rs | 11 ++-- codex-rs/core/src/tools/runtimes/shell.rs | 11 ++-- .../tools/runtimes/shell/unix_escalation.rs | 11 ++-- .../core/src/tools/runtimes/unified_exec.rs | 11 ++-- 7 files changed, 71 insertions(+), 57 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 53670eba73..9ef02e7ea6 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -3421,9 +3421,23 @@ impl Session { }); drop(ts); drop(active); + self.record_direct_approval_outcome( + &pending_approval.call_id, + decision, + pending_approval.approval_source, + ) + .await; + } + + pub async fn record_direct_approval_outcome( + &self, + call_id: &str, + decision: &ReviewDecision, + approval_source: ApprovalSourceMetadata, + ) { self.record_call_approval_outcome( - pending_approval.call_id, - ApprovalOutcomeMetadata::reviewed(decision, pending_approval.approval_source), + call_id.to_string(), + ApprovalOutcomeMetadata::reviewed(decision, approval_source), ) .await; } diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index c9f4ae18cb..376f94c143 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -4609,27 +4609,45 @@ async fn tool_call_metadata_stamps_non_escalated_false_when_feature_enabled() { async fn tool_call_metadata_stamps_guardian_direct_review_when_feature_enabled() { let (sess, tc, rx, expected_sandbox_policy) = setup_tool_call_metadata_runtime_test().await; - sess.record_call_approval_outcome( - "call-guardian-runtime-1".to_string(), - ApprovalOutcomeMetadata::reviewed( - &ReviewDecision::Denied, - codex_protocol::models::ApprovalSourceMetadata::Guardian, - ), + sess.record_direct_approval_outcome( + "call-guardian-runtime-1", + &ReviewDecision::Denied, + codex_protocol::models::ApprovalSourceMetadata::Guardian, ) .await; sess.record_response_item_and_emit_turn_item( tc.as_ref(), - function_call_item("call-guardian-runtime-1"), - ) - .await; - assert_next_emitted_function_call_metadata( - &rx, - expected_sandbox_policy, - true, - Some(codex_protocol::models::ReviewDecisionMetadata::Denied), - Some(codex_protocol::models::ApprovalSourceMetadata::Guardian), + ResponseItem::FunctionCall { + id: None, + name: "shell".to_string(), + namespace: None, + arguments: "{}".to_string(), + call_id: "call-guardian-runtime-1".to_string(), + metadata: None, + }, ) .await; + + let event = tokio::time::timeout(std::time::Duration::from_secs(2), rx.recv()) + .await + .expect("expected raw response item event") + .expect("channel open"); + assert!(matches!( + event.msg, + EventMsg::RawResponseItem(ref ev) + if matches!( + &ev.item, + ResponseItem::FunctionCall { + metadata: Some(metadata), + .. + } if metadata.is_tool_call_escalated == Some(true) + && metadata.review_decision + == Some(codex_protocol::models::ReviewDecisionMetadata::Denied) + && metadata.approval_source + == Some(codex_protocol::models::ApprovalSourceMetadata::Guardian) + && metadata.sandbox_policy == Some(expected_sandbox_policy) + ) + )); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -4746,12 +4764,10 @@ async fn tool_call_metadata_can_be_restamped_after_approval_outcome() { }, ) .await; - sess.record_call_approval_outcome( - "call-restamp-1".to_string(), - ApprovalOutcomeMetadata::reviewed( - &ReviewDecision::Denied, - codex_protocol::models::ApprovalSourceMetadata::User, - ), + sess.record_direct_approval_outcome( + "call-restamp-1", + &ReviewDecision::Denied, + codex_protocol::models::ApprovalSourceMetadata::User, ) .await; diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index ac3bd1c5ba..8b235d71a5 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -32,7 +32,6 @@ use crate::protocol::EventMsg; use crate::protocol::McpInvocation; use crate::protocol::McpToolCallBeginEvent; use crate::protocol::McpToolCallEndEvent; -use crate::state::ApprovalOutcomeMetadata; use crate::state_db; use codex_protocol::mcp::CallToolResult; use codex_protocol::models::ApprovalSourceMetadata; @@ -534,11 +533,8 @@ async fn maybe_request_mcp_tool_approval( monitor_reason.clone(), ) .await; - sess.record_call_approval_outcome( - call_id.to_string(), - ApprovalOutcomeMetadata::reviewed(&decision, ApprovalSourceMetadata::Guardian), - ) - .await; + sess.record_direct_approval_outcome(call_id, &decision, ApprovalSourceMetadata::Guardian) + .await; let decision = mcp_tool_approval_decision_from_guardian(decision); apply_mcp_tool_approval_decision( sess, diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index adbb0ab8a7..5a1a8d2888 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -11,7 +11,6 @@ use crate::guardian::routes_approval_to_guardian; use crate::sandboxing::CommandSpec; use crate::sandboxing::SandboxPermissions; use crate::sandboxing::execute_env; -use crate::state::ApprovalOutcomeMetadata; use crate::tools::sandboxing::Approvable; use crate::tools::sandboxing::ApprovalCtx; use crate::tools::sandboxing::ExecApprovalRequirement; @@ -144,12 +143,10 @@ impl Approvable for ApplyPatchRuntime { let action = ApplyPatchRuntime::build_guardian_review_request(req, ctx.call_id); let decision = review_approval_request(session, turn, action, retry_reason).await; session - .record_call_approval_outcome( - call_id.clone(), - ApprovalOutcomeMetadata::reviewed( - &decision, - ApprovalSourceMetadata::Guardian, - ), + .record_direct_approval_outcome( + &call_id, + &decision, + ApprovalSourceMetadata::Guardian, ) .await; return decision; diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 5282cde5ee..4557cd88e9 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -18,7 +18,6 @@ use crate::powershell::prefix_powershell_script_with_utf8; use crate::sandboxing::SandboxPermissions; use crate::sandboxing::execute_env; use crate::shell::ShellType; -use crate::state::ApprovalOutcomeMetadata; use crate::tools::network_approval::NetworkApprovalMode; use crate::tools::network_approval::NetworkApprovalSpec; use crate::tools::runtimes::build_command_spec; @@ -170,12 +169,10 @@ impl Approvable for ShellRuntime { ) .await; session - .record_call_approval_outcome( - call_id.clone(), - ApprovalOutcomeMetadata::reviewed( - &decision, - ApprovalSourceMetadata::Guardian, - ), + .record_direct_approval_outcome( + &call_id, + &decision, + ApprovalSourceMetadata::Guardian, ) .await; return decision; diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index b37966eb86..e18be56d75 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -13,7 +13,6 @@ use crate::sandboxing::ExecRequest; use crate::sandboxing::SandboxPermissions; use crate::shell::ShellType; use crate::skills::SkillMetadata; -use crate::state::ApprovalOutcomeMetadata; use crate::tools::runtimes::ExecveSessionApproval; use crate::tools::runtimes::build_command_spec; use crate::tools::sandboxing::SandboxAttempt; @@ -441,12 +440,10 @@ impl CoreShellActionProvider { ) .await; session - .record_call_approval_outcome( - call_id.clone(), - ApprovalOutcomeMetadata::reviewed( - &decision, - ApprovalSourceMetadata::Guardian, - ), + .record_direct_approval_outcome( + &call_id, + &decision, + ApprovalSourceMetadata::Guardian, ) .await; return decision; diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 2ea5f52123..b444938cd4 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -15,7 +15,6 @@ use crate::guardian::routes_approval_to_guardian; use crate::powershell::prefix_powershell_script_with_utf8; use crate::sandboxing::SandboxPermissions; use crate::shell::ShellType; -use crate::state::ApprovalOutcomeMetadata; use crate::tools::network_approval::NetworkApprovalMode; use crate::tools::network_approval::NetworkApprovalSpec; use crate::tools::runtimes::build_command_spec; @@ -139,12 +138,10 @@ impl Approvable for UnifiedExecRuntime<'_> { ) .await; session - .record_call_approval_outcome( - call_id.clone(), - ApprovalOutcomeMetadata::reviewed( - &decision, - ApprovalSourceMetadata::Guardian, - ), + .record_direct_approval_outcome( + &call_id, + &decision, + ApprovalSourceMetadata::Guardian, ) .await; return decision;