diff --git a/codex-rs/analytics/src/events.rs b/codex-rs/analytics/src/events.rs index 8a25215ec8..b243d0e3bd 100644 --- a/codex-rs/analytics/src/events.rs +++ b/codex-rs/analytics/src/events.rs @@ -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)] diff --git a/codex-rs/app-server-protocol/schema/json/ServerNotification.json b/codex-rs/app-server-protocol/schema/json/ServerNotification.json index e0651d1727..5d9e0dd61d 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerNotification.json +++ b/codex-rs/app-server-protocol/schema/json/ServerNotification.json @@ -538,7 +538,8 @@ "AutoReviewDecisionSource": { "description": "[UNSTABLE] Source that produced a terminal approval auto-review decision.", "enum": [ - "agent" + "agent", + "organizationPolicy" ], "type": "string" }, diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index d65c043a7d..9a7a975a02 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -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", diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index 8271dc485d..8e26a98d34 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -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", diff --git a/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json index def89d64e3..429d1a04d2 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json @@ -79,6 +79,12 @@ "null" ] }, + "allowGuardianFailOpen": { + "type": [ + "boolean", + "null" + ] + }, "allowManagedHooksOnly": { "type": [ "boolean", diff --git a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json index a366c99a41..d19e29d5a0 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json @@ -61,7 +61,8 @@ "AutoReviewDecisionSource": { "description": "[UNSTABLE] Source that produced a terminal approval auto-review decision.", "enum": [ - "agent" + "agent", + "organizationPolicy" ], "type": "string" }, diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDecisionSource.ts b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDecisionSource.ts index 8806981237..dc9c966c77 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDecisionSource.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDecisionSource.ts @@ -5,4 +5,4 @@ /** * [UNSTABLE] Source that produced a terminal approval auto-review decision. */ -export type AutoReviewDecisionSource = "agent"; +export type AutoReviewDecisionSource = "agent" | "organizationPolicy"; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ConfigRequirements.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ConfigRequirements.ts index 29704982ff..cf8e167245 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ConfigRequirements.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ConfigRequirements.ts @@ -8,4 +8,4 @@ import type { ResidencyRequirement } from "./ResidencyRequirement"; import type { SandboxMode } from "./SandboxMode"; import type { WindowsSandboxSetupMode } from "./WindowsSandboxSetupMode"; -export type ConfigRequirements = {allowedApprovalPolicies: Array | null, allowedSandboxModes: Array | null, allowedWindowsSandboxImplementations: Array | null, allowedPermissionProfiles: { [key in string]?: boolean } | null, defaultPermissions: string | null, allowedWebSearchModes: Array | null, allowManagedHooksOnly: boolean | null, allowAppshots: boolean | null, computerUse: ComputerUseRequirements | null, featureRequirements: { [key in string]?: boolean } | null, enforceResidency: ResidencyRequirement | null}; +export type ConfigRequirements = {allowedApprovalPolicies: Array | null, allowedSandboxModes: Array | null, allowedWindowsSandboxImplementations: Array | null, allowedPermissionProfiles: { [key in string]?: boolean } | null, defaultPermissions: string | null, allowedWebSearchModes: Array | null, allowManagedHooksOnly: boolean | null, allowAppshots: boolean | null, allowGuardianFailOpen: boolean | null, computerUse: ComputerUseRequirements | null, featureRequirements: { [key in string]?: boolean } | null, enforceResidency: ResidencyRequirement | null}; diff --git a/codex-rs/app-server-protocol/src/protocol/item_builders.rs b/codex-rs/app-server-protocol/src/protocol/item_builders.rs index 17e0f9aef4..40766e91c0 100644 --- a/codex-rs/app-server-protocol/src/protocol/item_builders.rs +++ b/codex-rs/app-server-protocol/src/protocol/item_builders.rs @@ -315,3 +315,7 @@ fn format_file_change_diff(change: &FileChange) -> String { } } } + +#[cfg(test)] +#[path = "item_builders_tests.rs"] +mod tests; diff --git a/codex-rs/app-server-protocol/src/protocol/item_builders_tests.rs b/codex-rs/app-server-protocol/src/protocol/item_builders_tests.rs new file mode 100644 index 0000000000..4fc03c319b --- /dev/null +++ b/codex-rs/app-server-protocol/src/protocol/item_builders_tests.rs @@ -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 + ); +} diff --git a/codex-rs/app-server-protocol/src/protocol/v2/config.rs b/codex-rs/app-server-protocol/src/protocol/v2/config.rs index 227adffc30..ede805b84b 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/config.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/config.rs @@ -380,6 +380,7 @@ pub struct ConfigRequirements { pub allowed_web_search_modes: Option>, pub allow_managed_hooks_only: Option, pub allow_appshots: Option, + pub allow_guardian_fail_open: Option, pub computer_use: Option, pub feature_requirements: Option>, #[experimental("configRequirements/read.hooks")] diff --git a/codex-rs/app-server-protocol/src/protocol/v2/item.rs b/codex-rs/app-server-protocol/src/protocol/v2/item.rs index 29d212ab15..a96065e7c9 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/item.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/item.rs @@ -425,12 +425,14 @@ pub enum GuardianApprovalReviewStatus { /// [UNSTABLE] Source that produced a terminal approval auto-review decision. pub enum AutoReviewDecisionSource { Agent, + OrganizationPolicy, } impl From for AutoReviewDecisionSource { fn from(value: CoreGuardianAssessmentDecisionSource) -> Self { match value { CoreGuardianAssessmentDecisionSource::Agent => Self::Agent, + CoreGuardianAssessmentDecisionSource::OrganizationPolicy => Self::OrganizationPolicy, } } } diff --git a/codex-rs/app-server-protocol/src/protocol/v2/tests.rs b/codex-rs/app-server-protocol/src/protocol/v2/tests.rs index c570b87ce4..b82c252342 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/tests.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/tests.rs @@ -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, diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 999cc8fa37..91eac8e0c4 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -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: diff --git a/codex-rs/app-server/src/request_processors/config_processor.rs b/codex-rs/app-server/src/request_processors/config_processor.rs index bdc8013950..1163c6ec6c 100644 --- a/codex-rs/app-server/src/request_processors/config_processor.rs +++ b/codex-rs/app-server/src/request_processors/config_processor.rs @@ -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 { diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 5b68ecf917..a466ac53bd 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -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, diff --git a/codex-rs/core/src/guardian/metrics.rs b/codex-rs/core/src/guardian/metrics.rs index 9b9d35a526..4ab4f946c8 100644 --- a/codex-rs/core/src/guardian/metrics.rs +++ b/codex-rs/core/src/guardian/metrics.rs @@ -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::>(); + + 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") + ); + } } diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index b4920f1ff6..d2be88e212 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -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; diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 934e68042f..3638bcf6cf 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -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, + }, + 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, turn: Arc, @@ -258,8 +294,14 @@ async fn run_guardian_review( request: GuardianApprovalRequest, retry_reason: Option, approval_request_source: GuardianApprovalRequestSource, - external_cancel: Option, + 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 { + review_rx.await.unwrap_or(ReviewDecision::Denied) +} + pub(crate) fn spawn_approval_request_review( session: Arc, turn: Arc, @@ -621,24 +739,43 @@ pub(crate) fn spawn_approval_request_review( cancel_token: CancellationToken, ) -> oneshot::Receiver { 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 + ); + } } diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index fcda9a5a23..a10e522bf3 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -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( } 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); } diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index ffadd801ca..0e82d8083d 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -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, + Arc, + async_channel::Receiver, +) { + 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, turn: &Arc) { 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; diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 2e5d5643da..5b3d4bb241 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -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 { .. } => { diff --git a/codex-rs/core/tests/suite/guardian_review.rs b/codex-rs/core/tests/suite/guardian_review.rs index ae87350c39..7acb89a3ce 100644 --- a/codex-rs/core/tests/suite/guardian_review.rs +++ b/codex-rs/core/tests/suite/guardian_review.rs @@ -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 +} diff --git a/codex-rs/protocol/src/approvals.rs b/codex-rs/protocol/src/approvals.rs index ace096359c..5cfd779ced 100644 --- a/codex-rs/protocol/src/approvals.rs +++ b/codex-rs/protocol/src/approvals.rs @@ -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)] diff --git a/codex-rs/tui/src/chatwidget/protocol_requests.rs b/codex-rs/tui/src/chatwidget/protocol_requests.rs index 5b53f44fdf..eb53ad4328 100644 --- a/codex-rs/tui/src/chatwidget/protocol_requests.rs +++ b/codex-rs/tui/src/chatwidget/protocol_requests.rs @@ -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(), });