diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 8560db415b..ea12c23ba4 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -91,7 +91,6 @@ use codex_protocol::mcp::CallToolResult; use codex_protocol::models::ApprovalSourceMetadata; use codex_protocol::models::BaseInstructions; use codex_protocol::models::PermissionProfile; -use codex_protocol::models::ReviewDecisionMetadata; use codex_protocol::models::UserMessageType; use codex_protocol::models::format_allow_prefixes; use codex_protocol::openai_models::ModelInfo; @@ -1082,28 +1081,6 @@ fn stamp_message_metadata_on_response_item( } } -fn review_decision_to_metadata(decision: &ReviewDecision) -> ReviewDecisionMetadata { - match decision { - ReviewDecision::Approved => ReviewDecisionMetadata::Approved, - ReviewDecision::ApprovedExecpolicyAmendment { .. } => { - ReviewDecisionMetadata::ApprovedWithAmendment - } - ReviewDecision::ApprovedForSession => ReviewDecisionMetadata::ApprovedForSession, - ReviewDecision::NetworkPolicyAmendment { - network_policy_amendment, - } => match network_policy_amendment.action { - codex_protocol::protocol::NetworkPolicyRuleAction::Allow => { - ReviewDecisionMetadata::ApprovedWithNetworkPolicyAllow - } - codex_protocol::protocol::NetworkPolicyRuleAction::Deny => { - ReviewDecisionMetadata::DeniedWithNetworkPolicyDeny - } - }, - ReviewDecision::Denied => ReviewDecisionMetadata::Denied, - ReviewDecision::Abort => ReviewDecisionMetadata::Abort, - } -} - fn approval_source_for_turn(turn_context: &TurnContext) -> ApprovalSourceMetadata { if routes_approval_to_guardian(turn_context) { ApprovalSourceMetadata::Guardian @@ -3438,23 +3415,9 @@ 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( - call_id.to_string(), - ApprovalOutcomeMetadata::reviewed_with_source(decision, approval_source), + pending_approval.call_id, + ApprovalOutcomeMetadata::reviewed(decision, pending_approval.approval_source), ) .await; } @@ -3463,7 +3426,6 @@ impl Session { self.record_call_approval_outcome(call_id.to_string(), ApprovalOutcomeMetadata::policy()) .await; } - pub(crate) async fn record_call_approval_outcome( &self, call_id: String, diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index be8ca88da2..de11b6af57 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -4392,47 +4392,72 @@ async fn task_finish_emits_prompt_queued_metadata_for_injected_user_input_when_f #[test] fn review_decision_metadata_mapping_is_stable() { assert_eq!( - ApprovalOutcomeMetadata::reviewed(&ReviewDecision::Approved).review_decision, + ApprovalOutcomeMetadata::reviewed( + &ReviewDecision::Approved, + codex_protocol::models::ApprovalSourceMetadata::User, + ) + .review_decision, Some(codex_protocol::models::ReviewDecisionMetadata::Approved) ); assert_eq!( - ApprovalOutcomeMetadata::reviewed(&ReviewDecision::Denied).review_decision, + ApprovalOutcomeMetadata::reviewed( + &ReviewDecision::Denied, + codex_protocol::models::ApprovalSourceMetadata::User, + ) + .review_decision, Some(codex_protocol::models::ReviewDecisionMetadata::Denied) ); assert_eq!( - ApprovalOutcomeMetadata::reviewed(&ReviewDecision::Abort).review_decision, + ApprovalOutcomeMetadata::reviewed( + &ReviewDecision::Abort, + codex_protocol::models::ApprovalSourceMetadata::User, + ) + .review_decision, Some(codex_protocol::models::ReviewDecisionMetadata::Abort) ); assert_eq!( - ApprovalOutcomeMetadata::reviewed(&ReviewDecision::ApprovedForSession).review_decision, + ApprovalOutcomeMetadata::reviewed( + &ReviewDecision::ApprovedForSession, + codex_protocol::models::ApprovalSourceMetadata::User, + ) + .review_decision, Some(codex_protocol::models::ReviewDecisionMetadata::ApprovedForSession) ); assert_eq!( - ApprovalOutcomeMetadata::reviewed(&ReviewDecision::ApprovedExecpolicyAmendment { - proposed_execpolicy_amendment: codex_protocol::approvals::ExecPolicyAmendment::new( - vec!["echo".to_string(), "hi".to_string()], - ), - }) + ApprovalOutcomeMetadata::reviewed( + &ReviewDecision::ApprovedExecpolicyAmendment { + proposed_execpolicy_amendment: codex_protocol::approvals::ExecPolicyAmendment::new( + vec!["echo".to_string(), "hi".to_string()], + ), + }, + codex_protocol::models::ApprovalSourceMetadata::User, + ) .review_decision, Some(codex_protocol::models::ReviewDecisionMetadata::ApprovedWithAmendment) ); assert_eq!( - ApprovalOutcomeMetadata::reviewed(&ReviewDecision::NetworkPolicyAmendment { - network_policy_amendment: codex_protocol::approvals::NetworkPolicyAmendment { - host: "example.com".to_string(), - action: codex_protocol::protocol::NetworkPolicyRuleAction::Allow, + ApprovalOutcomeMetadata::reviewed( + &ReviewDecision::NetworkPolicyAmendment { + network_policy_amendment: codex_protocol::approvals::NetworkPolicyAmendment { + host: "example.com".to_string(), + action: codex_protocol::protocol::NetworkPolicyRuleAction::Allow, + }, }, - }) + codex_protocol::models::ApprovalSourceMetadata::User, + ) .review_decision, Some(codex_protocol::models::ReviewDecisionMetadata::ApprovedWithNetworkPolicyAllow) ); assert_eq!( - ApprovalOutcomeMetadata::reviewed(&ReviewDecision::NetworkPolicyAmendment { - network_policy_amendment: codex_protocol::approvals::NetworkPolicyAmendment { - host: "example.com".to_string(), - action: codex_protocol::protocol::NetworkPolicyRuleAction::Deny, + ApprovalOutcomeMetadata::reviewed( + &ReviewDecision::NetworkPolicyAmendment { + network_policy_amendment: codex_protocol::approvals::NetworkPolicyAmendment { + host: "example.com".to_string(), + action: codex_protocol::protocol::NetworkPolicyRuleAction::Deny, + }, }, - }) + codex_protocol::models::ApprovalSourceMetadata::User, + ) .review_decision, Some(codex_protocol::models::ReviewDecisionMetadata::DeniedWithNetworkPolicyDeny) ); @@ -4553,7 +4578,10 @@ async fn tool_call_metadata_stamps_escalated_review_decision_when_feature_enable sess.record_call_approval_outcome( "call-1".to_string(), - ApprovalOutcomeMetadata::reviewed(&ReviewDecision::Denied), + ApprovalOutcomeMetadata::reviewed( + &ReviewDecision::Denied, + codex_protocol::models::ApprovalSourceMetadata::User, + ), ) .await; sess.record_response_item_and_emit_turn_item(tc.as_ref(), function_call_item("call-1")) @@ -4608,10 +4636,12 @@ async fn tool_call_metadata_stamps_guardian_direct_review_when_feature_enabled() .await; while rx.try_recv().is_ok() {} - sess.record_direct_approval_outcome( - "call-guardian-runtime-1", - &ReviewDecision::Denied, - codex_protocol::models::ApprovalSourceMetadata::Guardian, + sess.record_call_approval_outcome( + "call-guardian-runtime-1".to_string(), + ApprovalOutcomeMetadata::reviewed( + &ReviewDecision::Denied, + codex_protocol::models::ApprovalSourceMetadata::Guardian, + ), ) .await; sess.record_response_item_and_emit_turn_item( @@ -4653,7 +4683,11 @@ async fn tool_call_metadata_stamps_guardian_direct_review_when_feature_enabled() async fn tool_call_metadata_stamps_policy_source_without_review_decision_when_feature_enabled() { let (sess, tc, rx, expected_sandbox_policy) = setup_tool_call_metadata_runtime_test().await; - sess.record_policy_outcome("call-policy-runtime-1").await; + sess.record_call_approval_outcome( + "call-policy-runtime-1".to_string(), + ApprovalOutcomeMetadata::policy(), + ) + .await; sess.record_response_item_and_emit_turn_item( tc.as_ref(), function_call_item("call-policy-runtime-1"), @@ -4763,10 +4797,12 @@ async fn tool_call_metadata_can_be_restamped_after_approval_outcome() { }, ) .await; - sess.record_direct_approval_outcome( - "call-restamp-1", - &ReviewDecision::Denied, - codex_protocol::models::ApprovalSourceMetadata::User, + sess.record_call_approval_outcome( + "call-restamp-1".to_string(), + ApprovalOutcomeMetadata::reviewed( + &ReviewDecision::Denied, + codex_protocol::models::ApprovalSourceMetadata::User, + ), ) .await; @@ -4818,7 +4854,7 @@ async fn tool_call_metadata_snapshot_stamps_guardian_approval_source() { let snapshot = ToolApprovalMetadataSnapshot { approval_outcomes_by_call_id: HashMap::from([( "call-guardian-1".to_string(), - crate::state::ApprovalOutcomeMetadata { + ApprovalOutcomeMetadata { review_decision: Some(codex_protocol::models::ReviewDecisionMetadata::Denied), approval_source: codex_protocol::models::ApprovalSourceMetadata::Guardian, }, @@ -4859,7 +4895,7 @@ async fn tool_call_metadata_snapshot_stamps_policy_approval_source_without_revie let snapshot = ToolApprovalMetadataSnapshot { approval_outcomes_by_call_id: HashMap::from([( "call-policy-1".to_string(), - crate::state::ApprovalOutcomeMetadata { + ApprovalOutcomeMetadata { review_decision: None, approval_source: codex_protocol::models::ApprovalSourceMetadata::Policy, }, diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 8b235d71a5..ac3bd1c5ba 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -32,6 +32,7 @@ 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; @@ -533,8 +534,11 @@ async fn maybe_request_mcp_tool_approval( monitor_reason.clone(), ) .await; - sess.record_direct_approval_outcome(call_id, &decision, ApprovalSourceMetadata::Guardian) - .await; + sess.record_call_approval_outcome( + call_id.to_string(), + ApprovalOutcomeMetadata::reviewed(&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/state/turn.rs b/codex-rs/core/src/state/turn.rs index 55f4ee1f18..bdb79c4ba3 100644 --- a/codex-rs/core/src/state/turn.rs +++ b/codex-rs/core/src/state/turn.rs @@ -79,11 +79,7 @@ pub(crate) struct ApprovalOutcomeMetadata { } impl ApprovalOutcomeMetadata { - pub(crate) fn reviewed(decision: &ReviewDecision) -> Self { - Self::reviewed_with_source(decision, ApprovalSourceMetadata::User) - } - - pub(crate) fn reviewed_with_source( + pub(crate) fn reviewed( decision: &ReviewDecision, approval_source: ApprovalSourceMetadata, ) -> Self { diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index 5a1a8d2888..adbb0ab8a7 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -11,6 +11,7 @@ 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; @@ -143,10 +144,12 @@ 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_direct_approval_outcome( - &call_id, - &decision, - ApprovalSourceMetadata::Guardian, + .record_call_approval_outcome( + call_id.clone(), + ApprovalOutcomeMetadata::reviewed( + &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 4557cd88e9..5282cde5ee 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -18,6 +18,7 @@ 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; @@ -169,10 +170,12 @@ impl Approvable for ShellRuntime { ) .await; session - .record_direct_approval_outcome( - &call_id, - &decision, - ApprovalSourceMetadata::Guardian, + .record_call_approval_outcome( + call_id.clone(), + ApprovalOutcomeMetadata::reviewed( + &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 e18be56d75..db1de9a596 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -13,6 +13,7 @@ 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; @@ -440,10 +441,12 @@ impl CoreShellActionProvider { ) .await; session - .record_direct_approval_outcome( - &call_id, - &decision, - ApprovalSourceMetadata::Guardian, + .record_call_approval_outcome( + call_id.clone(), + ApprovalOutcomeMetadata::reviewed( + &decision, + ApprovalSourceMetadata::Guardian, + ), ) .await; return decision; @@ -529,14 +532,24 @@ impl CoreShellActionProvider { ) -> anyhow::Result { let action = match decision { Decision::Forbidden => { - self.session.record_policy_outcome(&self.call_id).await; + self.session + .record_call_approval_outcome( + self.call_id.clone(), + ApprovalOutcomeMetadata::policy(), + ) + .await; EscalationDecision::deny(Some("Execution forbidden by policy".to_string())) } Decision::Prompt => { if execve_prompt_is_rejected_by_policy(self.approval_policy, &decision_source) .is_some() { - self.session.record_policy_outcome(&self.call_id).await; + self.session + .record_call_approval_outcome( + self.call_id.clone(), + ApprovalOutcomeMetadata::policy(), + ) + .await; EscalationDecision::deny(Some("Execution forbidden by policy".to_string())) } else { match self diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index b444938cd4..2ea5f52123 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -15,6 +15,7 @@ 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; @@ -138,10 +139,12 @@ impl Approvable for UnifiedExecRuntime<'_> { ) .await; session - .record_direct_approval_outcome( - &call_id, - &decision, - ApprovalSourceMetadata::Guardian, + .record_call_approval_outcome( + call_id.clone(), + ApprovalOutcomeMetadata::reviewed( + &decision, + ApprovalSourceMetadata::Guardian, + ), ) .await; return decision;