openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-09-03 14:59:03 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(approvals): centralize tool review routing

Browse Source
This commit is contained in:
Dylan Hurd
2026-07-07 10:47:25 -07:00
parent 9732b406e7
commit 8a11ef3d83
6 changed files with 372 additions and 138 deletions
Download Patch File Download Diff File Expand all files Collapse all files

225
codex-rs/core/src/approval_coordinator.rs Normal file
View File

@@ -0,0 +1,225 @@
//! Central approval policy-stage execution and reviewer routing.
use std::sync::Arc;
use crate::guardian::guardian_rejection_message;
use crate::guardian::guardian_timeout_message;
use crate::guardian::new_guardian_review_id;
use crate::guardian::review_approval_request;
use crate::hook_runtime::run_permission_request_hooks;
use crate::session::session::Session;
use crate::session::turn_context::TurnContext;
use crate::tools::flat_tool_name;
use crate::tools::sandboxing::ApprovalCtx;
use crate::tools::sandboxing::ToolCtx;
use crate::tools::sandboxing::ToolError;
use crate::tools::sandboxing::ToolRuntime;
use codex_config::types::ApprovalsReviewer;
use codex_hooks::PermissionRequestDecision;
use codex_otel::ToolDecisionSource;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::NetworkPolicyRuleAction;
use codex_protocol::protocol::ReviewDecision;
pub(crate) type ApprovalAction = crate::guardian::GuardianApprovalRequest;
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub(crate) enum ApprovalReviewer {
Guardian,
User,
}
impl ApprovalReviewer {
pub(crate) fn for_turn(turn: &TurnContext) -> Self {
Self::for_reviewer(turn, turn.config.approvals_reviewer)
}
pub(crate) fn for_reviewer(turn: &TurnContext, reviewer: ApprovalsReviewer) -> Self {
if Self::routes_to_guardian(turn, reviewer) {
Self::Guardian
} else {
Self::User
}
}
fn routes_to_guardian(turn: &TurnContext, reviewer: ApprovalsReviewer) -> bool {
matches!(
turn.approval_policy.value(),
AskForApproval::OnRequest | AskForApproval::Granular(_)
) && reviewer == ApprovalsReviewer::AutoReview
}
}
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub(crate) enum ApprovalResolutionSource {
Hook,
Guardian,
User,
}
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) struct ApprovalResolution {
pub(crate) decision: ReviewDecision,
pub(crate) rejection: Option<String>,
pub(crate) source: ApprovalResolutionSource,
}
impl ApprovalResolution {
pub(crate) fn into_tool_result(self) -> Result<ReviewDecision, ToolError> {
if let Some(rejection) = self.rejection {
Err(ToolError::Rejected(rejection))
} else {
Ok(self.decision)
}
}
}
pub(crate) struct ApprovalCoordinator;
impl ApprovalCoordinator {
pub(crate) async fn resolve_tool<Rq, Out, T>(
tool: &mut T,
req: &Rq,
permission_request_run_id: &str,
ctx: ApprovalCtx<'_>,
tool_ctx: &ToolCtx,
reviewer: ApprovalReviewer,
otel: &codex_otel::SessionTelemetry,
) -> Result<ApprovalResolution, ToolError>
where
T: ToolRuntime<Rq, Out>,
{
if let Some(permission_request) = tool.permission_request_payload(req) {
match run_permission_request_hooks(
ctx.session,
ctx.turn,
permission_request_run_id,
permission_request,
)
.await
{
Some(PermissionRequestDecision::Allow) => {
let resolution = ApprovalResolution {
decision: ReviewDecision::Approved,
rejection: None,
source: ApprovalResolutionSource::Hook,
};
Self::record_resolution(otel, tool_ctx, &resolution);
return Ok(resolution);
}
Some(PermissionRequestDecision::Deny { message }) => {
let resolution = ApprovalResolution {
decision: ReviewDecision::Denied,
rejection: Some(message),
source: ApprovalResolutionSource::Hook,
};
Self::record_resolution(otel, tool_ctx, &resolution);
return Ok(resolution);
}
None => {}
}
}
let resolution = match reviewer {
ApprovalReviewer::Guardian => {
let review_id = new_guardian_review_id();
let action = match tool.approval_action(req, &ctx) {
Ok(action) => action,
Err(err) => {
tracing::error!(%err, "failed to build automatic approval action");
let resolution = ApprovalResolution {
decision: ReviewDecision::Abort,
rejection: Some(
"automatic approval review could not prepare the action"
.to_string(),
),
source: ApprovalResolutionSource::Guardian,
};
Self::record_resolution(otel, tool_ctx, &resolution);
return Ok(resolution);
}
};
let decision = review_approval_request(
ctx.session,
ctx.turn,
review_id.clone(),
action,
ctx.retry_reason.clone(),
)
.await;
Self::normalize_guardian(ctx.session, review_id, decision).await
}
ApprovalReviewer::User => ApprovalResolution {
decision: tool.start_approval_async(req, ctx.clone()).await,
rejection: None,
source: ApprovalResolutionSource::User,
},
};
let resolution = Self::normalize_user_rejection(resolution);
Self::record_resolution(otel, tool_ctx, &resolution);
Ok(resolution)
}
async fn normalize_guardian(
session: &Arc<Session>,
review_id: String,
decision: ReviewDecision,
) -> ApprovalResolution {
let rejection = match &decision {
ReviewDecision::Approved
| ReviewDecision::ApprovedForSession
| ReviewDecision::ApprovedExecpolicyAmendment { .. } => None,
ReviewDecision::NetworkPolicyAmendment {
network_policy_amendment,
} if network_policy_amendment.action == NetworkPolicyRuleAction::Allow => None,
ReviewDecision::TimedOut => Some(guardian_timeout_message()),
ReviewDecision::NetworkPolicyAmendment { .. }
| ReviewDecision::Denied
| ReviewDecision::Abort => {
Some(guardian_rejection_message(session.as_ref(), &review_id).await)
}
};
ApprovalResolution {
decision,
rejection,
source: ApprovalResolutionSource::Guardian,
}
}
fn normalize_user_rejection(mut resolution: ApprovalResolution) -> ApprovalResolution {
if resolution.source == ApprovalResolutionSource::User {
resolution.rejection = match &resolution.decision {
ReviewDecision::Approved
| ReviewDecision::ApprovedForSession
| ReviewDecision::ApprovedExecpolicyAmendment { .. } => None,
ReviewDecision::NetworkPolicyAmendment {
network_policy_amendment,
} if network_policy_amendment.action == NetworkPolicyRuleAction::Allow => None,
ReviewDecision::NetworkPolicyAmendment { .. }
| ReviewDecision::Denied
| ReviewDecision::Abort => Some("rejected by user".to_string()),
ReviewDecision::TimedOut => Some("approval request timed out".to_string()),
};
}
resolution
}
fn record_resolution(
otel: &codex_otel::SessionTelemetry,
tool_ctx: &ToolCtx,
resolution: &ApprovalResolution,
) {
let source = match resolution.source {
ApprovalResolutionSource::Hook => ToolDecisionSource::Config,
ApprovalResolutionSource::Guardian => ToolDecisionSource::AutomatedReviewer,
ApprovalResolutionSource::User => ToolDecisionSource::User,
};
let tool_name = flat_tool_name(&tool_ctx.tool_name);
otel.tool_decision(
tool_name.as_ref(),
&tool_ctx.call_id,
&resolution.decision,
source,
);
}
}

