diff --git a/README.md b/README.md index 4fed276405..b90e6d6d7e 100644 --- a/README.md +++ b/README.md @@ -92,15 +92,15 @@ prefix_rule( 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: +Use the `codex execpolicy check` subcommand to preview decisions before you save a rule (see the [`codex-execpolicy` README](./codex-rs/execpolicy/README.md) for syntax details): ```shell -cargo run -p codex-execpolicy2 -- check --policy ~/.codex/policy/default.codexpolicy git push origin main +codex execpolicy 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. +Pass multiple `--policy` flags to test how several files combine, and use `--pretty` for formatted JSON output. See the [`codex-rs/execpolicy` README](./codex-rs/execpolicy/README.md) for a more detailed walkthrough of the available syntax. ---- +## Note: `execpolicy` commands are still in preview. The API may have breaking changes in the future. ### Docs & FAQ diff --git a/codex-rs/.config/nextest.toml b/codex-rs/.config/nextest.toml index 3ca7cfe503..f432af88ea 100644 --- a/codex-rs/.config/nextest.toml +++ b/codex-rs/.config/nextest.toml @@ -7,3 +7,7 @@ slow-timeout = { period = "15s", terminate-after = 2 } # Do not add new tests here filter = 'test(rmcp_client) | test(humanlike_typing_1000_chars_appears_live_no_placeholder)' slow-timeout = { period = "1m", terminate-after = 4 } + +[[profile.default.overrides]] +filter = 'test(approval_matrix_covers_all_modes)' +slow-timeout = { period = "30s", terminate-after = 2 } diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index fbbdb3d1f0..fea573f8ac 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -891,6 +891,7 @@ dependencies = [ "serde", "serde_json", "strum_macros 0.27.2", + "thiserror 2.0.17", "ts-rs", "uuid", ] @@ -1001,6 +1002,7 @@ dependencies = [ "codex-common", "codex-core", "codex-exec", + "codex-execpolicy", "codex-login", "codex-mcp-server", "codex-process-hardening", diff --git a/codex-rs/app-server-protocol/Cargo.toml b/codex-rs/app-server-protocol/Cargo.toml index 4d1afadaa1..47753b344a 100644 --- a/codex-rs/app-server-protocol/Cargo.toml +++ b/codex-rs/app-server-protocol/Cargo.toml @@ -19,6 +19,7 @@ schemars = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } strum_macros = { workspace = true } +thiserror = { workspace = true } ts-rs = { workspace = true } uuid = { workspace = true, features = ["serde", "v7"] } diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index 650b6f31a6..fecdc5b71b 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -378,7 +378,7 @@ macro_rules! server_notification_definitions { impl TryFrom for ServerNotification { type Error = serde_json::Error; - fn try_from(value: JSONRPCNotification) -> Result { + fn try_from(value: JSONRPCNotification) -> Result { serde_json::from_value(serde_json::to_value(value)?) } } @@ -487,6 +487,7 @@ pub struct FuzzyFileSearchResponse { server_notification_definitions! { /// NEW NOTIFICATIONS + Error => "error" (v2::ErrorNotification), ThreadStarted => "thread/started" (v2::ThreadStartedNotification), TurnStarted => "turn/started" (v2::TurnStartedNotification), TurnCompleted => "turn/completed" (v2::TurnCompletedNotification), diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 6b61245bc6..6ecf0f9256 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -11,6 +11,7 @@ use codex_protocol::items::AgentMessageContent as CoreAgentMessageContent; use codex_protocol::items::TurnItem as CoreTurnItem; use codex_protocol::models::ResponseItem; use codex_protocol::parse_command::ParsedCommand as CoreParsedCommand; +use codex_protocol::protocol::CodexErrorInfo as CoreCodexErrorInfo; use codex_protocol::protocol::CreditsSnapshot as CoreCreditsSnapshot; use codex_protocol::protocol::RateLimitSnapshot as CoreRateLimitSnapshot; use codex_protocol::protocol::RateLimitWindow as CoreRateLimitWindow; @@ -20,6 +21,7 @@ use schemars::JsonSchema; use serde::Deserialize; use serde::Serialize; use serde_json::Value as JsonValue; +use thiserror::Error; use ts_rs::TS; // Macro to declare a camelCased API v2 enum mirroring a core enum which @@ -47,6 +49,69 @@ macro_rules! v2_enum_from_core { }; } +/// This translation layer make sure that we expose codex error code in camel case. +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub enum CodexErrorInfo { + ContextWindowExceeded, + UsageLimitExceeded, + HttpConnectionFailed { + #[serde(rename = "httpStatusCode")] + #[ts(rename = "httpStatusCode")] + http_status_code: Option, + }, + /// Failed to connect to the response SSE stream. + ResponseStreamConnectionFailed { + #[serde(rename = "httpStatusCode")] + #[ts(rename = "httpStatusCode")] + http_status_code: Option, + }, + InternalServerError, + Unauthorized, + BadRequest, + SandboxError, + /// The response SSE stream disconnected in the middle of a turn before completion. + ResponseStreamDisconnected { + #[serde(rename = "httpStatusCode")] + #[ts(rename = "httpStatusCode")] + http_status_code: Option, + }, + /// Reached the retry limit for responses. + ResponseTooManyFailedAttempts { + #[serde(rename = "httpStatusCode")] + #[ts(rename = "httpStatusCode")] + http_status_code: Option, + }, + Other, +} + +impl From for CodexErrorInfo { + fn from(value: CoreCodexErrorInfo) -> Self { + match value { + CoreCodexErrorInfo::ContextWindowExceeded => CodexErrorInfo::ContextWindowExceeded, + CoreCodexErrorInfo::UsageLimitExceeded => CodexErrorInfo::UsageLimitExceeded, + CoreCodexErrorInfo::HttpConnectionFailed { http_status_code } => { + CodexErrorInfo::HttpConnectionFailed { http_status_code } + } + CoreCodexErrorInfo::ResponseStreamConnectionFailed { http_status_code } => { + CodexErrorInfo::ResponseStreamConnectionFailed { http_status_code } + } + CoreCodexErrorInfo::InternalServerError => CodexErrorInfo::InternalServerError, + CoreCodexErrorInfo::Unauthorized => CodexErrorInfo::Unauthorized, + CoreCodexErrorInfo::BadRequest => CodexErrorInfo::BadRequest, + CoreCodexErrorInfo::SandboxError => CodexErrorInfo::SandboxError, + CoreCodexErrorInfo::ResponseStreamDisconnected { http_status_code } => { + CodexErrorInfo::ResponseStreamDisconnected { http_status_code } + } + CoreCodexErrorInfo::ResponseTooManyFailedAttempts { http_status_code } => { + CodexErrorInfo::ResponseTooManyFailedAttempts { http_status_code } + } + CoreCodexErrorInfo::Other => CodexErrorInfo::Other, + } + } +} + v2_enum_from_core!( pub enum AskForApproval from codex_protocol::protocol::AskForApproval { UnlessTrusted, OnFailure, OnRequest, Never @@ -544,11 +609,20 @@ pub struct Turn { pub status: TurnStatus, } +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS, Error)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +#[error("{message}")] +pub struct TurnError { + pub message: String, + pub codex_error_code: Option, +} + #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] -pub struct TurnError { - pub message: String, +pub struct ErrorNotification { + pub error: TurnError, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] @@ -1091,6 +1165,7 @@ mod tests { use codex_protocol::items::WebSearchItem; use codex_protocol::user_input::UserInput as CoreUserInput; use pretty_assertions::assert_eq; + use serde_json::json; use std::path::PathBuf; #[test] @@ -1176,4 +1251,20 @@ mod tests { } ); } + + #[test] + fn codex_error_code_serializes_http_status_code_in_camel_case() { + let value = CodexErrorInfo::ResponseTooManyFailedAttempts { + http_status_code: Some(401), + }; + + assert_eq!( + serde_json::to_value(value).unwrap(), + json!({ + "responseTooManyFailedAttempts": { + "httpStatusCode": 401 + } + }) + ); + } } diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index d4b00e67f9..69346d2e31 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -8,11 +8,13 @@ use codex_app_server_protocol::AgentMessageDeltaNotification; use codex_app_server_protocol::ApplyPatchApprovalParams; use codex_app_server_protocol::ApplyPatchApprovalResponse; use codex_app_server_protocol::ApprovalDecision; +use codex_app_server_protocol::CodexErrorInfo as V2CodexErrorInfo; use codex_app_server_protocol::CommandAction as V2ParsedCommand; use codex_app_server_protocol::CommandExecutionOutputDeltaNotification; use codex_app_server_protocol::CommandExecutionRequestApprovalParams; use codex_app_server_protocol::CommandExecutionRequestApprovalResponse; use codex_app_server_protocol::CommandExecutionStatus; +use codex_app_server_protocol::ErrorNotification; use codex_app_server_protocol::ExecCommandApprovalParams; use codex_app_server_protocol::ExecCommandApprovalResponse; use codex_app_server_protocol::FileChangeRequestApprovalParams; @@ -260,7 +262,29 @@ pub(crate) async fn apply_bespoke_event_handling( } } EventMsg::Error(ev) => { - handle_error(conversation_id, ev.message, &turn_summary_store).await; + let turn_error = TurnError { + message: ev.message, + codex_error_code: ev.codex_error_code.map(V2CodexErrorInfo::from), + }; + handle_error(conversation_id, turn_error.clone(), &turn_summary_store).await; + outgoing + .send_server_notification(ServerNotification::Error(ErrorNotification { + error: turn_error, + })) + .await; + } + EventMsg::StreamError(ev) => { + // We don't need to update the turn summary store for stream errors as they are intermediate error states for retries, + // but we notify the client. + let turn_error = TurnError { + message: ev.message, + codex_error_code: ev.codex_error_code.map(V2CodexErrorInfo::from), + }; + outgoing + .send_server_notification(ServerNotification::Error(ErrorNotification { + error: turn_error, + })) + .await; } EventMsg::EnteredReviewMode(review_request) => { let notification = ItemStartedNotification { @@ -508,10 +532,8 @@ async fn handle_turn_complete( ) { let turn_summary = find_and_remove_turn_summary(conversation_id, turn_summary_store).await; - let status = if let Some(message) = turn_summary.last_error_message { - TurnStatus::Failed { - error: TurnError { message }, - } + let status = if let Some(error) = turn_summary.last_error { + TurnStatus::Failed { error } } else { TurnStatus::Completed }; @@ -532,11 +554,11 @@ async fn handle_turn_interrupted( async fn handle_error( conversation_id: ConversationId, - message: String, + error: TurnError, turn_summary_store: &TurnSummaryStore, ) { let mut map = turn_summary_store.lock().await; - map.entry(conversation_id).or_default().last_error_message = Some(message); + map.entry(conversation_id).or_default().last_error = Some(error); } async fn on_patch_approval_response( @@ -873,10 +895,24 @@ mod tests { let conversation_id = ConversationId::new(); let turn_summary_store = new_turn_summary_store(); - handle_error(conversation_id, "boom".to_string(), &turn_summary_store).await; + handle_error( + conversation_id, + TurnError { + message: "boom".to_string(), + codex_error_code: Some(V2CodexErrorInfo::InternalServerError), + }, + &turn_summary_store, + ) + .await; let turn_summary = find_and_remove_turn_summary(conversation_id, &turn_summary_store).await; - assert_eq!(turn_summary.last_error_message, Some("boom".to_string())); + assert_eq!( + turn_summary.last_error, + Some(TurnError { + message: "boom".to_string(), + codex_error_code: Some(V2CodexErrorInfo::InternalServerError), + }) + ); Ok(()) } @@ -916,7 +952,15 @@ mod tests { let conversation_id = ConversationId::new(); let event_id = "interrupt1".to_string(); let turn_summary_store = new_turn_summary_store(); - handle_error(conversation_id, "oops".to_string(), &turn_summary_store).await; + handle_error( + conversation_id, + TurnError { + message: "oops".to_string(), + codex_error_code: None, + }, + &turn_summary_store, + ) + .await; let (tx, mut rx) = mpsc::channel(CHANNEL_CAPACITY); let outgoing = Arc::new(OutgoingMessageSender::new(tx)); @@ -948,7 +992,15 @@ mod tests { let conversation_id = ConversationId::new(); let event_id = "complete_err1".to_string(); let turn_summary_store = new_turn_summary_store(); - handle_error(conversation_id, "bad".to_string(), &turn_summary_store).await; + handle_error( + conversation_id, + TurnError { + message: "bad".to_string(), + codex_error_code: Some(V2CodexErrorInfo::Other), + }, + &turn_summary_store, + ) + .await; let (tx, mut rx) = mpsc::channel(CHANNEL_CAPACITY); let outgoing = Arc::new(OutgoingMessageSender::new(tx)); @@ -972,6 +1024,7 @@ mod tests { TurnStatus::Failed { error: TurnError { message: "bad".to_string(), + codex_error_code: Some(V2CodexErrorInfo::Other), } } ); @@ -1022,7 +1075,15 @@ mod tests { // Turn 1 on conversation A let a_turn1 = "a_turn1".to_string(); - handle_error(conversation_a, "a1".to_string(), &turn_summary_store).await; + handle_error( + conversation_a, + TurnError { + message: "a1".to_string(), + codex_error_code: Some(V2CodexErrorInfo::BadRequest), + }, + &turn_summary_store, + ) + .await; handle_turn_complete( conversation_a, a_turn1.clone(), @@ -1033,7 +1094,15 @@ mod tests { // Turn 1 on conversation B let b_turn1 = "b_turn1".to_string(); - handle_error(conversation_b, "b1".to_string(), &turn_summary_store).await; + handle_error( + conversation_b, + TurnError { + message: "b1".to_string(), + codex_error_code: None, + }, + &turn_summary_store, + ) + .await; handle_turn_complete( conversation_b, b_turn1.clone(), @@ -1065,6 +1134,7 @@ mod tests { TurnStatus::Failed { error: TurnError { message: "a1".to_string(), + codex_error_code: Some(V2CodexErrorInfo::BadRequest), } } ); @@ -1085,6 +1155,7 @@ mod tests { TurnStatus::Failed { error: TurnError { message: "b1".to_string(), + codex_error_code: None, } } ); diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index a8e6ba66f7..aaf92fe4c1 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -83,6 +83,7 @@ use codex_app_server_protocol::ThreadStartParams; use codex_app_server_protocol::ThreadStartResponse; use codex_app_server_protocol::ThreadStartedNotification; use codex_app_server_protocol::Turn; +use codex_app_server_protocol::TurnError; use codex_app_server_protocol::TurnInterruptParams; use codex_app_server_protocol::TurnStartParams; use codex_app_server_protocol::TurnStartResponse; @@ -161,8 +162,8 @@ pub(crate) type PendingInterrupts = Arc, pub(crate) file_change_started: HashSet, + pub(crate) last_error: Option, } pub(crate) type TurnSummaryStore = Arc>>; diff --git a/codex-rs/cli/Cargo.toml b/codex-rs/cli/Cargo.toml index deddc068cd..e7999b5ce7 100644 --- a/codex-rs/cli/Cargo.toml +++ b/codex-rs/cli/Cargo.toml @@ -26,6 +26,7 @@ codex-cloud-tasks = { path = "../cloud-tasks" } codex-common = { workspace = true, features = ["cli"] } codex-core = { workspace = true } codex-exec = { workspace = true } +codex-execpolicy = { workspace = true } codex-login = { workspace = true } codex-mcp-server = { workspace = true } codex-process-hardening = { workspace = true } diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index bb60870de5..2b066197af 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -18,6 +18,7 @@ use codex_cli::login::run_logout; use codex_cloud_tasks::Cli as CloudTasksCli; use codex_common::CliConfigOverrides; use codex_exec::Cli as ExecCli; +use codex_execpolicy::ExecPolicyCheckCommand; use codex_responses_api_proxy::Args as ResponsesApiProxyArgs; use codex_tui::AppExitInfo; use codex_tui::Cli as TuiCli; @@ -93,6 +94,10 @@ enum Subcommand { #[clap(visible_alias = "debug")] Sandbox(SandboxArgs), + /// Execpolicy tooling. + #[clap(hide = true)] + Execpolicy(ExecpolicyCommand), + /// Apply the latest diff produced by Codex agent as a `git apply` to your local working tree. #[clap(visible_alias = "a")] Apply(ApplyCommand), @@ -162,6 +167,19 @@ enum SandboxCommand { Windows(WindowsCommand), } +#[derive(Debug, Parser)] +struct ExecpolicyCommand { + #[command(subcommand)] + sub: ExecpolicySubcommand, +} + +#[derive(Debug, clap::Subcommand)] +enum ExecpolicySubcommand { + /// Check execpolicy files against a command. + #[clap(name = "check")] + Check(ExecPolicyCheckCommand), +} + #[derive(Debug, Parser)] struct LoginCommand { #[clap(skip)] @@ -327,6 +345,10 @@ fn run_update_action(action: UpdateAction) -> anyhow::Result<()> { Ok(()) } +fn run_execpolicycheck(cmd: ExecPolicyCheckCommand) -> anyhow::Result<()> { + cmd.run() +} + #[derive(Debug, Default, Parser, Clone)] struct FeatureToggles { /// Enable a feature (repeatable). Equivalent to `-c features.=true`. @@ -549,6 +571,9 @@ async fn cli_main(codex_linux_sandbox_exe: Option) -> anyhow::Result<() .await?; } }, + Some(Subcommand::Execpolicy(ExecpolicyCommand { sub })) => match sub { + ExecpolicySubcommand::Check(cmd) => run_execpolicycheck(cmd)?, + }, Some(Subcommand::Apply(mut apply_cli)) => { prepend_config_flags( &mut apply_cli.config_overrides, diff --git a/codex-rs/cli/tests/execpolicy.rs b/codex-rs/cli/tests/execpolicy.rs new file mode 100644 index 0000000000..c6bca85bc6 --- /dev/null +++ b/codex-rs/cli/tests/execpolicy.rs @@ -0,0 +1,58 @@ +use std::fs; + +use assert_cmd::Command; +use pretty_assertions::assert_eq; +use serde_json::json; +use tempfile::TempDir; + +#[test] +fn execpolicy_check_matches_expected_json() -> Result<(), Box> { + let codex_home = TempDir::new()?; + let policy_path = codex_home.path().join("policy.codexpolicy"); + fs::write( + &policy_path, + r#" +prefix_rule( + pattern = ["git", "push"], + decision = "forbidden", +) +"#, + )?; + + let output = Command::cargo_bin("codex")? + .env("CODEX_HOME", codex_home.path()) + .args([ + "execpolicy", + "check", + "--policy", + policy_path + .to_str() + .expect("policy path should be valid UTF-8"), + "git", + "push", + "origin", + "main", + ]) + .output()?; + + assert!(output.status.success()); + let result: serde_json::Value = serde_json::from_slice(&output.stdout)?; + assert_eq!( + result, + json!({ + "match": { + "decision": "forbidden", + "matchedRules": [ + { + "prefixRuleMatch": { + "matchedPrefix": ["git", "push"], + "decision": "forbidden" + } + } + ] + } + }) + ); + + Ok(()) +} diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 1cf4e923cd..8b8292cd16 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -79,7 +79,6 @@ 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; @@ -129,6 +128,7 @@ use codex_protocol::models::ContentItem; use codex_protocol::models::FunctionCallOutputPayload; use codex_protocol::models::ResponseInputItem; use codex_protocol::models::ResponseItem; +use codex_protocol::protocol::CodexErrorInfo; use codex_protocol::protocol::InitialHistory; use codex_protocol::user_input::UserInput; use codex_utils_readiness::Readiness; @@ -1189,9 +1189,14 @@ impl Session { &self, turn_context: &TurnContext, message: impl Into, + codex_error: CodexErr, ) { + let codex_error_code = CodexErrorInfo::ResponseStreamDisconnected { + http_status_code: codex_error.http_status_code_value(), + }; let event = EventMsg::StreamError(StreamErrorEvent { message: message.into(), + codex_error_code: Some(codex_error_code), }); self.send_event(turn_context, event).await; } @@ -1437,6 +1442,7 @@ mod handlers { use crate::tasks::UndoTask; use crate::tasks::UserShellCommandTask; use codex_protocol::custom_prompts::CustomPrompt; + use codex_protocol::protocol::CodexErrorInfo; use codex_protocol::protocol::ErrorEvent; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; @@ -1683,6 +1689,7 @@ mod handlers { id: sub_id.clone(), msg: EventMsg::Error(ErrorEvent { message: "Failed to shutdown rollout recorder".to_string(), + codex_error_code: Some(CodexErrorInfo::Other), }), }; sess.send_event_raw(event).await; @@ -1937,9 +1944,7 @@ pub(crate) async fn run_task( } Err(e) => { info!("Turn error: {e:#}"); - let event = EventMsg::Error(ErrorEvent { - message: e.to_string(), - }); + let event = EventMsg::Error(e.to_error_event(None)); sess.send_event(&turn_context, event).await; // let the user continue the conversation break; @@ -2064,6 +2069,7 @@ async fn run_turn( sess.notify_stream_error( &turn_context, format!("Reconnecting... {retries}/{max_retries}"), + e, ) .await; diff --git a/codex-rs/core/src/compact.rs b/codex-rs/core/src/compact.rs index 8c38f93937..1b3937b9fe 100644 --- a/codex-rs/core/src/compact.rs +++ b/codex-rs/core/src/compact.rs @@ -10,7 +10,6 @@ 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; @@ -128,9 +127,7 @@ async fn run_compact_task_inner( continue; } sess.set_total_tokens_full(turn_context.as_ref()).await; - let event = EventMsg::Error(ErrorEvent { - message: e.to_string(), - }); + let event = EventMsg::Error(e.to_error_event(None)); sess.send_event(&turn_context, event).await; return; } @@ -141,14 +138,13 @@ async fn run_compact_task_inner( sess.notify_stream_error( turn_context.as_ref(), format!("Reconnecting... {retries}/{max_retries}"), + e, ) .await; tokio::time::sleep(delay).await; continue; } else { - let event = EventMsg::Error(ErrorEvent { - message: e.to_string(), - }); + let event = EventMsg::Error(e.to_error_event(None)); 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 0d2e0f138d..534d794f0c 100644 --- a/codex-rs/core/src/compact_remote.rs +++ b/codex-rs/core/src/compact_remote.rs @@ -6,7 +6,6 @@ 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; @@ -30,9 +29,9 @@ 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 = EventMsg::Error(ErrorEvent { - message: format!("Error running remote compact task: {err}"), - }); + let event = EventMsg::Error( + err.to_error_event(Some("Error running remote compact task".to_string())), + ); sess.send_event(turn_context, event).await; } } diff --git a/codex-rs/core/src/error.rs b/codex-rs/core/src/error.rs index b2027dc940..ee72930499 100644 --- a/codex-rs/core/src/error.rs +++ b/codex-rs/core/src/error.rs @@ -10,6 +10,8 @@ use chrono::Local; use chrono::Utc; use codex_async_utils::CancelErr; use codex_protocol::ConversationId; +use codex_protocol::protocol::CodexErrorInfo; +use codex_protocol::protocol::ErrorEvent; use codex_protocol::protocol::RateLimitSnapshot; use reqwest::StatusCode; use serde_json; @@ -430,6 +432,57 @@ impl CodexErr { pub fn downcast_ref(&self) -> Option<&T> { (self as &dyn std::any::Any).downcast_ref::() } + + /// Translate core error to client-facing protocol error. + pub fn to_codex_protocol_error(&self) -> CodexErrorInfo { + match self { + CodexErr::ContextWindowExceeded => CodexErrorInfo::ContextWindowExceeded, + CodexErr::UsageLimitReached(_) + | CodexErr::QuotaExceeded + | CodexErr::UsageNotIncluded => CodexErrorInfo::UsageLimitExceeded, + CodexErr::RetryLimit(_) => CodexErrorInfo::ResponseTooManyFailedAttempts { + http_status_code: self.http_status_code_value(), + }, + CodexErr::ConnectionFailed(_) => CodexErrorInfo::HttpConnectionFailed { + http_status_code: self.http_status_code_value(), + }, + CodexErr::ResponseStreamFailed(_) => CodexErrorInfo::ResponseStreamConnectionFailed { + http_status_code: self.http_status_code_value(), + }, + CodexErr::RefreshTokenFailed(_) => CodexErrorInfo::Unauthorized, + CodexErr::SessionConfiguredNotFirstEvent + | CodexErr::InternalServerError + | CodexErr::InternalAgentDied => CodexErrorInfo::InternalServerError, + CodexErr::UnsupportedOperation(_) | CodexErr::ConversationNotFound(_) => { + CodexErrorInfo::BadRequest + } + CodexErr::Sandbox(_) => CodexErrorInfo::SandboxError, + _ => CodexErrorInfo::Other, + } + } + + 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, + codex_error_code: Some(self.to_codex_protocol_error()), + } + } + + pub fn http_status_code_value(&self) -> Option { + let http_status_code = match self { + CodexErr::RetryLimit(err) => Some(err.status), + CodexErr::UnexpectedStatus(err) => Some(err.status), + CodexErr::ConnectionFailed(err) => err.source.status(), + CodexErr::ResponseStreamFailed(err) => err.source.status(), + _ => None, + }; + http_status_code.as_ref().map(StatusCode::as_u16) + } } pub fn get_error_message_ui(e: &CodexErr) -> String { @@ -478,6 +531,10 @@ mod tests { use chrono::Utc; use codex_protocol::protocol::RateLimitWindow; use pretty_assertions::assert_eq; + use reqwest::Response; + use reqwest::ResponseBuilderExt; + use reqwest::StatusCode; + use reqwest::Url; fn rate_limit_snapshot() -> RateLimitSnapshot { let primary_reset_at = Utc @@ -573,6 +630,33 @@ mod tests { assert_eq!(get_error_message_ui(&err), "stdout only"); } + #[test] + fn to_error_event_handles_response_stream_failed() { + let response = http::Response::builder() + .status(StatusCode::TOO_MANY_REQUESTS) + .url(Url::parse("http://example.com").unwrap()) + .body("") + .unwrap(); + let source = Response::from(response).error_for_status_ref().unwrap_err(); + let err = CodexErr::ResponseStreamFailed(ResponseStreamFailed { + source, + request_id: Some("req-123".to_string()), + }); + + let event = err.to_error_event(Some("prefix".to_string())); + + assert_eq!( + event.message, + "prefix: Error while reading the server response: HTTP status client error (429 Too Many Requests) for url (http://example.com/), request id: req-123" + ); + assert_eq!( + event.codex_error_code, + Some(CodexErrorInfo::ResponseStreamConnectionFailed { + http_status_code: Some(429) + }) + ); + } + #[test] fn sandbox_denied_reports_exit_code_when_no_output_available() { let output = ExecToolCallOutput { diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index b7a78c3b7d..ed64d07dbf 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -749,6 +749,15 @@ mod tests { #[cfg(unix)] #[tokio::test] async fn kill_child_process_group_kills_grandchildren_on_timeout() -> Result<()> { + // On Linux/macOS, /bin/bash is typically present; on FreeBSD/OpenBSD, + // prefer /bin/sh to avoid NotFound errors. + #[cfg(any(target_os = "freebsd", target_os = "openbsd"))] + let command = vec![ + "/bin/sh".to_string(), + "-c".to_string(), + "sleep 60 & echo $!; sleep 60".to_string(), + ]; + #[cfg(all(unix, not(any(target_os = "freebsd", target_os = "openbsd"))))] let command = vec![ "/bin/bash".to_string(), "-c".to_string(), diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 9525cc1acf..2a5d3904eb 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -109,7 +109,7 @@ fn evaluate_with_policy( } Decision::Allow => Some(ApprovalRequirement::Skip), }, - Evaluation::NoMatch => None, + Evaluation::NoMatch { .. } => None, } } @@ -206,7 +206,7 @@ mod tests { let commands = [vec!["rm".to_string()]]; assert!(matches!( policy.check_multiple(commands.iter()), - Evaluation::NoMatch + Evaluation::NoMatch { .. } )); assert!(!temp_dir.path().join(POLICY_DIR_NAME).exists()); } @@ -259,7 +259,7 @@ mod tests { let command = [vec!["ls".to_string()]]; assert!(matches!( policy.check_multiple(command.iter()), - Evaluation::NoMatch + Evaluation::NoMatch { .. } )); } diff --git a/codex-rs/core/src/features.rs b/codex-rs/core/src/features.rs index 75622e5254..0c67c9ff5c 100644 --- a/codex-rs/core/src/features.rs +++ b/codex-rs/core/src/features.rs @@ -31,9 +31,6 @@ pub enum Feature { GhostCommit, /// Use the single unified PTY-backed exec tool. UnifiedExec, - /// Use the shell command tool that takes `command` as a single string of - /// shell instead of an array of args passed to `execvp(3)`. - ShellCommandTool, /// Enable experimental RMCP features such as OAuth login. RmcpClient, /// Include the freeform apply_patch tool. @@ -275,12 +272,6 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::Experimental, default_enabled: false, }, - FeatureSpec { - id: Feature::ShellCommandTool, - key: "shell_command_tool", - stage: Stage::Experimental, - default_enabled: false, - }, FeatureSpec { id: Feature::RmcpClient, key: "rmcp_client", diff --git a/codex-rs/core/src/shell.rs b/codex-rs/core/src/shell.rs index a275b2e399..ac115facb6 100644 --- a/codex-rs/core/src/shell.rs +++ b/codex-rs/core/src/shell.rs @@ -358,7 +358,8 @@ mod tests { assert!( shell_path == PathBuf::from("/bin/bash") - || shell_path == PathBuf::from("/usr/bin/bash"), + || shell_path == PathBuf::from("/usr/bin/bash") + || shell_path == PathBuf::from("/usr/local/bin/bash"), "shell path: {shell_path:?}", ); } diff --git a/codex-rs/core/src/tasks/user_shell.rs b/codex-rs/core/src/tasks/user_shell.rs index 45c0a6a92b..3ebc29070c 100644 --- a/codex-rs/core/src/tasks/user_shell.rs +++ b/codex-rs/core/src/tasks/user_shell.rs @@ -30,6 +30,8 @@ use crate::user_shell_command::user_shell_command_record_item; use super::SessionTask; use super::SessionTaskContext; +const USER_SHELL_TIMEOUT_MS: u64 = 60 * 60 * 1000; // 1 hour + #[derive(Clone)] pub(crate) struct UserShellCommandTask { command: String, @@ -92,7 +94,7 @@ impl SessionTask for UserShellCommandTask { command: command.clone(), cwd: cwd.clone(), env: create_env(&turn_context.shell_environment_policy), - timeout_ms: None, + timeout_ms: Some(USER_SHELL_TIMEOUT_MS), sandboxed: false, with_escalated_permissions: None, justification: None, diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index eb4585925d..b3d0330f83 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -57,8 +57,6 @@ impl ToolsConfig { ConfigShellToolType::Disabled } else if features.enabled(Feature::UnifiedExec) { ConfigShellToolType::UnifiedExec - } else if features.enabled(Feature::ShellCommandTool) { - ConfigShellToolType::ShellCommand } else { model_family.shell_type.clone() }; @@ -1468,22 +1466,6 @@ mod tests { assert_contains_tool_names(&tools, &subset); } - #[test] - fn test_build_specs_shell_command_present() { - assert_model_tools( - "codex-mini-latest", - Features::with_defaults().enable(Feature::ShellCommandTool), - &[ - "shell_command", - "list_mcp_resources", - "list_mcp_resource_templates", - "read_mcp_resource", - "update_plan", - "view_image", - ], - ); - } - #[test] #[ignore] fn test_parallel_support_flags() { diff --git a/codex-rs/core/tests/suite/model_tools.rs b/codex-rs/core/tests/suite/model_tools.rs index b44f53a7c3..e807ce7db0 100644 --- a/codex-rs/core/tests/suite/model_tools.rs +++ b/codex-rs/core/tests/suite/model_tools.rs @@ -1,21 +1,10 @@ #![allow(clippy::unwrap_used)] -use codex_core::CodexAuth; -use codex_core::ConversationManager; -use codex_core::ModelProviderInfo; -use codex_core::built_in_model_providers; -use codex_core::features::Feature; -use codex_core::model_family::find_family_for_model; -use codex_core::protocol::EventMsg; -use codex_core::protocol::Op; -use codex_protocol::user_input::UserInput; -use core_test_support::load_default_config_for_test; use core_test_support::load_sse_fixture_with_id; use core_test_support::responses; +use core_test_support::responses::start_mock_server; use core_test_support::skip_if_no_network; -use core_test_support::wait_for_event; -use tempfile::TempDir; -use wiremock::MockServer; +use core_test_support::test_codex::test_codex; fn sse_completed(id: &str) -> String { load_sse_fixture_with_id("tests/fixtures/completed_template.json", id) @@ -39,46 +28,17 @@ fn tool_identifiers(body: &serde_json::Value) -> Vec { #[allow(clippy::expect_used)] async fn collect_tool_identifiers_for_model(model: &str) -> Vec { - let server = MockServer::start().await; - + let server = start_mock_server().await; let sse = sse_completed(model); let resp_mock = responses::mount_sse_once(&server, sse).await; - let model_provider = ModelProviderInfo { - base_url: Some(format!("{}/v1", server.uri())), - ..built_in_model_providers()["openai"].clone() - }; - - let cwd = TempDir::new().unwrap(); - let codex_home = TempDir::new().unwrap(); - let mut config = load_default_config_for_test(&codex_home); - config.cwd = cwd.path().to_path_buf(); - config.model_provider = model_provider; - config.model = model.to_string(); - config.model_family = - find_family_for_model(model).unwrap_or_else(|| panic!("unknown model family for {model}")); - config.features.disable(Feature::ApplyPatchFreeform); - config.features.disable(Feature::ViewImageTool); - config.features.disable(Feature::WebSearchRequest); - config.features.disable(Feature::UnifiedExec); - - let conversation_manager = - ConversationManager::with_auth(CodexAuth::from_api_key("Test API Key")); - let codex = conversation_manager - .new_conversation(config) + let mut builder = test_codex().with_model(model); + let test = builder + .build(&server) .await - .expect("create new conversation") - .conversation; + .expect("create test Codex conversation"); - codex - .submit(Op::UserInput { - items: vec![UserInput::Text { - text: "hello tools".into(), - }], - }) - .await - .unwrap(); - wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + test.submit_turn("hello tools").await.expect("submit turn"); let body = resp_mock.single_request().body_json(); tool_identifiers(&body) @@ -97,7 +57,8 @@ async fn model_selects_expected_tools() { "list_mcp_resources".to_string(), "list_mcp_resource_templates".to_string(), "read_mcp_resource".to_string(), - "update_plan".to_string() + "update_plan".to_string(), + "view_image".to_string() ], "codex-mini-latest should expose the local shell tool", ); @@ -111,7 +72,8 @@ async fn model_selects_expected_tools() { "list_mcp_resource_templates".to_string(), "read_mcp_resource".to_string(), "update_plan".to_string(), - "apply_patch".to_string() + "apply_patch".to_string(), + "view_image".to_string() ], "gpt-5-codex should expose the apply_patch tool", ); @@ -125,7 +87,8 @@ async fn model_selects_expected_tools() { "list_mcp_resource_templates".to_string(), "read_mcp_resource".to_string(), "update_plan".to_string(), - "apply_patch".to_string() + "apply_patch".to_string(), + "view_image".to_string() ], "gpt-5.1-codex should expose the apply_patch tool", ); @@ -139,6 +102,7 @@ async fn model_selects_expected_tools() { "list_mcp_resource_templates".to_string(), "read_mcp_resource".to_string(), "update_plan".to_string(), + "view_image".to_string() ], "gpt-5 should expose the apply_patch tool", ); @@ -152,7 +116,8 @@ async fn model_selects_expected_tools() { "list_mcp_resource_templates".to_string(), "read_mcp_resource".to_string(), "update_plan".to_string(), - "apply_patch".to_string() + "apply_patch".to_string(), + "view_image".to_string() ], "gpt-5.1 should expose the apply_patch tool", ); diff --git a/codex-rs/core/tests/suite/shell_serialization.rs b/codex-rs/core/tests/suite/shell_serialization.rs index 24c90bde2b..9c49e95f57 100644 --- a/codex-rs/core/tests/suite/shell_serialization.rs +++ b/codex-rs/core/tests/suite/shell_serialization.rs @@ -2,6 +2,7 @@ #![allow(clippy::expect_used)] use anyhow::Result; +use codex_core::config::Config; use codex_core::features::Feature; use codex_core::model_family::find_family_for_model; use codex_core::protocol::SandboxPolicy; @@ -40,6 +41,20 @@ const FIXTURE_JSON: &str = r#"{ } "#; +fn configure_shell_command_model(output_type: ShellModelOutput, config: &mut Config) { + if !matches!(output_type, ShellModelOutput::ShellCommand) { + return; + } + + if let Some(shell_command_family) = find_family_for_model("test-gpt-5-codex") { + if config.model_family.shell_type == shell_command_family.shell_type { + return; + } + config.model = shell_command_family.slug.clone(); + config.model_family = shell_command_family; + } +} + fn shell_responses( call_id: &str, command: Vec<&str>, @@ -112,10 +127,7 @@ async fn shell_output_stays_json_without_freeform_apply_patch( config.features.disable(Feature::ApplyPatchFreeform); config.model = "gpt-5".to_string(); config.model_family = find_family_for_model("gpt-5").expect("gpt-5 is a model family"); - if matches!(output_type, ShellModelOutput::ShellCommand) { - config.features.enable(Feature::ShellCommandTool); - } - let _ = output_type; + configure_shell_command_model(output_type, config); }); let test = builder.build(&server).await?; @@ -170,10 +182,7 @@ async fn shell_output_is_structured_with_freeform_apply_patch( let server = start_mock_server().await; let mut builder = test_codex().with_config(move |config| { config.features.enable(Feature::ApplyPatchFreeform); - if matches!(output_type, ShellModelOutput::ShellCommand) { - config.features.enable(Feature::ShellCommandTool); - } - let _ = output_type; + configure_shell_command_model(output_type, config); }); let test = builder.build(&server).await?; @@ -223,10 +232,7 @@ async fn shell_output_preserves_fixture_json_without_serialization( config.features.disable(Feature::ApplyPatchFreeform); config.model = "gpt-5".to_string(); config.model_family = find_family_for_model("gpt-5").expect("gpt-5 is a model family"); - if matches!(output_type, ShellModelOutput::ShellCommand) { - config.features.enable(Feature::ShellCommandTool); - } - let _ = output_type; + configure_shell_command_model(output_type, config); }); let test = builder.build(&server).await?; @@ -293,10 +299,7 @@ async fn shell_output_structures_fixture_with_serialization( let server = start_mock_server().await; let mut builder = test_codex().with_config(move |config| { config.features.enable(Feature::ApplyPatchFreeform); - if matches!(output_type, ShellModelOutput::ShellCommand) { - config.features.enable(Feature::ShellCommandTool); - } - let _ = output_type; + configure_shell_command_model(output_type, config); }); let test = builder.build(&server).await?; @@ -358,15 +361,12 @@ async fn shell_output_for_freeform_tool_records_duration( let server = start_mock_server().await; let mut builder = test_codex().with_config(move |config| { config.include_apply_patch_tool = true; - if matches!(output_type, ShellModelOutput::ShellCommand) { - config.features.enable(Feature::ShellCommandTool); - } - let _ = output_type; + configure_shell_command_model(output_type, config); }); let test = builder.build(&server).await?; let call_id = "shell-structured"; - let responses = shell_responses(call_id, vec!["/bin/bash", "-c", "sleep 1"], output_type)?; + let responses = shell_responses(call_id, vec!["/bin/sh", "-c", "sleep 1"], output_type)?; let mock = mount_sse_sequence(&server, responses).await; test.submit_turn_with_policy( @@ -417,10 +417,7 @@ async fn shell_output_reserializes_truncated_content(output_type: ShellModelOutp config.model_family = find_family_for_model("gpt-5.1-codex").expect("gpt-5.1-codex is a model family"); config.tool_output_token_limit = Some(200); - if matches!(output_type, ShellModelOutput::ShellCommand) { - config.features.enable(Feature::ShellCommandTool); - } - let _ = output_type; + configure_shell_command_model(output_type, config); }); let test = builder.build(&server).await?; @@ -722,9 +719,7 @@ async fn shell_output_is_structured_for_nonzero_exit(output_type: ShellModelOutp config.model_family = find_family_for_model("gpt-5.1-codex").expect("gpt-5.1-codex is a model family"); config.include_apply_patch_tool = true; - if matches!(output_type, ShellModelOutput::ShellCommand) { - config.features.enable(Feature::ShellCommandTool); - } + configure_shell_command_model(output_type, config); }); let test = builder.build(&server).await?; @@ -760,7 +755,7 @@ async fn shell_command_output_is_freeform() -> Result<()> { let server = start_mock_server().await; let mut builder = test_codex().with_config(move |config| { - config.features.enable(Feature::ShellCommandTool); + configure_shell_command_model(ShellModelOutput::ShellCommand, config); }); let test = builder.build(&server).await?; @@ -812,11 +807,7 @@ async fn shell_command_output_is_not_truncated_under_10k_bytes() -> Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; - let mut builder = test_codex() - .with_model("gpt-5.1") - .with_config(move |config| { - config.features.enable(Feature::ShellCommandTool); - }); + let mut builder = test_codex().with_model("gpt-5.1"); let test = builder.build(&server).await?; let call_id = "shell-command"; @@ -866,11 +857,7 @@ async fn shell_command_output_is_not_truncated_over_10k_bytes() -> Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; - let mut builder = test_codex() - .with_model("gpt-5.1") - .with_config(move |config| { - config.features.enable(Feature::ShellCommandTool); - }); + let mut builder = test_codex().with_model("gpt-5.1"); let test = builder.build(&server).await?; let call_id = "shell-command"; diff --git a/codex-rs/core/tests/suite/unified_exec.rs b/codex-rs/core/tests/suite/unified_exec.rs index d3059aba42..aed4cecef7 100644 --- a/codex-rs/core/tests/suite/unified_exec.rs +++ b/codex-rs/core/tests/suite/unified_exec.rs @@ -1622,8 +1622,8 @@ async fn unified_exec_formats_large_output_summary() -> Result<()> { } = builder.build(&server).await?; let script = r#"python3 - <<'PY' -for i in range(10000): - print("token token ") +import sys +sys.stdout.write("token token \n" * 5000) PY "#; 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 1f007bbe0e..f0bb707207 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 cbcd5458bc..f716f0f0f4 100644 --- a/codex-rs/exec/tests/event_processor_with_json_output.rs +++ b/codex-rs/exec/tests/event_processor_with_json_output.rs @@ -47,6 +47,7 @@ use codex_exec::exec_events::WebSearchItem; use codex_protocol::plan_tool::PlanItemArg; use codex_protocol::plan_tool::StepStatus; use codex_protocol::plan_tool::UpdatePlanArgs; +use codex_protocol::protocol::CodexErrorInfo; use mcp_types::CallToolResult; use mcp_types::ContentBlock; use mcp_types::TextContent; @@ -539,6 +540,7 @@ fn error_event_produces_error() { "e1", EventMsg::Error(codex_core::protocol::ErrorEvent { message: "boom".to_string(), + codex_error_code: Some(CodexErrorInfo::Other), }), )); assert_eq!( @@ -578,6 +580,7 @@ fn stream_error_event_produces_error() { "e1", EventMsg::StreamError(codex_core::protocol::StreamErrorEvent { message: "retrying".to_string(), + codex_error_code: Some(CodexErrorInfo::Other), }), )); assert_eq!( @@ -596,6 +599,7 @@ fn error_followed_by_task_complete_produces_turn_failed() { "e1", EventMsg::Error(ErrorEvent { message: "boom".to_string(), + codex_error_code: Some(CodexErrorInfo::Other), }), ); assert_eq!( diff --git a/codex-rs/execpolicy/README.md b/codex-rs/execpolicy/README.md index f508c8ca64..9fd9c63306 100644 --- a/codex-rs/execpolicy/README.md +++ b/codex-rs/execpolicy/README.md @@ -20,18 +20,18 @@ prefix_rule( ``` ## CLI -- Provide one or more policy files (for example `src/default.codexpolicy`) to check a command: +- From the Codex CLI, run `codex execpolicy check` subcommand with one or more policy files (for example `src/default.codexpolicy`) to check a command: +```bash +codex execpolicy check --policy path/to/policy.codexpolicy git status +``` +- Pass multiple `--policy` flags to merge rules, evaluated in the order provided, and use `--pretty` for formatted JSON. +- You can also run the standalone dev binary directly during development: ```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"` + - No match: `{"noMatch": {}}` ## Response shapes - Match: @@ -53,8 +53,10 @@ cargo run -p codex-execpolicy -- check --policy base.codexpolicy --policy overri - No match: ```json -"noMatch" +{"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`). + +Note: `execpolicy` commands are still in preview. The API may have breaking changes in the future. diff --git a/codex-rs/execpolicy/src/execpolicycheck.rs b/codex-rs/execpolicy/src/execpolicycheck.rs new file mode 100644 index 0000000000..0b5e0dcafc --- /dev/null +++ b/codex-rs/execpolicy/src/execpolicycheck.rs @@ -0,0 +1,67 @@ +use std::fs; +use std::path::PathBuf; + +use anyhow::Context; +use anyhow::Result; +use clap::Parser; + +use crate::Evaluation; +use crate::Policy; +use crate::PolicyParser; + +/// Arguments for evaluating a command against one or more execpolicy files. +#[derive(Debug, Parser, Clone)] +pub struct ExecPolicyCheckCommand { + /// Paths to execpolicy files to evaluate (repeatable). + #[arg(short = 'p', long = "policy", value_name = "PATH", required = true)] + pub policies: Vec, + + /// Pretty-print the JSON output. + #[arg(long)] + pub pretty: bool, + + /// Command tokens to check against the policy. + #[arg( + value_name = "COMMAND", + required = true, + trailing_var_arg = true, + allow_hyphen_values = true + )] + pub command: Vec, +} + +impl ExecPolicyCheckCommand { + /// Load the policies for this command, evaluate the command, and render JSON output. + pub fn run(&self) -> Result<()> { + let policy = load_policies(&self.policies)?; + let evaluation = policy.check(&self.command); + + let json = format_evaluation_json(&evaluation, self.pretty)?; + println!("{json}"); + + Ok(()) + } +} + +pub fn format_evaluation_json(evaluation: &Evaluation, pretty: bool) -> Result { + if pretty { + serde_json::to_string_pretty(evaluation).map_err(Into::into) + } else { + serde_json::to_string(evaluation).map_err(Into::into) + } +} + +pub 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) + .with_context(|| format!("failed to parse policy at {}", policy_path.display()))?; + } + + Ok(parser.build()) +} diff --git a/codex-rs/execpolicy/src/lib.rs b/codex-rs/execpolicy/src/lib.rs index 1b789fd862..b459caea16 100644 --- a/codex-rs/execpolicy/src/lib.rs +++ b/codex-rs/execpolicy/src/lib.rs @@ -1,5 +1,6 @@ pub mod decision; pub mod error; +pub mod execpolicycheck; pub mod parser; pub mod policy; pub mod rule; @@ -7,6 +8,7 @@ pub mod rule; pub use decision::Decision; pub use error::Error; pub use error::Result; +pub use execpolicycheck::ExecPolicyCheckCommand; pub use parser::PolicyParser; pub use policy::Evaluation; pub use policy::Policy; diff --git a/codex-rs/execpolicy/src/main.rs b/codex-rs/execpolicy/src/main.rs index 318abb5cae..e1373b6d16 100644 --- a/codex-rs/execpolicy/src/main.rs +++ b/codex-rs/execpolicy/src/main.rs @@ -1,66 +1,22 @@ -use std::fs; -use std::path::PathBuf; - -use anyhow::Context; use anyhow::Result; use clap::Parser; -use codex_execpolicy::PolicyParser; +use codex_execpolicy::ExecPolicyCheckCommand; /// CLI for evaluating exec policies #[derive(Parser)] #[command(name = "codex-execpolicy")] 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, - }, + Check(ExecPolicyCheckCommand), } fn main() -> Result<()> { let cli = Cli::parse(); match cli { - Cli::Check { - policies, - command, - pretty, - } => cmd_check(policies, command, pretty), + Cli::Check(cmd) => cmd_check(cmd), } } -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()) +fn cmd_check(cmd: ExecPolicyCheckCommand) -> Result<()> { + cmd.run() } diff --git a/codex-rs/execpolicy/src/policy.rs b/codex-rs/execpolicy/src/policy.rs index a543e5c4b0..e048fce1ff 100644 --- a/codex-rs/execpolicy/src/policy.rs +++ b/codex-rs/execpolicy/src/policy.rs @@ -27,9 +27,9 @@ impl Policy { 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 {}, }, - None => return Evaluation::NoMatch, + None => return Evaluation::NoMatch {}, }; let matched_rules: Vec = @@ -39,7 +39,7 @@ impl Policy { decision, matched_rules, }, - None => Evaluation::NoMatch, + None => Evaluation::NoMatch {}, } } @@ -52,7 +52,7 @@ impl Policy { .into_iter() .flat_map(|command| match self.check(command.as_ref()) { Evaluation::Match { matched_rules, .. } => matched_rules, - Evaluation::NoMatch => Vec::new(), + Evaluation::NoMatch { .. } => Vec::new(), }) .collect(); @@ -61,7 +61,7 @@ impl Policy { decision, matched_rules, }, - None => Evaluation::NoMatch, + None => Evaluation::NoMatch {}, } } } @@ -69,7 +69,7 @@ impl Policy { #[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub enum Evaluation { - NoMatch, + NoMatch {}, Match { decision: Decision, #[serde(rename = "matchedRules")] diff --git a/codex-rs/execpolicy/tests/basic.rs b/codex-rs/execpolicy/tests/basic.rs index 301f962616..e4189caa21 100644 --- a/codex-rs/execpolicy/tests/basic.rs +++ b/codex-rs/execpolicy/tests/basic.rs @@ -288,7 +288,7 @@ prefix_rule( "color.status=always", "status", ])); - assert_eq!(Evaluation::NoMatch, no_match_eval); + assert_eq!(Evaluation::NoMatch {}, no_match_eval); } #[test] diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index f20e412831..5806e1a6f0 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -562,6 +562,35 @@ pub enum EventMsg { ReasoningRawContentDelta(ReasoningRawContentDeltaEvent), } +/// Codex errors that we expose to clients. +#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "snake_case")] +#[ts(rename_all = "snake_case")] +pub enum CodexErrorInfo { + ContextWindowExceeded, + UsageLimitExceeded, + HttpConnectionFailed { + http_status_code: Option, + }, + /// Failed to connect to the response SSE stream. + ResponseStreamConnectionFailed { + http_status_code: Option, + }, + InternalServerError, + Unauthorized, + BadRequest, + SandboxError, + /// The response SSE stream disconnected in the middle of a turnbefore completion. + ResponseStreamDisconnected { + http_status_code: Option, + }, + /// Reached the retry limit for responses. + ResponseTooManyFailedAttempts { + http_status_code: Option, + }, + Other, +} + #[derive(Debug, Clone, Deserialize, Serialize, TS, JsonSchema)] pub struct RawResponseItemEvent { pub item: ResponseItem, @@ -686,6 +715,8 @@ pub struct ExitedReviewModeEvent { #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] pub struct ErrorEvent { pub message: String, + #[serde(default)] + pub codex_error_code: Option, } #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] @@ -1363,6 +1394,8 @@ pub struct UndoCompletedEvent { #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] pub struct StreamErrorEvent { pub message: String, + #[serde(default)] + pub codex_error_code: Option, } #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index b8b154815a..ee3c09ffab 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -343,7 +343,8 @@ impl App { let env_map: std::collections::HashMap = std::env::vars().collect(); let tx = app.app_event_tx.clone(); let logs_base_dir = app.config.codex_home.clone(); - Self::spawn_world_writable_scan(cwd, env_map, logs_base_dir, tx); + let sandbox_policy = app.config.sandbox_policy.clone(); + Self::spawn_world_writable_scan(cwd, env_map, logs_base_dir, sandbox_policy, tx); } } @@ -717,7 +718,14 @@ impl App { std::env::vars().collect(); let tx = self.app_event_tx.clone(); let logs_base_dir = self.config.codex_home.clone(); - Self::spawn_world_writable_scan(cwd, env_map, logs_base_dir, tx); + let sandbox_policy = self.config.sandbox_policy.clone(); + Self::spawn_world_writable_scan( + cwd, + env_map, + logs_base_dir, + sandbox_policy, + tx, + ); } } } @@ -911,6 +919,7 @@ impl App { cwd: PathBuf, env_map: std::collections::HashMap, logs_base_dir: PathBuf, + sandbox_policy: codex_core::protocol::SandboxPolicy, tx: AppEventSender, ) { #[inline] @@ -920,8 +929,10 @@ impl App { } tokio::task::spawn_blocking(move || { let result = codex_windows_sandbox::preflight_audit_everyone_writable( + &logs_base_dir, &cwd, &env_map, + &sandbox_policy, Some(logs_base_dir.as_path()), ); if let Ok(ref paths) = result diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 7dca3814a3..4144ad4c3e 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1664,7 +1664,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 { @@ -1707,7 +1707,9 @@ 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 3c326fed86..240972347f 100644 --- a/codex-rs/tui/src/chatwidget/agent.rs +++ b/codex-rs/tui/src/chatwidget/agent.rs @@ -4,7 +4,6 @@ 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; @@ -37,7 +36,7 @@ pub(crate) fn spawn_agent( eprintln!("{message}"); app_event_tx_clone.send(AppEvent::CodexEvent(Event { id: "".to_string(), - msg: EventMsg::Error(ErrorEvent { message }), + msg: EventMsg::Error(err.to_error_event(None)), })); 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 6d3426cbea..45a9a8df21 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -50,6 +50,7 @@ use codex_protocol::parse_command::ParsedCommand; use codex_protocol::plan_tool::PlanItemArg; use codex_protocol::plan_tool::StepStatus; use codex_protocol::plan_tool::UpdatePlanArgs; +use codex_protocol::protocol::CodexErrorInfo; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use crossterm::event::KeyModifiers; @@ -2647,6 +2648,7 @@ fn stream_error_updates_status_indicator() { id: "sub-1".into(), msg: EventMsg::StreamError(StreamErrorEvent { message: msg.to_string(), + codex_error_code: Some(CodexErrorInfo::Other), }), }); diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs index 1ec75e6726..f2e1e09480 100644 --- a/codex-rs/windows-sandbox-rs/src/acl.rs +++ b/codex-rs/windows-sandbox-rs/src/acl.rs @@ -31,11 +31,17 @@ use windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_NORMAL; 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_APPEND_DATA; +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::FILE_SHARE_READ; use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_WRITE; use windows_sys::Win32::Storage::FileSystem::OPEN_EXISTING; const SE_KERNEL_OBJECT: u32 = 6; const INHERIT_ONLY_ACE: u8 = 0x08; +const GENERIC_WRITE_MASK: u32 = 0x4000_0000; +const DENY_ACCESS: i32 = 3; pub unsafe fn dacl_has_write_allow_for_sid(p_dacl: *mut ACL, psid: *mut c_void) -> bool { if p_dacl.is_null() { @@ -78,6 +84,49 @@ pub unsafe fn dacl_has_write_allow_for_sid(p_dacl: *mut ACL, psid: *mut c_void) false } +pub unsafe fn dacl_has_write_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) -> bool { + if p_dacl.is_null() { + return false; + } + let mut info: ACL_SIZE_INFORMATION = std::mem::zeroed(); + let ok = GetAclInformation( + p_dacl as *const ACL, + &mut info as *mut _ as *mut c_void, + std::mem::size_of::() as u32, + AclSizeInformation, + ); + if ok == 0 { + return false; + } + let deny_write_mask = FILE_GENERIC_WRITE + | FILE_WRITE_DATA + | FILE_APPEND_DATA + | FILE_WRITE_EA + | FILE_WRITE_ATTRIBUTES + | GENERIC_WRITE_MASK; + for i in 0..info.AceCount { + let mut p_ace: *mut c_void = std::ptr::null_mut(); + if GetAce(p_dacl as *const ACL, i, &mut p_ace) == 0 { + continue; + } + let hdr = &*(p_ace as *const ACE_HEADER); + if hdr.AceType != 1 { + continue; // ACCESS_DENIED_ACE_TYPE + } + if (hdr.AceFlags & INHERIT_ONLY_ACE) != 0 { + continue; + } + let ace = &*(p_ace as *const ACCESS_ALLOWED_ACE); + let base = p_ace as usize; + let sid_ptr = + (base + std::mem::size_of::() + std::mem::size_of::()) as *mut c_void; + if EqualSid(sid_ptr, psid) != 0 && (ace.Mask & deny_write_mask) != 0 { + return true; + } + } + false +} + // Compute effective rights for a trustee SID against a DACL and decide if write is effectively allowed. // This accounts for deny ACEs and ordering; falls back to a conservative per-ACE scan if the API fails. #[allow(dead_code)] @@ -168,6 +217,67 @@ pub unsafe fn add_allow_ace(path: &Path, psid: *mut c_void) -> Result { Ok(added) } +pub unsafe fn add_deny_write_ace(path: &Path, psid: *mut c_void) -> Result { + let mut p_sd: *mut c_void = std::ptr::null_mut(); + let mut p_dacl: *mut ACL = std::ptr::null_mut(); + let code = GetNamedSecurityInfoW( + to_wide(path).as_ptr(), + 1, + DACL_SECURITY_INFORMATION, + std::ptr::null_mut(), + std::ptr::null_mut(), + &mut p_dacl, + std::ptr::null_mut(), + &mut p_sd, + ); + if code != ERROR_SUCCESS { + return Err(anyhow!("GetNamedSecurityInfoW failed: {}", code)); + } + let mut added = false; + if !dacl_has_write_deny_for_sid(p_dacl, psid) { + let trustee = TRUSTEE_W { + pMultipleTrustee: std::ptr::null_mut(), + MultipleTrusteeOperation: 0, + TrusteeForm: TRUSTEE_IS_SID, + TrusteeType: TRUSTEE_IS_UNKNOWN, + ptstrName: psid as *mut u16, + }; + let mut explicit: EXPLICIT_ACCESS_W = std::mem::zeroed(); + explicit.grfAccessPermissions = FILE_GENERIC_WRITE + | FILE_WRITE_DATA + | FILE_APPEND_DATA + | FILE_WRITE_EA + | FILE_WRITE_ATTRIBUTES + | GENERIC_WRITE_MASK; + explicit.grfAccessMode = DENY_ACCESS; + explicit.grfInheritance = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE; + explicit.Trustee = trustee; + let mut p_new_dacl: *mut ACL = std::ptr::null_mut(); + let code2 = SetEntriesInAclW(1, &explicit, p_dacl, &mut p_new_dacl); + if code2 == ERROR_SUCCESS { + let code3 = SetNamedSecurityInfoW( + to_wide(path).as_ptr() as *mut u16, + 1, + DACL_SECURITY_INFORMATION, + std::ptr::null_mut(), + std::ptr::null_mut(), + p_new_dacl, + std::ptr::null_mut(), + ); + if code3 == ERROR_SUCCESS { + added = true; + } + if !p_new_dacl.is_null() { + LocalFree(p_new_dacl as HLOCAL); + } + } + } + if !p_sd.is_null() { + LocalFree(p_sd as HLOCAL); + } + Ok(added) +} + pub unsafe fn revoke_ace(path: &Path, psid: *mut c_void) { let mut p_sd: *mut c_void = std::ptr::null_mut(); let mut p_dacl: *mut ACL = std::ptr::null_mut(); diff --git a/codex-rs/windows-sandbox-rs/src/audit.rs b/codex-rs/windows-sandbox-rs/src/audit.rs index cba8d06335..210fd58ee3 100644 --- a/codex-rs/windows-sandbox-rs/src/audit.rs +++ b/codex-rs/windows-sandbox-rs/src/audit.rs @@ -1,4 +1,11 @@ +use crate::acl::add_deny_write_ace; +use crate::cap::cap_sid_file; +use crate::cap::load_or_create_cap_sids; +use crate::logging::log_note; +use crate::policy::SandboxPolicy; +use crate::token::convert_string_sid_to_sid; use crate::token::world_sid; +use anyhow::anyhow; use crate::winutil::to_wide; use anyhow::Result; use std::collections::HashMap; @@ -271,6 +278,13 @@ pub fn audit_everyone_writable( for p in &flagged { list.push_str(&format!("\n - {}", p.display())); } + crate::logging::log_note( + &format!( + "AUDIT: world-writable scan FAILED; cwd={cwd:?}; checked={checked}; duration_ms={elapsed_ms}; flagged:{}", + list + ), + logs_base_dir, + ); return Ok(flagged); } @@ -282,6 +296,70 @@ pub fn audit_everyone_writable( Ok(Vec::new()) } +pub fn apply_capability_denies_for_world_writable( + codex_home: &Path, + flagged: &[PathBuf], + sandbox_policy: &SandboxPolicy, + cwd: &Path, + logs_base_dir: Option<&Path>, +) -> Result<()> { + if flagged.is_empty() { + return Ok(()); + } + std::fs::create_dir_all(codex_home)?; + let cap_path = cap_sid_file(codex_home); + let caps = load_or_create_cap_sids(codex_home); + std::fs::write(&cap_path, serde_json::to_string(&caps)?)?; + let (active_sid, workspace_roots): (*mut c_void, Vec) = match sandbox_policy { + SandboxPolicy::WorkspaceWrite { writable_roots, .. } => { + let sid = unsafe { convert_string_sid_to_sid(&caps.workspace) } + .ok_or_else(|| anyhow!("ConvertStringSidToSidW failed for workspace capability"))?; + let mut roots: Vec = + vec![dunce::canonicalize(cwd).unwrap_or_else(|_| cwd.to_path_buf())]; + for root in writable_roots { + let candidate = if root.is_absolute() { + root.clone() + } else { + cwd.join(root) + }; + roots.push(dunce::canonicalize(&candidate).unwrap_or(candidate)); + } + (sid, roots) + } + SandboxPolicy::ReadOnly => ( + unsafe { convert_string_sid_to_sid(&caps.readonly) }.ok_or_else(|| { + anyhow!("ConvertStringSidToSidW failed for readonly capability") + })?, + Vec::new(), + ), + SandboxPolicy::DangerFullAccess => { + return Ok(()); + } + }; + for path in flagged { + if workspace_roots.iter().any(|root| path.starts_with(root)) { + continue; + } + let res = unsafe { add_deny_write_ace(path, active_sid) }; + match res { + Ok(true) => log_note( + &format!("AUDIT: applied capability deny ACE to {}", path.display()), + logs_base_dir, + ), + Ok(false) => {} + Err(err) => log_note( + &format!( + "AUDIT: failed to apply capability deny ACE to {}: {}", + path.display(), + err + ), + logs_base_dir, + ), + } + } + Ok(()) +} + fn normalize_windows_path_for_display(p: impl AsRef) -> String { let canon = dunce::canonicalize(p.as_ref()).unwrap_or_else(|_| p.as_ref().to_path_buf()); canon.display().to_string().replace('/', "\\") diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index a2b4b6a538..1720294f2e 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -38,6 +38,7 @@ mod windows_impl { use super::env::normalize_null_device_env; use super::logging::debug_log; use super::logging::log_failure; + use super::logging::log_note; use super::logging::log_start; use super::logging::log_success; use super::policy::parse_policy; @@ -178,11 +179,29 @@ mod windows_impl { } pub fn preflight_audit_everyone_writable( + codex_home: &Path, cwd: &Path, env_map: &HashMap, + sandbox_policy: &SandboxPolicy, logs_base_dir: Option<&Path>, ) -> Result> { - audit::audit_everyone_writable(cwd, env_map, logs_base_dir) + let flagged = audit::audit_everyone_writable(cwd, env_map, logs_base_dir)?; + if flagged.is_empty() { + return Ok(flagged); + } + if let Err(err) = audit::apply_capability_denies_for_world_writable( + codex_home, + &flagged, + sandbox_policy, + cwd, + logs_base_dir, + ) { + log_note( + &format!("AUDIT: failed to apply capability deny ACEs: {}", err), + logs_base_dir, + ); + } + Ok(Vec::new()) } pub fn run_windows_sandbox_capture( @@ -434,6 +453,7 @@ mod windows_impl { mod stub { use anyhow::bail; use anyhow::Result; + use codex_protocol::protocol::SandboxPolicy; use std::collections::HashMap; use std::path::Path; @@ -446,8 +466,10 @@ mod stub { } pub fn preflight_audit_everyone_writable( + _codex_home: &Path, _cwd: &Path, _env_map: &HashMap, + _sandbox_policy: &SandboxPolicy, _logs_base_dir: Option<&Path>, ) -> Result> { bail!("Windows sandbox is only available on Windows")