From 479f7046ebbb5039c587b72a33af4e3776eac281 Mon Sep 17 00:00:00 2001 From: jif Date: Tue, 27 Oct 2026 16:11:28 +0000 Subject: [PATCH] Move Guardian request mappings into the reviewer crate (#57506) ## What changed Move approval-request assessment and analytics mappings, target item lookup, and turn ID selection into `codex-guardian-reviewer`. Export the helpers from the crate and re-export them in core, preserving existing behavior. ## Testing Move the apply-patch serialization test into the reviewer crate. It checks that Windows paths are preserved and patch text is omitted from the assessment action. GitOrigin-RevId: 3a011db0bc51c653bb0877c4b0cb52eb61c63244 --- codex-rs/Cargo.lock | 1 + .../core/src/guardian/approval_request.rs | 160 +----------------- codex-rs/core/src/guardian/tests.rs | 35 ---- codex-rs/ext/guardian-reviewer/Cargo.toml | 1 + .../guardian-reviewer/src/approval_request.rs | 159 ++++++++++++++++- .../src/approval_request_tests.rs | 40 +++++ codex-rs/ext/guardian-reviewer/src/lib.rs | 4 + 7 files changed, 208 insertions(+), 192 deletions(-) create mode 100644 codex-rs/ext/guardian-reviewer/src/approval_request_tests.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 784236e99b..12036f3a3b 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3612,6 +3612,7 @@ dependencies = [ "codex-otel", "codex-prompts", "codex-protocol", + "codex-shell-command", "codex-utils-path-uri", "opentelemetry", "opentelemetry_sdk", diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index 8891bef5b9..757bbda849 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -1,161 +1,9 @@ -use codex_analytics::GuardianReviewedAction; 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; pub(crate) use codex_guardian_reviewer::guardian_approval_request_to_json; -use codex_protocol::approvals::GuardianAssessmentAction; -use codex_protocol::approvals::GuardianCommandSource; -use codex_utils_path_uri::LegacyAppPathString; - -fn command_assessment_action( - source: GuardianCommandSource, - command: &[String], - cwd: &LegacyAppPathString, -) -> GuardianAssessmentAction { - GuardianAssessmentAction::Command { - source, - command: codex_shell_command::parse_command::shlex_join(command), - cwd: cwd.clone(), - } -} - -pub(crate) fn guardian_assessment_action( - action: &GuardianApprovalRequest, -) -> Option { - Some(match action { - GuardianApprovalRequest::NativeToolCall { .. } => return None, - GuardianApprovalRequest::ExecCommand { - command, - guardian_cwd, - .. - } => command_assessment_action(GuardianCommandSource::UnifiedExec, command, guardian_cwd), - GuardianApprovalRequest::WriteStdin { - approval_id, - process_id, - input, - cwd, - .. - } => GuardianAssessmentAction::WriteStdin { - approval_id: approval_id.clone(), - process_id: process_id.to_string(), - stdin: input.clone(), - cwd: cwd.clone(), - }, - GuardianApprovalRequest::ApplyPatch { cwd, files, .. } => { - GuardianAssessmentAction::ApplyPatch { - cwd: cwd.clone().into(), - files: files.iter().cloned().map(Into::into).collect(), - } - } - GuardianApprovalRequest::NetworkAccess { - id: _id, - turn_id: _turn_id, - environment_id: _, - target, - host, - protocol, - port, - trigger: _trigger, - } => GuardianAssessmentAction::NetworkAccess { - target: target.clone(), - host: host.clone(), - protocol: *protocol, - port: *port, - }, - GuardianApprovalRequest::McpToolCall { - server, - tool_name, - connector_id, - connector_name, - tool_title, - .. - } => GuardianAssessmentAction::McpToolCall { - server: server.clone(), - tool_name: tool_name.clone(), - connector_id: connector_id.clone(), - connector_name: connector_name.clone(), - tool_title: tool_title.clone(), - }, - GuardianApprovalRequest::RequestPermissions { - reason, - permissions, - .. - } => GuardianAssessmentAction::RequestPermissions { - reason: reason.clone(), - permissions: permissions.clone(), - }, - }) -} - -pub(crate) fn guardian_reviewed_action( - request: &GuardianApprovalRequest, -) -> GuardianReviewedAction { - match request { - GuardianApprovalRequest::NativeToolCall { .. } => GuardianReviewedAction::NativeToolCall {}, - GuardianApprovalRequest::ExecCommand { - sandbox_permissions, - additional_permissions, - tty, - .. - } => GuardianReviewedAction::UnifiedExec { - sandbox_permissions: *sandbox_permissions, - additional_permissions: additional_permissions.as_ref().map(Into::into), - tty: *tty, - }, - GuardianApprovalRequest::WriteStdin { tty, .. } => { - GuardianReviewedAction::WriteStdin { tty: *tty } - } - GuardianApprovalRequest::ApplyPatch { .. } => GuardianReviewedAction::ApplyPatch {}, - GuardianApprovalRequest::NetworkAccess { protocol, port, .. } => { - GuardianReviewedAction::NetworkAccess { - protocol: *protocol, - port: *port, - } - } - GuardianApprovalRequest::McpToolCall { - server, - tool_name, - connector_id, - connector_name, - tool_title, - .. - } => GuardianReviewedAction::McpToolCall { - server: server.clone(), - tool_name: tool_name.clone(), - connector_id: connector_id.clone(), - connector_name: connector_name.clone(), - tool_title: tool_title.clone(), - }, - GuardianApprovalRequest::RequestPermissions { .. } => { - GuardianReviewedAction::RequestPermissions {} - } - } -} - -pub(crate) fn guardian_request_target_item_id(request: &GuardianApprovalRequest) -> Option<&str> { - match request { - GuardianApprovalRequest::ExecCommand { id, .. } - | GuardianApprovalRequest::WriteStdin { id, .. } - | GuardianApprovalRequest::ApplyPatch { id, .. } - | GuardianApprovalRequest::McpToolCall { id, .. } - | GuardianApprovalRequest::RequestPermissions { id, .. } - | GuardianApprovalRequest::NativeToolCall { id, .. } => Some(id), - GuardianApprovalRequest::NetworkAccess { .. } => None, - } -} - -pub(crate) fn guardian_request_turn_id<'a>( - request: &'a GuardianApprovalRequest, - default_turn_id: &'a str, -) -> &'a str { - match request { - GuardianApprovalRequest::NetworkAccess { turn_id, .. } - | GuardianApprovalRequest::RequestPermissions { turn_id, .. } => turn_id, - GuardianApprovalRequest::ExecCommand { .. } - | GuardianApprovalRequest::WriteStdin { .. } - | GuardianApprovalRequest::ApplyPatch { .. } - | GuardianApprovalRequest::McpToolCall { .. } - | GuardianApprovalRequest::NativeToolCall { .. } => default_turn_id, - } -} +pub(crate) use codex_guardian_reviewer::guardian_assessment_action; +pub(crate) use codex_guardian_reviewer::guardian_request_target_item_id; +pub(crate) use codex_guardian_reviewer::guardian_request_turn_id; +pub(crate) use codex_guardian_reviewer::guardian_reviewed_action; diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index fb6f95c85b..071a61716e 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -1522,41 +1522,6 @@ fn guardian_exec_command_uses_executor_cwd_convention( Ok(()) } -#[test] -fn guardian_apply_patch_preserves_foreign_paths_and_redacts_patch_text() -> serde_json::Result<()> { - let cwd = PathUri::parse("file:///C:/workspace").expect("valid executor cwd"); - let file = - PathUri::parse("file:///C:/workspace/guardian.txt").expect("valid executor file path"); - let patch = "*** Begin Patch\n*** Update File: guardian.txt\n@@\n+secret\n*** End Patch"; - let action = GuardianApprovalRequest::ApplyPatch { - environment_id: "local".to_string(), - id: "patch-1".to_string(), - cwd, - files: vec![file], - patch: patch.to_string(), - }; - - assert_eq!( - guardian_approval_request_to_json(&action)?, - serde_json::json!({ - "tool": "apply_patch", - "environment_id": "local", - "cwd": r"C:\workspace", - "files": [r"C:\workspace\guardian.txt"], - "patch": patch, - }), - ); - assert_eq!( - serde_json::to_value(guardian_assessment_action(&action))?, - serde_json::json!({ - "type": "apply_patch", - "cwd": r"C:\workspace", - "files": [r"C:\workspace\guardian.txt"], - }), - ); - Ok(()) -} - #[test] fn guardian_request_turn_id_prefers_network_access_owner_turn() { let network_access = GuardianApprovalRequest::NetworkAccess { diff --git a/codex-rs/ext/guardian-reviewer/Cargo.toml b/codex-rs/ext/guardian-reviewer/Cargo.toml index 59ce596479..04e7bc388e 100644 --- a/codex-rs/ext/guardian-reviewer/Cargo.toml +++ b/codex-rs/ext/guardian-reviewer/Cargo.toml @@ -22,6 +22,7 @@ codex-mcp = { workspace = true } codex-otel = { workspace = true } codex-prompts = { workspace = true } codex-protocol = { workspace = true } +codex-shell-command = { workspace = true } codex-utils-path-uri = { workspace = true } rand = { workspace = true } serde = { workspace = true } diff --git a/codex-rs/ext/guardian-reviewer/src/approval_request.rs b/codex-rs/ext/guardian-reviewer/src/approval_request.rs index 29ab475b55..740b6e053f 100644 --- a/codex-rs/ext/guardian-reviewer/src/approval_request.rs +++ b/codex-rs/ext/guardian-reviewer/src/approval_request.rs @@ -1,6 +1,9 @@ -//! Owns Guardian approval-request data and its existing environment and scope classification. +//! Owns Guardian approval-request data, classification, and assessment/reporting mappings. //! Request debug output excludes action arguments, which may contain credentials. +use codex_analytics::GuardianReviewedAction; +use codex_protocol::approvals::GuardianAssessmentAction; +use codex_protocol::approvals::GuardianCommandSource; use codex_protocol::approvals::NetworkApprovalProtocol; use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::models::SandboxPermissions; @@ -141,3 +144,157 @@ impl GuardianApprovalRequest { } } } + +fn command_assessment_action( + source: GuardianCommandSource, + command: &[String], + cwd: &LegacyAppPathString, +) -> GuardianAssessmentAction { + GuardianAssessmentAction::Command { + source, + command: codex_shell_command::parse_command::shlex_join(command), + cwd: cwd.clone(), + } +} + +pub fn guardian_assessment_action( + action: &GuardianApprovalRequest, +) -> Option { + Some(match action { + GuardianApprovalRequest::NativeToolCall { .. } => return None, + GuardianApprovalRequest::ExecCommand { + command, + guardian_cwd, + .. + } => command_assessment_action(GuardianCommandSource::UnifiedExec, command, guardian_cwd), + GuardianApprovalRequest::WriteStdin { + approval_id, + process_id, + input, + cwd, + .. + } => GuardianAssessmentAction::WriteStdin { + approval_id: approval_id.clone(), + process_id: process_id.to_string(), + stdin: input.clone(), + cwd: cwd.clone(), + }, + GuardianApprovalRequest::ApplyPatch { cwd, files, .. } => { + GuardianAssessmentAction::ApplyPatch { + cwd: cwd.clone().into(), + files: files.iter().cloned().map(Into::into).collect(), + } + } + GuardianApprovalRequest::NetworkAccess { + id: _id, + turn_id: _turn_id, + environment_id: _, + target, + host, + protocol, + port, + trigger: _trigger, + } => GuardianAssessmentAction::NetworkAccess { + target: target.clone(), + host: host.clone(), + protocol: *protocol, + port: *port, + }, + GuardianApprovalRequest::McpToolCall { + server, + tool_name, + connector_id, + connector_name, + tool_title, + .. + } => GuardianAssessmentAction::McpToolCall { + server: server.clone(), + tool_name: tool_name.clone(), + connector_id: connector_id.clone(), + connector_name: connector_name.clone(), + tool_title: tool_title.clone(), + }, + GuardianApprovalRequest::RequestPermissions { + reason, + permissions, + .. + } => GuardianAssessmentAction::RequestPermissions { + reason: reason.clone(), + permissions: permissions.clone(), + }, + }) +} + +pub fn guardian_reviewed_action(request: &GuardianApprovalRequest) -> GuardianReviewedAction { + match request { + GuardianApprovalRequest::NativeToolCall { .. } => GuardianReviewedAction::NativeToolCall {}, + GuardianApprovalRequest::ExecCommand { + sandbox_permissions, + additional_permissions, + tty, + .. + } => GuardianReviewedAction::UnifiedExec { + sandbox_permissions: *sandbox_permissions, + additional_permissions: additional_permissions.as_ref().map(Into::into), + tty: *tty, + }, + GuardianApprovalRequest::WriteStdin { tty, .. } => { + GuardianReviewedAction::WriteStdin { tty: *tty } + } + GuardianApprovalRequest::ApplyPatch { .. } => GuardianReviewedAction::ApplyPatch {}, + GuardianApprovalRequest::NetworkAccess { protocol, port, .. } => { + GuardianReviewedAction::NetworkAccess { + protocol: *protocol, + port: *port, + } + } + GuardianApprovalRequest::McpToolCall { + server, + tool_name, + connector_id, + connector_name, + tool_title, + .. + } => GuardianReviewedAction::McpToolCall { + server: server.clone(), + tool_name: tool_name.clone(), + connector_id: connector_id.clone(), + connector_name: connector_name.clone(), + tool_title: tool_title.clone(), + }, + GuardianApprovalRequest::RequestPermissions { .. } => { + GuardianReviewedAction::RequestPermissions {} + } + } +} + +pub fn guardian_request_target_item_id(request: &GuardianApprovalRequest) -> Option<&str> { + match request { + GuardianApprovalRequest::ExecCommand { id, .. } + | GuardianApprovalRequest::WriteStdin { id, .. } + | GuardianApprovalRequest::ApplyPatch { id, .. } + | GuardianApprovalRequest::McpToolCall { id, .. } + | GuardianApprovalRequest::RequestPermissions { id, .. } + | GuardianApprovalRequest::NativeToolCall { id, .. } => Some(id), + GuardianApprovalRequest::NetworkAccess { .. } => None, + } +} + +pub fn guardian_request_turn_id<'a>( + request: &'a GuardianApprovalRequest, + default_turn_id: &'a str, +) -> &'a str { + match request { + GuardianApprovalRequest::NetworkAccess { turn_id, .. } + | GuardianApprovalRequest::RequestPermissions { turn_id, .. } => turn_id, + GuardianApprovalRequest::ExecCommand { .. } + | GuardianApprovalRequest::WriteStdin { .. } + | GuardianApprovalRequest::ApplyPatch { .. } + | GuardianApprovalRequest::McpToolCall { .. } + | GuardianApprovalRequest::NativeToolCall { .. } => default_turn_id, + } +} + +#[cfg(test)] +#[path = "approval_request_tests.rs"] +mod tests; diff --git a/codex-rs/ext/guardian-reviewer/src/approval_request_tests.rs b/codex-rs/ext/guardian-reviewer/src/approval_request_tests.rs new file mode 100644 index 0000000000..342e97f37e --- /dev/null +++ b/codex-rs/ext/guardian-reviewer/src/approval_request_tests.rs @@ -0,0 +1,40 @@ +#![cfg(test)] + +use super::*; +use crate::guardian_approval_request_to_json; +use pretty_assertions::assert_eq; + +#[test] +fn guardian_apply_patch_preserves_foreign_paths_and_redacts_patch_text() -> serde_json::Result<()> { + let cwd = PathUri::parse("file:///C:/workspace").expect("valid executor cwd"); + let file = + PathUri::parse("file:///C:/workspace/guardian.txt").expect("valid executor file path"); + let patch = "*** Begin Patch\n*** Update File: guardian.txt\n@@\n+secret\n*** End Patch"; + let action = GuardianApprovalRequest::ApplyPatch { + environment_id: "local".to_string(), + id: "patch-1".to_string(), + cwd, + files: vec![file], + patch: patch.to_string(), + }; + + assert_eq!( + guardian_approval_request_to_json(&action)?, + serde_json::json!({ + "tool": "apply_patch", + "environment_id": "local", + "cwd": r"C:\workspace", + "files": [r"C:\workspace\guardian.txt"], + "patch": patch, + }), + ); + assert_eq!( + serde_json::to_value(guardian_assessment_action(&action))?, + serde_json::json!({ + "type": "apply_patch", + "cwd": r"C:\workspace", + "files": [r"C:\workspace\guardian.txt"], + }), + ); + Ok(()) +} diff --git a/codex-rs/ext/guardian-reviewer/src/lib.rs b/codex-rs/ext/guardian-reviewer/src/lib.rs index 6e6c9721fd..4b9dc40f26 100644 --- a/codex-rs/ext/guardian-reviewer/src/lib.rs +++ b/codex-rs/ext/guardian-reviewer/src/lib.rs @@ -31,6 +31,10 @@ mod settings; pub use approval_request::GuardianApprovalRequest; pub use approval_request::GuardianMcpAnnotations; pub use approval_request::GuardianNetworkAccessTrigger; +pub use approval_request::guardian_assessment_action; +pub use approval_request::guardian_request_target_item_id; +pub use approval_request::guardian_request_turn_id; +pub use approval_request::guardian_reviewed_action; pub use approval_request_json::format_guardian_action_pretty; pub use approval_request_json::guardian_approval_request_to_json; pub use assessment::GuardianAssessment;