diff --git a/README.md b/README.md index 8141610035..4fed276405 100644 --- a/README.md +++ b/README.md @@ -69,6 +69,37 @@ 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 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 +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 (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. + +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 +``` + +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 6ace22c766..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", @@ -1188,6 +1189,7 @@ name = "codex-exec-server" version = "0.0.0" dependencies = [ "anyhow", + "async-trait", "clap", "codex-core", "libc", @@ -1196,6 +1198,7 @@ dependencies = [ "rmcp", "serde", "serde_json", + "shlex", "socket2 0.6.0", "tempfile", "tokio", 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/common/src/config_summary.rs b/codex-rs/common/src/config_summary.rs index dabc606ce1..8fc1bb26f3 100644 --- a/codex-rs/common/src/config_summary.rs +++ b/codex-rs/common/src/config_summary.rs @@ -15,13 +15,12 @@ pub fn create_config_summary_entries(config: &Config) -> Vec<(&'static str, Stri if config.model_provider.wire_api == WireApi::Responses && config.model_family.supports_reasoning_summaries { - entries.push(( - "reasoning effort", - config - .model_reasoning_effort - .map(|effort| effort.to_string()) - .unwrap_or_else(|| "none".to_string()), - )); + let reasoning_effort = config + .model_reasoning_effort + .or(config.model_family.default_reasoning_effort) + .map(|effort| effort.to_string()) + .unwrap_or_else(|| "none".to_string()); + entries.push(("reasoning effort", reasoning_effort)); entries.push(( "reasoning summaries", config.model_reasoning_summary.to_string(), 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 e5b4e4c316..c793eff352 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; @@ -166,6 +167,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 { @@ -182,6 +187,7 @@ impl Codex { cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), features: config.features.clone(), + exec_policy, session_source, }; @@ -279,6 +285,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, } @@ -335,6 +342,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, @@ -435,6 +444,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), } } @@ -1785,6 +1795,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), }; @@ -2605,6 +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()), session_source: SessionSource::Exec, }; @@ -2682,6 +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()), 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/shell.rs b/codex-rs/core/src/shell.rs index 7bfec089c5..1fb2b97dd7 100644 --- a/codex-rs/core/src/shell.rs +++ b/codex-rs/core/src/shell.rs @@ -204,10 +204,21 @@ pub async fn default_user_shell() -> Shell { if cfg!(windows) { get_shell(ShellType::PowerShell, None).unwrap_or(Shell::Unknown) } else { - get_user_shell_path() + let user_default_shell = get_user_shell_path() .and_then(|shell| detect_shell_type(&shell)) - .and_then(|shell_type| get_shell(shell_type, None)) - .unwrap_or(Shell::Unknown) + .and_then(|shell_type| get_shell(shell_type, None)); + + let shell_with_fallback = if cfg!(target_os = "macos") { + user_default_shell + .or_else(|| get_shell(ShellType::Zsh, None)) + .or_else(|| get_shell(ShellType::Bash, None)) + } else { + user_default_shell + .or_else(|| get_shell(ShellType::Bash, None)) + .or_else(|| get_shell(ShellType::Zsh, None)) + }; + + shell_with_fallback.unwrap_or(Shell::Unknown) } } 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/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"; 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/exec-server/Cargo.toml b/codex-rs/exec-server/Cargo.toml index 24ee460f50..54cead4118 100644 --- a/codex-rs/exec-server/Cargo.toml +++ b/codex-rs/exec-server/Cargo.toml @@ -4,14 +4,23 @@ name = "codex-exec-server" version = { workspace = true } [[bin]] -name = "codex-exec-server" -path = "src/main.rs" +name = "codex-execve-wrapper" +path = "src/bin/main_execve_wrapper.rs" + +[[bin]] +name = "codex-exec-mcp-server" +path = "src/bin/main_mcp_server.rs" + +[lib] +name = "codex_exec_server" +path = "src/lib.rs" [lints] workspace = true [dependencies] anyhow = { workspace = true } +async-trait = { workspace = true } clap = { workspace = true, features = ["derive"] } codex-core = { workspace = true } libc = { workspace = true } @@ -31,6 +40,7 @@ rmcp = { workspace = true, default-features = false, features = [ ] } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } +shlex = { workspace = true } socket2 = { workspace = true } tokio = { workspace = true, features = [ "io-std", diff --git a/codex-rs/exec-server/src/bin/main_execve_wrapper.rs b/codex-rs/exec-server/src/bin/main_execve_wrapper.rs new file mode 100644 index 0000000000..3ab346e8ef --- /dev/null +++ b/codex-rs/exec-server/src/bin/main_execve_wrapper.rs @@ -0,0 +1,8 @@ +#[cfg(not(unix))] +fn main() { + eprintln!("codex-execve-wrapper is only implemented for UNIX"); + std::process::exit(1); +} + +#[cfg(unix)] +pub use codex_exec_server::main_execve_wrapper as main; diff --git a/codex-rs/exec-server/src/bin/main_mcp_server.rs b/codex-rs/exec-server/src/bin/main_mcp_server.rs new file mode 100644 index 0000000000..6c75ae4237 --- /dev/null +++ b/codex-rs/exec-server/src/bin/main_mcp_server.rs @@ -0,0 +1,8 @@ +#[cfg(not(unix))] +fn main() { + eprintln!("codex-exec-mcp-server is only implemented for UNIX"); + std::process::exit(1); +} + +#[cfg(unix)] +pub use codex_exec_server::main_mcp_server as main; diff --git a/codex-rs/exec-server/src/lib.rs b/codex-rs/exec-server/src/lib.rs new file mode 100644 index 0000000000..adec09d4de --- /dev/null +++ b/codex-rs/exec-server/src/lib.rs @@ -0,0 +1,8 @@ +#[cfg(unix)] +mod posix; + +#[cfg(unix)] +pub use posix::main_execve_wrapper; + +#[cfg(unix)] +pub use posix::main_mcp_server; diff --git a/codex-rs/exec-server/src/main.rs b/codex-rs/exec-server/src/main.rs deleted file mode 100644 index 23a18b2525..0000000000 --- a/codex-rs/exec-server/src/main.rs +++ /dev/null @@ -1,11 +0,0 @@ -#[cfg(target_os = "windows")] -fn main() { - eprintln!("codex-exec-server is not implemented on Windows targets"); - std::process::exit(1); -} - -#[cfg(not(target_os = "windows"))] -mod posix; - -#[cfg(not(target_os = "windows"))] -pub use posix::main; diff --git a/codex-rs/exec-server/src/posix.rs b/codex-rs/exec-server/src/posix.rs index 179fc7a6fc..239eaf61f2 100644 --- a/codex-rs/exec-server/src/posix.rs +++ b/codex-rs/exec-server/src/posix.rs @@ -56,109 +56,114 @@ //! o<-----x //! use std::path::Path; +use std::path::PathBuf; use clap::Parser; -use clap::Subcommand; use tracing_subscriber::EnvFilter; use tracing_subscriber::{self}; -use crate::posix::escalate_protocol::EscalateAction; -use crate::posix::escalate_server::EscalateServer; +use crate::posix::mcp_escalation_policy::ExecPolicyOutcome; mod escalate_client; mod escalate_protocol; mod escalate_server; +mod escalation_policy; mod mcp; +mod mcp_escalation_policy; mod socket; -fn dummy_exec_policy(file: &Path, argv: &[String], _workdir: &Path) -> EscalateAction { - // TODO: execpolicy - if file == Path::new("/opt/homebrew/bin/gh") - && let [_, arg1, arg2, ..] = argv - && arg1 == "issue" - && arg2 == "list" - { - return EscalateAction::Escalate; - } - EscalateAction::Run -} +/// Default value of --execve option relative to the current executable. +/// Note this must match the name of the binary as specified in Cargo.toml. +const CODEX_EXECVE_WRAPPER_EXE_NAME: &str = "codex-execve-wrapper"; #[derive(Parser)] -#[command(version)] -pub struct Cli { - #[command(subcommand)] - subcommand: Option, -} +struct McpServerCli { + /// Executable to delegate execve(2) calls to in Bash. + #[arg(long = "execve")] + execve_wrapper: Option, -#[derive(Subcommand)] -enum Commands { - Escalate(EscalateArgs), - ShellExec(ShellExecArgs), -} - -/// Invoked from within the sandbox to (potentially) escalate permissions. -#[derive(Parser, Debug)] -struct EscalateArgs { - file: String, - - #[arg(trailing_var_arg = true)] - argv: Vec, -} - -impl EscalateArgs { - /// This is the escalate client. It talks to the escalate server to determine whether to exec() - /// the command directly or to proxy to the escalate server. - async fn run(self) -> anyhow::Result { - let EscalateArgs { file, argv } = self; - escalate_client::run(file, argv).await - } -} - -/// Debugging command to emulate an MCP "shell" tool call. -#[derive(Parser, Debug)] -struct ShellExecArgs { - command: String, + /// Path to Bash that has been patched to support execve() wrapping. + #[arg(long = "bash")] + bash_path: Option, } #[tokio::main] -pub async fn main() -> anyhow::Result<()> { - let cli = Cli::parse(); +pub async fn main_mcp_server() -> anyhow::Result<()> { tracing_subscriber::fmt() .with_env_filter(EnvFilter::from_default_env()) .with_writer(std::io::stderr) .with_ansi(false) .init(); - match cli.subcommand { - Some(Commands::Escalate(args)) => { - std::process::exit(args.run().await?); - } - Some(Commands::ShellExec(args)) => { - let bash_path = mcp::get_bash_path()?; - let escalate_server = EscalateServer::new(bash_path, dummy_exec_policy); - let result = escalate_server - .exec( - args.command.clone(), - std::env::vars().collect(), - std::env::current_dir()?, - None, - ) - .await?; - println!("{result:?}"); - std::process::exit(result.exit_code); - } + let cli = McpServerCli::parse(); + let execve_wrapper = match cli.execve_wrapper { + Some(path) => path, None => { - let bash_path = mcp::get_bash_path()?; + let cwd = std::env::current_exe()?; + cwd.parent() + .map(|p| p.join(CODEX_EXECVE_WRAPPER_EXE_NAME)) + .ok_or_else(|| { + anyhow::anyhow!("failed to determine execve wrapper path from current exe") + })? + } + }; + let bash_path = match cli.bash_path { + Some(path) => path, + None => mcp::get_bash_path()?, + }; - tracing::info!("Starting MCP server"); - let service = mcp::serve(bash_path, dummy_exec_policy) - .await - .inspect_err(|e| { - tracing::error!("serving error: {:?}", e); - })?; + tracing::info!("Starting MCP server"); + let service = mcp::serve(bash_path, execve_wrapper, dummy_exec_policy) + .await + .inspect_err(|e| { + tracing::error!("serving error: {:?}", e); + })?; - service.waiting().await?; - Ok(()) + service.waiting().await?; + Ok(()) +} + +#[derive(Parser)] +pub struct ExecveWrapperCli { + file: String, + + #[arg(trailing_var_arg = true)] + argv: Vec, +} + +#[tokio::main] +pub async fn main_execve_wrapper() -> anyhow::Result<()> { + tracing_subscriber::fmt() + .with_env_filter(EnvFilter::from_default_env()) + .with_writer(std::io::stderr) + .with_ansi(false) + .init(); + + let ExecveWrapperCli { file, argv } = ExecveWrapperCli::parse(); + let exit_code = escalate_client::run(file, argv).await?; + std::process::exit(exit_code); +} + +// TODO: replace with execpolicy2 + +fn dummy_exec_policy(file: &Path, argv: &[String], _workdir: &Path) -> ExecPolicyOutcome { + if file.ends_with("rm") { + ExecPolicyOutcome::Forbidden + } else if file.ends_with("git") { + ExecPolicyOutcome::Prompt { + run_with_escalated_permissions: false, + } + } else if file == Path::new("/opt/homebrew/bin/gh") + && let [_, arg1, arg2, ..] = argv + && arg1 == "issue" + && arg2 == "list" + { + ExecPolicyOutcome::Allow { + run_with_escalated_permissions: true, + } + } else { + ExecPolicyOutcome::Allow { + run_with_escalated_permissions: false, } } } diff --git a/codex-rs/exec-server/src/posix/escalate_client.rs b/codex-rs/exec-server/src/posix/escalate_client.rs index 2add1dade2..bea4b6fa5f 100644 --- a/codex-rs/exec-server/src/posix/escalate_client.rs +++ b/codex-rs/exec-server/src/posix/escalate_client.rs @@ -98,5 +98,12 @@ pub(crate) async fn run(file: String, argv: Vec) -> anyhow::Result Err(err.into()) } + EscalateAction::Deny { reason } => { + match reason { + Some(reason) => eprintln!("Execution denied: {reason}"), + None => eprintln!("Execution denied"), + } + Ok(1) + } } } diff --git a/codex-rs/exec-server/src/posix/escalate_protocol.rs b/codex-rs/exec-server/src/posix/escalate_protocol.rs index 09d33fc982..e3fc27d071 100644 --- a/codex-rs/exec-server/src/posix/escalate_protocol.rs +++ b/codex-rs/exec-server/src/posix/escalate_protocol.rs @@ -34,6 +34,8 @@ pub(super) enum EscalateAction { Run, /// The command should be escalated to the server for execution. Escalate, + /// The command should not be executed. + Deny { reason: Option }, } /// The client sends this to the server to forward its open FDs. diff --git a/codex-rs/exec-server/src/posix/escalate_server.rs b/codex-rs/exec-server/src/posix/escalate_server.rs index 6d058ff0f0..1f76687ce0 100644 --- a/codex-rs/exec-server/src/posix/escalate_server.rs +++ b/codex-rs/exec-server/src/posix/escalate_server.rs @@ -1,8 +1,8 @@ use std::collections::HashMap; use std::os::fd::AsRawFd; -use std::path::Path; use std::path::PathBuf; use std::process::Stdio; +use std::sync::Arc; use std::time::Duration; use anyhow::Context as _; @@ -21,25 +21,26 @@ use crate::posix::escalate_protocol::EscalateRequest; use crate::posix::escalate_protocol::EscalateResponse; use crate::posix::escalate_protocol::SuperExecMessage; use crate::posix::escalate_protocol::SuperExecResult; +use crate::posix::escalation_policy::EscalationPolicy; use crate::posix::socket::AsyncDatagramSocket; use crate::posix::socket::AsyncSocket; -/// This is the policy which decides how to handle an exec() call. -/// -/// `file` is the absolute, canonical path to the executable to run, i.e. the first arg to exec. -/// `argv` is the argv, including the program name (`argv[0]`). -/// `workdir` is the absolute, canonical path to the working directory in which to execute the -/// command. -pub(crate) type ExecPolicy = fn(file: &Path, argv: &[String], workdir: &Path) -> EscalateAction; - pub(crate) struct EscalateServer { bash_path: PathBuf, - policy: ExecPolicy, + execve_wrapper: PathBuf, + policy: Arc, } impl EscalateServer { - pub fn new(bash_path: PathBuf, policy: ExecPolicy) -> Self { - Self { bash_path, policy } + pub fn new

(bash_path: PathBuf, execve_wrapper: PathBuf, policy: P) -> Self + where + P: EscalationPolicy + Send + Sync + 'static, + { + Self { + bash_path, + execve_wrapper, + policy: Arc::new(policy), + } } pub async fn exec( @@ -53,7 +54,7 @@ impl EscalateServer { let client_socket = escalate_client.into_inner(); client_socket.set_cloexec(false)?; - let escalate_task = tokio::spawn(escalate_task(escalate_server, self.policy)); + let escalate_task = tokio::spawn(escalate_task(escalate_server, self.policy.clone())); let mut env = env.clone(); env.insert( ESCALATE_SOCKET_ENV_VAR.to_string(), @@ -61,8 +62,15 @@ impl EscalateServer { ); env.insert( BASH_EXEC_WRAPPER_ENV_VAR.to_string(), - format!("{} escalate", std::env::current_exe()?.to_string_lossy()), + self.execve_wrapper.to_string_lossy().to_string(), ); + + // TODO: use the sandbox policy and cwd from the calling client. + // Note that sandbox_cwd is ignored for ReadOnly, but needs to be legit + // for `SandboxPolicy::WorkspaceWrite`. + let sandbox_policy = SandboxPolicy::ReadOnly; + let sandbox_cwd = PathBuf::from("/__NONEXISTENT__"); + let result = process_exec_tool_call( codex_core::exec::ExecParams { command: vec![ @@ -78,9 +86,8 @@ impl EscalateServer { arg0: None, }, get_platform_sandbox().unwrap_or(SandboxType::None), - // TODO: use the sandbox policy and cwd from the calling client - &SandboxPolicy::ReadOnly, - &PathBuf::from("/__NONEXISTENT__"), // This is ignored for ReadOnly + &sandbox_policy, + &sandbox_cwd, &None, None, ) @@ -96,7 +103,10 @@ impl EscalateServer { } } -async fn escalate_task(socket: AsyncDatagramSocket, policy: ExecPolicy) -> anyhow::Result<()> { +async fn escalate_task( + socket: AsyncDatagramSocket, + policy: Arc, +) -> anyhow::Result<()> { loop { let (_, mut fds) = socket.receive_with_fds().await?; if fds.len() != 1 { @@ -104,6 +114,7 @@ async fn escalate_task(socket: AsyncDatagramSocket, policy: ExecPolicy) -> anyho continue; } let stream_socket = AsyncSocket::from_fd(fds.remove(0))?; + let policy = policy.clone(); tokio::spawn(async move { if let Err(err) = handle_escalate_session_with_policy(stream_socket, policy).await { tracing::error!("escalate session failed: {err:?}"); @@ -122,7 +133,7 @@ pub(crate) struct ExecResult { async fn handle_escalate_session_with_policy( socket: AsyncSocket, - policy: ExecPolicy, + policy: Arc, ) -> anyhow::Result<()> { let EscalateRequest { file, @@ -132,8 +143,12 @@ async fn handle_escalate_session_with_policy( } = socket.receive::().await?; let file = PathBuf::from(&file).absolutize()?.into_owned(); let workdir = PathBuf::from(&workdir).absolutize()?.into_owned(); - let action = policy(file.as_path(), &argv, &workdir); + let action = policy + .determine_action(file.as_path(), &argv, &workdir) + .await?; + tracing::debug!("decided {action:?} for {file:?} {argv:?} {workdir:?}"); + match action { EscalateAction::Run => { socket @@ -195,6 +210,13 @@ async fn handle_escalate_session_with_policy( }) .await?; } + EscalateAction::Deny { reason } => { + socket + .send(EscalateResponse { + action: EscalateAction::Deny { reason }, + }) + .await?; + } } Ok(()) } @@ -204,14 +226,33 @@ mod tests { use super::*; use pretty_assertions::assert_eq; use std::collections::HashMap; + use std::path::Path; use std::path::PathBuf; + struct DeterministicEscalationPolicy { + action: EscalateAction, + } + + #[async_trait::async_trait] + impl EscalationPolicy for DeterministicEscalationPolicy { + async fn determine_action( + &self, + _file: &Path, + _argv: &[String], + _workdir: &Path, + ) -> Result { + Ok(self.action.clone()) + } + } + #[tokio::test] async fn handle_escalate_session_respects_run_in_sandbox_decision() -> anyhow::Result<()> { let (server, client) = AsyncSocket::pair()?; let server_task = tokio::spawn(handle_escalate_session_with_policy( server, - |_file, _argv, _workdir| EscalateAction::Run, + Arc::new(DeterministicEscalationPolicy { + action: EscalateAction::Run, + }), )); client @@ -238,7 +279,9 @@ mod tests { let (server, client) = AsyncSocket::pair()?; let server_task = tokio::spawn(handle_escalate_session_with_policy( server, - |_file, _argv, _workdir| EscalateAction::Escalate, + Arc::new(DeterministicEscalationPolicy { + action: EscalateAction::Escalate, + }), )); client diff --git a/codex-rs/exec-server/src/posix/escalation_policy.rs b/codex-rs/exec-server/src/posix/escalation_policy.rs new file mode 100644 index 0000000000..a7fcc4f627 --- /dev/null +++ b/codex-rs/exec-server/src/posix/escalation_policy.rs @@ -0,0 +1,14 @@ +use std::path::Path; + +use crate::posix::escalate_protocol::EscalateAction; + +/// Decides what action to take in response to an execve request from a client. +#[async_trait::async_trait] +pub(crate) trait EscalationPolicy: Send + Sync { + async fn determine_action( + &self, + file: &Path, + argv: &[String], + workdir: &Path, + ) -> Result; +} diff --git a/codex-rs/exec-server/src/posix/mcp.rs b/codex-rs/exec-server/src/posix/mcp.rs index f4e2b19d4f..f5785dc5d0 100644 --- a/codex-rs/exec-server/src/posix/mcp.rs +++ b/codex-rs/exec-server/src/posix/mcp.rs @@ -18,9 +18,10 @@ use rmcp::tool_handler; use rmcp::tool_router; use rmcp::transport::stdio; -use crate::posix::escalate_server; use crate::posix::escalate_server::EscalateServer; -use crate::posix::escalate_server::ExecPolicy; +use crate::posix::escalate_server::{self}; +use crate::posix::mcp_escalation_policy::ExecPolicy; +use crate::posix::mcp_escalation_policy::McpEscalationPolicy; /// Path to our patched bash. const CODEX_BASH_PATH_ENV_VAR: &str = "CODEX_BASH_PATH"; @@ -64,15 +65,17 @@ impl From for ExecResult { pub struct ExecTool { tool_router: ToolRouter, bash_path: PathBuf, + execve_wrapper: PathBuf, policy: ExecPolicy, } #[tool_router] impl ExecTool { - pub fn new(bash_path: PathBuf, policy: ExecPolicy) -> Self { + pub fn new(bash_path: PathBuf, execve_wrapper: PathBuf, policy: ExecPolicy) -> Self { Self { tool_router: Self::tool_router(), bash_path, + execve_wrapper, policy, } } @@ -81,10 +84,14 @@ impl ExecTool { #[tool] async fn shell( &self, - _context: RequestContext, + context: RequestContext, Parameters(params): Parameters, ) -> Result { - let escalate_server = EscalateServer::new(self.bash_path.clone(), self.policy); + let escalate_server = EscalateServer::new( + self.bash_path.clone(), + self.execve_wrapper.clone(), + McpEscalationPolicy::new(self.policy, context), + ); let result = escalate_server .exec( params.command, @@ -99,27 +106,6 @@ impl ExecTool { ExecResult::from(result), )?])) } - - #[allow(dead_code)] - async fn prompt( - &self, - command: String, - workdir: String, - context: RequestContext, - ) -> Result { - context - .peer - .create_elicitation(CreateElicitationRequestParam { - message: format!("Allow Codex to run `{command:?}` in `{workdir:?}`?"), - #[allow(clippy::expect_used)] - requested_schema: ElicitationSchema::builder() - .property("dummy", PrimitiveSchema::String(StringSchema::new())) - .build() - .expect("failed to build elicitation schema"), - }) - .await - .map_err(|e| McpError::internal_error(e.to_string(), None)) - } } #[tool_handler] @@ -147,8 +133,9 @@ impl ServerHandler for ExecTool { pub(crate) async fn serve( bash_path: PathBuf, + execve_wrapper: PathBuf, policy: ExecPolicy, ) -> Result, rmcp::service::ServerInitializeError> { - let tool = ExecTool::new(bash_path, policy); + let tool = ExecTool::new(bash_path, execve_wrapper, policy); tool.serve(stdio()).await } diff --git a/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs b/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs new file mode 100644 index 0000000000..24f42fe91b --- /dev/null +++ b/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs @@ -0,0 +1,117 @@ +use std::path::Path; + +use rmcp::ErrorData as McpError; +use rmcp::RoleServer; +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; +use crate::posix::escalation_policy::EscalationPolicy; + +/// This is the policy which decides how to handle an exec() call. +/// +/// `file` is the absolute, canonical path to the executable to run, i.e. the first arg to exec. +/// `argv` is the argv, including the program name (`argv[0]`). +/// `workdir` is the absolute, canonical path to the working directory in which to execute the +/// command. +pub(crate) type ExecPolicy = fn(file: &Path, argv: &[String], workdir: &Path) -> ExecPolicyOutcome; + +pub(crate) enum ExecPolicyOutcome { + Allow { + run_with_escalated_permissions: bool, + }, + Prompt { + run_with_escalated_permissions: bool, + }, + Forbidden, +} + +/// ExecPolicy with access to the MCP RequestContext so that it can leverage +/// elicitations. +pub(crate) struct McpEscalationPolicy { + policy: ExecPolicy, + context: RequestContext, +} + +impl McpEscalationPolicy { + pub(crate) fn new(policy: ExecPolicy, context: RequestContext) -> Self { + Self { policy, context } + } + + async fn prompt( + &self, + _file: &Path, + argv: &[String], + workdir: &Path, + context: RequestContext, + ) -> Result { + let command = shlex::try_join(argv.iter().map(String::as_str)).unwrap_or_default(); + context + .peer + .create_elicitation(CreateElicitationRequestParam { + message: format!("Allow Codex to run `{command:?}` in `{workdir:?}`?"), + #[allow(clippy::expect_used)] + requested_schema: ElicitationSchema::builder() + .property("dummy", PrimitiveSchema::String(StringSchema::new())) + .build() + .expect("failed to build elicitation schema"), + }) + .await + .map_err(|e| McpError::internal_error(e.to_string(), None)) + } +} + +#[async_trait::async_trait] +impl EscalationPolicy for McpEscalationPolicy { + async fn determine_action( + &self, + file: &Path, + argv: &[String], + workdir: &Path, + ) -> Result { + let outcome = (self.policy)(file, argv, workdir); + let action = match outcome { + ExecPolicyOutcome::Allow { + run_with_escalated_permissions, + } => { + if run_with_escalated_permissions { + EscalateAction::Escalate + } else { + EscalateAction::Run + } + } + ExecPolicyOutcome::Prompt { + run_with_escalated_permissions, + } => { + let result = self + .prompt(file, argv, workdir, self.context.clone()) + .await?; + // TODO: Extract reason from `result.content`. + match result.action { + ElicitationAction::Accept => { + if run_with_escalated_permissions { + EscalateAction::Escalate + } else { + EscalateAction::Run + } + } + ElicitationAction::Decline => EscalateAction::Deny { + reason: Some("User declined execution".to_string()), + }, + ElicitationAction::Cancel => EscalateAction::Deny { + reason: Some("User cancelled execution".to_string()), + }, + } + } + ExecPolicyOutcome::Forbidden => EscalateAction::Deny { + reason: Some("Execution forbidden by policy".to_string()), + }, + }; + Ok(action) + } +} 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/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index da2efb63c1..6738d7672d 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -114,6 +114,11 @@ impl BottomPane { self.status.as_ref() } + #[cfg(test)] + pub(crate) fn context_window_percent(&self) -> Option { + self.context_window_percent + } + fn active_view(&self) -> Option<&dyn BottomPaneView> { self.view_stack.last().map(std::convert::AsRef::as_ref) } diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 2642648a08..987a956fe6 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -290,6 +290,8 @@ pub(crate) struct ChatWidget { pending_notification: Option, // Simple review mode flag; used to adjust layout and banners. is_review_mode: bool, + // Snapshot of token usage to restore after review mode exits. + pre_review_token_info: Option>, // Whether to add a final message separator after the last message needs_final_message_separator: bool, @@ -489,16 +491,39 @@ impl ChatWidget { } pub(crate) fn set_token_info(&mut self, info: Option) { - if let Some(info) = info { - let context_window = info - .model_context_window - .or(self.config.model_context_window); - let percent = context_window.map(|window| { + match info { + Some(info) => self.apply_token_info(info), + None => { + self.bottom_pane.set_context_window_percent(None); + self.token_info = None; + } + } + } + + fn apply_token_info(&mut self, info: TokenUsageInfo) { + let percent = self.context_remaining_percent(&info); + self.bottom_pane.set_context_window_percent(percent); + self.token_info = Some(info); + } + + fn context_remaining_percent(&self, info: &TokenUsageInfo) -> Option { + info.model_context_window + .or(self.config.model_context_window) + .map(|window| { info.last_token_usage .percent_of_context_window_remaining(window) - }); - self.bottom_pane.set_context_window_percent(percent); - self.token_info = Some(info); + }) + } + + fn restore_pre_review_token_info(&mut self) { + if let Some(saved) = self.pre_review_token_info.take() { + match saved { + Some(info) => self.apply_token_info(info), + None => { + self.bottom_pane.set_context_window_percent(None); + self.token_info = None; + } + } } } @@ -1150,6 +1175,7 @@ impl ChatWidget { suppress_session_configured_redraw: false, pending_notification: None, is_review_mode: false, + pre_review_token_info: None, needs_final_message_separator: false, last_rendered_width: std::cell::Cell::new(None), feedback, @@ -1223,6 +1249,7 @@ impl ChatWidget { suppress_session_configured_redraw: true, pending_notification: None, is_review_mode: false, + pre_review_token_info: None, needs_final_message_separator: false, last_rendered_width: std::cell::Cell::new(None), feedback, @@ -1691,6 +1718,9 @@ impl ChatWidget { fn on_entered_review_mode(&mut self, review: ReviewRequest) { // Enter review mode and emit a concise banner + if self.pre_review_token_info.is_none() { + self.pre_review_token_info = Some(self.token_info.clone()); + } self.is_review_mode = true; let banner = format!(">> Code review started: {} <<", review.user_facing_hint); self.add_to_history(history_cell::new_review_status_line(banner)); @@ -1731,6 +1761,7 @@ impl ChatWidget { } self.is_review_mode = false; + self.restore_pre_review_token_info(); // Append a finishing banner at the end of this turn. self.add_to_history(history_cell::new_review_status_line( "<< Code review finished >>".to_string(), 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; } }; diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 0f67579b6e..d60a20b8f1 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -38,6 +38,9 @@ use codex_core::protocol::ReviewRequest; use codex_core::protocol::StreamErrorEvent; use codex_core::protocol::TaskCompleteEvent; use codex_core::protocol::TaskStartedEvent; +use codex_core::protocol::TokenCountEvent; +use codex_core::protocol::TokenUsage; +use codex_core::protocol::TokenUsageInfo; use codex_core::protocol::UndoCompletedEvent; use codex_core::protocol::UndoStartedEvent; use codex_core::protocol::ViewImageToolCallEvent; @@ -215,6 +218,81 @@ fn exited_review_mode_emits_results_and_finishes() { assert!(!chat.is_review_mode); } +/// Exiting review restores the pre-review context window indicator. +#[test] +fn review_restores_context_window_indicator() { + let (mut chat, mut rx, _ops) = make_chatwidget_manual(); + + let context_window = 13_000; + let pre_review_tokens = 12_700; // ~30% remaining after subtracting baseline. + let review_tokens = 12_030; // ~97% remaining after subtracting baseline. + + chat.handle_codex_event(Event { + id: "token-before".into(), + msg: EventMsg::TokenCount(TokenCountEvent { + info: Some(make_token_info(pre_review_tokens, context_window)), + rate_limits: None, + }), + }); + assert_eq!(chat.bottom_pane.context_window_percent(), Some(30)); + + chat.handle_codex_event(Event { + id: "review-start".into(), + msg: EventMsg::EnteredReviewMode(ReviewRequest { + prompt: "Review the latest changes".to_string(), + user_facing_hint: "feature branch".to_string(), + append_to_original_thread: true, + }), + }); + + chat.handle_codex_event(Event { + id: "token-review".into(), + msg: EventMsg::TokenCount(TokenCountEvent { + info: Some(make_token_info(review_tokens, context_window)), + rate_limits: None, + }), + }); + assert_eq!(chat.bottom_pane.context_window_percent(), Some(97)); + + chat.handle_codex_event(Event { + id: "review-end".into(), + msg: EventMsg::ExitedReviewMode(ExitedReviewModeEvent { + review_output: None, + }), + }); + let _ = drain_insert_history(&mut rx); + + assert_eq!(chat.bottom_pane.context_window_percent(), Some(30)); + assert!(!chat.is_review_mode); +} + +/// Receiving a TokenCount event without usage clears the context indicator. +#[test] +fn token_count_none_resets_context_indicator() { + let (mut chat, _rx, _ops) = make_chatwidget_manual(); + + let context_window = 13_000; + let pre_compact_tokens = 12_700; + + chat.handle_codex_event(Event { + id: "token-before".into(), + msg: EventMsg::TokenCount(TokenCountEvent { + info: Some(make_token_info(pre_compact_tokens, context_window)), + rate_limits: None, + }), + }); + assert_eq!(chat.bottom_pane.context_window_percent(), Some(30)); + + chat.handle_codex_event(Event { + id: "token-cleared".into(), + msg: EventMsg::TokenCount(TokenCountEvent { + info: None, + rate_limits: None, + }), + }); + assert_eq!(chat.bottom_pane.context_window_percent(), None); +} + #[cfg_attr( target_os = "macos", ignore = "system configuration APIs are blocked under macOS seatbelt" @@ -292,6 +370,7 @@ fn make_chatwidget_manual() -> ( suppress_session_configured_redraw: false, pending_notification: None, is_review_mode: false, + pre_review_token_info: None, needs_final_message_separator: false, last_rendered_width: std::cell::Cell::new(None), feedback: codex_feedback::CodexFeedback::new(), @@ -338,6 +417,21 @@ fn lines_to_single_string(lines: &[ratatui::text::Line<'static>]) -> String { s } +fn make_token_info(total_tokens: i64, context_window: i64) -> TokenUsageInfo { + fn usage(total_tokens: i64) -> TokenUsage { + TokenUsage { + total_tokens, + ..TokenUsage::default() + } + } + + TokenUsageInfo { + total_token_usage: usage(total_tokens), + last_token_usage: usage(total_tokens), + model_context_window: Some(context_window), + } +} + #[test] fn rate_limit_warnings_emit_thresholds() { let mut state = RateLimitWarningState::default(); diff --git a/codex-rs/windows-sandbox-rs/src/audit.rs b/codex-rs/windows-sandbox-rs/src/audit.rs index 8873bc2e91..cba8d06335 100644 --- a/codex-rs/windows-sandbox-rs/src/audit.rs +++ b/codex-rs/windows-sandbox-rs/src/audit.rs @@ -9,15 +9,29 @@ use std::path::PathBuf; use std::time::Duration; use std::time::Instant; use windows_sys::Win32::Foundation::CloseHandle; -use windows_sys::Win32::Foundation::LocalFree; use windows_sys::Win32::Foundation::ERROR_SUCCESS; use windows_sys::Win32::Foundation::HLOCAL; use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; +use windows_sys::Win32::Foundation::LocalFree; +use windows_sys::Win32::Security::ACCESS_ALLOWED_ACE; +use windows_sys::Win32::Security::ACE_HEADER; +use windows_sys::Win32::Security::ACL; +use windows_sys::Win32::Security::ACL_SIZE_INFORMATION; +use windows_sys::Win32::Security::AclSizeInformation; use windows_sys::Win32::Security::Authorization::GetNamedSecurityInfoW; use windows_sys::Win32::Security::Authorization::GetSecurityInfo; +use windows_sys::Win32::Security::DACL_SECURITY_INFORMATION; +use windows_sys::Win32::Security::EqualSid; +use windows_sys::Win32::Security::GetAce; +use windows_sys::Win32::Security::GetAclInformation; +use windows_sys::Win32::Security::MapGenericMask; +use windows_sys::Win32::Security::GENERIC_MAPPING; use windows_sys::Win32::Storage::FileSystem::CreateFileW; +use windows_sys::Win32::Storage::FileSystem::FILE_ALL_ACCESS; use windows_sys::Win32::Storage::FileSystem::FILE_APPEND_DATA; use windows_sys::Win32::Storage::FileSystem::FILE_FLAG_BACKUP_SEMANTICS; +use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_EXECUTE; +use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_READ; use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_WRITE; use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_DELETE; use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ; @@ -26,17 +40,6 @@ use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_ATTRIBUTES; use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_DATA; use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_EA; use windows_sys::Win32::Storage::FileSystem::OPEN_EXISTING; -const GENERIC_ALL_MASK: u32 = 0x1000_0000; -const GENERIC_WRITE_MASK: u32 = 0x4000_0000; -use windows_sys::Win32::Security::AclSizeInformation; -use windows_sys::Win32::Security::EqualSid; -use windows_sys::Win32::Security::GetAce; -use windows_sys::Win32::Security::GetAclInformation; -use windows_sys::Win32::Security::ACCESS_ALLOWED_ACE; -use windows_sys::Win32::Security::ACE_HEADER; -use windows_sys::Win32::Security::ACL; -use windows_sys::Win32::Security::ACL_SIZE_INFORMATION; -use windows_sys::Win32::Security::DACL_SECURITY_INFORMATION; // Preflight scan limits const MAX_ITEMS_PER_DIR: i32 = 1000; @@ -304,7 +307,7 @@ pub fn world_writable_warning_details( } } // Fast mask-based check: does the DACL contain any ACCESS_ALLOWED ACE for -// Everyone that includes generic or specific write bits? Skips inherit-only +// Everyone that grants write after generic bits are expanded? Skips inherit-only // ACEs (do not apply to the current object). unsafe fn dacl_quick_world_write_mask_allows(p_dacl: *mut ACL, psid_world: *mut c_void) -> bool { if p_dacl.is_null() { @@ -321,6 +324,12 @@ unsafe fn dacl_quick_world_write_mask_allows(p_dacl: *mut ACL, psid_world: *mut if ok == 0 { return false; } + let mapping = GENERIC_MAPPING { + GenericRead: FILE_GENERIC_READ, + GenericWrite: FILE_GENERIC_WRITE, + GenericExecute: FILE_GENERIC_EXECUTE, + GenericAll: FILE_ALL_ACCESS, + }; for i in 0..(info.AceCount as usize) { let mut p_ace: *mut c_void = std::ptr::null_mut(); if GetAce(p_dacl as *const ACL, i as u32, &mut p_ace) == 0 { @@ -337,19 +346,16 @@ unsafe fn dacl_quick_world_write_mask_allows(p_dacl: *mut ACL, psid_world: *mut let base = p_ace as usize; let sid_ptr = (base + std::mem::size_of::() + std::mem::size_of::()) as *mut c_void; // skip header + mask - if EqualSid(sid_ptr, psid_world) != 0 { - let ace = &*(p_ace as *const ACCESS_ALLOWED_ACE); - let mask = ace.Mask; - let writey = FILE_GENERIC_WRITE - | FILE_WRITE_DATA - | FILE_APPEND_DATA - | FILE_WRITE_EA - | FILE_WRITE_ATTRIBUTES - | GENERIC_WRITE_MASK - | GENERIC_ALL_MASK; - if (mask & writey) != 0 { - return true; - } + if EqualSid(sid_ptr, psid_world) == 0 { + continue; + } + let ace = &*(p_ace as *const ACCESS_ALLOWED_ACE); + let mut mask = ace.Mask; + // Expand generic bits to concrete file rights before checking for write. + MapGenericMask(&mut mask, &mapping); + let write_mask = FILE_WRITE_DATA | FILE_APPEND_DATA | FILE_WRITE_EA | FILE_WRITE_ATTRIBUTES; + if (mask & write_mask) != 0 { + return true; } } false