github/codex
4
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-28 16:06:15 +00:00
Code Issues Packages Projects Releases Wiki Activity

Let permission hooks resolve strict auto-review requests (#32232)

Browse Source 
## What changed

- Run permission-request hooks before routing approvals to the automated reviewer or user, including when `strict_auto_review` is enabled.
- Centralize approval resolution so hook, automated-reviewer, and user decisions share rejection handling and report the correct telemetry source.

## Testing

- Add an integration test showing that an allow hook can approve a shell command during strict auto-review without invoking the automated reviewer.

GitOrigin-RevId: 18e9d76baee6cbae9d35ae517250f2f82270a9d2
This commit is contained in:
Dylan Hurd
2026-07-10 18:12:27 +00:00
committed by copyberry
parent 4472698728
commit 2da1b12822
6 changed files with 385 additions and 154 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/tools/approvals.rs
+213
View File
@@ -0,0 +1,213 @@
//! 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::guardian::routes_approval_to_guardian_with_reviewer;
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::NetworkPolicyRuleAction;
use codex_protocol::protocol::ReviewDecision;
pub(super) type ApprovalAction = crate::guardian::GuardianApprovalRequest;
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub(super) enum ApprovalReviewer {
Guardian,
User,
}
impl ApprovalReviewer {
pub(super) fn for_turn(turn: &TurnContext) -> Self {
Self::for_reviewer(turn, turn.config.approvals_reviewer)
}
fn for_reviewer(turn: &TurnContext, reviewer: ApprovalsReviewer) -> Self {
if routes_approval_to_guardian_with_reviewer(turn, reviewer) {
Self::Guardian
} else {
Self::User
}
}
}
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
enum ApprovalResolutionSource {
Hook,
Guardian,
User,
}
#[derive(Clone, Debug, PartialEq, Eq)]
struct ApprovalResolution {
decision: ReviewDecision,
rejection: Option<String>,
source: ApprovalResolutionSource,
}
impl ApprovalResolution {
fn into_tool_result(self) -> Result<ReviewDecision, ToolError> {
if let Some(rejection) = self.rejection {
Err(ToolError::Rejected(rejection))
} else {
Ok(self.decision)
}
}
}
pub(super) async fn resolve_tool_apporval<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<ReviewDecision, 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,
};
record_resolution(otel, tool_ctx, &resolution);
return resolution.into_tool_result();
}
Some(PermissionRequestDecision::Deny { message }) => {
let resolution = ApprovalResolution {
decision: ReviewDecision::Denied,
rejection: Some(message),
source: ApprovalResolutionSource::Hook,
};
record_resolution(otel, tool_ctx, &resolution);
return resolution.into_tool_result();
}
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,
};
record_resolution(otel, tool_ctx, &resolution);
return resolution.into_tool_result();
}
};
let decision = review_approval_request(
ctx.session,
ctx.turn,
review_id.clone(),
action,
ctx.retry_reason.clone(),
)
.await;
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 = normalize_user_rejection(resolution);
record_resolution(otel, tool_ctx, &resolution);
resolution.into_tool_result()
}
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,
);
}
codex-rs/core/src/tools/mod.rs
+1
View File
@@ -1,3 +1,4 @@
mod approvals;
pub(crate) mod code_mode;
pub(crate) mod context;
pub(crate) mod events;
codex-rs/core/src/tools/orchestrator.rs
+16 -145
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 super::approvals::ApprovalReviewer;
use super::approvals::resolve_tool_apporval;
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(
resolve_tool_apporval(
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(
resolve_tool_apporval(
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(
resolve_tool_apporval(
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
@@ -517,121 +503,6 @@ impl ToolOrchestrator {
}
}
}
// PermissionRequest hooks take top precedence for answering approval
// prompts. If no matching hook returns a decision, fall back to the
// normal guardian or user approval path.
async fn request_approval<Rq, Out, T>(
tool: &mut T,
req: &Rq,
permission_request_run_id: &str,
approval_ctx: ApprovalCtx<'_>,
tool_ctx: &ToolCtx,
evaluate_permission_request_hooks: bool,
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()))
}
},
}
}
}
fn sandbox_outcome_from_tool_error(err: &ToolError) -> Option<&'static str> {
codex-rs/core/src/tools/sandboxing.rs
+1 -8
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 = super::approvals::ApprovalAction;
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) struct PermissionRequestPayload {