From 65c13f1ae72c6aac8016a63bf9d436547954c9bb Mon Sep 17 00:00:00 2001 From: zhao-oai Date: Wed, 19 Nov 2025 16:50:43 -0800 Subject: [PATCH 1/9] execpolicy2 core integration (#6641) This PR threads execpolicy2 into codex-core. activated via feature flag: exec_policy (on by default) reads and parses all .codexpolicy files in `codex_home/codex` refactored tool runtime API to integrate execpolicy logic --------- Co-authored-by: Michael Bolin --- README.md | 33 ++ codex-rs/Cargo.lock | 1 + codex-rs/Cargo.toml | 1 + codex-rs/core/Cargo.toml | 1 + codex-rs/core/src/codex.rs | 13 + .../command_safety/is_dangerous_command.rs | 7 +- codex-rs/core/src/exec_policy.rs | 365 ++++++++++++++++++ codex-rs/core/src/features.rs | 8 + codex-rs/core/src/lib.rs | 1 + codex-rs/core/src/sandboxing/mod.rs | 22 ++ codex-rs/core/src/tools/handlers/shell.rs | 9 + codex-rs/core/src/tools/orchestrator.rs | 70 ++-- codex-rs/core/src/tools/runtimes/shell.rs | 19 +- .../core/src/tools/runtimes/unified_exec.rs | 21 +- codex-rs/core/src/tools/sandboxing.rs | 52 ++- .../core/src/unified_exec/session_manager.rs | 9 + codex-rs/core/tests/suite/exec_policy.rs | 101 +++++ codex-rs/core/tests/suite/mod.rs | 1 + codex-rs/execpolicy2/README.md | 30 +- codex-rs/execpolicy2/src/policy.rs | 4 + codex-rs/tui/src/chatwidget/agent.rs | 19 +- 21 files changed, 692 insertions(+), 95 deletions(-) create mode 100644 codex-rs/core/src/exec_policy.rs create mode 100644 codex-rs/core/tests/suite/exec_policy.rs diff --git a/README.md b/README.md index 8141610035..e34afbd288 100644 --- a/README.md +++ b/README.md @@ -69,6 +69,39 @@ Codex can access MCP servers. To configure them, refer to the [config docs](./do Codex CLI supports a rich set of configuration options, with preferences stored in `~/.codex/config.toml`. For full configuration options, see [Configuration](./docs/config.md). +### Execpolicy quickstart + +Codex can enforce your own rules-based execution policy before it runs shell commands. + +1. Create a policy directory: `mkdir -p ~/.codex/policy`. +2. Create one or more `.codexpolicy` files into that folder. Codex automatically loads every `.codexpolicy` file in there on startup. +3. Write `prefix_rule` entries to describe the commands you want to allow, prompt, or block: + +```starlark +prefix_rule( + pattern = ["git", ["push", "fetch"]], + decision = "prompt", # allow | prompt | forbidden + match = [["git", "push", "origin", "main"]], # examples that must match + not_match = [["git", "status"]], # examples that must not match +) +``` + +- `pattern` is a list of shell tokens, evaluated from left to right; wrap tokens in a nested list to express alternatives (e.g., match both `push` and `fetch`). +- `decision` sets the severity; Codex picks the strictest decision when multiple rules match. +- `match` and `not_match` act as (optional) unit tests. Codex validates them when it loads your policy, so you get feedback if an example has unexpected behavior. + +In this example rule, if Codex wants to run commands with the prefix `git push` or `git fetch`, it will first ask for user approval. + +Note: If Codex wants to run a command that matches with multiple rules, it will use the strictest decision among the matched rules (forbidden > prompt > allow). + +Use the [`execpolicy2` CLI](./codex-rs/execpolicy2/README.md) to preview decisions before you save a rule: + +```shell +cargo run -p codex-execpolicy2 -- check --policy ~/.codex/policy/default.codexpolicy git push origin main +``` + +Pass multiple `--policy` flags to test how several files combine. See the [`codex-rs/execpolicy2` README](./codex-rs/execpolicy2/README.md) for a more detailed walkthrough of the available syntax. + --- ### Docs & FAQ diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 66a1c208e4..798b8d71b3 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1086,6 +1086,7 @@ dependencies = [ "codex-apply-patch", "codex-arg0", "codex-async-utils", + "codex-execpolicy2", "codex-file-search", "codex-git", "codex-keyring-store", diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 2e88aba713..b0cf1e70fb 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -67,6 +67,7 @@ codex-chatgpt = { path = "chatgpt" } codex-common = { path = "common" } codex-core = { path = "core" } codex-exec = { path = "exec" } +codex-execpolicy2 = { path = "execpolicy2" } codex-feedback = { path = "feedback" } codex-file-search = { path = "file-search" } codex-git = { path = "utils/git" } diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index 4d8f43778c..db06f6b795 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -22,6 +22,7 @@ chrono = { workspace = true, features = ["serde"] } codex-app-server-protocol = { workspace = true } codex-apply-patch = { workspace = true } codex-async-utils = { workspace = true } +codex-execpolicy2 = { workspace = true } codex-file-search = { workspace = true } codex-git = { workspace = true } codex-keyring-store = { workspace = true } diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 32589dabb3..897f1f2944 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -121,6 +121,7 @@ use crate::user_instructions::UserInstructions; use crate::user_notification::UserNotification; use crate::util::backoff; use codex_async_utils::OrCancelExt; +use codex_execpolicy2::Policy as ExecPolicy; use codex_otel::otel_event_manager::OtelEventManager; use codex_protocol::config_types::ReasoningEffort as ReasoningEffortConfig; use codex_protocol::config_types::ReasoningSummary as ReasoningSummaryConfig; @@ -167,6 +168,10 @@ impl Codex { let user_instructions = get_user_instructions(&config).await; + let exec_policy = crate::exec_policy::exec_policy_for(&config.features, &config.codex_home) + .await + .map_err(|err| CodexErr::Fatal(format!("failed to load execpolicy: {err}")))?; + let config = Arc::new(config); let session_configuration = SessionConfiguration { @@ -183,6 +188,7 @@ impl Codex { cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), features: config.features.clone(), + exec_policy, session_source, }; @@ -280,6 +286,7 @@ pub(crate) struct TurnContext { pub(crate) final_output_json_schema: Option, pub(crate) codex_linux_sandbox_exe: Option, pub(crate) tool_call_gate: Arc, + pub(crate) exec_policy: Arc, pub(crate) truncation_policy: TruncationPolicy, } @@ -336,6 +343,8 @@ pub(crate) struct SessionConfiguration { /// Set of feature flags for this session features: Features, + /// Execpolicy policy, applied only when enabled by feature flag. + exec_policy: Arc, // TODO(pakrym): Remove config from here original_config_do_not_use: Arc, @@ -436,6 +445,7 @@ impl Session { final_output_json_schema: None, codex_linux_sandbox_exe: config.codex_linux_sandbox_exe.clone(), tool_call_gate: Arc::new(ReadinessFlag::new()), + exec_policy: session_configuration.exec_policy.clone(), truncation_policy: TruncationPolicy::new(&per_turn_config), } } @@ -1789,6 +1799,7 @@ async fn spawn_review_thread( final_output_json_schema: None, codex_linux_sandbox_exe: parent_turn_context.codex_linux_sandbox_exe.clone(), tool_call_gate: Arc::new(ReadinessFlag::new()), + exec_policy: parent_turn_context.exec_policy.clone(), truncation_policy: TruncationPolicy::new(&per_turn_config), }; @@ -2608,6 +2619,7 @@ mod tests { cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), features: Features::default(), + exec_policy: Arc::new(codex_execpolicy2::Policy::empty()), session_source: SessionSource::Exec, }; @@ -2685,6 +2697,7 @@ mod tests { cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), features: Features::default(), + exec_policy: Arc::new(codex_execpolicy2::Policy::empty()), session_source: SessionSource::Exec, }; diff --git a/codex-rs/core/src/command_safety/is_dangerous_command.rs b/codex-rs/core/src/command_safety/is_dangerous_command.rs index 09594bb1c5..5df2023f06 100644 --- a/codex-rs/core/src/command_safety/is_dangerous_command.rs +++ b/codex-rs/core/src/command_safety/is_dangerous_command.rs @@ -1,6 +1,8 @@ use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::SandboxPolicy; +use crate::sandboxing::SandboxPermissions; + use crate::bash::parse_shell_lc_plain_commands; use crate::is_safe_command::is_known_safe_command; @@ -8,7 +10,7 @@ pub fn requires_initial_appoval( policy: AskForApproval, sandbox_policy: &SandboxPolicy, command: &[String], - with_escalated_permissions: bool, + sandbox_permissions: SandboxPermissions, ) -> bool { if is_known_safe_command(command) { return false; @@ -24,8 +26,7 @@ pub fn requires_initial_appoval( // In restricted sandboxes (ReadOnly/WorkspaceWrite), do not prompt for // non‑escalated, non‑dangerous commands — let the sandbox enforce // restrictions (e.g., block network/write) without a user prompt. - let wants_escalation: bool = with_escalated_permissions; - if wants_escalation { + if sandbox_permissions.requires_escalated_permissions() { return true; } command_might_be_dangerous(command) diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs new file mode 100644 index 0000000000..c55dbfed23 --- /dev/null +++ b/codex-rs/core/src/exec_policy.rs @@ -0,0 +1,365 @@ +use std::io::ErrorKind; +use std::path::Path; +use std::path::PathBuf; +use std::sync::Arc; + +use crate::command_safety::is_dangerous_command::requires_initial_appoval; +use codex_execpolicy2::Decision; +use codex_execpolicy2::Evaluation; +use codex_execpolicy2::Policy; +use codex_execpolicy2::PolicyParser; +use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::SandboxPolicy; +use thiserror::Error; +use tokio::fs; + +use crate::bash::parse_shell_lc_plain_commands; +use crate::features::Feature; +use crate::features::Features; +use crate::sandboxing::SandboxPermissions; +use crate::tools::sandboxing::ApprovalRequirement; + +const FORBIDDEN_REASON: &str = "execpolicy forbids this command"; +const PROMPT_REASON: &str = "execpolicy requires approval for this command"; +const POLICY_DIR_NAME: &str = "policy"; +const POLICY_EXTENSION: &str = "codexpolicy"; + +#[derive(Debug, Error)] +pub enum ExecPolicyError { + #[error("failed to read execpolicy files from {dir}: {source}")] + ReadDir { + dir: PathBuf, + source: std::io::Error, + }, + + #[error("failed to read execpolicy file {path}: {source}")] + ReadFile { + path: PathBuf, + source: std::io::Error, + }, + + #[error("failed to parse execpolicy file {path}: {source}")] + ParsePolicy { + path: String, + source: codex_execpolicy2::Error, + }, +} + +pub(crate) async fn exec_policy_for( + features: &Features, + codex_home: &Path, +) -> Result, ExecPolicyError> { + if !features.enabled(Feature::ExecPolicy) { + return Ok(Arc::new(Policy::empty())); + } + + let policy_dir = codex_home.join(POLICY_DIR_NAME); + let policy_paths = collect_policy_files(&policy_dir).await?; + + let mut parser = PolicyParser::new(); + for policy_path in &policy_paths { + let contents = + fs::read_to_string(policy_path) + .await + .map_err(|source| ExecPolicyError::ReadFile { + path: policy_path.clone(), + source, + })?; + let identifier = policy_path.to_string_lossy().to_string(); + parser + .parse(&identifier, &contents) + .map_err(|source| ExecPolicyError::ParsePolicy { + path: identifier, + source, + })?; + } + + let policy = Arc::new(parser.build()); + tracing::debug!( + "loaded execpolicy from {} files in {}", + policy_paths.len(), + policy_dir.display() + ); + + Ok(policy) +} + +fn evaluate_with_policy( + policy: &Policy, + command: &[String], + approval_policy: AskForApproval, +) -> Option { + let commands = parse_shell_lc_plain_commands(command).unwrap_or_else(|| vec![command.to_vec()]); + let evaluation = policy.check_multiple(commands.iter()); + + match evaluation { + Evaluation::Match { decision, .. } => match decision { + Decision::Forbidden => Some(ApprovalRequirement::Forbidden { + reason: FORBIDDEN_REASON.to_string(), + }), + Decision::Prompt => { + let reason = PROMPT_REASON.to_string(); + if matches!(approval_policy, AskForApproval::Never) { + Some(ApprovalRequirement::Forbidden { reason }) + } else { + Some(ApprovalRequirement::NeedsApproval { + reason: Some(reason), + }) + } + } + Decision::Allow => Some(ApprovalRequirement::Skip), + }, + Evaluation::NoMatch => None, + } +} + +pub(crate) fn create_approval_requirement_for_command( + policy: &Policy, + command: &[String], + approval_policy: AskForApproval, + sandbox_policy: &SandboxPolicy, + sandbox_permissions: SandboxPermissions, +) -> ApprovalRequirement { + if let Some(requirement) = evaluate_with_policy(policy, command, approval_policy) { + return requirement; + } + + if requires_initial_appoval( + approval_policy, + sandbox_policy, + command, + sandbox_permissions, + ) { + ApprovalRequirement::NeedsApproval { reason: None } + } else { + ApprovalRequirement::Skip + } +} + +async fn collect_policy_files(dir: &Path) -> Result, ExecPolicyError> { + let mut read_dir = match fs::read_dir(dir).await { + Ok(read_dir) => read_dir, + Err(err) if err.kind() == ErrorKind::NotFound => return Ok(Vec::new()), + Err(source) => { + return Err(ExecPolicyError::ReadDir { + dir: dir.to_path_buf(), + source, + }); + } + }; + + let mut policy_paths = Vec::new(); + while let Some(entry) = + read_dir + .next_entry() + .await + .map_err(|source| ExecPolicyError::ReadDir { + dir: dir.to_path_buf(), + source, + })? + { + let path = entry.path(); + let file_type = entry + .file_type() + .await + .map_err(|source| ExecPolicyError::ReadDir { + dir: dir.to_path_buf(), + source, + })?; + + if path + .extension() + .and_then(|ext| ext.to_str()) + .is_some_and(|ext| ext == POLICY_EXTENSION) + && file_type.is_file() + { + policy_paths.push(path); + } + } + + policy_paths.sort(); + + Ok(policy_paths) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::features::Feature; + use crate::features::Features; + use codex_protocol::protocol::AskForApproval; + use codex_protocol::protocol::SandboxPolicy; + use pretty_assertions::assert_eq; + use std::fs; + use tempfile::tempdir; + + #[tokio::test] + async fn returns_empty_policy_when_feature_disabled() { + let mut features = Features::with_defaults(); + features.disable(Feature::ExecPolicy); + let temp_dir = tempdir().expect("create temp dir"); + + let policy = exec_policy_for(&features, temp_dir.path()) + .await + .expect("policy result"); + + let commands = [vec!["rm".to_string()]]; + assert!(matches!( + policy.check_multiple(commands.iter()), + Evaluation::NoMatch + )); + assert!(!temp_dir.path().join(POLICY_DIR_NAME).exists()); + } + + #[tokio::test] + async fn collect_policy_files_returns_empty_when_dir_missing() { + let temp_dir = tempdir().expect("create temp dir"); + + let policy_dir = temp_dir.path().join(POLICY_DIR_NAME); + let files = collect_policy_files(&policy_dir) + .await + .expect("collect policy files"); + + assert!(files.is_empty()); + } + + #[tokio::test] + async fn loads_policies_from_policy_subdirectory() { + let temp_dir = tempdir().expect("create temp dir"); + let policy_dir = temp_dir.path().join(POLICY_DIR_NAME); + fs::create_dir_all(&policy_dir).expect("create policy dir"); + fs::write( + policy_dir.join("deny.codexpolicy"), + r#"prefix_rule(pattern=["rm"], decision="forbidden")"#, + ) + .expect("write policy file"); + + let policy = exec_policy_for(&Features::with_defaults(), temp_dir.path()) + .await + .expect("policy result"); + let command = [vec!["rm".to_string()]]; + assert!(matches!( + policy.check_multiple(command.iter()), + Evaluation::Match { .. } + )); + } + + #[tokio::test] + async fn ignores_policies_outside_policy_dir() { + let temp_dir = tempdir().expect("create temp dir"); + fs::write( + temp_dir.path().join("root.codexpolicy"), + r#"prefix_rule(pattern=["ls"], decision="prompt")"#, + ) + .expect("write policy file"); + + let policy = exec_policy_for(&Features::with_defaults(), temp_dir.path()) + .await + .expect("policy result"); + let command = [vec!["ls".to_string()]]; + assert!(matches!( + policy.check_multiple(command.iter()), + Evaluation::NoMatch + )); + } + + #[test] + fn evaluates_bash_lc_inner_commands() { + let policy_src = r#" +prefix_rule(pattern=["rm"], decision="forbidden") +"#; + let mut parser = PolicyParser::new(); + parser + .parse("test.codexpolicy", policy_src) + .expect("parse policy"); + let policy = parser.build(); + + let forbidden_script = vec![ + "bash".to_string(), + "-lc".to_string(), + "rm -rf /tmp".to_string(), + ]; + + let requirement = + evaluate_with_policy(&policy, &forbidden_script, AskForApproval::OnRequest) + .expect("expected match for forbidden command"); + + assert_eq!( + requirement, + ApprovalRequirement::Forbidden { + reason: FORBIDDEN_REASON.to_string() + } + ); + } + + #[test] + fn approval_requirement_prefers_execpolicy_match() { + let policy_src = r#"prefix_rule(pattern=["rm"], decision="prompt")"#; + let mut parser = PolicyParser::new(); + parser + .parse("test.codexpolicy", policy_src) + .expect("parse policy"); + let policy = parser.build(); + let command = vec!["rm".to_string()]; + + let requirement = create_approval_requirement_for_command( + &policy, + &command, + AskForApproval::OnRequest, + &SandboxPolicy::DangerFullAccess, + SandboxPermissions::UseDefault, + ); + + assert_eq!( + requirement, + ApprovalRequirement::NeedsApproval { + reason: Some(PROMPT_REASON.to_string()) + } + ); + } + + #[test] + fn approval_requirement_respects_approval_policy() { + let policy_src = r#"prefix_rule(pattern=["rm"], decision="prompt")"#; + let mut parser = PolicyParser::new(); + parser + .parse("test.codexpolicy", policy_src) + .expect("parse policy"); + let policy = parser.build(); + let command = vec!["rm".to_string()]; + + let requirement = create_approval_requirement_for_command( + &policy, + &command, + AskForApproval::Never, + &SandboxPolicy::DangerFullAccess, + SandboxPermissions::UseDefault, + ); + + assert_eq!( + requirement, + ApprovalRequirement::Forbidden { + reason: PROMPT_REASON.to_string() + } + ); + } + + #[test] + fn approval_requirement_falls_back_to_heuristics() { + let command = vec!["python".to_string()]; + + let empty_policy = Policy::empty(); + let requirement = create_approval_requirement_for_command( + &empty_policy, + &command, + AskForApproval::UnlessTrusted, + &SandboxPolicy::ReadOnly, + SandboxPermissions::UseDefault, + ); + + assert_eq!( + requirement, + ApprovalRequirement::NeedsApproval { reason: None } + ); + } +} diff --git a/codex-rs/core/src/features.rs b/codex-rs/core/src/features.rs index 1a7fa21b45..75622e5254 100644 --- a/codex-rs/core/src/features.rs +++ b/codex-rs/core/src/features.rs @@ -42,6 +42,8 @@ pub enum Feature { ViewImageTool, /// Allow the model to request web searches. WebSearchRequest, + /// Gate the execpolicy enforcement for shell/unified exec. + ExecPolicy, /// Enable the model-based risk assessments for sandboxed commands. SandboxCommandAssessment, /// Enable Windows sandbox (restricted token) on Windows. @@ -297,6 +299,12 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::Stable, default_enabled: false, }, + FeatureSpec { + id: Feature::ExecPolicy, + key: "exec_policy", + stage: Stage::Experimental, + default_enabled: true, + }, FeatureSpec { id: Feature::SandboxCommandAssessment, key: "experimental_sandbox_command_assessment", diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 3e74633456..2ae11d79d0 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -25,6 +25,7 @@ mod environment_context; pub mod error; pub mod exec; pub mod exec_env; +mod exec_policy; pub mod features; mod flags; pub mod git_info; diff --git a/codex-rs/core/src/sandboxing/mod.rs b/codex-rs/core/src/sandboxing/mod.rs index 5e564f5109..4ecb2a8c12 100644 --- a/codex-rs/core/src/sandboxing/mod.rs +++ b/codex-rs/core/src/sandboxing/mod.rs @@ -26,6 +26,28 @@ use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum SandboxPermissions { + UseDefault, + RequireEscalated, +} + +impl SandboxPermissions { + pub fn requires_escalated_permissions(self) -> bool { + matches!(self, SandboxPermissions::RequireEscalated) + } +} + +impl From for SandboxPermissions { + fn from(with_escalated_permissions: bool) -> Self { + if with_escalated_permissions { + SandboxPermissions::RequireEscalated + } else { + SandboxPermissions::UseDefault + } + } +} + #[derive(Clone, Debug)] pub struct CommandSpec { pub program: String, diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index b7d6f7511d..fcd2f5b0c9 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -9,9 +9,11 @@ use crate::apply_patch::convert_apply_patch_to_protocol; use crate::codex::TurnContext; use crate::exec::ExecParams; use crate::exec_env::create_env; +use crate::exec_policy::create_approval_requirement_for_command; use crate::function_tool::FunctionCallError; use crate::is_safe_command::is_known_safe_command; use crate::protocol::ExecCommandSource; +use crate::sandboxing::SandboxPermissions; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; @@ -302,6 +304,13 @@ impl ShellHandler { env: exec_params.env.clone(), with_escalated_permissions: exec_params.with_escalated_permissions, justification: exec_params.justification.clone(), + approval_requirement: create_approval_requirement_for_command( + &turn.exec_policy, + &exec_params.command, + turn.approval_policy, + &turn.sandbox_policy, + SandboxPermissions::from(exec_params.with_escalated_permissions.unwrap_or(false)), + ), }; let mut orchestrator = ToolOrchestrator::new(); let mut runtime = ShellRuntime::new(); diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 878e48e8be..7e8e152f67 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -11,11 +11,13 @@ use crate::error::get_error_message_ui; use crate::exec::ExecToolCallOutput; use crate::sandboxing::SandboxManager; use crate::tools::sandboxing::ApprovalCtx; +use crate::tools::sandboxing::ApprovalRequirement; use crate::tools::sandboxing::ProvidesSandboxRetryData; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; +use crate::tools::sandboxing::default_approval_requirement; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ReviewDecision; @@ -49,40 +51,52 @@ impl ToolOrchestrator { let otel_cfg = codex_otel::otel_event_manager::ToolDecisionSource::Config; // 1) Approval - let needs_initial_approval = - tool.wants_initial_approval(req, approval_policy, &turn_ctx.sandbox_policy); let mut already_approved = false; - if needs_initial_approval { - let mut risk = None; - - if let Some(metadata) = req.sandbox_retry_data() { - risk = tool_ctx - .session - .assess_sandbox_command(turn_ctx, &tool_ctx.call_id, &metadata.command, None) - .await; + let requirement = tool.approval_requirement(req).unwrap_or_else(|| { + default_approval_requirement(approval_policy, &turn_ctx.sandbox_policy) + }); + match requirement { + ApprovalRequirement::Skip => { + otel.tool_decision(otel_tn, otel_ci, ReviewDecision::Approved, otel_cfg); } + ApprovalRequirement::Forbidden { reason } => { + return Err(ToolError::Rejected(reason)); + } + ApprovalRequirement::NeedsApproval { reason } => { + let mut risk = None; - let approval_ctx = ApprovalCtx { - session: tool_ctx.session, - turn: turn_ctx, - call_id: &tool_ctx.call_id, - retry_reason: None, - risk, - }; - let decision = tool.start_approval_async(req, approval_ctx).await; - - otel.tool_decision(otel_tn, otel_ci, decision, otel_user.clone()); - - match decision { - ReviewDecision::Denied | ReviewDecision::Abort => { - return Err(ToolError::Rejected("rejected by user".to_string())); + if let Some(metadata) = req.sandbox_retry_data() { + risk = tool_ctx + .session + .assess_sandbox_command( + turn_ctx, + &tool_ctx.call_id, + &metadata.command, + None, + ) + .await; } - ReviewDecision::Approved | ReviewDecision::ApprovedForSession => {} + + let approval_ctx = ApprovalCtx { + session: tool_ctx.session, + turn: turn_ctx, + call_id: &tool_ctx.call_id, + retry_reason: reason, + risk, + }; + let decision = tool.start_approval_async(req, approval_ctx).await; + + otel.tool_decision(otel_tn, otel_ci, decision, otel_user.clone()); + + match decision { + ReviewDecision::Denied | ReviewDecision::Abort => { + return Err(ToolError::Rejected("rejected by user".to_string())); + } + ReviewDecision::Approved | ReviewDecision::ApprovedForSession => {} + } + already_approved = true; } - already_approved = true; - } else { - otel.tool_decision(otel_tn, otel_ci, ReviewDecision::Approved, otel_cfg); } // 2) First attempt under the selected sandbox. diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index bf7ae7fa3b..d71c4498e6 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -4,13 +4,12 @@ Runtime: shell Executes shell requests under the orchestrator: asks for approval when needed, builds a CommandSpec, and runs it under the current SandboxAttempt. */ -use crate::command_safety::is_dangerous_command::requires_initial_appoval; use crate::exec::ExecToolCallOutput; -use crate::protocol::SandboxPolicy; use crate::sandboxing::execute_env; use crate::tools::runtimes::build_command_spec; use crate::tools::sandboxing::Approvable; use crate::tools::sandboxing::ApprovalCtx; +use crate::tools::sandboxing::ApprovalRequirement; use crate::tools::sandboxing::ProvidesSandboxRetryData; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::SandboxRetryData; @@ -20,7 +19,6 @@ use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; use crate::tools::sandboxing::with_cached_approval; -use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ReviewDecision; use futures::future::BoxFuture; use std::path::PathBuf; @@ -33,6 +31,7 @@ pub struct ShellRequest { pub env: std::collections::HashMap, pub with_escalated_permissions: Option, pub justification: Option, + pub approval_requirement: ApprovalRequirement, } impl ProvidesSandboxRetryData for ShellRequest { @@ -114,18 +113,8 @@ impl Approvable for ShellRuntime { }) } - fn wants_initial_approval( - &self, - req: &ShellRequest, - policy: AskForApproval, - sandbox_policy: &SandboxPolicy, - ) -> bool { - requires_initial_appoval( - policy, - sandbox_policy, - &req.command, - req.with_escalated_permissions.unwrap_or(false), - ) + fn approval_requirement(&self, req: &ShellRequest) -> Option { + Some(req.approval_requirement.clone()) } fn wants_escalated_first_attempt(&self, req: &ShellRequest) -> bool { diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index cddac1924e..5b18476bfc 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -1,4 +1,3 @@ -use crate::command_safety::is_dangerous_command::requires_initial_appoval; /* Runtime: unified exec @@ -10,6 +9,7 @@ use crate::error::SandboxErr; use crate::tools::runtimes::build_command_spec; use crate::tools::sandboxing::Approvable; use crate::tools::sandboxing::ApprovalCtx; +use crate::tools::sandboxing::ApprovalRequirement; use crate::tools::sandboxing::ProvidesSandboxRetryData; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::SandboxRetryData; @@ -22,9 +22,7 @@ use crate::tools::sandboxing::with_cached_approval; use crate::unified_exec::UnifiedExecError; use crate::unified_exec::UnifiedExecSession; use crate::unified_exec::UnifiedExecSessionManager; -use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ReviewDecision; -use codex_protocol::protocol::SandboxPolicy; use futures::future::BoxFuture; use std::collections::HashMap; use std::path::PathBuf; @@ -36,6 +34,7 @@ pub struct UnifiedExecRequest { pub env: HashMap, pub with_escalated_permissions: Option, pub justification: Option, + pub approval_requirement: ApprovalRequirement, } impl ProvidesSandboxRetryData for UnifiedExecRequest { @@ -65,6 +64,7 @@ impl UnifiedExecRequest { env: HashMap, with_escalated_permissions: Option, justification: Option, + approval_requirement: ApprovalRequirement, ) -> Self { Self { command, @@ -72,6 +72,7 @@ impl UnifiedExecRequest { env, with_escalated_permissions, justification, + approval_requirement, } } } @@ -129,18 +130,8 @@ impl Approvable for UnifiedExecRuntime<'_> { }) } - fn wants_initial_approval( - &self, - req: &UnifiedExecRequest, - policy: AskForApproval, - sandbox_policy: &SandboxPolicy, - ) -> bool { - requires_initial_appoval( - policy, - sandbox_policy, - &req.command, - req.with_escalated_permissions.unwrap_or(false), - ) + fn approval_requirement(&self, req: &UnifiedExecRequest) -> Option { + Some(req.approval_requirement.clone()) } fn wants_escalated_first_attempt(&self, req: &UnifiedExecRequest) -> bool { diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index da1c22b549..e694c7fbef 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -86,6 +86,37 @@ pub(crate) struct ApprovalCtx<'a> { pub risk: Option, } +// Specifies what tool orchestrator should do with a given tool call. +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) enum ApprovalRequirement { + /// No approval required for this tool call + Skip, + /// Approval required for this tool call + NeedsApproval { reason: Option }, + /// Execution forbidden for this tool call + Forbidden { reason: String }, +} + +/// - Never, OnFailure: do not ask +/// - OnRequest: ask unless sandbox policy is DangerFullAccess +/// - UnlessTrusted: always ask +pub(crate) fn default_approval_requirement( + policy: AskForApproval, + sandbox_policy: &SandboxPolicy, +) -> ApprovalRequirement { + let needs_approval = match policy { + AskForApproval::Never | AskForApproval::OnFailure => false, + AskForApproval::OnRequest => !matches!(sandbox_policy, SandboxPolicy::DangerFullAccess), + AskForApproval::UnlessTrusted => true, + }; + + if needs_approval { + ApprovalRequirement::NeedsApproval { reason: None } + } else { + ApprovalRequirement::Skip + } +} + pub(crate) trait Approvable { type ApprovalKey: Hash + Eq + Clone + Debug + Serialize; @@ -106,22 +137,11 @@ pub(crate) trait Approvable { matches!(policy, AskForApproval::Never) } - /// Decide whether an initial user approval should be requested before the - /// first attempt. Defaults to the orchestrator's behavior (pre‑refactor): - /// - Never, OnFailure: do not ask - /// - OnRequest: ask unless sandbox policy is DangerFullAccess - /// - UnlessTrusted: always ask - fn wants_initial_approval( - &self, - _req: &Req, - policy: AskForApproval, - sandbox_policy: &SandboxPolicy, - ) -> bool { - match policy { - AskForApproval::Never | AskForApproval::OnFailure => false, - AskForApproval::OnRequest => !matches!(sandbox_policy, SandboxPolicy::DangerFullAccess), - AskForApproval::UnlessTrusted => true, - } + /// Override the default approval requirement. Return `Some(_)` to specify + /// a custom requirement, or `None` to fall back to + /// policy-based default. + fn approval_requirement(&self, _req: &Req) -> Option { + None } /// Decide we can request an approval for no-sandbox execution. diff --git a/codex-rs/core/src/unified_exec/session_manager.rs b/codex-rs/core/src/unified_exec/session_manager.rs index f6dce8644c..93340bb2d4 100644 --- a/codex-rs/core/src/unified_exec/session_manager.rs +++ b/codex-rs/core/src/unified_exec/session_manager.rs @@ -11,10 +11,12 @@ use crate::codex::TurnContext; use crate::exec::ExecToolCallOutput; use crate::exec::StreamOutput; use crate::exec_env::create_env; +use crate::exec_policy::create_approval_requirement_for_command; use crate::protocol::BackgroundEventEvent; use crate::protocol::EventMsg; use crate::protocol::ExecCommandSource; use crate::sandboxing::ExecEnv; +use crate::sandboxing::SandboxPermissions; use crate::tools::events::ToolEmitter; use crate::tools::events::ToolEventCtx; use crate::tools::events::ToolEventFailure; @@ -449,6 +451,13 @@ impl UnifiedExecSessionManager { create_env(&context.turn.shell_environment_policy), with_escalated_permissions, justification, + create_approval_requirement_for_command( + &context.turn.exec_policy, + command, + context.turn.approval_policy, + &context.turn.sandbox_policy, + SandboxPermissions::from(with_escalated_permissions.unwrap_or(false)), + ), ); let tool_ctx = ToolCtx { session: context.session.as_ref(), diff --git a/codex-rs/core/tests/suite/exec_policy.rs b/codex-rs/core/tests/suite/exec_policy.rs new file mode 100644 index 0000000000..bca917a252 --- /dev/null +++ b/codex-rs/core/tests/suite/exec_policy.rs @@ -0,0 +1,101 @@ +#![allow(clippy::unwrap_used, clippy::expect_used)] + +use anyhow::Result; +use codex_core::protocol::AskForApproval; +use codex_core::protocol::EventMsg; +use codex_core::protocol::Op; +use codex_core::protocol::SandboxPolicy; +use codex_protocol::config_types::ReasoningSummary; +use codex_protocol::user_input::UserInput; +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_sse_once; +use core_test_support::responses::sse; +use core_test_support::responses::start_mock_server; +use core_test_support::test_codex::test_codex; +use core_test_support::wait_for_event; +use serde_json::json; +use std::fs; + +#[tokio::test] +async fn execpolicy_blocks_shell_invocation() -> Result<()> { + let mut builder = test_codex().with_config(|config| { + let policy_path = config.codex_home.join("policy").join("policy.codexpolicy"); + fs::create_dir_all( + policy_path + .parent() + .expect("policy directory must have a parent"), + ) + .expect("create policy directory"); + fs::write( + &policy_path, + r#"prefix_rule(pattern=["echo"], decision="forbidden")"#, + ) + .expect("write policy file"); + }); + let server = start_mock_server().await; + let test = builder.build(&server).await?; + + let call_id = "shell-forbidden"; + let args = json!({ + "command": ["echo", "blocked"], + "timeout_ms": 1_000, + }); + + mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-1"), + ev_function_call(call_id, "shell", &serde_json::to_string(&args)?), + ev_completed("resp-1"), + ]), + ) + .await; + mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ]), + ) + .await; + + let session_model = test.session_configured.model.clone(); + test.codex + .submit(Op::UserTurn { + items: vec![UserInput::Text { + text: "run shell command".into(), + }], + final_output_json_schema: None, + cwd: test.cwd_path().to_path_buf(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::DangerFullAccess, + model: session_model, + effort: None, + summary: ReasoningSummary::Auto, + }) + .await?; + + let EventMsg::ExecCommandEnd(end) = wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::ExecCommandEnd(_)) + }) + .await + else { + unreachable!() + }; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TaskComplete(_)) + }) + .await; + + assert!( + end.aggregated_output + .contains("execpolicy forbids this command"), + "unexpected output: {}", + end.aggregated_output + ); + + Ok(()) +} diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index ef248901e5..60b828b1c3 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -28,6 +28,7 @@ mod compact_remote; mod compact_resume_fork; mod deprecation_notice; mod exec; +mod exec_policy; mod fork_conversation; mod grep_files; mod items; diff --git a/codex-rs/execpolicy2/README.md b/codex-rs/execpolicy2/README.md index 8cf5302fb6..d77ec3838d 100644 --- a/codex-rs/execpolicy2/README.md +++ b/codex-rs/execpolicy2/README.md @@ -5,7 +5,7 @@ - This release covers only the prefix-rule subset of the planned execpolicy v2 language; a richer language will follow. - Tokens are matched in order; any `pattern` element may be a list to denote alternatives. `decision` defaults to `allow`; valid values: `allow`, `prompt`, `forbidden`. - `match` / `not_match` supply example invocations that are validated at load time (think of them as unit tests); examples can be token arrays or strings (strings are tokenized with `shlex`). -- The CLI always prints the JSON serialization of the evaluation result (whether a match or not). +- The CLI always prints the JSON serialization of the evaluation result. ## Policy shapes - Prefix rules use Starlark syntax: @@ -18,6 +18,20 @@ prefix_rule( ) ``` +## CLI +- Provide one or more policy files (for example `src/default.codexpolicy`) to check a command: +```bash +cargo run -p codex-execpolicy2 -- check --policy path/to/policy.codexpolicy git status +``` +- Pass multiple `--policy` flags to merge rules, evaluated in the order provided: +```bash +cargo run -p codex-execpolicy2 -- check --policy base.codexpolicy --policy overrides.codexpolicy git status +``` +- Output is JSON by default; pass `--pretty` for pretty-printed JSON +- Example outcomes: + - Match: `{"match": { ... "decision": "allow" ... }}` + - No match: `"noMatch"` + ## Response shapes - Match: ```json @@ -43,17 +57,3 @@ prefix_rule( - `matchedRules` lists every rule whose prefix matched the command; `matchedPrefix` is the exact prefix that matched. - The effective `decision` is the strictest severity across all matches (`forbidden` > `prompt` > `allow`). - -## CLI -- Provide one or more policy files (for example `src/default.codexpolicy`) to check a command: -```bash -cargo run -p codex-execpolicy2 -- check --policy path/to/policy.codexpolicy git status -``` -- Pass multiple `--policy` flags to merge rules, evaluated in the order provided: -```bash -cargo run -p codex-execpolicy2 -- check --policy base.codexpolicy --policy overrides.codexpolicy git status -``` -- Output is newline-delimited JSON by default; pass `--pretty` for pretty-printed JSON if desired. -- Example outcomes: - - Match: `{"match": { ... "decision": "allow" ... }}` - - No match: `"noMatch"` diff --git a/codex-rs/execpolicy2/src/policy.rs b/codex-rs/execpolicy2/src/policy.rs index 12416b050b..a543e5c4b0 100644 --- a/codex-rs/execpolicy2/src/policy.rs +++ b/codex-rs/execpolicy2/src/policy.rs @@ -15,6 +15,10 @@ impl Policy { Self { rules_by_program } } + pub fn empty() -> Self { + Self::new(MultiMap::new()) + } + pub fn rules(&self) -> &MultiMap { &self.rules_by_program } diff --git a/codex-rs/tui/src/chatwidget/agent.rs b/codex-rs/tui/src/chatwidget/agent.rs index bf15b6c4ad..0abddac50d 100644 --- a/codex-rs/tui/src/chatwidget/agent.rs +++ b/codex-rs/tui/src/chatwidget/agent.rs @@ -4,6 +4,9 @@ use codex_core::CodexConversation; use codex_core::ConversationManager; use codex_core::NewConversation; use codex_core::config::Config; +use codex_core::protocol::ErrorEvent; +use codex_core::protocol::Event; +use codex_core::protocol::EventMsg; use codex_core::protocol::Op; use tokio::sync::mpsc::UnboundedSender; use tokio::sync::mpsc::unbounded_channel; @@ -28,9 +31,19 @@ pub(crate) fn spawn_agent( session_configured, } = match server.new_conversation(config).await { Ok(v) => v, - Err(e) => { - // TODO: surface this error to the user. - tracing::error!("failed to initialize codex: {e}"); + #[allow(clippy::print_stderr)] + Err(err) => { + let message = err.to_string(); + eprintln!("{message}"); + app_event_tx_clone.send(AppEvent::CodexEvent(Event { + id: "".to_string(), + msg: EventMsg::Error(ErrorEvent { + message, + http_status_code: None, + }), + })); + app_event_tx_clone.send(AppEvent::ExitRequest); + tracing::error!("failed to initialize codex: {err}"); return; } }; From 92e304673374c5da74673ee0a80ca3169897830e Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Wed, 19 Nov 2025 16:56:37 -0800 Subject: [PATCH 2/9] Single pass truncation (#6914) --- codex-rs/core/src/truncate.rs | 114 +++++++++++++++++++++++----------- 1 file changed, 79 insertions(+), 35 deletions(-) diff --git a/codex-rs/core/src/truncate.rs b/codex-rs/core/src/truncate.rs index 7756b0c09a..bf883c0612 100644 --- a/codex-rs/core/src/truncate.rs +++ b/codex-rs/core/src/truncate.rs @@ -185,6 +185,7 @@ fn truncate_with_byte_estimate(s: &str, policy: TruncationPolicy) -> String { if s.is_empty() { return String::new(); } + let total_chars = s.chars().count(); let max_bytes = policy.byte_budget(); @@ -204,24 +205,55 @@ fn truncate_with_byte_estimate(s: &str, policy: TruncationPolicy) -> String { let total_bytes = s.len(); let (left_budget, right_budget) = split_budget(max_bytes); - let prefix_end = pick_prefix_end(s, left_budget); - let mut suffix_start = pick_suffix_start(s, right_budget); - if suffix_start < prefix_end { - suffix_start = prefix_end; - } - let left_chars = s[..prefix_end].chars().count(); - let right_chars = s[suffix_start..].chars().count(); - let removed_chars = total_chars - .saturating_sub(left_chars) - .saturating_sub(right_chars); + let (removed_chars, left, right) = split_string(s, left_budget, right_budget); let marker = format_truncation_marker( policy, removed_units_for_source(policy, total_bytes.saturating_sub(max_bytes), removed_chars), ); - assemble_truncated_output(&s[..prefix_end], &s[suffix_start..], &marker) + assemble_truncated_output(left, right, &marker) +} + +fn split_string(s: &str, beginning_bytes: usize, end_bytes: usize) -> (usize, &str, &str) { + if s.is_empty() { + return (0, "", ""); + } + + let len = s.len(); + let tail_start_target = len.saturating_sub(end_bytes); + let mut prefix_end = 0usize; + let mut suffix_start = len; + let mut removed_chars = 0usize; + let mut suffix_started = false; + + for (idx, ch) in s.char_indices() { + let char_end = idx + ch.len_utf8(); + if char_end <= beginning_bytes { + prefix_end = char_end; + continue; + } + + if idx >= tail_start_target { + if !suffix_started { + suffix_start = idx; + suffix_started = true; + } + continue; + } + + removed_chars = removed_chars.saturating_add(1); + } + + if suffix_start < prefix_end { + suffix_start = prefix_end; + } + + let before = &s[..prefix_end]; + let after = &s[suffix_start..]; + + (removed_chars, before, after) } fn format_truncation_marker(policy: TruncationPolicy, removed_count: u64) -> String { @@ -270,42 +302,54 @@ fn approx_tokens_from_byte_count(bytes: usize) -> u64 { / (APPROX_BYTES_PER_TOKEN as u64) } -fn truncate_on_boundary(input: &str, max_len: usize) -> &str { - if input.len() <= max_len { - return input; - } - let mut end = max_len; - while end > 0 && !input.is_char_boundary(end) { - end -= 1; - } - &input[..end] -} - -fn pick_prefix_end(s: &str, left_budget: usize) -> usize { - truncate_on_boundary(s, left_budget).len() -} - -fn pick_suffix_start(s: &str, right_budget: usize) -> usize { - let start_tail = s.len().saturating_sub(right_budget); - let mut idx = start_tail.min(s.len()); - while idx < s.len() && !s.is_char_boundary(idx) { - idx += 1; - } - idx -} - #[cfg(test)] mod tests { use super::TruncationPolicy; use super::approx_token_count; use super::formatted_truncate_text; + use super::split_string; use super::truncate_function_output_items_with_policy; use super::truncate_text; use super::truncate_with_token_budget; use codex_protocol::models::FunctionCallOutputContentItem; use pretty_assertions::assert_eq; + #[test] + fn split_string_works() { + assert_eq!(split_string("hello world", 5, 5), (1, "hello", "world")); + assert_eq!(split_string("abc", 0, 0), (3, "", "")); + } + + #[test] + fn split_string_handles_empty_string() { + assert_eq!(split_string("", 4, 4), (0, "", "")); + } + + #[test] + fn split_string_only_keeps_prefix_when_tail_budget_is_zero() { + assert_eq!(split_string("abcdef", 3, 0), (3, "abc", "")); + } + + #[test] + fn split_string_only_keeps_suffix_when_prefix_budget_is_zero() { + assert_eq!(split_string("abcdef", 0, 3), (3, "", "def")); + } + + #[test] + fn split_string_handles_overlapping_budgets_without_removal() { + assert_eq!(split_string("abcdef", 4, 4), (0, "abcd", "ef")); + } + + #[test] + fn split_string_respects_utf8_boundaries() { + assert_eq!(split_string("😀abc😀", 5, 5), (1, "😀a", "c😀")); + + assert_eq!(split_string("😀😀😀😀😀", 1, 1), (5, "", "")); + assert_eq!(split_string("😀😀😀😀😀", 7, 7), (3, "😀", "😀")); + assert_eq!(split_string("😀😀😀😀😀", 8, 8), (1, "😀😀", "😀😀")); + } + #[test] fn truncate_bytes_less_than_placeholder_returns_placeholder() { let content = "example output"; From 74a75679d993908ee90d7194aec02386d507c8de Mon Sep 17 00:00:00 2001 From: zhao-oai Date: Wed, 19 Nov 2025 16:57:27 -0800 Subject: [PATCH 3/9] update execpolicy quickstart readme (#6952) --- README.md | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index e34afbd288..4fed276405 100644 --- a/README.md +++ b/README.md @@ -69,12 +69,12 @@ Codex can access MCP servers. To configure them, refer to the [config docs](./do Codex CLI supports a rich set of configuration options, with preferences stored in `~/.codex/config.toml`. For full configuration options, see [Configuration](./docs/config.md). -### Execpolicy quickstart +### Execpolicy Quickstart Codex can enforce your own rules-based execution policy before it runs shell commands. 1. Create a policy directory: `mkdir -p ~/.codex/policy`. -2. Create one or more `.codexpolicy` files into that folder. Codex automatically loads every `.codexpolicy` file in there on startup. +2. Create one or more `.codexpolicy` files in that folder. Codex automatically loads every `.codexpolicy` file in there on startup. 3. Write `prefix_rule` entries to describe the commands you want to allow, prompt, or block: ```starlark @@ -87,14 +87,12 @@ prefix_rule( ``` - `pattern` is a list of shell tokens, evaluated from left to right; wrap tokens in a nested list to express alternatives (e.g., match both `push` and `fetch`). -- `decision` sets the severity; Codex picks the strictest decision when multiple rules match. +- `decision` sets the severity; Codex picks the strictest decision when multiple rules match (forbidden > prompt > allow). - `match` and `not_match` act as (optional) unit tests. Codex validates them when it loads your policy, so you get feedback if an example has unexpected behavior. In this example rule, if Codex wants to run commands with the prefix `git push` or `git fetch`, it will first ask for user approval. -Note: If Codex wants to run a command that matches with multiple rules, it will use the strictest decision among the matched rules (forbidden > prompt > allow). - -Use the [`execpolicy2` CLI](./codex-rs/execpolicy2/README.md) to preview decisions before you save a rule: +Use [`execpolicy2` CLI](./codex-rs/execpolicy2/README.md) to preview decisions for policy files: ```shell cargo run -p codex-execpolicy2 -- check --policy ~/.codex/policy/default.codexpolicy git push origin main From 6d67b8b2832e614086bb9af41fe337f668507f5a Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Wed, 19 Nov 2025 17:17:04 -0800 Subject: [PATCH 4/9] stop model migration screen after first time. (#6954) it got serialized wrong. --- codex-rs/core/src/config/types.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/codex-rs/core/src/config/types.rs b/codex-rs/core/src/config/types.rs index b7f16a9028..8f5077410e 100644 --- a/codex-rs/core/src/config/types.rs +++ b/codex-rs/core/src/config/types.rs @@ -379,6 +379,7 @@ pub struct Notice { /// Tracks whether the user has seen the model migration prompt pub hide_gpt5_1_migration_prompt: Option, /// Tracks whether the user has seen the gpt-5.1-codex-max migration prompt + #[serde(rename = "hide_gpt-5.1-codex-max_migration_prompt")] pub hide_gpt_5_1_codex_max_migration_prompt: Option, } From 72a1453ac5b4ee60f19876542b7d7203e83cde62 Mon Sep 17 00:00:00 2001 From: Celia Chen Date: Wed, 19 Nov 2025 17:26:14 -0800 Subject: [PATCH 5/9] Revert "[core] add optional status_code to error events (#6865)" (#6955) This reverts commit c2ec477d939af2a275255666b37fe9b0acd7492a. # External (non-OpenAI) Pull Request Requirements Before opening this Pull Request, please read the dedicated "Contributing" markdown file or your PR may be closed: https://github.com/openai/codex/blob/main/docs/contributing.md If your PR conforms to our contribution guidelines, replace this text with a detailed and high quality description of your changes. Include a link to a bug report or enhancement request. --- codex-rs/core/src/codex.rs | 13 ++-- codex-rs/core/src/compact.rs | 14 ++-- codex-rs/core/src/compact_remote.rs | 7 +- codex-rs/core/src/error.rs | 71 ------------------- codex-rs/docs/protocol_v1.md | 2 +- .../src/event_processor_with_human_output.rs | 4 +- .../tests/event_processor_with_json_output.rs | 3 - codex-rs/protocol/src/protocol.rs | 4 -- codex-rs/tui/src/chatwidget.rs | 6 +- codex-rs/tui/src/chatwidget/agent.rs | 5 +- codex-rs/tui/src/chatwidget/tests.rs | 1 - 11 files changed, 25 insertions(+), 105 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 897f1f2944..c793eff352 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -66,7 +66,6 @@ use crate::context_manager::ContextManager; use crate::environment_context::EnvironmentContext; use crate::error::CodexErr; use crate::error::Result as CodexResult; -use crate::error::http_status_code_value; #[cfg(test)] use crate::exec::StreamOutput; use crate::mcp::auth::compute_auth_statuses; @@ -80,6 +79,7 @@ use crate::protocol::ApplyPatchApprovalRequestEvent; use crate::protocol::AskForApproval; use crate::protocol::BackgroundEventEvent; use crate::protocol::DeprecationNoticeEvent; +use crate::protocol::ErrorEvent; use crate::protocol::Event; use crate::protocol::EventMsg; use crate::protocol::ExecApprovalRequestEvent; @@ -134,7 +134,6 @@ use codex_protocol::user_input::UserInput; use codex_utils_readiness::Readiness; use codex_utils_readiness::ReadinessFlag; use codex_utils_tokenizer::warm_model_cache; -use reqwest::StatusCode; /// The high-level interface to the Codex system. /// It operates as a queue pair where you send submissions and receive events. @@ -1197,11 +1196,9 @@ impl Session { &self, turn_context: &TurnContext, message: impl Into, - http_status_code: Option, ) { let event = EventMsg::StreamError(StreamErrorEvent { message: message.into(), - http_status_code: http_status_code_value(http_status_code), }); self.send_event(turn_context, event).await; } @@ -1693,7 +1690,6 @@ mod handlers { id: sub_id.clone(), msg: EventMsg::Error(ErrorEvent { message: "Failed to shutdown rollout recorder".to_string(), - http_status_code: None, }), }; sess.send_event_raw(event).await; @@ -1948,8 +1944,10 @@ pub(crate) async fn run_task( } Err(e) => { info!("Turn error: {e:#}"); - sess.send_event(&turn_context, EventMsg::Error(e.to_error_event(None))) - .await; + let event = EventMsg::Error(ErrorEvent { + message: e.to_string(), + }); + sess.send_event(&turn_context, event).await; // let the user continue the conversation break; } @@ -2073,7 +2071,6 @@ async fn run_turn( sess.notify_stream_error( &turn_context, format!("Reconnecting... {retries}/{max_retries}"), - e.http_status_code(), ) .await; diff --git a/codex-rs/core/src/compact.rs b/codex-rs/core/src/compact.rs index 67b5b9342d..8c38f93937 100644 --- a/codex-rs/core/src/compact.rs +++ b/codex-rs/core/src/compact.rs @@ -10,6 +10,7 @@ use crate::error::Result as CodexResult; use crate::features::Feature; use crate::protocol::AgentMessageEvent; use crate::protocol::CompactedItem; +use crate::protocol::ErrorEvent; use crate::protocol::EventMsg; use crate::protocol::TaskStartedEvent; use crate::protocol::TurnContextItem; @@ -127,8 +128,10 @@ async fn run_compact_task_inner( continue; } sess.set_total_tokens_full(turn_context.as_ref()).await; - sess.send_event(&turn_context, EventMsg::Error(e.to_error_event(None))) - .await; + let event = EventMsg::Error(ErrorEvent { + message: e.to_string(), + }); + sess.send_event(&turn_context, event).await; return; } Err(e) => { @@ -138,14 +141,15 @@ async fn run_compact_task_inner( sess.notify_stream_error( turn_context.as_ref(), format!("Reconnecting... {retries}/{max_retries}"), - e.http_status_code(), ) .await; tokio::time::sleep(delay).await; continue; } else { - sess.send_event(&turn_context, EventMsg::Error(e.to_error_event(None))) - .await; + let event = EventMsg::Error(ErrorEvent { + message: e.to_string(), + }); + sess.send_event(&turn_context, event).await; return; } } diff --git a/codex-rs/core/src/compact_remote.rs b/codex-rs/core/src/compact_remote.rs index aa32942dce..0d2e0f138d 100644 --- a/codex-rs/core/src/compact_remote.rs +++ b/codex-rs/core/src/compact_remote.rs @@ -6,6 +6,7 @@ use crate::codex::TurnContext; use crate::error::Result as CodexResult; use crate::protocol::AgentMessageEvent; use crate::protocol::CompactedItem; +use crate::protocol::ErrorEvent; use crate::protocol::EventMsg; use crate::protocol::RolloutItem; use crate::protocol::TaskStartedEvent; @@ -29,8 +30,10 @@ pub(crate) async fn run_remote_compact_task(sess: Arc, turn_context: Ar async fn run_remote_compact_task_inner(sess: &Arc, turn_context: &Arc) { if let Err(err) = run_remote_compact_task_inner_impl(sess, turn_context).await { - let event = err.to_error_event(Some("Error running remote compact task".to_string())); - sess.send_event(turn_context, EventMsg::Error(event)).await; + let event = EventMsg::Error(ErrorEvent { + message: format!("Error running remote compact task: {err}"), + }); + sess.send_event(turn_context, event).await; } } diff --git a/codex-rs/core/src/error.rs b/codex-rs/core/src/error.rs index d20d5cfb46..b2027dc940 100644 --- a/codex-rs/core/src/error.rs +++ b/codex-rs/core/src/error.rs @@ -10,7 +10,6 @@ use chrono::Local; use chrono::Utc; use codex_async_utils::CancelErr; use codex_protocol::ConversationId; -use codex_protocol::protocol::ErrorEvent; use codex_protocol::protocol::RateLimitSnapshot; use reqwest::StatusCode; use serde_json; @@ -431,37 +430,6 @@ impl CodexErr { pub fn downcast_ref(&self) -> Option<&T> { (self as &dyn std::any::Any).downcast_ref::() } - - pub fn http_status_code(&self) -> Option { - match self { - CodexErr::UnexpectedStatus(err) => Some(err.status), - CodexErr::RetryLimit(err) => Some(err.status), - CodexErr::UsageLimitReached(_) | CodexErr::UsageNotIncluded => { - Some(StatusCode::TOO_MANY_REQUESTS) - } - CodexErr::InternalServerError => Some(StatusCode::INTERNAL_SERVER_ERROR), - CodexErr::ResponseStreamFailed(err) => err.source.status(), - CodexErr::ConnectionFailed(err) => err.source.status(), - _ => None, - } - } - - pub fn to_error_event(&self, message_prefix: Option) -> ErrorEvent { - let error_message = self.to_string(); - let message: String = match message_prefix { - Some(prefix) => format!("{prefix}: {error_message}"), - None => error_message, - }; - - ErrorEvent { - message, - http_status_code: http_status_code_value(self.http_status_code()), - } - } -} - -pub fn http_status_code_value(http_status_code: Option) -> Option { - http_status_code.as_ref().map(StatusCode::as_u16) } pub fn get_error_message_ui(e: &CodexErr) -> String { @@ -807,43 +775,4 @@ mod tests { assert_eq!(err.to_string(), expected); }); } - - #[test] - fn error_event_includes_http_status_code_when_available() { - let err = CodexErr::UnexpectedStatus(UnexpectedResponseError { - status: StatusCode::BAD_REQUEST, - body: "oops".to_string(), - request_id: Some("req-1".to_string()), - }); - let event = err.to_error_event(None); - - assert_eq!( - event.message, - "unexpected status 400 Bad Request: oops, request id: req-1" - ); - assert_eq!( - event.http_status_code, - Some(StatusCode::BAD_REQUEST.as_u16()) - ); - } - - #[test] - fn error_event_omits_http_status_code_when_unknown() { - let event = CodexErr::Fatal("boom".to_string()).to_error_event(None); - - assert_eq!(event.message, "Fatal error: boom"); - assert_eq!(event.http_status_code, None); - } - - #[test] - fn error_event_applies_message_wrapper() { - let event = CodexErr::Fatal("boom".to_string()) - .to_error_event(Some("Error running remote compact task".to_string())); - - assert_eq!( - event.message, - "Error running remote compact task: Fatal error: boom" - ); - assert_eq!(event.http_status_code, None); - } } diff --git a/codex-rs/docs/protocol_v1.md b/codex-rs/docs/protocol_v1.md index c6af7c9260..f6405c9bc3 100644 --- a/codex-rs/docs/protocol_v1.md +++ b/codex-rs/docs/protocol_v1.md @@ -72,7 +72,7 @@ For complete documentation of the `Op` and `EventMsg` variants, refer to [protoc - `EventMsg::AgentMessage` – Messages from the `Model` - `EventMsg::ExecApprovalRequest` – Request approval from user to execute a command - `EventMsg::TaskComplete` – A task completed successfully - - `EventMsg::Error` – A task stopped with an error (includes an optional `http_status_code` when available) + - `EventMsg::Error` – A task stopped with an error - `EventMsg::Warning` – A non-fatal warning that the client should surface to the user - `EventMsg::TurnComplete` – Contains a `response_id` bookmark for last `response_id` executed by the task. This can be used to continue the task at a later point in time, perhaps with additional user input. diff --git a/codex-rs/exec/src/event_processor_with_human_output.rs b/codex-rs/exec/src/event_processor_with_human_output.rs index 86af82118b..2d550fea46 100644 --- a/codex-rs/exec/src/event_processor_with_human_output.rs +++ b/codex-rs/exec/src/event_processor_with_human_output.rs @@ -161,7 +161,7 @@ impl EventProcessor for EventProcessorWithHumanOutput { fn process_event(&mut self, event: Event) -> CodexStatus { let Event { id: _, msg } = event; match msg { - EventMsg::Error(ErrorEvent { message, .. }) => { + EventMsg::Error(ErrorEvent { message }) => { let prefix = "ERROR:".style(self.red); ts_msg!(self, "{prefix} {message}"); } @@ -221,7 +221,7 @@ impl EventProcessor for EventProcessorWithHumanOutput { EventMsg::BackgroundEvent(BackgroundEventEvent { message }) => { ts_msg!(self, "{}", message.style(self.dimmed)); } - EventMsg::StreamError(StreamErrorEvent { message, .. }) => { + EventMsg::StreamError(StreamErrorEvent { message }) => { ts_msg!(self, "{}", message.style(self.dimmed)); } EventMsg::TaskStarted(_) => { diff --git a/codex-rs/exec/tests/event_processor_with_json_output.rs b/codex-rs/exec/tests/event_processor_with_json_output.rs index 444d32304a..5053f61921 100644 --- a/codex-rs/exec/tests/event_processor_with_json_output.rs +++ b/codex-rs/exec/tests/event_processor_with_json_output.rs @@ -539,7 +539,6 @@ fn error_event_produces_error() { "e1", EventMsg::Error(codex_core::protocol::ErrorEvent { message: "boom".to_string(), - http_status_code: Some(500), }), )); assert_eq!( @@ -579,7 +578,6 @@ fn stream_error_event_produces_error() { "e1", EventMsg::StreamError(codex_core::protocol::StreamErrorEvent { message: "retrying".to_string(), - http_status_code: Some(500), }), )); assert_eq!( @@ -598,7 +596,6 @@ fn error_followed_by_task_complete_produces_turn_failed() { "e1", EventMsg::Error(ErrorEvent { message: "boom".to_string(), - http_status_code: Some(500), }), ); assert_eq!( diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 0158ce9cfe..1825d7636b 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -686,8 +686,6 @@ pub struct ExitedReviewModeEvent { #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] pub struct ErrorEvent { pub message: String, - #[serde(default)] - pub http_status_code: Option, } #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] @@ -1365,8 +1363,6 @@ pub struct UndoCompletedEvent { #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] pub struct StreamErrorEvent { pub message: String, - #[serde(default)] - pub http_status_code: Option, } #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 3a79330b74..987a956fe6 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1654,7 +1654,7 @@ impl ChatWidget { self.on_rate_limit_snapshot(ev.rate_limits); } EventMsg::Warning(WarningEvent { message }) => self.on_warning(message), - EventMsg::Error(ErrorEvent { message, .. }) => self.on_error(message), + EventMsg::Error(ErrorEvent { message }) => self.on_error(message), EventMsg::McpStartupUpdate(ev) => self.on_mcp_startup_update(ev), EventMsg::McpStartupComplete(ev) => self.on_mcp_startup_complete(ev), EventMsg::TurnAborted(ev) => match ev.reason { @@ -1697,9 +1697,7 @@ impl ChatWidget { } EventMsg::UndoStarted(ev) => self.on_undo_started(ev), EventMsg::UndoCompleted(ev) => self.on_undo_completed(ev), - EventMsg::StreamError(StreamErrorEvent { message, .. }) => { - self.on_stream_error(message) - } + EventMsg::StreamError(StreamErrorEvent { message }) => self.on_stream_error(message), EventMsg::UserMessage(ev) => { if from_replay { self.on_user_message_event(ev); diff --git a/codex-rs/tui/src/chatwidget/agent.rs b/codex-rs/tui/src/chatwidget/agent.rs index 0abddac50d..3c326fed86 100644 --- a/codex-rs/tui/src/chatwidget/agent.rs +++ b/codex-rs/tui/src/chatwidget/agent.rs @@ -37,10 +37,7 @@ pub(crate) fn spawn_agent( eprintln!("{message}"); app_event_tx_clone.send(AppEvent::CodexEvent(Event { id: "".to_string(), - msg: EventMsg::Error(ErrorEvent { - message, - http_status_code: None, - }), + msg: EventMsg::Error(ErrorEvent { message }), })); app_event_tx_clone.send(AppEvent::ExitRequest); tracing::error!("failed to initialize codex: {err}"); diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index a4cef22418..d60a20b8f1 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -2596,7 +2596,6 @@ fn stream_error_updates_status_indicator() { id: "sub-1".into(), msg: EventMsg::StreamError(StreamErrorEvent { message: msg.to_string(), - http_status_code: None, }), }); From fb9849e1e3252048638ceef5715d8cfbfece34e3 Mon Sep 17 00:00:00 2001 From: zhao-oai Date: Wed, 19 Nov 2025 19:14:10 -0800 Subject: [PATCH 6/9] migrating execpolicy -> execpolicy-legacy and execpolicy2 -> execpolicy (#6956) --- codex-rs/Cargo.lock | 32 +-- codex-rs/Cargo.toml | 4 +- codex-rs/core/Cargo.toml | 2 +- codex-rs/core/src/codex.rs | 6 +- codex-rs/core/src/exec_policy.rs | 10 +- codex-rs/exec-server/src/posix.rs | 2 +- codex-rs/execpolicy-legacy/Cargo.toml | 34 +++ codex-rs/execpolicy-legacy/README.md | 183 ++++++++++++++++ .../build.rs | 0 .../src/arg_matcher.rs | 0 .../src/arg_resolver.rs | 0 .../src/arg_type.rs | 0 .../src/default.policy | 0 codex-rs/execpolicy-legacy/src/error.rs | 96 +++++++++ .../src/exec_call.rs | 0 .../src/execv_checker.rs | 0 codex-rs/execpolicy-legacy/src/lib.rs | 45 ++++ codex-rs/execpolicy-legacy/src/main.rs | 169 +++++++++++++++ .../src/opt.rs | 0 codex-rs/execpolicy-legacy/src/policy.rs | 103 +++++++++ .../src/policy_parser.rs | 0 .../src/program.rs | 0 .../src/sed_command.rs | 0 .../src/valid_exec.rs | 0 .../tests/all.rs | 0 .../tests/suite/bad.rs | 4 +- .../tests/suite/cp.rs | 22 +- .../tests/suite/good.rs | 4 +- .../tests/suite/head.rs | 24 +-- .../tests/suite/literal.rs | 18 +- .../tests/suite/ls.rs | 22 +- .../tests/suite/mod.rs | 0 .../tests/suite/parse_sed_command.rs | 4 +- .../tests/suite/pwd.rs | 18 +- .../tests/suite/sed.rs | 24 +-- codex-rs/execpolicy/Cargo.toml | 22 +- codex-rs/execpolicy/README.md | 204 ++++-------------- .../examples/example.codexpolicy | 0 .../src/decision.rs | 0 codex-rs/execpolicy/src/error.rs | 110 ++-------- codex-rs/execpolicy/src/lib.rs | 52 +---- codex-rs/execpolicy/src/main.rs | 199 +++++------------ .../{execpolicy2 => execpolicy}/src/parser.rs | 0 codex-rs/execpolicy/src/policy.rs | 161 ++++++-------- .../{execpolicy2 => execpolicy}/src/rule.rs | 0 .../tests/basic.rs | 16 +- codex-rs/execpolicy2/Cargo.toml | 29 --- codex-rs/execpolicy2/README.md | 59 ----- codex-rs/execpolicy2/src/error.rs | 26 --- codex-rs/execpolicy2/src/lib.rs | 15 -- codex-rs/execpolicy2/src/main.rs | 66 ------ codex-rs/execpolicy2/src/policy.rs | 84 -------- 52 files changed, 938 insertions(+), 931 deletions(-) create mode 100644 codex-rs/execpolicy-legacy/Cargo.toml create mode 100644 codex-rs/execpolicy-legacy/README.md rename codex-rs/{execpolicy => execpolicy-legacy}/build.rs (100%) rename codex-rs/{execpolicy => execpolicy-legacy}/src/arg_matcher.rs (100%) rename codex-rs/{execpolicy => execpolicy-legacy}/src/arg_resolver.rs (100%) rename codex-rs/{execpolicy => execpolicy-legacy}/src/arg_type.rs (100%) rename codex-rs/{execpolicy => execpolicy-legacy}/src/default.policy (100%) create mode 100644 codex-rs/execpolicy-legacy/src/error.rs rename codex-rs/{execpolicy => execpolicy-legacy}/src/exec_call.rs (100%) rename codex-rs/{execpolicy => execpolicy-legacy}/src/execv_checker.rs (100%) create mode 100644 codex-rs/execpolicy-legacy/src/lib.rs create mode 100644 codex-rs/execpolicy-legacy/src/main.rs rename codex-rs/{execpolicy => execpolicy-legacy}/src/opt.rs (100%) create mode 100644 codex-rs/execpolicy-legacy/src/policy.rs rename codex-rs/{execpolicy => execpolicy-legacy}/src/policy_parser.rs (100%) rename codex-rs/{execpolicy => execpolicy-legacy}/src/program.rs (100%) rename codex-rs/{execpolicy => execpolicy-legacy}/src/sed_command.rs (100%) rename codex-rs/{execpolicy => execpolicy-legacy}/src/valid_exec.rs (100%) rename codex-rs/{execpolicy => execpolicy-legacy}/tests/all.rs (100%) rename codex-rs/{execpolicy => execpolicy-legacy}/tests/suite/bad.rs (72%) rename codex-rs/{execpolicy => execpolicy-legacy}/tests/suite/cp.rs (81%) rename codex-rs/{execpolicy => execpolicy-legacy}/tests/suite/good.rs (72%) rename codex-rs/{execpolicy => execpolicy-legacy}/tests/suite/head.rs (87%) rename codex-rs/{execpolicy => execpolicy-legacy}/tests/suite/literal.rs (78%) rename codex-rs/{execpolicy => execpolicy-legacy}/tests/suite/ls.rs (91%) rename codex-rs/{execpolicy => execpolicy-legacy}/tests/suite/mod.rs (100%) rename codex-rs/{execpolicy => execpolicy-legacy}/tests/suite/parse_sed_command.rs (84%) rename codex-rs/{execpolicy => execpolicy-legacy}/tests/suite/pwd.rs (82%) rename codex-rs/{execpolicy => execpolicy-legacy}/tests/suite/sed.rs (82%) rename codex-rs/{execpolicy2 => execpolicy}/examples/example.codexpolicy (100%) rename codex-rs/{execpolicy2 => execpolicy}/src/decision.rs (100%) rename codex-rs/{execpolicy2 => execpolicy}/src/parser.rs (100%) rename codex-rs/{execpolicy2 => execpolicy}/src/rule.rs (100%) rename codex-rs/{execpolicy2 => execpolicy}/tests/basic.rs (97%) delete mode 100644 codex-rs/execpolicy2/Cargo.toml delete mode 100644 codex-rs/execpolicy2/README.md delete mode 100644 codex-rs/execpolicy2/src/error.rs delete mode 100644 codex-rs/execpolicy2/src/lib.rs delete mode 100644 codex-rs/execpolicy2/src/main.rs delete mode 100644 codex-rs/execpolicy2/src/policy.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 798b8d71b3..b8175ee36b 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1086,7 +1086,7 @@ dependencies = [ "codex-apply-patch", "codex-arg0", "codex-async-utils", - "codex-execpolicy2", + "codex-execpolicy", "codex-file-search", "codex-git", "codex-keyring-store", @@ -1209,6 +1209,21 @@ dependencies = [ [[package]] name = "codex-execpolicy" version = "0.0.0" +dependencies = [ + "anyhow", + "clap", + "multimap", + "pretty_assertions", + "serde", + "serde_json", + "shlex", + "starlark", + "thiserror 2.0.17", +] + +[[package]] +name = "codex-execpolicy-legacy" +version = "0.0.0" dependencies = [ "allocative", "anyhow", @@ -1226,21 +1241,6 @@ dependencies = [ "tempfile", ] -[[package]] -name = "codex-execpolicy2" -version = "0.0.0" -dependencies = [ - "anyhow", - "clap", - "multimap", - "pretty_assertions", - "serde", - "serde_json", - "shlex", - "starlark", - "thiserror 2.0.17", -] - [[package]] name = "codex-feedback" version = "0.0.0" diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index b0cf1e70fb..e2aa981381 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -18,7 +18,7 @@ members = [ "exec", "exec-server", "execpolicy", - "execpolicy2", + "execpolicy-legacy", "keyring-store", "file-search", "linux-sandbox", @@ -67,7 +67,7 @@ codex-chatgpt = { path = "chatgpt" } codex-common = { path = "common" } codex-core = { path = "core" } codex-exec = { path = "exec" } -codex-execpolicy2 = { path = "execpolicy2" } +codex-execpolicy = { path = "execpolicy" } codex-feedback = { path = "feedback" } codex-file-search = { path = "file-search" } codex-git = { path = "utils/git" } diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index db06f6b795..bc7b3668d0 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -22,7 +22,7 @@ chrono = { workspace = true, features = ["serde"] } codex-app-server-protocol = { workspace = true } codex-apply-patch = { workspace = true } codex-async-utils = { workspace = true } -codex-execpolicy2 = { workspace = true } +codex-execpolicy = { workspace = true } codex-file-search = { workspace = true } codex-git = { workspace = true } codex-keyring-store = { workspace = true } diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index c793eff352..182ca67f41 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -121,7 +121,7 @@ use crate::user_instructions::UserInstructions; use crate::user_notification::UserNotification; use crate::util::backoff; use codex_async_utils::OrCancelExt; -use codex_execpolicy2::Policy as ExecPolicy; +use codex_execpolicy::Policy as ExecPolicy; use codex_otel::otel_event_manager::OtelEventManager; use codex_protocol::config_types::ReasoningEffort as ReasoningEffortConfig; use codex_protocol::config_types::ReasoningSummary as ReasoningSummaryConfig; @@ -2616,7 +2616,7 @@ mod tests { cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), features: Features::default(), - exec_policy: Arc::new(codex_execpolicy2::Policy::empty()), + exec_policy: Arc::new(ExecPolicy::empty()), session_source: SessionSource::Exec, }; @@ -2694,7 +2694,7 @@ mod tests { cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), features: Features::default(), - exec_policy: Arc::new(codex_execpolicy2::Policy::empty()), + exec_policy: Arc::new(ExecPolicy::empty()), session_source: SessionSource::Exec, }; diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index c55dbfed23..9525cc1acf 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -4,10 +4,10 @@ use std::path::PathBuf; use std::sync::Arc; use crate::command_safety::is_dangerous_command::requires_initial_appoval; -use codex_execpolicy2::Decision; -use codex_execpolicy2::Evaluation; -use codex_execpolicy2::Policy; -use codex_execpolicy2::PolicyParser; +use codex_execpolicy::Decision; +use codex_execpolicy::Evaluation; +use codex_execpolicy::Policy; +use codex_execpolicy::PolicyParser; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::SandboxPolicy; use thiserror::Error; @@ -41,7 +41,7 @@ pub enum ExecPolicyError { #[error("failed to parse execpolicy file {path}: {source}")] ParsePolicy { path: String, - source: codex_execpolicy2::Error, + source: codex_execpolicy::Error, }, } diff --git a/codex-rs/exec-server/src/posix.rs b/codex-rs/exec-server/src/posix.rs index 239eaf61f2..ea0fab1a7e 100644 --- a/codex-rs/exec-server/src/posix.rs +++ b/codex-rs/exec-server/src/posix.rs @@ -144,7 +144,7 @@ pub async fn main_execve_wrapper() -> anyhow::Result<()> { std::process::exit(exit_code); } -// TODO: replace with execpolicy2 +// TODO: replace with execpolicy fn dummy_exec_policy(file: &Path, argv: &[String], _workdir: &Path) -> ExecPolicyOutcome { if file.ends_with("rm") { diff --git a/codex-rs/execpolicy-legacy/Cargo.toml b/codex-rs/execpolicy-legacy/Cargo.toml new file mode 100644 index 0000000000..89e6e43b41 --- /dev/null +++ b/codex-rs/execpolicy-legacy/Cargo.toml @@ -0,0 +1,34 @@ +[package] +edition = "2024" +name = "codex-execpolicy-legacy" +description = "Legacy exec policy engine for validating proposed exec calls." +version = { workspace = true } + +[[bin]] +name = "codex-execpolicy-legacy" +path = "src/main.rs" + +[lib] +name = "codex_execpolicy_legacy" +path = "src/lib.rs" + +[lints] +workspace = true + +[dependencies] +allocative = { workspace = true } +anyhow = { workspace = true } +clap = { workspace = true, features = ["derive"] } +derive_more = { workspace = true, features = ["display"] } +env_logger = { workspace = true } +log = { workspace = true } +multimap = { workspace = true } +path-absolutize = { workspace = true } +regex-lite = { workspace = true } +serde = { workspace = true, features = ["derive"] } +serde_json = { workspace = true } +serde_with = { workspace = true, features = ["macros"] } +starlark = { workspace = true } + +[dev-dependencies] +tempfile = { workspace = true } diff --git a/codex-rs/execpolicy-legacy/README.md b/codex-rs/execpolicy-legacy/README.md new file mode 100644 index 0000000000..1351377e5d --- /dev/null +++ b/codex-rs/execpolicy-legacy/README.md @@ -0,0 +1,183 @@ +# codex-execpolicy-legacy + +This crate hosts the original execpolicy implementation. The newer prefix-rule +engine lives in `codex-execpolicy`. + +The goal of this library is to classify a proposed [`execv(3)`](https://linux.die.net/man/3/execv) command into one of the following states: + +- `safe` The command is safe to run (\*). +- `match` The command matched a rule in the policy, but the caller should decide whether it is safe to run based on the files it will write. +- `forbidden` The command is not allowed to be run. +- `unverified` The safety cannot be determined: make the user decide. + +(\*) Whether an `execv(3)` call should be considered "safe" often requires additional context beyond the arguments to `execv()` itself. For example, if you trust an autonomous software agent to write files in your source tree, then deciding whether `/bin/cp foo bar` is "safe" depends on `getcwd(3)` for the calling process as well as the `realpath` of `foo` and `bar` when resolved against `getcwd()`. +To that end, rather than returning a boolean, the validator returns a structured result that the client is expected to use to determine the "safety" of the proposed `execv()` call. + +For example, to check the command `ls -l foo`, the checker would be invoked as follows: + +```shell +cargo run -p codex-execpolicy-legacy -- check ls -l foo | jq +``` + +It will exit with `0` and print the following to stdout: + +```json +{ + "result": "safe", + "match": { + "program": "ls", + "flags": [ + { + "name": "-l" + } + ], + "opts": [], + "args": [ + { + "index": 1, + "type": "ReadableFile", + "value": "foo" + } + ], + "system_path": ["/bin/ls", "/usr/bin/ls"] + } +} +``` + +Of note: + +- `foo` is tagged as a `ReadableFile`, so the caller should resolve `foo` relative to `getcwd()` and `realpath` it (as it may be a symlink) to determine whether `foo` is safe to read. +- While the specified executable is `ls`, `"system_path"` offers `/bin/ls` and `/usr/bin/ls` as viable alternatives to avoid using whatever `ls` happens to appear first on the user's `$PATH`. If either exists on the host, it is recommended to use it as the first argument to `execv(3)` instead of `ls`. + +Further, "safety" in this system is not a guarantee that the command will execute successfully. As an example, `cat /Users/mbolin/code/codex/README.md` may be considered "safe" if the system has decided the agent is allowed to read anything under `/Users/mbolin/code/codex`, but it will fail at runtime if `README.md` does not exist. (Though this is "safe" in that the agent did not read any files that it was not authorized to read.) + +## Policy + +Currently, the default policy is defined in [`default.policy`](./src/default.policy) within the crate. + +The system uses [Starlark](https://bazel.build/rules/language) as the file format because, unlike something like JSON or YAML, it supports "macros" without compromising on safety or reproducibility. (Under the hood, we use [`starlark-rust`](https://github.com/facebook/starlark-rust) as the specific Starlark implementation.) + +This policy contains "rules" such as: + +```python +define_program( + program="cp", + options=[ + flag("-r"), + flag("-R"), + flag("--recursive"), + ], + args=[ARG_RFILES, ARG_WFILE], + system_path=["/bin/cp", "/usr/bin/cp"], + should_match=[ + ["foo", "bar"], + ], + should_not_match=[ + ["foo"], + ], +) +``` + +This rule means that: + +- `cp` can be used with any of the following flags (where "flag" means "an option that does not take an argument"): `-r`, `-R`, `--recursive`. +- The initial `ARG_RFILES` passed to `args` means that it expects one or more arguments that correspond to "readable files" +- The final `ARG_WFILE` passed to `args` means that it expects exactly one argument that corresponds to a "writeable file." +- As a means of a lightweight way of including a unit test alongside the definition, the `should_match` list is a list of examples of `execv(3)` args that should match the rule and `should_not_match` is a list of examples that should not match. These examples are verified when the `.policy` file is loaded. + +Note that the language of the `.policy` file is still evolving, as we have to continue to expand it so it is sufficiently expressive to accept all commands we want to consider "safe" without allowing unsafe commands to pass through. + +The integrity of `default.policy` is verified [via unit tests](./tests). + +Further, the CLI supports a `--policy` option to specify a custom `.policy` file for ad-hoc testing. + +## Output Type: `match` + +Going back to the `cp` example, because the rule matches an `ARG_WFILE`, it will return `match` instead of `safe`: + +```shell +cargo run -p codex-execpolicy-legacy -- check cp src1 src2 dest | jq +``` + +If the caller wants to consider allowing this command, it should parse the JSON to pick out the `WriteableFile` arguments and decide whether they are safe to write: + +```json +{ + "result": "match", + "match": { + "program": "cp", + "flags": [], + "opts": [], + "args": [ + { + "index": 0, + "type": "ReadableFile", + "value": "src1" + }, + { + "index": 1, + "type": "ReadableFile", + "value": "src2" + }, + { + "index": 2, + "type": "WriteableFile", + "value": "dest" + } + ], + "system_path": ["/bin/cp", "/usr/bin/cp"] + } +} +``` + +Note the exit code is still `0` for a `match` unless the `--require-safe` flag is specified, in which case the exit code is `12`. + +## Output Type: `forbidden` + +It is also possible to define a rule that, if it matches a command, should flag it as _forbidden_. For example, we do not want agents to be able to run `applied deploy` _ever_, so we define the following rule: + +```python +define_program( + program="applied", + args=["deploy"], + forbidden="Infrastructure Risk: command contains 'applied deploy'", + should_match=[ + ["deploy"], + ], + should_not_match=[ + ["lint"], + ], +) +``` + +Note that for a rule to be forbidden, the `forbidden` keyword arg must be specified as the reason the command is forbidden. This will be included in the output: + +```shell +cargo run -p codex-execpolicy-legacy -- check applied deploy | jq +``` + +```json +{ + "result": "forbidden", + "reason": "Infrastructure Risk: command contains 'applied deploy'", + "cause": { + "Exec": { + "exec": { + "program": "applied", + "flags": [], + "opts": [], + "args": [ + { + "index": 0, + "type": { + "Literal": "deploy" + }, + "value": "deploy" + } + ], + "system_path": [] + } + } + } +} +``` diff --git a/codex-rs/execpolicy/build.rs b/codex-rs/execpolicy-legacy/build.rs similarity index 100% rename from codex-rs/execpolicy/build.rs rename to codex-rs/execpolicy-legacy/build.rs diff --git a/codex-rs/execpolicy/src/arg_matcher.rs b/codex-rs/execpolicy-legacy/src/arg_matcher.rs similarity index 100% rename from codex-rs/execpolicy/src/arg_matcher.rs rename to codex-rs/execpolicy-legacy/src/arg_matcher.rs diff --git a/codex-rs/execpolicy/src/arg_resolver.rs b/codex-rs/execpolicy-legacy/src/arg_resolver.rs similarity index 100% rename from codex-rs/execpolicy/src/arg_resolver.rs rename to codex-rs/execpolicy-legacy/src/arg_resolver.rs diff --git a/codex-rs/execpolicy/src/arg_type.rs b/codex-rs/execpolicy-legacy/src/arg_type.rs similarity index 100% rename from codex-rs/execpolicy/src/arg_type.rs rename to codex-rs/execpolicy-legacy/src/arg_type.rs diff --git a/codex-rs/execpolicy/src/default.policy b/codex-rs/execpolicy-legacy/src/default.policy similarity index 100% rename from codex-rs/execpolicy/src/default.policy rename to codex-rs/execpolicy-legacy/src/default.policy diff --git a/codex-rs/execpolicy-legacy/src/error.rs b/codex-rs/execpolicy-legacy/src/error.rs new file mode 100644 index 0000000000..e6443d69dc --- /dev/null +++ b/codex-rs/execpolicy-legacy/src/error.rs @@ -0,0 +1,96 @@ +use std::path::PathBuf; + +use serde::Serialize; + +use crate::arg_matcher::ArgMatcher; +use crate::arg_resolver::PositionalArg; +use serde_with::DisplayFromStr; +use serde_with::serde_as; + +pub type Result = std::result::Result; + +#[serde_as] +#[derive(Debug, Eq, PartialEq, Serialize)] +#[serde(tag = "type")] +pub enum Error { + NoSpecForProgram { + program: String, + }, + OptionMissingValue { + program: String, + option: String, + }, + OptionFollowedByOptionInsteadOfValue { + program: String, + option: String, + value: String, + }, + UnknownOption { + program: String, + option: String, + }, + UnexpectedArguments { + program: String, + args: Vec, + }, + DoubleDashNotSupportedYet { + program: String, + }, + MultipleVarargPatterns { + program: String, + first: ArgMatcher, + second: ArgMatcher, + }, + RangeStartExceedsEnd { + start: usize, + end: usize, + }, + RangeEndOutOfBounds { + end: usize, + len: usize, + }, + PrefixOverlapsSuffix {}, + NotEnoughArgs { + program: String, + args: Vec, + arg_patterns: Vec, + }, + InternalInvariantViolation { + message: String, + }, + VarargMatcherDidNotMatchAnything { + program: String, + matcher: ArgMatcher, + }, + EmptyFileName {}, + LiteralValueDidNotMatch { + expected: String, + actual: String, + }, + InvalidPositiveInteger { + value: String, + }, + MissingRequiredOptions { + program: String, + options: Vec, + }, + SedCommandNotProvablySafe { + command: String, + }, + ReadablePathNotInReadableFolders { + file: PathBuf, + folders: Vec, + }, + WriteablePathNotInWriteableFolders { + file: PathBuf, + folders: Vec, + }, + CannotCheckRelativePath { + file: PathBuf, + }, + CannotCanonicalizePath { + file: String, + #[serde_as(as = "DisplayFromStr")] + error: std::io::ErrorKind, + }, +} diff --git a/codex-rs/execpolicy/src/exec_call.rs b/codex-rs/execpolicy-legacy/src/exec_call.rs similarity index 100% rename from codex-rs/execpolicy/src/exec_call.rs rename to codex-rs/execpolicy-legacy/src/exec_call.rs diff --git a/codex-rs/execpolicy/src/execv_checker.rs b/codex-rs/execpolicy-legacy/src/execv_checker.rs similarity index 100% rename from codex-rs/execpolicy/src/execv_checker.rs rename to codex-rs/execpolicy-legacy/src/execv_checker.rs diff --git a/codex-rs/execpolicy-legacy/src/lib.rs b/codex-rs/execpolicy-legacy/src/lib.rs new file mode 100644 index 0000000000..6f12225981 --- /dev/null +++ b/codex-rs/execpolicy-legacy/src/lib.rs @@ -0,0 +1,45 @@ +#![allow(clippy::type_complexity)] +#![allow(clippy::too_many_arguments)] +#[macro_use] +extern crate starlark; + +mod arg_matcher; +mod arg_resolver; +mod arg_type; +mod error; +mod exec_call; +mod execv_checker; +mod opt; +mod policy; +mod policy_parser; +mod program; +mod sed_command; +mod valid_exec; + +pub use arg_matcher::ArgMatcher; +pub use arg_resolver::PositionalArg; +pub use arg_type::ArgType; +pub use error::Error; +pub use error::Result; +pub use exec_call::ExecCall; +pub use execv_checker::ExecvChecker; +pub use opt::Opt; +pub use policy::Policy; +pub use policy_parser::PolicyParser; +pub use program::Forbidden; +pub use program::MatchedExec; +pub use program::NegativeExamplePassedCheck; +pub use program::PositiveExampleFailedCheck; +pub use program::ProgramSpec; +pub use sed_command::parse_sed_command; +pub use valid_exec::MatchedArg; +pub use valid_exec::MatchedFlag; +pub use valid_exec::MatchedOpt; +pub use valid_exec::ValidExec; + +const DEFAULT_POLICY: &str = include_str!("default.policy"); + +pub fn get_default_policy() -> starlark::Result { + let parser = PolicyParser::new("#default", DEFAULT_POLICY); + parser.parse() +} diff --git a/codex-rs/execpolicy-legacy/src/main.rs b/codex-rs/execpolicy-legacy/src/main.rs new file mode 100644 index 0000000000..f5b66dfe50 --- /dev/null +++ b/codex-rs/execpolicy-legacy/src/main.rs @@ -0,0 +1,169 @@ +use anyhow::Result; +use clap::Parser; +use clap::Subcommand; +use codex_execpolicy_legacy::ExecCall; +use codex_execpolicy_legacy::MatchedExec; +use codex_execpolicy_legacy::Policy; +use codex_execpolicy_legacy::PolicyParser; +use codex_execpolicy_legacy::ValidExec; +use codex_execpolicy_legacy::get_default_policy; +use serde::Deserialize; +use serde::Serialize; +use serde::de; +use starlark::Error as StarlarkError; +use std::path::PathBuf; +use std::str::FromStr; + +const MATCHED_BUT_WRITES_FILES_EXIT_CODE: i32 = 12; +const MIGHT_BE_SAFE_EXIT_CODE: i32 = 13; +const FORBIDDEN_EXIT_CODE: i32 = 14; + +#[derive(Parser, Deserialize, Debug)] +#[command(version, about, long_about = None)] +pub struct Args { + /// If the command fails the policy, exit with 13, but print parseable JSON + /// to stdout. + #[clap(long)] + pub require_safe: bool, + + /// Path to the policy file. + #[clap(long, short = 'p')] + pub policy: Option, + + #[command(subcommand)] + pub command: Command, +} + +#[derive(Clone, Debug, Deserialize, Subcommand)] +pub enum Command { + /// Checks the command as if the arguments were the inputs to execv(3). + Check { + #[arg(trailing_var_arg = true)] + command: Vec, + }, + + /// Checks the command encoded as a JSON object. + #[clap(name = "check-json")] + CheckJson { + /// JSON object with "program" (str) and "args" (list[str]) fields. + #[serde(deserialize_with = "deserialize_from_json")] + exec: ExecArg, + }, +} + +#[derive(Clone, Debug, Deserialize)] +pub struct ExecArg { + pub program: String, + + #[serde(default)] + pub args: Vec, +} + +fn main() -> Result<()> { + env_logger::init(); + + let args = Args::parse(); + let policy = match args.policy { + Some(policy) => { + let policy_source = policy.to_string_lossy().to_string(); + let unparsed_policy = std::fs::read_to_string(policy)?; + let parser = PolicyParser::new(&policy_source, &unparsed_policy); + parser.parse() + } + None => get_default_policy(), + }; + let policy = policy.map_err(StarlarkError::into_anyhow)?; + + let exec = match args.command { + Command::Check { command } => match command.split_first() { + Some((first, rest)) => ExecArg { + program: first.to_string(), + args: rest.to_vec(), + }, + None => { + eprintln!("no command provided"); + std::process::exit(1); + } + }, + Command::CheckJson { exec } => exec, + }; + + let (output, exit_code) = check_command(&policy, exec, args.require_safe); + let json = serde_json::to_string(&output)?; + println!("{json}"); + std::process::exit(exit_code); +} + +fn check_command( + policy: &Policy, + ExecArg { program, args }: ExecArg, + check: bool, +) -> (Output, i32) { + let exec_call = ExecCall { program, args }; + match policy.check(&exec_call) { + Ok(MatchedExec::Match { exec }) => { + if exec.might_write_files() { + let exit_code = if check { + MATCHED_BUT_WRITES_FILES_EXIT_CODE + } else { + 0 + }; + (Output::Match { r#match: exec }, exit_code) + } else { + (Output::Safe { r#match: exec }, 0) + } + } + Ok(MatchedExec::Forbidden { reason, cause }) => { + let exit_code = if check { FORBIDDEN_EXIT_CODE } else { 0 }; + (Output::Forbidden { reason, cause }, exit_code) + } + Err(err) => { + let exit_code = if check { MIGHT_BE_SAFE_EXIT_CODE } else { 0 }; + (Output::Unverified { error: err }, exit_code) + } + } +} + +#[derive(Debug, Serialize)] +#[serde(tag = "result")] +pub enum Output { + /// The command is verified as safe. + #[serde(rename = "safe")] + Safe { r#match: ValidExec }, + + /// The command has matched a rule in the policy, but the caller should + /// decide whether it is "safe" given the files it wants to write. + #[serde(rename = "match")] + Match { r#match: ValidExec }, + + /// The user is forbidden from running the command. + #[serde(rename = "forbidden")] + Forbidden { + reason: String, + cause: codex_execpolicy_legacy::Forbidden, + }, + + /// The safety of the command could not be verified. + #[serde(rename = "unverified")] + Unverified { + error: codex_execpolicy_legacy::Error, + }, +} + +fn deserialize_from_json<'de, D>(deserializer: D) -> Result +where + D: de::Deserializer<'de>, +{ + let s = String::deserialize(deserializer)?; + let decoded = serde_json::from_str(&s) + .map_err(|e| serde::de::Error::custom(format!("JSON parse error: {e}")))?; + Ok(decoded) +} + +impl FromStr for ExecArg { + type Err = anyhow::Error; + + fn from_str(s: &str) -> Result { + serde_json::from_str(s).map_err(Into::into) + } +} diff --git a/codex-rs/execpolicy/src/opt.rs b/codex-rs/execpolicy-legacy/src/opt.rs similarity index 100% rename from codex-rs/execpolicy/src/opt.rs rename to codex-rs/execpolicy-legacy/src/opt.rs diff --git a/codex-rs/execpolicy-legacy/src/policy.rs b/codex-rs/execpolicy-legacy/src/policy.rs new file mode 100644 index 0000000000..825d6164a5 --- /dev/null +++ b/codex-rs/execpolicy-legacy/src/policy.rs @@ -0,0 +1,103 @@ +use multimap::MultiMap; +use regex_lite::Error as RegexError; +use regex_lite::Regex; + +use crate::ExecCall; +use crate::Forbidden; +use crate::MatchedExec; +use crate::NegativeExamplePassedCheck; +use crate::ProgramSpec; +use crate::error::Error; +use crate::error::Result; +use crate::policy_parser::ForbiddenProgramRegex; +use crate::program::PositiveExampleFailedCheck; + +pub struct Policy { + programs: MultiMap, + forbidden_program_regexes: Vec, + forbidden_substrings_pattern: Option, +} + +impl Policy { + pub fn new( + programs: MultiMap, + forbidden_program_regexes: Vec, + forbidden_substrings: Vec, + ) -> std::result::Result { + let forbidden_substrings_pattern = if forbidden_substrings.is_empty() { + None + } else { + let escaped_substrings = forbidden_substrings + .iter() + .map(|s| regex_lite::escape(s)) + .collect::>() + .join("|"); + Some(Regex::new(&format!("({escaped_substrings})"))?) + }; + Ok(Self { + programs, + forbidden_program_regexes, + forbidden_substrings_pattern, + }) + } + + pub fn check(&self, exec_call: &ExecCall) -> Result { + let ExecCall { program, args } = &exec_call; + for ForbiddenProgramRegex { regex, reason } in &self.forbidden_program_regexes { + if regex.is_match(program) { + return Ok(MatchedExec::Forbidden { + cause: Forbidden::Program { + program: program.clone(), + exec_call: exec_call.clone(), + }, + reason: reason.clone(), + }); + } + } + + for arg in args { + if let Some(regex) = &self.forbidden_substrings_pattern + && regex.is_match(arg) + { + return Ok(MatchedExec::Forbidden { + cause: Forbidden::Arg { + arg: arg.clone(), + exec_call: exec_call.clone(), + }, + reason: format!("arg `{arg}` contains forbidden substring"), + }); + } + } + + let mut last_err = Err(Error::NoSpecForProgram { + program: program.clone(), + }); + if let Some(spec_list) = self.programs.get_vec(program) { + for spec in spec_list { + match spec.check(exec_call) { + Ok(matched_exec) => return Ok(matched_exec), + Err(err) => { + last_err = Err(err); + } + } + } + } + last_err + } + + pub fn check_each_good_list_individually(&self) -> Vec { + let mut violations = Vec::new(); + for (_program, spec) in self.programs.flat_iter() { + violations.extend(spec.verify_should_match_list()); + } + violations + } + + pub fn check_each_bad_list_individually(&self) -> Vec { + let mut violations = Vec::new(); + for (_program, spec) in self.programs.flat_iter() { + violations.extend(spec.verify_should_not_match_list()); + } + violations + } +} diff --git a/codex-rs/execpolicy/src/policy_parser.rs b/codex-rs/execpolicy-legacy/src/policy_parser.rs similarity index 100% rename from codex-rs/execpolicy/src/policy_parser.rs rename to codex-rs/execpolicy-legacy/src/policy_parser.rs diff --git a/codex-rs/execpolicy/src/program.rs b/codex-rs/execpolicy-legacy/src/program.rs similarity index 100% rename from codex-rs/execpolicy/src/program.rs rename to codex-rs/execpolicy-legacy/src/program.rs diff --git a/codex-rs/execpolicy/src/sed_command.rs b/codex-rs/execpolicy-legacy/src/sed_command.rs similarity index 100% rename from codex-rs/execpolicy/src/sed_command.rs rename to codex-rs/execpolicy-legacy/src/sed_command.rs diff --git a/codex-rs/execpolicy/src/valid_exec.rs b/codex-rs/execpolicy-legacy/src/valid_exec.rs similarity index 100% rename from codex-rs/execpolicy/src/valid_exec.rs rename to codex-rs/execpolicy-legacy/src/valid_exec.rs diff --git a/codex-rs/execpolicy/tests/all.rs b/codex-rs/execpolicy-legacy/tests/all.rs similarity index 100% rename from codex-rs/execpolicy/tests/all.rs rename to codex-rs/execpolicy-legacy/tests/all.rs diff --git a/codex-rs/execpolicy/tests/suite/bad.rs b/codex-rs/execpolicy-legacy/tests/suite/bad.rs similarity index 72% rename from codex-rs/execpolicy/tests/suite/bad.rs rename to codex-rs/execpolicy-legacy/tests/suite/bad.rs index 8b6e195fb0..e1f8675330 100644 --- a/codex-rs/execpolicy/tests/suite/bad.rs +++ b/codex-rs/execpolicy-legacy/tests/suite/bad.rs @@ -1,5 +1,5 @@ -use codex_execpolicy::NegativeExamplePassedCheck; -use codex_execpolicy::get_default_policy; +use codex_execpolicy_legacy::NegativeExamplePassedCheck; +use codex_execpolicy_legacy::get_default_policy; #[test] fn verify_everything_in_bad_list_is_rejected() { diff --git a/codex-rs/execpolicy/tests/suite/cp.rs b/codex-rs/execpolicy-legacy/tests/suite/cp.rs similarity index 81% rename from codex-rs/execpolicy/tests/suite/cp.rs rename to codex-rs/execpolicy-legacy/tests/suite/cp.rs index aa19f0b5d5..3cfc9ac5c1 100644 --- a/codex-rs/execpolicy/tests/suite/cp.rs +++ b/codex-rs/execpolicy-legacy/tests/suite/cp.rs @@ -1,15 +1,15 @@ -extern crate codex_execpolicy; +extern crate codex_execpolicy_legacy; -use codex_execpolicy::ArgMatcher; -use codex_execpolicy::ArgType; -use codex_execpolicy::Error; -use codex_execpolicy::ExecCall; -use codex_execpolicy::MatchedArg; -use codex_execpolicy::MatchedExec; -use codex_execpolicy::Policy; -use codex_execpolicy::Result; -use codex_execpolicy::ValidExec; -use codex_execpolicy::get_default_policy; +use codex_execpolicy_legacy::ArgMatcher; +use codex_execpolicy_legacy::ArgType; +use codex_execpolicy_legacy::Error; +use codex_execpolicy_legacy::ExecCall; +use codex_execpolicy_legacy::MatchedArg; +use codex_execpolicy_legacy::MatchedExec; +use codex_execpolicy_legacy::Policy; +use codex_execpolicy_legacy::Result; +use codex_execpolicy_legacy::ValidExec; +use codex_execpolicy_legacy::get_default_policy; #[expect(clippy::expect_used)] fn setup() -> Policy { diff --git a/codex-rs/execpolicy/tests/suite/good.rs b/codex-rs/execpolicy-legacy/tests/suite/good.rs similarity index 72% rename from codex-rs/execpolicy/tests/suite/good.rs rename to codex-rs/execpolicy-legacy/tests/suite/good.rs index 3b7313a335..3c86c7acb8 100644 --- a/codex-rs/execpolicy/tests/suite/good.rs +++ b/codex-rs/execpolicy-legacy/tests/suite/good.rs @@ -1,5 +1,5 @@ -use codex_execpolicy::PositiveExampleFailedCheck; -use codex_execpolicy::get_default_policy; +use codex_execpolicy_legacy::PositiveExampleFailedCheck; +use codex_execpolicy_legacy::get_default_policy; #[test] fn verify_everything_in_good_list_is_allowed() { diff --git a/codex-rs/execpolicy/tests/suite/head.rs b/codex-rs/execpolicy-legacy/tests/suite/head.rs similarity index 87% rename from codex-rs/execpolicy/tests/suite/head.rs rename to codex-rs/execpolicy-legacy/tests/suite/head.rs index 3c32ccfbfe..390b4ddb3e 100644 --- a/codex-rs/execpolicy/tests/suite/head.rs +++ b/codex-rs/execpolicy-legacy/tests/suite/head.rs @@ -1,16 +1,16 @@ -use codex_execpolicy::ArgMatcher; -use codex_execpolicy::ArgType; -use codex_execpolicy::Error; -use codex_execpolicy::ExecCall; -use codex_execpolicy::MatchedArg; -use codex_execpolicy::MatchedExec; -use codex_execpolicy::MatchedOpt; -use codex_execpolicy::Policy; -use codex_execpolicy::Result; -use codex_execpolicy::ValidExec; -use codex_execpolicy::get_default_policy; +use codex_execpolicy_legacy::ArgMatcher; +use codex_execpolicy_legacy::ArgType; +use codex_execpolicy_legacy::Error; +use codex_execpolicy_legacy::ExecCall; +use codex_execpolicy_legacy::MatchedArg; +use codex_execpolicy_legacy::MatchedExec; +use codex_execpolicy_legacy::MatchedOpt; +use codex_execpolicy_legacy::Policy; +use codex_execpolicy_legacy::Result; +use codex_execpolicy_legacy::ValidExec; +use codex_execpolicy_legacy::get_default_policy; -extern crate codex_execpolicy; +extern crate codex_execpolicy_legacy; #[expect(clippy::expect_used)] fn setup() -> Policy { diff --git a/codex-rs/execpolicy/tests/suite/literal.rs b/codex-rs/execpolicy-legacy/tests/suite/literal.rs similarity index 78% rename from codex-rs/execpolicy/tests/suite/literal.rs rename to codex-rs/execpolicy-legacy/tests/suite/literal.rs index d849371e3b..296206dbc5 100644 --- a/codex-rs/execpolicy/tests/suite/literal.rs +++ b/codex-rs/execpolicy-legacy/tests/suite/literal.rs @@ -1,13 +1,13 @@ -use codex_execpolicy::ArgType; -use codex_execpolicy::Error; -use codex_execpolicy::ExecCall; -use codex_execpolicy::MatchedArg; -use codex_execpolicy::MatchedExec; -use codex_execpolicy::PolicyParser; -use codex_execpolicy::Result; -use codex_execpolicy::ValidExec; +use codex_execpolicy_legacy::ArgType; +use codex_execpolicy_legacy::Error; +use codex_execpolicy_legacy::ExecCall; +use codex_execpolicy_legacy::MatchedArg; +use codex_execpolicy_legacy::MatchedExec; +use codex_execpolicy_legacy::PolicyParser; +use codex_execpolicy_legacy::Result; +use codex_execpolicy_legacy::ValidExec; -extern crate codex_execpolicy; +extern crate codex_execpolicy_legacy; #[test] fn test_invalid_subcommand() -> Result<()> { diff --git a/codex-rs/execpolicy/tests/suite/ls.rs b/codex-rs/execpolicy-legacy/tests/suite/ls.rs similarity index 91% rename from codex-rs/execpolicy/tests/suite/ls.rs rename to codex-rs/execpolicy-legacy/tests/suite/ls.rs index e52316c06e..b4b27b0e22 100644 --- a/codex-rs/execpolicy/tests/suite/ls.rs +++ b/codex-rs/execpolicy-legacy/tests/suite/ls.rs @@ -1,15 +1,15 @@ -extern crate codex_execpolicy; +extern crate codex_execpolicy_legacy; -use codex_execpolicy::ArgType; -use codex_execpolicy::Error; -use codex_execpolicy::ExecCall; -use codex_execpolicy::MatchedArg; -use codex_execpolicy::MatchedExec; -use codex_execpolicy::MatchedFlag; -use codex_execpolicy::Policy; -use codex_execpolicy::Result; -use codex_execpolicy::ValidExec; -use codex_execpolicy::get_default_policy; +use codex_execpolicy_legacy::ArgType; +use codex_execpolicy_legacy::Error; +use codex_execpolicy_legacy::ExecCall; +use codex_execpolicy_legacy::MatchedArg; +use codex_execpolicy_legacy::MatchedExec; +use codex_execpolicy_legacy::MatchedFlag; +use codex_execpolicy_legacy::Policy; +use codex_execpolicy_legacy::Result; +use codex_execpolicy_legacy::ValidExec; +use codex_execpolicy_legacy::get_default_policy; #[expect(clippy::expect_used)] fn setup() -> Policy { diff --git a/codex-rs/execpolicy/tests/suite/mod.rs b/codex-rs/execpolicy-legacy/tests/suite/mod.rs similarity index 100% rename from codex-rs/execpolicy/tests/suite/mod.rs rename to codex-rs/execpolicy-legacy/tests/suite/mod.rs diff --git a/codex-rs/execpolicy/tests/suite/parse_sed_command.rs b/codex-rs/execpolicy-legacy/tests/suite/parse_sed_command.rs similarity index 84% rename from codex-rs/execpolicy/tests/suite/parse_sed_command.rs rename to codex-rs/execpolicy-legacy/tests/suite/parse_sed_command.rs index 20f5bbf301..f1da55d641 100644 --- a/codex-rs/execpolicy/tests/suite/parse_sed_command.rs +++ b/codex-rs/execpolicy-legacy/tests/suite/parse_sed_command.rs @@ -1,5 +1,5 @@ -use codex_execpolicy::Error; -use codex_execpolicy::parse_sed_command; +use codex_execpolicy_legacy::Error; +use codex_execpolicy_legacy::parse_sed_command; #[test] fn parses_simple_print_command() { diff --git a/codex-rs/execpolicy/tests/suite/pwd.rs b/codex-rs/execpolicy-legacy/tests/suite/pwd.rs similarity index 82% rename from codex-rs/execpolicy/tests/suite/pwd.rs rename to codex-rs/execpolicy-legacy/tests/suite/pwd.rs index fdf5a4f1a5..73d1caada8 100644 --- a/codex-rs/execpolicy/tests/suite/pwd.rs +++ b/codex-rs/execpolicy-legacy/tests/suite/pwd.rs @@ -1,15 +1,15 @@ -extern crate codex_execpolicy; +extern crate codex_execpolicy_legacy; use std::vec; -use codex_execpolicy::Error; -use codex_execpolicy::ExecCall; -use codex_execpolicy::MatchedExec; -use codex_execpolicy::MatchedFlag; -use codex_execpolicy::Policy; -use codex_execpolicy::PositionalArg; -use codex_execpolicy::ValidExec; -use codex_execpolicy::get_default_policy; +use codex_execpolicy_legacy::Error; +use codex_execpolicy_legacy::ExecCall; +use codex_execpolicy_legacy::MatchedExec; +use codex_execpolicy_legacy::MatchedFlag; +use codex_execpolicy_legacy::Policy; +use codex_execpolicy_legacy::PositionalArg; +use codex_execpolicy_legacy::ValidExec; +use codex_execpolicy_legacy::get_default_policy; #[expect(clippy::expect_used)] fn setup() -> Policy { diff --git a/codex-rs/execpolicy/tests/suite/sed.rs b/codex-rs/execpolicy-legacy/tests/suite/sed.rs similarity index 82% rename from codex-rs/execpolicy/tests/suite/sed.rs rename to codex-rs/execpolicy-legacy/tests/suite/sed.rs index bf35bf6d46..d732cc7c9c 100644 --- a/codex-rs/execpolicy/tests/suite/sed.rs +++ b/codex-rs/execpolicy-legacy/tests/suite/sed.rs @@ -1,16 +1,16 @@ -extern crate codex_execpolicy; +extern crate codex_execpolicy_legacy; -use codex_execpolicy::ArgType; -use codex_execpolicy::Error; -use codex_execpolicy::ExecCall; -use codex_execpolicy::MatchedArg; -use codex_execpolicy::MatchedExec; -use codex_execpolicy::MatchedFlag; -use codex_execpolicy::MatchedOpt; -use codex_execpolicy::Policy; -use codex_execpolicy::Result; -use codex_execpolicy::ValidExec; -use codex_execpolicy::get_default_policy; +use codex_execpolicy_legacy::ArgType; +use codex_execpolicy_legacy::Error; +use codex_execpolicy_legacy::ExecCall; +use codex_execpolicy_legacy::MatchedArg; +use codex_execpolicy_legacy::MatchedExec; +use codex_execpolicy_legacy::MatchedFlag; +use codex_execpolicy_legacy::MatchedOpt; +use codex_execpolicy_legacy::Policy; +use codex_execpolicy_legacy::Result; +use codex_execpolicy_legacy::ValidExec; +use codex_execpolicy_legacy::get_default_policy; #[expect(clippy::expect_used)] fn setup() -> Policy { diff --git a/codex-rs/execpolicy/Cargo.toml b/codex-rs/execpolicy/Cargo.toml index 0fe7cd486a..bececed4b2 100644 --- a/codex-rs/execpolicy/Cargo.toml +++ b/codex-rs/execpolicy/Cargo.toml @@ -1,33 +1,29 @@ [package] -edition = "2024" name = "codex-execpolicy" version = { workspace = true } - -[[bin]] -name = "codex-execpolicy" -path = "src/main.rs" +edition = "2024" +description = "Codex exec policy: prefix-based Starlark rules for command decisions." [lib] name = "codex_execpolicy" path = "src/lib.rs" +[[bin]] +name = "codex-execpolicy" +path = "src/main.rs" + [lints] workspace = true [dependencies] -allocative = { workspace = true } anyhow = { workspace = true } clap = { workspace = true, features = ["derive"] } -derive_more = { workspace = true, features = ["display"] } -env_logger = { workspace = true } -log = { workspace = true } multimap = { workspace = true } -path-absolutize = { workspace = true } -regex-lite = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } -serde_with = { workspace = true, features = ["macros"] } +shlex = { workspace = true } starlark = { workspace = true } +thiserror = { workspace = true } [dev-dependencies] -tempfile = { workspace = true } +pretty_assertions = { workspace = true } diff --git a/codex-rs/execpolicy/README.md b/codex-rs/execpolicy/README.md index ca95829440..f508c8ca64 100644 --- a/codex-rs/execpolicy/README.md +++ b/codex-rs/execpolicy/README.md @@ -1,180 +1,60 @@ -# codex_execpolicy +# codex-execpolicy -The goal of this library is to classify a proposed [`execv(3)`](https://linux.die.net/man/3/execv) command into one of the following states: +## Overview +- Policy engine and CLI built around `prefix_rule(pattern=[...], decision?, match?, not_match?)`. +- This release covers the prefix-rule subset of the execpolicy language; a richer language will follow. +- Tokens are matched in order; any `pattern` element may be a list to denote alternatives. `decision` defaults to `allow`; valid values: `allow`, `prompt`, `forbidden`. +- `match` / `not_match` supply example invocations that are validated at load time (think of them as unit tests); examples can be token arrays or strings (strings are tokenized with `shlex`). +- The CLI always prints the JSON serialization of the evaluation result. +- The legacy rule matcher lives in `codex-execpolicy-legacy`. -- `safe` The command is safe to run (\*). -- `match` The command matched a rule in the policy, but the caller should decide whether it is safe to run based on the files it will write. -- `forbidden` The command is not allowed to be run. -- `unverified` The safety cannot be determined: make the user decide. - -(\*) Whether an `execv(3)` call should be considered "safe" often requires additional context beyond the arguments to `execv()` itself. For example, if you trust an autonomous software agent to write files in your source tree, then deciding whether `/bin/cp foo bar` is "safe" depends on `getcwd(3)` for the calling process as well as the `realpath` of `foo` and `bar` when resolved against `getcwd()`. -To that end, rather than returning a boolean, the validator returns a structured result that the client is expected to use to determine the "safety" of the proposed `execv()` call. - -For example, to check the command `ls -l foo`, the checker would be invoked as follows: - -```shell -cargo run -- check ls -l foo | jq -``` - -It will exit with `0` and print the following to stdout: - -```json -{ - "result": "safe", - "match": { - "program": "ls", - "flags": [ - { - "name": "-l" - } - ], - "opts": [], - "args": [ - { - "index": 1, - "type": "ReadableFile", - "value": "foo" - } - ], - "system_path": ["/bin/ls", "/usr/bin/ls"] - } -} -``` - -Of note: - -- `foo` is tagged as a `ReadableFile`, so the caller should resolve `foo` relative to `getcwd()` and `realpath` it (as it may be a symlink) to determine whether `foo` is safe to read. -- While the specified executable is `ls`, `"system_path"` offers `/bin/ls` and `/usr/bin/ls` as viable alternatives to avoid using whatever `ls` happens to appear first on the user's `$PATH`. If either exists on the host, it is recommended to use it as the first argument to `execv(3)` instead of `ls`. - -Further, "safety" in this system is not a guarantee that the command will execute successfully. As an example, `cat /Users/mbolin/code/codex/README.md` may be considered "safe" if the system has decided the agent is allowed to read anything under `/Users/mbolin/code/codex`, but it will fail at runtime if `README.md` does not exist. (Though this is "safe" in that the agent did not read any files that it was not authorized to read.) - -## Policy - -Currently, the default policy is defined in [`default.policy`](./src/default.policy) within the crate. - -The system uses [Starlark](https://bazel.build/rules/language) as the file format because, unlike something like JSON or YAML, it supports "macros" without compromising on safety or reproducibility. (Under the hood, we use [`starlark-rust`](https://github.com/facebook/starlark-rust) as the specific Starlark implementation.) - -This policy contains "rules" such as: - -```python -define_program( - program="cp", - options=[ - flag("-r"), - flag("-R"), - flag("--recursive"), - ], - args=[ARG_RFILES, ARG_WFILE], - system_path=["/bin/cp", "/usr/bin/cp"], - should_match=[ - ["foo", "bar"], - ], - should_not_match=[ - ["foo"], - ], +## Policy shapes +- Prefix rules use Starlark syntax: +```starlark +prefix_rule( + pattern = ["cmd", ["alt1", "alt2"]], # ordered tokens; list entries denote alternatives + decision = "prompt", # allow | prompt | forbidden; defaults to allow + match = [["cmd", "alt1"], "cmd alt2"], # examples that must match this rule + not_match = [["cmd", "oops"], "cmd alt3"], # examples that must not match this rule ) ``` -This rule means that: - -- `cp` can be used with any of the following flags (where "flag" means "an option that does not take an argument"): `-r`, `-R`, `--recursive`. -- The initial `ARG_RFILES` passed to `args` means that it expects one or more arguments that correspond to "readable files" -- The final `ARG_WFILE` passed to `args` means that it expects exactly one argument that corresponds to a "writeable file." -- As a means of a lightweight way of including a unit test alongside the definition, the `should_match` list is a list of examples of `execv(3)` args that should match the rule and `should_not_match` is a list of examples that should not match. These examples are verified when the `.policy` file is loaded. - -Note that the language of the `.policy` file is still evolving, as we have to continue to expand it so it is sufficiently expressive to accept all commands we want to consider "safe" without allowing unsafe commands to pass through. - -The integrity of `default.policy` is verified [via unit tests](./tests). - -Further, the CLI supports a `--policy` option to specify a custom `.policy` file for ad-hoc testing. - -## Output Type: `match` - -Going back to the `cp` example, because the rule matches an `ARG_WFILE`, it will return `match` instead of `safe`: - -```shell -cargo run -- check cp src1 src2 dest | jq +## CLI +- Provide one or more policy files (for example `src/default.codexpolicy`) to check a command: +```bash +cargo run -p codex-execpolicy -- check --policy path/to/policy.codexpolicy git status ``` +- Pass multiple `--policy` flags to merge rules, evaluated in the order provided: +```bash +cargo run -p codex-execpolicy -- check --policy base.codexpolicy --policy overrides.codexpolicy git status +``` +- Output is JSON by default; pass `--pretty` for pretty-printed JSON +- Example outcomes: + - Match: `{"match": { ... "decision": "allow" ... }}` + - No match: `"noMatch"` -If the caller wants to consider allowing this command, it should parse the JSON to pick out the `WriteableFile` arguments and decide whether they are safe to write: - +## Response shapes +- Match: ```json { - "result": "match", "match": { - "program": "cp", - "flags": [], - "opts": [], - "args": [ + "decision": "allow|prompt|forbidden", + "matchedRules": [ { - "index": 0, - "type": "ReadableFile", - "value": "src1" - }, - { - "index": 1, - "type": "ReadableFile", - "value": "src2" - }, - { - "index": 2, - "type": "WriteableFile", - "value": "dest" + "prefixRuleMatch": { + "matchedPrefix": ["", "..."], + "decision": "allow|prompt|forbidden" + } } - ], - "system_path": ["/bin/cp", "/usr/bin/cp"] + ] } } ``` -Note the exit code is still `0` for a `match` unless the `--require-safe` flag is specified, in which case the exit code is `12`. - -## Output Type: `forbidden` - -It is also possible to define a rule that, if it matches a command, should flag it as _forbidden_. For example, we do not want agents to be able to run `applied deploy` _ever_, so we define the following rule: - -```python -define_program( - program="applied", - args=["deploy"], - forbidden="Infrastructure Risk: command contains 'applied deploy'", - should_match=[ - ["deploy"], - ], - should_not_match=[ - ["lint"], - ], -) -``` - -Note that for a rule to be forbidden, the `forbidden` keyword arg must be specified as the reason the command is forbidden. This will be included in the output: - -```shell -cargo run -- check applied deploy | jq -``` - +- No match: ```json -{ - "result": "forbidden", - "reason": "Infrastructure Risk: command contains 'applied deploy'", - "cause": { - "Exec": { - "exec": { - "program": "applied", - "flags": [], - "opts": [], - "args": [ - { - "index": 0, - "type": { - "Literal": "deploy" - }, - "value": "deploy" - } - ], - "system_path": [] - } - } - } -} +"noMatch" ``` + +- `matchedRules` lists every rule whose prefix matched the command; `matchedPrefix` is the exact prefix that matched. +- The effective `decision` is the strictest severity across all matches (`forbidden` > `prompt` > `allow`). diff --git a/codex-rs/execpolicy2/examples/example.codexpolicy b/codex-rs/execpolicy/examples/example.codexpolicy similarity index 100% rename from codex-rs/execpolicy2/examples/example.codexpolicy rename to codex-rs/execpolicy/examples/example.codexpolicy diff --git a/codex-rs/execpolicy2/src/decision.rs b/codex-rs/execpolicy/src/decision.rs similarity index 100% rename from codex-rs/execpolicy2/src/decision.rs rename to codex-rs/execpolicy/src/decision.rs diff --git a/codex-rs/execpolicy/src/error.rs b/codex-rs/execpolicy/src/error.rs index e6443d69dc..2f168a027e 100644 --- a/codex-rs/execpolicy/src/error.rs +++ b/codex-rs/execpolicy/src/error.rs @@ -1,96 +1,26 @@ -use std::path::PathBuf; - -use serde::Serialize; - -use crate::arg_matcher::ArgMatcher; -use crate::arg_resolver::PositionalArg; -use serde_with::DisplayFromStr; -use serde_with::serde_as; +use starlark::Error as StarlarkError; +use thiserror::Error; pub type Result = std::result::Result; -#[serde_as] -#[derive(Debug, Eq, PartialEq, Serialize)] -#[serde(tag = "type")] +#[derive(Debug, Error)] pub enum Error { - NoSpecForProgram { - program: String, - }, - OptionMissingValue { - program: String, - option: String, - }, - OptionFollowedByOptionInsteadOfValue { - program: String, - option: String, - value: String, - }, - UnknownOption { - program: String, - option: String, - }, - UnexpectedArguments { - program: String, - args: Vec, - }, - DoubleDashNotSupportedYet { - program: String, - }, - MultipleVarargPatterns { - program: String, - first: ArgMatcher, - second: ArgMatcher, - }, - RangeStartExceedsEnd { - start: usize, - end: usize, - }, - RangeEndOutOfBounds { - end: usize, - len: usize, - }, - PrefixOverlapsSuffix {}, - NotEnoughArgs { - program: String, - args: Vec, - arg_patterns: Vec, - }, - InternalInvariantViolation { - message: String, - }, - VarargMatcherDidNotMatchAnything { - program: String, - matcher: ArgMatcher, - }, - EmptyFileName {}, - LiteralValueDidNotMatch { - expected: String, - actual: String, - }, - InvalidPositiveInteger { - value: String, - }, - MissingRequiredOptions { - program: String, - options: Vec, - }, - SedCommandNotProvablySafe { - command: String, - }, - ReadablePathNotInReadableFolders { - file: PathBuf, - folders: Vec, - }, - WriteablePathNotInWriteableFolders { - file: PathBuf, - folders: Vec, - }, - CannotCheckRelativePath { - file: PathBuf, - }, - CannotCanonicalizePath { - file: String, - #[serde_as(as = "DisplayFromStr")] - error: std::io::ErrorKind, + #[error("invalid decision: {0}")] + InvalidDecision(String), + #[error("invalid pattern element: {0}")] + InvalidPattern(String), + #[error("invalid example: {0}")] + InvalidExample(String), + #[error( + "expected every example to match at least one rule. rules: {rules:?}; unmatched examples: \ + {examples:?}" + )] + ExampleDidNotMatch { + rules: Vec, + examples: Vec, }, + #[error("expected example to not match rule `{rule}`: {example}")] + ExampleDidMatch { rule: String, example: String }, + #[error("starlark error: {0}")] + Starlark(StarlarkError), } diff --git a/codex-rs/execpolicy/src/lib.rs b/codex-rs/execpolicy/src/lib.rs index 6f12225981..1b789fd862 100644 --- a/codex-rs/execpolicy/src/lib.rs +++ b/codex-rs/execpolicy/src/lib.rs @@ -1,45 +1,15 @@ -#![allow(clippy::type_complexity)] -#![allow(clippy::too_many_arguments)] -#[macro_use] -extern crate starlark; +pub mod decision; +pub mod error; +pub mod parser; +pub mod policy; +pub mod rule; -mod arg_matcher; -mod arg_resolver; -mod arg_type; -mod error; -mod exec_call; -mod execv_checker; -mod opt; -mod policy; -mod policy_parser; -mod program; -mod sed_command; -mod valid_exec; - -pub use arg_matcher::ArgMatcher; -pub use arg_resolver::PositionalArg; -pub use arg_type::ArgType; +pub use decision::Decision; pub use error::Error; pub use error::Result; -pub use exec_call::ExecCall; -pub use execv_checker::ExecvChecker; -pub use opt::Opt; +pub use parser::PolicyParser; +pub use policy::Evaluation; pub use policy::Policy; -pub use policy_parser::PolicyParser; -pub use program::Forbidden; -pub use program::MatchedExec; -pub use program::NegativeExamplePassedCheck; -pub use program::PositiveExampleFailedCheck; -pub use program::ProgramSpec; -pub use sed_command::parse_sed_command; -pub use valid_exec::MatchedArg; -pub use valid_exec::MatchedFlag; -pub use valid_exec::MatchedOpt; -pub use valid_exec::ValidExec; - -const DEFAULT_POLICY: &str = include_str!("default.policy"); - -pub fn get_default_policy() -> starlark::Result { - let parser = PolicyParser::new("#default", DEFAULT_POLICY); - parser.parse() -} +pub use rule::Rule; +pub use rule::RuleMatch; +pub use rule::RuleRef; diff --git a/codex-rs/execpolicy/src/main.rs b/codex-rs/execpolicy/src/main.rs index 68a72c04ff..318abb5cae 100644 --- a/codex-rs/execpolicy/src/main.rs +++ b/codex-rs/execpolicy/src/main.rs @@ -1,167 +1,66 @@ +use std::fs; +use std::path::PathBuf; + +use anyhow::Context; use anyhow::Result; use clap::Parser; -use clap::Subcommand; -use codex_execpolicy::ExecCall; -use codex_execpolicy::MatchedExec; -use codex_execpolicy::Policy; use codex_execpolicy::PolicyParser; -use codex_execpolicy::ValidExec; -use codex_execpolicy::get_default_policy; -use serde::Deserialize; -use serde::Serialize; -use serde::de; -use starlark::Error as StarlarkError; -use std::path::PathBuf; -use std::str::FromStr; -const MATCHED_BUT_WRITES_FILES_EXIT_CODE: i32 = 12; -const MIGHT_BE_SAFE_EXIT_CODE: i32 = 13; -const FORBIDDEN_EXIT_CODE: i32 = 14; - -#[derive(Parser, Deserialize, Debug)] -#[command(version, about, long_about = None)] -pub struct Args { - /// If the command fails the policy, exit with 13, but print parseable JSON - /// to stdout. - #[clap(long)] - pub require_safe: bool, - - /// Path to the policy file. - #[clap(long, short = 'p')] - pub policy: Option, - - #[command(subcommand)] - pub command: Command, -} - -#[derive(Clone, Debug, Deserialize, Subcommand)] -pub enum Command { - /// Checks the command as if the arguments were the inputs to execv(3). +/// CLI for evaluating exec policies +#[derive(Parser)] +#[command(name = "codex-execpolicy")] +enum Cli { + /// Evaluate a command against a policy. Check { - #[arg(trailing_var_arg = true)] + #[arg(short, long = "policy", value_name = "PATH", required = true)] + policies: Vec, + + /// Pretty-print the JSON output. + #[arg(long)] + pretty: bool, + + /// Command tokens to check. + #[arg( + value_name = "COMMAND", + required = true, + trailing_var_arg = true, + allow_hyphen_values = true + )] command: Vec, }, - - /// Checks the command encoded as a JSON object. - #[clap(name = "check-json")] - CheckJson { - /// JSON object with "program" (str) and "args" (list[str]) fields. - #[serde(deserialize_with = "deserialize_from_json")] - exec: ExecArg, - }, -} - -#[derive(Clone, Debug, Deserialize)] -pub struct ExecArg { - pub program: String, - - #[serde(default)] - pub args: Vec, } fn main() -> Result<()> { - env_logger::init(); + let cli = Cli::parse(); + match cli { + Cli::Check { + policies, + command, + pretty, + } => cmd_check(policies, command, pretty), + } +} - let args = Args::parse(); - let policy = match args.policy { - Some(policy) => { - let policy_source = policy.to_string_lossy().to_string(); - let unparsed_policy = std::fs::read_to_string(policy)?; - let parser = PolicyParser::new(&policy_source, &unparsed_policy); - parser.parse() - } - None => get_default_policy(), +fn cmd_check(policy_paths: Vec, args: Vec, pretty: bool) -> Result<()> { + let policy = load_policies(&policy_paths)?; + + let eval = policy.check(&args); + let json = if pretty { + serde_json::to_string_pretty(&eval)? + } else { + serde_json::to_string(&eval)? }; - let policy = policy.map_err(StarlarkError::into_anyhow)?; - - let exec = match args.command { - Command::Check { command } => match command.split_first() { - Some((first, rest)) => ExecArg { - program: first.to_string(), - args: rest.to_vec(), - }, - None => { - eprintln!("no command provided"); - std::process::exit(1); - } - }, - Command::CheckJson { exec } => exec, - }; - - let (output, exit_code) = check_command(&policy, exec, args.require_safe); - let json = serde_json::to_string(&output)?; println!("{json}"); - std::process::exit(exit_code); + Ok(()) } -fn check_command( - policy: &Policy, - ExecArg { program, args }: ExecArg, - check: bool, -) -> (Output, i32) { - let exec_call = ExecCall { program, args }; - match policy.check(&exec_call) { - Ok(MatchedExec::Match { exec }) => { - if exec.might_write_files() { - let exit_code = if check { - MATCHED_BUT_WRITES_FILES_EXIT_CODE - } else { - 0 - }; - (Output::Match { r#match: exec }, exit_code) - } else { - (Output::Safe { r#match: exec }, 0) - } - } - Ok(MatchedExec::Forbidden { reason, cause }) => { - let exit_code = if check { FORBIDDEN_EXIT_CODE } else { 0 }; - (Output::Forbidden { reason, cause }, exit_code) - } - Err(err) => { - let exit_code = if check { MIGHT_BE_SAFE_EXIT_CODE } else { 0 }; - (Output::Unverified { error: err }, exit_code) - } - } -} - -#[derive(Debug, Serialize)] -#[serde(tag = "result")] -pub enum Output { - /// The command is verified as safe. - #[serde(rename = "safe")] - Safe { r#match: ValidExec }, - - /// The command has matched a rule in the policy, but the caller should - /// decide whether it is "safe" given the files it wants to write. - #[serde(rename = "match")] - Match { r#match: ValidExec }, - - /// The user is forbidden from running the command. - #[serde(rename = "forbidden")] - Forbidden { - reason: String, - cause: codex_execpolicy::Forbidden, - }, - - /// The safety of the command could not be verified. - #[serde(rename = "unverified")] - Unverified { error: codex_execpolicy::Error }, -} - -fn deserialize_from_json<'de, D>(deserializer: D) -> Result -where - D: de::Deserializer<'de>, -{ - let s = String::deserialize(deserializer)?; - let decoded = serde_json::from_str(&s) - .map_err(|e| serde::de::Error::custom(format!("JSON parse error: {e}")))?; - Ok(decoded) -} - -impl FromStr for ExecArg { - type Err = anyhow::Error; - - fn from_str(s: &str) -> Result { - serde_json::from_str(s).map_err(Into::into) +fn load_policies(policy_paths: &[PathBuf]) -> Result { + let mut parser = PolicyParser::new(); + for policy_path in policy_paths { + let policy_file_contents = fs::read_to_string(policy_path) + .with_context(|| format!("failed to read policy at {}", policy_path.display()))?; + let policy_identifier = policy_path.to_string_lossy().to_string(); + parser.parse(&policy_identifier, &policy_file_contents)?; } + Ok(parser.build()) } diff --git a/codex-rs/execpolicy2/src/parser.rs b/codex-rs/execpolicy/src/parser.rs similarity index 100% rename from codex-rs/execpolicy2/src/parser.rs rename to codex-rs/execpolicy/src/parser.rs diff --git a/codex-rs/execpolicy/src/policy.rs b/codex-rs/execpolicy/src/policy.rs index 825d6164a5..a543e5c4b0 100644 --- a/codex-rs/execpolicy/src/policy.rs +++ b/codex-rs/execpolicy/src/policy.rs @@ -1,103 +1,84 @@ +use crate::decision::Decision; +use crate::rule::RuleMatch; +use crate::rule::RuleRef; use multimap::MultiMap; -use regex_lite::Error as RegexError; -use regex_lite::Regex; - -use crate::ExecCall; -use crate::Forbidden; -use crate::MatchedExec; -use crate::NegativeExamplePassedCheck; -use crate::ProgramSpec; -use crate::error::Error; -use crate::error::Result; -use crate::policy_parser::ForbiddenProgramRegex; -use crate::program::PositiveExampleFailedCheck; +use serde::Deserialize; +use serde::Serialize; +#[derive(Clone, Debug)] pub struct Policy { - programs: MultiMap, - forbidden_program_regexes: Vec, - forbidden_substrings_pattern: Option, + rules_by_program: MultiMap, } impl Policy { - pub fn new( - programs: MultiMap, - forbidden_program_regexes: Vec, - forbidden_substrings: Vec, - ) -> std::result::Result { - let forbidden_substrings_pattern = if forbidden_substrings.is_empty() { - None - } else { - let escaped_substrings = forbidden_substrings - .iter() - .map(|s| regex_lite::escape(s)) - .collect::>() - .join("|"); - Some(Regex::new(&format!("({escaped_substrings})"))?) + pub fn new(rules_by_program: MultiMap) -> Self { + Self { rules_by_program } + } + + pub fn empty() -> Self { + Self::new(MultiMap::new()) + } + + pub fn rules(&self) -> &MultiMap { + &self.rules_by_program + } + + pub fn check(&self, cmd: &[String]) -> Evaluation { + let rules = match cmd.first() { + Some(first) => match self.rules_by_program.get_vec(first) { + Some(rules) => rules, + None => return Evaluation::NoMatch, + }, + None => return Evaluation::NoMatch, }; - Ok(Self { - programs, - forbidden_program_regexes, - forbidden_substrings_pattern, - }) + + let matched_rules: Vec = + rules.iter().filter_map(|rule| rule.matches(cmd)).collect(); + match matched_rules.iter().map(RuleMatch::decision).max() { + Some(decision) => Evaluation::Match { + decision, + matched_rules, + }, + None => Evaluation::NoMatch, + } } - pub fn check(&self, exec_call: &ExecCall) -> Result { - let ExecCall { program, args } = &exec_call; - for ForbiddenProgramRegex { regex, reason } in &self.forbidden_program_regexes { - if regex.is_match(program) { - return Ok(MatchedExec::Forbidden { - cause: Forbidden::Program { - program: program.clone(), - exec_call: exec_call.clone(), - }, - reason: reason.clone(), - }); - } - } + pub fn check_multiple(&self, commands: Commands) -> Evaluation + where + Commands: IntoIterator, + Commands::Item: AsRef<[String]>, + { + let matched_rules: Vec = commands + .into_iter() + .flat_map(|command| match self.check(command.as_ref()) { + Evaluation::Match { matched_rules, .. } => matched_rules, + Evaluation::NoMatch => Vec::new(), + }) + .collect(); - for arg in args { - if let Some(regex) = &self.forbidden_substrings_pattern - && regex.is_match(arg) - { - return Ok(MatchedExec::Forbidden { - cause: Forbidden::Arg { - arg: arg.clone(), - exec_call: exec_call.clone(), - }, - reason: format!("arg `{arg}` contains forbidden substring"), - }); - } + match matched_rules.iter().map(RuleMatch::decision).max() { + Some(decision) => Evaluation::Match { + decision, + matched_rules, + }, + None => Evaluation::NoMatch, } - - let mut last_err = Err(Error::NoSpecForProgram { - program: program.clone(), - }); - if let Some(spec_list) = self.programs.get_vec(program) { - for spec in spec_list { - match spec.check(exec_call) { - Ok(matched_exec) => return Ok(matched_exec), - Err(err) => { - last_err = Err(err); - } - } - } - } - last_err - } - - pub fn check_each_good_list_individually(&self) -> Vec { - let mut violations = Vec::new(); - for (_program, spec) in self.programs.flat_iter() { - violations.extend(spec.verify_should_match_list()); - } - violations - } - - pub fn check_each_bad_list_individually(&self) -> Vec { - let mut violations = Vec::new(); - for (_program, spec) in self.programs.flat_iter() { - violations.extend(spec.verify_should_not_match_list()); - } - violations + } +} + +#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub enum Evaluation { + NoMatch, + Match { + decision: Decision, + #[serde(rename = "matchedRules")] + matched_rules: Vec, + }, +} + +impl Evaluation { + pub fn is_match(&self) -> bool { + matches!(self, Self::Match { .. }) } } diff --git a/codex-rs/execpolicy2/src/rule.rs b/codex-rs/execpolicy/src/rule.rs similarity index 100% rename from codex-rs/execpolicy2/src/rule.rs rename to codex-rs/execpolicy/src/rule.rs diff --git a/codex-rs/execpolicy2/tests/basic.rs b/codex-rs/execpolicy/tests/basic.rs similarity index 97% rename from codex-rs/execpolicy2/tests/basic.rs rename to codex-rs/execpolicy/tests/basic.rs index 75921ecd57..301f962616 100644 --- a/codex-rs/execpolicy2/tests/basic.rs +++ b/codex-rs/execpolicy/tests/basic.rs @@ -1,14 +1,14 @@ use std::any::Any; use std::sync::Arc; -use codex_execpolicy2::Decision; -use codex_execpolicy2::Evaluation; -use codex_execpolicy2::PolicyParser; -use codex_execpolicy2::RuleMatch; -use codex_execpolicy2::RuleRef; -use codex_execpolicy2::rule::PatternToken; -use codex_execpolicy2::rule::PrefixPattern; -use codex_execpolicy2::rule::PrefixRule; +use codex_execpolicy::Decision; +use codex_execpolicy::Evaluation; +use codex_execpolicy::PolicyParser; +use codex_execpolicy::RuleMatch; +use codex_execpolicy::RuleRef; +use codex_execpolicy::rule::PatternToken; +use codex_execpolicy::rule::PrefixPattern; +use codex_execpolicy::rule::PrefixRule; use pretty_assertions::assert_eq; fn tokens(cmd: &[&str]) -> Vec { diff --git a/codex-rs/execpolicy2/Cargo.toml b/codex-rs/execpolicy2/Cargo.toml deleted file mode 100644 index c86fb7ff7a..0000000000 --- a/codex-rs/execpolicy2/Cargo.toml +++ /dev/null @@ -1,29 +0,0 @@ -[package] -name = "codex-execpolicy2" -version = { workspace = true } -edition = "2024" -description = "Codex exec policy v2: prefix-based Starlark rules for command decisions." - -[lib] -name = "codex_execpolicy2" -path = "src/lib.rs" - -[[bin]] -name = "codex-execpolicy2" -path = "src/main.rs" - -[lints] -workspace = true - -[dependencies] -anyhow = { workspace = true } -clap = { workspace = true, features = ["derive"] } -multimap = { workspace = true } -serde = { workspace = true, features = ["derive"] } -serde_json = { workspace = true } -shlex = { workspace = true } -starlark = { workspace = true } -thiserror = { workspace = true } - -[dev-dependencies] -pretty_assertions = { workspace = true } diff --git a/codex-rs/execpolicy2/README.md b/codex-rs/execpolicy2/README.md deleted file mode 100644 index d77ec3838d..0000000000 --- a/codex-rs/execpolicy2/README.md +++ /dev/null @@ -1,59 +0,0 @@ -# codex-execpolicy2 - -## Overview -- Policy engine and CLI built around `prefix_rule(pattern=[...], decision?, match?, not_match?)`. -- This release covers only the prefix-rule subset of the planned execpolicy v2 language; a richer language will follow. -- Tokens are matched in order; any `pattern` element may be a list to denote alternatives. `decision` defaults to `allow`; valid values: `allow`, `prompt`, `forbidden`. -- `match` / `not_match` supply example invocations that are validated at load time (think of them as unit tests); examples can be token arrays or strings (strings are tokenized with `shlex`). -- The CLI always prints the JSON serialization of the evaluation result. - -## Policy shapes -- Prefix rules use Starlark syntax: -```starlark -prefix_rule( - pattern = ["cmd", ["alt1", "alt2"]], # ordered tokens; list entries denote alternatives - decision = "prompt", # allow | prompt | forbidden; defaults to allow - match = [["cmd", "alt1"], "cmd alt2"], # examples that must match this rule - not_match = [["cmd", "oops"], "cmd alt3"], # examples that must not match this rule -) -``` - -## CLI -- Provide one or more policy files (for example `src/default.codexpolicy`) to check a command: -```bash -cargo run -p codex-execpolicy2 -- check --policy path/to/policy.codexpolicy git status -``` -- Pass multiple `--policy` flags to merge rules, evaluated in the order provided: -```bash -cargo run -p codex-execpolicy2 -- check --policy base.codexpolicy --policy overrides.codexpolicy git status -``` -- Output is JSON by default; pass `--pretty` for pretty-printed JSON -- Example outcomes: - - Match: `{"match": { ... "decision": "allow" ... }}` - - No match: `"noMatch"` - -## Response shapes -- Match: -```json -{ - "match": { - "decision": "allow|prompt|forbidden", - "matchedRules": [ - { - "prefixRuleMatch": { - "matchedPrefix": ["", "..."], - "decision": "allow|prompt|forbidden" - } - } - ] - } -} -``` - -- No match: -```json -"noMatch" -``` - -- `matchedRules` lists every rule whose prefix matched the command; `matchedPrefix` is the exact prefix that matched. -- The effective `decision` is the strictest severity across all matches (`forbidden` > `prompt` > `allow`). diff --git a/codex-rs/execpolicy2/src/error.rs b/codex-rs/execpolicy2/src/error.rs deleted file mode 100644 index 2f168a027e..0000000000 --- a/codex-rs/execpolicy2/src/error.rs +++ /dev/null @@ -1,26 +0,0 @@ -use starlark::Error as StarlarkError; -use thiserror::Error; - -pub type Result = std::result::Result; - -#[derive(Debug, Error)] -pub enum Error { - #[error("invalid decision: {0}")] - InvalidDecision(String), - #[error("invalid pattern element: {0}")] - InvalidPattern(String), - #[error("invalid example: {0}")] - InvalidExample(String), - #[error( - "expected every example to match at least one rule. rules: {rules:?}; unmatched examples: \ - {examples:?}" - )] - ExampleDidNotMatch { - rules: Vec, - examples: Vec, - }, - #[error("expected example to not match rule `{rule}`: {example}")] - ExampleDidMatch { rule: String, example: String }, - #[error("starlark error: {0}")] - Starlark(StarlarkError), -} diff --git a/codex-rs/execpolicy2/src/lib.rs b/codex-rs/execpolicy2/src/lib.rs deleted file mode 100644 index 1b789fd862..0000000000 --- a/codex-rs/execpolicy2/src/lib.rs +++ /dev/null @@ -1,15 +0,0 @@ -pub mod decision; -pub mod error; -pub mod parser; -pub mod policy; -pub mod rule; - -pub use decision::Decision; -pub use error::Error; -pub use error::Result; -pub use parser::PolicyParser; -pub use policy::Evaluation; -pub use policy::Policy; -pub use rule::Rule; -pub use rule::RuleMatch; -pub use rule::RuleRef; diff --git a/codex-rs/execpolicy2/src/main.rs b/codex-rs/execpolicy2/src/main.rs deleted file mode 100644 index f654c5f84a..0000000000 --- a/codex-rs/execpolicy2/src/main.rs +++ /dev/null @@ -1,66 +0,0 @@ -use std::fs; -use std::path::PathBuf; - -use anyhow::Context; -use anyhow::Result; -use clap::Parser; -use codex_execpolicy2::PolicyParser; - -/// CLI for evaluating exec policies -#[derive(Parser)] -#[command(name = "codex-execpolicy2")] -enum Cli { - /// Evaluate a command against a policy. - Check { - #[arg(short, long = "policy", value_name = "PATH", required = true)] - policies: Vec, - - /// Pretty-print the JSON output. - #[arg(long)] - pretty: bool, - - /// Command tokens to check. - #[arg( - value_name = "COMMAND", - required = true, - trailing_var_arg = true, - allow_hyphen_values = true - )] - command: Vec, - }, -} - -fn main() -> Result<()> { - let cli = Cli::parse(); - match cli { - Cli::Check { - policies, - command, - pretty, - } => cmd_check(policies, command, pretty), - } -} - -fn cmd_check(policy_paths: Vec, args: Vec, pretty: bool) -> Result<()> { - let policy = load_policies(&policy_paths)?; - - let eval = policy.check(&args); - let json = if pretty { - serde_json::to_string_pretty(&eval)? - } else { - serde_json::to_string(&eval)? - }; - println!("{json}"); - Ok(()) -} - -fn load_policies(policy_paths: &[PathBuf]) -> Result { - let mut parser = PolicyParser::new(); - for policy_path in policy_paths { - let policy_file_contents = fs::read_to_string(policy_path) - .with_context(|| format!("failed to read policy at {}", policy_path.display()))?; - let policy_identifier = policy_path.to_string_lossy().to_string(); - parser.parse(&policy_identifier, &policy_file_contents)?; - } - Ok(parser.build()) -} diff --git a/codex-rs/execpolicy2/src/policy.rs b/codex-rs/execpolicy2/src/policy.rs deleted file mode 100644 index a543e5c4b0..0000000000 --- a/codex-rs/execpolicy2/src/policy.rs +++ /dev/null @@ -1,84 +0,0 @@ -use crate::decision::Decision; -use crate::rule::RuleMatch; -use crate::rule::RuleRef; -use multimap::MultiMap; -use serde::Deserialize; -use serde::Serialize; - -#[derive(Clone, Debug)] -pub struct Policy { - rules_by_program: MultiMap, -} - -impl Policy { - pub fn new(rules_by_program: MultiMap) -> Self { - Self { rules_by_program } - } - - pub fn empty() -> Self { - Self::new(MultiMap::new()) - } - - pub fn rules(&self) -> &MultiMap { - &self.rules_by_program - } - - pub fn check(&self, cmd: &[String]) -> Evaluation { - let rules = match cmd.first() { - Some(first) => match self.rules_by_program.get_vec(first) { - Some(rules) => rules, - None => return Evaluation::NoMatch, - }, - None => return Evaluation::NoMatch, - }; - - let matched_rules: Vec = - rules.iter().filter_map(|rule| rule.matches(cmd)).collect(); - match matched_rules.iter().map(RuleMatch::decision).max() { - Some(decision) => Evaluation::Match { - decision, - matched_rules, - }, - None => Evaluation::NoMatch, - } - } - - pub fn check_multiple(&self, commands: Commands) -> Evaluation - where - Commands: IntoIterator, - Commands::Item: AsRef<[String]>, - { - let matched_rules: Vec = commands - .into_iter() - .flat_map(|command| match self.check(command.as_ref()) { - Evaluation::Match { matched_rules, .. } => matched_rules, - Evaluation::NoMatch => Vec::new(), - }) - .collect(); - - match matched_rules.iter().map(RuleMatch::decision).max() { - Some(decision) => Evaluation::Match { - decision, - matched_rules, - }, - None => Evaluation::NoMatch, - } - } -} - -#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] -#[serde(rename_all = "camelCase")] -pub enum Evaluation { - NoMatch, - Match { - decision: Decision, - #[serde(rename = "matchedRules")] - matched_rules: Vec, - }, -} - -impl Evaluation { - pub fn is_match(&self) -> bool { - matches!(self, Self::Match { .. }) - } -} From d6c30ed25eb9667bb14eb8af20e7b712b25aee64 Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Wed, 19 Nov 2025 20:13:31 -0800 Subject: [PATCH 7/9] [app-server] feat: v2 apply_patch approval flow (#6760) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR adds the API V2 version of the apply_patch approval flow, which centers around `ThreadItem::FileChange`. This PR wires the new RPC (`item/fileChange/requestApproval`, V2 only) and related events (`item/started`, `item/completed` for `ThreadItem::FileChange`, which are emitted in both V1 and V2) through the app-server protocol. The new approval RPC is only sent when the user initiates a turn with the new `turn/start` API so we don't break backwards compatibility with VSCE. Similar to https://github.com/openai/codex/pull/6758, the approach I took was to make as few changes to the Codex core as possible, leveraging existing `EventMsg` core events, and translating those in app-server. I did have to add a few additional fields to `EventMsg::PatchApplyBegin` and `EventMsg::PatchApplyEnd`, but those were fairly lightweight. However, the `EventMsg`s emitted by core are the following: ``` 1) Auto-approved (no request for approval)
 - EventMsg::PatchApplyBegin - EventMsg::PatchApplyEnd 2) Approved by user - EventMsg::ApplyPatchApprovalRequest - EventMsg::PatchApplyBegin - EventMsg::PatchApplyEnd 3) Declined by user - EventMsg::ApplyPatchApprovalRequest - EventMsg::PatchApplyBegin - EventMsg::PatchApplyEnd ``` For a request triggering an approval, this would result in: ``` item/fileChange/requestApproval item/started item/completed ``` which is different from the `ThreadItem::CommandExecution` flow introduced in https://github.com/openai/codex/pull/6758, which does the below and is preferable: ``` item/started item/commandExecution/requestApproval item/completed ``` To fix this, we leverage `TurnSummaryStore` on codex_message_processor to store a little bit of state, allowing us to fire `item/started` and `item/fileChange/requestApproval` whenever we receive the underlying `EventMsg::ApplyPatchApprovalRequest`, and no-oping when we receive the `EventMsg::PatchApplyBegin` later. This is much less invasive than modifying the order of EventMsg within core (I tried). The resulting payloads: ``` { "method": "item/started", "params": { "item": { "changes": [ { "diff": "Hello from Codex!\n", "kind": "add", "path": "/Users/owen/repos/codex/codex-rs/APPROVAL_DEMO.txt" } ], "id": "call_Nxnwj7B3YXigfV6Mwh03d686", "status": "inProgress", "type": "fileChange" } } } ``` ``` { "id": 0, "method": "item/fileChange/requestApproval", "params": { "grantRoot": null, "itemId": "call_Nxnwj7B3YXigfV6Mwh03d686", "reason": null, "threadId": "019a9e11-8295-7883-a283-779e06502c6f", "turnId": "1" } } ``` ``` { "id": 0, "result": { "decision": "accept" } } ``` ``` { "method": "item/completed", "params": { "item": { "changes": [ { "diff": "Hello from Codex!\n", "kind": "add", "path": "/Users/owen/repos/codex/codex-rs/APPROVAL_DEMO.txt" } ], "id": "call_Nxnwj7B3YXigfV6Mwh03d686", "status": "completed", "type": "fileChange" } } } ``` --- .../src/protocol/common.rs | 7 + .../app-server-protocol/src/protocol/v2.rs | 27 +- codex-rs/app-server-test-client/src/main.rs | 36 +++ .../app-server/src/bespoke_event_handling.rs | 261 ++++++++++++++- .../app-server/src/codex_message_processor.rs | 2 + .../app-server/tests/suite/v2/turn_start.rs | 300 ++++++++++++++++++ codex-rs/core/src/codex.rs | 1 + codex-rs/core/src/tools/events.rs | 28 +- .../src/event_processor_with_human_output.rs | 1 + codex-rs/exec/src/exec_events.rs | 1 + .../tests/event_processor_with_json_output.rs | 6 + codex-rs/mcp-server/src/codex_tool_runner.rs | 1 + codex-rs/protocol/src/approvals.rs | 4 + codex-rs/protocol/src/protocol.rs | 11 + codex-rs/tui/src/chatwidget.rs | 1 + codex-rs/tui/src/chatwidget/tests.rs | 28 ++ 16 files changed, 693 insertions(+), 22 deletions(-) diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index 027c2561b0..650b6f31a6 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -438,6 +438,13 @@ server_request_definitions! { response: v2::CommandExecutionRequestApprovalResponse, }, + /// Sent when approval is requested for a specific file change. + /// This request is used for Turns started via turn/start. + FileChangeRequestApproval => "item/fileChange/requestApproval" { + params: v2::FileChangeRequestApprovalParams, + response: v2::FileChangeRequestApprovalResponse, + }, + /// DEPRECATED APIs below /// Request to approve a patch. /// This request is used for Turns started via the legacy APIs (i.e. SendUserTurn, SendUserMessage). diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 3c82d908d3..6b61245bc6 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -794,20 +794,23 @@ pub struct FileUpdateChange { } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] -#[serde(rename_all = "camelCase")] +#[serde(tag = "type", rename_all = "camelCase")] +#[ts(tag = "type")] #[ts(export_to = "v2/")] pub enum PatchChangeKind { Add, Delete, - Update, + Update { move_path: Option }, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] pub enum PatchApplyStatus { + InProgress, Completed, Failed, + Declined, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] @@ -982,6 +985,26 @@ pub struct CommandExecutionRequestApprovalResponse { pub accept_settings: Option, } +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct FileChangeRequestApprovalParams { + pub thread_id: String, + pub turn_id: String, + pub item_id: String, + /// Optional explanatory reason (e.g. request for extra write access). + pub reason: Option, + /// [UNSTABLE] When set, the agent is asking the user to allow writes under this root + /// for the remainder of the session (unclear if this is honored today). + pub grant_root: Option, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] +#[ts(export_to = "v2/")] +pub struct FileChangeRequestApprovalResponse { + pub decision: ApprovalDecision, +} + #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] diff --git a/codex-rs/app-server-test-client/src/main.rs b/codex-rs/app-server-test-client/src/main.rs index 6130b2d511..7f2b75bd21 100644 --- a/codex-rs/app-server-test-client/src/main.rs +++ b/codex-rs/app-server-test-client/src/main.rs @@ -24,6 +24,8 @@ use codex_app_server_protocol::ClientRequest; use codex_app_server_protocol::CommandExecutionRequestAcceptSettings; use codex_app_server_protocol::CommandExecutionRequestApprovalParams; use codex_app_server_protocol::CommandExecutionRequestApprovalResponse; +use codex_app_server_protocol::FileChangeRequestApprovalParams; +use codex_app_server_protocol::FileChangeRequestApprovalResponse; use codex_app_server_protocol::GetAccountRateLimitsResponse; use codex_app_server_protocol::InitializeParams; use codex_app_server_protocol::InitializeResponse; @@ -677,6 +679,9 @@ impl CodexClient { ServerRequest::CommandExecutionRequestApproval { request_id, params } => { self.handle_command_execution_request_approval(request_id, params)?; } + ServerRequest::FileChangeRequestApproval { request_id, params } => { + self.approve_file_change_request(request_id, params)?; + } other => { bail!("received unsupported server request: {other:?}"); } @@ -717,6 +722,37 @@ impl CodexClient { Ok(()) } + fn approve_file_change_request( + &mut self, + request_id: RequestId, + params: FileChangeRequestApprovalParams, + ) -> Result<()> { + let FileChangeRequestApprovalParams { + thread_id, + turn_id, + item_id, + reason, + grant_root, + } = params; + + println!( + "\n< fileChange approval requested for thread {thread_id}, turn {turn_id}, item {item_id}" + ); + if let Some(reason) = reason.as_deref() { + println!("< reason: {reason}"); + } + if let Some(grant_root) = grant_root.as_deref() { + println!("< grant root: {}", grant_root.display()); + } + + let response = FileChangeRequestApprovalResponse { + decision: ApprovalDecision::Accept, + }; + self.send_server_request_response(request_id, &response)?; + println!("< approved fileChange request for item {item_id}"); + Ok(()) + } + fn send_server_request_response(&mut self, request_id: RequestId, response: &T) -> Result<()> where T: Serialize, diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index b4fdbe3e03..d4b00e67f9 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -15,12 +15,17 @@ use codex_app_server_protocol::CommandExecutionRequestApprovalResponse; use codex_app_server_protocol::CommandExecutionStatus; use codex_app_server_protocol::ExecCommandApprovalParams; use codex_app_server_protocol::ExecCommandApprovalResponse; +use codex_app_server_protocol::FileChangeRequestApprovalParams; +use codex_app_server_protocol::FileChangeRequestApprovalResponse; +use codex_app_server_protocol::FileUpdateChange; use codex_app_server_protocol::InterruptConversationResponse; use codex_app_server_protocol::ItemCompletedNotification; use codex_app_server_protocol::ItemStartedNotification; use codex_app_server_protocol::McpToolCallError; use codex_app_server_protocol::McpToolCallResult; use codex_app_server_protocol::McpToolCallStatus; +use codex_app_server_protocol::PatchApplyStatus; +use codex_app_server_protocol::PatchChangeKind as V2PatchChangeKind; use codex_app_server_protocol::ReasoningSummaryPartAddedNotification; use codex_app_server_protocol::ReasoningSummaryTextDeltaNotification; use codex_app_server_protocol::ReasoningTextDeltaNotification; @@ -40,6 +45,7 @@ use codex_core::protocol::Event; use codex_core::protocol::EventMsg; use codex_core::protocol::ExecApprovalRequestEvent; use codex_core::protocol::ExecCommandEndEvent; +use codex_core::protocol::FileChange as CoreFileChange; use codex_core::protocol::McpToolCallBeginEvent; use codex_core::protocol::McpToolCallEndEvent; use codex_core::protocol::Op; @@ -47,7 +53,9 @@ use codex_core::protocol::ReviewDecision; use codex_core::review_format::format_review_findings_block; use codex_protocol::ConversationId; use codex_protocol::protocol::ReviewOutputEvent; +use std::collections::HashMap; use std::convert::TryFrom; +use std::path::PathBuf; use std::sync::Arc; use tokio::sync::oneshot; use tracing::error; @@ -70,24 +78,74 @@ pub(crate) async fn apply_bespoke_event_handling( } EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { call_id, + turn_id, changes, reason, grant_root, - }) => { - let params = ApplyPatchApprovalParams { - conversation_id, - call_id, - file_changes: changes, - reason, - grant_root, - }; - let rx = outgoing - .send_request(ServerRequestPayload::ApplyPatchApproval(params)) - .await; - tokio::spawn(async move { - on_patch_approval_response(event_id, rx, conversation).await; - }); - } + }) => match api_version { + ApiVersion::V1 => { + let params = ApplyPatchApprovalParams { + conversation_id, + call_id, + file_changes: changes.clone(), + reason, + grant_root, + }; + let rx = outgoing + .send_request(ServerRequestPayload::ApplyPatchApproval(params)) + .await; + tokio::spawn(async move { + on_patch_approval_response(event_id, rx, conversation).await; + }); + } + ApiVersion::V2 => { + // Until we migrate the core to be aware of a first class FileChangeItem + // and emit the corresponding EventMsg, we repurpose the call_id as the item_id. + let item_id = call_id.clone(); + let patch_changes = convert_patch_changes(&changes); + + let first_start = { + let mut map = turn_summary_store.lock().await; + let summary = map.entry(conversation_id).or_default(); + summary.file_change_started.insert(item_id.clone()) + }; + if first_start { + let item = ThreadItem::FileChange { + id: item_id.clone(), + changes: patch_changes.clone(), + status: PatchApplyStatus::InProgress, + }; + let notification = ItemStartedNotification { item }; + outgoing + .send_server_notification(ServerNotification::ItemStarted(notification)) + .await; + } + + let params = FileChangeRequestApprovalParams { + thread_id: conversation_id.to_string(), + turn_id: turn_id.clone(), + item_id: item_id.clone(), + reason, + grant_root, + }; + let rx = outgoing + .send_request(ServerRequestPayload::FileChangeRequestApproval(params)) + .await; + tokio::spawn(async move { + on_file_change_request_approval_response( + event_id, + conversation_id, + item_id, + patch_changes, + rx, + conversation, + outgoing, + turn_summary_store, + ) + .await; + }); + } + }, EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent { call_id, turn_id, @@ -244,6 +302,49 @@ pub(crate) async fn apply_bespoke_event_handling( .send_server_notification(ServerNotification::ItemCompleted(notification)) .await; } + EventMsg::PatchApplyBegin(patch_begin_event) => { + // Until we migrate the core to be aware of a first class FileChangeItem + // and emit the corresponding EventMsg, we repurpose the call_id as the item_id. + let item_id = patch_begin_event.call_id.clone(); + + let first_start = { + let mut map = turn_summary_store.lock().await; + let summary = map.entry(conversation_id).or_default(); + summary.file_change_started.insert(item_id.clone()) + }; + if first_start { + let item = ThreadItem::FileChange { + id: item_id.clone(), + changes: convert_patch_changes(&patch_begin_event.changes), + status: PatchApplyStatus::InProgress, + }; + let notification = ItemStartedNotification { item }; + outgoing + .send_server_notification(ServerNotification::ItemStarted(notification)) + .await; + } + } + EventMsg::PatchApplyEnd(patch_end_event) => { + // Until we migrate the core to be aware of a first class FileChangeItem + // and emit the corresponding EventMsg, we repurpose the call_id as the item_id. + let item_id = patch_end_event.call_id.clone(); + + let status = if patch_end_event.success { + PatchApplyStatus::Completed + } else { + PatchApplyStatus::Failed + }; + let changes = convert_patch_changes(&patch_end_event.changes); + complete_file_change_item( + conversation_id, + item_id, + changes, + status, + outgoing.as_ref(), + &turn_summary_store, + ) + .await; + } EventMsg::ExecCommandBegin(exec_command_begin_event) => { let item = ThreadItem::CommandExecution { id: exec_command_begin_event.call_id.clone(), @@ -365,6 +466,32 @@ async fn emit_turn_completed_with_status( .await; } +async fn complete_file_change_item( + conversation_id: ConversationId, + item_id: String, + changes: Vec, + status: PatchApplyStatus, + outgoing: &OutgoingMessageSender, + turn_summary_store: &TurnSummaryStore, +) { + { + let mut map = turn_summary_store.lock().await; + if let Some(summary) = map.get_mut(&conversation_id) { + summary.file_change_started.remove(&item_id); + } + } + + let item = ThreadItem::FileChange { + id: item_id, + changes, + status, + }; + let notification = ItemCompletedNotification { item }; + outgoing + .send_server_notification(ServerNotification::ItemCompleted(notification)) + .await; +} + async fn find_and_remove_turn_summary( conversation_id: ConversationId, turn_summary_store: &TurnSummaryStore, @@ -512,6 +639,110 @@ fn render_review_output_text(output: &ReviewOutputEvent) -> String { } } +fn convert_patch_changes(changes: &HashMap) -> Vec { + let mut converted: Vec = changes + .iter() + .map(|(path, change)| FileUpdateChange { + path: path.to_string_lossy().into_owned(), + kind: map_patch_change_kind(change), + diff: format_file_change_diff(change), + }) + .collect(); + converted.sort_by(|a, b| a.path.cmp(&b.path)); + converted +} + +fn map_patch_change_kind(change: &CoreFileChange) -> V2PatchChangeKind { + match change { + CoreFileChange::Add { .. } => V2PatchChangeKind::Add, + CoreFileChange::Delete { .. } => V2PatchChangeKind::Delete, + CoreFileChange::Update { move_path, .. } => V2PatchChangeKind::Update { + move_path: move_path.clone(), + }, + } +} + +fn format_file_change_diff(change: &CoreFileChange) -> String { + match change { + CoreFileChange::Add { content } => content.clone(), + CoreFileChange::Delete { content } => content.clone(), + CoreFileChange::Update { + unified_diff, + move_path, + } => { + if let Some(path) = move_path { + format!("{unified_diff}\n\nMoved to: {}", path.display()) + } else { + unified_diff.clone() + } + } + } +} + +#[allow(clippy::too_many_arguments)] +async fn on_file_change_request_approval_response( + event_id: String, + conversation_id: ConversationId, + item_id: String, + changes: Vec, + receiver: oneshot::Receiver, + codex: Arc, + outgoing: Arc, + turn_summary_store: TurnSummaryStore, +) { + let response = receiver.await; + let (decision, completion_status) = match response { + Ok(value) => { + let response = serde_json::from_value::(value) + .unwrap_or_else(|err| { + error!("failed to deserialize FileChangeRequestApprovalResponse: {err}"); + FileChangeRequestApprovalResponse { + decision: ApprovalDecision::Decline, + } + }); + + let (decision, completion_status) = match response.decision { + ApprovalDecision::Accept => (ReviewDecision::Approved, None), + ApprovalDecision::Decline => { + (ReviewDecision::Denied, Some(PatchApplyStatus::Declined)) + } + ApprovalDecision::Cancel => { + (ReviewDecision::Abort, Some(PatchApplyStatus::Declined)) + } + }; + // Allow EventMsg::PatchApplyEnd to emit ItemCompleted for accepted patches. + // Only short-circuit on declines/cancels/failures. + (decision, completion_status) + } + Err(err) => { + error!("request failed: {err:?}"); + (ReviewDecision::Denied, Some(PatchApplyStatus::Failed)) + } + }; + + if let Some(status) = completion_status { + complete_file_change_item( + conversation_id, + item_id, + changes, + status, + outgoing.as_ref(), + &turn_summary_store, + ) + .await; + } + + if let Err(err) = codex + .submit(Op::PatchApproval { + id: event_id, + decision, + }) + .await + { + error!("failed to submit PatchApproval: {err}"); + } +} + async fn on_command_execution_request_approval_response( event_id: String, receiver: oneshot::Receiver, diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 8b6539eb61..bf9f0b9403 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -139,6 +139,7 @@ use codex_protocol::protocol::USER_MESSAGE_BEGIN; use codex_protocol::user_input::UserInput as CoreInputItem; use codex_utils_json_to_toml::json_to_toml; use std::collections::HashMap; +use std::collections::HashSet; use std::ffi::OsStr; use std::io::Error as IoError; use std::path::Path; @@ -162,6 +163,7 @@ pub(crate) type PendingInterrupts = Arc, + pub(crate) file_change_started: HashSet, } pub(crate) type TurnSummaryStore = Arc>>; diff --git a/codex-rs/app-server/tests/suite/v2/turn_start.rs b/codex-rs/app-server/tests/suite/v2/turn_start.rs index 8c4cf4e677..b644315d97 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs @@ -1,14 +1,20 @@ use anyhow::Result; use app_test_support::McpProcess; +use app_test_support::create_apply_patch_sse_response; use app_test_support::create_final_assistant_message_sse_response; use app_test_support::create_mock_chat_completions_server; use app_test_support::create_mock_chat_completions_server_unchecked; use app_test_support::create_shell_sse_response; use app_test_support::to_response; +use codex_app_server_protocol::ApprovalDecision; use codex_app_server_protocol::CommandExecutionStatus; +use codex_app_server_protocol::FileChangeRequestApprovalResponse; +use codex_app_server_protocol::ItemCompletedNotification; use codex_app_server_protocol::ItemStartedNotification; use codex_app_server_protocol::JSONRPCNotification; use codex_app_server_protocol::JSONRPCResponse; +use codex_app_server_protocol::PatchApplyStatus; +use codex_app_server_protocol::PatchChangeKind; use codex_app_server_protocol::RequestId; use codex_app_server_protocol::ServerRequest; use codex_app_server_protocol::ThreadItem; @@ -471,6 +477,300 @@ async fn turn_start_updates_sandbox_and_cwd_between_turns_v2() -> Result<()> { Ok(()) } +#[tokio::test] +async fn turn_start_file_change_approval_v2() -> Result<()> { + skip_if_no_network!(Ok(())); + + let tmp = TempDir::new()?; + let codex_home = tmp.path().join("codex_home"); + std::fs::create_dir(&codex_home)?; + let workspace = tmp.path().join("workspace"); + std::fs::create_dir(&workspace)?; + + let patch = r#"*** Begin Patch +*** Add File: README.md ++new line +*** End Patch +"#; + let responses = vec![ + create_apply_patch_sse_response(patch, "patch-call")?, + create_final_assistant_message_sse_response("patch applied")?, + ]; + let server = create_mock_chat_completions_server(responses).await; + create_config_toml(&codex_home, &server.uri(), "untrusted")?; + + let mut mcp = McpProcess::new(&codex_home).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let start_req = mcp + .send_thread_start_request(ThreadStartParams { + model: Some("mock-model".to_string()), + cwd: Some(workspace.to_string_lossy().into_owned()), + ..Default::default() + }) + .await?; + let start_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(start_req)), + ) + .await??; + let ThreadStartResponse { thread, .. } = to_response::(start_resp)?; + + let turn_req = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id.clone(), + input: vec![V2UserInput::Text { + text: "apply patch".into(), + }], + cwd: Some(workspace.clone()), + ..Default::default() + }) + .await?; + let turn_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(turn_req)), + ) + .await??; + let TurnStartResponse { turn } = to_response::(turn_resp)?; + + let started_file_change = timeout(DEFAULT_READ_TIMEOUT, async { + loop { + let started_notif = mcp + .read_stream_until_notification_message("item/started") + .await?; + let started: ItemStartedNotification = + serde_json::from_value(started_notif.params.clone().expect("item/started params"))?; + if let ThreadItem::FileChange { .. } = started.item { + return Ok::(started.item); + } + } + }) + .await??; + let ThreadItem::FileChange { + ref id, + status, + ref changes, + } = started_file_change + else { + unreachable!("loop ensures we break on file change items"); + }; + assert_eq!(id, "patch-call"); + assert_eq!(status, PatchApplyStatus::InProgress); + let started_changes = changes.clone(); + + let server_req = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_request_message(), + ) + .await??; + let ServerRequest::FileChangeRequestApproval { request_id, params } = server_req else { + panic!("expected FileChangeRequestApproval request") + }; + assert_eq!(params.item_id, "patch-call"); + assert_eq!(params.thread_id, thread.id); + assert_eq!(params.turn_id, turn.id); + let expected_readme_path = workspace.join("README.md"); + let expected_readme_path = expected_readme_path.to_string_lossy().into_owned(); + pretty_assertions::assert_eq!( + started_changes, + vec![codex_app_server_protocol::FileUpdateChange { + path: expected_readme_path.clone(), + kind: PatchChangeKind::Add, + diff: "new line\n".to_string(), + }] + ); + + mcp.send_response( + request_id, + serde_json::to_value(FileChangeRequestApprovalResponse { + decision: ApprovalDecision::Accept, + })?, + ) + .await?; + + let completed_file_change = timeout(DEFAULT_READ_TIMEOUT, async { + loop { + let completed_notif = mcp + .read_stream_until_notification_message("item/completed") + .await?; + let completed: ItemCompletedNotification = serde_json::from_value( + completed_notif + .params + .clone() + .expect("item/completed params"), + )?; + if let ThreadItem::FileChange { .. } = completed.item { + return Ok::(completed.item); + } + } + }) + .await??; + let ThreadItem::FileChange { ref id, status, .. } = completed_file_change else { + unreachable!("loop ensures we break on file change items"); + }; + assert_eq!(id, "patch-call"); + assert_eq!(status, PatchApplyStatus::Completed); + + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("codex/event/task_complete"), + ) + .await??; + + let readme_contents = std::fs::read_to_string(expected_readme_path)?; + assert_eq!(readme_contents, "new line\n"); + + Ok(()) +} + +#[tokio::test] +async fn turn_start_file_change_approval_decline_v2() -> Result<()> { + skip_if_no_network!(Ok(())); + + let tmp = TempDir::new()?; + let codex_home = tmp.path().join("codex_home"); + std::fs::create_dir(&codex_home)?; + let workspace = tmp.path().join("workspace"); + std::fs::create_dir(&workspace)?; + + let patch = r#"*** Begin Patch +*** Add File: README.md ++new line +*** End Patch +"#; + let responses = vec![ + create_apply_patch_sse_response(patch, "patch-call")?, + create_final_assistant_message_sse_response("patch declined")?, + ]; + let server = create_mock_chat_completions_server(responses).await; + create_config_toml(&codex_home, &server.uri(), "untrusted")?; + + let mut mcp = McpProcess::new(&codex_home).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let start_req = mcp + .send_thread_start_request(ThreadStartParams { + model: Some("mock-model".to_string()), + cwd: Some(workspace.to_string_lossy().into_owned()), + ..Default::default() + }) + .await?; + let start_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(start_req)), + ) + .await??; + let ThreadStartResponse { thread, .. } = to_response::(start_resp)?; + + let turn_req = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id.clone(), + input: vec![V2UserInput::Text { + text: "apply patch".into(), + }], + cwd: Some(workspace.clone()), + ..Default::default() + }) + .await?; + let turn_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(turn_req)), + ) + .await??; + let TurnStartResponse { turn } = to_response::(turn_resp)?; + + let started_file_change = timeout(DEFAULT_READ_TIMEOUT, async { + loop { + let started_notif = mcp + .read_stream_until_notification_message("item/started") + .await?; + let started: ItemStartedNotification = + serde_json::from_value(started_notif.params.clone().expect("item/started params"))?; + if let ThreadItem::FileChange { .. } = started.item { + return Ok::(started.item); + } + } + }) + .await??; + let ThreadItem::FileChange { + ref id, + status, + ref changes, + } = started_file_change + else { + unreachable!("loop ensures we break on file change items"); + }; + assert_eq!(id, "patch-call"); + assert_eq!(status, PatchApplyStatus::InProgress); + let started_changes = changes.clone(); + + let server_req = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_request_message(), + ) + .await??; + let ServerRequest::FileChangeRequestApproval { request_id, params } = server_req else { + panic!("expected FileChangeRequestApproval request") + }; + assert_eq!(params.item_id, "patch-call"); + assert_eq!(params.thread_id, thread.id); + assert_eq!(params.turn_id, turn.id); + let expected_readme_path = workspace.join("README.md"); + let expected_readme_path_str = expected_readme_path.to_string_lossy().into_owned(); + pretty_assertions::assert_eq!( + started_changes, + vec![codex_app_server_protocol::FileUpdateChange { + path: expected_readme_path_str.clone(), + kind: PatchChangeKind::Add, + diff: "new line\n".to_string(), + }] + ); + + mcp.send_response( + request_id, + serde_json::to_value(FileChangeRequestApprovalResponse { + decision: ApprovalDecision::Decline, + })?, + ) + .await?; + + let completed_file_change = timeout(DEFAULT_READ_TIMEOUT, async { + loop { + let completed_notif = mcp + .read_stream_until_notification_message("item/completed") + .await?; + let completed: ItemCompletedNotification = serde_json::from_value( + completed_notif + .params + .clone() + .expect("item/completed params"), + )?; + if let ThreadItem::FileChange { .. } = completed.item { + return Ok::(completed.item); + } + } + }) + .await??; + let ThreadItem::FileChange { ref id, status, .. } = completed_file_change else { + unreachable!("loop ensures we break on file change items"); + }; + assert_eq!(id, "patch-call"); + assert_eq!(status, PatchApplyStatus::Declined); + + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("codex/event/task_complete"), + ) + .await??; + + assert!( + !expected_readme_path.exists(), + "declined patch should not be applied" + ); + + Ok(()) +} + // Helper to create a config.toml pointing at the mock model server. fn create_config_toml( codex_home: &Path, diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 182ca67f41..4d9285dd24 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -919,6 +919,7 @@ impl Session { let event = EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { call_id, + turn_id: turn_context.sub_id.clone(), changes, reason, grant_root, diff --git a/codex-rs/core/src/tools/events.rs b/codex-rs/core/src/tools/events.rs index 131b8f698d..37df12d4b0 100644 --- a/codex-rs/core/src/tools/events.rs +++ b/codex-rs/core/src/tools/events.rs @@ -179,15 +179,17 @@ impl ToolEmitter { ctx.turn, EventMsg::PatchApplyBegin(PatchApplyBeginEvent { call_id: ctx.call_id.to_string(), + turn_id: ctx.turn.sub_id.clone(), auto_approved: *auto_approved, changes: changes.clone(), }), ) .await; } - (Self::ApplyPatch { .. }, ToolEventStage::Success(output)) => { + (Self::ApplyPatch { changes, .. }, ToolEventStage::Success(output)) => { emit_patch_end( ctx, + changes.clone(), output.stdout.text.clone(), output.stderr.text.clone(), output.exit_code == 0, @@ -195,11 +197,12 @@ impl ToolEmitter { .await; } ( - Self::ApplyPatch { .. }, + Self::ApplyPatch { changes, .. }, ToolEventStage::Failure(ToolEventFailure::Output(output)), ) => { emit_patch_end( ctx, + changes.clone(), output.stdout.text.clone(), output.stderr.text.clone(), output.exit_code == 0, @@ -207,10 +210,17 @@ impl ToolEmitter { .await; } ( - Self::ApplyPatch { .. }, + Self::ApplyPatch { changes, .. }, ToolEventStage::Failure(ToolEventFailure::Message(message)), ) => { - emit_patch_end(ctx, String::new(), (*message).to_string(), false).await; + emit_patch_end( + ctx, + changes.clone(), + String::new(), + (*message).to_string(), + false, + ) + .await; } ( Self::UnifiedExec { @@ -409,15 +419,23 @@ async fn emit_exec_end( .await; } -async fn emit_patch_end(ctx: ToolEventCtx<'_>, stdout: String, stderr: String, success: bool) { +async fn emit_patch_end( + ctx: ToolEventCtx<'_>, + changes: HashMap, + stdout: String, + stderr: String, + success: bool, +) { ctx.session .send_event( ctx.turn, EventMsg::PatchApplyEnd(PatchApplyEndEvent { call_id: ctx.call_id.to_string(), + turn_id: ctx.turn.sub_id.clone(), stdout, stderr, success, + changes, }), ) .await; diff --git a/codex-rs/exec/src/event_processor_with_human_output.rs b/codex-rs/exec/src/event_processor_with_human_output.rs index 2d550fea46..1f007bbe0e 100644 --- a/codex-rs/exec/src/event_processor_with_human_output.rs +++ b/codex-rs/exec/src/event_processor_with_human_output.rs @@ -346,6 +346,7 @@ impl EventProcessor for EventProcessorWithHumanOutput { call_id, auto_approved, changes, + .. }) => { // Store metadata so we can calculate duration later when we // receive the corresponding PatchApplyEnd event. diff --git a/codex-rs/exec/src/exec_events.rs b/codex-rs/exec/src/exec_events.rs index 64288e1388..23f471e224 100644 --- a/codex-rs/exec/src/exec_events.rs +++ b/codex-rs/exec/src/exec_events.rs @@ -166,6 +166,7 @@ pub struct FileUpdateChange { #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, TS)] #[serde(rename_all = "snake_case")] pub enum PatchApplyStatus { + InProgress, Completed, Failed, } diff --git a/codex-rs/exec/tests/event_processor_with_json_output.rs b/codex-rs/exec/tests/event_processor_with_json_output.rs index 5053f61921..cbcd5458bc 100644 --- a/codex-rs/exec/tests/event_processor_with_json_output.rs +++ b/codex-rs/exec/tests/event_processor_with_json_output.rs @@ -822,6 +822,7 @@ fn patch_apply_success_produces_item_completed_patchapply() { "p1", EventMsg::PatchApplyBegin(PatchApplyBeginEvent { call_id: "call-1".to_string(), + turn_id: "turn-1".to_string(), auto_approved: true, changes: changes.clone(), }), @@ -834,9 +835,11 @@ fn patch_apply_success_produces_item_completed_patchapply() { "p2", EventMsg::PatchApplyEnd(PatchApplyEndEvent { call_id: "call-1".to_string(), + turn_id: "turn-1".to_string(), stdout: "applied 3 changes".to_string(), stderr: String::new(), success: true, + changes: changes.clone(), }), ); let out_end = ep.collect_thread_events(&end); @@ -891,6 +894,7 @@ fn patch_apply_failure_produces_item_completed_patchapply_failed() { "p1", EventMsg::PatchApplyBegin(PatchApplyBeginEvent { call_id: "call-2".to_string(), + turn_id: "turn-2".to_string(), auto_approved: false, changes: changes.clone(), }), @@ -902,9 +906,11 @@ fn patch_apply_failure_produces_item_completed_patchapply_failed() { "p2", EventMsg::PatchApplyEnd(PatchApplyEndEvent { call_id: "call-2".to_string(), + turn_id: "turn-2".to_string(), stdout: String::new(), stderr: "failed to apply".to_string(), success: false, + changes: changes.clone(), }), ); let out_end = ep.collect_thread_events(&end); diff --git a/codex-rs/mcp-server/src/codex_tool_runner.rs b/codex-rs/mcp-server/src/codex_tool_runner.rs index 93dc7764dc..8dccb51250 100644 --- a/codex-rs/mcp-server/src/codex_tool_runner.rs +++ b/codex-rs/mcp-server/src/codex_tool_runner.rs @@ -210,6 +210,7 @@ async fn run_codex_tool_session_inner( } EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { call_id, + turn_id: _, reason, grant_root, changes, diff --git a/codex-rs/protocol/src/approvals.rs b/codex-rs/protocol/src/approvals.rs index f7c5fc6049..25f5e90e9e 100644 --- a/codex-rs/protocol/src/approvals.rs +++ b/codex-rs/protocol/src/approvals.rs @@ -57,6 +57,10 @@ pub struct ExecApprovalRequestEvent { pub struct ApplyPatchApprovalRequestEvent { /// Responses API call id for the associated patch apply call, if available. pub call_id: String, + /// Turn ID that this patch belongs to. + /// Uses `#[serde(default)]` for backwards compatibility with older senders. + #[serde(default)] + pub turn_id: String, pub changes: HashMap, /// Optional explanatory reason (e.g. request for extra write access). #[serde(skip_serializing_if = "Option::is_none")] diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 1825d7636b..f20e412831 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -1374,6 +1374,10 @@ pub struct StreamInfoEvent { pub struct PatchApplyBeginEvent { /// Identifier so this can be paired with the PatchApplyEnd event. pub call_id: String, + /// Turn ID that this patch belongs to. + /// Uses `#[serde(default)]` for backwards compatibility. + #[serde(default)] + pub turn_id: String, /// If true, there was no ApplyPatchApprovalRequest for this patch. pub auto_approved: bool, /// The changes to be applied. @@ -1384,12 +1388,19 @@ pub struct PatchApplyBeginEvent { pub struct PatchApplyEndEvent { /// Identifier for the PatchApplyBegin that finished. pub call_id: String, + /// Turn ID that this patch belongs to. + /// Uses `#[serde(default)]` for backwards compatibility. + #[serde(default)] + pub turn_id: String, /// Captured stdout (summary printed by apply_patch). pub stdout: String, /// Captured stderr (parser errors, IO failures, etc.). pub stderr: String, /// Whether the patch was applied successfully. pub success: bool, + /// The changes that were applied (mirrors PatchApplyBeginEvent::changes). + #[serde(default)] + pub changes: HashMap, } #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 987a956fe6..a5728ab14f 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1464,6 +1464,7 @@ impl ChatWidget { // }), msg: EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { call_id: "1".to_string(), + turn_id: "turn-1".to_string(), changes: HashMap::from([ ( PathBuf::from("/tmp/test.txt"), diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index d60a20b8f1..1393e52886 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -1912,6 +1912,7 @@ fn approval_modal_patch_snapshot() { ); let ev = ApplyPatchApprovalRequestEvent { call_id: "call-approve-patch".into(), + turn_id: "turn-approve-patch".into(), changes, reason: Some("The model wants to apply changes".into()), grant_root: Some(PathBuf::from("/tmp")), @@ -2164,6 +2165,7 @@ fn apply_patch_events_emit_history_cells() { ); let ev = ApplyPatchApprovalRequestEvent { call_id: "c1".into(), + turn_id: "turn-c1".into(), changes, reason: None, grant_root: None, @@ -2204,6 +2206,7 @@ fn apply_patch_events_emit_history_cells() { ); let begin = PatchApplyBeginEvent { call_id: "c1".into(), + turn_id: "turn-c1".into(), auto_approved: true, changes: changes2, }; @@ -2220,11 +2223,20 @@ fn apply_patch_events_emit_history_cells() { ); // 3) End apply success -> success cell + let mut end_changes = HashMap::new(); + end_changes.insert( + PathBuf::from("foo.txt"), + FileChange::Add { + content: "hello\n".to_string(), + }, + ); let end = PatchApplyEndEvent { call_id: "c1".into(), + turn_id: "turn-c1".into(), stdout: "ok\n".into(), stderr: String::new(), success: true, + changes: end_changes, }; chat.handle_codex_event(Event { id: "s1".into(), @@ -2252,6 +2264,7 @@ fn apply_patch_manual_approval_adjusts_header() { id: "s1".into(), msg: EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { call_id: "c1".into(), + turn_id: "turn-c1".into(), changes: proposed_changes, reason: None, grant_root: None, @@ -2270,6 +2283,7 @@ fn apply_patch_manual_approval_adjusts_header() { id: "s1".into(), msg: EventMsg::PatchApplyBegin(PatchApplyBeginEvent { call_id: "c1".into(), + turn_id: "turn-c1".into(), auto_approved: false, changes: apply_changes, }), @@ -2299,6 +2313,7 @@ fn apply_patch_manual_flow_snapshot() { id: "s1".into(), msg: EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { call_id: "c1".into(), + turn_id: "turn-c1".into(), changes: proposed_changes, reason: Some("Manual review required".into()), grant_root: None, @@ -2321,6 +2336,7 @@ fn apply_patch_manual_flow_snapshot() { id: "s1".into(), msg: EventMsg::PatchApplyBegin(PatchApplyBeginEvent { call_id: "c1".into(), + turn_id: "turn-c1".into(), auto_approved: false, changes: apply_changes, }), @@ -2348,6 +2364,7 @@ fn apply_patch_approval_sends_op_with_submission_id() { ); let ev = ApplyPatchApprovalRequestEvent { call_id: "call-999".into(), + turn_id: "turn-999".into(), changes, reason: None, grant_root: None, @@ -2387,6 +2404,7 @@ fn apply_patch_full_flow_integration_like() { id: "sub-xyz".into(), msg: EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { call_id: "call-1".into(), + turn_id: "turn-call-1".into(), changes, reason: None, grant_root: None, @@ -2427,17 +2445,25 @@ fn apply_patch_full_flow_integration_like() { id: "sub-xyz".into(), msg: EventMsg::PatchApplyBegin(PatchApplyBeginEvent { call_id: "call-1".into(), + turn_id: "turn-call-1".into(), auto_approved: false, changes: changes2, }), }); + let mut end_changes = HashMap::new(); + end_changes.insert( + PathBuf::from("pkg.rs"), + FileChange::Add { content: "".into() }, + ); chat.handle_codex_event(Event { id: "sub-xyz".into(), msg: EventMsg::PatchApplyEnd(PatchApplyEndEvent { call_id: "call-1".into(), + turn_id: "turn-call-1".into(), stdout: String::from("ok"), stderr: String::new(), success: true, + changes: end_changes, }), }); } @@ -2458,6 +2484,7 @@ fn apply_patch_untrusted_shows_approval_modal() { id: "sub-1".into(), msg: EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { call_id: "call-1".into(), + turn_id: "turn-call-1".into(), changes, reason: None, grant_root: None, @@ -2506,6 +2533,7 @@ fn apply_patch_request_shows_diff_summary() { id: "sub-apply".into(), msg: EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { call_id: "call-apply".into(), + turn_id: "turn-apply".into(), changes, reason: None, grant_root: None, From e8af41de8a71d8dd653adb0ac29e1c12013615d3 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 19 Nov 2025 20:59:17 -0800 Subject: [PATCH 8/9] fix: clean up elicitation used by exec-server (#6958) Using appropriate message/title fields, I think this looks better now: image Though note that in the current version of the Inspector (`0.17.2`), you cannot hit **Submit** until you fill out the field. I believe this is a bug in the Inspector, as it does not properly handle the case when all fields are optional. I put up a fix: https://github.com/modelcontextprotocol/inspector/pull/926 --- .../src/posix/mcp_escalation_policy.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs b/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs index 24f42fe91b..39958bb4f9 100644 --- a/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs +++ b/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs @@ -6,8 +6,6 @@ use rmcp::model::CreateElicitationRequestParam; use rmcp::model::CreateElicitationResult; use rmcp::model::ElicitationAction; use rmcp::model::ElicitationSchema; -use rmcp::model::PrimitiveSchema; -use rmcp::model::StringSchema; use rmcp::service::RequestContext; use crate::posix::escalate_protocol::EscalateAction; @@ -54,12 +52,19 @@ impl McpEscalationPolicy { context .peer .create_elicitation(CreateElicitationRequestParam { - message: format!("Allow Codex to run `{command:?}` in `{workdir:?}`?"), - #[allow(clippy::expect_used)] + message: format!("Allow agent to run `{command}` in `{}`?", workdir.display()), requested_schema: ElicitationSchema::builder() - .property("dummy", PrimitiveSchema::String(StringSchema::new())) + .title("Execution Permission Request") + .optional_string_with("reason", |schema| { + schema.description("Optional reason for allowing or denying execution") + }) .build() - .expect("failed to build elicitation schema"), + .map_err(|e| { + McpError::internal_error( + format!("failed to build elicitation schema: {e}"), + None, + ) + })?, }) .await .map_err(|e| McpError::internal_error(e.to_string(), None)) From e7ed3c2884e7fb3f54bae44441413d2bd8c3fc41 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 19 Nov 2025 21:00:07 -0800 Subject: [PATCH 9/9] fix: when displaying execv, show file instead of arg0 --- codex-rs/exec-server/src/posix/mcp_escalation_policy.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs b/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs index 39958bb4f9..069948ea06 100644 --- a/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs +++ b/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs @@ -43,12 +43,17 @@ impl McpEscalationPolicy { async fn prompt( &self, - _file: &Path, + file: &Path, argv: &[String], workdir: &Path, context: RequestContext, ) -> Result { - let command = shlex::try_join(argv.iter().map(String::as_str)).unwrap_or_default(); + let args = shlex::try_join(argv.iter().skip(1).map(String::as_str)).unwrap_or_default(); + let command = if args.is_empty() { + file.display().to_string() + } else { + format!("{} {}", file.display(), args) + }; context .peer .create_elicitation(CreateElicitationRequestParam {