1
codex-rs/core/src/lib.rs
View File

@@ -6,6 +6,7 @@
#![deny(clippy::print_stdout, clippy::print_stderr)]
mod apply_patch;
mod approval_coordinator;
mod apps;
mod client;
mod client_common;

157
codex-rs/core/src/tools/orchestrator.rs
View File

@@ -6,12 +6,8 @@ simple sequence for any ToolRuntime: approval → select sandbox → attempt →
retry with an escalated sandbox strategy on denial (no re‑approval thanks to
caching).
*/
use crate::guardian::guardian_rejection_message;
use crate::guardian::guardian_timeout_message;
use crate::guardian::new_guardian_review_id;
use crate::guardian::review_approval_request;
use crate::guardian::routes_approval_to_guardian;
use crate::hook_runtime::run_permission_request_hooks;
use crate::approval_coordinator::ApprovalCoordinator;
use crate::approval_coordinator::ApprovalReviewer;
use crate::network_policy_decision::network_approval_context_from_payload;
use crate::tools::flat_tool_name;
use crate::tools::network_approval::ActiveNetworkApproval;
@@ -30,13 +26,11 @@ use crate::tools::sandboxing::ToolRuntime;
use crate::tools::sandboxing::default_exec_approval_requirement;
use crate::tools::sandboxing::sandbox_override_for_first_attempt;
use crate::tools::sandboxing::unsandboxed_execution_allowed;
use codex_hooks::PermissionRequestDecision;
use codex_otel::ToolDecisionSource;
use codex_protocol::error::CodexErr;
use codex_protocol::error::SandboxErr;
use codex_protocol::exec_output::ExecToolCallOutput;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::NetworkPolicyRuleAction;
use codex_protocol::protocol::ReviewDecision;
use codex_sandboxing::SandboxManager;
use codex_sandboxing::SandboxType;
@@ -155,8 +149,6 @@ impl ToolOrchestrator {
let otel_tn = flat_tool_name(&tool_ctx.tool_name).into_owned();
let otel_ci = &tool_ctx.call_id;
let strict_auto_review = tool_ctx.session.strict_auto_review_enabled_for_turn().await;
let use_guardian = routes_approval_to_guardian(turn_ctx) || strict_auto_review;
// 1) Approval
let mut already_approved = false;
@@ -168,27 +160,23 @@ impl ToolOrchestrator {
match &requirement {
ExecApprovalRequirement::Skip { .. } => {
if strict_auto_review {
let guardian_review_id = Some(new_guardian_review_id());
let approval_ctx = ApprovalCtx {
session: &tool_ctx.session,
turn: &tool_ctx.turn,
call_id: &tool_ctx.call_id,
guardian_review_id: guardian_review_id.clone(),
retry_reason: None,
network_approval_context: None,
};
let decision = Self::request_approval(
Self::request_approval(
tool,
req,
tool_ctx.call_id.as_str(),
approval_ctx,
tool_ctx,
/*evaluate_permission_request_hooks*/ false,
ApprovalReviewer::Guardian,
&otel,
)
.await?;
Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision)
.await?;
already_approved = true;
} else {
otel.tool_decision(
@@ -203,28 +191,27 @@ impl ToolOrchestrator {
return Err(ToolError::Rejected(reason.clone()));
}
ExecApprovalRequirement::NeedsApproval { reason, .. } => {
let guardian_review_id = use_guardian.then(new_guardian_review_id);
let approval_ctx = ApprovalCtx {
session: &tool_ctx.session,
turn: &tool_ctx.turn,
call_id: &tool_ctx.call_id,
guardian_review_id: guardian_review_id.clone(),
retry_reason: reason.clone(),
network_approval_context: None,
};
let decision = Self::request_approval(
Self::request_approval(
tool,
req,
tool_ctx.call_id.as_str(),
approval_ctx,
tool_ctx,
/*evaluate_permission_request_hooks*/ !strict_auto_review,
if strict_auto_review {
ApprovalReviewer::Guardian
} else {
ApprovalReviewer::for_turn(turn_ctx)
},
&otel,
)
.await?;
Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision)
.await?;
already_approved = true;
}
}
@@ -400,30 +387,29 @@ impl ToolOrchestrator {
&& tool.should_bypass_approval(approval_policy, already_approved)
&& network_approval_context.is_none();
if !bypass_retry_approval {
let guardian_review_id = use_guardian.then(new_guardian_review_id);
let approval_ctx = ApprovalCtx {
session: &tool_ctx.session,
turn: &tool_ctx.turn,
call_id: &tool_ctx.call_id,
guardian_review_id: guardian_review_id.clone(),
retry_reason: Some(retry_reason),
network_approval_context: network_approval_context.clone(),
};
let permission_request_run_id = format!("{}:retry", tool_ctx.call_id);
let decision = Self::request_approval(
Self::request_approval(
tool,
req,
&permission_request_run_id,
approval_ctx,
tool_ctx,
/*evaluate_permission_request_hooks*/ !strict_auto_review,
if strict_auto_review {
ApprovalReviewer::Guardian
} else {
ApprovalReviewer::for_turn(turn_ctx)
},
&otel,
)
.await?;
Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision)
.await?;
}
let retry_sandbox_requested = !unsandboxed_allowed
@@ -527,110 +513,23 @@ impl ToolOrchestrator {
permission_request_run_id: &str,
approval_ctx: ApprovalCtx<'_>,
tool_ctx: &ToolCtx,
evaluate_permission_request_hooks: bool,
reviewer: ApprovalReviewer,
otel: &codex_otel::SessionTelemetry,
) -> Result<ReviewDecision, ToolError>
where
T: ToolRuntime<Rq, Out>,
{
if evaluate_permission_request_hooks
&& let Some(permission_request) = tool.permission_request_payload(req)
{
let tool_name = flat_tool_name(&tool_ctx.tool_name);
match run_permission_request_hooks(
approval_ctx.session,
approval_ctx.turn,
permission_request_run_id,
permission_request,
)
.await
{
Some(PermissionRequestDecision::Allow) => {
let decision = ReviewDecision::Approved;
otel.tool_decision(
tool_name.as_ref(),
&tool_ctx.call_id,
&decision,
ToolDecisionSource::Config,
);
return Ok(decision);
}
Some(PermissionRequestDecision::Deny { message }) => {
let decision = ReviewDecision::Denied;
otel.tool_decision(
tool_name.as_ref(),
&tool_ctx.call_id,
&decision,
ToolDecisionSource::Config,
);
return Err(ToolError::Rejected(message));
}
None => {}
}
}
let otel_source = if approval_ctx.guardian_review_id.is_some() {
ToolDecisionSource::AutomatedReviewer
} else {
ToolDecisionSource::User
};
let decision = if let Some(review_id) = approval_ctx.guardian_review_id.clone() {
match tool.approval_action(req, &approval_ctx) {
Ok(action) => {
review_approval_request(
approval_ctx.session,
approval_ctx.turn,
review_id,
action,
approval_ctx.retry_reason.clone(),
)
.await
}
Err(err) => {
tracing::error!(%err, "failed to build guardian approval action");
ReviewDecision::Abort
}
}
} else {
tool.start_approval_async(req, approval_ctx).await
};
let tool_name = flat_tool_name(&tool_ctx.tool_name);
otel.tool_decision(
tool_name.as_ref(),
&tool_ctx.call_id,
&decision,
otel_source,
);
Ok(decision)
}
async fn reject_if_not_approved(
tool_ctx: &ToolCtx,
guardian_review_id: Option<&str>,
decision: ReviewDecision,
) -> Result<(), ToolError> {
match decision {
ReviewDecision::Denied | ReviewDecision::Abort => {
let reason = if let Some(review_id) = guardian_review_id {
guardian_rejection_message(tool_ctx.session.as_ref(), review_id).await
} else {
"rejected by user".to_string()
};
Err(ToolError::Rejected(reason))
}
ReviewDecision::TimedOut => Err(ToolError::Rejected(guardian_timeout_message())),
ReviewDecision::Approved
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
| ReviewDecision::ApprovedForSession => Ok(()),
ReviewDecision::NetworkPolicyAmendment {
network_policy_amendment,
} => match network_policy_amendment.action {
NetworkPolicyRuleAction::Allow => Ok(()),
NetworkPolicyRuleAction::Deny => {
Err(ToolError::Rejected("rejected by user".to_string()))
}
},
}
ApprovalCoordinator::resolve_tool(
tool,
req,
permission_request_run_id,
approval_ctx,
tool_ctx,
reviewer,
otel,
)
.await?
.into_tool_result()
}
}

9
codex-rs/core/src/tools/sandboxing.rs
View File

@@ -122,18 +122,11 @@ pub(crate) struct ApprovalCtx<'a> {
pub session: &'a Arc<Session>,
pub turn: &'a Arc<TurnContext>,
pub call_id: &'a str,
/// Guardian review lifecycle ID for this approval, when guardian is reviewing it.
///
/// This is separate from `call_id`: `call_id` identifies the tool item under
/// review, while this ID identifies the review itself. Keeping both lets
/// denial handling, overrides, and app-server notifications refer to the
/// review without overloading the tool call ID as a review ID.
pub guardian_review_id: Option<String>,
pub retry_reason: Option<String>,
pub network_approval_context: Option<NetworkApprovalContext>,
}
pub(crate) type ApprovalAction = crate::guardian::GuardianApprovalRequest;
pub(crate) type ApprovalAction = crate::approval_coordinator::ApprovalAction;
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) struct PermissionRequestPayload {

116
codex-rs/core/tests/suite/hooks.rs
View File

@@ -12,6 +12,7 @@ use codex_plugin::PluginHookSource;
use codex_plugin::PluginId;
use codex_protocol::items::parse_hook_prompt_fragment;
use codex_protocol::models::ContentItem;
use codex_protocol::models::NetworkPermissions;
use codex_protocol::models::PermissionProfile;
use codex_protocol::models::ResponseItem;
use codex_protocol::permissions::NetworkSandboxPolicy;
@@ -20,6 +21,9 @@ use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::Op;
use codex_protocol::protocol::RolloutItem;
use codex_protocol::protocol::RolloutLine;
use codex_protocol::request_permissions::PermissionGrantScope;
use codex_protocol::request_permissions::RequestPermissionProfile;
use codex_protocol::request_permissions::RequestPermissionsResponse;
use codex_protocol::user_input::UserInput;
use codex_utils_absolute_path::AbsolutePathBuf;
use core_test_support::hooks::trust_discovered_hooks;
@@ -2012,6 +2016,118 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() ->
Ok(())
}
#[tokio::test]
async fn permission_request_hook_allow_bypasses_strict_auto_review() -> Result<()> {
skip_if_no_network!(Ok(()));
let server = start_mock_server().await;
let permission_call_id = "strict-hook-permissions";
let command_call_id = "strict-hook-shell-command";
let marker = std::env::temp_dir().join("strict-hook-shell-command-marker");
let command = format!("rm -f {}", marker.display());
let requested_permissions = RequestPermissionProfile {
network: Some(NetworkPermissions {
enabled: Some(true),
}),
..Default::default()
};
let request_permissions_args = serde_json::json!({
"reason": "Enable strict auto review",
"permissions": requested_permissions,
});
let command_args = serde_json::json!({ "command": command });
let responses = mount_sse_sequence(
&server,
vec![
sse(vec![
ev_response_created("resp-strict-hook-1"),
ev_function_call(
permission_call_id,
"request_permissions",
&serde_json::to_string(&request_permissions_args)?,
),
ev_completed("resp-strict-hook-1"),
]),
sse(vec![
ev_response_created("resp-strict-hook-2"),
ev_function_call(
command_call_id,
"shell_command",
&serde_json::to_string(&command_args)?,
),
ev_completed("resp-strict-hook-2"),
]),
sse(vec![
ev_response_created("resp-strict-hook-3"),
ev_assistant_message("msg-strict-hook", "permission hook allowed it"),
ev_completed("resp-strict-hook-3"),
]),
],
)
.await;
let mut builder = test_codex()
.with_pre_build_hook(|home| {
install_allow_permission_request_hook(home)
.expect("failed to write permission request hook test fixture");
})
.with_config(|config| {
trust_discovered_hooks(config);
config
.features
.enable(Feature::RequestPermissionsTool)
.expect("test config should allow feature update");
});
let test = builder.build(&server).await?;
fs::write(&marker, "seed").context("create strict auto-review marker")?;
test.submit_turn_with_approval_and_permission_profile(
"request strict review, then run the shell command",
AskForApproval::OnRequest,
PermissionProfile::Disabled,
)
.await?;
let request = wait_for_event(&test.codex, |event| {
matches!(event, EventMsg::RequestPermissions(_))
})
.await;
let EventMsg::RequestPermissions(request) = request else {
panic!("expected request permissions event");
};
assert_eq!(request.call_id, permission_call_id);
test.codex
.submit(Op::RequestPermissionsResponse {
id: permission_call_id.to_string(),
response: RequestPermissionsResponse {
permissions: request.permissions,
scope: PermissionGrantScope::Turn,
strict_auto_review: true,
},
})
.await?;
wait_for_event(&test.codex, |event| {
matches!(event, EventMsg::TurnComplete(_))
})
.await;
let requests = responses.requests();
assert_eq!(requests.len(), 3);
requests[2].function_call_output(command_call_id);
assert!(
!marker.exists(),
"hook-approved command should remove marker without Guardian review"
);
assert_single_permission_request_hook_input(
test.codex_home_path(),
&command,
/*description*/ None,
)?;
Ok(())
}
#[tokio::test]
async fn permission_request_hook_allows_apply_patch_with_write_alias() -> Result<()> {
skip_if_no_network!(Ok(()));

2
codex-rs/protocol/src/request_permissions.rs
View File

@@ -66,7 +66,7 @@ pub struct RequestPermissionsResponse {
pub permissions: RequestPermissionProfile,
#[serde(default)]
pub scope: PermissionGrantScope,
/// Review every subsequent command in this turn before normal sandboxed execution.
/// Review subsequent commands in this turn unless a permission hook resolves the request.
#[serde(default, skip_serializing_if = "std::ops::Not::not")]
pub strict_auto_review: bool,
}