From f3e9faef93a6cc2b816fc9f29d4e11b11b0d7275 Mon Sep 17 00:00:00 2001 From: Roy Han Date: Mon, 23 Mar 2026 12:49:19 -0700 Subject: [PATCH] policy review --- codex-rs/core/src/codex_tests.rs | 20 +++++++ codex-rs/core/src/state/turn.rs | 7 +++ .../tools/runtimes/shell/unix_escalation.rs | 49 +++++++++++++++++ .../runtimes/shell/unix_escalation_tests.rs | 55 +++++++++++++++++++ 4 files changed, 131 insertions(+) diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index cb7e8e6b1a..828d4ea077 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -4597,6 +4597,26 @@ async fn tool_call_metadata_stamps_escalated_review_decision_when_feature_enable .await; } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn tool_call_metadata_stamps_policy_source_without_review_decision_when_feature_enabled() { + let (sess, tc, rx) = setup_tool_call_metadata_runtime_test().await; + + sess.record_call_approval_outcome( + "call-policy-1".to_string(), + ApprovalOutcomeMetadata::policy(), + ) + .await; + sess.record_response_item_and_emit_turn_item(tc.as_ref(), function_call_item("call-policy-1")) + .await; + assert_next_emitted_function_call_metadata( + &rx, + true, + None, + Some(codex_protocol::models::ApprovalSourceMetadata::Policy), + ) + .await; +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn tool_call_metadata_stamps_non_escalated_false_when_feature_enabled() { let (sess, tc, rx) = setup_tool_call_metadata_runtime_test().await; diff --git a/codex-rs/core/src/state/turn.rs b/codex-rs/core/src/state/turn.rs index 9448536ded..afc96487aa 100644 --- a/codex-rs/core/src/state/turn.rs +++ b/codex-rs/core/src/state/turn.rs @@ -72,6 +72,13 @@ pub(crate) struct ApprovalOutcomeMetadata { } impl ApprovalOutcomeMetadata { + pub(crate) fn policy() -> Self { + Self { + review_decision: None, + approval_source: ApprovalSourceMetadata::Policy, + } + } + pub(crate) fn reviewed( decision: &ReviewDecision, approval_source: ApprovalSourceMetadata, 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 948018dae6..f3c7aa2fc2 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; @@ -26,6 +27,7 @@ use codex_execpolicy::Policy; use codex_execpolicy::RuleMatch; use codex_features::Feature; use codex_protocol::config_types::WindowsSandboxLevel; +use codex_protocol::models::ApprovalSourceMetadata; use codex_protocol::models::MacOsSeatbeltProfileExtensions; use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemSandboxPolicy; @@ -359,6 +361,26 @@ fn execve_prompt_is_rejected_by_policy( } } +fn approval_source_for_policy_resolved_decision( + approval_policy: AskForApproval, + decision: Decision, + decision_source: &DecisionSource, +) -> Option { + match decision { + Decision::Allow | Decision::Forbidden + if matches!(decision_source, DecisionSource::PrefixRule) => + { + Some(ApprovalSourceMetadata::Policy) + } + Decision::Prompt + if execve_prompt_is_rejected_by_policy(approval_policy, decision_source).is_some() => + { + Some(ApprovalSourceMetadata::Policy) + } + _ => None, + } +} + impl CoreShellActionProvider { fn decision_driven_by_policy(matched_rules: &[RuleMatch], decision: Decision) -> bool { matched_rules.iter().any(|rule_match| { @@ -522,12 +544,16 @@ impl CoreShellActionProvider { ) -> anyhow::Result { let action = match decision { Decision::Forbidden => { + self.record_policy_approval_outcome_if_needed(decision, &decision_source) + .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.record_policy_approval_outcome_if_needed(decision, &decision_source) + .await; EscalationDecision::deny(Some("Execution forbidden by policy".to_string())) } else { match self @@ -600,6 +626,8 @@ impl CoreShellActionProvider { } } Decision::Allow => { + self.record_policy_approval_outcome_if_needed(decision, &decision_source) + .await; if needs_escalation { EscalationDecision::escalate(escalation_execution) } else { @@ -612,6 +640,27 @@ impl CoreShellActionProvider { ); Ok(action) } + + async fn record_policy_approval_outcome_if_needed( + &self, + decision: Decision, + decision_source: &DecisionSource, + ) { + if approval_source_for_policy_resolved_decision( + self.approval_policy, + decision, + decision_source, + ) + .is_some() + { + self.session + .record_call_approval_outcome( + self.call_id.clone(), + ApprovalOutcomeMetadata::policy(), + ) + .await; + } + } } // Shell-wrapper parsing is weaker than direct exec interception because it can diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs index 37c1b53a13..e0c25e11a0 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs @@ -29,6 +29,7 @@ use codex_execpolicy::PolicyParser; use codex_execpolicy::RuleMatch; #[cfg(target_os = "macos")] use codex_protocol::config_types::WindowsSandboxLevel; +use codex_protocol::models::ApprovalSourceMetadata; use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::MacOsPreferencesPermission; use codex_protocol::models::MacOsSeatbeltProfileExtensions; @@ -166,6 +167,60 @@ fn execve_prompt_rejection_keeps_unmatched_commands_on_sandbox_flag() { ); } +#[test] +fn policy_resolved_approval_source_marks_rule_driven_allow_as_policy() { + assert_eq!( + super::approval_source_for_policy_resolved_decision( + AskForApproval::OnRequest, + Decision::Allow, + &super::DecisionSource::PrefixRule, + ), + Some(ApprovalSourceMetadata::Policy), + ); +} + +#[test] +fn policy_resolved_approval_source_marks_rule_driven_forbidden_as_policy() { + assert_eq!( + super::approval_source_for_policy_resolved_decision( + AskForApproval::OnRequest, + Decision::Forbidden, + &super::DecisionSource::PrefixRule, + ), + Some(ApprovalSourceMetadata::Policy), + ); +} + +#[test] +fn policy_resolved_approval_source_marks_prompt_rejected_by_policy_as_policy() { + assert_eq!( + super::approval_source_for_policy_resolved_decision( + AskForApproval::Granular(GranularApprovalConfig { + sandbox_approval: false, + rules: true, + skill_approval: true, + request_permissions: true, + mcp_elicitations: true, + }), + Decision::Prompt, + &super::DecisionSource::UnmatchedCommandFallback, + ), + Some(ApprovalSourceMetadata::Policy), + ); +} + +#[test] +fn policy_resolved_approval_source_does_not_mark_fallback_allow_as_policy() { + assert_eq!( + super::approval_source_for_policy_resolved_decision( + AskForApproval::OnRequest, + Decision::Allow, + &super::DecisionSource::UnmatchedCommandFallback, + ), + None, + ); +} + #[test] fn approval_sandbox_permissions_only_downgrades_preapproved_additional_permissions() { assert_eq!(