feat(auto-review) Handle request_permissions calls (#18393)

## Summary
When auto-review is enabled, it should handle request_permissions tool.
We'll need to clean up the UX but I'm planning to do that in a separate
pass

## Testing
- [x] Ran locally
<img width="893" height="396" alt="Screenshot 2026-04-17 at 1 16 13 PM"
src="https://github.com/user-attachments/assets/4c045c5f-1138-4c6c-ac6e-2cb6be4514d8"
/>

---------

Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
Dylan Hurd
2026-04-20 21:48:57 -07:00
committed by GitHub
co-authored by Codex
parent 543a08dac9
commit 86535c9901
31 changed files with 2232 additions and 370 deletions
+116 -26
View File
@@ -1894,12 +1894,13 @@ impl Session {
}
pub async fn request_permissions(
&self,
turn_context: &TurnContext,
self: &Arc<Self>,
turn_context: &Arc<TurnContext>,
call_id: String,
args: RequestPermissionsArgs,
cancellation_token: CancellationToken,
) -> Option<RequestPermissionsResponse> {
match turn_context.approval_policy.value() {
match turn_context.as_ref().approval_policy.value() {
AskForApproval::Never => {
return Some(RequestPermissionsResponse {
permissions: RequestPermissionProfile::default(),
@@ -1920,6 +1921,72 @@ impl Session {
| AskForApproval::Granular(_) => {}
}
if crate::guardian::routes_approval_to_guardian(turn_context.as_ref()) {
let requested_permissions = args.permissions;
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,
/*retry_reason*/ None,
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 {
ReviewDecision::Approved | ReviewDecision::ApprovedExecpolicyAmendment { .. } => {
RequestPermissionsResponse {
permissions: requested_permissions,
scope: PermissionGrantScope::Turn,
}
}
ReviewDecision::ApprovedForSession => RequestPermissionsResponse {
permissions: requested_permissions,
scope: PermissionGrantScope::Session,
},
ReviewDecision::NetworkPolicyAmendment {
network_policy_amendment,
} => match network_policy_amendment.action {
NetworkPolicyRuleAction::Allow => RequestPermissionsResponse {
permissions: requested_permissions,
scope: PermissionGrantScope::Turn,
},
NetworkPolicyRuleAction::Deny => RequestPermissionsResponse {
permissions: RequestPermissionProfile::default(),
scope: PermissionGrantScope::Turn,
},
},
ReviewDecision::Abort | ReviewDecision::Denied | ReviewDecision::TimedOut => {
RequestPermissionsResponse {
permissions: RequestPermissionProfile::default(),
scope: PermissionGrantScope::Turn,
}
}
};
self.record_granted_request_permissions_for_turn(
&response,
originating_turn_state.as_ref(),
)
.await;
return Some(response);
}
let (tx_response, rx_response) = oneshot::channel();
let prev_entry = {
let mut active = self.active_turn.lock().await;
@@ -1935,17 +2002,25 @@ impl Session {
warn!("Overwriting existing pending request_permissions for call_id: {call_id}");
}
// TODO(ccunningham): Support auto-review for request_permissions /
// with_additional_permissions. V0 still routes this surface through
// the existing manual RequestPermissions event flow.
let event = EventMsg::RequestPermissions(RequestPermissionsEvent {
call_id,
call_id: call_id.clone(),
turn_id: turn_context.sub_id.clone(),
reason: args.reason,
permissions: args.permissions,
});
self.send_event(turn_context, event).await;
rx_response.await.ok()
self.send_event(turn_context.as_ref(), event).await;
tokio::select! {
biased;
_ = cancellation_token.cancelled() => {
let mut active = self.active_turn.lock().await;
if let Some(at) = active.as_mut() {
let mut ts = at.turn_state.lock().await;
let _ = ts.remove_pending_request_permissions(&call_id);
}
None
}
response = rx_response => response.ok(),
}
}
pub async fn request_user_input(
@@ -2010,31 +2085,24 @@ impl Session {
call_id: &str,
response: RequestPermissionsResponse,
) {
let mut granted_for_session = None;
let entry = {
let (entry, originating_turn_state) = {
let mut active = self.active_turn.lock().await;
match active.as_mut() {
Some(at) => {
let mut ts = at.turn_state.lock().await;
let entry = ts.remove_pending_request_permissions(call_id);
if entry.is_some() && !response.permissions.is_empty() {
match response.scope {
PermissionGrantScope::Turn => {
ts.record_granted_permissions(response.permissions.clone().into());
}
PermissionGrantScope::Session => {
granted_for_session = Some(response.permissions.clone());
}
}
}
entry
let originating_turn_state = entry.as_ref().map(|_| Arc::clone(&at.turn_state));
(entry, originating_turn_state)
}
None => None,
None => (None, None),
}
};
if let Some(permissions) = granted_for_session {
let mut state = self.state.lock().await;
state.record_granted_permissions(permissions.into());
if entry.is_some() {
self.record_granted_request_permissions_for_turn(
&response,
originating_turn_state.as_ref(),
)
.await;
}
match entry {
Some(tx_response) => {
@@ -2046,6 +2114,28 @@ impl Session {
}
}
async fn record_granted_request_permissions_for_turn(
&self,
response: &RequestPermissionsResponse,
originating_turn_state: Option<&Arc<Mutex<crate::state::TurnState>>>,
) {
if response.permissions.is_empty() {
return;
}
match response.scope {
PermissionGrantScope::Turn => {
if let Some(turn_state) = originating_turn_state {
let mut ts = turn_state.lock().await;
ts.record_granted_permissions(response.permissions.clone().into());
}
}
PermissionGrantScope::Session => {
let mut state = self.state.lock().await;
state.record_granted_permissions(response.permissions.clone().into());
}
}
}
pub(crate) async fn granted_turn_permissions(&self) -> Option<PermissionProfile> {
let active = self.active_turn.lock().await;
let active = active.as_ref()?;
+42 -2
View File
@@ -3438,6 +3438,41 @@ async fn notify_request_permissions_response_ignores_unmatched_call_id() {
assert_eq!(session.granted_turn_permissions().await, None);
}
#[tokio::test]
async fn record_granted_request_permissions_for_turn_uses_originating_turn() {
let (session, _turn_context) = make_session_and_context().await;
let originating_active_turn = ActiveTurn::default();
let originating_turn_state = Arc::clone(&originating_active_turn.turn_state);
*session.active_turn.lock().await = Some(originating_active_turn);
let current_active_turn = ActiveTurn::default();
let current_turn_state = Arc::clone(&current_active_turn.turn_state);
*session.active_turn.lock().await = Some(current_active_turn);
let requested_permissions = RequestPermissionProfile {
network: Some(codex_protocol::models::NetworkPermissions {
enabled: Some(true),
}),
..RequestPermissionProfile::default()
};
session
.record_granted_request_permissions_for_turn(
&codex_protocol::request_permissions::RequestPermissionsResponse {
permissions: requested_permissions.clone(),
scope: PermissionGrantScope::Turn,
},
Some(&originating_turn_state),
)
.await;
assert_eq!(
originating_turn_state.lock().await.granted_permissions(),
Some(requested_permissions.into())
);
assert_eq!(current_turn_state.lock().await.granted_permissions(), None);
assert_eq!(session.granted_turn_permissions().await, None);
}
#[tokio::test]
async fn request_permissions_emits_event_when_granular_policy_allows_requests() {
let (session, mut turn_context, rx) = make_session_and_context_with_rx().await;
@@ -3474,7 +3509,7 @@ async fn request_permissions_emits_event_when_granular_policy_allows_requests()
async move {
session
.request_permissions(
turn_context.as_ref(),
&turn_context,
call_id,
codex_protocol::request_permissions::RequestPermissionsArgs {
reason: Some("need network".to_string()),
@@ -3485,6 +3520,7 @@ async fn request_permissions_emits_event_when_granular_policy_allows_requests()
..RequestPermissionProfile::default()
},
},
CancellationToken::new(),
)
.await
}
@@ -3532,7 +3568,7 @@ async fn request_permissions_is_auto_denied_when_granular_policy_blocks_tool_req
let call_id = "call-1".to_string();
let response = session
.request_permissions(
turn_context.as_ref(),
&turn_context,
call_id,
codex_protocol::request_permissions::RequestPermissionsArgs {
reason: Some("need network".to_string()),
@@ -3543,6 +3579,7 @@ async fn request_permissions_is_auto_denied_when_granular_policy_blocks_tool_req
..RequestPermissionProfile::default()
},
},
CancellationToken::new(),
)
.await;
@@ -6360,6 +6397,7 @@ async fn fatal_tool_error_stops_turn_and_reports_error() {
.dispatch_tool_call_with_code_mode_result(
Arc::clone(&session),
Arc::clone(&turn_context),
CancellationToken::new(),
tracker,
call,
ToolCallSource::Direct,
@@ -6602,6 +6640,7 @@ async fn rejects_escalated_permissions_when_policy_not_on_request() {
.handle(ToolInvocation {
session: Arc::clone(&session),
turn: Arc::clone(&turn_context),
cancellation_token: CancellationToken::new(),
tracker: Arc::clone(&turn_diff_tracker),
call_id,
tool_name: codex_tools::ToolName::plain(tool_name),
@@ -6680,6 +6719,7 @@ async fn unified_exec_rejects_escalated_permissions_when_policy_not_on_request()
.handle(ToolInvocation {
session: Arc::clone(&session),
turn: Arc::clone(&turn_context),
cancellation_token: CancellationToken::new(),
tracker: Arc::clone(&tracker),
call_id: "exec-call".to_string(),
tool_name: codex_tools::ToolName::plain("exec_command"),
@@ -17,6 +17,7 @@ use codex_execpolicy::Evaluation;
use codex_execpolicy::RuleMatch;
use codex_features::Feature;
use codex_model_provider::create_model_provider;
use codex_protocol::config_types::ApprovalsReviewer;
use codex_protocol::models::ContentItem;
use codex_protocol::models::NetworkPermissions;
use codex_protocol::models::PermissionProfile;
@@ -25,26 +26,214 @@ use codex_protocol::models::function_call_output_content_items_to_text;
use codex_protocol::permissions::FileSystemSandboxPolicy;
use codex_protocol::permissions::NetworkSandboxPolicy;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::request_permissions::PermissionGrantScope;
use codex_protocol::request_permissions::RequestPermissionProfile;
use codex_protocol::request_permissions::RequestPermissionsArgs;
use codex_protocol::request_permissions::RequestPermissionsResponse;
use core_test_support::PathExt;
use core_test_support::TempDirExt;
use core_test_support::codex_linux_sandbox_exe_or_skip;
use core_test_support::responses::ev_assistant_message;
use core_test_support::responses::ev_completed;
use core_test_support::responses::ev_response_created;
use core_test_support::responses::mount_response_once;
use core_test_support::responses::mount_sse_once;
use core_test_support::responses::sse;
use core_test_support::responses::sse_response;
use core_test_support::responses::start_mock_server;
use pretty_assertions::assert_eq;
use serde::Deserialize;
use std::collections::HashMap;
use std::fs;
use std::sync::Arc;
use std::time::Duration;
use tempfile::tempdir;
use tokio_util::sync::CancellationToken;
fn expect_text_output(output: &FunctionToolOutput) -> String {
function_call_output_content_items_to_text(&output.body).unwrap_or_default()
}
#[tokio::test]
async fn request_permissions_routes_to_guardian_when_reviewer_is_enabled() {
let server = start_mock_server().await;
let guardian_request_log = mount_sse_once(
&server,
sse(vec![
ev_response_created("resp-guardian"),
ev_assistant_message(
"msg-guardian",
&serde_json::json!({
"risk_level": "low",
"user_authorization": "high",
"outcome": "allow",
"rationale": "The request grants narrowly scoped network access for this turn.",
})
.to_string(),
),
ev_completed("resp-guardian"),
]),
)
.await;
let (mut session, mut turn_context_raw) = make_session_and_context().await;
*session.active_turn.lock().await = Some(ActiveTurn::default());
turn_context_raw
.approval_policy
.set(AskForApproval::OnRequest)
.expect("test setup should allow updating approval policy");
turn_context_raw
.features
.enable(Feature::GuardianApproval)
.expect("test setup should allow enabling guardian approvals");
let mut config = (*turn_context_raw.config).clone();
config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
config.model_provider.base_url = Some(format!("{}/v1", server.uri()));
let config = Arc::new(config);
let models_manager = Arc::new(crate::test_support::models_manager_with_provider(
config.codex_home.to_path_buf(),
Arc::clone(&session.services.auth_manager),
config.model_provider.clone(),
));
session.services.models_manager = models_manager;
turn_context_raw.config = Arc::clone(&config);
turn_context_raw.provider = create_model_provider(
config.model_provider.clone(),
turn_context_raw.auth_manager.clone(),
);
let session = Arc::new(session);
let turn_context = Arc::new(turn_context_raw);
let requested_permissions = RequestPermissionProfile {
network: Some(NetworkPermissions {
enabled: Some(true),
}),
..RequestPermissionProfile::default()
};
let response = tokio::time::timeout(
Duration::from_secs(45),
session.request_permissions(
&turn_context,
"perm-call-1".to_string(),
RequestPermissionsArgs {
reason: Some("need network".to_string()),
permissions: requested_permissions.clone(),
},
CancellationToken::new(),
),
)
.await
.expect("request_permissions should not wait for a client approval");
assert_eq!(
response,
Some(RequestPermissionsResponse {
permissions: requested_permissions.clone(),
scope: PermissionGrantScope::Turn,
})
);
assert_eq!(
session.granted_turn_permissions().await,
Some(requested_permissions.into())
);
let guardian_request = guardian_request_log.single_request();
assert_eq!(guardian_request.path(), "/v1/responses");
assert!(guardian_request.body_contains_text("request_permissions"));
assert!(guardian_request.body_contains_text("need network"));
}
#[tokio::test]
async fn request_permissions_guardian_review_stops_when_cancelled() {
let server = start_mock_server().await;
let _guardian_request_log = mount_response_once(
&server,
sse_response(sse(vec![ev_response_created("resp-guardian-delayed")]))
.set_delay(Duration::from_secs(60)),
)
.await;
let (mut session, mut turn_context, rx_event) = make_session_and_context_with_rx().await;
*session.active_turn.lock().await = Some(ActiveTurn::default());
let turn_context_raw = Arc::get_mut(&mut turn_context).expect("single turn context ref");
turn_context_raw
.approval_policy
.set(AskForApproval::OnRequest)
.expect("test setup should allow updating approval policy");
turn_context_raw
.features
.enable(Feature::GuardianApproval)
.expect("test setup should allow enabling guardian approvals");
let mut config = (*turn_context_raw.config).clone();
config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
config.model_provider.base_url = Some(format!("{}/v1", server.uri()));
let config = Arc::new(config);
let models_manager = Arc::new(crate::test_support::models_manager_with_provider(
config.codex_home.to_path_buf(),
Arc::clone(&session.services.auth_manager),
config.model_provider.clone(),
));
Arc::get_mut(&mut session)
.expect("single session ref")
.services
.models_manager = models_manager;
turn_context_raw.config = Arc::clone(&config);
turn_context_raw.provider = create_model_provider(
config.model_provider.clone(),
turn_context_raw.auth_manager.clone(),
);
let requested_permissions = RequestPermissionProfile {
network: Some(NetworkPermissions {
enabled: Some(true),
}),
..RequestPermissionProfile::default()
};
let cancellation_token = CancellationToken::new();
let request_handle = tokio::spawn({
let session = Arc::clone(&session);
let turn_context = Arc::clone(&turn_context);
let requested_permissions = requested_permissions.clone();
let cancellation_token = cancellation_token.clone();
async move {
session
.request_permissions(
&turn_context,
"perm-call-cancelled".to_string(),
RequestPermissionsArgs {
reason: Some("need network".to_string()),
permissions: requested_permissions,
},
cancellation_token,
)
.await
}
});
timeout(Duration::from_secs(5), async {
loop {
let event = rx_event.recv().await.expect("event channel should be open");
if matches!(
event.msg,
codex_protocol::protocol::EventMsg::GuardianAssessment(_)
) {
break;
}
}
})
.await
.expect("guardian review should start before cancellation");
cancellation_token.cancel();
let response = timeout(Duration::from_secs(5), request_handle)
.await
.expect("request_permissions should stop when cancelled")
.expect("request_permissions task should not panic");
assert_eq!(response, None);
assert_eq!(session.granted_turn_permissions().await, None);
}
#[tokio::test]
async fn guardian_allows_shell_additional_permissions_requests_past_policy_validation() {
let server = start_mock_server().await;
@@ -146,6 +335,7 @@ async fn guardian_allows_shell_additional_permissions_requests_past_policy_valid
.handle(ToolInvocation {
session: Arc::clone(&session),
turn: Arc::clone(&turn_context),
cancellation_token: CancellationToken::new(),
tracker: Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())),
call_id: "test-call".to_string(),
tool_name: codex_tools::ToolName::plain("shell"),
@@ -212,6 +402,7 @@ async fn guardian_allows_unified_exec_additional_permissions_requests_past_polic
.handle(ToolInvocation {
session: Arc::clone(&session),
turn: Arc::clone(&turn_context),
cancellation_token: CancellationToken::new(),
tracker: Arc::clone(&tracker),
call_id: "exec-call".to_string(),
tool_name: codex_tools::ToolName::plain("exec_command"),
@@ -325,6 +516,7 @@ async fn shell_handler_allows_sticky_turn_permissions_without_inline_request_per
.handle(ToolInvocation {
session: Arc::clone(&session),
turn: Arc::clone(&turn_context),
cancellation_token: CancellationToken::new(),
tracker: Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())),
call_id: "sticky-turn-grant".to_string(),
tool_name: codex_tools::ToolName::plain("shell"),