openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-02 19:43:24 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor

Browse Source
This commit is contained in:
Roy Han
2026-03-19 18:24:21 -07:00
parent aab34f8037
commit 4a02049e49
8 changed files with 116 additions and 96 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/codex.rs
+2 -40
View File
@@ -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,
codex-rs/core/src/codex_tests.rs
+67 -31
View File
@@ -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,
},
codex-rs/core/src/mcp_tool_call.rs
+6 -2
View File
@@ -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,
codex-rs/core/src/state/turn.rs
+1 -5
View File
@@ -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 {
codex-rs/core/src/tools/runtimes/apply_patch.rs
+7 -4
View File
@@ -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<ApplyPatchRequest> 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;
codex-rs/core/src/tools/runtimes/shell.rs
+7 -4
View File
@@ -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<ShellRequest> 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;
codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs
+19 -6
View File
@@ -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<EscalationDecision> {
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
codex-rs/core/src/tools/runtimes/unified_exec.rs
+7 -4
View File
@@ -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<UnifiedExecRequest> 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;