openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-18 17:18:52 +00:00
Code Issues Packages Projects Releases Wiki Activity

Route permission requests through shared Guardian approvals (#38701)

Browse Source 
## What changed

- Represent `request_permissions` calls as shared approval actions and convert them into Guardian permission requests through the common approval path.
- Preserve turn cancellation while an automatic permission review is pending.
- Cover Guardian allow and deny decisions, cancellation without a user-approval fallback, and clean follow-up turns.

GitOrigin-RevId: 95d0df0288e232f32b162756aa7d64a30df7efa5
This commit is contained in:
Dylan Hurd
2026-08-15 05:09:19 +00:00
committed by copyberry
parent 4861236f06
commit 53f3fa7496
3 changed files with 341 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/session/mod.rs
+19 -21
View File
@@ -263,7 +263,6 @@ pub(crate) struct PreviousTurnSettings {
}
use crate::exec_policy::ExecPolicyUpdateError;
use crate::guardian::GuardianReviewOptions;
use crate::guardian::GuardianReviewSessionManager;
use crate::mcp::McpManager;
use crate::mcp::McpThreadIdentity;
@@ -281,11 +280,13 @@ use crate::stream_events_utils::HandleOutputCtx;
#[cfg(test)]
use crate::stream_events_utils::handle_output_item_done;
use crate::tasks::ReviewTask;
use crate::tools::ApprovalContext;
use crate::tools::network_approval::NetworkApprovalService;
use crate::tools::network_approval::build_blocked_request_observer;
use crate::tools::network_approval::build_network_policy_decider;
#[cfg(test)]
use crate::tools::parallel::ToolCallRuntime;
use crate::tools::sandboxing::ApprovalAction;
use crate::tools::sandboxing::ApprovalStore;
use crate::turn_timing::TurnTimingState;
use crate::turn_timing::record_turn_ttfm_metric;
@@ -347,6 +348,7 @@ use codex_protocol::turn_input::TurnInputRequest;
use codex_protocol::turn_input::TurnInputSubmission;
use codex_protocol::user_input::UserInput;
use codex_skills_extension::HostSkillsService;
use codex_tools::ToolName;
use codex_tools::UnifiedExecShellMode;
use codex_utils_absolute_path::AbsolutePathBuf;
#[cfg(test)]
@@ -2518,33 +2520,29 @@ impl Session {
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 request = crate::guardian::GuardianApprovalRequest::RequestPermissions {
id: call_id,
let action = ApprovalAction::RequestPermissions {
id: call_id.clone(),
turn_id: turn_context.sub_id.clone(),
reason: args.reason,
permissions: requested_permissions.clone(),
};
let review_rx = crate::guardian::spawn_approval_request_review(
session,
review_context.clone(),
review_id,
request,
/*retry_reason*/ None,
GuardianReviewOptions {
plugin_attribution_override: None,
approval_request_source:
codex_analytics::GuardianApprovalRequestSource::MainTurn,
external_cancel: Some(cancellation_token.clone()),
},
);
let approval_context = ApprovalContext {
review_context: review_context.clone(),
call_id,
tool_name: ToolName::plain("request_permissions"),
strict_auto_review: false,
approval_reason: None,
retry_reason: None,
network_approval_context: None,
};
let decision = tokio::select! {
biased;
_ = cancellation_token.cancelled() => return None,
decision = review_rx => decision.unwrap_or_else(|_| {
ReviewDecision::denied("automatic approval review could not complete")
}),
decision = self.request_guardian_approval(
action,
&approval_context,
Some(cancellation_token.clone()),
) => decision,
};
let response = match decision {
ReviewDecision::Approved | ReviewDecision::ApprovedExecpolicyAmendment { .. } => {
codex-rs/core/src/tools/approvals.rs
+62 -5
View File
@@ -9,6 +9,7 @@ use crate::guardian::new_guardian_review_id;
use crate::guardian::review_approval_request;
use crate::guardian::review_approval_request_with_cancel;
use crate::guardian::routes_approval_policy_to_guardian;
use crate::guardian::spawn_approval_request_review;
use crate::hook_runtime::run_permission_request_hooks;
use crate::mcp_tool_call::request_mcp_tool_user_approval;
use crate::sandboxing::SandboxPermissions;
@@ -39,6 +40,7 @@ use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::FileChange;
use codex_protocol::protocol::NetworkPolicyRuleAction;
use codex_protocol::protocol::ReviewDecision;
use codex_protocol::request_permissions::RequestPermissionProfile;
use codex_tools::ToolName;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_path_uri::PathUri;
@@ -139,6 +141,12 @@ pub(crate) enum ApprovalAction {
command: Vec<String>,
cwd: AbsolutePathBuf,
},
RequestPermissions {
id: String,
turn_id: String,
reason: Option<String>,
permissions: RequestPermissionProfile,
},
}
#[derive(Clone, Debug, Eq, Hash, PartialEq, serde::Serialize)]
@@ -189,6 +197,17 @@ impl ApprovalAction {
hook_command.clone(),
Some(format!("network-access {target}")),
),
Self::RequestPermissions {
reason,
permissions,
..
} => PermissionRequestPayload {
tool_name: HookToolName::new("request_permissions"),
tool_input: serde_json::json!({
"reason": reason,
"permissions": permissions,
}),
},
}
}
@@ -226,7 +245,9 @@ impl ApprovalAction {
})],
#[cfg(unix)]
Self::Execve { .. } => Vec::new(),
Self::McpToolCall { .. } | Self::NetworkAccess { .. } => Vec::new(),
Self::McpToolCall { .. }
| Self::NetworkAccess { .. }
| Self::RequestPermissions { .. } => Vec::new(),
Self::ApplyPatch {
environment_id,
files,
@@ -359,6 +380,17 @@ impl ApprovalAction {
port,
trigger,
},
Self::RequestPermissions {
id,
turn_id,
reason,
permissions,
} => crate::guardian::GuardianApprovalRequest::RequestPermissions {
id,
turn_id,
reason,
permissions,
},
})
}
}
@@ -532,7 +564,10 @@ impl Session {
};
let decision = match reviewer {
ApprovalReviewer::Guardian => self.request_guardian_approval(action, ctx).await,
ApprovalReviewer::Guardian => {
self.request_guardian_approval(action, ctx, /*cancellation_token*/ None)
.await
}
ApprovalReviewer::User => self.request_user_approval(&action, ctx).await,
};
let source = match reviewer {
@@ -542,10 +577,11 @@ impl Session {
ApprovalResolution { decision, source }
}
async fn request_guardian_approval(
pub(crate) async fn request_guardian_approval(
self: &Arc<Self>,
action: ApprovalAction,
ctx: &ApprovalContext,
cancellation_token: Option<CancellationToken>,
) -> ReviewDecision {
let is_network_approval = matches!(&action, ApprovalAction::NetworkAccess { .. });
let review_id = new_guardian_review_id();
@@ -559,7 +595,23 @@ impl Session {
}
};
if is_network_approval {
if let Some(cancellation_token) = cancellation_token {
let review = spawn_approval_request_review(
Arc::clone(self),
ctx.review_context.clone(),
review_id,
action,
ctx.retry_reason.clone(),
GuardianReviewOptions {
plugin_attribution_override: None,
approval_request_source: GuardianApprovalRequestSource::MainTurn,
external_cancel: Some(cancellation_token.clone()),
},
);
review.await.unwrap_or_else(|_| {
ReviewDecision::denied("automatic approval review could not complete")
})
} else if is_network_approval {
let review_cancel = CancellationToken::new();
let review_cancel_guard = review_cancel.clone().drop_guard();
let review_session = Arc::clone(self);
@@ -640,7 +692,9 @@ impl Session {
#[cfg(unix)]
ApprovalAction::Execve { .. } => unreachable!("matched command approval"),
ApprovalAction::ApplyPatch { .. } => unreachable!("matched command approval"),
ApprovalAction::McpToolCall { .. } | ApprovalAction::NetworkAccess { .. } => {
ApprovalAction::McpToolCall { .. }
| ApprovalAction::NetworkAccess { .. }
| ApprovalAction::RequestPermissions { .. } => {
unreachable!("matched command approval")
}
};
@@ -764,6 +818,9 @@ impl Session {
)
.await
}
ApprovalAction::RequestPermissions { .. } => {
unreachable!("permission requests are routed directly to Guardian")
}
}
}
}
codex-rs/core/tests/suite/request_permissions.rs
+260
View File
@@ -1,5 +1,6 @@
#![allow(clippy::unwrap_used)]
use anyhow::Context;
use anyhow::Result;
use codex_core::TurnInputRequest;
use codex_core::config::Constrained;
@@ -17,6 +18,7 @@ use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::ExecApprovalRequestEvent;
use codex_protocol::protocol::GranularApprovalConfig;
use codex_protocol::protocol::GuardianAssessmentStatus;
use codex_protocol::protocol::Op;
use codex_protocol::protocol::ReviewDecision;
use codex_protocol::protocol::ThreadSettingsOverrides;
@@ -29,12 +31,15 @@ use core_test_support::responses::ev_assistant_message;
use core_test_support::responses::ev_completed;
use core_test_support::responses::ev_function_call;
use core_test_support::responses::ev_response_created;
use core_test_support::responses::mount_response_once_match;
use core_test_support::responses::mount_sse_once;
use core_test_support::responses::mount_sse_sequence;
use core_test_support::responses::sse;
use core_test_support::responses::sse_response;
use core_test_support::responses::start_mock_server;
use core_test_support::skip_if_no_network;
use core_test_support::skip_if_sandbox;
use core_test_support::skip_if_wine_exec;
use core_test_support::test_codex::TestCodex;
use core_test_support::test_codex::local_selections;
use core_test_support::test_codex::test_codex;
@@ -46,6 +51,7 @@ use serde_json::Value;
use serde_json::json;
use std::fs;
use std::path::Path;
use std::time::Duration;
use test_case::test_case;
fn absolute_path(path: &Path) -> AbsolutePathBuf {
@@ -493,6 +499,260 @@ async fn request_permissions_tool_is_auto_denied_when_granular_request_permissio
Ok(())
}
#[test_case("allow" ; "guardian approval grants the requested permissions")]
#[test_case("deny" ; "guardian denial grants no permissions")]
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn request_permissions_auto_review_applies_guardian_decision(outcome: &str) -> Result<()> {
skip_if_no_network!(Ok(()));
skip_if_sandbox!(Ok(()));
skip_if_wine_exec!(
Ok(()),
"request_permissions requires a cwd native to the Codex host"
);
let server = start_mock_server().await;
let approval_policy = AskForApproval::OnRequest;
let mut builder = test_codex().with_config(move |config| {
config.permissions.approval_policy = Constrained::allow_any(approval_policy);
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
config
.features
.enable(Feature::RequestPermissionsTool)
.expect("test config should allow feature update");
});
let test = builder.build_with_auto_env(&server).await?;
let requested_dir = test.workspace_path("guardian-requested-permissions");
fs::create_dir_all(&requested_dir)?;
let requested_permissions = requested_directory_write_permissions(&requested_dir);
let normalized_permissions = normalized_directory_write_permissions(&requested_dir)?;
let call_id = "guardian-request-permissions";
let reason = "Guardian should review access to the requested directory";
let responses = mount_sse_sequence(
&server,
vec![
sse(vec![
ev_response_created("resp-guardian-permissions-parent"),
request_permissions_tool_event(call_id, reason, &requested_permissions)?,
ev_completed("resp-guardian-permissions-parent"),
]),
sse(vec![
ev_response_created("resp-guardian-permissions-review"),
ev_assistant_message(
"msg-guardian-permissions-review",
&json!({
"risk_level": "low",
"user_authorization": "high",
"outcome": outcome,
"rationale": "Exercise the request_permissions Guardian decision.",
})
.to_string(),
),
ev_completed("resp-guardian-permissions-review"),
]),
sse(vec![
ev_response_created("resp-guardian-permissions-followup"),
ev_assistant_message("msg-guardian-permissions-followup", "done"),
ev_completed("resp-guardian-permissions-followup"),
]),
],
)
.await;
test.codex
.start_or_steer_turn(
TurnInputRequest::user_input(vec![UserInput::Text {
text: "request Guardian-reviewed directory permissions".into(),
text_elements: Vec::new(),
}])
.with_thread_settings(ThreadSettingsOverrides {
approval_policy: Some(approval_policy),
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
..Default::default()
}),
)
.await?;
let event = wait_for_event(&test.codex, |event| {
matches!(
event,
EventMsg::RequestPermissions(_) | EventMsg::TurnComplete(_)
)
})
.await;
assert!(
matches!(event, EventMsg::TurnComplete(_)),
"Guardian-reviewed permissions should not prompt the user: {event:?}"
);
let requests = responses.requests();
let guardian_request = requests
.iter()
.find(|request| {
request.body_json()["client_metadata"]["x-openai-subagent"].as_str() == Some("guardian")
})
.context("expected Guardian review request")?;
assert!(guardian_request.body_contains_text(reason));
assert!(
guardian_request.body_contains_text(&requested_dir.canonicalize()?.display().to_string())
);
let output = requests
.iter()
.find_map(|request| request.function_call_output_text(call_id))
.context("expected request_permissions output")?;
let actual: RequestPermissionsResponse = serde_json::from_str(&output)?;
let expected_permissions = if outcome == "allow" {
normalized_permissions
} else {
RequestPermissionProfile::default()
};
assert_eq!(
actual,
RequestPermissionsResponse {
permissions: expected_permissions,
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
}
);
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn interrupted_request_permissions_auto_review_aborts_guardian_review() -> Result<()> {
skip_if_no_network!(Ok(()));
skip_if_sandbox!(Ok(()));
skip_if_wine_exec!(
Ok(()),
"request_permissions requires a cwd native to the Codex host"
);
let server = start_mock_server().await;
let approval_policy = AskForApproval::OnRequest;
let mut builder = test_codex().with_config(move |config| {
config.permissions.approval_policy = Constrained::allow_any(approval_policy);
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
config
.features
.enable(Feature::RequestPermissionsTool)
.expect("test config should allow feature update");
});
let test = builder.build_with_auto_env(&server).await?;
let requested_dir = test.workspace_path("guardian-interrupted-permissions");
fs::create_dir_all(&requested_dir)?;
let requested_permissions = requested_directory_write_permissions(&requested_dir);
let call_id = "guardian-request-permissions-interrupted";
mount_sse_once(
&server,
sse(vec![
ev_response_created("resp-guardian-interrupted-permissions-parent"),
request_permissions_tool_event(
call_id,
"Interrupt Guardian before it grants permissions",
&requested_permissions,
)?,
ev_completed("resp-guardian-interrupted-permissions-parent"),
]),
)
.await;
let pending_guardian = mount_response_once_match(
&server,
|request: &wiremock::Request| {
serde_json::from_slice::<Value>(&request.body)
.ok()
.and_then(|body| {
body.pointer("/client_metadata/x-openai-subagent")
.and_then(Value::as_str)
.map(str::to_owned)
})
.as_deref()
== Some("guardian")
},
sse_response(sse(vec![
ev_response_created("resp-guardian-interrupted-permissions-review"),
ev_assistant_message(
"msg-guardian-interrupted-permissions-review",
r#"{"outcome":"allow"}"#,
),
ev_completed("resp-guardian-interrupted-permissions-review"),
]))
.set_delay(Duration::from_secs(30)),
)
.await;
test.codex
.start_or_steer_turn(
TurnInputRequest::user_input(vec![UserInput::Text {
text: "interrupt a Guardian-reviewed permissions request".into(),
text_elements: Vec::new(),
}])
.with_thread_settings(ThreadSettingsOverrides {
approval_policy: Some(approval_policy),
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
..Default::default()
}),
)
.await?;
tokio::time::timeout(Duration::from_secs(5), async {
while pending_guardian.requests().is_empty() {
tokio::task::yield_now().await;
}
})
.await
.context("timed out waiting for the request_permissions Guardian review")?;
test.codex.submit(Op::Interrupt).await?;
let mut saw_turn_aborted = false;
let mut saw_guardian_aborted = false;
while !saw_turn_aborted || !saw_guardian_aborted {
let event = tokio::time::timeout(Duration::from_secs(5), test.codex.next_event())
.await
.context("timed out waiting for parent and Guardian cancellation")?
.context("event stream ended while waiting for cancellation")?;
saw_turn_aborted |= matches!(&event.msg, EventMsg::TurnAborted(_));
saw_guardian_aborted |= matches!(
&event.msg,
EventMsg::GuardianAssessment(assessment)
if assessment.status == GuardianAssessmentStatus::Aborted
);
assert!(
!matches!(&event.msg, EventMsg::RequestPermissions(_)),
"interrupted Guardian review should not fall back to a user prompt"
);
}
let follow_up = mount_sse_once(
&server,
sse(vec![
ev_response_created("resp-parent-after-interrupted-permissions-review"),
ev_assistant_message("msg-parent-after-interrupted-permissions-review", "done"),
ev_completed("resp-parent-after-interrupted-permissions-review"),
]),
)
.await;
test.codex
.start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text {
text: "verify interrupted permissions review left the next turn clean".into(),
text_elements: Vec::new(),
}]))
.await?;
wait_for_completion(&test).await;
let follow_up_request = follow_up.single_request();
assert!(follow_up_request.has_function_call(call_id));
let interrupted_output = follow_up_request
.function_call_output_text(call_id)
.context("expected the interrupted permissions request's tool output")?;
assert!(
interrupted_output.contains("aborted") || interrupted_output.contains("cancelled"),
"unexpected interrupted permissions output: {interrupted_output}"
);
Ok(())
}
#[derive(Clone, Copy, Debug)]
enum AdditionalPermissionsCommandTool {
ShellCommand,