diff --git a/codex-rs/common/src/config_summary.rs b/codex-rs/common/src/config_summary.rs index 5a5901880..2254eeae8 100644 --- a/codex-rs/common/src/config_summary.rs +++ b/codex-rs/common/src/config_summary.rs @@ -9,7 +9,7 @@ pub fn create_config_summary_entries(config: &Config, model: &str) -> Vec<(&'sta ("workdir", config.cwd.display().to_string()), ("model", model.to_string()), ("provider", config.model_provider_id.clone()), - ("approval", config.approval_policy.to_string()), + ("approval", config.approval_policy.value().to_string()), ("sandbox", summarize_sandbox_policy(&config.sandbox_policy)), ]; if config.model_provider.wire_api == WireApi::Responses { diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 6322c471f..0def94fa1 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -77,6 +77,9 @@ use crate::client_common::Prompt; use crate::client_common::ResponseEvent; use crate::compact::collect_user_messages; use crate::config::Config; +use crate::config::Constrained; +use crate::config::ConstraintError; +use crate::config::ConstraintResult; use crate::config::GhostSnapshotConfig; use crate::config::types::ShellEnvironmentPolicy; use crate::context_manager::ContextManager; @@ -96,6 +99,7 @@ use crate::protocol::ApplyPatchApprovalRequestEvent; use crate::protocol::AskForApproval; use crate::protocol::BackgroundEventEvent; use crate::protocol::DeprecationNoticeEvent; +use crate::protocol::ErrorEvent; use crate::protocol::Event; use crate::protocol::EventMsg; use crate::protocol::ExecApprovalRequestEvent; @@ -260,7 +264,7 @@ impl Codex { user_instructions, base_instructions: config.base_instructions.clone(), compact_prompt: config.compact_prompt.clone(), - approval_policy: config.approval_policy, + approval_policy: config.approval_policy.clone(), sandbox_policy: config.sandbox_policy.clone(), cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), @@ -411,7 +415,7 @@ pub(crate) struct SessionConfiguration { compact_prompt: Option, /// When to escalate for approval for execution - approval_policy: AskForApproval, + approval_policy: Constrained, /// How to sandbox commands executed in the system sandbox_policy: SandboxPolicy, @@ -434,7 +438,7 @@ pub(crate) struct SessionConfiguration { } impl SessionConfiguration { - pub(crate) fn apply(&self, updates: &SessionSettingsUpdate) -> Self { + pub(crate) fn apply(&self, updates: &SessionSettingsUpdate) -> ConstraintResult { let mut next_configuration = self.clone(); if let Some(model) = updates.model.clone() { next_configuration.model = model; @@ -446,7 +450,7 @@ impl SessionConfiguration { next_configuration.model_reasoning_summary = summary; } if let Some(approval_policy) = updates.approval_policy { - next_configuration.approval_policy = approval_policy; + next_configuration.approval_policy.set(approval_policy)?; } if let Some(sandbox_policy) = updates.sandbox_policy.clone() { next_configuration.sandbox_policy = sandbox_policy; @@ -454,7 +458,7 @@ impl SessionConfiguration { if let Some(cwd) = updates.cwd.clone() { next_configuration.cwd = cwd; } - next_configuration + Ok(next_configuration) } } @@ -523,7 +527,7 @@ impl Session { base_instructions: session_configuration.base_instructions.clone(), compact_prompt: session_configuration.compact_prompt.clone(), user_instructions: session_configuration.user_instructions.clone(), - approval_policy: session_configuration.approval_policy, + approval_policy: session_configuration.approval_policy.value(), sandbox_policy: session_configuration.sandbox_policy.clone(), shell_environment_policy: per_turn_config.shell_environment_policy.clone(), tools_config, @@ -640,7 +644,7 @@ impl Session { config.model_reasoning_summary, config.model_context_window, config.model_auto_compact_token_limit, - config.approval_policy, + config.approval_policy.value(), config.sandbox_policy.clone(), config.mcp_servers.keys().map(String::as_str).collect(), config.active_profile.clone(), @@ -690,7 +694,7 @@ impl Session { session_id: conversation_id, model: session_configuration.model.clone(), model_provider_id: config.model_provider_id.clone(), - approval_policy: session_configuration.approval_policy, + approval_policy: session_configuration.approval_policy.value(), sandbox_policy: session_configuration.sandbox_policy.clone(), cwd: session_configuration.cwd.clone(), reasoning_effort: session_configuration.model_reasoning_effort, @@ -739,8 +743,7 @@ impl Session { loop { match rx.recv().await { Ok(()) => { - let turn_context = - sess.new_turn(SessionSettingsUpdate::default()).await; + let turn_context = sess.new_default_turn().await; sess.send_event(turn_context.as_ref(), EventMsg::SkillsUpdateAvailable) .await; } @@ -784,7 +787,7 @@ impl Session { } async fn record_initial_history(&self, conversation_history: InitialHistory) { - let turn_context = self.new_turn(SessionSettingsUpdate::default()).await; + let turn_context = self.new_default_turn().await; match conversation_history { InitialHistory::New => { // Build and record initial items (user instructions + environment context) @@ -843,30 +846,76 @@ impl Session { } } - pub(crate) async fn update_settings(&self, updates: SessionSettingsUpdate) { + pub(crate) async fn update_settings( + &self, + updates: SessionSettingsUpdate, + ) -> ConstraintResult<()> { let mut state = self.state.lock().await; - state.session_configuration = state.session_configuration.apply(&updates); - } - - pub(crate) async fn new_turn(&self, updates: SessionSettingsUpdate) -> Arc { - let sub_id = self.next_internal_sub_id(); - self.new_turn_with_sub_id(sub_id, updates).await + match state.session_configuration.apply(&updates) { + Ok(updated) => { + state.session_configuration = updated; + Ok(()) + } + Err(err) => { + let wrapped = ConstraintError { + message: format!("Could not update config: {err}"), + }; + warn!(%wrapped, "rejected session settings update"); + Err(wrapped) + } + } } pub(crate) async fn new_turn_with_sub_id( &self, sub_id: String, updates: SessionSettingsUpdate, - ) -> Arc { + ) -> ConstraintResult> { let (session_configuration, sandbox_policy_changed) = { let mut state = self.state.lock().await; - let session_configuration = state.session_configuration.clone().apply(&updates); - let sandbox_policy_changed = - state.session_configuration.sandbox_policy != session_configuration.sandbox_policy; - state.session_configuration = session_configuration.clone(); - (session_configuration, sandbox_policy_changed) + match state.session_configuration.clone().apply(&updates) { + Ok(next) => { + let sandbox_policy_changed = + state.session_configuration.sandbox_policy != next.sandbox_policy; + state.session_configuration = next.clone(); + (next, sandbox_policy_changed) + } + Err(err) => { + drop(state); + let wrapped = ConstraintError { + message: format!("Could not update config: {err}"), + }; + self.send_event_raw(Event { + id: sub_id.clone(), + msg: EventMsg::Error(ErrorEvent { + message: wrapped.to_string(), + codex_error_info: Some(CodexErrorInfo::BadRequest), + }), + }) + .await; + return Err(wrapped); + } + } }; + + Ok(self + .new_turn_from_configuration( + sub_id, + session_configuration, + updates.final_output_json_schema, + sandbox_policy_changed, + ) + .await) + } + + async fn new_turn_from_configuration( + &self, + sub_id: String, + session_configuration: SessionConfiguration, + final_output_json_schema: Option>, + sandbox_policy_changed: bool, + ) -> Arc { let per_turn_config = Self::build_per_turn_config(&session_configuration); if sandbox_policy_changed { @@ -902,12 +951,26 @@ impl Session { self.conversation_id, sub_id, ); - if let Some(final_schema) = updates.final_output_json_schema { + if let Some(final_schema) = final_output_json_schema { turn_context.final_output_json_schema = final_schema; } Arc::new(turn_context) } + pub(crate) async fn new_default_turn(&self) -> Arc { + self.new_default_turn_with_sub_id(self.next_internal_sub_id()) + .await + } + + pub(crate) async fn new_default_turn_with_sub_id(&self, sub_id: String) -> Arc { + let session_configuration = { + let state = self.state.lock().await; + state.session_configuration.clone() + }; + self.new_turn_from_configuration(sub_id, session_configuration, None, false) + .await + } + fn build_environment_update_item( &self, previous: Option<&Arc>, @@ -1551,8 +1614,7 @@ impl Session { async fn submission_loop(sess: Arc, config: Arc, rx_sub: Receiver) { // Seed with context in case there is an OverrideTurnContext first. - let mut previous_context: Option> = - Some(sess.new_turn(SessionSettingsUpdate::default()).await); + let mut previous_context: Option> = Some(sess.new_default_turn().await); // To break out of this loop, send Op::Shutdown. while let Ok(sub) = rx_sub.recv().await { @@ -1571,6 +1633,7 @@ async fn submission_loop(sess: Arc, config: Arc, rx_sub: Receiv } => { handlers::override_turn_context( &sess, + sub.id.clone(), SessionSettingsUpdate { cwd, approval_policy, @@ -1688,8 +1751,21 @@ mod handlers { sess.interrupt_task().await; } - pub async fn override_turn_context(sess: &Session, updates: SessionSettingsUpdate) { - sess.update_settings(updates).await; + pub async fn override_turn_context( + sess: &Session, + sub_id: String, + updates: SessionSettingsUpdate, + ) { + if let Err(err) = sess.update_settings(updates).await { + sess.send_event_raw(Event { + id: sub_id, + msg: EventMsg::Error(ErrorEvent { + message: err.to_string(), + codex_error_info: Some(CodexErrorInfo::BadRequest), + }), + }) + .await; + } } pub async fn user_input_or_turn( @@ -1724,7 +1800,10 @@ mod handlers { _ => unreachable!(), }; - let current_context = sess.new_turn_with_sub_id(sub_id, updates).await; + let Ok(current_context) = sess.new_turn_with_sub_id(sub_id, updates).await else { + // new_turn_with_sub_id already emits the error event. + return; + }; current_context .client .get_otel_manager() @@ -1751,9 +1830,7 @@ mod handlers { command: String, previous_context: &mut Option>, ) { - let turn_context = sess - .new_turn_with_sub_id(sub_id, SessionSettingsUpdate::default()) - .await; + let turn_context = sess.new_default_turn_with_sub_id(sub_id).await; sess.spawn_task( Arc::clone(&turn_context), Vec::new(), @@ -1950,17 +2027,13 @@ mod handlers { } pub async fn undo(sess: &Arc, sub_id: String) { - let turn_context = sess - .new_turn_with_sub_id(sub_id, SessionSettingsUpdate::default()) - .await; + let turn_context = sess.new_default_turn_with_sub_id(sub_id).await; sess.spawn_task(turn_context, Vec::new(), UndoTask::new()) .await; } pub async fn compact(sess: &Arc, sub_id: String) { - let turn_context = sess - .new_turn_with_sub_id(sub_id, SessionSettingsUpdate::default()) - .await; + let turn_context = sess.new_default_turn_with_sub_id(sub_id).await; sess.spawn_task( Arc::clone(&turn_context), @@ -2014,9 +2087,7 @@ mod handlers { sub_id: String, review_request: ReviewRequest, ) { - let turn_context = sess - .new_turn_with_sub_id(sub_id.clone(), SessionSettingsUpdate::default()) - .await; + let turn_context = sess.new_default_turn_with_sub_id(sub_id.clone()).await; match resolve_review_request(review_request, config.cwd.as_path()) { Ok(resolved) => { spawn_review_thread( @@ -2806,7 +2877,7 @@ mod tests { user_instructions: config.user_instructions.clone(), base_instructions: config.base_instructions.clone(), compact_prompt: config.compact_prompt.clone(), - approval_policy: config.approval_policy, + approval_policy: config.approval_policy.clone(), sandbox_policy: config.sandbox_policy.clone(), cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), @@ -2878,7 +2949,7 @@ mod tests { user_instructions: config.user_instructions.clone(), base_instructions: config.base_instructions.clone(), compact_prompt: config.compact_prompt.clone(), - approval_policy: config.approval_policy, + approval_policy: config.approval_policy.clone(), sandbox_policy: config.sandbox_policy.clone(), cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), @@ -3082,7 +3153,7 @@ mod tests { user_instructions: config.user_instructions.clone(), base_instructions: config.base_instructions.clone(), compact_prompt: config.compact_prompt.clone(), - approval_policy: config.approval_policy, + approval_policy: config.approval_policy.clone(), sandbox_policy: config.sandbox_policy.clone(), cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), @@ -3173,7 +3244,7 @@ mod tests { user_instructions: config.user_instructions.clone(), base_instructions: config.base_instructions.clone(), compact_prompt: config.compact_prompt.clone(), - approval_policy: config.approval_policy, + approval_policy: config.approval_policy.clone(), sandbox_policy: config.sandbox_policy.clone(), cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), diff --git a/codex-rs/core/src/config/constraint.rs b/codex-rs/core/src/config/constraint.rs new file mode 100644 index 000000000..f10df09dd --- /dev/null +++ b/codex-rs/core/src/config/constraint.rs @@ -0,0 +1,195 @@ +use std::fmt; +use std::sync::Arc; + +use thiserror::Error; + +#[derive(Debug, Error, PartialEq, Eq)] +#[error("{message}")] +pub struct ConstraintError { + pub message: String, +} + +impl ConstraintError { + pub fn invalid_value(candidate: impl Into, allowed: impl Into) -> Self { + Self { + message: format!( + "value `{}` is not in the allowed set {}", + candidate.into(), + allowed.into() + ), + } + } +} + +pub type ConstraintResult = Result; + +impl From for std::io::Error { + fn from(err: ConstraintError) -> Self { + std::io::Error::new(std::io::ErrorKind::InvalidInput, err) + } +} + +type ConstraintValidator = dyn Fn(&T) -> ConstraintResult<()> + Send + Sync; + +#[derive(Clone)] +pub struct Constrained { + value: T, + validator: Arc>, +} + +impl Constrained { + pub fn new( + initial_value: T, + validator: impl Fn(&T) -> ConstraintResult<()> + Send + Sync + 'static, + ) -> ConstraintResult { + let validator: Arc> = Arc::new(validator); + validator(&initial_value)?; + Ok(Self { + value: initial_value, + validator, + }) + } + + pub fn allow_any(initial_value: T) -> Self { + Self { + value: initial_value, + validator: Arc::new(|_| Ok(())), + } + } + + pub fn allow_values(initial_value: T, allowed: Vec) -> ConstraintResult + where + T: PartialEq + Send + Sync + fmt::Debug + 'static, + { + Self::new(initial_value, move |candidate| { + if allowed.contains(candidate) { + Ok(()) + } else { + Err(ConstraintError::invalid_value( + format!("{candidate:?}"), + format!("{allowed:?}"), + )) + } + }) + } + + pub fn get(&self) -> &T { + &self.value + } + + pub fn value(&self) -> T + where + T: Copy, + { + self.value + } + + pub fn can_set(&self, candidate: &T) -> ConstraintResult<()> { + (self.validator)(candidate) + } + + pub fn set(&mut self, value: T) -> ConstraintResult<()> { + (self.validator)(&value)?; + self.value = value; + Ok(()) + } +} + +impl std::ops::Deref for Constrained { + type Target = T; + + fn deref(&self) -> &Self::Target { + &self.value + } +} + +impl fmt::Debug for Constrained { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("Constrained") + .field("value", &self.value) + .finish() + } +} + +impl PartialEq for Constrained { + fn eq(&self, other: &Self) -> bool { + self.value == other.value + } +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn constrained_allow_any_accepts_any_value() { + let mut constrained = Constrained::allow_any(5); + constrained.set(-10).expect("allow any accepts all values"); + assert_eq!(constrained.value(), -10); + } + + #[test] + fn constrained_new_rejects_invalid_initial_value() { + let result = Constrained::new(0, |value| { + if *value > 0 { + Ok(()) + } else { + Err(ConstraintError::invalid_value( + value.to_string(), + "positive values", + )) + } + }); + + assert_eq!( + result, + Err(ConstraintError::invalid_value("0", "positive values")) + ); + } + + #[test] + fn constrained_set_rejects_invalid_value_and_leaves_previous() { + let mut constrained = Constrained::new(1, |value| { + if *value > 0 { + Ok(()) + } else { + Err(ConstraintError::invalid_value( + value.to_string(), + "positive values", + )) + } + }) + .expect("initial value should be accepted"); + + let err = constrained + .set(-5) + .expect_err("negative values should be rejected"); + assert_eq!(err, ConstraintError::invalid_value("-5", "positive values")); + assert_eq!(constrained.value(), 1); + } + + #[test] + fn constrained_can_set_allows_probe_without_setting() { + let constrained = Constrained::new(1, |value| { + if *value > 0 { + Ok(()) + } else { + Err(ConstraintError::invalid_value( + value.to_string(), + "positive values", + )) + } + }) + .expect("initial value should be accepted"); + + constrained + .can_set(&2) + .expect("can_set should accept positive value"); + let err = constrained + .can_set(&-1) + .expect_err("can_set should reject negative value"); + assert_eq!(err, ConstraintError::invalid_value("-1", "positive values")); + assert_eq!(constrained.value(), 1); + } +} diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index a7e11f45e..75309c9bb 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -54,10 +54,14 @@ use crate::config::profile::ConfigProfile; use toml::Value as TomlValue; use toml_edit::DocumentMut; +mod constraint; pub mod edit; pub mod profile; pub mod service; pub mod types; +pub use constraint::Constrained; +pub use constraint::ConstraintError; +pub use constraint::ConstraintResult; pub use service::ConfigService; pub use service::ConfigServiceError; @@ -106,7 +110,7 @@ pub struct Config { pub model_provider: ModelProviderInfo, /// Approval policy for executing commands. - pub approval_policy: AskForApproval, + pub approval_policy: Constrained, pub sandbox_policy: SandboxPolicy, @@ -1026,15 +1030,14 @@ impl Config { .or(cfg.approval_policy) .unwrap_or_else(|| { if active_project.is_trusted() { - // If no explicit approval policy is set, but we trust cwd, default to OnRequest AskForApproval::OnRequest } else if active_project.is_untrusted() { - // If project is explicitly marked untrusted, require approval for non-safe commands AskForApproval::UnlessTrusted } else { AskForApproval::default() } }); + let approval_policy = Constrained::allow_any(approval_policy); let did_user_set_custom_approval_policy_or_sandbox_mode = approval_policy_override .is_some() || config_profile.approval_policy.is_some() @@ -2945,7 +2948,7 @@ model_verbosity = "high" model_auto_compact_token_limit: None, model_provider_id: "openai".to_string(), model_provider: fixture.openai_provider.clone(), - approval_policy: AskForApproval::Never, + approval_policy: Constrained::allow_any(AskForApproval::Never), sandbox_policy: SandboxPolicy::new_read_only_policy(), did_user_set_custom_approval_policy_or_sandbox_mode: true, forced_auto_mode_downgraded_on_windows: false, @@ -3020,7 +3023,7 @@ model_verbosity = "high" model_auto_compact_token_limit: None, model_provider_id: "openai-chat-completions".to_string(), model_provider: fixture.openai_chat_completions_provider.clone(), - approval_policy: AskForApproval::UnlessTrusted, + approval_policy: Constrained::allow_any(AskForApproval::UnlessTrusted), sandbox_policy: SandboxPolicy::new_read_only_policy(), did_user_set_custom_approval_policy_or_sandbox_mode: true, forced_auto_mode_downgraded_on_windows: false, @@ -3110,7 +3113,7 @@ model_verbosity = "high" model_auto_compact_token_limit: None, model_provider_id: "openai".to_string(), model_provider: fixture.openai_provider.clone(), - approval_policy: AskForApproval::OnFailure, + approval_policy: Constrained::allow_any(AskForApproval::OnFailure), sandbox_policy: SandboxPolicy::new_read_only_policy(), did_user_set_custom_approval_policy_or_sandbox_mode: true, forced_auto_mode_downgraded_on_windows: false, @@ -3186,7 +3189,7 @@ model_verbosity = "high" model_auto_compact_token_limit: None, model_provider_id: "openai".to_string(), model_provider: fixture.openai_provider.clone(), - approval_policy: AskForApproval::OnFailure, + approval_policy: Constrained::allow_any(AskForApproval::OnFailure), sandbox_policy: SandboxPolicy::new_read_only_policy(), did_user_set_custom_approval_policy_or_sandbox_mode: true, forced_auto_mode_downgraded_on_windows: false, @@ -3500,26 +3503,21 @@ trust_level = "untrusted" } #[test] - fn test_untrusted_project_gets_unless_trusted_approval_policy() -> std::io::Result<()> { + fn test_untrusted_project_gets_unless_trusted_approval_policy() -> anyhow::Result<()> { let codex_home = TempDir::new()?; let test_project_dir = TempDir::new()?; let test_path = test_project_dir.path(); - let mut projects = std::collections::HashMap::new(); - projects.insert( - test_path.to_string_lossy().to_string(), - ProjectConfig { - trust_level: Some(TrustLevel::Untrusted), - }, - ); - - let cfg = ConfigToml { - projects: Some(projects), - ..Default::default() - }; - let config = Config::load_from_base_config_with_overrides( - cfg, + ConfigToml { + projects: Some(HashMap::from([( + test_path.to_string_lossy().to_string(), + ProjectConfig { + trust_level: Some(TrustLevel::Untrusted), + }, + )])), + ..Default::default() + }, ConfigOverrides { cwd: Some(test_path.to_path_buf()), ..Default::default() @@ -3529,7 +3527,7 @@ trust_level = "untrusted" // Verify that untrusted projects get UnlessTrusted approval policy assert_eq!( - config.approval_policy, + config.approval_policy.value(), AskForApproval::UnlessTrusted, "Expected UnlessTrusted approval policy for untrusted project" ); diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index d1b1a515d..c22868009 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -1,6 +1,7 @@ #![allow(clippy::unwrap_used, clippy::expect_used)] use anyhow::Result; +use codex_core::config::Constrained; use codex_core::features::Feature; use codex_core::protocol::ApplyPatchApprovalRequestEvent; use codex_core::protocol::AskForApproval; @@ -126,7 +127,7 @@ impl ActionKind { ); let command = format!("python3 -c \"{script}\""); - let event = shell_event(call_id, &command, 3_000, sandbox_permissions)?; + let event = shell_event(call_id, &command, 5_000, sandbox_permissions)?; Ok((event, Some(command))) } ActionKind::RunCommand { command } => { @@ -1462,7 +1463,7 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> { let model = model_override.unwrap_or("gpt-5.1"); let mut builder = test_codex().with_model(model).with_config(move |config| { - config.approval_policy = approval_policy; + config.approval_policy = Constrained::allow_any(approval_policy); config.sandbox_policy = sandbox_policy.clone(); for feature in features { config.features.enable(feature); @@ -1568,7 +1569,7 @@ async fn approving_execpolicy_amendment_persists_policy_and_skips_future_prompts let sandbox_policy = SandboxPolicy::ReadOnly; let sandbox_policy_for_config = sandbox_policy.clone(); let mut builder = test_codex().with_config(move |config| { - config.approval_policy = approval_policy; + config.approval_policy = Constrained::allow_any(approval_policy); config.sandbox_policy = sandbox_policy_for_config; }); let test = builder.build(&server).await?; diff --git a/codex-rs/core/tests/suite/codex_delegate.rs b/codex-rs/core/tests/suite/codex_delegate.rs index 2bd156d6a..f0c4cb9fe 100644 --- a/codex-rs/core/tests/suite/codex_delegate.rs +++ b/codex-rs/core/tests/suite/codex_delegate.rs @@ -1,3 +1,4 @@ +use codex_core::config::Constrained; use codex_core::protocol::AskForApproval; use codex_core::protocol::EventMsg; use codex_core::protocol::Op; @@ -61,7 +62,7 @@ async fn codex_delegate_forwards_exec_approval_and_proceeds_on_approval() { // Build a conversation configured to require approvals so the delegate // routes ExecApprovalRequest via the parent. let mut builder = test_codex().with_model("gpt-5.1").with_config(|config| { - config.approval_policy = AskForApproval::OnRequest; + config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); config.sandbox_policy = SandboxPolicy::ReadOnly; }); let test = builder.build(&server).await.expect("build test codex"); @@ -137,7 +138,7 @@ async fn codex_delegate_forwards_patch_approval_and_proceeds_on_decision() { mount_sse_sequence(&server, vec![sse1, sse2]).await; let mut builder = test_codex().with_model("gpt-5.1").with_config(|config| { - config.approval_policy = AskForApproval::OnRequest; + config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); // Use a restricted sandbox so patch approval is required config.sandbox_policy = SandboxPolicy::ReadOnly; config.include_apply_patch_tool = true; diff --git a/codex-rs/core/tests/suite/otel.rs b/codex-rs/core/tests/suite/otel.rs index 922c7b9cf..596cf719b 100644 --- a/codex-rs/core/tests/suite/otel.rs +++ b/codex-rs/core/tests/suite/otel.rs @@ -1,3 +1,4 @@ +use codex_core::config::Constrained; use codex_core::features::Feature; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; @@ -933,7 +934,7 @@ async fn handle_container_exec_autoapprove_from_config_records_tool_decision() { let TestCodex { codex, .. } = test_codex() .with_config(|config| { - config.approval_policy = AskForApproval::OnRequest; + config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); config.sandbox_policy = SandboxPolicy::DangerFullAccess; }) .build(&server) @@ -982,7 +983,7 @@ async fn handle_container_exec_user_approved_records_tool_decision() { let TestCodex { codex, .. } = test_codex() .with_config(|config| { - config.approval_policy = AskForApproval::UnlessTrusted; + config.approval_policy = Constrained::allow_any(AskForApproval::UnlessTrusted); }) .build(&server) .await @@ -1040,7 +1041,7 @@ async fn handle_container_exec_user_approved_for_session_records_tool_decision() let TestCodex { codex, .. } = test_codex() .with_config(|config| { - config.approval_policy = AskForApproval::UnlessTrusted; + config.approval_policy = Constrained::allow_any(AskForApproval::UnlessTrusted); }) .build(&server) .await @@ -1098,7 +1099,7 @@ async fn handle_sandbox_error_user_approves_retry_records_tool_decision() { let TestCodex { codex, .. } = test_codex() .with_config(|config| { - config.approval_policy = AskForApproval::UnlessTrusted; + config.approval_policy = Constrained::allow_any(AskForApproval::UnlessTrusted); }) .build(&server) .await @@ -1156,7 +1157,7 @@ async fn handle_container_exec_user_denies_records_tool_decision() { .await; let TestCodex { codex, .. } = test_codex() .with_config(|config| { - config.approval_policy = AskForApproval::UnlessTrusted; + config.approval_policy = Constrained::allow_any(AskForApproval::UnlessTrusted); }) .build(&server) .await @@ -1214,7 +1215,7 @@ async fn handle_sandbox_error_user_approves_for_session_records_tool_decision() let TestCodex { codex, .. } = test_codex() .with_config(|config| { - config.approval_policy = AskForApproval::UnlessTrusted; + config.approval_policy = Constrained::allow_any(AskForApproval::UnlessTrusted); }) .build(&server) .await @@ -1273,7 +1274,7 @@ async fn handle_sandbox_error_user_denies_records_tool_decision() { let TestCodex { codex, .. } = test_codex() .with_config(|config| { - config.approval_policy = AskForApproval::UnlessTrusted; + config.approval_policy = Constrained::allow_any(AskForApproval::UnlessTrusted); }) .build(&server) .await diff --git a/codex-rs/core/tests/suite/prompt_caching.rs b/codex-rs/core/tests/suite/prompt_caching.rs index e11139090..77fd19ad9 100644 --- a/codex-rs/core/tests/suite/prompt_caching.rs +++ b/codex-rs/core/tests/suite/prompt_caching.rs @@ -593,7 +593,7 @@ async fn send_user_turn_with_no_changes_does_not_send_environment_context() -> a .await?; let default_cwd = config.cwd.clone(); - let default_approval_policy = config.approval_policy; + let default_approval_policy = config.approval_policy.value(); let default_sandbox_policy = config.sandbox_policy.clone(); let default_model = session_configured.model; let default_effort = config.model_reasoning_effort; @@ -685,7 +685,7 @@ async fn send_user_turn_with_changes_sends_environment_context() -> anyhow::Resu .await?; let default_cwd = config.cwd.clone(); - let default_approval_policy = config.approval_policy; + let default_approval_policy = config.approval_policy.value(); let default_sandbox_policy = config.sandbox_policy.clone(); let default_model = session_configured.model; let default_effort = config.model_reasoning_effort; diff --git a/codex-rs/core/tests/suite/resume_warning.rs b/codex-rs/core/tests/suite/resume_warning.rs index cb83ab06d..4b6a13315 100644 --- a/codex-rs/core/tests/suite/resume_warning.rs +++ b/codex-rs/core/tests/suite/resume_warning.rs @@ -24,7 +24,7 @@ fn resume_history( ) -> InitialHistory { let turn_ctx = TurnContextItem { cwd: config.cwd.clone(), - approval_policy: config.approval_policy, + approval_policy: config.approval_policy.value(), sandbox_policy: config.sandbox_policy.clone(), model: previous_model.to_string(), effort: config.model_reasoning_effort, diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index ee0ae45d4..98debd520 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -257,7 +257,7 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any } let default_cwd = config.cwd.to_path_buf(); - let default_approval_policy = config.approval_policy; + let default_approval_policy = config.approval_policy.value(); let default_sandbox_policy = config.sandbox_policy.clone(); let default_effort = config.model_reasoning_effort; let default_summary = config.model_reasoning_summary; diff --git a/codex-rs/tui/src/bottom_pane/command_popup.rs b/codex-rs/tui/src/bottom_pane/command_popup.rs index e1e35ae94..dc123f6c2 100644 --- a/codex-rs/tui/src/bottom_pane/command_popup.rs +++ b/codex-rs/tui/src/bottom_pane/command_popup.rs @@ -185,6 +185,7 @@ impl CommandPopup { display_shortcut: None, description: Some(description), wrap_indent: None, + disabled_reason: None, } }) .collect() diff --git a/codex-rs/tui/src/bottom_pane/file_search_popup.rs b/codex-rs/tui/src/bottom_pane/file_search_popup.rs index 064e4f013..48de1cff5 100644 --- a/codex-rs/tui/src/bottom_pane/file_search_popup.rs +++ b/codex-rs/tui/src/bottom_pane/file_search_popup.rs @@ -132,6 +132,7 @@ impl WidgetRef for &FileSearchPopup { display_shortcut: None, description: None, wrap_indent: None, + disabled_reason: None, }) .collect() }; diff --git a/codex-rs/tui/src/bottom_pane/list_selection_view.rs b/codex-rs/tui/src/bottom_pane/list_selection_view.rs index 46d6daac6..e387b6686 100644 --- a/codex-rs/tui/src/bottom_pane/list_selection_view.rs +++ b/codex-rs/tui/src/bottom_pane/list_selection_view.rs @@ -44,6 +44,7 @@ pub(crate) struct SelectionItem { pub actions: Vec, pub dismiss_on_select: bool, pub search_value: Option, + pub disabled_reason: Option, } pub(crate) struct SelectionViewParams { @@ -217,6 +218,7 @@ impl ListSelectionView { match_indices: None, description, wrap_indent, + disabled_reason: item.disabled_reason.clone(), } }) }) @@ -228,6 +230,7 @@ impl ListSelectionView { self.state.move_up_wrap(len); let visible = Self::max_visible_rows(len); self.state.ensure_visible(len, visible); + self.skip_disabled_up(); } fn move_down(&mut self) { @@ -235,12 +238,14 @@ impl ListSelectionView { self.state.move_down_wrap(len); let visible = Self::max_visible_rows(len); self.state.ensure_visible(len, visible); + self.skip_disabled_down(); } fn accept(&mut self) { if let Some(idx) = self.state.selected_idx && let Some(actual_idx) = self.filtered_indices.get(idx) && let Some(item) = self.items.get(*actual_idx) + && item.disabled_reason.is_none() { self.last_selected_actual_idx = Some(*actual_idx); for act in &item.actions { @@ -267,6 +272,40 @@ impl ListSelectionView { fn rows_width(total_width: u16) -> u16 { total_width.saturating_sub(2) } + + fn skip_disabled_down(&mut self) { + let len = self.visible_len(); + for _ in 0..len { + if let Some(idx) = self.state.selected_idx + && let Some(actual_idx) = self.filtered_indices.get(idx) + && self + .items + .get(*actual_idx) + .is_some_and(|item| item.disabled_reason.is_some()) + { + self.state.move_down_wrap(len); + } else { + break; + } + } + } + + fn skip_disabled_up(&mut self) { + let len = self.visible_len(); + for _ in 0..len { + if let Some(idx) = self.state.selected_idx + && let Some(actual_idx) = self.filtered_indices.get(idx) + && self + .items + .get(*actual_idx) + .is_some_and(|item| item.disabled_reason.is_some()) + { + self.state.move_up_wrap(len); + } else { + break; + } + } + } } impl BottomPaneView for ListSelectionView { @@ -348,6 +387,10 @@ impl BottomPaneView for ListSelectionView { .map(|d| d as usize) .and_then(|d| d.checked_sub(1)) && idx < self.items.len() + && self + .items + .get(idx) + .is_some_and(|item| item.disabled_reason.is_none()) { self.state.selected_idx = Some(idx); self.accept(); diff --git a/codex-rs/tui/src/bottom_pane/selection_popup_common.rs b/codex-rs/tui/src/bottom_pane/selection_popup_common.rs index 5107ab0ca..af7fbb5d1 100644 --- a/codex-rs/tui/src/bottom_pane/selection_popup_common.rs +++ b/codex-rs/tui/src/bottom_pane/selection_popup_common.rs @@ -20,6 +20,7 @@ pub(crate) struct GenericDisplayRow { pub display_shortcut: Option, pub match_indices: Option>, // indices to bold (char positions) pub description: Option, // optional grey text after the name + pub disabled_reason: Option, // optional disabled message pub wrap_indent: Option, // optional indent for wrapped lines } @@ -37,7 +38,13 @@ fn compute_desc_col( .iter() .enumerate() .filter(|(i, _)| visible_range.contains(i)) - .map(|(_, r)| Line::from(r.name.clone()).width()) + .map(|(_, r)| { + let mut spans: Vec = vec![r.name.clone().into()]; + if r.disabled_reason.is_some() { + spans.push(" (disabled)".dim()); + } + Line::from(spans).width() + }) .max() .unwrap_or(0); let mut desc_col = max_name_width.saturating_add(2); @@ -51,7 +58,7 @@ fn compute_desc_col( fn wrap_indent(row: &GenericDisplayRow, desc_col: usize, max_width: u16) -> usize { let max_indent = max_width.saturating_sub(1) as usize; let indent = row.wrap_indent.unwrap_or_else(|| { - if row.description.is_some() { + if row.description.is_some() || row.disabled_reason.is_some() { desc_col } else { 0 @@ -64,10 +71,16 @@ fn wrap_indent(row: &GenericDisplayRow, desc_col: usize, max_width: u16) -> usiz /// at `desc_col`. Applies fuzzy-match bolding when indices are present and /// dims the description. fn build_full_line(row: &GenericDisplayRow, desc_col: usize) -> Line<'static> { + let combined_description = match (&row.description, &row.disabled_reason) { + (Some(desc), Some(reason)) => Some(format!("{desc} (disabled: {reason})")), + (Some(desc), None) => Some(desc.clone()), + (None, Some(reason)) => Some(format!("disabled: {reason}")), + (None, None) => None, + }; + // Enforce single-line name: allow at most desc_col - 2 cells for name, // reserving two spaces before the description column. - let name_limit = row - .description + let name_limit = combined_description .as_ref() .map(|_| desc_col.saturating_sub(2)) .unwrap_or(usize::MAX); @@ -113,6 +126,10 @@ fn build_full_line(row: &GenericDisplayRow, desc_col: usize) -> Line<'static> { name_spans.push("…".into()); } + if row.disabled_reason.is_some() { + name_spans.push(" (disabled)".dim()); + } + let this_name_width = Line::from(name_spans.clone()).width(); let mut full_spans: Vec = name_spans; if let Some(display_shortcut) = row.display_shortcut { @@ -120,7 +137,7 @@ fn build_full_line(row: &GenericDisplayRow, desc_col: usize) -> Line<'static> { full_spans.push(display_shortcut.into()); full_spans.push(")".into()); } - if let Some(desc) = row.description.as_ref() { + if let Some(desc) = combined_description.as_ref() { let gap = desc_col.saturating_sub(this_name_width); if gap > 0 { full_spans.push(" ".repeat(gap).into()); diff --git a/codex-rs/tui/src/bottom_pane/skill_popup.rs b/codex-rs/tui/src/bottom_pane/skill_popup.rs index 3e0f79f84..2e1e5878c 100644 --- a/codex-rs/tui/src/bottom_pane/skill_popup.rs +++ b/codex-rs/tui/src/bottom_pane/skill_popup.rs @@ -92,6 +92,7 @@ impl SkillPopup { match_indices: indices, display_shortcut: None, description: Some(description), + disabled_reason: None, wrap_indent: None, } }) diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 464d3441f..89d2fa7ac 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -2556,7 +2556,7 @@ impl ChatWidget { /// Open a popup to choose the approvals mode (ask for approval policy + sandbox policy). pub(crate) fn open_approvals_popup(&mut self) { - let current_approval = self.config.approval_policy; + let current_approval = self.config.approval_policy.value(); let current_sandbox = self.config.sandbox_policy.clone(); let mut items: Vec = Vec::new(); let presets: Vec = builtin_approval_presets(); @@ -2564,8 +2564,11 @@ impl ChatWidget { let is_current = Self::preset_matches_current(current_approval, ¤t_sandbox, &preset); let name = preset.label.to_string(); - let description_text = preset.description; - let description = Some(description_text.to_string()); + let description = Some(preset.description.to_string()); + let disabled_reason = match self.config.approval_policy.can_set(&preset.approval) { + Ok(()) => None, + Err(err) => Some(err.to_string()), + }; let requires_confirmation = preset.id == "full-access" && !self .config @@ -2618,6 +2621,7 @@ impl ChatWidget { is_current, actions, dismiss_on_select: true, + disabled_reason, ..Default::default() }); } @@ -2954,7 +2958,9 @@ impl ChatWidget { /// Set the approval policy in the widget's config copy. pub(crate) fn set_approval_policy(&mut self, policy: AskForApproval) { - self.config.approval_policy = policy; + if let Err(err) = self.config.approval_policy.set(policy) { + tracing::warn!(%err, "failed to set approval_policy on chat config"); + } } /// Set the sandbox policy in the widget's config copy. diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 35771301d..49b4a1efa 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -10,6 +10,8 @@ use codex_core::CodexAuth; use codex_core::config::Config; use codex_core::config::ConfigOverrides; use codex_core::config::ConfigToml; +use codex_core::config::Constrained; +use codex_core::config::ConstraintError; use codex_core::openai_models::models_manager::ModelsManager; use codex_core::protocol::AgentMessageDeltaEvent; use codex_core::protocol::AgentMessageEvent; @@ -2039,17 +2041,125 @@ fn disabled_slash_command_while_task_running_snapshot() { assert_snapshot!(blob); } +#[test] +fn approvals_popup_shows_disabled_presets() { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None); + + chat.config.approval_policy = + Constrained::new(AskForApproval::OnRequest, |candidate| match candidate { + AskForApproval::OnRequest => Ok(()), + _ => Err(ConstraintError { + message: "this message should be printed in the description".to_string(), + }), + }) + .expect("construct constrained approval policy"); + + chat.open_approvals_popup(); + + let width = 80; + let height = chat.desired_height(width); + let mut terminal = + ratatui::Terminal::new(VT100Backend::new(width, height)).expect("create terminal"); + terminal.set_viewport_area(Rect::new(0, 0, width, height)); + terminal + .draw(|f| chat.render(f.area(), f.buffer_mut())) + .expect("render approvals popup"); + + let screen = terminal.backend().vt100().screen().contents(); + let collapsed = screen.split_whitespace().collect::>().join(" "); + assert!( + collapsed.contains("(disabled)"), + "disabled preset label should be shown" + ); + assert!( + collapsed.contains("this message should be printed in the description"), + "disabled preset reason should be shown" + ); +} + +#[test] +fn approvals_popup_navigation_skips_disabled() { + let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(None); + + chat.config.approval_policy = + Constrained::new(AskForApproval::OnRequest, |candidate| match candidate { + AskForApproval::OnRequest => Ok(()), + _ => Err(ConstraintError { + message: "disabled preset".to_string(), + }), + }) + .expect("construct constrained approval policy"); + + chat.open_approvals_popup(); + + // The approvals popup is the active bottom-pane view; drive navigation via chat handle_key_event. + // Start selected at idx 0 (enabled), move down twice; the disabled option should be skipped + // and selection should wrap back to idx 0 (also enabled). + chat.handle_key_event(KeyEvent::from(KeyCode::Down)); + chat.handle_key_event(KeyEvent::from(KeyCode::Down)); + + // Press numeric shortcut for the disabled row (3 => idx 2); should not close or accept. + chat.handle_key_event(KeyEvent::from(KeyCode::Char('3'))); + + // Ensure the popup remains open and no selection actions were sent. + let width = 80; + let height = chat.desired_height(width); + let mut terminal = + ratatui::Terminal::new(VT100Backend::new(width, height)).expect("create terminal"); + terminal.set_viewport_area(Rect::new(0, 0, width, height)); + terminal + .draw(|f| chat.render(f.area(), f.buffer_mut())) + .expect("render approvals popup after disabled selection"); + let screen = terminal.backend().vt100().screen().contents(); + assert!( + screen.contains("Select Approval Mode"), + "popup should remain open after selecting a disabled entry" + ); + assert!( + op_rx.try_recv().is_err(), + "no actions should be dispatched yet" + ); + assert!(rx.try_recv().is_err(), "no history should be emitted"); + + // Press Enter; selection should land on an enabled preset and dispatch updates. + chat.handle_key_event(KeyEvent::from(KeyCode::Enter)); + let mut app_events = Vec::new(); + while let Ok(ev) = rx.try_recv() { + app_events.push(ev); + } + assert!( + app_events.iter().any(|ev| matches!( + ev, + AppEvent::CodexOp(Op::OverrideTurnContext { + approval_policy: Some(AskForApproval::OnRequest), + .. + }) + )), + "enter should select an enabled preset" + ); + assert!( + !app_events.iter().any(|ev| matches!( + ev, + AppEvent::CodexOp(Op::OverrideTurnContext { + approval_policy: Some(AskForApproval::Never), + .. + }) + )), + "disabled preset should not be selected" + ); +} + // // Snapshot test: command approval modal // // Synthesizes a Codex ExecApprovalRequest event to trigger the approval modal // and snapshots the visual output using the ratatui TestBackend. #[test] -fn approval_modal_exec_snapshot() { +fn approval_modal_exec_snapshot() -> anyhow::Result<()> { // Build a chat widget with manual channels to avoid spawning the agent. let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None); // Ensure policy allows surfacing approvals explicitly (not strictly required for direct event). - chat.config.approval_policy = AskForApproval::OnRequest; + chat.config.approval_policy.set(AskForApproval::OnRequest)?; // Inject an exec approval request to display the approval modal. let ev = ExecApprovalRequestEvent { call_id: "call-approve-cmd".into(), @@ -2095,14 +2205,16 @@ fn approval_modal_exec_snapshot() { "approval_modal_exec", terminal.backend().vt100().screen().contents() ); + + Ok(()) } // Snapshot test: command approval modal without a reason // Ensures spacing looks correct when no reason text is provided. #[test] -fn approval_modal_exec_without_reason_snapshot() { +fn approval_modal_exec_without_reason_snapshot() -> anyhow::Result<()> { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None); - chat.config.approval_policy = AskForApproval::OnRequest; + chat.config.approval_policy.set(AskForApproval::OnRequest)?; let ev = ExecApprovalRequestEvent { call_id: "call-approve-cmd-noreason".into(), @@ -2134,13 +2246,15 @@ fn approval_modal_exec_without_reason_snapshot() { "approval_modal_exec_no_reason", terminal.backend().vt100().screen().contents() ); + + Ok(()) } // Snapshot test: patch approval modal #[test] -fn approval_modal_patch_snapshot() { +fn approval_modal_patch_snapshot() -> anyhow::Result<()> { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None); - chat.config.approval_policy = AskForApproval::OnRequest; + chat.config.approval_policy.set(AskForApproval::OnRequest)?; // Build a small changeset and a reason/grant_root to exercise the prompt text. let mut changes = HashMap::new(); @@ -2174,6 +2288,8 @@ fn approval_modal_patch_snapshot() { "approval_modal_patch", terminal.backend().vt100().screen().contents() ); + + Ok(()) } #[test] @@ -2736,10 +2852,10 @@ fn apply_patch_full_flow_integration_like() { } #[test] -fn apply_patch_untrusted_shows_approval_modal() { +fn apply_patch_untrusted_shows_approval_modal() -> anyhow::Result<()> { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None); // Ensure approval policy is untrusted (OnRequest) - chat.config.approval_policy = AskForApproval::OnRequest; + chat.config.approval_policy.set(AskForApproval::OnRequest)?; // Simulate a patch approval request from backend let mut changes = HashMap::new(); @@ -2778,14 +2894,16 @@ fn apply_patch_untrusted_shows_approval_modal() { contains_title, "expected approval modal to be visible with title 'Would you like to make the following edits?'" ); + + Ok(()) } #[test] -fn apply_patch_request_shows_diff_summary() { +fn apply_patch_request_shows_diff_summary() -> anyhow::Result<()> { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None); // Ensure we are in OnRequest so an approval is surfaced - chat.config.approval_policy = AskForApproval::OnRequest; + chat.config.approval_policy.set(AskForApproval::OnRequest)?; // Simulate backend asking to apply a patch adding two lines to README.md let mut changes = HashMap::new(); @@ -2844,6 +2962,8 @@ fn apply_patch_request_shows_diff_summary() { saw_line1 && saw_line2, "expected modal to show per-line diff summary" ); + + Ok(()) } #[test] diff --git a/codex-rs/tui/src/test_backend.rs b/codex-rs/tui/src/test_backend.rs index a5460af2e..6e340fa20 100644 --- a/codex-rs/tui/src/test_backend.rs +++ b/codex-rs/tui/src/test_backend.rs @@ -25,6 +25,7 @@ pub struct VT100Backend { impl VT100Backend { /// Creates a new `TestBackend` with the specified width and height. pub fn new(width: u16, height: u16) -> Self { + crossterm::style::force_color_output(true); Self { crossterm_backend: CrosstermBackend::new(vt100::Parser::new(height, width, 0)), } diff --git a/codex-rs/tui2/src/chatwidget.rs b/codex-rs/tui2/src/chatwidget.rs index ece866b07..71a3014d1 100644 --- a/codex-rs/tui2/src/chatwidget.rs +++ b/codex-rs/tui2/src/chatwidget.rs @@ -2556,7 +2556,7 @@ impl ChatWidget { /// Open a popup to choose the approvals mode (ask for approval policy + sandbox policy). pub(crate) fn open_approvals_popup(&mut self) { - let current_approval = self.config.approval_policy; + let current_approval = self.config.approval_policy.value(); let current_sandbox = self.config.sandbox_policy.clone(); let mut items: Vec = Vec::new(); let presets: Vec = builtin_approval_presets(); @@ -2954,7 +2954,9 @@ impl ChatWidget { /// Set the approval policy in the widget's config copy. pub(crate) fn set_approval_policy(&mut self, policy: AskForApproval) { - self.config.approval_policy = policy; + if let Err(err) = self.config.approval_policy.set(policy) { + tracing::warn!(%err, "failed to set approval_policy on chat config"); + } } /// Set the sandbox policy in the widget's config copy. diff --git a/codex-rs/tui2/src/chatwidget/tests.rs b/codex-rs/tui2/src/chatwidget/tests.rs index 35771301d..15517cea2 100644 --- a/codex-rs/tui2/src/chatwidget/tests.rs +++ b/codex-rs/tui2/src/chatwidget/tests.rs @@ -10,6 +10,7 @@ use codex_core::CodexAuth; use codex_core::config::Config; use codex_core::config::ConfigOverrides; use codex_core::config::ConfigToml; +use codex_core::config::Constrained; use codex_core::openai_models::models_manager::ModelsManager; use codex_core::protocol::AgentMessageDeltaEvent; use codex_core::protocol::AgentMessageEvent; @@ -2049,7 +2050,7 @@ fn approval_modal_exec_snapshot() { // Build a chat widget with manual channels to avoid spawning the agent. let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None); // Ensure policy allows surfacing approvals explicitly (not strictly required for direct event). - chat.config.approval_policy = AskForApproval::OnRequest; + chat.config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); // Inject an exec approval request to display the approval modal. let ev = ExecApprovalRequestEvent { call_id: "call-approve-cmd".into(), @@ -2102,7 +2103,7 @@ fn approval_modal_exec_snapshot() { #[test] fn approval_modal_exec_without_reason_snapshot() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None); - chat.config.approval_policy = AskForApproval::OnRequest; + chat.config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); let ev = ExecApprovalRequestEvent { call_id: "call-approve-cmd-noreason".into(), @@ -2140,7 +2141,7 @@ fn approval_modal_exec_without_reason_snapshot() { #[test] fn approval_modal_patch_snapshot() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None); - chat.config.approval_policy = AskForApproval::OnRequest; + chat.config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); // Build a small changeset and a reason/grant_root to exercise the prompt text. let mut changes = HashMap::new(); @@ -2739,7 +2740,7 @@ fn apply_patch_full_flow_integration_like() { fn apply_patch_untrusted_shows_approval_modal() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None); // Ensure approval policy is untrusted (OnRequest) - chat.config.approval_policy = AskForApproval::OnRequest; + chat.config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); // Simulate a patch approval request from backend let mut changes = HashMap::new(); @@ -2785,7 +2786,7 @@ fn apply_patch_request_shows_diff_summary() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None); // Ensure we are in OnRequest so an approval is surfaced - chat.config.approval_policy = AskForApproval::OnRequest; + chat.config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); // Simulate backend asking to apply a patch adding two lines to README.md let mut changes = HashMap::new();