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

Allow managed Guardian failures to fail open

Browse Source
This commit is contained in:
Ray Morgan
2026-06-10 01:02:55 -07:00
parent dfc6fd55b3
commit aa5ef0e820
25 changed files with 687 additions and 99 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/analytics/src/events.rs
+3
View File
@@ -192,6 +192,7 @@ pub enum GuardianReviewTerminalStatus {
Aborted,
TimedOut,
FailedClosed,
FailedOpen,
}
#[derive(Clone, Copy, Debug, Serialize)]
@@ -444,6 +445,7 @@ pub(crate) enum FinalApprovalOutcome {
GuardianApproved,
GuardianDenied,
GuardianAborted,
GuardianFailedOpen,
UserApproved,
UserApprovedForSession,
UserDenied,
@@ -537,6 +539,7 @@ pub(crate) enum ReviewStatus {
Denied,
Aborted,
TimedOut,
FailedOpen,
}
#[allow(dead_code)]
codex-rs/app-server-protocol/schema/json/ServerNotification.json
Generated
+2 -1
View File
@@ -538,7 +538,8 @@
"AutoReviewDecisionSource": {
"description": "[UNSTABLE] Source that produced a terminal approval auto-review decision.",
"enum": [
"agent"
"agent",
"organizationPolicy"
],
"type": "string"
},
codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json
Generated
+8 -1
View File
@@ -6709,7 +6709,8 @@
"AutoReviewDecisionSource": {
"description": "[UNSTABLE] Source that produced a terminal approval auto-review decision.",
"enum": [
"agent"
"agent",
"organizationPolicy"
],
"type": "string"
},
@@ -8035,6 +8036,12 @@
"null"
]
},
"allowGuardianFailOpen": {
"type": [
"boolean",
"null"
]
},
"allowManagedHooksOnly": {
"type": [
"boolean",
codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json
Generated
+8 -1
View File
@@ -1031,7 +1031,8 @@
"AutoReviewDecisionSource": {
"description": "[UNSTABLE] Source that produced a terminal approval auto-review decision.",
"enum": [
"agent"
"agent",
"organizationPolicy"
],
"type": "string"
},
@@ -4377,6 +4378,12 @@
"null"
]
},
"allowGuardianFailOpen": {
"type": [
"boolean",
"null"
]
},
"allowManagedHooksOnly": {
"type": [
"boolean",
codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json
Generated
+6
View File
@@ -79,6 +79,12 @@
"null"
]
},
"allowGuardianFailOpen": {
"type": [
"boolean",
"null"
]
},
"allowManagedHooksOnly": {
"type": [
"boolean",
codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json
Generated
+2 -1
View File
@@ -61,7 +61,8 @@
"AutoReviewDecisionSource": {
"description": "[UNSTABLE] Source that produced a terminal approval auto-review decision.",
"enum": [
"agent"
"agent",
"organizationPolicy"
],
"type": "string"
},
codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDecisionSource.ts
Generated
+1 -1
View File
@@ -5,4 +5,4 @@
/**
* [UNSTABLE] Source that produced a terminal approval auto-review decision.
*/
export type AutoReviewDecisionSource = "agent";
export type AutoReviewDecisionSource = "agent" | "organizationPolicy";
codex-rs/app-server-protocol/schema/typescript/v2/ConfigRequirements.ts
Generated
+1 -1
View File
@@ -8,4 +8,4 @@ import type { ResidencyRequirement } from "./ResidencyRequirement";
import type { SandboxMode } from "./SandboxMode";
import type { WindowsSandboxSetupMode } from "./WindowsSandboxSetupMode";
export type ConfigRequirements = {allowedApprovalPolicies: Array<AskForApproval> | null, allowedSandboxModes: Array<SandboxMode> | null, allowedWindowsSandboxImplementations: Array<WindowsSandboxSetupMode> | null, allowedPermissionProfiles: { [key in string]?: boolean } | null, defaultPermissions: string | null, allowedWebSearchModes: Array<WebSearchMode> | null, allowManagedHooksOnly: boolean | null, allowAppshots: boolean | null, computerUse: ComputerUseRequirements | null, featureRequirements: { [key in string]?: boolean } | null, enforceResidency: ResidencyRequirement | null};
export type ConfigRequirements = {allowedApprovalPolicies: Array<AskForApproval> | null, allowedSandboxModes: Array<SandboxMode> | null, allowedWindowsSandboxImplementations: Array<WindowsSandboxSetupMode> | null, allowedPermissionProfiles: { [key in string]?: boolean } | null, defaultPermissions: string | null, allowedWebSearchModes: Array<WebSearchMode> | null, allowManagedHooksOnly: boolean | null, allowAppshots: boolean | null, allowGuardianFailOpen: boolean | null, computerUse: ComputerUseRequirements | null, featureRequirements: { [key in string]?: boolean } | null, enforceResidency: ResidencyRequirement | null};
codex-rs/app-server-protocol/src/protocol/item_builders.rs
+4
View File
@@ -315,3 +315,7 @@ fn format_file_change_diff(change: &FileChange) -> String {
}
}
}
#[cfg(test)]
#[path = "item_builders_tests.rs"]
mod tests;
codex-rs/app-server-protocol/src/protocol/item_builders_tests.rs
+40
View File
@@ -0,0 +1,40 @@
use super::*;
use codex_protocol::protocol::GuardianAssessmentDecisionSource;
use codex_protocol::protocol::GuardianAssessmentStatus;
use pretty_assertions::assert_eq;
#[test]
fn guardian_notification_preserves_organization_policy_decision_source() {
let thread_id = ThreadId::new();
let notification = guardian_auto_approval_review_notification(
&thread_id,
"event-turn",
&GuardianAssessmentEvent {
id: "guardian-1".to_string(),
target_item_id: None,
turn_id: "assessment-turn".to_string(),
started_at_ms: 10,
completed_at_ms: Some(20),
status: GuardianAssessmentStatus::Approved,
risk_level: None,
user_authorization: None,
rationale: Some("Organization policy allowed the request to proceed.".to_string()),
decision_source: Some(GuardianAssessmentDecisionSource::OrganizationPolicy),
action: GuardianAssessmentAction::McpToolCall {
server: "github".to_string(),
tool_name: "create_pull_request".to_string(),
connector_id: None,
connector_name: None,
tool_title: None,
},
},
);
let ServerNotification::ItemGuardianApprovalReviewCompleted(notification) = notification else {
panic!("expected completed Guardian review notification");
};
assert_eq!(
notification.decision_source,
AutoReviewDecisionSource::OrganizationPolicy
);
}
codex-rs/app-server-protocol/src/protocol/v2/config.rs
+1
View File
@@ -380,6 +380,7 @@ pub struct ConfigRequirements {
pub allowed_web_search_modes: Option<Vec<WebSearchMode>>,
pub allow_managed_hooks_only: Option<bool>,
pub allow_appshots: Option<bool>,
pub allow_guardian_fail_open: Option<bool>,
pub computer_use: Option<ComputerUseRequirements>,
pub feature_requirements: Option<BTreeMap<String, bool>>,
#[experimental("configRequirements/read.hooks")]
codex-rs/app-server-protocol/src/protocol/v2/item.rs
+2
View File
@@ -425,12 +425,14 @@ pub enum GuardianApprovalReviewStatus {
/// [UNSTABLE] Source that produced a terminal approval auto-review decision.
pub enum AutoReviewDecisionSource {
Agent,
OrganizationPolicy,
}
impl From<CoreGuardianAssessmentDecisionSource> for AutoReviewDecisionSource {
fn from(value: CoreGuardianAssessmentDecisionSource) -> Self {
match value {
CoreGuardianAssessmentDecisionSource::Agent => Self::Agent,
CoreGuardianAssessmentDecisionSource::OrganizationPolicy => Self::OrganizationPolicy,
}
}
}
codex-rs/app-server-protocol/src/protocol/v2/tests.rs
+1
View File
@@ -1703,6 +1703,7 @@ fn config_requirements_granular_allowed_approval_policy_is_marked_experimental()
allowed_web_search_modes: None,
allow_managed_hooks_only: None,
allow_appshots: None,
allow_guardian_fail_open: None,
computer_use: None,
feature_requirements: None,
hooks: None,
codex-rs/app-server/README.md
+2 -2
View File
@@ -231,7 +231,7 @@ Example with notification opt-out:
- `externalAgentConfig/import` — apply selected external-agent migration items by passing explicit `migrationItems` with `cwd` (`null` for home) and any plugin/session `details` returned by detect. When a request includes migration items, the server emits `externalAgentConfig/import/completed` once after the full import finishes (immediately after the response when everything completed synchronously, or after background imports finish).
- `config/value/write` — write a single config key/value to the user's config.toml on disk; dotted paths such as `desktop.someKey` use the same generic write surface.
- `config/batchWrite` — apply multiple config edits atomically to the user's config.toml on disk, with optional `reloadUserConfig: true` to hot-reload loaded threads, including multiple `desktop.*` edits.
- `configRequirements/read` — fetch loaded requirements constraints from `requirements.toml` and/or MDM (or `null` if none are configured), including allow-lists (`allowedApprovalPolicies`, `allowedSandboxModes`, `allowedWebSearchModes`), the layered permission-profile allow map (`allowedPermissionProfiles`), the managed permission-profile default (`defaultPermissions`), lifecycle hook lockdown (`allowManagedHooksOnly`), computer use policy (`computerUse`), pinned feature values (`featureRequirements`), managed lifecycle hooks (`hooks`), `enforceResidency`, and `network` constraints such as canonical domain/socket permissions plus `managedAllowedDomainsOnly` and `dangerFullAccessDenylistOnly`.
- `configRequirements/read` — fetch loaded requirements constraints from `requirements.toml` and/or MDM (or `null` if none are configured), including allow-lists (`allowedApprovalPolicies`, `allowedSandboxModes`, `allowedWebSearchModes`), the layered permission-profile allow map (`allowedPermissionProfiles`), the managed permission-profile default (`defaultPermissions`), lifecycle hook lockdown (`allowManagedHooksOnly`), Guardian failure policy (`allowGuardianFailOpen`), computer use policy (`computerUse`), pinned feature values (`featureRequirements`), managed lifecycle hooks (`hooks`), `enforceResidency`, and `network` constraints such as canonical domain/socket permissions plus `managedAllowedDomainsOnly` and `dangerFullAccessDenylistOnly`.
### Example: Start or resume a thread
@@ -1289,7 +1289,7 @@ All items emit shared lifecycle events:
- `item/autoApprovalReview/started` — [UNSTABLE] temporary auto-review notification carrying `{threadId, turnId, targetItemId, review, action}` when approval auto-review begins. This shape is expected to change soon.
- `item/autoApprovalReview/completed` — [UNSTABLE] temporary auto-review notification carrying `{threadId, turnId, targetItemId, review, action}` when approval auto-review resolves. This shape is expected to change soon.
`review` is [UNSTABLE] and currently has `{status, riskLevel?, userAuthorization?, rationale?}`, where `status` is one of `inProgress`, `approved`, `denied`, or `aborted`. `riskLevel` is one of `"low"`, `"medium"`, `"high"`, or `"critical"` when present. `userAuthorization` is one of `"unknown"`, `"low"`, `"medium"`, or `"high"` when present. `action` is a tagged union with `type: "command" | "execve" | "applyPatch" | "networkAccess" | "mcpToolCall"`. Command-like actions include a `source` discriminator (`"shell"` or `"unifiedExec"`). These notifications are separate from the target item's own `item/completed` lifecycle and are intentionally temporary while the auto-review app protocol is still being designed.
`review` is [UNSTABLE] and currently has `{status, riskLevel?, userAuthorization?, rationale?}`, where `status` is one of `inProgress`, `approved`, `denied`, `timedOut`, or `aborted`. `riskLevel` is one of `"low"`, `"medium"`, `"high"`, or `"critical"` when present. `userAuthorization` is one of `"unknown"`, `"low"`, `"medium"`, or `"high"` when present. Completed notifications also include `decisionSource`, which is `agent` for a Guardian decision or `organizationPolicy` when a managed fail-open policy approves the request after Guardian could not complete its review. `action` is a tagged union with `type: "command" | "execve" | "applyPatch" | "networkAccess" | "mcpToolCall" | "requestPermissions"`. Command-like actions include a `source` discriminator (`"shell"` or `"unifiedExec"`). These notifications are separate from the target item's own `item/completed` lifecycle and are intentionally temporary while the auto-review app protocol is still being designed.
There are additional item-specific events:
codex-rs/app-server/src/request_processors/config_processor.rs
+11
View File
@@ -364,6 +364,7 @@ fn map_requirements_toml_to_api(requirements: ConfigRequirementsToml) -> ConfigR
}),
allow_managed_hooks_only: requirements.allow_managed_hooks_only,
allow_appshots: requirements.allow_appshots,
allow_guardian_fail_open: requirements.allow_guardian_fail_open,
computer_use: requirements
.computer_use
.map(map_computer_use_requirements_to_api),
@@ -618,6 +619,16 @@ mod tests {
assert_eq!(mapped.hooks, None);
}
#[test]
fn requirements_api_includes_allow_guardian_fail_open() {
let mapped = map_requirements_toml_to_api(ConfigRequirementsToml {
allow_guardian_fail_open: Some(false),
..ConfigRequirementsToml::default()
});
assert_eq!(mapped.allow_guardian_fail_open, Some(false));
}
#[test]
fn requirements_api_includes_computer_use_requirements() {
let mapped = map_requirements_toml_to_api(ConfigRequirementsToml {
codex-rs/core/src/codex_delegate.rs
+4 -3
View File
@@ -33,6 +33,7 @@ use tokio_util::sync::CancellationToken;
use crate::config::Config;
use crate::guardian::GuardianApprovalRequest;
use crate::guardian::new_guardian_review_id;
use crate::guardian::resolve_spawned_approval_review;
use crate::guardian::routes_approval_to_guardian;
use crate::guardian::routes_approval_to_guardian_with_reviewer;
use crate::guardian::spawn_approval_request_review;
@@ -483,7 +484,7 @@ async fn handle_exec_approval(
review_cancel.clone(),
);
await_approval_with_cancel(
async move { review_rx.await.unwrap_or_default() },
resolve_spawned_approval_review(review_rx),
parent_session,
&approval_id_for_op,
cancel_token,
@@ -591,7 +592,7 @@ async fn handle_patch_approval(
);
Some(
await_approval_with_cancel(
async move { review_rx.await.unwrap_or_default() },
resolve_spawned_approval_review(review_rx),
parent_session,
&approval_id,
cancel_token,
@@ -711,7 +712,7 @@ async fn maybe_auto_review_mcp_request_user_input(
review_cancel.clone(),
);
let decision = await_approval_with_cancel(
async move { review_rx.await.unwrap_or_default() },
resolve_spawned_approval_review(review_rx),
parent_session,
&event.call_id,
cancel_token,
codex-rs/core/src/guardian/metrics.rs
+27
View File
@@ -149,6 +149,7 @@ fn terminal_status_tag(status: GuardianReviewTerminalStatus) -> &'static str {
GuardianReviewTerminalStatus::Aborted => "aborted",
GuardianReviewTerminalStatus::TimedOut => "timed_out",
GuardianReviewTerminalStatus::FailedClosed => "failed_closed",
GuardianReviewTerminalStatus::FailedOpen => "failed_open",
}
}
@@ -415,4 +416,30 @@ mod tests {
BTreeMap::from([("sample".to_string(), 123)])
);
}
#[test]
fn guardian_review_metrics_tag_failed_open_outcomes() {
let tags = guardian_review_metric_tags(
&GuardianReviewAnalyticsResult {
decision: GuardianReviewDecision::Approved,
terminal_status: GuardianReviewTerminalStatus::FailedOpen,
failure_reason: Some(GuardianReviewFailureReason::SessionError),
..GuardianReviewAnalyticsResult::without_session()
},
GuardianApprovalRequestSource::MainTurn,
&GuardianReviewedAction::ApplyPatch {},
)
.into_iter()
.collect::<BTreeMap<_, _>>();
assert_eq!(tags.get("decision").map(String::as_str), Some("approved"));
assert_eq!(
tags.get("terminal_status").map(String::as_str),
Some("failed_open")
);
assert_eq!(
tags.get("failure_reason").map(String::as_str),
Some("session_error")
);
}
}
codex-rs/core/src/guardian/mod.rs
+3 -1
View File
@@ -8,7 +8,8 @@
//! action and return strict JSON.
//! The guardian clones the parent config, so it inherits any managed
//! network proxy / allowlist that the parent turn already had.
//! 3. Fail closed on timeout, execution failure, or malformed output.
//! 3. Apply the managed failure policy to timeouts, execution failures, and
//! malformed output. The default remains fail closed.
//! 4. Apply the guardian's explicit allow/deny outcome.
mod approval_request;
@@ -35,6 +36,7 @@ pub(crate) use review::is_guardian_reviewer_source;
pub(crate) use review::new_guardian_review_id;
#[cfg(test)]
pub(crate) use review::record_guardian_denial_for_test;
pub(crate) use review::resolve_spawned_approval_review;
pub(crate) use review::review_approval_request;
#[cfg(test)]
pub(crate) use review::review_approval_request_with_cancel;
codex-rs/core/src/guardian/review.rs
+208 -32
View File
@@ -80,6 +80,11 @@ pub(crate) async fn guardian_rejection_message(session: &Session, review_id: &st
rejection.rationale.trim(),
GUARDIAN_REJECTION_INSTRUCTIONS
),
GuardianAssessmentDecisionSource::OrganizationPolicy => format!(
"This action was rejected by organization policy.\nReason: {}\n{}",
rejection.rationale.trim(),
GUARDIAN_REJECTION_INSTRUCTIONS
),
}
}
@@ -93,6 +98,26 @@ pub(super) enum GuardianReviewOutcome {
Error(GuardianReviewError),
}
impl GuardianReviewOutcome {
fn with_cancellation_precedence(self, external_cancel: Option<&CancellationToken>) -> Self {
if external_cancel.is_some_and(CancellationToken::is_cancelled) {
Self::Error(GuardianReviewError::Cancelled)
} else {
self
}
}
}
enum GuardianReviewExecution {
RunSession {
external_cancel: Option<CancellationToken>,
},
Error {
error: GuardianReviewError,
external_cancel: CancellationToken,
},
}
#[derive(Debug)]
pub(super) enum GuardianReviewError {
PromptBuild { message: String },
@@ -130,6 +155,16 @@ impl GuardianReviewError {
Self::Cancelled => GuardianReviewFailureReason::Cancelled,
}
}
fn can_fail_open(&self) -> bool {
match self {
Self::PromptBuild { .. }
| Self::Session { .. }
| Self::Parse { .. }
| Self::Timeout => true,
Self::Cancelled => false,
}
}
}
fn guardian_risk_level_str(level: GuardianRiskLevel) -> &'static str {
@@ -248,9 +283,10 @@ pub(crate) async fn record_guardian_denial_for_test(
record_guardian_denial(session, turn, turn_id).await;
}
/// This function always fails closed: timeouts, review-session failures, and
/// parse failures all block execution, but timeouts are still surfaced to the
/// caller as distinct from explicit guardian denials.
/// Reviews an approval request and applies the managed Guardian failure policy.
/// Explicit denials and cancellations always block execution. Infrastructure
/// failures only approve the request when organization policy opts into
/// fail-open behavior.
async fn run_guardian_review(
session: Arc<Session>,
turn: Arc<TurnContext>,
@@ -258,8 +294,14 @@ async fn run_guardian_review(
request: GuardianApprovalRequest,
retry_reason: Option<String>,
approval_request_source: GuardianApprovalRequestSource,
external_cancel: Option<CancellationToken>,
execution: GuardianReviewExecution,
) -> ReviewDecision {
let external_cancel_for_policy = match &execution {
GuardianReviewExecution::RunSession { external_cancel } => external_cancel.clone(),
GuardianReviewExecution::Error {
external_cancel, ..
} => Some(external_cancel.clone()),
};
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);
@@ -293,7 +335,7 @@ async fn run_guardian_review(
)
.await;
if external_cancel
if external_cancel_for_policy
.as_ref()
.is_some_and(CancellationToken::is_cancelled)
{
@@ -335,15 +377,24 @@ async fn run_guardian_review(
let schema = guardian_output_schema();
let terminal_action = action_summary.clone();
let (outcome, analytics_result) = Box::pin(run_guardian_review_session(
session.clone(),
turn.clone(),
request,
retry_reason.clone(),
schema,
external_cancel,
))
.await;
let (outcome, analytics_result) = match execution {
GuardianReviewExecution::RunSession { external_cancel } => {
Box::pin(run_guardian_review_session(
session.clone(),
turn.clone(),
request,
retry_reason.clone(),
schema,
external_cancel,
))
.await
}
GuardianReviewExecution::Error { error, .. } => (
GuardianReviewOutcome::Error(error),
GuardianReviewAnalyticsResult::without_session(),
),
};
let outcome = outcome.with_cancellation_precedence(external_cancel_for_policy.as_ref());
let completed_at_ms = now_unix_timestamp_ms();
let (assessment, count_denial_for_circuit_breaker) = match outcome {
@@ -377,6 +428,63 @@ async fn run_guardian_review(
matches!(assessment.outcome, GuardianAssessmentOutcome::Deny);
(assessment, count_denial_for_circuit_breaker)
}
GuardianReviewOutcome::Error(error)
if turn.config.allow_guardian_fail_open && error.can_fail_open() =>
{
let rationale = match &error {
GuardianReviewError::Timeout => {
"Automatic approval review timed out. Organization policy allowed the request to proceed."
.to_string()
}
GuardianReviewError::PromptBuild { message }
| GuardianReviewError::Session { message }
| GuardianReviewError::Parse { message } => format!(
"Automatic approval review failed: {message}. Organization policy allowed the request to proceed."
),
GuardianReviewError::Cancelled => unreachable!(),
};
track_guardian_review(
session.as_ref(),
&review_tracking,
approval_request_source,
&reviewed_action,
GuardianReviewAnalyticsResult {
decision: GuardianReviewDecision::Approved,
terminal_status: GuardianReviewTerminalStatus::FailedOpen,
failure_reason: Some(error.failure_reason()),
..analytics_result
},
completed_at_ms.try_into().unwrap_or_default(),
);
session
.send_event(
turn.as_ref(),
EventMsg::GuardianWarning(WarningEvent {
message: rationale.clone(),
}),
)
.await;
session
.send_event(
turn.as_ref(),
EventMsg::GuardianAssessment(GuardianAssessmentEvent {
id: review_id,
target_item_id,
turn_id: assessment_turn_id.clone(),
started_at_ms,
completed_at_ms: Some(completed_at_ms),
status: GuardianAssessmentStatus::Approved,
risk_level: None,
user_authorization: None,
rationale: Some(rationale),
decision_source: Some(GuardianAssessmentDecisionSource::OrganizationPolicy),
action: terminal_action,
}),
)
.await;
record_guardian_non_denial(&session, &assessment_turn_id).await;
return ReviewDecision::Approved;
}
GuardianReviewOutcome::Error(error) => match error {
GuardianReviewError::Timeout => {
let rationale =
@@ -585,7 +693,9 @@ pub(crate) async fn review_approval_request(
request,
retry_reason,
GuardianApprovalRequestSource::MainTurn,
/*external_cancel*/ None,
GuardianReviewExecution::RunSession {
external_cancel: None,
},
))
.await
}
@@ -606,11 +716,19 @@ pub(crate) async fn review_approval_request_with_cancel(
request,
retry_reason,
approval_request_source,
Some(cancel_token),
GuardianReviewExecution::RunSession {
external_cancel: Some(cancel_token),
},
)
.await
}
pub(crate) async fn resolve_spawned_approval_review(
review_rx: oneshot::Receiver<ReviewDecision>,
) -> ReviewDecision {
review_rx.await.unwrap_or(ReviewDecision::Denied)
}
pub(crate) fn spawn_approval_request_review(
session: Arc<Session>,
turn: Arc<TurnContext>,
@@ -621,24 +739,43 @@ pub(crate) fn spawn_approval_request_review(
cancel_token: CancellationToken,
) -> oneshot::Receiver<ReviewDecision> {
let (tx, rx) = oneshot::channel();
let fallback_runtime_handle = session.services.runtime_handle.clone();
std::thread::spawn(move || {
let Ok(runtime) = tokio::runtime::Builder::new_current_thread()
match tokio::runtime::Builder::new_current_thread()
.enable_all()
.build()
else {
let _ = tx.send(ReviewDecision::Denied);
return;
};
let decision = runtime.block_on(review_approval_request_with_cancel(
&session,
&turn,
review_id,
request,
retry_reason,
approval_request_source,
cancel_token,
));
let _ = tx.send(decision);
{
Ok(runtime) => {
let decision = runtime.block_on(review_approval_request_with_cancel(
&session,
&turn,
review_id,
request,
retry_reason,
approval_request_source,
cancel_token,
));
let _ = tx.send(decision);
}
Err(err) => {
drop(fallback_runtime_handle.spawn(async move {
let decision = run_guardian_review(
session,
turn,
review_id,
request,
retry_reason,
approval_request_source,
GuardianReviewExecution::Error {
error: GuardianReviewError::session(err.into()),
external_cancel: cancel_token,
},
)
.await;
let _ = tx.send(decision);
}));
}
}
});
rx
}
@@ -795,7 +932,7 @@ pub(super) async fn run_guardian_review_session(
GuardianReviewOutcome::Error(GuardianReviewError::Timeout),
session_analytics_result,
),
GuardianReviewSessionOutcome::Aborted => (
GuardianReviewSessionOutcome::Cancelled => (
GuardianReviewOutcome::Error(GuardianReviewError::Cancelled),
session_analytics_result,
),
@@ -826,4 +963,43 @@ mod review_tests {
GuardianReviewFailureReason::SessionError
));
}
#[test]
fn guardian_failure_policy_excludes_only_cancellation() {
let failures = [
GuardianReviewError::prompt_build(anyhow::anyhow!("bad prompt/config")),
GuardianReviewError::session(anyhow::anyhow!("guardian runtime failed")),
GuardianReviewError::parse(anyhow::anyhow!("bad guardian JSON")),
GuardianReviewError::Timeout,
];
assert!(failures.iter().all(GuardianReviewError::can_fail_open));
assert!(!GuardianReviewError::Cancelled.can_fail_open());
}
#[test]
fn guardian_cancellation_takes_precedence_over_fail_open_error() {
let external_cancel = CancellationToken::new();
external_cancel.cancel();
let outcome = GuardianReviewOutcome::Error(GuardianReviewError::session(anyhow::anyhow!(
"guardian runtime failed"
)))
.with_cancellation_precedence(Some(&external_cancel));
assert!(matches!(
outcome,
GuardianReviewOutcome::Error(GuardianReviewError::Cancelled)
));
}
#[tokio::test]
async fn closed_spawned_review_channel_fails_closed() {
let (tx, rx) = oneshot::channel();
drop(tx);
assert_eq!(
resolve_spawned_approval_review(rx).await,
ReviewDecision::Denied
);
}
}
codex-rs/core/src/guardian/review_session.rs
+39 -9
View File
@@ -62,7 +62,7 @@ pub(crate) enum GuardianReviewSessionOutcome {
PromptBuildFailed(anyhow::Error),
SessionFailed(anyhow::Error),
TimedOut,
Aborted,
Cancelled,
}
pub(crate) struct GuardianReviewSessionParams {
@@ -851,7 +851,7 @@ async fn wait_for_guardian_review(
)
.await
.is_ok();
return (GuardianReviewSessionOutcome::Aborted, keep_review_session, false);
return (GuardianReviewSessionOutcome::Cancelled, keep_review_session, false);
}
event = review_session.codex.next_event() => {
match event {
@@ -880,7 +880,7 @@ async fn wait_for_guardian_review(
last_error_message = Some(error.message);
}
EventMsg::TurnAborted(_) => {
return (GuardianReviewSessionOutcome::Aborted, true, false);
return (GuardianReviewSessionOutcome::Cancelled, true, false);
}
_ => {}
},
@@ -999,7 +999,7 @@ async fn run_before_review_deadline<T>(
} else {
std::future::pending::<()>().await;
}
} => Err(GuardianReviewSessionOutcome::Aborted),
} => Err(GuardianReviewSessionOutcome::Cancelled),
}
}
@@ -1101,18 +1101,22 @@ mod tests {
}
}
fn turn_aborted_event(turn_id: &str) -> Event {
fn turn_aborted_event_with_reason(turn_id: &str, reason: TurnAbortReason) -> Event {
Event {
id: turn_id.to_string(),
msg: EventMsg::TurnAborted(TurnAbortedEvent {
turn_id: Some(turn_id.to_string()),
reason: TurnAbortReason::Interrupted,
reason,
completed_at: None,
duration_ms: None,
}),
}
}
fn turn_aborted_event(turn_id: &str) -> Event {
turn_aborted_event_with_reason(turn_id, TurnAbortReason::Interrupted)
}
async fn test_review_params() -> GuardianReviewSessionParams {
let (session, turn) = crate::session::tests::make_session_and_context().await;
let model = turn.model_info.slug.clone();
@@ -1321,7 +1325,7 @@ mod tests {
assert!(matches!(
outcome,
Err(GuardianReviewSessionOutcome::Aborted)
Err(GuardianReviewSessionOutcome::Cancelled)
));
}
@@ -1366,7 +1370,7 @@ mod tests {
assert!(matches!(
outcome,
Err(GuardianReviewSessionOutcome::Aborted)
Err(GuardianReviewSessionOutcome::Cancelled)
));
assert!(cancel_token.is_cancelled());
}
@@ -1576,6 +1580,32 @@ mod tests {
assert!(capture_token_usage);
}
#[tokio::test]
async fn wait_for_guardian_review_treats_current_turn_abort_as_cancelled() {
let (review_session, tx_event, _rx_sub) = test_review_session().await;
tx_event
.send(turn_aborted_event_with_reason(
"current-turn",
TurnAbortReason::Replaced,
))
.await
.expect("queue current turn abort");
let mut analytics_result = GuardianReviewAnalyticsResult::without_session();
let (outcome, keep_review_session, capture_token_usage) = wait_for_guardian_review(
&review_session,
"current-turn",
tokio::time::Instant::now() + Duration::from_secs(1),
/*external_cancel*/ None,
&mut analytics_result,
)
.await;
assert!(matches!(outcome, GuardianReviewSessionOutcome::Cancelled));
assert!(keep_review_session);
assert!(!capture_token_usage);
}
#[tokio::test]
async fn wait_for_guardian_review_timeout_drains_expected_turn_after_stale_terminal_event() {
let (review_session, tx_event, rx_sub) = test_review_session().await;
@@ -1643,7 +1673,7 @@ mod tests {
interrupt_response
.await
.expect("interrupt response task should complete");
assert!(matches!(outcome, GuardianReviewSessionOutcome::Aborted));
assert!(matches!(outcome, GuardianReviewSessionOutcome::Cancelled));
assert!(keep_review_session);
assert!(!capture_token_usage);
}
codex-rs/core/src/guardian/tests.rs
+139 -42
View File
@@ -59,6 +59,7 @@ use core_test_support::responses::mount_response_once;
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::streaming_sse::StreamingSseChunk;
@@ -194,6 +195,37 @@ async fn guardian_test_session_and_turn_with_base_url(
(Arc::new(session), Arc::new(turn))
}
async fn guardian_test_session_turn_and_rx(
server: &wiremock::MockServer,
) -> (
Arc<Session>,
Arc<TurnContext>,
async_channel::Receiver<Event>,
) {
let (mut session, mut turn, rx) =
crate::session::tests::make_session_and_context_with_rx().await;
let mut config = (*turn.config).clone();
config.model_provider.base_url = Some(format!("{}/v1", server.uri()));
config.user_instructions = None;
let config = Arc::new(config);
let models_manager = test_support::models_manager_with_provider(
config.codex_home.to_path_buf(),
Arc::clone(&session.services.auth_manager),
config.model_provider.clone(),
);
Arc::get_mut(&mut session)
.expect("session should be uniquely owned")
.services
.models_manager = models_manager;
let turn_mut = Arc::get_mut(&mut turn).expect("turn should be uniquely owned");
turn_mut.config = Arc::clone(&config);
turn_mut.provider =
create_model_provider(config.model_provider.clone(), turn_mut.auth_manager.clone());
turn_mut.user_instructions = None;
(session, turn, rx)
}
async fn seed_guardian_parent_history(session: &Arc<Session>, turn: &Arc<TurnContext>) {
session
.record_conversation_items(
@@ -1031,32 +1063,66 @@ fn guardian_request_target_item_id_omits_network_access_trigger_call_id() {
assert_eq!(guardian_request_target_item_id(&network_access), None);
}
#[tokio::test]
async fn cancelled_guardian_review_emits_terminal_abort_without_warning() {
let (session, turn, rx) = crate::session::tests::make_session_and_context_with_rx().await;
let cancel_token = CancellationToken::new();
cancel_token.cancel();
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn in_flight_guardian_cancellation_emits_abort_without_warning() -> anyhow::Result<()> {
skip_if_no_network!(Ok(()));
let decision = review_approval_request_with_cancel(
&session,
&turn,
"review-cancelled-guardian".to_string(),
GuardianApprovalRequest::ApplyPatch {
id: "patch-1".to_string(),
cwd: test_path_buf("/tmp").abs(),
files: vec![test_path_buf("/tmp/guardian.txt").abs()],
patch: "*** Begin Patch\n*** Update File: guardian.txt\n@@\n+hello\n*** End Patch"
.to_string(),
},
/*retry_reason*/ None,
GuardianApprovalRequestSource::MainTurn,
cancel_token,
let server = start_mock_server().await;
let request_log = mount_response_once(
&server,
sse_response(sse(vec![ev_response_created("resp-cancelled-guardian")]))
.set_delay(Duration::from_secs(60)),
)
.await;
let (session, mut turn, rx) = guardian_test_session_turn_and_rx(&server).await;
let mut config = (*turn.config).clone();
config.allow_guardian_fail_open = true;
Arc::get_mut(&mut turn)
.expect("turn should be uniquely owned")
.config = Arc::new(config);
seed_guardian_parent_history(&session, &turn).await;
let cancel_token = CancellationToken::new();
let cancel_for_review = cancel_token.clone();
let review_task = tokio::spawn(async move {
review_approval_request_with_cancel(
&session,
&turn,
"review-cancelled-guardian".to_string(),
GuardianApprovalRequest::ApplyPatch {
id: "patch-1".to_string(),
cwd: test_path_buf("/tmp").abs(),
files: vec![test_path_buf("/tmp/guardian.txt").abs()],
patch: "*** Begin Patch\n*** Update File: guardian.txt\n@@\n+hello\n*** End Patch"
.to_string(),
},
/*retry_reason*/ None,
GuardianApprovalRequestSource::MainTurn,
cancel_for_review,
)
.await
});
let started_status = loop {
let event = rx.recv().await.expect("Guardian review event");
if let EventMsg::GuardianAssessment(assessment) = event.msg
&& assessment.status == GuardianAssessmentStatus::InProgress
{
break assessment.status;
}
};
tokio::time::timeout(Duration::from_secs(30), async {
while request_log.requests().is_empty() {
tokio::task::yield_now().await;
}
})
.await
.expect("Guardian request should reach the Responses API");
cancel_token.cancel();
let decision = review_task.await.expect("Guardian review task");
assert_eq!(decision, ReviewDecision::Abort);
let mut guardian_statuses = Vec::new();
let mut guardian_statuses = vec![started_status];
let mut warnings = Vec::new();
while let Ok(event) = rx.try_recv() {
match event.msg {
@@ -1074,6 +1140,7 @@ async fn cancelled_guardian_review_emits_terminal_abort_without_warning() {
]
);
assert!(warnings.is_empty());
Ok(())
}
#[test]
@@ -1972,7 +2039,7 @@ async fn guardian_reused_trunk_ignores_stale_prior_turn_completion() -> anyhow::
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn guardian_review_surfaces_responses_api_errors_in_rejection_reason() -> anyhow::Result<()> {
async fn guardian_review_applies_failure_policy_to_responses_api_errors() -> anyhow::Result<()> {
skip_if_no_network!(Ok(()));
let server = start_mock_server().await;
@@ -1990,27 +2057,7 @@ async fn guardian_review_surfaces_responses_api_errors_in_rejection_reason() ->
)
.await;
let (mut session, mut turn, rx) =
crate::session::tests::make_session_and_context_with_rx().await;
let mut config = (*turn.config).clone();
config.model_provider.base_url = Some(format!("{}/v1", server.uri()));
config.user_instructions = None;
let config = Arc::new(config);
let models_manager = test_support::models_manager_with_provider(
config.codex_home.to_path_buf(),
Arc::clone(&session.services.auth_manager),
config.model_provider.clone(),
);
Arc::get_mut(&mut session)
.expect("session should be uniquely owned")
.services
.models_manager = models_manager;
let turn_mut = Arc::get_mut(&mut turn).expect("turn should be uniquely owned");
turn_mut.config = Arc::clone(&config);
turn_mut.provider =
create_model_provider(config.model_provider.clone(), turn_mut.auth_manager.clone());
turn_mut.user_instructions = None;
let (session, turn, rx) = guardian_test_session_turn_and_rx(&server).await;
seed_guardian_parent_history(&session, &turn).await;
let decision = review_approval_request(
@@ -2080,6 +2127,56 @@ async fn guardian_review_surfaces_responses_api_errors_in_rejection_reason() ->
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn guardian_explicit_denial_still_blocks_when_fail_open_is_allowed() -> anyhow::Result<()> {
skip_if_no_network!(Ok(()));
let server = start_mock_server().await;
let assessment = serde_json::json!({
"risk_level": "high",
"user_authorization": "low",
"outcome": "deny",
"rationale": "The command would publish unreviewed changes.",
})
.to_string();
let _request_log = mount_sse_once(
&server,
sse(vec![
ev_response_created("resp-guardian-deny"),
ev_assistant_message("msg-guardian-deny", &assessment),
ev_completed("resp-guardian-deny"),
]),
)
.await;
let (session, mut turn) = guardian_test_session_and_turn(&server).await;
let mut config = (*turn.config).clone();
config.allow_guardian_fail_open = true;
Arc::get_mut(&mut turn)
.expect("turn should be uniquely owned")
.config = Arc::new(config);
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("Publish the current branch.".to_string()),
},
/*retry_reason*/ None,
)
.await;
assert_eq!(decision, ReviewDecision::Denied);
Ok(())
}
#[tokio::test]
async fn guardian_parallel_reviews_fork_from_last_committed_trunk_history() -> anyhow::Result<()> {
const TEST_STACK_SIZE_BYTES: usize = 4 * 1024 * 1024;
codex-rs/core/src/session/mod.rs
+1 -1
View File
@@ -2193,7 +2193,7 @@ impl Session {
let decision = tokio::select! {
biased;
_ = cancellation_token.cancelled() => return None,
decision = review_rx => decision.unwrap_or(ReviewDecision::Denied),
decision = crate::guardian::resolve_spawned_approval_review(review_rx) => decision,
};
let response = match decision {
ReviewDecision::Approved | ReviewDecision::ApprovedExecpolicyAmendment { .. } => {
codex-rs/core/tests/suite/guardian_review.rs
+170 -3
View File
@@ -6,6 +6,8 @@ use codex_core::sandboxing::SandboxPermissions;
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::GuardianAssessmentStatus;
use codex_protocol::protocol::Op;
use codex_protocol::protocol::SandboxPolicy;
use codex_protocol::user_input::UserInput;
@@ -14,8 +16,10 @@ 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_sequence;
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;
@@ -29,6 +33,7 @@ use std::fs;
use std::os::unix::fs::PermissionsExt;
use std::time::Duration;
use tempfile::TempDir;
use wiremock::ResponseTemplate;
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn guardian_review_session_does_not_inherit_legacy_notify() -> Result<()> {
@@ -61,9 +66,12 @@ printf '%s\n' "${@: -1}" >> "${payload_path}""#,
let mut builder = test_codex().with_config(move |config| {
config.notify = Some(vec![notify_script_str]);
config.permissions.approval_policy = Constrained::allow_any(approval_policy);
config
.set_legacy_sandbox_policy(sandbox_policy_for_config)
.expect("set sandbox policy");
assert!(
config
.set_legacy_sandbox_policy(sandbox_policy_for_config)
.is_ok(),
"set sandbox policy"
);
});
let test = builder.build(&server).await?;
@@ -168,3 +176,162 @@ printf '%s\n' "${@: -1}" >> "${payload_path}""#,
Ok(())
}
#[derive(Clone, Copy)]
enum GuardianFailurePolicy {
FailClosed,
FailOpen,
}
async fn run_guardian_api_failure_command(policy: GuardianFailurePolicy) -> Result<()> {
skip_if_no_network!(Ok(()));
skip_if_sandbox!(Ok(()));
let server = start_mock_server().await;
let approval_policy = AskForApproval::OnRequest;
let sandbox_policy = SandboxPolicy::WorkspaceWrite {
writable_roots: vec![],
network_access: false,
exclude_tmpdir_env_var: true,
exclude_slash_tmp: true,
};
let sandbox_policy_for_config = sandbox_policy.clone();
let mut builder = test_codex().with_config(move |config| {
config.allow_guardian_fail_open = matches!(policy, GuardianFailurePolicy::FailOpen);
config.permissions.approval_policy = Constrained::allow_any(approval_policy);
config
.set_legacy_sandbox_policy(sandbox_policy_for_config)
.unwrap_or_else(|err| panic!("set sandbox policy: {err}"));
});
let test = builder.build(&server).await?;
let (output_name, output_contents) = match policy {
GuardianFailurePolicy::FailClosed => {
("guardian-failed-closed.txt", "guardian-failed-closed")
}
GuardianFailurePolicy::FailOpen => ("guardian-failed-open.txt", "guardian-failed-open"),
};
let output_file = test.cwd.path().join(output_name);
let command = format!("printf {output_contents} > {}", output_file.display());
let tool_args = json!({
"cmd": command,
"yield_time_ms": 1_000_u64,
"sandbox_permissions": SandboxPermissions::RequireEscalated,
"justification": "Exercise Guardian fail-open routing.",
});
let error_message = "Guardian service unavailable";
let responses = mount_response_sequence(
&server,
vec![
sse_response(sse(vec![
ev_response_created("resp-parent-tool"),
ev_function_call(
"exec-call",
"exec_command",
&serde_json::to_string(&tool_args)?,
),
ev_completed("resp-parent-tool"),
])),
ResponseTemplate::new(400).set_body_json(json!({
"error": {
"message": error_message,
"type": "invalid_request_error",
}
})),
sse_response(sse(vec![
ev_response_created("resp-parent-done"),
ev_assistant_message("msg-parent-done", "done"),
ev_completed("resp-parent-done"),
])),
],
)
.await;
test.codex
.submit(Op::UserInput {
items: vec![UserInput::Text {
text: "run a command that requires Guardian review".into(),
text_elements: Vec::new(),
}],
final_output_json_schema: None,
responsesapi_client_metadata: None,
additional_context: Default::default(),
thread_settings: codex_protocol::protocol::ThreadSettingsOverrides {
environments: Some(local_selections(test.config.cwd.clone())),
approval_policy: Some(approval_policy),
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
sandbox_policy: Some(sandbox_policy),
..Default::default()
},
})
.await?;
let mut terminal_assessment = None;
loop {
let event = wait_for_event(&test.codex, |_| true).await;
match event {
EventMsg::GuardianAssessment(assessment)
if assessment.status != GuardianAssessmentStatus::InProgress =>
{
terminal_assessment = Some(assessment);
}
EventMsg::TurnComplete(_) => break,
_ => {}
}
}
let Some(assessment) = terminal_assessment else {
anyhow::bail!("expected terminal Guardian assessment");
};
assert!(
assessment
.rationale
.as_deref()
.is_some_and(|rationale| rationale.contains(error_message))
);
match policy {
GuardianFailurePolicy::FailClosed => {
assert_eq!(assessment.status, GuardianAssessmentStatus::Denied);
assert_eq!(
assessment.decision_source,
Some(GuardianAssessmentDecisionSource::Agent)
);
assert!(!output_file.exists());
}
GuardianFailurePolicy::FailOpen => {
assert_eq!(assessment.status, GuardianAssessmentStatus::Approved);
assert_eq!(
assessment.decision_source,
Some(GuardianAssessmentDecisionSource::OrganizationPolicy)
);
assert!(
assessment
.rationale
.as_deref()
.is_some_and(|rationale| rationale.contains("Organization policy"))
);
assert_eq!(fs::read_to_string(&output_file)?, output_contents);
}
}
let Some(guardian_request) = responses
.requests()
.into_iter()
.find(|request| request.body_contains_text("Exercise Guardian fail-open routing."))
else {
anyhow::bail!("expected Guardian review request");
};
assert!(guardian_request.body_contains_text(&command));
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn guardian_api_failure_blocks_command_by_default() -> Result<()> {
run_guardian_api_failure_command(GuardianFailurePolicy::FailClosed).await
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn guardian_api_failure_executes_command_when_organization_allows_fail_open() -> Result<()> {
run_guardian_api_failure_command(GuardianFailurePolicy::FailOpen).await
}
codex-rs/protocol/src/approvals.rs
+1
View File
@@ -122,6 +122,7 @@ pub enum GuardianAssessmentStatus {
#[serde(rename_all = "snake_case")]
pub enum GuardianAssessmentDecisionSource {
Agent,
OrganizationPolicy,
}
#[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)]
codex-rs/tui/src/chatwidget/protocol_requests.rs
+3
View File
@@ -127,6 +127,9 @@ impl ChatWidget {
codex_app_server_protocol::AutoReviewDecisionSource::Agent => {
GuardianAssessmentDecisionSource::Agent
}
codex_app_server_protocol::AutoReviewDecisionSource::OrganizationPolicy => {
GuardianAssessmentDecisionSource::OrganizationPolicy
}
}),
action: action.into(),
});