From 2cd1a0a45e8552dced0241ae024ffc354208a8ad Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 13 Jan 2026 15:21:32 -0800 Subject: [PATCH] fix: report an appropriate error in the TUI for malformed rules (#9011) The underlying issue is that when we encountered an error starting a conversation (any sort of error, though making `$CODEX_HOME/rules` a file rather than folder was the example in #8803), then we were writing the message to stderr, but this could be printed over by our UI framework so the user would not see it. In general, we disallow the use of `eprintln!()` in this part of the code for exactly this reason, though this was suppressed by an `#[allow(clippy::print_stderr)]`. This attempts to clean things up by changing `handle_event()` and `handle_tui_event()` to return a `Result` instead of a `Result`, which is a new type introduced in this PR (and depends on `ExitReason`, also a new type): ```rust #[derive(Debug)] pub(crate) enum AppRunControl { Continue, Exit(ExitReason), } #[derive(Debug, Clone)] pub enum ExitReason { UserRequested, Fatal(String), } ``` This makes it possible to exit the primary control flow of the TUI with richer information. This PR adds `ExitReason` to the existing `AppExitInfo` struct and updates `handle_app_exit()` to print the error and exit code `1` in the event of `ExitReason::Fatal`. I tried to create an integration test for this, but it was a bit involved, so I published it as a separate PR: https://github.com/openai/codex/pull/9166. For this PR, please have faith in my manual testing! Fixes https://github.com/openai/codex/issues/8803. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/9011). * #9166 * __->__ #9011 --- codex-rs/cli/src/main.rs | 11 ++++ codex-rs/tui/src/app.rs | 87 +++++++++++++++++-------- codex-rs/tui/src/app_event.rs | 3 + codex-rs/tui/src/chatwidget/agent.rs | 8 +-- codex-rs/tui/src/lib.rs | 15 ++--- codex-rs/tui2/src/app.rs | 91 +++++++++++++++++++-------- codex-rs/tui2/src/app_event.rs | 3 + codex-rs/tui2/src/chatwidget/agent.rs | 8 +-- codex-rs/tui2/src/lib.rs | 14 +++-- 9 files changed, 164 insertions(+), 76 deletions(-) diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index c29da7271..68b54d5b1 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -26,6 +26,7 @@ use codex_execpolicy::ExecPolicyCheckCommand; use codex_responses_api_proxy::Args as ResponsesApiProxyArgs; use codex_tui::AppExitInfo; use codex_tui::Cli as TuiCli; +use codex_tui::ExitReason; use codex_tui::update_action::UpdateAction; use codex_tui2 as tui2; use owo_colors::OwoColorize; @@ -353,6 +354,14 @@ fn format_exit_messages(exit_info: AppExitInfo, color_enabled: bool) -> Vec anyhow::Result<()> { + match exit_info.exit_reason { + ExitReason::Fatal(message) => { + eprintln!("ERROR: {message}"); + std::process::exit(1); + } + ExitReason::UserRequested => { /* normal exit */ } + } + let update_action = exit_info.update_action; let color_enabled = supports_color::on(Stream::Stdout).is_some(); for line in format_exit_messages(exit_info, color_enabled) { @@ -947,6 +956,7 @@ mod tests { token_usage, thread_id: conversation.map(ThreadId::from_string).map(Result::unwrap), update_action: None, + exit_reason: ExitReason::UserRequested, } } @@ -956,6 +966,7 @@ mod tests { token_usage: TokenUsage::default(), thread_id: None, update_action: None, + exit_reason: ExitReason::UserRequested, }; let lines = format_exit_messages(exit_info, false); assert!(lines.is_empty()); diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 312d4e063..d8df24e65 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -77,6 +77,19 @@ pub struct AppExitInfo { pub token_usage: TokenUsage, pub thread_id: Option, pub update_action: Option, + pub exit_reason: ExitReason, +} + +#[derive(Debug)] +pub(crate) enum AppRunControl { + Continue, + Exit(ExitReason), +} + +#[derive(Debug, Clone)] +pub enum ExitReason { + UserRequested, + Fatal(String), } fn session_summary(token_usage: TokenUsage, thread_id: Option) -> Option { @@ -289,6 +302,7 @@ async fn handle_model_migration_prompt_if_needed( token_usage: TokenUsage::default(), thread_id: None, update_action: None, + exit_reason: ExitReason::UserRequested, }); } } @@ -506,14 +520,23 @@ impl App { #[cfg(not(debug_assertions))] if let Some(latest_version) = upgrade_version { - app.handle_event( - tui, - AppEvent::InsertHistoryCell(Box::new(UpdateAvailableHistoryCell::new( - latest_version, - crate::update_action::get_update_action(), - ))), - ) - .await?; + let control = app + .handle_event( + tui, + AppEvent::InsertHistoryCell(Box::new(UpdateAvailableHistoryCell::new( + latest_version, + crate::update_action::get_update_action(), + ))), + ) + .await?; + if let AppRunControl::Exit(exit_reason) = control { + return Ok(AppExitInfo { + token_usage: app.token_usage(), + thread_id: app.chat_widget.thread_id(), + update_action: app.pending_update_action, + exit_reason, + }); + } } let tui_events = tui.event_stream(); @@ -521,19 +544,26 @@ impl App { tui.frame_requester().schedule_frame(); - while select! { - Some(event) = app_event_rx.recv() => { - app.handle_event(tui, event).await? + let exit_reason = loop { + let control = select! { + Some(event) = app_event_rx.recv() => { + app.handle_event(tui, event).await? + } + Some(event) = tui_events.next() => { + app.handle_tui_event(tui, event).await? + } + }; + match control { + AppRunControl::Continue => {} + AppRunControl::Exit(reason) => break reason, } - Some(event) = tui_events.next() => { - app.handle_tui_event(tui, event).await? - } - } {} + }; tui.terminal.clear()?; Ok(AppExitInfo { token_usage: app.token_usage(), thread_id: app.chat_widget.thread_id(), update_action: app.pending_update_action, + exit_reason, }) } @@ -541,7 +571,7 @@ impl App { &mut self, tui: &mut tui::Tui, event: TuiEvent, - ) -> Result { + ) -> Result { if self.overlay.is_some() { let _ = self.handle_backtrack_overlay_event(tui, event).await?; } else { @@ -563,7 +593,7 @@ impl App { .chat_widget .handle_paste_burst_tick(tui.frame_requester()) { - return Ok(true); + return Ok(AppRunControl::Continue); } tui.draw( self.chat_widget.desired_height(tui.terminal.size()?.width), @@ -582,10 +612,10 @@ impl App { } } } - Ok(true) + Ok(AppRunControl::Continue) } - async fn handle_event(&mut self, tui: &mut tui::Tui, event: AppEvent) -> Result { + async fn handle_event(&mut self, tui: &mut tui::Tui, event: AppEvent) -> Result { let model_info = self .server .get_models_manager() @@ -816,7 +846,7 @@ impl App { && matches!(event.msg, EventMsg::ShutdownComplete) { self.suppress_shutdown_complete = false; - return Ok(true); + return Ok(AppRunControl::Continue); } if let EventMsg::ListSkillsResponse(response) = &event.msg { let cwd = self.chat_widget.config_ref().cwd.clone(); @@ -826,7 +856,10 @@ impl App { self.chat_widget.handle_codex_event(event); } AppEvent::ExitRequest => { - return Ok(false); + return Ok(AppRunControl::Exit(ExitReason::UserRequested)); + } + AppEvent::FatalExitRequest(message) => { + return Ok(AppRunControl::Exit(ExitReason::Fatal(message))); } AppEvent::CodexOp(op) => self.chat_widget.submit_op(op), AppEvent::DiffResult(text) => { @@ -926,7 +959,7 @@ impl App { preset, mode: WindowsSandboxEnableMode::Elevated, }); - return Ok(true); + return Ok(AppRunControl::Continue); } self.chat_widget.show_windows_sandbox_setup_status(); @@ -1095,7 +1128,7 @@ impl App { tracing::warn!(%err, "failed to set sandbox policy on app config"); self.chat_widget .add_error_message(format!("Failed to set sandbox policy: {err}")); - return Ok(true); + return Ok(AppRunControl::Continue); } #[cfg(target_os = "windows")] if !matches!(&policy, codex_core::protocol::SandboxPolicy::ReadOnly) @@ -1107,7 +1140,7 @@ impl App { tracing::warn!(%err, "failed to set sandbox policy on chat config"); self.chat_widget .add_error_message(format!("Failed to set sandbox policy: {err}")); - return Ok(true); + return Ok(AppRunControl::Continue); } // If sandbox policy becomes workspace-write or read-only, run the Windows world-writable scan. @@ -1116,7 +1149,7 @@ impl App { // One-shot suppression if the user just confirmed continue. if self.skip_world_writable_scan_once { self.skip_world_writable_scan_once = false; - return Ok(true); + return Ok(AppRunControl::Continue); } let should_check = codex_core::get_platform_sandbox().is_some() @@ -1141,7 +1174,7 @@ impl App { } AppEvent::UpdateFeatureFlags { updates } => { if updates.is_empty() { - return Ok(true); + return Ok(AppRunControl::Continue); } let mut builder = ConfigEditsBuilder::new(&self.config.codex_home) .with_profile(self.active_profile.as_deref()); @@ -1300,7 +1333,7 @@ impl App { } }, } - Ok(true) + Ok(AppRunControl::Continue) } fn reasoning_label(reasoning_effort: Option) -> &'static str { diff --git a/codex-rs/tui/src/app_event.rs b/codex-rs/tui/src/app_event.rs index e63a53487..42f96d909 100644 --- a/codex-rs/tui/src/app_event.rs +++ b/codex-rs/tui/src/app_event.rs @@ -44,6 +44,9 @@ pub(crate) enum AppEvent { /// Request to exit the application gracefully. ExitRequest, + /// Request to exit the application due to a fatal error. + FatalExitRequest(String), + /// Forward an `Op` to the Agent. Using an `AppEvent` for this avoids /// bubbling channels through layers of widgets. CodexOp(codex_core::protocol::Op), diff --git a/codex-rs/tui/src/chatwidget/agent.rs b/codex-rs/tui/src/chatwidget/agent.rs index d8428b221..21ed92d0e 100644 --- a/codex-rs/tui/src/chatwidget/agent.rs +++ b/codex-rs/tui/src/chatwidget/agent.rs @@ -30,16 +30,14 @@ pub(crate) fn spawn_agent( .. } = match server.start_thread(config).await { Ok(v) => v, - #[allow(clippy::print_stderr)] Err(err) => { - let message = err.to_string(); - eprintln!("{message}"); + let message = format!("Failed to initialize codex: {err}"); + tracing::error!("{message}"); app_event_tx_clone.send(AppEvent::CodexEvent(Event { id: "".to_string(), msg: EventMsg::Error(err.to_error_event(None)), })); - app_event_tx_clone.send(AppEvent::ExitRequest); - tracing::error!("failed to initialize codex: {err}"); + app_event_tx_clone.send(AppEvent::FatalExitRequest(message)); return; } }; diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 5612f6615..115c9c030 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -6,6 +6,7 @@ use additional_dirs::add_dir_warning_message; use app::App; pub use app::AppExitInfo; +pub use app::ExitReason; use codex_app_server_protocol::AuthMode; use codex_common::oss::ensure_oss_provider_ready; use codex_common::oss::get_default_model_for_oss_provider; @@ -99,7 +100,6 @@ pub use cli::Cli; pub use markdown_render::render_markdown_text; pub use public_widgets::composer_input::ComposerAction; pub use public_widgets::composer_input::ComposerInput; -use std::io::Write as _; // (tests access modules directly within the crate) pub async fn run_main( @@ -377,6 +377,7 @@ async fn run_ratatui_app( token_usage: codex_core::protocol::TokenUsage::default(), thread_id: None, update_action: Some(action), + exit_reason: ExitReason::UserRequested, }); } } @@ -416,6 +417,7 @@ async fn run_ratatui_app( token_usage: codex_core::protocol::TokenUsage::default(), thread_id: None, update_action: None, + exit_reason: ExitReason::UserRequested, }); } // if the user acknowledged windows or made an explicit decision ato trust the directory, reload the config accordingly @@ -444,16 +446,13 @@ async fn run_ratatui_app( restore(); session_log::log_session_end(); let _ = tui.terminal.clear(); - if let Err(err) = writeln!( - std::io::stdout(), - "No saved session found with ID {id_str}. Run `codex {action}` without an ID to choose from existing sessions." - ) { - error!("Failed to write session error message: {err}"); - } Ok(AppExitInfo { token_usage: codex_core::protocol::TokenUsage::default(), thread_id: None, update_action: None, + exit_reason: ExitReason::Fatal(format!( + "No saved session found with ID {id_str}. Run `codex {action}` without an ID to choose from existing sessions." + )), }) }; @@ -499,6 +498,7 @@ async fn run_ratatui_app( token_usage: codex_core::protocol::TokenUsage::default(), thread_id: None, update_action: None, + exit_reason: ExitReason::UserRequested, }); } other => other, @@ -546,6 +546,7 @@ async fn run_ratatui_app( token_usage: codex_core::protocol::TokenUsage::default(), thread_id: None, update_action: None, + exit_reason: ExitReason::UserRequested, }); } other => other, diff --git a/codex-rs/tui2/src/app.rs b/codex-rs/tui2/src/app.rs index 4e3ac3795..afedcded2 100644 --- a/codex-rs/tui2/src/app.rs +++ b/codex-rs/tui2/src/app.rs @@ -98,6 +98,7 @@ pub struct AppExitInfo { pub token_usage: TokenUsage, pub conversation_id: Option, pub update_action: Option, + pub exit_reason: ExitReason, /// ANSI-styled transcript lines to print after the TUI exits. /// /// These lines are rendered against the same width as the final TUI @@ -106,12 +107,29 @@ pub struct AppExitInfo { pub session_lines: Vec, } +#[derive(Debug)] +pub(crate) enum AppRunControl { + Continue, + Exit(ExitReason), +} + +#[derive(Debug, Clone)] +pub enum ExitReason { + UserRequested, + Fatal(String), +} + impl From for codex_tui::AppExitInfo { fn from(info: AppExitInfo) -> Self { + let exit_reason = match info.exit_reason { + ExitReason::UserRequested => codex_tui::ExitReason::UserRequested, + ExitReason::Fatal(message) => codex_tui::ExitReason::Fatal(message), + }; codex_tui::AppExitInfo { token_usage: info.token_usage, thread_id: info.conversation_id, update_action: info.update_action.map(Into::into), + exit_reason, } } } @@ -326,6 +344,7 @@ async fn handle_model_migration_prompt_if_needed( token_usage: TokenUsage::default(), conversation_id: None, update_action: None, + exit_reason: ExitReason::UserRequested, session_lines: Vec::new(), }); } @@ -596,14 +615,24 @@ impl App { #[cfg(not(debug_assertions))] if let Some(latest_version) = upgrade_version { - app.handle_event( - tui, - AppEvent::InsertHistoryCell(Box::new(UpdateAvailableHistoryCell::new( - latest_version, - crate::update_action::get_update_action(), - ))), - ) - .await?; + let control = app + .handle_event( + tui, + AppEvent::InsertHistoryCell(Box::new(UpdateAvailableHistoryCell::new( + latest_version, + crate::update_action::get_update_action(), + ))), + ) + .await?; + if let AppRunControl::Exit(exit_reason) = control { + return Ok(AppExitInfo { + token_usage: app.token_usage(), + conversation_id: app.chat_widget.conversation_id(), + update_action: app.pending_update_action, + exit_reason, + session_lines: Vec::new(), + }); + } } let tui_events = tui.event_stream(); @@ -611,14 +640,20 @@ impl App { tui.frame_requester().schedule_frame(); - while select! { - Some(event) = app_event_rx.recv() => { - app.handle_event(tui, event).await? + let exit_reason = loop { + let control = select! { + Some(event) = app_event_rx.recv() => { + app.handle_event(tui, event).await? + } + Some(event) = tui_events.next() => { + app.handle_tui_event(tui, event).await? + } + }; + match control { + AppRunControl::Continue => {} + AppRunControl::Exit(reason) => break reason, } - Some(event) = tui_events.next() => { - app.handle_tui_event(tui, event).await? - } - } {} + }; let width = tui.terminal.last_known_screen_size.width; let session_lines = if width == 0 { Vec::new() @@ -639,6 +674,7 @@ impl App { token_usage: app.token_usage(), conversation_id: app.chat_widget.conversation_id(), update_action: app.pending_update_action, + exit_reason, session_lines, }) } @@ -647,7 +683,7 @@ impl App { &mut self, tui: &mut tui::Tui, event: TuiEvent, - ) -> Result { + ) -> Result { if matches!(&event, TuiEvent::Draw) { self.handle_scroll_tick(tui); } @@ -676,7 +712,7 @@ impl App { .chat_widget .handle_paste_burst_tick(tui.frame_requester()) { - return Ok(true); + return Ok(AppRunControl::Continue); } let cells = self.transcript_cells.clone(); tui.draw(tui.terminal.size()?.height, |frame| { @@ -736,7 +772,7 @@ impl App { } } } - Ok(true) + Ok(AppRunControl::Continue) } pub(crate) fn render_transcript_cells( @@ -1387,7 +1423,7 @@ impl App { Some(TranscriptSelectionPoint { line_index, column }) } - async fn handle_event(&mut self, tui: &mut tui::Tui, event: AppEvent) -> Result { + async fn handle_event(&mut self, tui: &mut tui::Tui, event: AppEvent) -> Result { match event { AppEvent::NewSession => { let summary = session_summary( @@ -1597,7 +1633,7 @@ impl App { && matches!(event.msg, EventMsg::ShutdownComplete) { self.suppress_shutdown_complete = false; - return Ok(true); + return Ok(AppRunControl::Continue); } if let EventMsg::ListSkillsResponse(response) = &event.msg { let cwd = self.chat_widget.config_ref().cwd.clone(); @@ -1607,7 +1643,10 @@ impl App { self.chat_widget.handle_codex_event(event); } AppEvent::ExitRequest => { - return Ok(false); + return Ok(AppRunControl::Exit(ExitReason::UserRequested)); + } + AppEvent::FatalExitRequest(message) => { + return Ok(AppRunControl::Exit(ExitReason::Fatal(message))); } AppEvent::CodexOp(op) => self.chat_widget.submit_op(op), AppEvent::DiffResult(text) => { @@ -1702,7 +1741,7 @@ impl App { preset, mode: WindowsSandboxEnableMode::Elevated, }); - return Ok(true); + return Ok(AppRunControl::Continue); } self.chat_widget.show_windows_sandbox_setup_status(); @@ -1871,7 +1910,7 @@ impl App { tracing::warn!(%err, "failed to set sandbox policy on app config"); self.chat_widget .add_error_message(format!("Failed to set sandbox policy: {err}")); - return Ok(true); + return Ok(AppRunControl::Continue); } #[cfg(target_os = "windows")] if !matches!(&policy, codex_core::protocol::SandboxPolicy::ReadOnly) @@ -1883,7 +1922,7 @@ impl App { tracing::warn!(%err, "failed to set sandbox policy on chat config"); self.chat_widget .add_error_message(format!("Failed to set sandbox policy: {err}")); - return Ok(true); + return Ok(AppRunControl::Continue); } // If sandbox policy becomes workspace-write or read-only, run the Windows world-writable scan. @@ -1892,7 +1931,7 @@ impl App { // One-shot suppression if the user just confirmed continue. if self.skip_world_writable_scan_once { self.skip_world_writable_scan_once = false; - return Ok(true); + return Ok(AppRunControl::Continue); } let should_check = codex_core::get_platform_sandbox().is_some() @@ -2040,7 +2079,7 @@ impl App { } }, } - Ok(true) + Ok(AppRunControl::Continue) } fn reasoning_label(reasoning_effort: Option) -> &'static str { diff --git a/codex-rs/tui2/src/app_event.rs b/codex-rs/tui2/src/app_event.rs index 3396d5ca0..59cc18047 100644 --- a/codex-rs/tui2/src/app_event.rs +++ b/codex-rs/tui2/src/app_event.rs @@ -43,6 +43,9 @@ pub(crate) enum AppEvent { /// Request to exit the application gracefully. ExitRequest, + /// Request to exit the application due to a fatal error. + FatalExitRequest(String), + /// Forward an `Op` to the Agent. Using an `AppEvent` for this avoids /// bubbling channels through layers of widgets. CodexOp(codex_core::protocol::Op), diff --git a/codex-rs/tui2/src/chatwidget/agent.rs b/codex-rs/tui2/src/chatwidget/agent.rs index 0e6fa2712..24c403653 100644 --- a/codex-rs/tui2/src/chatwidget/agent.rs +++ b/codex-rs/tui2/src/chatwidget/agent.rs @@ -30,16 +30,14 @@ pub(crate) fn spawn_agent( thread_id: _, } = match server.start_thread(config).await { Ok(v) => v, - #[allow(clippy::print_stderr)] Err(err) => { - let message = err.to_string(); - eprintln!("{message}"); + let message = format!("Failed to initialize codex: {err}"); + tracing::error!("{message}"); app_event_tx_clone.send(AppEvent::CodexEvent(Event { id: "".to_string(), msg: EventMsg::Error(err.to_error_event(None)), })); - app_event_tx_clone.send(AppEvent::ExitRequest); - tracing::error!("failed to initialize codex: {err}"); + app_event_tx_clone.send(AppEvent::FatalExitRequest(message)); return; } }; diff --git a/codex-rs/tui2/src/lib.rs b/codex-rs/tui2/src/lib.rs index e4de7a3cb..8c81315b4 100644 --- a/codex-rs/tui2/src/lib.rs +++ b/codex-rs/tui2/src/lib.rs @@ -6,6 +6,7 @@ use additional_dirs::add_dir_warning_message; use app::App; pub use app::AppExitInfo; +pub use app::ExitReason; use codex_app_server_protocol::AuthMode; use codex_common::oss::ensure_oss_provider_ready; use codex_common::oss::get_default_model_for_oss_provider; @@ -395,6 +396,7 @@ async fn run_ratatui_app( token_usage: codex_core::protocol::TokenUsage::default(), conversation_id: None, update_action: Some(action), + exit_reason: ExitReason::UserRequested, session_lines: Vec::new(), }); } @@ -435,6 +437,7 @@ async fn run_ratatui_app( token_usage: codex_core::protocol::TokenUsage::default(), conversation_id: None, update_action: None, + exit_reason: ExitReason::UserRequested, session_lines: Vec::new(), }); } @@ -464,16 +467,13 @@ async fn run_ratatui_app( restore(); session_log::log_session_end(); let _ = tui.terminal.clear(); - if let Err(err) = writeln!( - std::io::stdout(), - "No saved session found with ID {id_str}. Run `codex {action}` without an ID to choose from existing sessions." - ) { - error!("Failed to write session error message: {err}"); - } Ok(AppExitInfo { token_usage: codex_core::protocol::TokenUsage::default(), conversation_id: None, update_action: None, + exit_reason: ExitReason::Fatal(format!( + "No saved session found with ID {id_str}. Run `codex {action}` without an ID to choose from existing sessions." + )), session_lines: Vec::new(), }) }; @@ -520,6 +520,7 @@ async fn run_ratatui_app( token_usage: codex_core::protocol::TokenUsage::default(), conversation_id: None, update_action: None, + exit_reason: ExitReason::UserRequested, session_lines: Vec::new(), }); } @@ -568,6 +569,7 @@ async fn run_ratatui_app( token_usage: codex_core::protocol::TokenUsage::default(), conversation_id: None, update_action: None, + exit_reason: ExitReason::UserRequested, session_lines: Vec::new(), }); }