mirror of
https://github.com/openai/codex.git
synced 2026-08-23 13:09:46 +00:00
Route escalated commands through synchronous Guardian review (#40005)
## Why Commands requesting `sandbox_permissions=require_escalated` need a full Guardian review even when they are not marked as retries. ## What changed - Treat escalated command requests, along with retries, as requiring synchronous Guardian review. - Bypass extension approval and Guardian V2 shortcuts for these requests. ## Testing Add an integration test that installs an auto-approving extension and verifies that an escalated command still reaches Guardian and honors its denial. GitOrigin-RevId: 30eed273460f3c3c6b24d1ce2d29889e34513afd
This commit is contained in:
@@ -319,14 +319,23 @@ async fn run_guardian_review(
|
||||
options: GuardianReviewOptions,
|
||||
) -> ReviewDecision {
|
||||
let turn = Arc::clone(context.turn());
|
||||
// Required models must use Guardian, but an enabled V2 monitor can satisfy the review.
|
||||
let requires_synchronous_review = reasons.retry.is_some()
|
||||
|| matches!(
|
||||
&request,
|
||||
GuardianApprovalRequest::ExecCommand {
|
||||
sandbox_permissions,
|
||||
..
|
||||
} if sandbox_permissions.requires_escalated_permissions()
|
||||
);
|
||||
// Guardian V2 may satisfy ordinary reviews, including required-model reviews, but broader
|
||||
// permission requests and retries must run Guardian synchronously.
|
||||
if (!turn
|
||||
.config
|
||||
.config_layer_stack
|
||||
.requirements()
|
||||
.auto_review_required_for_model(&turn.model_info.slug)
|
||||
|| turn.config.features.enabled(Feature::GuardianV2))
|
||||
&& reasons.retry.is_none()
|
||||
&& !requires_synchronous_review
|
||||
&& options
|
||||
.external_cancel
|
||||
.as_ref()
|
||||
|
||||
@@ -80,6 +80,23 @@ impl ApprovalReviewContributor for ApprovedReviewContributor {
|
||||
}
|
||||
}
|
||||
|
||||
struct EscalationApprovingReviewContributor;
|
||||
|
||||
impl ApprovalReviewContributor for EscalationApprovingReviewContributor {
|
||||
fn contribute<'a>(
|
||||
&'a self,
|
||||
_session_store: &'a ExtensionData,
|
||||
_thread_store: &'a ExtensionData,
|
||||
prompt: &'a str,
|
||||
_extension_metrics: Option<Arc<dyn ExtensionMetrics>>,
|
||||
) -> ExtensionFuture<'a, Option<ReviewDecision>> {
|
||||
Box::pin(async move {
|
||||
assert!(prompt.contains("\"sandbox_permissions\":\"require_escalated\""));
|
||||
Some(ReviewDecision::Approved)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn approval_review_contributor_skips_existing_guardian_model_call() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
@@ -160,6 +177,103 @@ async fn approval_review_contributor_skips_existing_guardian_model_call() -> Res
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn require_escalated_bypasses_extension_approval_and_runs_guardian() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
skip_if_wine_exec!(Ok(()), "exec_command requires host-native paths");
|
||||
|
||||
let server = MockServer::start().await;
|
||||
let call_id = "require-escalated-command";
|
||||
let justification = "run outside the sandbox after a blocked attempt";
|
||||
let responses = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-parent-1"),
|
||||
ev_function_call(
|
||||
call_id,
|
||||
"exec_command",
|
||||
&json!({
|
||||
"cmd": "pwd",
|
||||
"sandbox_permissions": "require_escalated",
|
||||
"justification": justification,
|
||||
})
|
||||
.to_string(),
|
||||
),
|
||||
ev_completed("resp-parent-1"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-guardian"),
|
||||
ev_assistant_message(
|
||||
"msg-guardian",
|
||||
&json!({
|
||||
"risk_level": "high",
|
||||
"user_authorization": "low",
|
||||
"outcome": "deny",
|
||||
"rationale": "The unsandboxed command is not authorized.",
|
||||
})
|
||||
.to_string(),
|
||||
),
|
||||
ev_completed("resp-guardian"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-parent-2"),
|
||||
ev_assistant_message("msg-parent", "done"),
|
||||
ev_completed("resp-parent-2"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
let mut extensions = ExtensionRegistryBuilder::new();
|
||||
extensions.approval_review_contributor(Arc::new(EscalationApprovingReviewContributor));
|
||||
let mut builder = test_codex()
|
||||
.with_extensions(Arc::new(extensions.build()))
|
||||
.with_config(|config| {
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
|
||||
config
|
||||
.permissions
|
||||
.set_permission_profile(PermissionProfile::read_only())
|
||||
.expect("set read-only permission profile");
|
||||
});
|
||||
let test = builder.build_with_auto_env(&server).await?;
|
||||
test.codex
|
||||
.start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text {
|
||||
text: "retry the blocked command outside the sandbox".into(),
|
||||
text_elements: Vec::new(),
|
||||
}]))
|
||||
.await?;
|
||||
|
||||
loop {
|
||||
match wait_for_event(&test.codex, |_| true).await {
|
||||
EventMsg::ExecApprovalRequest(event) => {
|
||||
panic!("escalated command should not prompt the user: {event:?}")
|
||||
}
|
||||
EventMsg::TurnComplete(_) => break,
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
|
||||
let requests = responses.requests();
|
||||
assert_eq!(requests.len(), 3);
|
||||
let guardian_request = requests
|
||||
.iter()
|
||||
.find(|request| {
|
||||
request.body_json()["client_metadata"]["x-openai-subagent"].as_str() == Some("guardian")
|
||||
})
|
||||
.context("expected full Guardian review for require_escalated")?;
|
||||
assert!(guardian_request.body_contains_text(justification));
|
||||
assert!(
|
||||
responses
|
||||
.function_call_output_text(call_id)
|
||||
.context("expected exec_command output")?
|
||||
.contains("not authorized")
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy)]
|
||||
enum GuardianV2DisableSource {
|
||||
Config,
|
||||
|
||||
@@ -1171,15 +1171,21 @@ async fn guardian_timeout_rejects_tool_call_with_acting_model_instructions(
|
||||
});
|
||||
})
|
||||
.with_config(|config| {
|
||||
let rules_dir = config.codex_home.join("rules");
|
||||
fs::create_dir_all(&rules_dir).expect("create execution policy directory");
|
||||
fs::write(
|
||||
rules_dir.join("default.rules"),
|
||||
r#"prefix_rule(pattern=["touch"], decision="prompt")"#,
|
||||
)
|
||||
.expect("write execution policy rule");
|
||||
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
});
|
||||
let test = builder.build_with_auto_env(&server).await?;
|
||||
let output_file = test.cwd.path().join("guardian-timed-out.txt");
|
||||
let tool_args = json!({
|
||||
"cmd": format!("printf should-not-run > {}", output_file.display()),
|
||||
"sandbox_permissions": SandboxPermissions::RequireEscalated,
|
||||
"justification": "Exercise Guardian timeout routing.",
|
||||
"cmd": format!("touch {}", output_file.display()),
|
||||
"sandbox_permissions": SandboxPermissions::UseDefault,
|
||||
});
|
||||
let responses = mount_sse_sequence(
|
||||
&server,
|
||||
|
||||
Reference in New Issue
Block a user