openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-29 19:08:43 +00:00
Code Issues Packages Projects Releases Wiki Activity

Move ReviewAction into the Guardian reviewer crate (#57720)

Browse Source 
## 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
This commit is contained in:
jif
2026-10-28 12:31:24 +00:00
committed by copyberry
parent e742db9f7b
commit 5d5e5c507d
9 changed files with 203 additions and 178 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/guardian/approval_request.rs
+1
View File
@@ -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;
codex-rs/core/src/guardian/mod.rs
+1
View File
@@ -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;
codex-rs/core/src/guardian/review_request.rs
+3 -2
View File
@@ -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),
codex-rs/core/src/guardian/runtime.rs
+65 -105
View File
@@ -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<serde_json::Value, String>,
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<String>,
pub(crate) request: Result<GuardianApprovalRequest, String>,
}
impl From<GuardianApprovalRequest> 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<Session>,
codex-rs/core/src/guardian/tests.rs
+8 -70
View File
@@ -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");
codex-rs/core/src/tools/approvals.rs
+1 -1
View File
@@ -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(),
codex-rs/ext/guardian-reviewer/src/lib.rs
+2
View File
@@ -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;
codex-rs/ext/guardian-reviewer/src/review_action.rs
+44
View File
@@ -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<serde_json::Value, String>,
pub category: codex_protocol::openai_models::GuardianScope,
/// Actual host invocation, absent when the approval has no originating tool call.
pub tool_call_id: Option<String>,
pub request: Result<GuardianApprovalRequest, String>,
}
impl From<GuardianApprovalRequest> 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;
codex-rs/ext/guardian-reviewer/src/review_action_tests.rs
+78
View File
@@ -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(())
}