From d0a905a557151dce77213b0026c177fc137beb99 Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Sat, 9 May 2026 15:32:54 -0300 Subject: [PATCH] fix(tui): avoid update loops for mismatched npm installs --- codex-cli/bin/codex.js | 8 +- codex-rs/cli/src/main.rs | 34 ++- codex-rs/tui/src/app.rs | 25 ++- codex-rs/tui/src/lib.rs | 7 + codex-rs/tui/src/npm_registry.rs | 3 +- ...te_prompt__tests__update_prompt_modal.snap | 1 + ...ests__update_remediation_prompt_modal.snap | 14 ++ codex-rs/tui/src/update_action.rs | 164 ++++++++++++++- codex-rs/tui/src/update_prompt.rs | 194 +++++++++++++++++- codex-rs/tui/src/updates.rs | 193 +++++++++++++++-- 10 files changed, 598 insertions(+), 45 deletions(-) create mode 100644 codex-rs/tui/src/snapshots/codex_tui__update_prompt__tests__update_remediation_prompt_modal.snap diff --git a/codex-cli/bin/codex.js b/codex-cli/bin/codex.js index 67ab3e2d95..758080f128 100755 --- a/codex-cli/bin/codex.js +++ b/codex-cli/bin/codex.js @@ -2,7 +2,7 @@ // Unified entry point for the Codex CLI. import { spawn } from "node:child_process"; -import { existsSync } from "fs"; +import { existsSync, realpathSync } from "fs"; import { createRequire } from "node:module"; import path from "path"; import { fileURLToPath } from "url"; @@ -171,6 +171,12 @@ const packageManagerEnvVar = ? "CODEX_MANAGED_BY_BUN" : "CODEX_MANAGED_BY_NPM"; env[packageManagerEnvVar] = "1"; +try { + env.CODEX_MANAGED_PACKAGE_ROOT = realpathSync(path.join(__dirname, "..")); +} catch { + // Best effort only. Older or unusual package layouts can omit this extra + // provenance without preventing Codex from starting. +} const child = spawn(binaryPath, process.argv.slice(2), { stdio: "inherit", diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index d70e401664..5de7f526ef 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -34,7 +34,10 @@ use codex_state::state_db_path; use codex_tui::AppExitInfo; use codex_tui::Cli as TuiCli; use codex_tui::ExitReason; +use codex_tui::PromptedUpdate; use codex_tui::UpdateAction; +#[cfg(not(debug_assertions))] +use codex_tui::UpdateActionStatus; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_cli::CliConfigOverrides; use owo_colors::OwoColorize; @@ -618,13 +621,13 @@ fn handle_app_exit(exit_info: AppExitInfo) -> anyhow::Result<()> { ExitReason::UserRequested => { /* normal exit */ } } - let update_action = exit_info.update_action; + let prompted_update = exit_info.update_action.clone(); let color_enabled = supports_color::on(Stream::Stdout).is_some(); for line in format_exit_messages(exit_info, color_enabled) { println!("{line}"); } - if let Some(action) = update_action { - run_update_action(action)?; + if let Some(prompted_update) = prompted_update { + run_prompted_update(prompted_update)?; } Ok(()) } @@ -672,6 +675,20 @@ fn run_update_action(action: UpdateAction) -> anyhow::Result<()> { Ok(()) } +fn run_prompted_update(prompted_update: PromptedUpdate) -> anyhow::Result<()> { + run_update_action(prompted_update.action)?; + #[cfg(not(debug_assertions))] + { + if let Err(err) = codex_tui::record_successful_prompt_update_attempt( + &prompted_update.version_file, + &prompted_update.target_version, + ) { + tracing::warn!("Failed to record successful prompted update attempt: {err}"); + } + } + Ok(()) +} + fn run_update_command() -> anyhow::Result<()> { #[cfg(debug_assertions)] { @@ -682,12 +699,13 @@ fn run_update_command() -> anyhow::Result<()> { #[cfg(not(debug_assertions))] { - let Some(action) = codex_tui::get_update_action() else { - anyhow::bail!( + match codex_tui::get_update_action_status() { + UpdateActionStatus::Ready(action) => run_update_action(action), + UpdateActionStatus::Blocked(blocker) => anyhow::bail!("{blocker}"), + UpdateActionStatus::Unavailable => anyhow::bail!( "Could not detect the Codex installation method. Please update manually: https://developers.openai.com/codex/cli/" - ); - }; - run_update_action(action) + ), + } } } diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index dff288493c..5bc4dc320a 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -73,7 +73,9 @@ use crate::token_usage::TokenUsage; use crate::transcript_reflow::TranscriptReflowState; use crate::tui; use crate::tui::TuiEvent; -use crate::update_action::UpdateAction; +use crate::update_action::PromptedUpdate; +#[cfg(not(debug_assertions))] +use crate::update_action::UpdateActionStatus; use crate::version::CODEX_CLI_VERSION; use crate::workspace_command::AppServerWorkspaceCommandRunner; use crate::workspace_command::WorkspaceCommandRunner; @@ -335,7 +337,7 @@ pub struct AppExitInfo { pub token_usage: TokenUsage, pub thread_id: Option, pub thread_name: Option, - pub update_action: Option, + pub update_action: Option, pub exit_reason: ExitReason, } @@ -478,7 +480,7 @@ pub(crate) struct App { remote_app_server_url: Option, remote_app_server_auth_token: Option, /// Set when the user confirms an update; propagated on exit. - pub(crate) pending_update_action: Option, + pub(crate) pending_update_action: Option, /// Tracks the thread we intentionally shut down while exiting the app. /// @@ -863,7 +865,16 @@ See the Codex keymap documentation for supported actions and examples." ) })?; #[cfg(not(debug_assertions))] - let upgrade_version = crate::updates::get_upgrade_version(&config); + let upgrade_notice = + crate::updates::get_upgrade_version_for_history(&config).and_then(|latest_version| { + match crate::update_action::get_update_action_status() { + UpdateActionStatus::Ready(update_action) => { + Some((latest_version, Some(update_action))) + } + UpdateActionStatus::Unavailable => Some((latest_version, None)), + UpdateActionStatus::Blocked(_) => None, + } + }); let mut app = Self { model_catalog, @@ -968,14 +979,14 @@ See the Codex keymap documentation for supported actions and examples." let mut waiting_for_initial_session_configured = wait_for_initial_session_configured; #[cfg(not(debug_assertions))] - let pre_loop_exit_reason = if let Some(latest_version) = upgrade_version { + let pre_loop_exit_reason = if let Some((latest_version, update_action)) = upgrade_notice { let control = app .handle_event( tui, &mut app_server, AppEvent::InsertHistoryCell(Box::new(UpdateAvailableHistoryCell::new( latest_version, - crate::update_action::get_update_action(), + update_action, ))), ) .await?; @@ -1078,7 +1089,7 @@ See the Codex keymap documentation for supported actions and examples." token_usage: app.token_usage(), thread_id: resumable_thread.as_ref().map(|thread| thread.thread_id), thread_name: resumable_thread.and_then(|thread| thread.thread_name), - update_action: app.pending_update_action, + update_action: app.pending_update_action.clone(), exit_reason, }) } diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index ac5b489af1..e205f62799 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -177,13 +177,20 @@ mod transcript_reflow; mod tui; mod ui_consts; pub(crate) mod update_action; +pub use update_action::PromptedUpdate; pub use update_action::UpdateAction; +pub use update_action::UpdateActionStatus; +pub use update_action::UpdateBlocker; #[cfg(not(debug_assertions))] pub use update_action::get_update_action; +#[cfg(any(not(debug_assertions), test))] +pub use update_action::get_update_action_status; mod update_prompt; #[cfg(any(not(debug_assertions), test))] mod update_versions; mod updates; +#[cfg(not(debug_assertions))] +pub use updates::record_successful_prompt_update_attempt; mod version; #[cfg(not(target_os = "linux"))] mod voice; diff --git a/codex-rs/tui/src/npm_registry.rs b/codex-rs/tui/src/npm_registry.rs index 61ff2425e6..a45a612dfb 100644 --- a/codex-rs/tui/src/npm_registry.rs +++ b/codex-rs/tui/src/npm_registry.rs @@ -1,7 +1,8 @@ use serde::Deserialize; use std::collections::HashMap; -#[cfg(not(debug_assertions))] +#[cfg(any(not(debug_assertions), test))] +#[cfg_attr(test, allow(dead_code))] pub(crate) const PACKAGE_URL: &str = "https://registry.npmjs.org/@openai%2fcodex"; #[derive(Deserialize, Debug, Clone)] diff --git a/codex-rs/tui/src/snapshots/codex_tui__update_prompt__tests__update_prompt_modal.snap b/codex-rs/tui/src/snapshots/codex_tui__update_prompt__tests__update_prompt_modal.snap index 24d8831c95..0db66c14eb 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__update_prompt__tests__update_prompt_modal.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__update_prompt__tests__update_prompt_modal.snap @@ -2,6 +2,7 @@ source: tui/src/update_prompt.rs expression: terminal.backend() --- + ✨ Update available! 0.0.0 -> 9.9.9 Release notes: https://github.com/openai/codex/releases/latest diff --git a/codex-rs/tui/src/snapshots/codex_tui__update_prompt__tests__update_remediation_prompt_modal.snap b/codex-rs/tui/src/snapshots/codex_tui__update_prompt__tests__update_remediation_prompt_modal.snap new file mode 100644 index 0000000000..1f2dfbcdf7 --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__update_prompt__tests__update_remediation_prompt_modal.snap @@ -0,0 +1,14 @@ +--- +source: tui/src/update_prompt.rs +expression: terminal.backend() +--- + + ✨ Update needs attention + + You are running Codex from: + /prefix-a/lib/node_modules/@openai/codex + but `npm install -g @openai/codex@latest` would update: + /prefix-b/lib/node_modules/@openai/codex + Fix your shell PATH or remove the stale Codex install, then restart Codex. + + Press enter to continue diff --git a/codex-rs/tui/src/update_action.rs b/codex-rs/tui/src/update_action.rs index aca065440c..678e9d550f 100644 --- a/codex-rs/tui/src/update_action.rs +++ b/codex-rs/tui/src/update_action.rs @@ -2,6 +2,15 @@ use codex_install_context::InstallContext; #[cfg(any(not(debug_assertions), test))] use codex_install_context::StandalonePlatform; +use std::fmt; +#[cfg(any(not(debug_assertions), test))] +use std::path::Path; +use std::path::PathBuf; +#[cfg(any(not(debug_assertions), test))] +use std::process::Command; + +#[cfg(any(not(debug_assertions), test))] +const MANAGED_PACKAGE_ROOT_ENV: &str = "CODEX_MANAGED_PACKAGE_ROOT"; /// Update action the CLI should perform after the TUI exits. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -18,6 +27,62 @@ pub enum UpdateAction { StandaloneWindows, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum UpdateActionStatus { + Ready(UpdateAction), + Blocked(UpdateBlocker), + Unavailable, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum UpdateBlocker { + NpmGlobalRootMismatch { + running_package_root: PathBuf, + npm_package_root: PathBuf, + }, +} + +#[derive(Debug, Clone)] +pub struct PromptedUpdate { + pub action: UpdateAction, + pub target_version: String, + pub version_file: PathBuf, +} + +impl UpdateBlocker { + pub fn remediation_lines(&self) -> Vec { + match self { + Self::NpmGlobalRootMismatch { + running_package_root, + npm_package_root, + } => vec![ + "You are running Codex from:".to_string(), + format!(" {}", running_package_root.display()), + "but `npm install -g @openai/codex@latest` would update:".to_string(), + format!(" {}", npm_package_root.display()), + "Fix your shell PATH or remove the stale Codex install, then restart Codex." + .to_string(), + ], + } + } +} + +impl fmt::Display for UpdateBlocker { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::NpmGlobalRootMismatch { + running_package_root, + npm_package_root, + } => write!( + f, + "You are running Codex from {}, but `npm install -g @openai/codex@latest` would update {}. Fix your shell PATH or remove the stale Codex install, then restart Codex.", + running_package_root.display(), + npm_package_root.display(), + ), + } + } +} + impl UpdateAction { #[cfg(any(not(debug_assertions), test))] pub(crate) fn from_install_context(context: &InstallContext) -> Option { @@ -36,8 +101,8 @@ impl UpdateAction { /// Returns the list of command-line arguments for invoking the update. pub fn command_args(self) -> (&'static str, &'static [&'static str]) { match self { - UpdateAction::NpmGlobalLatest => ("npm", &["install", "-g", "@openai/codex"]), - UpdateAction::BunGlobalLatest => ("bun", &["install", "-g", "@openai/codex"]), + UpdateAction::NpmGlobalLatest => ("npm", &["install", "-g", "@openai/codex@latest"]), + UpdateAction::BunGlobalLatest => ("bun", &["install", "-g", "@openai/codex@latest"]), UpdateAction::BrewUpgrade => ("brew", &["upgrade", "--cask", "codex"]), UpdateAction::StandaloneUnix => ( "sh", @@ -58,11 +123,79 @@ impl UpdateAction { } } -#[cfg(not(debug_assertions))] +#[cfg(any(not(debug_assertions), test))] +#[cfg_attr(test, allow(dead_code))] pub fn get_update_action() -> Option { UpdateAction::from_install_context(InstallContext::current()) } +#[cfg(any(not(debug_assertions), test))] +pub fn get_update_action_status() -> UpdateActionStatus { + let Some(action) = UpdateAction::from_install_context(InstallContext::current()) else { + return UpdateActionStatus::Unavailable; + }; + + if let Some(blocker) = update_blocker(action) { + UpdateActionStatus::Blocked(blocker) + } else { + UpdateActionStatus::Ready(action) + } +} + +#[cfg(any(not(debug_assertions), test))] +fn update_blocker(action: UpdateAction) -> Option { + match action { + UpdateAction::NpmGlobalLatest => npm_global_root_mismatch(), + UpdateAction::BunGlobalLatest + | UpdateAction::BrewUpgrade + | UpdateAction::StandaloneUnix + | UpdateAction::StandaloneWindows => None, + } +} + +#[cfg(any(not(debug_assertions), test))] +fn npm_global_root_mismatch() -> Option { + let running_package_root = std::env::var_os(MANAGED_PACKAGE_ROOT_ENV)?; + let running_package_root = std::fs::canonicalize(PathBuf::from(running_package_root)).ok()?; + let npm_global_root = npm_global_root()?; + mismatch_from_npm_roots(&running_package_root, &npm_global_root) +} + +#[cfg(any(not(debug_assertions), test))] +fn npm_global_root() -> Option { + #[cfg(windows)] + let output = Command::new("cmd") + .args(["/C", "npm", "root", "-g"]) + .output() + .ok()?; + #[cfg(not(windows))] + let output = Command::new("npm").args(["root", "-g"]).output().ok()?; + + if !output.status.success() { + return None; + } + let stdout = String::from_utf8(output.stdout).ok()?; + let npm_global_root = stdout.trim(); + if npm_global_root.is_empty() { + return None; + } + std::fs::canonicalize(npm_global_root).ok() +} + +#[cfg(any(not(debug_assertions), test))] +fn mismatch_from_npm_roots( + running_package_root: &Path, + npm_global_root: &Path, +) -> Option { + let npm_package_root = npm_global_root.join("@openai").join("codex"); + (running_package_root != npm_package_root.as_path()).then(|| { + UpdateBlocker::NpmGlobalRootMismatch { + running_package_root: running_package_root.to_path_buf(), + npm_package_root, + } + }) +} + #[cfg(test)] mod tests { use super::*; @@ -124,4 +257,29 @@ mod tests { ) ); } + + #[test] + fn npm_root_mismatch_is_blocked_when_update_targets_another_install() { + assert_eq!( + mismatch_from_npm_roots( + Path::new("/prefix-a/lib/node_modules/@openai/codex"), + Path::new("/prefix-b/lib/node_modules"), + ), + Some(UpdateBlocker::NpmGlobalRootMismatch { + running_package_root: PathBuf::from("/prefix-a/lib/node_modules/@openai/codex"), + npm_package_root: PathBuf::from("/prefix-b/lib/node_modules/@openai/codex"), + }) + ); + } + + #[test] + fn npm_root_match_keeps_update_available() { + assert_eq!( + mismatch_from_npm_roots( + Path::new("/prefix-a/lib/node_modules/@openai/codex"), + Path::new("/prefix-a/lib/node_modules"), + ), + None + ); + } } diff --git a/codex-rs/tui/src/update_prompt.rs b/codex-rs/tui/src/update_prompt.rs index 4d5a9e1287..450c43bf45 100644 --- a/codex-rs/tui/src/update_prompt.rs +++ b/codex-rs/tui/src/update_prompt.rs @@ -1,4 +1,5 @@ -#![cfg(not(debug_assertions))] +#![cfg(any(not(debug_assertions), test))] +#![cfg_attr(test, allow(dead_code))] use crate::history_cell::padded_emoji; use crate::key_hint; @@ -11,8 +12,12 @@ use crate::selection_list::selection_option_row; use crate::tui::FrameRequester; use crate::tui::Tui; use crate::tui::TuiEvent; +use crate::update_action::PromptedUpdate; use crate::update_action::UpdateAction; +use crate::update_action::UpdateActionStatus; +use crate::update_action::UpdateBlocker; use crate::updates; +use crate::updates::UpgradeNotice; use color_eyre::Result; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; @@ -29,20 +34,53 @@ use tokio_stream::StreamExt; pub(crate) enum UpdatePromptOutcome { Continue, - RunUpdate(UpdateAction), + RunUpdate(PromptedUpdate), } pub(crate) async fn run_update_prompt_if_needed( tui: &mut Tui, config: &Config, ) -> Result { - let Some(latest_version) = updates::get_upgrade_version_for_popup(config) else { - return Ok(UpdatePromptOutcome::Continue); - }; - let Some(update_action) = crate::update_action::get_update_action() else { + let Some(notice) = updates::get_upgrade_notice_for_popup(config) else { return Ok(UpdatePromptOutcome::Continue); }; + match notice { + UpgradeNotice::Available(latest_version) => { + let update_action = match crate::update_action::get_update_action_status() { + UpdateActionStatus::Ready(update_action) => update_action, + UpdateActionStatus::Blocked(blocker) => { + run_remediation_prompt(tui, RemediationPromptReason::Blocked(blocker)).await?; + return Ok(UpdatePromptOutcome::Continue); + } + UpdateActionStatus::Unavailable => return Ok(UpdatePromptOutcome::Continue), + }; + run_standard_update_prompt(tui, config, latest_version, update_action).await + } + UpgradeNotice::RemediationNeeded(latest_version) => { + run_remediation_prompt( + tui, + RemediationPromptReason::NoOpUpdate { + latest_version: latest_version.clone(), + }, + ) + .await?; + if let Err(err) = + updates::suppress_version_after_remediation(config, &latest_version).await + { + tracing::error!("Failed to persist update remediation suppression: {err}"); + } + Ok(UpdatePromptOutcome::Continue) + } + } +} + +async fn run_standard_update_prompt( + tui: &mut Tui, + config: &Config, + latest_version: String, + update_action: UpdateAction, +) -> Result { let mut screen = UpdatePromptScreen::new(tui.frame_requester(), latest_version.clone(), update_action); tui.draw(u16::MAX, |frame| { @@ -71,7 +109,11 @@ pub(crate) async fn run_update_prompt_if_needed( match screen.selection() { Some(UpdateSelection::UpdateNow) => { tui.terminal.clear()?; - Ok(UpdatePromptOutcome::RunUpdate(update_action)) + Ok(UpdatePromptOutcome::RunUpdate(PromptedUpdate { + action: update_action, + target_version: latest_version, + version_file: updates::version_filepath(config), + })) } Some(UpdateSelection::NotNow) | None => Ok(UpdatePromptOutcome::Continue), Some(UpdateSelection::DontRemind) => { @@ -83,6 +125,33 @@ pub(crate) async fn run_update_prompt_if_needed( } } +async fn run_remediation_prompt(tui: &mut Tui, reason: RemediationPromptReason) -> Result<()> { + let mut screen = RemediationPromptScreen::new(tui.frame_requester(), reason); + tui.draw(u16::MAX, |frame| { + frame.render_widget_ref(&screen, frame.area()); + })?; + + let events = tui.event_stream(); + tokio::pin!(events); + + while !screen.is_done() { + if let Some(event) = events.next().await { + match event { + TuiEvent::Key(key_event) => screen.handle_key(key_event), + TuiEvent::Paste(_) => {} + TuiEvent::Draw | TuiEvent::Resize => { + tui.draw(u16::MAX, |frame| { + frame.render_widget_ref(&screen, frame.area()); + })?; + } + } + } else { + break; + } + } + Ok(()) +} + #[derive(Clone, Copy, Debug, PartialEq, Eq)] enum UpdateSelection { UpdateNow, @@ -239,6 +308,100 @@ impl WidgetRef for &UpdatePromptScreen { } } +#[derive(Clone, Debug)] +enum RemediationPromptReason { + Blocked(UpdateBlocker), + NoOpUpdate { latest_version: String }, +} + +impl RemediationPromptReason { + fn lines(&self) -> Vec { + match self { + Self::Blocked(blocker) => blocker.remediation_lines(), + Self::NoOpUpdate { latest_version } => vec![ + "The previous update command completed, but this Codex executable did not change." + .to_string(), + format!( + "Codex is still running {} while {latest_version} is available.", + env!("CARGO_PKG_VERSION") + ), + "Check which `codex` your shell runs before trying again.".to_string(), + ], + } + } +} + +struct RemediationPromptScreen { + request_frame: FrameRequester, + reason: RemediationPromptReason, + done: bool, +} + +impl RemediationPromptScreen { + fn new(request_frame: FrameRequester, reason: RemediationPromptReason) -> Self { + Self { + request_frame, + reason, + done: false, + } + } + + fn handle_key(&mut self, key_event: KeyEvent) { + if key_event.kind == KeyEventKind::Release { + return; + } + if key_event.modifiers.contains(KeyModifiers::CONTROL) + && matches!(key_event.code, KeyCode::Char('c') | KeyCode::Char('d')) + { + self.finish(); + return; + } + if matches!(key_event.code, KeyCode::Enter | KeyCode::Esc) { + self.finish(); + } + } + + fn finish(&mut self) { + self.done = true; + self.request_frame.schedule_frame(); + } + + fn is_done(&self) -> bool { + self.done + } +} + +impl WidgetRef for &RemediationPromptScreen { + fn render_ref(&self, area: Rect, buf: &mut Buffer) { + Clear.render(area, buf); + let mut column = ColumnRenderable::new(); + + column.push(""); + column.push(Line::from(vec![ + padded_emoji(" ✨").bold().cyan(), + "Update needs attention".bold(), + ])); + column.push(""); + for line in self.reason.lines() { + column.push(Line::from(line).inset(Insets::tlbr( + /*top*/ 0, /*left*/ 2, /*bottom*/ 0, /*right*/ 0, + ))); + } + column.push(""); + column.push( + Line::from(vec![ + "Press ".dim(), + key_hint::plain(KeyCode::Enter).into(), + " to continue".dim(), + ]) + .inset(Insets::tlbr( + /*top*/ 0, /*left*/ 2, /*bottom*/ 0, /*right*/ 0, + )), + ); + column.render(area, buf); + } +} + #[cfg(test)] mod tests { use super::*; @@ -310,4 +473,21 @@ mod tests { screen.handle_key(KeyEvent::new(KeyCode::Down, KeyModifiers::NONE)); assert_eq!(screen.highlighted, UpdateSelection::UpdateNow); } + + #[test] + fn update_remediation_prompt_snapshot() { + let screen = RemediationPromptScreen::new( + FrameRequester::test_dummy(), + RemediationPromptReason::Blocked(UpdateBlocker::NpmGlobalRootMismatch { + running_package_root: "/prefix-a/lib/node_modules/@openai/codex".into(), + npm_package_root: "/prefix-b/lib/node_modules/@openai/codex".into(), + }), + ); + let mut terminal = + Terminal::new(VT100Backend::new(/*width*/ 96, /*height*/ 12)).expect("terminal"); + terminal + .draw(|frame| frame.render_widget_ref(&screen, frame.area())) + .expect("render update remediation prompt"); + insta::assert_snapshot!("update_remediation_prompt_modal", terminal.backend()); + } } diff --git a/codex-rs/tui/src/updates.rs b/codex-rs/tui/src/updates.rs index e99852ead9..cf7f361ec7 100644 --- a/codex-rs/tui/src/updates.rs +++ b/codex-rs/tui/src/updates.rs @@ -1,4 +1,5 @@ -#![cfg(not(debug_assertions))] +#![cfg(any(not(debug_assertions), test))] +#![cfg_attr(test, allow(dead_code))] use crate::legacy_core::config::Config; use crate::npm_registry; @@ -19,6 +20,12 @@ use std::path::PathBuf; use crate::version::CODEX_CLI_VERSION; +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum UpgradeNotice { + Available(String), + RemediationNeeded(String), +} + pub fn get_upgrade_version(config: &Config) -> Option { if !config.check_for_update_on_startup || is_source_build_version(CODEX_CLI_VERSION) { return None; @@ -42,13 +49,18 @@ pub fn get_upgrade_version(config: &Config) -> Option { }); } - info.and_then(|info| { - if is_newer(&info.latest_version, CODEX_CLI_VERSION).unwrap_or(false) { - Some(info.latest_version) - } else { - None - } - }) + info.and_then(latest_upgrade_version) +} + +pub fn get_upgrade_version_for_history(config: &Config) -> Option { + let latest = get_upgrade_version(config)?; + let version_file = version_filepath(config); + if let Ok(info) = read_version_info(&version_file) + && should_show_prompt_update_remediation(&info, &latest) + { + return None; + } + Some(latest) } #[derive(Serialize, Deserialize, Debug, Clone)] @@ -58,6 +70,16 @@ struct VersionInfo { last_checked_at: DateTime, #[serde(default)] dismissed_version: Option, + #[serde(default)] + successful_prompt_update: Option, + #[serde(default)] + suppressed_version: Option, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] +struct SuccessfulPromptUpdate { + from_version: String, + target_version: String, } const VERSION_FILENAME: &str = "version.json"; @@ -75,7 +97,7 @@ struct HomebrewCaskInfo { version: String, } -fn version_filepath(config: &Config) -> PathBuf { +pub(crate) fn version_filepath(config: &Config) -> PathBuf { config.codex_home.join(VERSION_FILENAME).into_path_buf() } @@ -113,12 +135,16 @@ async fn check_for_update(version_file: &Path, action: Option) -> } }; - // Preserve any previously dismissed version if present. + // Preserve local prompt state across version refreshes. let prev_info = read_version_info(version_file).ok(); let info = VersionInfo { latest_version, last_checked_at: Utc::now(), - dismissed_version: prev_info.and_then(|p| p.dismissed_version), + dismissed_version: prev_info.as_ref().and_then(|p| p.dismissed_version.clone()), + successful_prompt_update: prev_info + .as_ref() + .and_then(|p| p.successful_prompt_update.clone()), + suppressed_version: prev_info.and_then(|p| p.suppressed_version), }; let json_line = format!("{}\n", serde_json::to_string(&info)?); @@ -142,22 +168,27 @@ async fn fetch_latest_github_release_version() -> anyhow::Result { extract_version_from_latest_tag(&latest_tag_name) } -/// Returns the latest version to show in a popup, if it should be shown. +/// Returns the upgrade notice to show in a popup, if one should be shown. /// This respects the user's dismissal choice for the current latest version. -pub fn get_upgrade_version_for_popup(config: &Config) -> Option { +pub fn get_upgrade_notice_for_popup(config: &Config) -> Option { if !config.check_for_update_on_startup || is_source_build_version(CODEX_CLI_VERSION) { return None; } let version_file = version_filepath(config); let latest = get_upgrade_version(config)?; - // If the user dismissed this exact version previously, do not show the popup. - if let Ok(info) = read_version_info(&version_file) - && info.dismissed_version.as_deref() == Some(latest.as_str()) - { + let Ok(info) = read_version_info(&version_file) else { + return Some(UpgradeNotice::Available(latest)); + }; + + if info.dismissed_version.as_deref() == Some(latest.as_str()) { return None; } - Some(latest) + if should_show_prompt_update_remediation(&info, &latest) { + Some(UpgradeNotice::RemediationNeeded(latest)) + } else { + Some(UpgradeNotice::Available(latest)) + } } /// Persist a dismissal for the current latest version so we don't show @@ -176,3 +207,129 @@ pub async fn dismiss_version(config: &Config, version: &str) -> anyhow::Result<( tokio::fs::write(version_file, json_line).await?; Ok(()) } + +/// Persist a successful prompt-triggered update attempt so the next launch can +/// detect whether the running executable actually changed. +pub fn record_successful_prompt_update_attempt( + version_file: &Path, + target_version: &str, +) -> anyhow::Result<()> { + let mut info = read_version_info(version_file)?; + info.successful_prompt_update = Some(SuccessfulPromptUpdate { + from_version: CODEX_CLI_VERSION.to_string(), + target_version: target_version.to_string(), + }); + write_version_info_sync(version_file, &info) +} + +/// Suppress future notices for the current latest version after we explain a +/// likely no-op update once. +pub async fn suppress_version_after_remediation( + config: &Config, + version: &str, +) -> anyhow::Result<()> { + let version_file = version_filepath(config); + let mut info = match read_version_info(&version_file) { + Ok(info) => info, + Err(_) => return Ok(()), + }; + info.suppressed_version = Some(version.to_string()); + let json_line = format!("{}\n", serde_json::to_string(&info)?); + if let Some(parent) = version_file.parent() { + tokio::fs::create_dir_all(parent).await?; + } + tokio::fs::write(version_file, json_line).await?; + Ok(()) +} + +fn should_show_prompt_update_remediation(info: &VersionInfo, latest: &str) -> bool { + matches!( + info.successful_prompt_update.as_ref(), + Some(SuccessfulPromptUpdate { + from_version, + target_version, + }) if from_version == CODEX_CLI_VERSION && target_version == latest + ) +} + +fn latest_upgrade_version(info: VersionInfo) -> Option { + if info.suppressed_version.as_deref() == Some(info.latest_version.as_str()) { + return None; + } + if is_newer(&info.latest_version, CODEX_CLI_VERSION).unwrap_or(/*default*/ false) { + Some(info.latest_version) + } else { + None + } +} + +fn write_version_info_sync(version_file: &Path, info: &VersionInfo) -> anyhow::Result<()> { + let json_line = format!("{}\n", serde_json::to_string(info)?); + if let Some(parent) = version_file.parent() { + std::fs::create_dir_all(parent)?; + } + std::fs::write(version_file, json_line)?; + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn version_info(latest_version: &str) -> VersionInfo { + VersionInfo { + latest_version: latest_version.to_string(), + last_checked_at: Utc::now(), + dismissed_version: None, + successful_prompt_update: None, + suppressed_version: None, + } + } + + #[test] + fn successful_prompt_update_for_current_binary_triggers_remediation() { + let mut info = version_info("9.9.9"); + info.successful_prompt_update = Some(SuccessfulPromptUpdate { + from_version: CODEX_CLI_VERSION.to_string(), + target_version: "9.9.9".to_string(), + }); + + assert!(should_show_prompt_update_remediation(&info, "9.9.9")); + assert!(!should_show_prompt_update_remediation(&info, "9.9.10")); + } + + #[test] + fn remediation_suppression_hides_the_same_latest_version() { + let mut info = version_info("9.9.9"); + info.suppressed_version = Some("9.9.9".to_string()); + + assert_eq!(latest_upgrade_version(info), None); + } + + #[test] + fn newer_latest_version_ignores_stale_remediation_suppression() { + let mut info = version_info("9.9.10"); + info.suppressed_version = Some("9.9.9".to_string()); + + assert_eq!(latest_upgrade_version(info), Some("9.9.10".to_string())); + } + + #[test] + fn successful_prompt_update_attempt_is_persisted() -> anyhow::Result<()> { + let tempdir = tempfile::tempdir()?; + let version_file = tempdir.path().join(VERSION_FILENAME); + write_version_info_sync(&version_file, &version_info("9.9.9"))?; + + record_successful_prompt_update_attempt(&version_file, "9.9.9")?; + + let info = read_version_info(&version_file)?; + assert_eq!( + info.successful_prompt_update, + Some(SuccessfulPromptUpdate { + from_version: CODEX_CLI_VERSION.to_string(), + target_version: "9.9.9".to_string(), + }) + ); + Ok(()) + } +}