From 31d9b6f4d2033438d3375d4c3fbadcd5c08660d9 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Tue, 13 Jan 2026 18:57:09 -0700 Subject: [PATCH] Improve handling of config and rules errors for app server clients (#9182) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When an invalid config.toml key or value is detected, the CLI currently just quits. This leaves the VSCE in a dead state. This PR changes the behavior to not quit and bubble up the config error to users to make it actionable. It also surfaces errors related to "rules" parsing. This allows us to surface these errors to users in the VSCE, like this: Screenshot 2026-01-13 at 4 29 22 PM Screenshot 2026-01-13 at 4 45 06 PM --- .../src/protocol/common.rs | 1 + .../app-server-protocol/src/protocol/v2.rs | 10 +++++ codex-rs/app-server/src/lib.rs | 42 ++++++++++++++++-- codex-rs/app-server/src/message_processor.rs | 15 +++++++ codex-rs/app-server/src/outgoing_message.rs | 23 ++++++++++ codex-rs/common/src/lib.rs | 2 +- codex-rs/core/src/codex.rs | 2 +- codex-rs/core/src/config/mod.rs | 22 ++++++++++ codex-rs/core/src/config_loader/mod.rs | 1 + codex-rs/core/src/config_loader/overrides.rs | 4 +- codex-rs/core/src/exec_policy.rs | 44 +++++++++++++------ codex-rs/core/src/lib.rs | 1 + codex-rs/exec/src/lib.rs | 18 ++++---- codex-rs/tui/src/lib.rs | 26 +++++++---- .../tui/tests/suite/no_panic_on_startup.rs | 8 ++-- codex-rs/tui2/src/lib.rs | 26 +++++++---- .../tui2/tests/suite/no_panic_on_startup.rs | 8 ++-- 17 files changed, 197 insertions(+), 56 deletions(-) diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index 59d073ea8..dd54eb25d 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -567,6 +567,7 @@ server_notification_definitions! { ReasoningTextDelta => "item/reasoning/textDelta" (v2::ReasoningTextDeltaNotification), ContextCompacted => "thread/compacted" (v2::ContextCompactedNotification), DeprecationNotice => "deprecationNotice" (v2::DeprecationNoticeNotification), + ConfigWarning => "configWarning" (v2::ConfigWarningNotification), /// Notifies the user of world-writable directories on Windows, which cannot be protected by the sandbox. WindowsWorldWritableWarning => "windows/worldWritableWarning" (v2::WindowsWorldWritableWarningNotification), diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 30505cf06..5441a8a6d 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -2107,6 +2107,16 @@ pub struct DeprecationNoticeNotification { pub details: Option, } +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct ConfigWarningNotification { + /// Concise summary of the warning. + pub summary: String, + /// Optional extra guidance or error details. + pub details: Option, +} + #[cfg(test)] mod tests { use super::*; diff --git a/codex-rs/app-server/src/lib.rs b/codex-rs/app-server/src/lib.rs index d9aaabd1c..fef98fc97 100644 --- a/codex-rs/app-server/src/lib.rs +++ b/codex-rs/app-server/src/lib.rs @@ -1,6 +1,7 @@ #![deny(clippy::print_stdout, clippy::print_stderr)] use codex_common::CliConfigOverrides; +use codex_core::config::Config; use codex_core::config::ConfigBuilder; use codex_core::config_loader::LoaderOverrides; use std::io::ErrorKind; @@ -10,7 +11,9 @@ use std::path::PathBuf; use crate::message_processor::MessageProcessor; use crate::outgoing_message::OutgoingMessage; use crate::outgoing_message::OutgoingMessageSender; +use codex_app_server_protocol::ConfigWarningNotification; use codex_app_server_protocol::JSONRPCMessage; +use codex_core::check_execpolicy_for_warnings; use codex_feedback::CodexFeedback; use tokio::io::AsyncBufReadExt; use tokio::io::AsyncWriteExt; @@ -82,14 +85,38 @@ pub async fn run_main( ) })?; let loader_overrides_for_config_api = loader_overrides.clone(); - let config = ConfigBuilder::default() + let mut config_warnings = Vec::new(); + let config = match ConfigBuilder::default() .cli_overrides(cli_kv_overrides.clone()) .loader_overrides(loader_overrides) .build() .await - .map_err(|e| { - std::io::Error::new(ErrorKind::InvalidData, format!("error loading config: {e}")) - })?; + { + Ok(config) => config, + Err(err) => { + let message = ConfigWarningNotification { + summary: "Invalid configuration; using defaults.".to_string(), + details: Some(err.to_string()), + }; + config_warnings.push(message); + Config::load_default_with_cli_overrides(cli_kv_overrides.clone()).map_err(|e| { + std::io::Error::new( + ErrorKind::InvalidData, + format!("error loading default config after config error: {e}"), + ) + })? + } + }; + + if let Ok(Some(err)) = + check_execpolicy_for_warnings(&config.features, &config.config_layer_stack).await + { + let message = ConfigWarningNotification { + summary: "Error parsing rules; custom rules not applied.".to_string(), + details: Some(err.to_string()), + }; + config_warnings.push(message); + } let feedback = CodexFeedback::new(); @@ -127,6 +154,12 @@ pub async fn run_main( .with(otel_logger_layer) .with(otel_tracing_layer) .try_init(); + for warning in &config_warnings { + match &warning.details { + Some(details) => error!("{} {}", warning.summary, details), + None => error!("{}", warning.summary), + } + } // Task: process incoming messages. let processor_handle = tokio::spawn({ @@ -140,6 +173,7 @@ pub async fn run_main( cli_overrides, loader_overrides, feedback.clone(), + config_warnings, ); async move { while let Some(msg) = incoming_rx.recv().await { diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index 1f442b995..338428db1 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -10,6 +10,7 @@ use codex_app_server_protocol::ClientRequest; use codex_app_server_protocol::ConfigBatchWriteParams; use codex_app_server_protocol::ConfigReadParams; use codex_app_server_protocol::ConfigValueWriteParams; +use codex_app_server_protocol::ConfigWarningNotification; use codex_app_server_protocol::InitializeResponse; use codex_app_server_protocol::JSONRPCError; use codex_app_server_protocol::JSONRPCErrorError; @@ -17,6 +18,7 @@ use codex_app_server_protocol::JSONRPCNotification; use codex_app_server_protocol::JSONRPCRequest; use codex_app_server_protocol::JSONRPCResponse; use codex_app_server_protocol::RequestId; +use codex_app_server_protocol::ServerNotification; use codex_core::AuthManager; use codex_core::ThreadManager; use codex_core::config::Config; @@ -34,6 +36,7 @@ pub(crate) struct MessageProcessor { codex_message_processor: CodexMessageProcessor, config_api: ConfigApi, initialized: bool, + config_warnings: Vec, } impl MessageProcessor { @@ -46,6 +49,7 @@ impl MessageProcessor { cli_overrides: Vec<(String, TomlValue)>, loader_overrides: LoaderOverrides, feedback: CodexFeedback, + config_warnings: Vec, ) -> Self { let outgoing = Arc::new(outgoing); let auth_manager = AuthManager::shared( @@ -74,6 +78,7 @@ impl MessageProcessor { codex_message_processor, config_api, initialized: false, + config_warnings, } } @@ -155,6 +160,16 @@ impl MessageProcessor { self.initialized = true; + if !self.config_warnings.is_empty() { + for notification in self.config_warnings.drain(..) { + self.outgoing + .send_server_notification(ServerNotification::ConfigWarning( + notification, + )) + .await; + } + } + return; } } diff --git a/codex-rs/app-server/src/outgoing_message.rs b/codex-rs/app-server/src/outgoing_message.rs index 83ac26fd4..cf720ef83 100644 --- a/codex-rs/app-server/src/outgoing_message.rs +++ b/codex-rs/app-server/src/outgoing_message.rs @@ -162,6 +162,7 @@ mod tests { use codex_app_server_protocol::AccountRateLimitsUpdatedNotification; use codex_app_server_protocol::AccountUpdatedNotification; use codex_app_server_protocol::AuthMode; + use codex_app_server_protocol::ConfigWarningNotification; use codex_app_server_protocol::LoginChatGptCompleteNotification; use codex_app_server_protocol::RateLimitSnapshot; use codex_app_server_protocol::RateLimitWindow; @@ -279,4 +280,26 @@ mod tests { "ensure the notification serializes correctly" ); } + + #[test] + fn verify_config_warning_notification_serialization() { + let notification = ServerNotification::ConfigWarning(ConfigWarningNotification { + summary: "Config error: using defaults".to_string(), + details: Some("error loading config: bad config".to_string()), + }); + + let jsonrpc_notification = OutgoingMessage::AppServerNotification(notification); + assert_eq!( + json!( { + "method": "configWarning", + "params": { + "summary": "Config error: using defaults", + "details": "error loading config: bad config", + }, + }), + serde_json::to_value(jsonrpc_notification) + .expect("ensure the notification serializes correctly"), + "ensure the notification serializes correctly" + ); + } } diff --git a/codex-rs/common/src/lib.rs b/codex-rs/common/src/lib.rs index d5513b832..20c22c684 100644 --- a/codex-rs/common/src/lib.rs +++ b/codex-rs/common/src/lib.rs @@ -16,7 +16,7 @@ pub use sandbox_mode_cli_arg::SandboxModeCliArg; #[cfg(feature = "cli")] pub mod format_env_display; -#[cfg(any(feature = "cli", test))] +#[cfg(feature = "cli")] mod config_override; #[cfg(feature = "cli")] diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 564181fe3..5cf79bd90 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -251,7 +251,7 @@ impl Codex { let exec_policy = ExecPolicyManager::load(&config.features, &config.config_layer_stack) .await - .map_err(|err| CodexErr::Fatal(format!("failed to load execpolicy: {err}")))?; + .map_err(|err| CodexErr::Fatal(format!("failed to load rules: {err}")))?; let config = Arc::new(config); let _ = models_manager diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index fa1fee6c0..f4961cc6b 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -460,6 +460,28 @@ impl Config { .await } + /// Load a default configuration when user config files are invalid. + pub fn load_default_with_cli_overrides( + cli_overrides: Vec<(String, TomlValue)>, + ) -> std::io::Result { + let codex_home = find_codex_home()?; + let mut merged = toml::Value::try_from(ConfigToml::default()).map_err(|e| { + std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!("failed to serialize default config: {e}"), + ) + })?; + let cli_layer = crate::config_loader::build_cli_overrides_layer(&cli_overrides); + crate::config_loader::merge_toml_values(&mut merged, &cli_layer); + let config_toml = deserialize_config_toml_with_base(merged, &codex_home)?; + Self::load_config_with_layer_stack( + config_toml, + ConfigOverrides::default(), + codex_home, + ConfigLayerStack::default(), + ) + } + /// This is a secondary way of creating [Config], which is appropriate when /// the harness is meant to be used with a specific configuration that /// ignores user settings. For example, the `codex exec` subcommand is diff --git a/codex-rs/core/src/config_loader/mod.rs b/codex-rs/core/src/config_loader/mod.rs index a793aa223..7e6d4223c 100644 --- a/codex-rs/core/src/config_loader/mod.rs +++ b/codex-rs/core/src/config_loader/mod.rs @@ -31,6 +31,7 @@ pub use config_requirements::McpServerRequirement; pub use config_requirements::RequirementSource; pub use config_requirements::SandboxModeRequirement; pub use merge::merge_toml_values; +pub(crate) use overrides::build_cli_overrides_layer; pub use state::ConfigLayerEntry; pub use state::ConfigLayerStack; pub use state::ConfigLayerStackOrdering; diff --git a/codex-rs/core/src/config_loader/overrides.rs b/codex-rs/core/src/config_loader/overrides.rs index e2ae6375a..a9fe8eff9 100644 --- a/codex-rs/core/src/config_loader/overrides.rs +++ b/codex-rs/core/src/config_loader/overrides.rs @@ -1,10 +1,10 @@ use toml::Value as TomlValue; -pub(super) fn default_empty_table() -> TomlValue { +pub(crate) fn default_empty_table() -> TomlValue { TomlValue::Table(Default::default()) } -pub(super) fn build_cli_overrides_layer(cli_overrides: &[(String, TomlValue)]) -> TomlValue { +pub(crate) fn build_cli_overrides_layer(cli_overrides: &[(String, TomlValue)]) -> TomlValue { let mut root = default_empty_table(); for (path, value) in cli_overrides { apply_toml_override(&mut root, path, value.clone()); diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index b057035bc..e3a6751b5 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -46,19 +46,19 @@ fn is_policy_match(rule_match: &RuleMatch) -> bool { #[derive(Debug, Error)] pub enum ExecPolicyError { - #[error("failed to read execpolicy files from {dir}: {source}")] + #[error("failed to read rules files from {dir}: {source}")] ReadDir { dir: PathBuf, source: std::io::Error, }, - #[error("failed to read execpolicy file {path}: {source}")] + #[error("failed to read rules file {path}: {source}")] ReadFile { path: PathBuf, source: std::io::Error, }, - #[error("failed to parse execpolicy file {path}: {source}")] + #[error("failed to parse rules file {path}: {source}")] ParsePolicy { path: String, source: codex_execpolicy::Error, @@ -67,19 +67,19 @@ pub enum ExecPolicyError { #[derive(Debug, Error)] pub enum ExecPolicyUpdateError { - #[error("failed to update execpolicy file {path}: {source}")] + #[error("failed to update rules file {path}: {source}")] AppendRule { path: PathBuf, source: AmendError }, - #[error("failed to join blocking execpolicy update task: {source}")] + #[error("failed to join blocking rules update task: {source}")] JoinBlockingTask { source: tokio::task::JoinError }, - #[error("failed to update in-memory execpolicy: {source}")] + #[error("failed to update in-memory rules: {source}")] AddRule { #[from] source: ExecPolicyRuleError, }, - #[error("cannot append execpolicy rule because execpolicy feature is disabled")] + #[error("cannot append rule because rules feature is disabled")] FeatureDisabled, } @@ -98,7 +98,11 @@ impl ExecPolicyManager { features: &Features, config_stack: &ConfigLayerStack, ) -> Result { - let policy = load_exec_policy_for_features(features, config_stack).await?; + let (policy, warning) = + load_exec_policy_for_features_with_warning(features, config_stack).await?; + if let Some(err) = warning.as_ref() { + tracing::warn!("failed to parse rules: {err}"); + } Ok(Self::new(Arc::new(policy))) } @@ -195,14 +199,26 @@ impl Default for ExecPolicyManager { } } -async fn load_exec_policy_for_features( +pub async fn check_execpolicy_for_warnings( features: &Features, config_stack: &ConfigLayerStack, -) -> Result { +) -> Result, ExecPolicyError> { + let (_, warning) = load_exec_policy_for_features_with_warning(features, config_stack).await?; + Ok(warning) +} + +async fn load_exec_policy_for_features_with_warning( + features: &Features, + config_stack: &ConfigLayerStack, +) -> Result<(Policy, Option), ExecPolicyError> { if !features.enabled(Feature::ExecPolicy) { - Ok(Policy::empty()) - } else { - load_exec_policy(config_stack).await + return Ok((Policy::empty(), None)); + } + + match load_exec_policy(config_stack).await { + Ok(policy) => Ok((policy, None)), + Err(err @ ExecPolicyError::ParsePolicy { .. }) => Ok((Policy::empty(), Some(err))), + Err(err) => Err(err), } } @@ -239,7 +255,7 @@ pub async fn load_exec_policy(config_stack: &ConfigLayerStack) -> Result) -> any } }; - let otel = - codex_core::otel_init::build_provider(&config, env!("CARGO_PKG_VERSION"), None, false); - - #[allow(clippy::print_stderr)] - let otel = match otel { - Ok(otel) => otel, - Err(e) => { + let otel = match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + codex_core::otel_init::build_provider(&config, env!("CARGO_PKG_VERSION"), None, false) + })) { + Ok(Ok(otel)) => otel, + Ok(Err(e)) => { eprintln!("Could not create otel exporter: {e}"); - std::process::exit(1); + None + } + Err(_) => { + eprintln!("Could not create otel exporter: panicked during initialization"); + None } }; diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 115c9c030..519ceeb12 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -301,15 +301,23 @@ pub async fn run_main( ensure_oss_provider_ready(provider_id, &config).await?; } - let otel = - codex_core::otel_init::build_provider(&config, env!("CARGO_PKG_VERSION"), None, true); - - #[allow(clippy::print_stderr)] - let otel = match otel { - Ok(otel) => otel, - Err(e) => { - eprintln!("Could not create otel exporter: {e}"); - std::process::exit(1); + let otel = match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + codex_core::otel_init::build_provider(&config, env!("CARGO_PKG_VERSION"), None, true) + })) { + Ok(Ok(otel)) => otel, + Ok(Err(e)) => { + #[allow(clippy::print_stderr)] + { + eprintln!("Could not create otel exporter: {e}"); + } + None + } + Err(_) => { + #[allow(clippy::print_stderr)] + { + eprintln!("Could not create otel exporter: panicked during initialization"); + } + None } }; diff --git a/codex-rs/tui/tests/suite/no_panic_on_startup.rs b/codex-rs/tui/tests/suite/no_panic_on_startup.rs index e9cd55ef3..eade57d08 100644 --- a/codex-rs/tui/tests/suite/no_panic_on_startup.rs +++ b/codex-rs/tui/tests/suite/no_panic_on_startup.rs @@ -35,14 +35,14 @@ model_provider = "ollama" std::fs::write(codex_home.join("config.toml"), config_contents)?; let CodexCliOutput { exit_code, output } = run_codex_cli(codex_home, cwd).await?; - assert_eq!(1, exit_code, "Codex CLI should exit nonzero."); + assert_ne!(0, exit_code, "Codex CLI should exit nonzero."); assert!( output.contains("ERROR: Failed to initialize codex:"), "expected startup error in output, got: {output}" ); assert!( - output.contains("failed to read execpolicy files"), - "expected execpolicy read error in output, got: {output}" + output.contains("failed to read rules files"), + "expected rules read error in output, got: {output}" ); Ok(()) } @@ -63,7 +63,7 @@ async fn run_codex_cli( codex_home.as_ref().display().to_string(), ); - let args = vec!["-c".to_string(), "analytics_enabled=false".to_string()]; + let args = vec!["-c".to_string(), "analytics.enabled=false".to_string()]; let spawned = codex_utils_pty::spawn_pty_process( codex_cli.to_string_lossy().as_ref(), &args, diff --git a/codex-rs/tui2/src/lib.rs b/codex-rs/tui2/src/lib.rs index 8c81315b4..f7ae3aac5 100644 --- a/codex-rs/tui2/src/lib.rs +++ b/codex-rs/tui2/src/lib.rs @@ -317,15 +317,23 @@ pub async fn run_main( ensure_oss_provider_ready(provider_id, &config).await?; } - let otel = - codex_core::otel_init::build_provider(&config, env!("CARGO_PKG_VERSION"), None, true); - - #[allow(clippy::print_stderr)] - let otel = match otel { - Ok(otel) => otel, - Err(e) => { - eprintln!("Could not create otel exporter: {e}"); - std::process::exit(1); + let otel = match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + codex_core::otel_init::build_provider(&config, env!("CARGO_PKG_VERSION"), None, true) + })) { + Ok(Ok(otel)) => otel, + Ok(Err(e)) => { + #[allow(clippy::print_stderr)] + { + eprintln!("Could not create otel exporter: {e}"); + } + None + } + Err(_) => { + #[allow(clippy::print_stderr)] + { + eprintln!("Could not create otel exporter: panicked during initialization"); + } + None } }; diff --git a/codex-rs/tui2/tests/suite/no_panic_on_startup.rs b/codex-rs/tui2/tests/suite/no_panic_on_startup.rs index e9cd55ef3..eade57d08 100644 --- a/codex-rs/tui2/tests/suite/no_panic_on_startup.rs +++ b/codex-rs/tui2/tests/suite/no_panic_on_startup.rs @@ -35,14 +35,14 @@ model_provider = "ollama" std::fs::write(codex_home.join("config.toml"), config_contents)?; let CodexCliOutput { exit_code, output } = run_codex_cli(codex_home, cwd).await?; - assert_eq!(1, exit_code, "Codex CLI should exit nonzero."); + assert_ne!(0, exit_code, "Codex CLI should exit nonzero."); assert!( output.contains("ERROR: Failed to initialize codex:"), "expected startup error in output, got: {output}" ); assert!( - output.contains("failed to read execpolicy files"), - "expected execpolicy read error in output, got: {output}" + output.contains("failed to read rules files"), + "expected rules read error in output, got: {output}" ); Ok(()) } @@ -63,7 +63,7 @@ async fn run_codex_cli( codex_home.as_ref().display().to_string(), ); - let args = vec!["-c".to_string(), "analytics_enabled=false".to_string()]; + let args = vec!["-c".to_string(), "analytics.enabled=false".to_string()]; let spawned = codex_utils_pty::spawn_pty_process( codex_cli.to_string_lossy().as_ref(), &args,