mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
feat(request-permissions) approve with strict review (#19050)
## Summary Allow the user to approve a request_permissions_tool request with the condition that all commands in the rest of the turn are reviewed by guardian, regardless of sandbox status. ## Testing - [x] Added unit tests - [x] Ran locally
This commit is contained in:
Dylan Hurd
@@ -116,7 +116,8 @@ impl ToolOrchestrator {
|
||||
let otel = turn_ctx.session_telemetry.clone();
|
||||
let otel_tn = &tool_ctx.tool_name;
|
||||
let otel_ci = &tool_ctx.call_id;
|
||||
let use_guardian = routes_approval_to_guardian(turn_ctx);
|
||||
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;
|
||||
@@ -126,12 +127,37 @@ impl ToolOrchestrator {
|
||||
});
|
||||
match requirement {
|
||||
ExecApprovalRequirement::Skip { .. } => {
|
||||
otel.tool_decision(
|
||||
otel_tn,
|
||||
otel_ci,
|
||||
&ReviewDecision::Approved,
|
||||
ToolDecisionSource::Config,
|
||||
);
|
||||
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(
|
||||
tool,
|
||||
req,
|
||||
tool_ctx.call_id.as_str(),
|
||||
approval_ctx,
|
||||
tool_ctx,
|
||||
/*evaluate_permission_request_hooks*/ false,
|
||||
&otel,
|
||||
)
|
||||
.await?;
|
||||
Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision)
|
||||
.await?;
|
||||
already_approved = true;
|
||||
} else {
|
||||
otel.tool_decision(
|
||||
otel_tn,
|
||||
otel_ci,
|
||||
&ReviewDecision::Approved,
|
||||
ToolDecisionSource::Config,
|
||||
);
|
||||
}
|
||||
}
|
||||
ExecApprovalRequirement::Forbidden { reason } => {
|
||||
return Err(ToolError::Rejected(reason));
|
||||
@@ -152,35 +178,13 @@ impl ToolOrchestrator {
|
||||
tool_ctx.call_id.as_str(),
|
||||
approval_ctx,
|
||||
tool_ctx,
|
||||
use_guardian,
|
||||
/*evaluate_permission_request_hooks*/ !strict_auto_review,
|
||||
&otel,
|
||||
)
|
||||
.await?;
|
||||
|
||||
match decision {
|
||||
ReviewDecision::Denied | ReviewDecision::Abort => {
|
||||
let reason = if let Some(review_id) = guardian_review_id.as_deref() {
|
||||
guardian_rejection_message(tool_ctx.session.as_ref(), review_id).await
|
||||
} else {
|
||||
"rejected by user".to_string()
|
||||
};
|
||||
return Err(ToolError::Rejected(reason));
|
||||
}
|
||||
ReviewDecision::TimedOut => {
|
||||
return Err(ToolError::Rejected(guardian_timeout_message()));
|
||||
}
|
||||
ReviewDecision::Approved
|
||||
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
|
||||
| ReviewDecision::ApprovedForSession => {}
|
||||
ReviewDecision::NetworkPolicyAmendment {
|
||||
network_policy_amendment,
|
||||
} => match network_policy_amendment.action {
|
||||
NetworkPolicyRuleAction::Allow => {}
|
||||
NetworkPolicyRuleAction::Deny => {
|
||||
return Err(ToolError::Rejected("rejected by user".to_string()));
|
||||
}
|
||||
},
|
||||
}
|
||||
Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision)
|
||||
.await?;
|
||||
already_approved = true;
|
||||
}
|
||||
}
|
||||
@@ -287,9 +291,10 @@ impl ToolOrchestrator {
|
||||
build_denial_reason_from_output(output.as_ref())
|
||||
};
|
||||
|
||||
// Ask for approval before retrying with the escalated sandbox.
|
||||
let bypass_retry_approval = tool
|
||||
.should_bypass_approval(approval_policy, already_approved)
|
||||
// Strict auto-review approval covers the sandboxed attempt only;
|
||||
// retrying without the sandbox requires a fresh guardian review.
|
||||
let bypass_retry_approval = !strict_auto_review
|
||||
&& 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);
|
||||
@@ -309,36 +314,13 @@ impl ToolOrchestrator {
|
||||
&permission_request_run_id,
|
||||
approval_ctx,
|
||||
tool_ctx,
|
||||
use_guardian,
|
||||
/*evaluate_permission_request_hooks*/ !strict_auto_review,
|
||||
&otel,
|
||||
)
|
||||
.await?;
|
||||
|
||||
match decision {
|
||||
ReviewDecision::Denied | ReviewDecision::Abort => {
|
||||
let reason = if let Some(review_id) = guardian_review_id.as_deref() {
|
||||
guardian_rejection_message(tool_ctx.session.as_ref(), review_id)
|
||||
.await
|
||||
} else {
|
||||
"rejected by user".to_string()
|
||||
};
|
||||
return Err(ToolError::Rejected(reason));
|
||||
}
|
||||
ReviewDecision::TimedOut => {
|
||||
return Err(ToolError::Rejected(guardian_timeout_message()));
|
||||
}
|
||||
ReviewDecision::Approved
|
||||
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
|
||||
| ReviewDecision::ApprovedForSession => {}
|
||||
ReviewDecision::NetworkPolicyAmendment {
|
||||
network_policy_amendment,
|
||||
} => match network_policy_amendment.action {
|
||||
NetworkPolicyRuleAction::Allow => {}
|
||||
NetworkPolicyRuleAction::Deny => {
|
||||
return Err(ToolError::Rejected("rejected by user".to_string()));
|
||||
}
|
||||
},
|
||||
}
|
||||
Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision)
|
||||
.await?;
|
||||
}
|
||||
|
||||
let escalated_attempt = SandboxAttempt {
|
||||
@@ -385,13 +367,15 @@ impl ToolOrchestrator {
|
||||
permission_request_run_id: &str,
|
||||
approval_ctx: ApprovalCtx<'_>,
|
||||
tool_ctx: &ToolCtx,
|
||||
use_guardian: bool,
|
||||
evaluate_permission_request_hooks: bool,
|
||||
otel: &codex_otel::SessionTelemetry,
|
||||
) -> Result<ReviewDecision, ToolError>
|
||||
where
|
||||
T: ToolRuntime<Rq, Out>,
|
||||
{
|
||||
if let Some(permission_request) = tool.permission_request_payload(req) {
|
||||
if evaluate_permission_request_hooks
|
||||
&& let Some(permission_request) = tool.permission_request_payload(req)
|
||||
{
|
||||
match run_permission_request_hooks(
|
||||
approval_ctx.session,
|
||||
approval_ctx.turn,
|
||||
@@ -424,12 +408,12 @@ impl ToolOrchestrator {
|
||||
}
|
||||
}
|
||||
|
||||
let decision = tool.start_approval_async(req, approval_ctx).await;
|
||||
let otel_source = if use_guardian {
|
||||
let otel_source = if approval_ctx.guardian_review_id.is_some() {
|
||||
ToolDecisionSource::AutomatedReviewer
|
||||
} else {
|
||||
ToolDecisionSource::User
|
||||
};
|
||||
let decision = tool.start_approval_async(req, approval_ctx).await;
|
||||
otel.tool_decision(
|
||||
&tool_ctx.tool_name,
|
||||
&tool_ctx.call_id,
|
||||
@@ -438,6 +422,35 @@ impl ToolOrchestrator {
|
||||
);
|
||||
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 build_denial_reason_from_output(_output: &ExecToolCallOutput) -> String {
|
||||
|
||||
@@ -139,14 +139,14 @@ impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
|
||||
let changes = req.changes.clone();
|
||||
let guardian_review_id = ctx.guardian_review_id.clone();
|
||||
Box::pin(async move {
|
||||
if req.permissions_preapproved && retry_reason.is_none() {
|
||||
return ReviewDecision::Approved;
|
||||
}
|
||||
if let Some(review_id) = guardian_review_id {
|
||||
let action = ApplyPatchRuntime::build_guardian_review_request(req, ctx.call_id);
|
||||
return review_approval_request(session, turn, review_id, action, retry_reason)
|
||||
.await;
|
||||
}
|
||||
if req.permissions_preapproved && retry_reason.is_none() {
|
||||
return ReviewDecision::Approved;
|
||||
}
|
||||
if let Some(reason) = retry_reason {
|
||||
let rx_approve = session
|
||||
.request_patch_approval(
|
||||
|
||||
Reference in New Issue
Block a user