openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-05 20:10:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(approvals): centralize permission reviews

Browse Source
This commit is contained in:
Dylan Hurd
2026-07-07 15:33:06 -07:00
parent 0d10edef5c
commit a0b20bd461
2 changed files with 205 additions and 76 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/approval_coordinator.rs
+135 -44
View File
@@ -7,6 +7,7 @@ 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::spawn_approval_request_review;
use crate::hook_runtime::run_permission_request_hooks;
use crate::session::session::Session;
use crate::session::turn_context::TurnContext;
@@ -16,12 +17,14 @@ use crate::tools::sandboxing::PermissionRequestPayload;
use crate::tools::sandboxing::ToolCtx;
use crate::tools::sandboxing::ToolError;
use crate::tools::sandboxing::ToolRuntime;
use codex_analytics::GuardianApprovalRequestSource;
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;
use tokio_util::sync::CancellationToken;
pub(crate) type ApprovalAction = crate::guardian::GuardianApprovalRequest;
@@ -29,6 +32,8 @@ pub(crate) type ApprovalAction = crate::guardian::GuardianApprovalRequest;
pub(crate) struct ApprovalReview {
pub(crate) action: ApprovalAction,
pub(crate) retry_reason: Option<String>,
request_source: GuardianApprovalRequestSource,
cancellation: Option<CancellationToken>,
}
impl ApprovalReview {
@@ -36,6 +41,34 @@ impl ApprovalReview {
Self {
action,
retry_reason,
request_source: GuardianApprovalRequestSource::MainTurn,
cancellation: None,
}
}
pub(crate) fn main_turn_cancellable(
action: ApprovalAction,
retry_reason: Option<String>,
cancellation: CancellationToken,
) -> Self {
Self {
action,
retry_reason,
request_source: GuardianApprovalRequestSource::MainTurn,
cancellation: Some(cancellation),
}
}
pub(crate) fn delegated(
action: ApprovalAction,
retry_reason: Option<String>,
cancellation: CancellationToken,
) -> Self {
Self {
action,
retry_reason,
request_source: GuardianApprovalRequestSource::DelegatedSubagent,
cancellation: Some(cancellation),
}
}
}
@@ -218,11 +251,11 @@ impl ApprovalCoordinator {
hook_request,
review,
|| async {
ApprovalUserReview {
decision: user_review().await,
output: (),
}
},
ApprovalUserReview {
decision: user_review().await,
output: (),
}
},
)
.await
.resolution
@@ -236,14 +269,96 @@ impl ApprovalCoordinator {
review: ApprovalReview,
) -> ApprovalResolution {
debug_assert_eq!(reviewer, ApprovalReviewer::Guardian);
Self::resolve_event_with_user_output(session, turn, reviewer, hook_request, review, || async {
ApprovalUserReview {
Self::try_resolve_automatic(session, turn, reviewer, hook_request, review)
.await
.unwrap_or(ApprovalResolution {
decision: ReviewDecision::Denied,
output: (),
rejection: Some("automatic approval reviewer routed to the user".to_string()),
source: ApprovalResolutionSource::User,
})
}
pub(crate) async fn try_resolve_automatic(
session: &Arc<Session>,
turn: &Arc<TurnContext>,
reviewer: ApprovalReviewer,
hook_request: Option<ApprovalHookRequest<'_>>,
review: ApprovalReview,
) -> Option<ApprovalResolution> {
if let Some(hook_request) = hook_request
&& let Some(resolution) =
Self::resolve_hook(session, turn, hook_request.run_id, hook_request.payload).await
{
return Some(resolution);
}
match reviewer {
ApprovalReviewer::Guardian => {
debug_assert!(review.cancellation.is_none());
debug_assert!(matches!(
review.request_source,
GuardianApprovalRequestSource::MainTurn
));
let review_id = new_guardian_review_id();
let decision = review_approval_request(
session,
turn,
review_id.clone(),
review.action,
review.retry_reason,
)
.await;
Some(Self::normalize_guardian(session, review_id, decision).await)
}
})
.await
.resolution
ApprovalReviewer::User => None,
}
}
pub(crate) async fn try_resolve_automatic_cancellable(
session: &Arc<Session>,
turn: &Arc<TurnContext>,
reviewer: ApprovalReviewer,
hook_request: Option<ApprovalHookRequest<'_>>,
review: ApprovalReview,
) -> Option<ApprovalResolution> {
if let Some(hook_request) = hook_request
&& let Some(resolution) =
Self::resolve_hook(session, turn, hook_request.run_id, hook_request.payload).await
{
return Some(resolution);
}
match reviewer {
ApprovalReviewer::Guardian => {
let Some(cancellation) = review.cancellation else {
tracing::error!(
"cancellable approval review is missing its cancellation token"
);
return Some(ApprovalResolution {
decision: ReviewDecision::Abort,
rejection: Some(
"automatic approval review is missing its cancellation token"
.to_string(),
),
source: ApprovalResolutionSource::Guardian,
});
};
let review_id = new_guardian_review_id();
let decision = spawn_approval_request_review(
Arc::clone(session),
Arc::clone(turn),
review_id.clone(),
review.action,
review.retry_reason,
review.request_source,
cancellation,
)
.await
.unwrap_or(ReviewDecision::Denied);
Some(Self::normalize_guardian(session, review_id, decision).await)
}
ApprovalReviewer::User => None,
}
}
pub(crate) async fn resolve_event_with_user_output<T, F, Fut>(
@@ -258,9 +373,8 @@ impl ApprovalCoordinator {
F: FnOnce() -> Fut,
Fut: Future<Output = ApprovalUserReview<T>>,
{
if let Some(hook_request) = hook_request
&& let Some(resolution) =
Self::resolve_hook(session, turn, hook_request.run_id, hook_request.payload).await
if let Some(resolution) =
Self::try_resolve_automatic(session, turn, reviewer, hook_request, review).await
{
return ApprovalEventResolution {
resolution,
@@ -268,37 +382,14 @@ impl ApprovalCoordinator {
};
}
let (resolution, user_output) = match reviewer {
ApprovalReviewer::Guardian => {
let review_id = new_guardian_review_id();
let decision = review_approval_request(
session,
turn,
review_id.clone(),
review.action.clone(),
review.retry_reason.clone(),
)
.await;
(
Self::normalize_guardian(session, review_id, decision).await,
None,
)
}
ApprovalReviewer::User => {
let user_review = user_review().await;
(
ApprovalResolution {
decision: user_review.decision,
rejection: None,
source: ApprovalResolutionSource::User,
},
Some(user_review.output),
)
}
};
let user_review = user_review().await;
ApprovalEventResolution {
resolution: Self::normalize_user_rejection(resolution),
user_output,
resolution: Self::normalize_user_rejection(ApprovalResolution {
decision: user_review.decision,
rejection: None,
source: ApprovalResolutionSource::User,
}),
user_output: Some(user_review.output),
}
}
codex-rs/core/src/session/mod.rs
+70 -32
View File
@@ -16,6 +16,10 @@ use crate::agent::agent_status_from_event;
use crate::agent::status::is_final;
use crate::agent_communication::AgentCommunicationContext;
use crate::agent_communication::AgentCommunicationKind;
use crate::approval_coordinator::ApprovalAction;
use crate::approval_coordinator::ApprovalCoordinator;
use crate::approval_coordinator::ApprovalReview;
use crate::approval_coordinator::ApprovalReviewer;
use crate::attestation::AttestationProvider;
use crate::build_available_skills;
use crate::compact;
@@ -977,6 +981,12 @@ fn push_prompt_fragment(
}
}
#[derive(Clone, Copy, Debug)]
enum RequestPermissionsReviewOrigin {
MainTurn,
DelegatedSubagent,
}
impl Session {
pub(crate) async fn app_server_client_metadata(&self) -> AppServerClientMetadata {
let state = self.state.lock().await;
@@ -2228,10 +2238,6 @@ impl Session {
rx_approve
}
#[expect(
clippy::await_holding_invalid_type,
reason = "active turn checks and turn state updates must remain atomic"
)]
pub(crate) async fn request_permissions_for_environment(
self: &Arc<Self>,
turn_context: &Arc<TurnContext>,
@@ -2239,6 +2245,30 @@ impl Session {
args: RequestPermissionsArgs,
environment: TurnEnvironmentSelection,
cancellation_token: CancellationToken,
) -> Option<RequestPermissionsResponse> {
self.request_permissions_for_environment_with_origin(
turn_context,
call_id,
args,
environment,
cancellation_token,
RequestPermissionsReviewOrigin::MainTurn,
)
.await
}
#[expect(
clippy::await_holding_invalid_type,
reason = "active turn checks and turn state updates must remain atomic"
)]
async fn request_permissions_for_environment_with_origin(
self: &Arc<Self>,
turn_context: &Arc<TurnContext>,
call_id: String,
args: RequestPermissionsArgs,
environment: TurnEnvironmentSelection,
cancellation_token: CancellationToken,
review_origin: RequestPermissionsReviewOrigin,
) -> Option<RequestPermissionsResponse> {
match turn_context.as_ref().approval_policy.value() {
AskForApproval::Never => {
@@ -2276,35 +2306,42 @@ impl Session {
});
};
if crate::guardian::routes_approval_to_guardian(turn_context.as_ref()) {
let originating_turn_state = {
let active = self.active_turn.lock().await;
active.as_ref().map(|active| Arc::clone(&active.turn_state))
};
let review_id = crate::guardian::new_guardian_review_id();
let session = Arc::clone(self);
let turn = Arc::clone(turn_context);
let request = crate::guardian::GuardianApprovalRequest::RequestPermissions {
id: call_id,
turn_id: turn_context.sub_id.clone(),
reason: args.reason,
permissions: requested_permissions.clone(),
};
let review_rx = crate::guardian::spawn_approval_request_review(
session,
turn,
review_id,
request,
let originating_turn_state = {
let active = self.active_turn.lock().await;
active.as_ref().map(|active| Arc::clone(&active.turn_state))
};
let action = ApprovalAction::RequestPermissions {
id: call_id.clone(),
turn_id: turn_context.sub_id.clone(),
reason: args.reason.clone(),
permissions: requested_permissions.clone(),
};
let review = match review_origin {
RequestPermissionsReviewOrigin::MainTurn => ApprovalReview::main_turn_cancellable(
action,
/*retry_reason*/ None,
codex_analytics::GuardianApprovalRequestSource::MainTurn,
cancellation_token.clone(),
);
let decision = tokio::select! {
biased;
_ = cancellation_token.cancelled() => return None,
decision = review_rx => decision.unwrap_or(ReviewDecision::Denied),
};
let response = match decision {
),
RequestPermissionsReviewOrigin::DelegatedSubagent => ApprovalReview::delegated(
action,
/*retry_reason*/ None,
cancellation_token.clone(),
),
};
let automatic_review = ApprovalCoordinator::try_resolve_automatic_cancellable(
self,
turn_context,
ApprovalReviewer::for_turn(turn_context),
/*hook_request*/ None,
review,
);
let automatic_resolution = tokio::select! {
biased;
_ = cancellation_token.cancelled() => return None,
resolution = automatic_review => resolution,
};
if let Some(resolution) = automatic_resolution {
let response = match resolution.decision {
ReviewDecision::Approved | ReviewDecision::ApprovedExecpolicyAmendment { .. } => {
RequestPermissionsResponse {
permissions: requested_permissions.clone(),
@@ -2424,12 +2461,13 @@ impl Session {
};
let mut environment = turn_environment.selection();
environment.cwd = PathUri::from_abs_path(&cwd);
self.request_permissions_for_environment(
self.request_permissions_for_environment_with_origin(
turn_context,
call_id,
args,
environment,
cancellation_token,
RequestPermissionsReviewOrigin::DelegatedSubagent,
)
.await
}