mirror of
https://github.com/openai/codex.git
synced 2026-11-03 16:23:51 +00:00
fix(guardian, app-server): introduce guardian review ids (#17298)
## Description This PR introduces `review_id` as the stable identifier for guardian reviews and exposes it in app-server `item/autoApprovalReview/started` and `item/autoApprovalReview/completed` events. Internally, guardian rejection state is now keyed by `review_id` instead of the reviewed tool item ID. `target_item_id` is still included when a review maps to a concrete thread item, but it is no longer overloaded as the review lifecycle identifier. ## Motivation We'd like to give users the ability to preempt a guardian review while it's running (approve or decline). However, we can't implement the API that allows the user to override a running guardian review because we didn't have a unique `review_id` per guardian review. Using `target_item_id` is not correct since: - with execve reviews, there can be multiple execve calls (and therefore guardian reviews) per shell command - with network policy reviews, there is no target item ID The PR that actually implements user overrides will use `review_id` as the stable identifier.
This commit is contained in:
1 parent
7999b0f60f
commit
a3be74143a
36 files changed
+577
-172
No files matched your search
@@ -2026,7 +2026,7 @@ impl Session {
|
||||
session_telemetry,
|
||||
models_manager: Arc::clone(&models_manager),
|
||||
tool_approvals: Mutex::new(ApprovalStore::default()),
|
||||
guardian_rejection_rationales: Mutex::new(HashMap::new()),
|
||||
guardian_rejections: Mutex::new(HashMap::new()),
|
||||
skills_manager,
|
||||
plugins_manager: Arc::clone(&plugins_manager),
|
||||
mcp_manager: Arc::clone(&mcp_manager),
|
||||
|
||||
@@ -39,6 +39,7 @@ use crate::codex::TurnContext;
|
||||
use crate::codex::emit_subagent_session_started;
|
||||
use crate::config::Config;
|
||||
use crate::guardian::GuardianApprovalRequest;
|
||||
use crate::guardian::new_guardian_review_id;
|
||||
use crate::guardian::review_approval_request_with_cancel;
|
||||
use crate::guardian::routes_approval_to_guardian;
|
||||
use crate::mcp_tool_call::MCP_TOOL_APPROVAL_ACCEPT;
|
||||
@@ -459,6 +460,7 @@ async fn handle_exec_approval(
|
||||
let review_rx = spawn_guardian_review(
|
||||
Arc::clone(parent_session),
|
||||
Arc::clone(parent_ctx),
|
||||
new_guardian_review_id(),
|
||||
GuardianApprovalRequest::Shell {
|
||||
id: call_id.clone(),
|
||||
command,
|
||||
@@ -566,6 +568,7 @@ async fn handle_patch_approval(
|
||||
let review_rx = spawn_guardian_review(
|
||||
Arc::clone(parent_session),
|
||||
Arc::clone(parent_ctx),
|
||||
new_guardian_review_id(),
|
||||
GuardianApprovalRequest::ApplyPatch {
|
||||
id: approval_id.clone(),
|
||||
cwd: parent_ctx.cwd.to_path_buf(),
|
||||
@@ -686,6 +689,7 @@ async fn maybe_auto_review_mcp_request_user_input(
|
||||
let review_rx = spawn_guardian_review(
|
||||
Arc::clone(parent_session),
|
||||
Arc::clone(parent_ctx),
|
||||
new_guardian_review_id(),
|
||||
build_guardian_mcp_tool_review_request(&event.call_id, &invocation, metadata.as_ref()),
|
||||
/*retry_reason*/ None,
|
||||
review_cancel.clone(),
|
||||
@@ -729,6 +733,7 @@ async fn maybe_auto_review_mcp_request_user_input(
|
||||
fn spawn_guardian_review(
|
||||
session: Arc<Session>,
|
||||
turn: Arc<TurnContext>,
|
||||
review_id: String,
|
||||
request: GuardianApprovalRequest,
|
||||
retry_reason: Option<String>,
|
||||
cancel_token: CancellationToken,
|
||||
@@ -745,6 +750,7 @@ fn spawn_guardian_review(
|
||||
let decision = runtime.block_on(review_approval_request_with_cancel(
|
||||
&session,
|
||||
&turn,
|
||||
review_id,
|
||||
request,
|
||||
retry_reason,
|
||||
cancel_token,
|
||||
|
||||
@@ -10,7 +10,6 @@ use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::ExecApprovalRequestEvent;
|
||||
use codex_protocol::protocol::GuardianAssessmentAction;
|
||||
use codex_protocol::protocol::GuardianAssessmentEvent;
|
||||
use codex_protocol::protocol::GuardianAssessmentStatus;
|
||||
use codex_protocol::protocol::GuardianCommandSource;
|
||||
use codex_protocol::protocol::McpInvocation;
|
||||
@@ -311,22 +310,26 @@ async fn handle_exec_approval_uses_call_id_for_guardian_review_and_approval_id_f
|
||||
})
|
||||
.await
|
||||
.expect("timed out waiting for guardian assessment");
|
||||
let expected_action = GuardianAssessmentAction::Command {
|
||||
source: GuardianCommandSource::Shell,
|
||||
command: "rm -rf tmp".to_string(),
|
||||
cwd: "/tmp".into(),
|
||||
};
|
||||
assert!(!assessment_event.id.is_empty());
|
||||
assert_eq!(
|
||||
assessment_event,
|
||||
GuardianAssessmentEvent {
|
||||
id: "command-item-1".to_string(),
|
||||
turn_id: parent_ctx.sub_id.clone(),
|
||||
status: GuardianAssessmentStatus::InProgress,
|
||||
risk_level: None,
|
||||
user_authorization: None,
|
||||
rationale: None,
|
||||
action: GuardianAssessmentAction::Command {
|
||||
source: GuardianCommandSource::Shell,
|
||||
command: "rm -rf tmp".to_string(),
|
||||
cwd: "/tmp".into(),
|
||||
},
|
||||
}
|
||||
assessment_event.target_item_id.as_deref(),
|
||||
Some("command-item-1")
|
||||
);
|
||||
assert_eq!(assessment_event.turn_id, parent_ctx.sub_id);
|
||||
assert_eq!(
|
||||
assessment_event.status,
|
||||
GuardianAssessmentStatus::InProgress
|
||||
);
|
||||
assert_eq!(assessment_event.risk_level, None);
|
||||
assert_eq!(assessment_event.user_authorization, None);
|
||||
assert_eq!(assessment_event.rationale, None);
|
||||
assert_eq!(assessment_event.decision_source, None);
|
||||
assert_eq!(assessment_event.action, expected_action);
|
||||
|
||||
cancel_token.cancel();
|
||||
|
||||
|
||||
@@ -2879,7 +2879,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) {
|
||||
session_telemetry: session_telemetry.clone(),
|
||||
models_manager: Arc::clone(&models_manager),
|
||||
tool_approvals: Mutex::new(ApprovalStore::default()),
|
||||
guardian_rejection_rationales: Mutex::new(std::collections::HashMap::new()),
|
||||
guardian_rejections: Mutex::new(std::collections::HashMap::new()),
|
||||
skills_manager,
|
||||
plugins_manager,
|
||||
mcp_manager,
|
||||
@@ -3724,7 +3724,7 @@ pub(crate) async fn make_session_and_context_with_dynamic_tools_and_rx(
|
||||
session_telemetry: session_telemetry.clone(),
|
||||
models_manager: Arc::clone(&models_manager),
|
||||
tool_approvals: Mutex::new(ApprovalStore::default()),
|
||||
guardian_rejection_rationales: Mutex::new(std::collections::HashMap::new()),
|
||||
guardian_rejections: Mutex::new(std::collections::HashMap::new()),
|
||||
skills_manager,
|
||||
plugins_manager,
|
||||
mcp_manager,
|
||||
|
||||
@@ -359,15 +359,15 @@ pub(crate) fn guardian_assessment_action(
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn guardian_request_id(request: &GuardianApprovalRequest) -> &str {
|
||||
pub(crate) fn guardian_request_target_item_id(request: &GuardianApprovalRequest) -> Option<&str> {
|
||||
match request {
|
||||
GuardianApprovalRequest::Shell { id, .. }
|
||||
| GuardianApprovalRequest::ExecCommand { id, .. }
|
||||
| GuardianApprovalRequest::ApplyPatch { id, .. }
|
||||
| GuardianApprovalRequest::NetworkAccess { id, .. }
|
||||
| GuardianApprovalRequest::McpToolCall { id, .. } => id,
|
||||
| GuardianApprovalRequest::McpToolCall { id, .. } => Some(id),
|
||||
GuardianApprovalRequest::NetworkAccess { .. } => None,
|
||||
#[cfg(unix)]
|
||||
GuardianApprovalRequest::Execve { id, .. } => id,
|
||||
GuardianApprovalRequest::Execve { id, .. } => Some(id),
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -18,6 +18,7 @@ mod review_session;
|
||||
|
||||
use std::time::Duration;
|
||||
|
||||
use codex_protocol::protocol::GuardianAssessmentDecisionSource;
|
||||
use serde::Deserialize;
|
||||
use serde::Serialize;
|
||||
|
||||
@@ -26,6 +27,7 @@ pub(crate) use approval_request::GuardianMcpAnnotations;
|
||||
pub(crate) use approval_request::guardian_approval_request_to_json;
|
||||
pub(crate) use review::guardian_rejection_message;
|
||||
pub(crate) use review::is_guardian_reviewer_source;
|
||||
pub(crate) use review::new_guardian_review_id;
|
||||
pub(crate) use review::review_approval_request;
|
||||
pub(crate) use review::review_approval_request_with_cancel;
|
||||
pub(crate) use review::routes_approval_to_guardian;
|
||||
@@ -59,6 +61,12 @@ pub(crate) struct GuardianAssessment {
|
||||
pub(crate) rationale: String,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub(crate) struct GuardianRejection {
|
||||
pub(crate) rationale: String,
|
||||
pub(crate) source: GuardianAssessmentDecisionSource,
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
use approval_request::format_guardian_action_pretty;
|
||||
#[cfg(test)]
|
||||
|
||||
@@ -3,6 +3,7 @@ use std::sync::Arc;
|
||||
use codex_protocol::config_types::ApprovalsReviewer;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::GuardianAssessmentDecisionSource;
|
||||
use codex_protocol::protocol::GuardianAssessmentEvent;
|
||||
use codex_protocol::protocol::GuardianAssessmentStatus;
|
||||
use codex_protocol::protocol::GuardianRiskLevel;
|
||||
@@ -19,8 +20,9 @@ use super::GUARDIAN_REVIEWER_NAME;
|
||||
use super::GuardianApprovalRequest;
|
||||
use super::GuardianAssessment;
|
||||
use super::GuardianAssessmentOutcome;
|
||||
use super::GuardianRejection;
|
||||
use super::approval_request::guardian_assessment_action;
|
||||
use super::approval_request::guardian_request_id;
|
||||
use super::approval_request::guardian_request_target_item_id;
|
||||
use super::approval_request::guardian_request_turn_id;
|
||||
use super::prompt::guardian_output_schema;
|
||||
use super::prompt::parse_guardian_assessment;
|
||||
@@ -36,20 +38,29 @@ const GUARDIAN_REJECTION_INSTRUCTIONS: &str = concat!(
|
||||
"Otherwise, stop and request user input.",
|
||||
);
|
||||
|
||||
pub(crate) async fn guardian_rejection_message(session: &Session, assessment_id: &str) -> String {
|
||||
let rationale = session
|
||||
pub(crate) fn new_guardian_review_id() -> String {
|
||||
uuid::Uuid::new_v4().to_string()
|
||||
}
|
||||
|
||||
pub(crate) async fn guardian_rejection_message(session: &Session, review_id: &str) -> String {
|
||||
let rejection = session
|
||||
.services
|
||||
.guardian_rejection_rationales
|
||||
.guardian_rejections
|
||||
.lock()
|
||||
.await
|
||||
.remove(assessment_id)
|
||||
.filter(|rationale| !rationale.trim().is_empty())
|
||||
.unwrap_or_else(|| "Guardian denied the action without a specific rationale.".to_string());
|
||||
format!(
|
||||
"This action was rejected due to unacceptable risk.\nReason: {}\n{}",
|
||||
rationale.trim(),
|
||||
GUARDIAN_REJECTION_INSTRUCTIONS
|
||||
)
|
||||
.remove(review_id)
|
||||
.filter(|rejection| !rejection.rationale.trim().is_empty())
|
||||
.unwrap_or_else(|| GuardianRejection {
|
||||
rationale: "Guardian denied the action without a specific rationale.".to_string(),
|
||||
source: GuardianAssessmentDecisionSource::Agent,
|
||||
});
|
||||
match rejection.source {
|
||||
GuardianAssessmentDecisionSource::Agent => format!(
|
||||
"This action was rejected due to unacceptable risk.\nReason: {}\n{}",
|
||||
rejection.rationale.trim(),
|
||||
GUARDIAN_REJECTION_INSTRUCTIONS
|
||||
),
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug)]
|
||||
@@ -91,23 +102,26 @@ pub(crate) fn is_guardian_reviewer_source(
|
||||
async fn run_guardian_review(
|
||||
session: Arc<Session>,
|
||||
turn: Arc<TurnContext>,
|
||||
review_id: String,
|
||||
request: GuardianApprovalRequest,
|
||||
retry_reason: Option<String>,
|
||||
external_cancel: Option<CancellationToken>,
|
||||
) -> ReviewDecision {
|
||||
let assessment_id = guardian_request_id(&request).to_string();
|
||||
let target_item_id = guardian_request_target_item_id(&request).map(str::to_string);
|
||||
let assessment_turn_id = guardian_request_turn_id(&request, &turn.sub_id).to_string();
|
||||
let action_summary = guardian_assessment_action(&request);
|
||||
session
|
||||
.send_event(
|
||||
turn.as_ref(),
|
||||
EventMsg::GuardianAssessment(GuardianAssessmentEvent {
|
||||
id: assessment_id.clone(),
|
||||
id: review_id.clone(),
|
||||
target_item_id: target_item_id.clone(),
|
||||
turn_id: assessment_turn_id.clone(),
|
||||
status: GuardianAssessmentStatus::InProgress,
|
||||
risk_level: None,
|
||||
user_authorization: None,
|
||||
rationale: None,
|
||||
decision_source: None,
|
||||
action: action_summary.clone(),
|
||||
}),
|
||||
)
|
||||
@@ -121,12 +135,14 @@ async fn run_guardian_review(
|
||||
.send_event(
|
||||
turn.as_ref(),
|
||||
EventMsg::GuardianAssessment(GuardianAssessmentEvent {
|
||||
id: assessment_id,
|
||||
id: review_id,
|
||||
target_item_id,
|
||||
turn_id: assessment_turn_id,
|
||||
status: GuardianAssessmentStatus::Aborted,
|
||||
risk_level: None,
|
||||
user_authorization: None,
|
||||
rationale: None,
|
||||
decision_source: Some(GuardianAssessmentDecisionSource::Agent),
|
||||
action: action_summary,
|
||||
}),
|
||||
)
|
||||
@@ -167,12 +183,14 @@ async fn run_guardian_review(
|
||||
.send_event(
|
||||
turn.as_ref(),
|
||||
EventMsg::GuardianAssessment(GuardianAssessmentEvent {
|
||||
id: assessment_id,
|
||||
id: review_id,
|
||||
target_item_id,
|
||||
turn_id: assessment_turn_id,
|
||||
status: GuardianAssessmentStatus::Aborted,
|
||||
risk_level: None,
|
||||
user_authorization: None,
|
||||
rationale: None,
|
||||
decision_source: Some(GuardianAssessmentDecisionSource::Agent),
|
||||
action: action_summary,
|
||||
}),
|
||||
)
|
||||
@@ -209,23 +227,29 @@ async fn run_guardian_review(
|
||||
GuardianAssessmentStatus::Denied
|
||||
};
|
||||
{
|
||||
let mut rationales = session.services.guardian_rejection_rationales.lock().await;
|
||||
let mut rationales = session.services.guardian_rejections.lock().await;
|
||||
if approved {
|
||||
rationales.remove(&assessment_id);
|
||||
rationales.remove(&review_id);
|
||||
} else {
|
||||
rationales.insert(assessment_id.clone(), assessment.rationale.clone());
|
||||
let rejection = GuardianRejection {
|
||||
rationale: assessment.rationale.clone(),
|
||||
source: GuardianAssessmentDecisionSource::Agent,
|
||||
};
|
||||
rationales.insert(review_id.clone(), rejection);
|
||||
}
|
||||
}
|
||||
session
|
||||
.send_event(
|
||||
turn.as_ref(),
|
||||
EventMsg::GuardianAssessment(GuardianAssessmentEvent {
|
||||
id: assessment_id,
|
||||
id: review_id,
|
||||
target_item_id,
|
||||
turn_id: assessment_turn_id,
|
||||
status,
|
||||
risk_level: Some(assessment.risk_level),
|
||||
user_authorization: Some(assessment.user_authorization),
|
||||
rationale: Some(assessment.rationale.clone()),
|
||||
decision_source: Some(GuardianAssessmentDecisionSource::Agent),
|
||||
action: terminal_action,
|
||||
}),
|
||||
)
|
||||
@@ -242,12 +266,14 @@ async fn run_guardian_review(
|
||||
pub(crate) async fn review_approval_request(
|
||||
session: &Arc<Session>,
|
||||
turn: &Arc<TurnContext>,
|
||||
review_id: String,
|
||||
request: GuardianApprovalRequest,
|
||||
retry_reason: Option<String>,
|
||||
) -> ReviewDecision {
|
||||
run_guardian_review(
|
||||
Arc::clone(session),
|
||||
Arc::clone(turn),
|
||||
review_id,
|
||||
request,
|
||||
retry_reason,
|
||||
/*external_cancel*/ None,
|
||||
@@ -258,6 +284,7 @@ pub(crate) async fn review_approval_request(
|
||||
pub(crate) async fn review_approval_request_with_cancel(
|
||||
session: &Arc<Session>,
|
||||
turn: &Arc<TurnContext>,
|
||||
review_id: String,
|
||||
request: GuardianApprovalRequest,
|
||||
retry_reason: Option<String>,
|
||||
cancel_token: CancellationToken,
|
||||
@@ -265,6 +292,7 @@ pub(crate) async fn review_approval_request_with_cancel(
|
||||
run_guardian_review(
|
||||
Arc::clone(session),
|
||||
Arc::clone(turn),
|
||||
review_id,
|
||||
request,
|
||||
retry_reason,
|
||||
Some(cancel_token),
|
||||
|
||||
@@ -683,6 +683,7 @@ async fn cancelled_guardian_review_emits_terminal_abort_without_warning() {
|
||||
let decision = review_approval_request_with_cancel(
|
||||
&session,
|
||||
&turn,
|
||||
"review-cancelled-guardian".to_string(),
|
||||
GuardianApprovalRequest::ApplyPatch {
|
||||
id: "patch-1".to_string(),
|
||||
cwd: PathBuf::from("/tmp"),
|
||||
@@ -1248,6 +1249,7 @@ async fn guardian_review_surfaces_responses_api_errors_in_rejection_reason() ->
|
||||
let decision = review_approval_request(
|
||||
&session,
|
||||
&turn,
|
||||
"review-shell-guardian-error".to_string(),
|
||||
GuardianApprovalRequest::Shell {
|
||||
id: "shell-guardian-error".to_string(),
|
||||
command: vec!["git".to_string(), "push".to_string()],
|
||||
@@ -1295,8 +1297,13 @@ async fn guardian_review_surfaces_responses_api_errors_in_rejection_reason() ->
|
||||
}),
|
||||
"denial rationale should not fall back to the generic missing payload error"
|
||||
);
|
||||
{
|
||||
let rationales = session.services.guardian_rejections.lock().await;
|
||||
assert!(rationales.contains_key("review-shell-guardian-error"));
|
||||
assert!(!rationales.contains_key("shell-guardian-error"));
|
||||
}
|
||||
let rejection_message =
|
||||
guardian_rejection_message(session.as_ref(), "shell-guardian-error").await;
|
||||
guardian_rejection_message(session.as_ref(), "review-shell-guardian-error").await;
|
||||
assert!(
|
||||
rejection_message.contains("Reason: Automatic approval review failed:")
|
||||
&& rejection_message.contains(error_message),
|
||||
@@ -1375,7 +1382,14 @@ async fn guardian_parallel_reviews_fork_from_last_committed_trunk_history() -> a
|
||||
justification: Some("Inspect repo state before proceeding.".to_string()),
|
||||
};
|
||||
assert_eq!(
|
||||
review_approval_request(&session, &turn, initial_request, /*retry_reason*/ None).await,
|
||||
review_approval_request(
|
||||
&session,
|
||||
&turn,
|
||||
"review-shell-guardian-1".to_string(),
|
||||
initial_request,
|
||||
/*retry_reason*/ None
|
||||
)
|
||||
.await,
|
||||
ReviewDecision::Approved
|
||||
);
|
||||
session
|
||||
@@ -1427,6 +1441,7 @@ async fn guardian_parallel_reviews_fork_from_last_committed_trunk_history() -> a
|
||||
review_approval_request(
|
||||
&session_for_second,
|
||||
&turn_for_second,
|
||||
"review-shell-guardian-2".to_string(),
|
||||
second_request,
|
||||
Some("trunk follow-up".to_string()),
|
||||
)
|
||||
@@ -1475,6 +1490,7 @@ async fn guardian_parallel_reviews_fork_from_last_committed_trunk_history() -> a
|
||||
let third_decision = review_approval_request(
|
||||
&session,
|
||||
&turn,
|
||||
"review-shell-guardian-3".to_string(),
|
||||
third_request,
|
||||
Some("parallel follow-up".to_string()),
|
||||
)
|
||||
|
||||
@@ -24,6 +24,7 @@ use crate::guardian::GuardianApprovalRequest;
|
||||
use crate::guardian::GuardianMcpAnnotations;
|
||||
use crate::guardian::guardian_approval_request_to_json;
|
||||
use crate::guardian::guardian_rejection_message;
|
||||
use crate::guardian::new_guardian_review_id;
|
||||
use crate::guardian::review_approval_request;
|
||||
use crate::guardian::routes_approval_to_guardian;
|
||||
use crate::mcp_openai_file::rewrite_mcp_tool_arguments_for_openai_files;
|
||||
@@ -772,14 +773,16 @@ async fn maybe_request_mcp_tool_approval(
|
||||
.enabled(Feature::ToolCallMcpElicitation);
|
||||
|
||||
if routes_approval_to_guardian(turn_context) {
|
||||
let review_id = new_guardian_review_id();
|
||||
let decision = review_approval_request(
|
||||
sess,
|
||||
turn_context,
|
||||
review_id.clone(),
|
||||
build_guardian_mcp_tool_review_request(call_id, invocation, metadata),
|
||||
monitor_reason.clone(),
|
||||
)
|
||||
.await;
|
||||
let decision = mcp_tool_approval_decision_from_guardian(sess, call_id, decision).await;
|
||||
let decision = mcp_tool_approval_decision_from_guardian(sess, &review_id, decision).await;
|
||||
apply_mcp_tool_approval_decision(
|
||||
sess,
|
||||
turn_context,
|
||||
@@ -969,7 +972,7 @@ pub(crate) fn build_guardian_mcp_tool_review_request(
|
||||
|
||||
async fn mcp_tool_approval_decision_from_guardian(
|
||||
sess: &Session,
|
||||
call_id: &str,
|
||||
review_id: &str,
|
||||
decision: ReviewDecision,
|
||||
) -> McpToolApprovalDecision {
|
||||
match decision {
|
||||
@@ -978,7 +981,7 @@ async fn mcp_tool_approval_decision_from_guardian(
|
||||
| ReviewDecision::NetworkPolicyAmendment { .. } => McpToolApprovalDecision::Accept,
|
||||
ReviewDecision::ApprovedForSession => McpToolApprovalDecision::AcceptForSession,
|
||||
ReviewDecision::Denied => McpToolApprovalDecision::Decline {
|
||||
message: Some(guardian_rejection_message(sess, call_id).await),
|
||||
message: Some(guardian_rejection_message(sess, review_id).await),
|
||||
},
|
||||
ReviewDecision::Abort => McpToolApprovalDecision::Decline { message: None },
|
||||
}
|
||||
|
||||
@@ -816,21 +816,22 @@ async fn guardian_review_decision_maps_to_mcp_tool_decision() {
|
||||
assert_eq!(
|
||||
mcp_tool_approval_decision_from_guardian(
|
||||
session.as_ref(),
|
||||
"approval-id",
|
||||
"review-id",
|
||||
ReviewDecision::Approved
|
||||
)
|
||||
.await,
|
||||
McpToolApprovalDecision::Accept
|
||||
);
|
||||
session
|
||||
.services
|
||||
.guardian_rejection_rationales
|
||||
.lock()
|
||||
.await
|
||||
.insert("approval-id".to_string(), "too risky".to_string());
|
||||
session.services.guardian_rejections.lock().await.insert(
|
||||
"review-id".to_string(),
|
||||
crate::guardian::GuardianRejection {
|
||||
rationale: "too risky".to_string(),
|
||||
source: codex_protocol::protocol::GuardianAssessmentDecisionSource::Agent,
|
||||
},
|
||||
);
|
||||
let denial = mcp_tool_approval_decision_from_guardian(
|
||||
session.as_ref(),
|
||||
"approval-id",
|
||||
"review-id",
|
||||
ReviewDecision::Denied,
|
||||
)
|
||||
.await;
|
||||
@@ -845,7 +846,7 @@ async fn guardian_review_decision_maps_to_mcp_tool_decision() {
|
||||
assert_eq!(
|
||||
mcp_tool_approval_decision_from_guardian(
|
||||
session.as_ref(),
|
||||
"approval-id",
|
||||
"review-id",
|
||||
ReviewDecision::Abort
|
||||
)
|
||||
.await,
|
||||
|
||||
@@ -7,6 +7,7 @@ use crate::agent::AgentControl;
|
||||
use crate::client::ModelClient;
|
||||
use crate::config::StartedNetworkProxy;
|
||||
use crate::exec_policy::ExecPolicyManager;
|
||||
use crate::guardian::GuardianRejection;
|
||||
use crate::mcp::McpManager;
|
||||
use crate::plugins::PluginsManager;
|
||||
use crate::skills_watcher::SkillsWatcher;
|
||||
@@ -47,7 +48,7 @@ pub(crate) struct SessionServices {
|
||||
pub(crate) models_manager: Arc<ModelsManager>,
|
||||
pub(crate) session_telemetry: SessionTelemetry,
|
||||
pub(crate) tool_approvals: Mutex<ApprovalStore>,
|
||||
pub(crate) guardian_rejection_rationales: Mutex<HashMap<String, String>>,
|
||||
pub(crate) guardian_rejections: Mutex<HashMap<String, GuardianRejection>>,
|
||||
pub(crate) skills_manager: Arc<SkillsManager>,
|
||||
pub(crate) plugins_manager: Arc<PluginsManager>,
|
||||
pub(crate) mcp_manager: Arc<McpManager>,
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
use crate::codex::Session;
|
||||
use crate::guardian::GuardianApprovalRequest;
|
||||
use crate::guardian::guardian_rejection_message;
|
||||
use crate::guardian::new_guardian_review_id;
|
||||
use crate::guardian::review_approval_request;
|
||||
use crate::guardian::routes_approval_to_guardian;
|
||||
use crate::network_policy_decision::denied_network_policy_message;
|
||||
@@ -369,12 +370,13 @@ impl NetworkApprovalService {
|
||||
};
|
||||
let owner_call = self.resolve_single_active_call().await;
|
||||
let guardian_approval_id = Self::approval_id_for_key(&key);
|
||||
let approval_decision = if routes_approval_to_guardian(&turn_context) {
|
||||
// TODO(ccunningham): Attach guardian network reviews to the reviewed tool item
|
||||
// lifecycle instead of this temporary standalone network approval id.
|
||||
let use_guardian = routes_approval_to_guardian(&turn_context);
|
||||
let guardian_review_id = use_guardian.then(new_guardian_review_id);
|
||||
let approval_decision = if let Some(review_id) = guardian_review_id.clone() {
|
||||
review_approval_request(
|
||||
&session,
|
||||
&turn_context,
|
||||
review_id,
|
||||
GuardianApprovalRequest::NetworkAccess {
|
||||
id: guardian_approval_id.clone(),
|
||||
turn_id: owner_call
|
||||
@@ -487,11 +489,9 @@ impl NetworkApprovalService {
|
||||
}
|
||||
},
|
||||
ReviewDecision::Denied | ReviewDecision::Abort => {
|
||||
if routes_approval_to_guardian(&turn_context) {
|
||||
if let Some(review_id) = guardian_review_id.as_deref() {
|
||||
if let Some(owner_call) = owner_call.as_ref() {
|
||||
let message =
|
||||
guardian_rejection_message(session.as_ref(), &guardian_approval_id)
|
||||
.await;
|
||||
let message = guardian_rejection_message(session.as_ref(), review_id).await;
|
||||
self.record_call_outcome(
|
||||
&owner_call.registration_id,
|
||||
NetworkApprovalOutcome::DeniedByPolicy(message),
|
||||
|
||||
@@ -7,6 +7,7 @@ retry with an escalated sandbox strategy on denial (no re‑approval thanks to
|
||||
caching).
|
||||
*/
|
||||
use crate::guardian::guardian_rejection_message;
|
||||
use crate::guardian::new_guardian_review_id;
|
||||
use crate::guardian::routes_approval_to_guardian;
|
||||
use crate::network_policy_decision::network_approval_context_from_payload;
|
||||
use crate::tools::network_approval::DeferredNetworkApproval;
|
||||
@@ -115,6 +116,7 @@ impl ToolOrchestrator {
|
||||
let otel_user = ToolDecisionSource::User;
|
||||
let otel_automated_reviewer = ToolDecisionSource::AutomatedReviewer;
|
||||
let otel_cfg = ToolDecisionSource::Config;
|
||||
let use_guardian = routes_approval_to_guardian(turn_ctx);
|
||||
|
||||
// 1) Approval
|
||||
let mut already_approved = false;
|
||||
@@ -130,15 +132,17 @@ impl ToolOrchestrator {
|
||||
return Err(ToolError::Rejected(reason));
|
||||
}
|
||||
ExecApprovalRequirement::NeedsApproval { reason, .. } => {
|
||||
let guardian_review_id = use_guardian.then(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: reason,
|
||||
network_approval_context: None,
|
||||
};
|
||||
let decision = tool.start_approval_async(req, approval_ctx).await;
|
||||
let otel_source = if routes_approval_to_guardian(turn_ctx) {
|
||||
let otel_source = if use_guardian {
|
||||
otel_automated_reviewer.clone()
|
||||
} else {
|
||||
otel_user.clone()
|
||||
@@ -148,9 +152,8 @@ impl ToolOrchestrator {
|
||||
|
||||
match decision {
|
||||
ReviewDecision::Denied | ReviewDecision::Abort => {
|
||||
let reason = if routes_approval_to_guardian(turn_ctx) {
|
||||
guardian_rejection_message(tool_ctx.session.as_ref(), &tool_ctx.call_id)
|
||||
.await
|
||||
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()
|
||||
};
|
||||
@@ -284,16 +287,18 @@ impl ToolOrchestrator {
|
||||
.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);
|
||||
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: Some(retry_reason),
|
||||
network_approval_context: network_approval_context.clone(),
|
||||
};
|
||||
|
||||
let decision = tool.start_approval_async(req, approval_ctx).await;
|
||||
let otel_source = if routes_approval_to_guardian(turn_ctx) {
|
||||
let otel_source = if use_guardian {
|
||||
otel_automated_reviewer
|
||||
} else {
|
||||
otel_user
|
||||
@@ -302,12 +307,9 @@ impl ToolOrchestrator {
|
||||
|
||||
match decision {
|
||||
ReviewDecision::Denied | ReviewDecision::Abort => {
|
||||
let reason = if routes_approval_to_guardian(turn_ctx) {
|
||||
guardian_rejection_message(
|
||||
tool_ctx.session.as_ref(),
|
||||
&tool_ctx.call_id,
|
||||
)
|
||||
.await
|
||||
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()
|
||||
};
|
||||
|
||||
@@ -8,7 +8,6 @@
|
||||
use crate::exec::ExecCapturePolicy;
|
||||
use crate::guardian::GuardianApprovalRequest;
|
||||
use crate::guardian::review_approval_request;
|
||||
use crate::guardian::routes_approval_to_guardian;
|
||||
use crate::sandboxing::ExecOptions;
|
||||
use crate::sandboxing::execute_env;
|
||||
use crate::tools::sandboxing::Approvable;
|
||||
@@ -147,13 +146,15 @@ impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
|
||||
let retry_reason = ctx.retry_reason.clone();
|
||||
let approval_keys = self.approval_keys(req);
|
||||
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 routes_approval_to_guardian(turn) {
|
||||
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, action, retry_reason).await;
|
||||
return review_approval_request(session, turn, review_id, action, retry_reason)
|
||||
.await;
|
||||
}
|
||||
if let Some(reason) = retry_reason {
|
||||
let rx_approve = session
|
||||
|
||||
@@ -12,7 +12,6 @@ use crate::command_canonicalization::canonicalize_command_for_approval;
|
||||
use crate::exec::ExecCapturePolicy;
|
||||
use crate::guardian::GuardianApprovalRequest;
|
||||
use crate::guardian::review_approval_request;
|
||||
use crate::guardian::routes_approval_to_guardian;
|
||||
use crate::sandboxing::ExecOptions;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
use crate::sandboxing::execute_env;
|
||||
@@ -152,11 +151,13 @@ impl Approvable<ShellRequest> for ShellRuntime {
|
||||
let session = ctx.session;
|
||||
let turn = ctx.turn;
|
||||
let call_id = ctx.call_id.to_string();
|
||||
let guardian_review_id = ctx.guardian_review_id.clone();
|
||||
Box::pin(async move {
|
||||
if routes_approval_to_guardian(turn) {
|
||||
if let Some(review_id) = guardian_review_id {
|
||||
return review_approval_request(
|
||||
session,
|
||||
turn,
|
||||
review_id,
|
||||
GuardianApprovalRequest::Shell {
|
||||
id: call_id,
|
||||
command,
|
||||
|
||||
@@ -4,6 +4,7 @@ use crate::exec::ExecExpiration;
|
||||
use crate::exec::is_likely_sandbox_denied;
|
||||
use crate::guardian::GuardianApprovalRequest;
|
||||
use crate::guardian::guardian_rejection_message;
|
||||
use crate::guardian::new_guardian_review_id;
|
||||
use crate::guardian::review_approval_request;
|
||||
use crate::guardian::routes_approval_to_guardian;
|
||||
use crate::sandboxing::ExecOptions;
|
||||
@@ -315,6 +316,11 @@ enum DecisionSource {
|
||||
UnmatchedCommandFallback,
|
||||
}
|
||||
|
||||
struct PromptDecision {
|
||||
decision: ReviewDecision,
|
||||
guardian_review_id: Option<String>,
|
||||
}
|
||||
|
||||
fn execve_prompt_is_rejected_by_policy(
|
||||
approval_policy: AskForApproval,
|
||||
decision_source: &DecisionSource,
|
||||
@@ -376,7 +382,7 @@ impl CoreShellActionProvider {
|
||||
workdir: &AbsolutePathBuf,
|
||||
stopwatch: &Stopwatch,
|
||||
additional_permissions: Option<PermissionProfile>,
|
||||
) -> anyhow::Result<ReviewDecision> {
|
||||
) -> anyhow::Result<PromptDecision> {
|
||||
let command = join_program_and_argv(program, argv);
|
||||
let workdir = workdir.to_path_buf();
|
||||
let session = self.session.clone();
|
||||
@@ -384,12 +390,14 @@ impl CoreShellActionProvider {
|
||||
let call_id = self.call_id.clone();
|
||||
let approval_id = Some(Uuid::new_v4().to_string());
|
||||
let source = self.tool_name;
|
||||
let guardian_review_id = routes_approval_to_guardian(&turn).then(new_guardian_review_id);
|
||||
Ok(stopwatch
|
||||
.pause_for(async move {
|
||||
if routes_approval_to_guardian(&turn) {
|
||||
return review_approval_request(
|
||||
if let Some(review_id) = guardian_review_id.clone() {
|
||||
let decision = review_approval_request(
|
||||
&session,
|
||||
&turn,
|
||||
review_id,
|
||||
GuardianApprovalRequest::Execve {
|
||||
id: call_id.clone(),
|
||||
source,
|
||||
@@ -401,8 +409,12 @@ impl CoreShellActionProvider {
|
||||
/*retry_reason*/ None,
|
||||
)
|
||||
.await;
|
||||
return PromptDecision {
|
||||
decision,
|
||||
guardian_review_id,
|
||||
};
|
||||
}
|
||||
session
|
||||
let decision = session
|
||||
.request_command_approval(
|
||||
&turn,
|
||||
call_id,
|
||||
@@ -415,7 +427,11 @@ impl CoreShellActionProvider {
|
||||
additional_permissions,
|
||||
Some(vec![ReviewDecision::Approved, ReviewDecision::Abort]),
|
||||
)
|
||||
.await
|
||||
.await;
|
||||
PromptDecision {
|
||||
decision,
|
||||
guardian_review_id: None,
|
||||
}
|
||||
})
|
||||
.await)
|
||||
}
|
||||
@@ -442,10 +458,10 @@ impl CoreShellActionProvider {
|
||||
{
|
||||
EscalationDecision::deny(Some("Execution forbidden by policy".to_string()))
|
||||
} else {
|
||||
match self
|
||||
let prompt_decision = self
|
||||
.prompt(program, argv, workdir, &self.stopwatch, prompt_permissions)
|
||||
.await?
|
||||
{
|
||||
.await?;
|
||||
match prompt_decision.decision {
|
||||
ReviewDecision::Approved
|
||||
| ReviewDecision::ApprovedForSession
|
||||
| ReviewDecision::ApprovedExecpolicyAmendment { .. } => {
|
||||
@@ -470,9 +486,10 @@ impl CoreShellActionProvider {
|
||||
}
|
||||
},
|
||||
ReviewDecision::Denied => {
|
||||
let message = if routes_approval_to_guardian(&self.turn) {
|
||||
guardian_rejection_message(self.session.as_ref(), &self.call_id)
|
||||
.await
|
||||
let message = if let Some(review_id) =
|
||||
prompt_decision.guardian_review_id.as_deref()
|
||||
{
|
||||
guardian_rejection_message(self.session.as_ref(), review_id).await
|
||||
} else {
|
||||
"User denied execution".to_string()
|
||||
};
|
||||
|
||||
@@ -9,7 +9,6 @@ use crate::exec::ExecCapturePolicy;
|
||||
use crate::exec::ExecExpiration;
|
||||
use crate::guardian::GuardianApprovalRequest;
|
||||
use crate::guardian::review_approval_request;
|
||||
use crate::guardian::routes_approval_to_guardian;
|
||||
use crate::sandboxing::ExecOptions;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
use crate::shell::ShellType;
|
||||
@@ -128,11 +127,13 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
|
||||
let cwd = req.cwd.to_path_buf();
|
||||
let retry_reason = ctx.retry_reason.clone();
|
||||
let reason = retry_reason.clone().or_else(|| req.justification.clone());
|
||||
let guardian_review_id = ctx.guardian_review_id.clone();
|
||||
Box::pin(async move {
|
||||
if routes_approval_to_guardian(turn) {
|
||||
if let Some(review_id) = guardian_review_id {
|
||||
return review_approval_request(
|
||||
session,
|
||||
turn,
|
||||
review_id,
|
||||
GuardianApprovalRequest::ExecCommand {
|
||||
id: call_id,
|
||||
command,
|
||||
|
||||
@@ -120,6 +120,13 @@ pub(crate) struct ApprovalCtx<'a> {
|
||||
pub session: &'a Arc<Session>,
|
||||
pub turn: &'a Arc<TurnContext>,
|
||||
pub call_id: &'a str,
|
||||
/// Guardian review lifecycle ID for this approval, when guardian is reviewing it.
|
||||
///
|
||||
/// This is separate from `call_id`: `call_id` identifies the tool item under
|
||||
/// review, while this ID identifies the review itself. Keeping both lets
|
||||
/// denial handling, overrides, and app-server notifications refer to the
|
||||
/// review without overloading the tool call ID as a review ID.
|
||||
pub guardian_review_id: Option<String>,
|
||||
pub retry_reason: Option<String>,
|
||||
pub network_approval_context: Option<NetworkApprovalContext>,
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user