mirror of
https://github.com/openai/codex.git
synced 2026-10-26 18:38:47 +00:00
Clarify Guardian fail-closed wording
This commit is contained in:
@@ -72,6 +72,13 @@ pub(crate) struct GuardianAssessment {
|
||||
pub(crate) struct GuardianRejection {
|
||||
pub(crate) rationale: String,
|
||||
pub(crate) source: GuardianAssessmentDecisionSource,
|
||||
pub(crate) kind: GuardianRejectionKind,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub(crate) enum GuardianRejectionKind {
|
||||
PolicyDenial,
|
||||
ReviewFailure,
|
||||
}
|
||||
|
||||
#[derive(Debug, Default)]
|
||||
|
||||
@@ -33,6 +33,7 @@ use super::GuardianAssessment;
|
||||
use super::GuardianAssessmentOutcome;
|
||||
use super::GuardianRejection;
|
||||
use super::GuardianRejectionCircuitBreakerAction;
|
||||
use super::GuardianRejectionKind;
|
||||
use super::approval_request::guardian_assessment_action;
|
||||
use super::approval_request::guardian_request_target_item_id;
|
||||
use super::approval_request::guardian_request_turn_id;
|
||||
@@ -73,13 +74,20 @@ pub(crate) async fn guardian_rejection_message(session: &Session, review_id: &st
|
||||
.unwrap_or_else(|| GuardianRejection {
|
||||
rationale: "Auto-reviewer denied the action without a specific rationale.".to_string(),
|
||||
source: GuardianAssessmentDecisionSource::Agent,
|
||||
kind: GuardianRejectionKind::PolicyDenial,
|
||||
});
|
||||
match rejection.source {
|
||||
GuardianAssessmentDecisionSource::Agent => format!(
|
||||
"This action was rejected due to unacceptable risk.\nReason: {}\n{}",
|
||||
rejection.rationale.trim(),
|
||||
GUARDIAN_REJECTION_INSTRUCTIONS
|
||||
),
|
||||
GuardianAssessmentDecisionSource::Agent => {
|
||||
let rationale = rejection.rationale.trim();
|
||||
match rejection.kind {
|
||||
GuardianRejectionKind::PolicyDenial => format!(
|
||||
"This action was rejected due to unacceptable risk.\nReason: {rationale}\n{GUARDIAN_REJECTION_INSTRUCTIONS}"
|
||||
),
|
||||
GuardianRejectionKind::ReviewFailure => format!(
|
||||
"Automatic approval review failed, so this action was not run.\nReason: {rationale}\n{GUARDIAN_REJECTION_INSTRUCTIONS}"
|
||||
),
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -346,7 +354,7 @@ async fn run_guardian_review(
|
||||
.await;
|
||||
|
||||
let completed_at_ms = now_unix_timestamp_ms();
|
||||
let (assessment, count_denial_for_circuit_breaker) = match outcome {
|
||||
let (assessment, count_denial_for_circuit_breaker, rejection_kind) = match outcome {
|
||||
GuardianReviewOutcome::Completed(assessment) => {
|
||||
let approved = matches!(assessment.outcome, GuardianAssessmentOutcome::Allow);
|
||||
track_guardian_review(
|
||||
@@ -375,7 +383,11 @@ async fn run_guardian_review(
|
||||
);
|
||||
let count_denial_for_circuit_breaker =
|
||||
matches!(assessment.outcome, GuardianAssessmentOutcome::Deny);
|
||||
(assessment, count_denial_for_circuit_breaker)
|
||||
(
|
||||
assessment,
|
||||
count_denial_for_circuit_breaker,
|
||||
GuardianRejectionKind::PolicyDenial,
|
||||
)
|
||||
}
|
||||
GuardianReviewOutcome::Error(error) => match error {
|
||||
GuardianReviewError::Timeout => {
|
||||
@@ -492,6 +504,7 @@ async fn run_guardian_review(
|
||||
rationale,
|
||||
},
|
||||
false,
|
||||
GuardianRejectionKind::ReviewFailure,
|
||||
)
|
||||
}
|
||||
},
|
||||
@@ -532,6 +545,7 @@ async fn run_guardian_review(
|
||||
let rejection = GuardianRejection {
|
||||
rationale: assessment.rationale.clone(),
|
||||
source: GuardianAssessmentDecisionSource::Agent,
|
||||
kind: rejection_kind,
|
||||
};
|
||||
rationales.insert(review_id.clone(), rejection);
|
||||
}
|
||||
|
||||
@@ -2072,10 +2072,59 @@ async fn guardian_review_surfaces_responses_api_errors_in_rejection_reason() ->
|
||||
let rejection_message =
|
||||
guardian_rejection_message(session.as_ref(), "review-shell-guardian-error").await;
|
||||
assert!(
|
||||
rejection_message.contains("Reason: Automatic approval review failed:")
|
||||
rejection_message.contains("Automatic approval review failed, so this action was not run.")
|
||||
&& rejection_message.contains("Reason: Automatic approval review failed:")
|
||||
&& rejection_message.contains(error_message),
|
||||
"rejection message should include guardian rationale: {rejection_message}"
|
||||
);
|
||||
assert!(!rejection_message.contains("rejected due to unacceptable risk"));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn guardian_review_completed_denial_uses_policy_rejection_message() -> anyhow::Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let _request_log = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_response_created("resp-guardian-deny"),
|
||||
ev_assistant_message(
|
||||
"msg-guardian-deny",
|
||||
"{\"risk_level\":\"high\",\"user_authorization\":\"low\",\"outcome\":\"deny\",\"rationale\":\"Pushing without review is too risky.\"}",
|
||||
),
|
||||
ev_completed("resp-guardian-deny"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
let (session, turn) = guardian_test_session_and_turn(&server).await;
|
||||
seed_guardian_parent_history(&session, &turn).await;
|
||||
|
||||
let decision = review_approval_request(
|
||||
&session,
|
||||
&turn,
|
||||
"review-shell-guardian-deny".to_string(),
|
||||
GuardianApprovalRequest::Shell {
|
||||
id: "shell-guardian-deny".to_string(),
|
||||
command: vec!["git".to_string(), "push".to_string()],
|
||||
cwd: test_path_buf("/repo/codex-rs/core").abs(),
|
||||
sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault,
|
||||
additional_permissions: None,
|
||||
justification: Some("Need to push the reviewed docs fix.".to_string()),
|
||||
},
|
||||
/*retry_reason*/ None,
|
||||
)
|
||||
.await;
|
||||
|
||||
assert_eq!(decision, ReviewDecision::Denied);
|
||||
let rejection_message =
|
||||
guardian_rejection_message(session.as_ref(), "review-shell-guardian-deny").await;
|
||||
assert!(rejection_message.contains("This action was rejected due to unacceptable risk."));
|
||||
assert!(rejection_message.contains("Reason: Pushing without review is too risky."));
|
||||
assert!(!rejection_message.contains("Automatic approval review failed"));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -1668,6 +1668,7 @@ async fn guardian_review_decision_maps_to_mcp_tool_decision() {
|
||||
crate::guardian::GuardianRejection {
|
||||
rationale: "too risky".to_string(),
|
||||
source: codex_protocol::protocol::GuardianAssessmentDecisionSource::Agent,
|
||||
kind: crate::guardian::GuardianRejectionKind::PolicyDenial,
|
||||
},
|
||||
);
|
||||
let denial = mcp_tool_approval_decision_from_guardian(
|
||||
|
||||
Reference in New Issue
Block a user