From 5d5e5c507def46c4e53b30441cf8198c8663c9d2 Mon Sep 17 00:00:00 2001 From: jif Date: Wed, 28 Oct 2026 12:22:31 +0000 Subject: [PATCH] Move `ReviewAction` into the Guardian reviewer crate (#57720) ## What changed Export `ReviewAction` and its conversion from `GuardianApprovalRequest` from `codex-guardian-reviewer`. Keep host-specific approval preparation and environment validation in core as standalone functions, and update callers accordingly. ## Testing Move the `write_stdin` serialization and tool-call provenance test into the reviewer crate, preserving coverage for input, Windows working directories, and sandbox permission modes. GitOrigin-RevId: 9a1caf9bbb67ba017e4950a2da9ef2543b013007 --- .../core/src/guardian/approval_request.rs | 1 + codex-rs/core/src/guardian/mod.rs | 1 + codex-rs/core/src/guardian/review_request.rs | 5 +- codex-rs/core/src/guardian/runtime.rs | 170 +++++++----------- codex-rs/core/src/guardian/tests.rs | 78 +------- codex-rs/core/src/tools/approvals.rs | 2 +- codex-rs/ext/guardian-reviewer/src/lib.rs | 2 + .../guardian-reviewer/src/review_action.rs | 44 +++++ .../src/review_action_tests.rs | 78 ++++++++ 9 files changed, 203 insertions(+), 178 deletions(-) create mode 100644 codex-rs/ext/guardian-reviewer/src/review_action.rs create mode 100644 codex-rs/ext/guardian-reviewer/src/review_action_tests.rs diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index 757bbda849..7d63be2ab6 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -2,6 +2,7 @@ pub(crate) use codex_guardian_reviewer::GuardianApprovalRequest; pub(crate) use codex_guardian_reviewer::GuardianMcpAnnotations; pub(crate) use codex_guardian_reviewer::GuardianNetworkAccessTrigger; pub(crate) use codex_guardian_reviewer::format_guardian_action_pretty; +#[cfg(test)] pub(crate) use codex_guardian_reviewer::guardian_approval_request_to_json; pub(crate) use codex_guardian_reviewer::guardian_assessment_action; pub(crate) use codex_guardian_reviewer::guardian_request_target_item_id; diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index a714e4dd40..86bf379f8b 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -66,6 +66,7 @@ pub use review_session::prepare_review_prewarm; pub(crate) use codex_guardian_reviewer::prompt_cache_key_override_for_review_session; pub(crate) use runtime::ReviewAction; +pub(crate) use runtime::from_approval_action; pub(crate) use codex_guardian_reviewer::GUARDIAN_REVIEWER_NAME; pub(crate) use codex_guardian_reviewer::REVIEW_TIMEOUT as GUARDIAN_REVIEW_TIMEOUT; diff --git a/codex-rs/core/src/guardian/review_request.rs b/codex-rs/core/src/guardian/review_request.rs index e7ea989cc1..6d5466dce4 100644 --- a/codex-rs/core/src/guardian/review_request.rs +++ b/codex-rs/core/src/guardian/review_request.rs @@ -1,6 +1,7 @@ //! Binds an immutable action and root lifetime to fresh per-attempt authorization evidence. //! The extension chooses effects; this adapter supplies evidence, validation and publication. +use super::super::runtime::validate; use super::*; use crate::codex_thread::GuardianAuthorizationVersion; use codex_config::config_toml::CircuitBreakAction; @@ -130,7 +131,7 @@ impl ReviewHost for super::super::runtime::ReviewRuntime { reasons: _, options, } = self.clone(); - let request = match request.validate(&context) { + let request = match validate(&request, &context) { Ok(request) => request.clone(), Err(decision) => return Err(decision), }; @@ -307,7 +308,7 @@ impl ReviewHost for super::super::runtime::ReviewRuntime { } fn validate_action(&self) -> Result<(&str, Option<&str>), ReviewDecision> { - let request = self.request.validate(&self.context)?; + let request = validate(&self.request, &self.context)?; Ok(( guardian_request_turn_id(request, &self.context.turn().sub_id), guardian_request_target_item_id(request), diff --git a/codex-rs/core/src/guardian/runtime.rs b/codex-rs/core/src/guardian/runtime.rs index ac534dbd52..ff50d95ea6 100644 --- a/codex-rs/core/src/guardian/runtime.rs +++ b/codex-rs/core/src/guardian/runtime.rs @@ -1,5 +1,4 @@ -//! Captures one approval action for the extension-owned synchronous reviewer. -//! Tool-call provenance is separate from the item's display identity. +//! Prepares and validates host approval actions for the extension-owned reviewer. use codex_protocol::protocol::ReviewDecision; use codex_utils_path_uri::PathConvention; @@ -10,117 +9,78 @@ use super::ApprovalRequestReasons; use super::GuardianApprovalRequest; use super::GuardianReviewContext; use super::GuardianReviewOptions; -use super::approval_request::guardian_approval_request_to_json; use crate::session::session::Session; use crate::tools::sandboxing::ApprovalAction; +pub(crate) use codex_guardian_reviewer::ReviewAction; -/// Carries the original action even when the legacy synchronous renderer cannot handle its paths. -/// This lets the extension return AskUser before invoking that renderer. -#[derive(Clone)] -pub(crate) struct ReviewAction { - pub(crate) action: Result, - pub(crate) category: codex_protocol::openai_models::GuardianScope, - /// Actual host invocation, absent when the approval has no originating tool call. - pub(crate) tool_call_id: Option, - pub(crate) request: Result, -} - -impl From for ReviewAction { - fn from(request: GuardianApprovalRequest) -> Self { - let tool_call_id = match &request { - // Stdin's display target is the launch; freshness belongs to this write. - GuardianApprovalRequest::WriteStdin { approval_id, .. } => Some(approval_id.clone()), - GuardianApprovalRequest::ExecCommand { .. } - | GuardianApprovalRequest::ApplyPatch { .. } - | GuardianApprovalRequest::NetworkAccess { .. } - | GuardianApprovalRequest::McpToolCall { .. } - | GuardianApprovalRequest::RequestPermissions { .. } - | GuardianApprovalRequest::NativeToolCall { .. } => { - super::approval_request::guardian_request_target_item_id(&request) - .map(str::to_owned) - } - }; - Self { - action: guardian_approval_request_to_json(&request).map_err(|error| error.to_string()), - category: request.guardian_scope(), - tool_call_id, - request: Ok(request), - } - } -} - -impl ReviewAction { - pub(crate) fn from_approval_action( - action: ApprovalAction, - context: &GuardianReviewContext, - ) -> Self { - let exec_command_cwd_convention = match &action { - ApprovalAction::ExecCommand { - environment_id, - cwd, - .. - } => Some( - match context - .environments() - .turn_environments() - .find(|environment| environment.selection.environment_id == *environment_id) - .and_then(|environment| environment.executor_platform_os.as_deref()) - { - Some("windows") => PathConvention::Windows, - Some(_) => PathConvention::Posix, - // Legacy executors did not report their OS. Preserve the - // previous spelling-based behavior for those executors. - None => cwd - .infer_path_convention() - .unwrap_or_else(PathConvention::native), - }, - ), - _ => None, - }; - let category = action.guardian_scope(); - let original = serde_json::to_value(&action).map_err(|error| error.to_string()); - match action.into_guardian_request(exec_command_cwd_convention) { - Ok(request) => Self { - category, - ..Self::from(request) - }, - Err(error) => Self { - action: original, - category, - tool_call_id: None, - request: Err(error.to_string()), - }, - } - } -} - -impl ReviewAction { - /// Preserve the checks previously made before entering Guardian from tool approvals. - pub(super) fn validate( - &self, - context: &GuardianReviewContext, - ) -> Result<&GuardianApprovalRequest, ReviewDecision> { - let request = self.request.as_ref().map_err(|error| { - tracing::error!(%error, "failed to build automatic approval action"); - ReviewDecision::denied("automatic approval review could not prepare the action") - })?; - if let Some(environment_id) = request.target_environment_id() - && !context +pub(crate) fn from_approval_action( + action: ApprovalAction, + context: &GuardianReviewContext, +) -> ReviewAction { + let exec_command_cwd_convention = match &action { + ApprovalAction::ExecCommand { + environment_id, + cwd, + .. + } => Some( + match context .environments() .turn_environments() - .any(|environment| environment.selection.environment_id == environment_id) - { - let message = if matches!(request, GuardianApprovalRequest::WriteStdin { .. }) { - "automatic approval review cannot access the terminal's environment; select it before retrying" - } else { - "automatic approval review cannot access the request's environment" - }; - return Err(ReviewDecision::denied(message)); - } - Ok(request) + .find(|environment| environment.selection.environment_id == *environment_id) + .and_then(|environment| environment.executor_platform_os.as_deref()) + { + Some("windows") => PathConvention::Windows, + Some(_) => PathConvention::Posix, + // Legacy executors did not report their OS. Preserve the + // previous spelling-based behavior for those executors. + None => cwd + .infer_path_convention() + .unwrap_or_else(PathConvention::native), + }, + ), + _ => None, + }; + let category = action.guardian_scope(); + let original = serde_json::to_value(&action).map_err(|error| error.to_string()); + match action.into_guardian_request(exec_command_cwd_convention) { + Ok(request) => ReviewAction { + category, + ..ReviewAction::from(request) + }, + Err(error) => ReviewAction { + action: original, + category, + tool_call_id: None, + request: Err(error.to_string()), + }, } } +/// Preserve the checks previously made before entering Guardian from tool approvals. +pub(super) fn validate<'a>( + action: &'a ReviewAction, + context: &GuardianReviewContext, +) -> Result<&'a GuardianApprovalRequest, ReviewDecision> { + let request = action.request.as_ref().map_err(|error| { + tracing::error!(%error, "failed to build automatic approval action"); + ReviewDecision::denied("automatic approval review could not prepare the action") + })?; + if let Some(environment_id) = request.target_environment_id() + && !context + .environments() + .turn_environments() + .any(|environment| environment.selection.environment_id == environment_id) + { + let message = if matches!(request, GuardianApprovalRequest::WriteStdin { .. }) { + "automatic approval review cannot access the terminal's environment; select it before retrying" + } else { + "automatic approval review cannot access the request's environment" + }; + return Err(ReviewDecision::denied(message)); + } + Ok(request) +} + #[derive(Clone)] pub(super) struct ReviewRuntime { pub(super) session: Arc, diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index a879ac16f8..fed2b2860c 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -664,8 +664,8 @@ async fn approval_permissions_use_the_owning_environment() -> anyhow::Result<()> missing_context.environments.environments.truncate(1); for request in requests { let action = runtime::ReviewAction::from(request.clone()); - assert!(action.validate(&context).is_ok()); - assert!(action.validate(&missing_context).is_err()); + assert!(runtime::validate(&action, &context).is_ok()); + assert!(runtime::validate(&action, &missing_context).is_err()); let environment_id = request.target_environment_id().unwrap().to_string(); assert_eq!( guardian_approval_request_to_json(&request)?["environment_id"], @@ -702,9 +702,11 @@ async fn approval_permissions_use_the_owning_environment() -> anyhow::Result<()> } } assert!( - runtime::ReviewAction::from(network_request("missing")) - .validate(&context) - .is_err() + runtime::validate( + &runtime::ReviewAction::from(network_request("missing")), + &context, + ) + .is_err() ); Ok(()) } @@ -1402,70 +1404,6 @@ async fn build_guardian_prompt_items_explains_network_access_review_scope() -> a Ok(()) } -#[test_case::test_case(SandboxPermissions::UseDefault)] -#[test_case::test_case(SandboxPermissions::RequireEscalated)] -#[test_case::test_case(SandboxPermissions::WithAdditionalPermissions)] -fn guardian_write_stdin_preserves_input_and_foreign_cwd( - sandbox_permissions: SandboxPermissions, -) -> serde_json::Result<()> { - let cwd = PathUri::parse("file:///C:/workspace").expect("valid executor cwd"); - let input = "confirm\n"; - let additional_permissions = - if sandbox_permissions == SandboxPermissions::WithAdditionalPermissions { - Some(serde_json::from_value( - serde_json::json!({"network":{"enabled":true}}), - )?) - } else { - None - }; - let action = GuardianApprovalRequest::WriteStdin { - id: "terminal-open".to_string(), - approval_id: "terminal-write".to_string(), - environment_id: "windows-executor".to_string(), - process_id: 1000, - input: input.to_string(), - cwd: cwd.clone(), - tty: true, - sandbox_permissions, - additional_permissions: additional_permissions.clone(), - }; - - let mut expected = serde_json::json!({ - "tool": "write_stdin", - "environment_id": "windows-executor", - "session_id": 1000, - "chars": input, - "cwd": r"C:\workspace", - "sandbox_permissions": sandbox_permissions, - "tty": true, - }); - if let Some(permissions) = additional_permissions { - expected["additional_permissions"] = serde_json::to_value(permissions)?; - } - assert_eq!(guardian_approval_request_to_json(&action)?, expected); - assert_eq!( - guardian_assessment_action(&action).expect("reported approval action"), - GuardianAssessmentAction::WriteStdin { - approval_id: "terminal-write".to_string(), - process_id: "1000".to_string(), - stdin: input.to_string(), - cwd, - }, - ); - assert_eq!( - ( - guardian_request_target_item_id(&action), - guardian_request_turn_id(&action, "current-turn"), - ), - (Some("terminal-open"), "current-turn"), - ); - assert_eq!( - ReviewAction::from(action).tool_call_id.as_deref(), - Some("terminal-write") - ); - Ok(()) -} - #[test_case::test_case(Some("windows"), r"C:\workspace"; "windows")] #[test_case::test_case(Some("linux"), "/C:/workspace"; "posix")] #[test_case::test_case(None, r"C:\workspace"; "legacy")] @@ -1495,7 +1433,7 @@ async fn guardian_exec_command_uses_executor_cwd_convention( tty: false, proposed_execpolicy_amendment: None, }; - let action = ReviewAction::from_approval_action(action, &context) + let action = from_approval_action(action, &context) .request .expect("Guardian request should render executor cwd"); diff --git a/codex-rs/core/src/tools/approvals.rs b/codex-rs/core/src/tools/approvals.rs index bf1ed4f94b..058382cf25 100644 --- a/codex-rs/core/src/tools/approvals.rs +++ b/codex-rs/core/src/tools/approvals.rs @@ -623,7 +623,7 @@ impl Session { } let is_network_approval = matches!(&action, ApprovalAction::NetworkAccess { .. }); let review_id = new_guardian_review_id(); - let action = crate::guardian::ReviewAction::from_approval_action(action, &context); + let action = crate::guardian::from_approval_action(action, &context); let reasons = ApprovalRequestReasons { approval: ctx.approval_reason.clone(), diff --git a/codex-rs/ext/guardian-reviewer/src/lib.rs b/codex-rs/ext/guardian-reviewer/src/lib.rs index 7c5d436d86..2e272c0dc5 100644 --- a/codex-rs/ext/guardian-reviewer/src/lib.rs +++ b/codex-rs/ext/guardian-reviewer/src/lib.rs @@ -34,6 +34,7 @@ mod prompt; mod reporting; mod retry; mod review; +mod review_action; mod review_evidence; mod review_state; pub use review_state::CheckpointRecovery; @@ -93,6 +94,7 @@ pub use prompt::GuardianPromptItems; pub use prompt::compose_guardian_prompt; pub use review::ReviewHost; +pub use review_action::ReviewAction; pub use completion::ReviewCompletion; pub use completion::complete_review; diff --git a/codex-rs/ext/guardian-reviewer/src/review_action.rs b/codex-rs/ext/guardian-reviewer/src/review_action.rs new file mode 100644 index 0000000000..6ec4132204 --- /dev/null +++ b/codex-rs/ext/guardian-reviewer/src/review_action.rs @@ -0,0 +1,44 @@ +//! Captures one approval action for the synchronous reviewer. +//! Tool-call provenance is separate from the item's display identity. + +use crate::GuardianApprovalRequest; +use crate::guardian_approval_request_to_json; +use crate::guardian_request_target_item_id; + +/// Carries the original action even when the legacy synchronous renderer cannot handle its paths. +/// This lets the extension return AskUser before invoking that renderer. +#[derive(Clone)] +pub struct ReviewAction { + pub action: Result, + pub category: codex_protocol::openai_models::GuardianScope, + /// Actual host invocation, absent when the approval has no originating tool call. + pub tool_call_id: Option, + pub request: Result, +} + +impl From for ReviewAction { + fn from(request: GuardianApprovalRequest) -> Self { + let tool_call_id = match &request { + // Stdin's display target is the launch; freshness belongs to this write. + GuardianApprovalRequest::WriteStdin { approval_id, .. } => Some(approval_id.clone()), + GuardianApprovalRequest::ExecCommand { .. } + | GuardianApprovalRequest::ApplyPatch { .. } + | GuardianApprovalRequest::NetworkAccess { .. } + | GuardianApprovalRequest::McpToolCall { .. } + | GuardianApprovalRequest::RequestPermissions { .. } + | GuardianApprovalRequest::NativeToolCall { .. } => { + guardian_request_target_item_id(&request).map(str::to_owned) + } + }; + Self { + action: guardian_approval_request_to_json(&request).map_err(|error| error.to_string()), + category: request.guardian_scope(), + tool_call_id, + request: Ok(request), + } + } +} + +#[cfg(test)] +#[path = "review_action_tests.rs"] +mod tests; diff --git a/codex-rs/ext/guardian-reviewer/src/review_action_tests.rs b/codex-rs/ext/guardian-reviewer/src/review_action_tests.rs new file mode 100644 index 0000000000..103a884eba --- /dev/null +++ b/codex-rs/ext/guardian-reviewer/src/review_action_tests.rs @@ -0,0 +1,78 @@ +#![cfg(test)] + +//! Exercises approval action serialization and originating tool-call provenance. + +use super::ReviewAction; +use crate::GuardianApprovalRequest; +use crate::guardian_approval_request_to_json; +use crate::guardian_assessment_action; +use crate::guardian_request_target_item_id; +use crate::guardian_request_turn_id; +use codex_protocol::approvals::GuardianAssessmentAction; +use codex_protocol::models::SandboxPermissions; +use codex_utils_path_uri::PathUri; +use pretty_assertions::assert_eq; + +#[test_case::test_case(SandboxPermissions::UseDefault)] +#[test_case::test_case(SandboxPermissions::RequireEscalated)] +#[test_case::test_case(SandboxPermissions::WithAdditionalPermissions)] +fn guardian_write_stdin_preserves_input_and_foreign_cwd( + sandbox_permissions: SandboxPermissions, +) -> serde_json::Result<()> { + let cwd = PathUri::parse("file:///C:/workspace").expect("valid executor cwd"); + let input = "confirm\n"; + let additional_permissions = + if sandbox_permissions == SandboxPermissions::WithAdditionalPermissions { + Some(serde_json::from_value( + serde_json::json!({"network":{"enabled":true}}), + )?) + } else { + None + }; + let action = GuardianApprovalRequest::WriteStdin { + id: "terminal-open".to_string(), + approval_id: "terminal-write".to_string(), + environment_id: "windows-executor".to_string(), + process_id: 1000, + input: input.to_string(), + cwd: cwd.clone(), + tty: true, + sandbox_permissions, + additional_permissions: additional_permissions.clone(), + }; + + let mut expected = serde_json::json!({ + "tool": "write_stdin", + "environment_id": "windows-executor", + "session_id": 1000, + "chars": input, + "cwd": r"C:\workspace", + "sandbox_permissions": sandbox_permissions, + "tty": true, + }); + if let Some(permissions) = additional_permissions { + expected["additional_permissions"] = serde_json::to_value(permissions)?; + } + assert_eq!(guardian_approval_request_to_json(&action)?, expected); + assert_eq!( + guardian_assessment_action(&action).expect("reported approval action"), + GuardianAssessmentAction::WriteStdin { + approval_id: "terminal-write".to_string(), + process_id: "1000".to_string(), + stdin: input.to_string(), + cwd, + }, + ); + assert_eq!( + ( + guardian_request_target_item_id(&action), + guardian_request_turn_id(&action, "current-turn"), + ), + (Some("terminal-open"), "current-turn"), + ); + assert_eq!( + ReviewAction::from(action).tool_call_id.as_deref(), + Some("terminal-write") + ); + Ok(()) +}