diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index f05879a243..132aaef41c 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1470,6 +1470,7 @@ dependencies = [ "codex-arg0", "codex-chatgpt", "codex-cloud-tasks", + "codex-config", "codex-core", "codex-exec", "codex-execpolicy", @@ -1600,10 +1601,13 @@ dependencies = [ "pretty_assertions", "serde", "serde_json", + "serde_path_to_error", "sha2", "thiserror 2.0.18", "tokio", "toml 0.9.11+spec-1.1.0", + "toml_edit 0.24.0+spec-1.1.0", + "tracing", ] [[package]] diff --git a/codex-rs/app-server/tests/suite/v2/turn_start.rs b/codex-rs/app-server/tests/suite/v2/turn_start.rs index d2d850176d..8d6db34731 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs @@ -847,7 +847,9 @@ async fn turn_start_exec_approval_toggle_v2() -> Result<()> { // Approve and wait for task completion mcp.send_response( request_id, - serde_json::json!({ "decision": codex_core::protocol::ReviewDecision::Approved }), + serde_json::to_value(CommandExecutionRequestApprovalResponse { + decision: CommandExecutionApprovalDecision::Accept, + })?, ) .await?; timeout( @@ -1035,7 +1037,7 @@ async fn turn_start_exec_approval_decline_v2() -> Result<()> { timeout( DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_notification_message("codex/event/task_complete"), + mcp.read_stream_until_notification_message("turn/completed"), ) .await??; @@ -1129,7 +1131,7 @@ async fn turn_start_updates_sandbox_and_cwd_between_turns_v2() -> Result<()> { .await??; timeout( DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_notification_message("codex/event/task_complete"), + mcp.read_stream_until_notification_message("turn/completed"), ) .await??; mcp.clear_message_buffer(); diff --git a/codex-rs/cli/Cargo.toml b/codex-rs/cli/Cargo.toml index fd15f87916..1d955bbaaf 100644 --- a/codex-rs/cli/Cargo.toml +++ b/codex-rs/cli/Cargo.toml @@ -26,6 +26,7 @@ codex-arg0 = { workspace = true } codex-chatgpt = { workspace = true } codex-cloud-tasks = { path = "../cloud-tasks" } codex-utils-cli = { workspace = true } +codex-config = { workspace = true } codex-core = { workspace = true } codex-exec = { workspace = true } codex-execpolicy = { workspace = true } diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index 33c9f161bb..98a5ab0779 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -870,7 +870,7 @@ fn maybe_print_under_development_feature_warning( return; } - let config_path = codex_home.join(codex_core::config::CONFIG_TOML_FILE); + let config_path = codex_home.join(codex_config::CONFIG_TOML_FILE); eprintln!( "Under-development features enabled: {feature}. Under-development features are incomplete and may behave unpredictably. To suppress this warning, set `suppress_unstable_features_warning = true` in {}.", config_path.display() diff --git a/codex-rs/config/Cargo.toml b/codex-rs/config/Cargo.toml index 1c804fb8db..02446ed6f3 100644 --- a/codex-rs/config/Cargo.toml +++ b/codex-rs/config/Cargo.toml @@ -16,9 +16,13 @@ futures = { workspace = true, features = ["alloc", "std"] } multimap = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } +serde_path_to_error = { workspace = true } sha2 = { workspace = true } thiserror = { workspace = true } +tokio = { workspace = true, features = ["fs"] } toml = { workspace = true } +toml_edit = { workspace = true } +tracing = { workspace = true } [dev-dependencies] anyhow = { workspace = true } diff --git a/codex-rs/core/src/config_loader/diagnostics.rs b/codex-rs/config/src/diagnostics.rs similarity index 90% rename from codex-rs/core/src/config_loader/diagnostics.rs rename to codex-rs/config/src/diagnostics.rs index 64f9c83888..be6e123d33 100644 --- a/codex-rs/core/src/config_loader/diagnostics.rs +++ b/codex-rs/config/src/diagnostics.rs @@ -1,10 +1,12 @@ //! Helpers for mapping config parse/validation failures to file locations and //! rendering them in a user-friendly way. -use crate::config::CONFIG_TOML_FILE; -use crate::config::ConfigToml; +use crate::ConfigLayerEntry; +use crate::ConfigLayerStack; +use crate::ConfigLayerStackOrdering; use codex_app_server_protocol::ConfigLayerSource; use codex_utils_absolute_path::AbsolutePathBufGuard; +use serde::de::DeserializeOwned; use serde_path_to_error::Path as SerdePath; use serde_path_to_error::Segment as SerdeSegment; use std::fmt; @@ -17,10 +19,6 @@ use toml_edit::Item; use toml_edit::Table; use toml_edit::Value; -use super::ConfigLayerEntry; -use super::ConfigLayerStack; -use super::ConfigLayerStackOrdering; - #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct TextPosition { pub line: usize, @@ -88,7 +86,7 @@ impl std::error::Error for ConfigLoadError { } } -pub(crate) fn io_error_from_config_error( +pub fn io_error_from_config_error( kind: io::ErrorKind, error: ConfigError, source: Option, @@ -96,7 +94,7 @@ pub(crate) fn io_error_from_config_error( io::Error::new(kind, ConfigLoadError::new(error, source)) } -pub(crate) fn config_error_from_toml( +pub fn config_error_from_toml( path: impl AsRef, contents: &str, err: toml::de::Error, @@ -108,7 +106,7 @@ pub(crate) fn config_error_from_toml( ConfigError::new(path.as_ref().to_path_buf(), range, err.message()) } -pub(crate) fn config_error_from_config_toml( +pub fn config_error_from_typed_toml( path: impl AsRef, contents: &str, ) -> Option { @@ -117,7 +115,7 @@ pub(crate) fn config_error_from_config_toml( Err(err) => return Some(config_error_from_toml(path, contents, err)), }; - let result: Result = serde_path_to_error::deserialize(deserializer); + let result: Result = serde_path_to_error::deserialize(deserializer); match result { Ok(_) => None, Err(err) => { @@ -136,28 +134,36 @@ pub(crate) fn config_error_from_config_toml( } } -pub(crate) async fn first_layer_config_error(layers: &ConfigLayerStack) -> Option { +pub async fn first_layer_config_error( + layers: &ConfigLayerStack, + config_toml_file: &str, +) -> Option { // When the merged config fails schema validation, we surface the first concrete // per-file error to point users at a specific file and range rather than an // opaque merged-layer failure. - first_layer_config_error_for_entries( + first_layer_config_error_for_entries::( layers.get_layers(ConfigLayerStackOrdering::LowestPrecedenceFirst, false), + config_toml_file, ) .await } -pub(crate) async fn first_layer_config_error_from_entries( +pub async fn first_layer_config_error_from_entries( layers: &[ConfigLayerEntry], + config_toml_file: &str, ) -> Option { - first_layer_config_error_for_entries(layers.iter()).await + first_layer_config_error_for_entries::(layers.iter(), config_toml_file).await } -async fn first_layer_config_error_for_entries<'a, I>(layers: I) -> Option +async fn first_layer_config_error_for_entries<'a, T: DeserializeOwned, I>( + layers: I, + config_toml_file: &str, +) -> Option where I: IntoIterator, { for layer in layers { - let Some(path) = config_path_for_layer(layer) else { + let Some(path) = config_path_for_layer(layer, config_toml_file) else { continue; }; let contents = match tokio::fs::read_to_string(&path).await { @@ -174,7 +180,7 @@ where continue; }; let _guard = AbsolutePathBufGuard::new(parent); - if let Some(error) = config_error_from_config_toml(&path, &contents) { + if let Some(error) = config_error_from_typed_toml::(&path, &contents) { return Some(error); } } @@ -182,12 +188,12 @@ where None } -fn config_path_for_layer(layer: &ConfigLayerEntry) -> Option { +fn config_path_for_layer(layer: &ConfigLayerEntry, config_toml_file: &str) -> Option { match &layer.name { ConfigLayerSource::System { file } => Some(file.to_path_buf()), ConfigLayerSource::User { file } => Some(file.to_path_buf()), ConfigLayerSource::Project { dot_codex_folder } => { - Some(dot_codex_folder.as_path().join(CONFIG_TOML_FILE)) + Some(dot_codex_folder.as_path().join(config_toml_file)) } ConfigLayerSource::LegacyManagedConfigTomlFromFile { file } => Some(file.to_path_buf()), ConfigLayerSource::Mdm { .. } diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index 42f3ad76af..41b9a3ae06 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -1,12 +1,15 @@ mod cloud_requirements; mod config_requirements; mod constraint; +mod diagnostics; mod fingerprint; mod merge; mod overrides; mod requirements_exec_policy; mod state; +pub const CONFIG_TOML_FILE: &str = "config.toml"; + pub use cloud_requirements::CloudRequirementsLoader; pub use config_requirements::ConfigRequirements; pub use config_requirements::ConfigRequirementsToml; @@ -24,6 +27,17 @@ pub use config_requirements::WebSearchModeRequirement; pub use constraint::Constrained; pub use constraint::ConstraintError; pub use constraint::ConstraintResult; +pub use diagnostics::ConfigError; +pub use diagnostics::ConfigLoadError; +pub use diagnostics::TextPosition; +pub use diagnostics::TextRange; +pub use diagnostics::config_error_from_toml; +pub use diagnostics::config_error_from_typed_toml; +pub use diagnostics::first_layer_config_error; +pub use diagnostics::first_layer_config_error_from_entries; +pub use diagnostics::format_config_error; +pub use diagnostics::format_config_error_with_source; +pub use diagnostics::io_error_from_config_error; pub use fingerprint::version_for_toml; pub use merge::merge_toml_values; pub use overrides::build_cli_overrides_layer; diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 88e6fef893..6017db9a75 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -119,7 +119,6 @@ use crate::client_common::Prompt; use crate::client_common::ResponseEvent; use crate::codex_thread::ThreadConfigSnapshot; use crate::compact::collect_user_messages; -use crate::config::CONFIG_TOML_FILE; use crate::config::Config; use crate::config::Constrained; use crate::config::ConstraintResult; @@ -135,6 +134,7 @@ use crate::error::CodexErr; use crate::error::Result as CodexResult; #[cfg(test)] use crate::exec::StreamOutput; +use codex_config::CONFIG_TOML_FILE; #[derive(Debug, PartialEq)] pub enum SteerInputError { diff --git a/codex-rs/core/src/config/edit.rs b/codex-rs/core/src/config/edit.rs index 07443fa665..0dfb4efd8e 100644 --- a/codex-rs/core/src/config/edit.rs +++ b/codex-rs/core/src/config/edit.rs @@ -1,9 +1,9 @@ -use crate::config::CONFIG_TOML_FILE; use crate::config::types::McpServerConfig; use crate::config::types::Notice; use crate::path_utils::resolve_symlink_write_paths; use crate::path_utils::write_atomically; use anyhow::Context; +use codex_config::CONFIG_TOML_FILE; use codex_protocol::config_types::Personality; use codex_protocol::config_types::TrustLevel; use codex_protocol::openai_models::ReasoningEffort; diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index edaabed1fa..ec3d68b15f 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -117,8 +117,6 @@ pub(crate) const PROJECT_DOC_MAX_BYTES: usize = 32 * 1024; // 32 KiB pub(crate) const DEFAULT_AGENT_MAX_THREADS: Option = Some(6); pub(crate) const DEFAULT_AGENT_MAX_DEPTH: i32 = 1; -pub const CONFIG_TOML_FILE: &str = "config.toml"; - #[cfg(test)] pub(crate) fn test_config() -> Config { let codex_home = tempdir().expect("create temp dir"); diff --git a/codex-rs/core/src/config/service.rs b/codex-rs/core/src/config/service.rs index 1033db728d..da675d2389 100644 --- a/codex-rs/core/src/config/service.rs +++ b/codex-rs/core/src/config/service.rs @@ -1,4 +1,3 @@ -use super::CONFIG_TOML_FILE; use super::ConfigToml; use crate::config::edit::ConfigEdit; use crate::config::edit::ConfigEditsBuilder; @@ -26,6 +25,7 @@ use codex_app_server_protocol::ConfigWriteResponse; use codex_app_server_protocol::MergeStrategy; use codex_app_server_protocol::OverriddenMetadata; use codex_app_server_protocol::WriteStatus; +use codex_config::CONFIG_TOML_FILE; use codex_utils_absolute_path::AbsolutePathBuf; use serde_json::Value as JsonValue; use std::borrow::Cow; diff --git a/codex-rs/core/src/config_loader/layer_io.rs b/codex-rs/core/src/config_loader/layer_io.rs index a97f285549..5caebdf527 100644 --- a/codex-rs/core/src/config_loader/layer_io.rs +++ b/codex-rs/core/src/config_loader/layer_io.rs @@ -1,10 +1,10 @@ use super::LoaderOverrides; -use super::diagnostics::config_error_from_toml; -use super::diagnostics::io_error_from_config_error; #[cfg(target_os = "macos")] use super::macos::ManagedAdminConfigLayer; #[cfg(target_os = "macos")] use super::macos::load_managed_admin_config_layer; +use codex_config::config_error_from_toml; +use codex_config::io_error_from_config_error; use codex_utils_absolute_path::AbsolutePathBuf; use std::io; use std::path::Path; diff --git a/codex-rs/core/src/config_loader/mod.rs b/codex-rs/core/src/config_loader/mod.rs index f30a627c40..70014c03b8 100644 --- a/codex-rs/core/src/config_loader/mod.rs +++ b/codex-rs/core/src/config_loader/mod.rs @@ -1,4 +1,3 @@ -mod diagnostics; mod layer_io; #[cfg(target_os = "macos")] mod macos; @@ -6,12 +5,12 @@ mod macos; #[cfg(test)] mod tests; -use crate::config::CONFIG_TOML_FILE; use crate::config::ConfigToml; use crate::config::deserialize_config_toml_with_base; use crate::config_loader::layer_io::LoadedConfigLayers; use crate::git_info::resolve_root_git_project_for_trust; use codex_app_server_protocol::ConfigLayerSource; +use codex_config::CONFIG_TOML_FILE; use codex_config::ConfigRequirementsWithSources; use codex_protocol::config_types::SandboxMode; use codex_protocol::config_types::TrustLevel; @@ -27,9 +26,11 @@ use std::path::PathBuf; use toml::Value as TomlValue; pub use codex_config::CloudRequirementsLoader; +pub use codex_config::ConfigError; pub use codex_config::ConfigLayerEntry; pub use codex_config::ConfigLayerStack; pub use codex_config::ConfigLayerStackOrdering; +pub use codex_config::ConfigLoadError; pub use codex_config::ConfigRequirements; pub use codex_config::ConfigRequirementsToml; pub use codex_config::ConstrainedWithSource; @@ -42,21 +43,17 @@ pub use codex_config::RequirementSource; pub use codex_config::ResidencyRequirement; pub use codex_config::SandboxModeRequirement; pub use codex_config::Sourced; +pub use codex_config::TextPosition; +pub use codex_config::TextRange; pub use codex_config::WebSearchModeRequirement; pub(crate) use codex_config::build_cli_overrides_layer; +pub(crate) use codex_config::config_error_from_toml; +pub use codex_config::format_config_error; +pub use codex_config::format_config_error_with_source; +pub(crate) use codex_config::io_error_from_config_error; pub use codex_config::merge_toml_values; #[cfg(test)] pub(crate) use codex_config::version_for_toml; -pub use diagnostics::ConfigError; -pub use diagnostics::ConfigLoadError; -pub use diagnostics::TextPosition; -pub use diagnostics::TextRange; -pub(crate) use diagnostics::config_error_from_toml; -pub(crate) use diagnostics::first_layer_config_error; -pub(crate) use diagnostics::first_layer_config_error_from_entries; -pub use diagnostics::format_config_error; -pub use diagnostics::format_config_error_with_source; -pub(crate) use diagnostics::io_error_from_config_error; /// On Unix systems, load default settings from this file path, if present. /// Note that /etc/codex/ is treated as a "config folder," so subfolders such @@ -68,6 +65,17 @@ const DEFAULT_PROGRAM_DATA_DIR_WINDOWS: &str = r"C:\ProgramData"; const DEFAULT_PROJECT_ROOT_MARKERS: &[&str] = &[".git"]; +pub(crate) async fn first_layer_config_error(layers: &ConfigLayerStack) -> Option { + codex_config::first_layer_config_error::(layers, CONFIG_TOML_FILE).await +} + +pub(crate) async fn first_layer_config_error_from_entries( + layers: &[ConfigLayerEntry], +) -> Option { + codex_config::first_layer_config_error_from_entries::(layers, CONFIG_TOML_FILE) + .await +} + /// To build up the set of admin-enforced constraints, we build up from multiple /// configuration layers in the following order, but a constraint defined in an /// earlier layer cannot be overridden by a later layer: diff --git a/codex-rs/core/src/config_loader/tests.rs b/codex-rs/core/src/config_loader/tests.rs index 08bbb5304d..3eab05ba18 100644 --- a/codex-rs/core/src/config_loader/tests.rs +++ b/codex-rs/core/src/config_loader/tests.rs @@ -1,6 +1,5 @@ use super::LoaderOverrides; use super::load_config_layers_state; -use crate::config::CONFIG_TOML_FILE; use crate::config::ConfigBuilder; use crate::config::ConfigOverrides; use crate::config::ConfigToml; @@ -15,6 +14,7 @@ use crate::config_loader::ConfigRequirementsWithSources; use crate::config_loader::RequirementSource; use crate::config_loader::load_requirements_toml; use crate::config_loader::version_for_toml; +use codex_config::CONFIG_TOML_FILE; use codex_protocol::config_types::TrustLevel; use codex_protocol::config_types::WebSearchMode; use codex_protocol::protocol::AskForApproval; @@ -153,7 +153,7 @@ async fn returns_config_error_for_schema_error_in_user_config() { let config_error = config_error_from_io(&err); let _guard = codex_utils_absolute_path::AbsolutePathBufGuard::new(tmp.path()); let expected_config_error = - super::diagnostics::config_error_from_config_toml(&config_path, contents) + codex_config::config_error_from_typed_toml::(&config_path, contents) .expect("schema error"); assert_eq!(config_error, &expected_config_error); } @@ -166,7 +166,7 @@ fn schema_error_points_to_feature_value() { std::fs::write(&config_path, contents).expect("write config"); let _guard = codex_utils_absolute_path::AbsolutePathBufGuard::new(tmp.path()); - let error = super::diagnostics::config_error_from_config_toml(&config_path, contents) + let error = codex_config::config_error_from_typed_toml::(&config_path, contents) .expect("schema error"); let value_line = contents.lines().nth(1).expect("value line"); diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index f2cedb3f94..52329a255f 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -224,6 +224,9 @@ impl ExecPolicyManager { let requested_amendment = derive_requested_execpolicy_amendment_from_prefix_rule( prefix_rule.as_ref(), &evaluation.matched_rules, + exec_policy.as_ref(), + &commands, + &exec_policy_fallback, ); match evaluation.decision { @@ -592,6 +595,9 @@ fn try_derive_execpolicy_amendment_for_allow_rules( fn derive_requested_execpolicy_amendment_from_prefix_rule( prefix_rule: Option<&Vec>, matched_rules: &[RuleMatch], + exec_policy: &Policy, + commands: &[Vec], + exec_policy_fallback: &impl Fn(&[String]) -> Decision, ) -> Option { let prefix_rule = prefix_rule?; if prefix_rule.is_empty() { @@ -612,7 +618,39 @@ fn derive_requested_execpolicy_amendment_from_prefix_rule( return None; } - Some(ExecPolicyAmendment::new(prefix_rule.clone())) + let amendment = ExecPolicyAmendment::new(prefix_rule.clone()); + if prefix_rule_would_approve_all_commands( + exec_policy, + &amendment.command, + commands, + exec_policy_fallback, + ) { + Some(amendment) + } else { + None + } +} + +fn prefix_rule_would_approve_all_commands( + exec_policy: &Policy, + prefix_rule: &[String], + commands: &[Vec], + exec_policy_fallback: &impl Fn(&[String]) -> Decision, +) -> bool { + let mut policy_with_prefix_rule = exec_policy.clone(); + if policy_with_prefix_rule + .add_prefix_rule(prefix_rule, Decision::Allow) + .is_err() + { + return false; + } + + commands.iter().all(|command| { + policy_with_prefix_rule + .check(command, exec_policy_fallback) + .decision + == Decision::Allow + }) } /// Only return a reason when a policy rule drove the prompt decision. @@ -1125,7 +1163,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") } #[tokio::test] - async fn keeps_requested_amendment_for_heredoc_fallback_prompts() { + async fn drops_requested_amendment_for_heredoc_fallback_prompts_when_it_wont_match() { let command = vec![ "bash".to_string(), "-lc".to_string(), @@ -1147,7 +1185,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") requirement, ExecApprovalRequirement::NeedsApproval { reason: None, - proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(requested_prefix)), + proposed_execpolicy_amendment: None, } ); } @@ -1472,6 +1510,38 @@ prefix_rule( ); } + #[tokio::test] + async fn request_rule_falls_back_when_prefix_rule_does_not_approve_all_commands() { + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + "cargo install cargo-insta && rm -rf /tmp/codex".to_string(), + ]; + let manager = ExecPolicyManager::default(); + + let requirement = manager + .create_exec_approval_requirement_for_command(ExecApprovalRequest { + command: &command, + approval_policy: AskForApproval::OnRequest, + sandbox_policy: &SandboxPolicy::DangerFullAccess, + sandbox_permissions: SandboxPermissions::RequireEscalated, + prefix_rule: Some(vec!["cargo".to_string(), "install".to_string()]), + }) + .await; + + assert_eq!( + requirement, + ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![ + "rm".to_string(), + "-rf".to_string(), + "/tmp/codex".to_string(), + ])), + } + ); + } + #[tokio::test] async fn heuristics_apply_when_other_commands_match_policy() { let policy_src = r#"prefix_rule(pattern=["apple"], decision="allow")"#; @@ -1728,11 +1798,28 @@ prefix_rule( ); } + fn derive_requested_execpolicy_amendment_for_test( + prefix_rule: Option<&Vec>, + matched_rules: &[RuleMatch], + ) -> Option { + let commands = prefix_rule + .cloned() + .map(|prefix_rule| vec![prefix_rule]) + .unwrap_or_else(|| vec![vec!["echo".to_string()]]); + derive_requested_execpolicy_amendment_from_prefix_rule( + prefix_rule, + matched_rules, + &Policy::empty(), + &commands, + &|_: &[String]| Decision::Allow, + ) + } + #[test] fn derive_requested_execpolicy_amendment_returns_none_for_missing_prefix_rule() { assert_eq!( None, - derive_requested_execpolicy_amendment_from_prefix_rule(None, &[]) + derive_requested_execpolicy_amendment_for_test(None, &[]) ); } @@ -1740,7 +1827,7 @@ prefix_rule( fn derive_requested_execpolicy_amendment_returns_none_for_empty_prefix_rule() { assert_eq!( None, - derive_requested_execpolicy_amendment_from_prefix_rule(Some(&Vec::new()), &[]) + derive_requested_execpolicy_amendment_for_test(Some(&Vec::new()), &[]) ); } @@ -1748,7 +1835,7 @@ prefix_rule( fn derive_requested_execpolicy_amendment_returns_none_for_exact_banned_prefix_rule() { assert_eq!( None, - derive_requested_execpolicy_amendment_from_prefix_rule( + derive_requested_execpolicy_amendment_for_test( Some(&vec!["python".to_string(), "-c".to_string()]), &[], ) @@ -1767,7 +1854,7 @@ prefix_rule( ] { assert_eq!( None, - derive_requested_execpolicy_amendment_from_prefix_rule(Some(&prefix_rule), &[]) + derive_requested_execpolicy_amendment_for_test(Some(&prefix_rule), &[]) ); } } @@ -1793,7 +1880,7 @@ prefix_rule( ] { assert_eq!( None, - derive_requested_execpolicy_amendment_from_prefix_rule(Some(&prefix_rule), &[]) + derive_requested_execpolicy_amendment_for_test(Some(&prefix_rule), &[]) ); } } @@ -1808,7 +1895,7 @@ prefix_rule( assert_eq!( Some(ExecPolicyAmendment::new(prefix_rule.clone())), - derive_requested_execpolicy_amendment_from_prefix_rule(Some(&prefix_rule), &[]) + derive_requested_execpolicy_amendment_for_test(Some(&prefix_rule), &[]) ); } @@ -1823,7 +1910,7 @@ prefix_rule( }]; assert_eq!( None, - derive_requested_execpolicy_amendment_from_prefix_rule( + derive_requested_execpolicy_amendment_for_test( Some(&prefix_rule), &matched_rules_prompt ), @@ -1836,7 +1923,7 @@ prefix_rule( }]; assert_eq!( None, - derive_requested_execpolicy_amendment_from_prefix_rule( + derive_requested_execpolicy_amendment_for_test( Some(&prefix_rule), &matched_rules_allow ), @@ -1849,9 +1936,9 @@ prefix_rule( }]; assert_eq!( None, - derive_requested_execpolicy_amendment_from_prefix_rule( + derive_requested_execpolicy_amendment_for_test( Some(&prefix_rule), - &matched_rules_forbidden + &matched_rules_forbidden, ), "should return none when prompt policy matches" ); diff --git a/codex-rs/core/src/features.rs b/codex-rs/core/src/features.rs index a4be91046c..4cefe4b216 100644 --- a/codex-rs/core/src/features.rs +++ b/codex-rs/core/src/features.rs @@ -5,13 +5,13 @@ //! booleans through multiple types, call sites consult a single `Features` //! container attached to `Config`. -use crate::config::CONFIG_TOML_FILE; use crate::config::Config; use crate::config::ConfigToml; use crate::config::profile::ConfigProfile; use crate::protocol::Event; use crate::protocol::EventMsg; use crate::protocol::WarningEvent; +use codex_config::CONFIG_TOML_FILE; use codex_otel::OtelManager; use schemars::JsonSchema; use serde::Deserialize; diff --git a/codex-rs/core/src/network_proxy_loader.rs b/codex-rs/core/src/network_proxy_loader.rs index 1af36b2acb..15d4e41664 100644 --- a/codex-rs/core/src/network_proxy_loader.rs +++ b/codex-rs/core/src/network_proxy_loader.rs @@ -1,4 +1,3 @@ -use crate::config::CONFIG_TOML_FILE; use crate::config::NetworkToml; use crate::config::PermissionsToml; use crate::config::find_codex_home; @@ -11,6 +10,7 @@ use anyhow::Context; use anyhow::Result; use async_trait::async_trait; use codex_app_server_protocol::ConfigLayerSource; +use codex_config::CONFIG_TOML_FILE; use codex_network_proxy::ConfigReloader; use codex_network_proxy::ConfigState; use codex_network_proxy::NetworkProxyConfig; diff --git a/codex-rs/core/src/skills/loader.rs b/codex-rs/core/src/skills/loader.rs index 3c497bfcf8..1ad231438a 100644 --- a/codex-rs/core/src/skills/loader.rs +++ b/codex-rs/core/src/skills/loader.rs @@ -806,7 +806,6 @@ fn extract_frontmatter(contents: &str) -> Option { #[cfg(test)] mod tests { use super::*; - use crate::config::CONFIG_TOML_FILE; use crate::config::ConfigBuilder; use crate::config::ConfigOverrides; use crate::config::ConfigToml; @@ -817,6 +816,7 @@ mod tests { use crate::config_loader::ConfigLayerStack; use crate::config_loader::ConfigRequirements; use crate::config_loader::ConfigRequirementsToml; + use codex_config::CONFIG_TOML_FILE; use codex_protocol::config_types::TrustLevel; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 14d9c79e5e..374cc1ada7 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -184,6 +184,16 @@ fn shell_event( command: &str, timeout_ms: u64, sandbox_permissions: SandboxPermissions, +) -> Result { + shell_event_with_prefix_rule(call_id, command, timeout_ms, sandbox_permissions, None) +} + +fn shell_event_with_prefix_rule( + call_id: &str, + command: &str, + timeout_ms: u64, + sandbox_permissions: SandboxPermissions, + prefix_rule: Option>, ) -> Result { let mut args = json!({ "command": command, @@ -192,6 +202,9 @@ fn shell_event( if sandbox_permissions.requires_escalated_permissions() { args["sandbox_permissions"] = json!(sandbox_permissions); } + if let Some(prefix_rule) = prefix_rule { + args["prefix_rule"] = json!(prefix_rule); + } let args_str = serde_json::to_string(&args)?; Ok(ev_function_call(call_id, "shell_command", &args_str)) } @@ -1928,6 +1941,171 @@ async fn approving_execpolicy_amendment_persists_policy_and_skips_future_prompts Ok(()) } +#[tokio::test(flavor = "current_thread")] +#[cfg(unix)] +async fn invalid_requested_prefix_rule_falls_back_for_compound_command() -> Result<()> { + let server = start_mock_server().await; + let approval_policy = AskForApproval::OnRequest; + let sandbox_policy = SandboxPolicy::new_read_only_policy(); + let sandbox_policy_for_config = sandbox_policy.clone(); + let mut builder = test_codex().with_config(move |config| { + config.permissions.approval_policy = Constrained::allow_any(approval_policy); + config.permissions.sandbox_policy = Constrained::allow_any(sandbox_policy_for_config); + }); + let test = builder.build(&server).await?; + + let call_id = "invalid-prefix-rule"; + let command = + "touch /tmp/codex-fallback-rule-test.txt && echo hello > /tmp/codex-fallback-rule-test.txt"; + let event = shell_event_with_prefix_rule( + call_id, + command, + 1_000, + SandboxPermissions::RequireEscalated, + Some(vec!["touch".to_string()]), + )?; + + let _ = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-invalid-prefix-1"), + event, + ev_completed("resp-invalid-prefix-1"), + ]), + ) + .await; + + submit_turn( + &test, + "invalid-prefix-rule", + approval_policy, + sandbox_policy.clone(), + ) + .await?; + + let approval = expect_exec_approval(&test, command).await; + let amendment = approval + .proposed_execpolicy_amendment + .expect("should have a proposed execpolicy amendment"); + assert!(amendment.command.contains(&command.to_string())); + + Ok(()) +} + +#[tokio::test(flavor = "current_thread")] +#[cfg(unix)] +async fn approving_fallback_rule_for_compound_command_works() -> Result<()> { + let server = start_mock_server().await; + let approval_policy = AskForApproval::OnRequest; + let sandbox_policy = SandboxPolicy::new_read_only_policy(); + let sandbox_policy_for_config = sandbox_policy.clone(); + let mut builder = test_codex().with_config(move |config| { + config.permissions.approval_policy = Constrained::allow_any(approval_policy); + config.permissions.sandbox_policy = Constrained::allow_any(sandbox_policy_for_config); + }); + let test = builder.build(&server).await?; + + let call_id = "invalid-prefix-rule"; + let command = + "touch /tmp/codex-fallback-rule-test.txt && echo hello > /tmp/codex-fallback-rule-test.txt"; + let event = shell_event_with_prefix_rule( + call_id, + command, + 1_000, + SandboxPermissions::RequireEscalated, + Some(vec!["touch".to_string()]), + )?; + + let _ = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-invalid-prefix-1"), + event, + ev_completed("resp-invalid-prefix-1"), + ]), + ) + .await; + + submit_turn( + &test, + "invalid-prefix-rule", + approval_policy, + sandbox_policy.clone(), + ) + .await?; + + let approval = expect_exec_approval(&test, command).await; + let approval_id = approval.effective_approval_id(); + let amendment = approval + .proposed_execpolicy_amendment + .expect("should have a proposed execpolicy amendment"); + assert!(amendment.command.contains(&command.to_string())); + + test.codex + .submit(Op::ExecApproval { + id: approval_id, + turn_id: None, + decision: ReviewDecision::ApprovedExecpolicyAmendment { + proposed_execpolicy_amendment: amendment.clone(), + }, + }) + .await?; + wait_for_completion(&test).await; + + let call_id = "invalid-prefix-rule-again"; + let command = + "touch /tmp/codex-fallback-rule-test.txt && echo hello > /tmp/codex-fallback-rule-test.txt"; + let event = shell_event_with_prefix_rule( + call_id, + command, + 1_000, + SandboxPermissions::RequireEscalated, + Some(vec!["touch".to_string()]), + )?; + + let _ = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-invalid-prefix-1"), + event, + ev_completed("resp-invalid-prefix-1"), + ]), + ) + .await; + let second_results = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-invalid-prefix-1", "done"), + ev_completed("resp-invalid-prefix-2"), + ]), + ) + .await; + + submit_turn( + &test, + "invalid-prefix-rule", + approval_policy, + sandbox_policy.clone(), + ) + .await?; + + wait_for_completion_without_approval(&test).await; + + let second_output = parse_result( + &second_results + .single_request() + .function_call_output(call_id), + ); + assert_eq!(second_output.exit_code.unwrap_or(0), 0); + assert!( + second_output.stdout.is_empty(), + "unexpected stdout: {}", + second_output.stdout + ); + + Ok(()) +} + // todo(dylan) add ScenarioSpec support for rules #[tokio::test(flavor = "current_thread")] #[cfg(unix)] diff --git a/codex-rs/core/tests/suite/unstable_features_warning.rs b/codex-rs/core/tests/suite/unstable_features_warning.rs index 1cb07c1f4e..e5fdf61fdb 100644 --- a/codex-rs/core/tests/suite/unstable_features_warning.rs +++ b/codex-rs/core/tests/suite/unstable_features_warning.rs @@ -1,8 +1,8 @@ #![allow(clippy::unwrap_used, clippy::expect_used)] +use codex_config::CONFIG_TOML_FILE; use codex_core::CodexAuth; use codex_core::NewThread; -use codex_core::config::CONFIG_TOML_FILE; use codex_core::features::Feature; use codex_core::protocol::EventMsg; use codex_core::protocol::InitialHistory;