From 3acd71fedb1284e0f27f400e2d354a942b273421 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Fri, 5 Jun 2026 08:32:07 -0700 Subject: [PATCH] Surface TUI config write error causes (#26537) ## Summary TUI config writes currently wrap app-server failures with local context like `config/batchWrite failed in TUI`, but several user-visible paths only render the outer error. That hides the actionable app-server message, such as validation constraints or read-only `CODEX_HOME` failures, leaving users with a dead-end diagnostic. This change adds a small formatter next to the TUI config write helpers that renders the error source chain, then uses it for model persistence, feature persistence, project trust, status line writes, hook trust, and hook enablement. Fixes #26077 --- codex-rs/tui/src/app/background_requests.rs | 12 ++++-- codex-rs/tui/src/app/config_persistence.rs | 5 ++- codex-rs/tui/src/app/event_dispatch.rs | 11 +++-- ...onfig_error_wraps_in_history_snapshot.snap | 7 ++++ codex-rs/tui/src/chatwidget/tests.rs | 2 + .../chatwidget/tests/config_errors_tests.rs | 25 ++++++++++++ codex-rs/tui/src/config_update.rs | 31 ++++---------- codex-rs/tui/src/config_update_tests.rs | 40 +++++++++++++++++++ .../tui/src/onboarding/onboarding_screen.rs | 4 +- ...sts__renders_snapshot_for_trust_error.snap | 19 +++++++++ .../tui/src/onboarding/trust_directory.rs | 38 +++++++++++++----- ...w__tests__startup_hooks_review_prompt.snap | 18 ++++----- ..._hooks_review_prompt_with_trust_error.snap | 24 ++++++----- codex-rs/tui/src/startup_hooks_review.rs | 17 +++++--- 14 files changed, 185 insertions(+), 68 deletions(-) create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chained_config_error_wraps_in_history_snapshot.snap create mode 100644 codex-rs/tui/src/chatwidget/tests/config_errors_tests.rs create mode 100644 codex-rs/tui/src/config_update_tests.rs create mode 100644 codex-rs/tui/src/onboarding/snapshots/codex_tui__onboarding__trust_directory__tests__renders_snapshot_for_trust_error.snap diff --git a/codex-rs/tui/src/app/background_requests.rs b/codex-rs/tui/src/app/background_requests.rs index d90f511d6..456676f79 100644 --- a/codex-rs/tui/src/app/background_requests.rs +++ b/codex-rs/tui/src/app/background_requests.rs @@ -7,6 +7,7 @@ use super::plugin_mentions::fetch_plugin_mentions; use super::*; use crate::app_event::ConnectorsSnapshot; +use crate::config_update::format_config_error; use codex_app_server_protocol::AppsListParams; use codex_app_server_protocol::AppsListResponse; use codex_app_server_protocol::MarketplaceAddParams; @@ -357,7 +358,12 @@ impl App { let result = write_hook_enabled(request_handle, key, enabled) .await .map(|_| ()) - .map_err(|err| format!("Failed to update hook config: {err}")); + .map_err(|err| { + format!( + "Failed to update hook config: {}", + format_config_error(&err) + ) + }); app_event_tx.send(AppEvent::HookEnabledSet { key: key_for_event, enabled, @@ -378,7 +384,7 @@ impl App { let result = write_hook_trust(request_handle, key, current_hash) .await .map(|_| ()) - .map_err(|err| format!("Failed to trust hook: {err}")); + .map_err(|err| format!("Failed to trust hook: {}", format_config_error(&err))); app_event_tx.send(AppEvent::HookTrusted { result }); }); } @@ -394,7 +400,7 @@ impl App { let result = write_hook_trusts(request_handle, updates) .await .map(|_| ()) - .map_err(|err| format!("Failed to trust hooks: {err}")); + .map_err(|err| format!("Failed to trust hooks: {}", format_config_error(&err))); app_event_tx.send(AppEvent::HookTrusted { result }); }); } diff --git a/codex-rs/tui/src/app/config_persistence.rs b/codex-rs/tui/src/app/config_persistence.rs index 946223ec3..319830b82 100644 --- a/codex-rs/tui/src/app/config_persistence.rs +++ b/codex-rs/tui/src/app/config_persistence.rs @@ -482,9 +482,10 @@ impl App { { Ok(response) => response, Err(err) => { - tracing::error!(error = %err, "failed to persist feature flags"); + let error = crate::config_update::format_config_error(&err); + tracing::error!(error = %error, "failed to persist feature flags"); self.chat_widget - .add_error_message(format!("Failed to update experimental features: {err}")); + .add_error_message(format!("Failed to update experimental features: {error}")); return; } }; diff --git a/codex-rs/tui/src/app/event_dispatch.rs b/codex-rs/tui/src/app/event_dispatch.rs index 58f090e4c..548aa5442 100644 --- a/codex-rs/tui/src/app/event_dispatch.rs +++ b/codex-rs/tui/src/app/event_dispatch.rs @@ -5,6 +5,7 @@ use super::resize_reflow::trailing_run_start; use super::*; +use crate::config_update::format_config_error; #[cfg(target_os = "windows")] use codex_config::types::WindowsSandboxModeToml; @@ -1321,12 +1322,13 @@ impl App { self.chat_widget.add_info_message(message, /*hint*/ None); } Err(err) => { + let error = format_config_error(&err); tracing::error!( - error = %err, + error = %error, "failed to persist model selection" ); self.chat_widget - .add_error_message(format!("Failed to save default model: {err}")); + .add_error_message(format!("Failed to save default model: {error}")); } } } @@ -1945,9 +1947,10 @@ impl App { self.chat_widget.setup_status_line(items, use_theme_colors); } Err(err) => { - tracing::error!(error = %err, "failed to persist status line settings; keeping previous selection"); + let error = format_config_error(&err); + tracing::error!(error = %error, "failed to persist status line settings; keeping previous selection"); self.chat_widget.add_error_message(format!( - "Failed to save status line settings: {err}" + "Failed to save status line settings: {error}" )); } } diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chained_config_error_wraps_in_history_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chained_config_error_wraps_in_history_snapshot.snap new file mode 100644 index 000000000..a3a7e2136 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chained_config_error_wraps_in_history_snapshot.snap @@ -0,0 +1,7 @@ +--- +source: tui/src/chatwidget/tests/config_errors_tests.rs +expression: normalize_snapshot_paths(term.backend().vt100().screen().contents()) +--- +■ Failed to save default model: config/batchWrite failed +in TUI: Invalid configuration: features.fast_mode=true +is not supported; allowed set [fast_mode=false] diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 2da524910..ba89379e7 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -219,6 +219,8 @@ macro_rules! assert_chatwidget_snapshot { mod app_server; mod approval_requests; mod composer_submission; +#[path = "tests/config_errors_tests.rs"] +mod config_errors; mod exec_flow; mod goal_menu; mod goal_validation; diff --git a/codex-rs/tui/src/chatwidget/tests/config_errors_tests.rs b/codex-rs/tui/src/chatwidget/tests/config_errors_tests.rs new file mode 100644 index 000000000..645c01f0f --- /dev/null +++ b/codex-rs/tui/src/chatwidget/tests/config_errors_tests.rs @@ -0,0 +1,25 @@ +use super::*; + +#[tokio::test] +async fn chained_config_error_wraps_in_history_snapshot() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + chat.add_error_message( + "Failed to save default model: config/batchWrite failed in TUI: Invalid configuration: features.fast_mode=true is not supported; allowed set [fast_mode=false]" + .to_string(), + ); + + let width = 56; + let height = 8; + let backend = VT100Backend::new(width, height); + let mut term = crate::custom_terminal::Terminal::with_options(backend).expect("terminal"); + term.set_viewport_area(ratatui::layout::Rect::new(0, 0, width, height)); + for lines in drain_insert_history(&mut rx) { + crate::insert_history::insert_history_lines(&mut term, lines) + .expect("insert history lines"); + } + + assert_chatwidget_snapshot!( + "chained_config_error_wraps_in_history_snapshot", + normalize_snapshot_paths(term.backend().vt100().screen().contents()) + ); +} diff --git a/codex-rs/tui/src/config_update.rs b/codex-rs/tui/src/config_update.rs index a882b2123..a71f34b52 100644 --- a/codex-rs/tui/src/config_update.rs +++ b/codex-rs/tui/src/config_update.rs @@ -23,6 +23,7 @@ use codex_utils_absolute_path::AbsolutePathBuf; use color_eyre::eyre::Result; use color_eyre::eyre::WrapErr; use serde_json::Value as JsonValue; +use std::fmt::Display; use std::path::Path; use uuid::Uuid; @@ -43,6 +44,10 @@ pub(crate) fn app_scoped_key_path(app_id: &str, key_path: &str) -> String { format!("apps.{app_id}.{key_path}") } +pub(crate) fn format_config_error(err: &impl Display) -> String { + format!("{err:#}") +} + fn trusted_project_edit(project_path: &Path) -> ConfigEdit { let project_key = project_trust_key(project_path) .replace('\\', "\\\\") @@ -203,27 +208,5 @@ pub(crate) async fn write_skill_enabled( } #[cfg(test)] -mod tests { - use super::*; - use pretty_assertions::assert_eq; - - #[test] - fn app_scoped_key_path_quotes_dotted_app_ids() { - assert_eq!( - app_scoped_key_path("plugin.linear", "enabled"), - "apps.\"plugin.linear\".enabled" - ); - } - - #[test] - fn trusted_project_edit_targets_project_trust_level() { - assert_eq!( - trusted_project_edit(Path::new("/workspace/team.project")), - ConfigEdit { - key_path: "projects.\"/workspace/team.project\".trust_level".to_string(), - value: serde_json::json!("trusted"), - merge_strategy: MergeStrategy::Replace, - } - ); - } -} +#[path = "config_update_tests.rs"] +mod tests; diff --git a/codex-rs/tui/src/config_update_tests.rs b/codex-rs/tui/src/config_update_tests.rs new file mode 100644 index 000000000..ee7508862 --- /dev/null +++ b/codex-rs/tui/src/config_update_tests.rs @@ -0,0 +1,40 @@ +use super::*; +use color_eyre::eyre::WrapErr; +use pretty_assertions::assert_eq; +use std::path::Path; + +#[test] +fn app_scoped_key_path_quotes_dotted_app_ids() { + assert_eq!( + app_scoped_key_path("plugin.linear", "enabled"), + "apps.\"plugin.linear\".enabled" + ); +} + +#[test] +fn trusted_project_edit_targets_project_trust_level() { + assert_eq!( + trusted_project_edit(Path::new("/workspace/team.project")), + ConfigEdit { + key_path: "projects.\"/workspace/team.project\".trust_level".to_string(), + value: serde_json::json!("trusted"), + merge_strategy: MergeStrategy::Replace, + } + ); +} + +#[test] +fn format_config_error_preserves_server_validation_message() { + let err = Err::<(), _>(color_eyre::eyre::eyre!( + "config/batchWrite failed: Invalid configuration: features.fast_mode=true violates \ + managed requirements; allowed set [fast_mode=false]" + )) + .wrap_err("config/batchWrite failed in TUI") + .unwrap_err(); + + assert_eq!( + format_config_error(&err), + "config/batchWrite failed in TUI: config/batchWrite failed: Invalid configuration: \ + features.fast_mode=true violates managed requirements; allowed set [fast_mode=false]" + ); +} diff --git a/codex-rs/tui/src/onboarding/onboarding_screen.rs b/codex-rs/tui/src/onboarding/onboarding_screen.rs index 5291934a3..e4c4b346d 100644 --- a/codex-rs/tui/src/onboarding/onboarding_screen.rs +++ b/codex-rs/tui/src/onboarding/onboarding_screen.rs @@ -32,6 +32,7 @@ use codex_protocol::config_types::ForcedLoginMethod; use crate::LoginStatus; use crate::app_server_session::AppServerSession; +use crate::config_update::format_config_error; use crate::config_update::write_trusted_project; use crate::key_hint::KeyBindingListExt; use crate::legacy_core::config::Config; @@ -605,8 +606,9 @@ async fn persist_selected_trust( match result { Ok(()) => true, Err(error) => { + let error = format_config_error(&error); tracing::error!( - "failed to persist trusted project state for {}: {error:?}", + "failed to persist trusted project state for {}: {error}", trust_target.display() ); if let Step::TrustDirectory(widget) = &mut onboarding_screen.steps[trust_step_index] { diff --git a/codex-rs/tui/src/onboarding/snapshots/codex_tui__onboarding__trust_directory__tests__renders_snapshot_for_trust_error.snap b/codex-rs/tui/src/onboarding/snapshots/codex_tui__onboarding__trust_directory__tests__renders_snapshot_for_trust_error.snap new file mode 100644 index 000000000..6f7a495d1 --- /dev/null +++ b/codex-rs/tui/src/onboarding/snapshots/codex_tui__onboarding__trust_directory__tests__renders_snapshot_for_trust_error.snap @@ -0,0 +1,19 @@ +--- +source: tui/src/onboarding/trust_directory.rs +expression: terminal.backend() +--- +> You are in /workspace/project + + Do you trust the contents of this directory? Working with untrusted + contents comes with higher risk of prompt injection. Trusting the + directory allows project-local config, hooks, and exec policies to + load. + +› 1. Yes, continue + 2. No, quit + + Failed to set trust for /workspace/project: config/batchWrite failed + in TUI: Invalid configuration: features.fast_mode=true is not + supported; allowed set [fast_mode=false] + + Press enter to continue diff --git a/codex-rs/tui/src/onboarding/trust_directory.rs b/codex-rs/tui/src/onboarding/trust_directory.rs index 36d13e5a5..8a98cbca6 100644 --- a/codex-rs/tui/src/onboarding/trust_directory.rs +++ b/codex-rs/tui/src/onboarding/trust_directory.rs @@ -191,6 +191,18 @@ mod tests { use ratatui::Terminal; use std::path::PathBuf; + fn widget(error: Option) -> TrustDirectoryWidget { + TrustDirectoryWidget { + cwd: PathBuf::from("/workspace/project"), + trust_target: PathBuf::from("/workspace/project"), + show_windows_create_sandbox_hint: false, + should_quit: false, + selection: None, + highlighted: TrustDirectorySelection::Trust, + error, + } + } + #[test] fn release_event_does_not_change_selection() { let mut widget = TrustDirectoryWidget { @@ -217,15 +229,7 @@ mod tests { #[test] fn renders_snapshot_for_git_repo() { - let widget = TrustDirectoryWidget { - cwd: PathBuf::from("/workspace/project"), - trust_target: PathBuf::from("/workspace/project"), - show_windows_create_sandbox_hint: false, - should_quit: false, - selection: None, - highlighted: TrustDirectorySelection::Trust, - error: None, - }; + let widget = widget(/*error*/ None); let mut terminal = Terminal::new(VT100Backend::new(/*width*/ 70, /*height*/ 14)).expect("terminal"); @@ -235,4 +239,20 @@ mod tests { insta::assert_snapshot!(terminal.backend()); } + + #[test] + fn renders_snapshot_for_trust_error() { + let widget = widget(Some( + "Failed to set trust for /workspace/project: config/batchWrite failed in TUI: Invalid configuration: features.fast_mode=true is not supported; allowed set [fast_mode=false]" + .to_string(), + )); + + let mut terminal = + Terminal::new(VT100Backend::new(/*width*/ 70, /*height*/ 18)).expect("terminal"); + terminal + .draw(|f| (&widget).render_ref(f.area(), f.buffer_mut())) + .expect("draw"); + + insta::assert_snapshot!(terminal.backend()); + } } diff --git a/codex-rs/tui/src/snapshots/codex_tui__startup_hooks_review__tests__startup_hooks_review_prompt.snap b/codex-rs/tui/src/snapshots/codex_tui__startup_hooks_review__tests__startup_hooks_review_prompt.snap index 038534e98..49106a3f5 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__startup_hooks_review__tests__startup_hooks_review_prompt.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__startup_hooks_review__tests__startup_hooks_review_prompt.snap @@ -2,13 +2,13 @@ source: tui/src/startup_hooks_review.rs expression: "render_lines(&view, 80)" --- - - Hooks need review - 2 hooks are new or changed. - Hooks can run outside the sandbox after you trust them. - -› 1. Review hooks - 2. Trust all and continue - 3. Continue without trusting (hooks won't run) - + + Hooks need review + 2 hooks are new or changed. + Hooks can run outside the sandbox after you trust them. + +› 1. Review hooks + 2. Trust all and continue + 3. Continue without trusting (hooks won't run) + Press enter to confirm or esc to go back diff --git a/codex-rs/tui/src/snapshots/codex_tui__startup_hooks_review__tests__startup_hooks_review_prompt_with_trust_error.snap b/codex-rs/tui/src/snapshots/codex_tui__startup_hooks_review__tests__startup_hooks_review_prompt_with_trust_error.snap index 340c0d233..574c31b49 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__startup_hooks_review__tests__startup_hooks_review_prompt_with_trust_error.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__startup_hooks_review__tests__startup_hooks_review_prompt_with_trust_error.snap @@ -1,15 +1,17 @@ --- source: tui/src/startup_hooks_review.rs -expression: "render_lines(&view, 80)" +expression: "render_lines(&view, 62)" --- - - Hooks need review - 2 hooks are new or changed. - Hooks can run outside the sandbox after you trust them. - Failed to trust hooks: disk full - -› 1. Review hooks - 2. Trust all and continue - 3. Continue without trusting (hooks won't run) - + + Hooks need review + 2 hooks are new or changed. + Hooks can run outside the sandbox after you trust them. + Failed to trust hooks: config/batchWrite failed in TUI: + Invalid configuration: features.fast_mode=true is not + supported; allowed set [fast_mode=false] + +› 1. Review hooks + 2. Trust all and continue + 3. Continue without trusting (hooks won't run) + Press enter to confirm or esc to go back diff --git a/codex-rs/tui/src/startup_hooks_review.rs b/codex-rs/tui/src/startup_hooks_review.rs index 559c796aa..67d6133a1 100644 --- a/codex-rs/tui/src/startup_hooks_review.rs +++ b/codex-rs/tui/src/startup_hooks_review.rs @@ -5,7 +5,9 @@ use ratatui::layout::Rect; use ratatui::style::Stylize; use ratatui::text::Line; use ratatui::widgets::Clear; +use ratatui::widgets::Paragraph; use ratatui::widgets::WidgetRef; +use ratatui::widgets::Wrap; use tokio::sync::mpsc::unbounded_channel; use tokio_stream::StreamExt; @@ -17,6 +19,7 @@ use crate::bottom_pane::ListSelectionView; use crate::bottom_pane::SelectionItem; use crate::bottom_pane::SelectionViewParams; use crate::bottom_pane::popup_consts::standard_popup_hint_line_for_keymap; +use crate::config_update::format_config_error; use crate::hooks_rpc::HookTrustUpdate; use crate::hooks_rpc::fetch_hooks_list; use crate::hooks_rpc::hook_needs_review; @@ -130,7 +133,9 @@ async fn run_startup_hooks_review_app( ) .await .map(|_| ()) - .map_err(|err| format!("Failed to trust hooks: {err}")); + .map_err(|err| { + format!("Failed to trust hooks: {}", format_config_error(&err)) + }); match result { Ok(()) => return Ok(StartupHooksReviewOutcome::Continue), Err(err) => { @@ -199,7 +204,7 @@ fn selection_view_params( "Hooks can run outside the sandbox after you trust them.".dim(), )); if let Some(error) = trust_all_error { - header.push(Line::from(error.to_string()).red()); + header.push(Paragraph::new(Line::from(error.to_string()).red()).wrap(Wrap { trim: false })); } else if trusting_all { header.push(Line::from("Trusting hooks...".dim())); } @@ -333,7 +338,7 @@ mod tests { } }) .collect::(); - format!("{rendered:width$}", width = area.width as usize) + rendered.trim_end().to_string() }) .collect::>() .join("\n") @@ -373,7 +378,9 @@ mod tests { let keymap = RuntimeKeymap::defaults(); let view = selection_view( &entry(), - Some("Failed to trust hooks: disk full"), + Some( + "Failed to trust hooks: config/batchWrite failed in TUI: Invalid configuration: features.fast_mode=true is not supported; allowed set [fast_mode=false]", + ), /*trusting_all*/ false, AppEventSender::new(tx_raw), &keymap, @@ -381,7 +388,7 @@ mod tests { assert_snapshot!( "startup_hooks_review_prompt_with_trust_error", - render_lines(&view, /*width*/ 80) + render_lines(&view, /*width*/ 62) ); } }