fix(tui/update): harden guided upgrade flows

This commit is contained in:
James Peter
2025-09-29 17:42:34 +10:00
parent 740be39c47
commit 4a4b70dd71
5 changed files with 117 additions and 34 deletions
+1 -1
View File
@@ -1874,7 +1874,7 @@ impl App<'_> {
SlashCommand::Plan | SlashCommand::Solve | SlashCommand::Code => {
// These should have been expanded already, but handle them anyway
if let AppState::Chat { widget } = &mut self.app_state {
let expanded = command.expand_prompt(&command_text);
let expanded = command.expand_prompt(&command_args);
if let Some(prompt) = expanded {
widget.submit_prompt_with_display(command_text.clone(), prompt);
}
+3 -1
View File
@@ -8655,7 +8655,7 @@ impl ChatWidget<'_> {
crate::updates::UpgradeResolution::Command { command, display } => {
if command.is_empty() {
self.history_push(history_cell::new_error_event(
"`/upgrade` — no upgrade command available for this install.".to_string(),
"`/update` — no upgrade command available for this install.".to_string(),
));
self.request_redraw();
return;
@@ -10366,6 +10366,7 @@ impl ChatWidget<'_> {
Some(cwd),
controller,
controller_rx,
self.config.clone(),
self.config.debug,
);
@@ -10732,6 +10733,7 @@ impl ChatWidget<'_> {
if pending_id == id {
self.bottom_pane
.flash_footer_notice(format!("Upgraded to {version}"));
self.latest_upgrade_version = None;
} else {
self.pending_upgrade_notice = Some((pending_id, version));
}
+46 -2
View File
@@ -1,5 +1,6 @@
use std::sync::mpsc::{channel, Receiver};
use std::sync::{Arc, Mutex};
use std::time::Duration;
use anyhow::{anyhow, Context, Result};
use codex_core::config::Config;
@@ -72,6 +73,7 @@ pub(super) fn start_agent_install_session(
cwd,
controller,
controller_rx,
None,
debug_enabled,
);
}
@@ -92,6 +94,7 @@ pub(super) fn start_prompt_terminal_session(
cwd,
controller,
controller_rx,
None,
debug_enabled,
);
}
@@ -112,6 +115,7 @@ pub(super) fn start_direct_terminal_session(
cwd,
controller,
controller_rx,
None,
debug_enabled,
);
}
@@ -124,6 +128,7 @@ pub(super) fn start_upgrade_terminal_session(
cwd: Option<String>,
controller: TerminalRunController,
controller_rx: Receiver<TerminalRunEvent>,
config: Config,
debug_enabled: bool,
) {
#[cfg(test)]
@@ -136,6 +141,7 @@ pub(super) fn start_upgrade_terminal_session(
cwd,
controller,
controller_rx,
config,
debug_enabled,
);
return;
@@ -151,6 +157,7 @@ pub(super) fn start_upgrade_terminal_session(
cwd,
controller,
controller_rx,
Some(config),
debug_enabled,
);
}
@@ -162,6 +169,7 @@ fn start_guided_terminal_session(
cwd: Option<String>,
controller: TerminalRunController,
controller_rx: Receiver<TerminalRunEvent>,
config: Option<Config>,
debug_enabled: bool,
) {
std::thread::spawn(move || {
@@ -200,6 +208,7 @@ fn start_guided_terminal_session(
cwd.as_deref(),
controller,
&mut controller_rx,
config,
debug_enabled,
) {
let helper = match &mode {
@@ -234,10 +243,14 @@ fn run_guided_loop(
cwd: Option<&str>,
controller: TerminalRunController,
controller_rx: &mut Receiver<TerminalRunEvent>,
provided_config: Option<Config>,
debug_enabled: bool,
) -> Result<()> {
let cfg = Config::load_with_cli_overrides(vec![], ConfigOverrides::default())
.context("loading config")?;
let cfg = match provided_config {
Some(cfg) => cfg,
None => Config::load_with_cli_overrides(vec![], ConfigOverrides::default())
.context("loading config")?,
};
let preferred_auth = if cfg.using_chatgpt_auth {
codex_protocol::mcp_protocol::AuthMode::ChatGPT
} else {
@@ -423,6 +436,8 @@ fn run_guided_loop(
_ => 0,
};
let sandbox_restricted = !matches!(cfg.sandbox_policy, SandboxPolicy::DangerFullAccess);
if let GuidedTerminalMode::DirectCommand { command } = mode {
let wrapped = wrap_command(command);
if wrapped.is_empty() {
@@ -468,6 +483,35 @@ fn run_guided_loop(
message: "Analyzing output…".to_string(),
});
} else if let GuidedTerminalMode::Upgrade { initial_command, .. } = mode {
if sandbox_restricted {
let notice = format!(
"Automatic upgrades require Full Access. Run `{initial_command}` manually in your own shell, then re-run `/update` once it finishes.\n"
);
app_event_tx.send(AppEvent::TerminalChunk {
id: terminal_id,
chunk: notice.clone().into_bytes(),
_is_stderr: true,
});
app_event_tx.send(AppEvent::TerminalUpdateMessage {
id: terminal_id,
message: notice.trim().to_string(),
});
app_event_tx.send(AppEvent::TerminalSetAssistantMessage {
id: terminal_id,
message: "Waiting for manual upgrade…".to_string(),
});
conversation.push(make_message(
"user",
"Automatic execution blocked by sandbox. Await user's manual upgrade before continuing.".to_string(),
));
app_event_tx.send(AppEvent::TerminalExit {
id: terminal_id,
exit_code: None,
_duration: Duration::from_millis(0),
});
return Ok(());
}
let wrapped = wrap_command(initial_command);
if wrapped.is_empty() {
app_event_tx.send(AppEvent::TerminalChunk {
+39 -23
View File
@@ -285,7 +285,7 @@ fn wait_tool_updates_exec_record_via_history_state() {
}),
order: Some(order_meta(0)),
});
pump_app_events(&mut chat, &rx);
let _ = pump_app_events(&mut chat, &rx);
let _ = drain_insert_history(&rx);
let wait_params = json!({
@@ -303,7 +303,7 @@ fn wait_tool_updates_exec_record_via_history_state() {
}),
order: Some(order_meta(1)),
});
pump_app_events(&mut chat, &rx);
let _ = pump_app_events(&mut chat, &rx);
let _ = drain_insert_history(&rx);
chat.handle_codex_event(Event {
@@ -318,7 +318,7 @@ fn wait_tool_updates_exec_record_via_history_state() {
}),
order: Some(order_meta(2)),
});
pump_app_events(&mut chat, &rx);
let _ = pump_app_events(&mut chat, &rx);
let _ = drain_insert_history(&rx);
let exec_record = chat
@@ -369,7 +369,7 @@ fn explore_updates_history_state_via_domain_events() {
}),
order: Some(order_meta(0)),
});
pump_app_events(&mut chat, &rx);
let _ = pump_app_events(&mut chat, &rx);
let _ = drain_insert_history(&rx);
let explore_id = chat
@@ -407,7 +407,7 @@ fn explore_updates_history_state_via_domain_events() {
}),
order: Some(order_meta(1)),
});
pump_app_events(&mut chat, &rx);
let _ = pump_app_events(&mut chat, &rx);
let _ = drain_insert_history(&rx);
let explore_state = chat
@@ -781,14 +781,20 @@ impl ScriptStep {
}
}
fn run_script(chat: &mut ChatWidget<'_>, steps: &[ScriptStep], rx: &std::sync::mpsc::Receiver<AppEvent>) {
fn run_script(
chat: &mut ChatWidget<'_>,
steps: &[ScriptStep],
rx: &std::sync::mpsc::Receiver<AppEvent>,
) -> Vec<AppEvent> {
let mut captured = Vec::new();
for step in steps {
if let ScriptStep::Key(code, modifiers) = step {
chat.handle_key_event(KeyEvent::new(*code, *modifiers));
pump_app_events(chat, rx);
captured.extend(pump_app_events(chat, rx));
}
}
pump_app_events(chat, rx);
captured.extend(pump_app_events(chat, rx));
captured
}
#[test]
@@ -897,12 +903,12 @@ fn agents_terminal_toggle_via_shortcuts() {
assert!(!chat.agents_terminal.active);
chat.handle_key_event(KeyEvent::new(KeyCode::Char('a'), KeyModifiers::CONTROL));
pump_app_events(&mut chat, &rx);
let _ = pump_app_events(&mut chat, &rx);
assert!(chat.agents_terminal.active, "Ctrl+A should open terminal");
// Esc should exit the terminal view
chat.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE));
pump_app_events(&mut chat, &rx);
let _ = pump_app_events(&mut chat, &rx);
assert!(!chat.agents_terminal.active, "Esc should exit terminal");
}
@@ -932,27 +938,27 @@ fn agents_terminal_focus_and_scroll_controls() {
});
chat.handle_key_event(KeyEvent::new(KeyCode::Char('a'), KeyModifiers::CONTROL));
pump_app_events(&mut chat, &rx);
let _ = pump_app_events(&mut chat, &rx);
assert_eq!(chat.agents_terminal.focus(), AgentsTerminalFocus::Sidebar);
chat.layout.last_history_viewport_height.set(5);
chat.layout.last_max_scroll.set(5);
chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
pump_app_events(&mut chat, &rx);
let _ = pump_app_events(&mut chat, &rx);
assert_eq!(chat.agents_terminal.focus(), AgentsTerminalFocus::Detail);
chat.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::NONE));
pump_app_events(&mut chat, &rx);
let _ = pump_app_events(&mut chat, &rx);
assert_eq!(chat.layout.scroll_offset, 1, "Up should scroll output when focused");
chat.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE));
pump_app_events(&mut chat, &rx);
let _ = pump_app_events(&mut chat, &rx);
assert!(chat.agents_terminal.active, "Overlay should remain open after first Esc");
assert_eq!(chat.agents_terminal.focus(), AgentsTerminalFocus::Sidebar);
chat.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE));
pump_app_events(&mut chat, &rx);
let _ = pump_app_events(&mut chat, &rx);
assert!(!chat.agents_terminal.active, "Second Esc should close overlay");
}
@@ -982,16 +988,20 @@ fn agents_terminal_esc_closes_from_sidebar() {
});
chat.handle_key_event(KeyEvent::new(KeyCode::Char('a'), KeyModifiers::CONTROL));
pump_app_events(&mut chat, &rx);
let _ = pump_app_events(&mut chat, &rx);
assert!(chat.agents_terminal.active, "overlay should open");
assert_eq!(chat.agents_terminal.focus(), AgentsTerminalFocus::Sidebar);
chat.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE));
pump_app_events(&mut chat, &rx);
let _ = pump_app_events(&mut chat, &rx);
assert!(!chat.agents_terminal.active, "Esc should close from sidebar focus");
}
fn pump_app_events(chat: &mut ChatWidget<'_>, rx: &std::sync::mpsc::Receiver<AppEvent>) {
fn pump_app_events(
chat: &mut ChatWidget<'_>,
rx: &std::sync::mpsc::Receiver<AppEvent>,
) -> Vec<AppEvent> {
let mut captured = Vec::new();
while let Ok(event) = rx.try_recv() {
match event {
AppEvent::PrepareAgents => chat.prepare_agents(),
@@ -1012,9 +1022,13 @@ fn pump_app_events(chat: &mut ChatWidget<'_>, rx: &std::sync::mpsc::Receiver<App
}
AppEvent::ShowAgentsOverview => chat.show_agents_overview_ui(),
AppEvent::RequestRedraw | AppEvent::Redraw | AppEvent::ScheduleFrameIn(_) => {}
AppEvent::OpenTerminal(_) => {
captured.push(event);
}
_ => {}
}
}
captured
}
fn buffer_to_string(buffer: &ratatui::buffer::Buffer) -> String {
@@ -1070,7 +1084,7 @@ fn slash_agents_opens_overview() {
ScriptStep::key_char('s'),
ScriptStep::enter(),
];
run_script(&mut chat, &script, &rx);
let _ = run_script(&mut chat, &script, &rx);
let width: u16 = 120;
let height = chat.desired_height(width).max(40);
@@ -1092,7 +1106,8 @@ fn slash_agents_opens_overview() {
#[test]
fn slash_upgrade_opens_guided_terminal() {
let _guard = EnvGuard::set("CODEX_MANAGED_BY_NPM", "1");
let _npm_guard = EnvGuard::set("CODEX_MANAGED_BY_NPM", "1");
let _upgrade_guard = EnvGuard::set("SHOW_UPGRADE", "1");
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual();
chat.latest_upgrade_version = Some("9.9.9".to_string());
@@ -1107,10 +1122,11 @@ fn slash_upgrade_opens_guided_terminal() {
ScriptStep::key_char('e'),
ScriptStep::enter(),
];
run_script(&mut chat, &script, &rx);
let mut events = run_script(&mut chat, &script, &rx);
events.extend(rx.try_iter());
let mut saw_open_terminal = false;
while let Ok(event) = rx.try_recv() {
for event in events {
if let AppEvent::OpenTerminal(launch) = event {
saw_open_terminal = true;
assert_eq!(launch.title, "Upgrade Code");
@@ -1137,7 +1153,7 @@ fn slash_undo_shows_no_snapshot_state() {
ScriptStep::key_char('o'),
ScriptStep::enter(),
];
run_script(&mut chat, &script, &rx);
let _ = run_script(&mut chat, &script, &rx);
assert!(
chat.bottom_pane.has_active_modal_view(),
+28 -7
View File
@@ -231,7 +231,7 @@ pub async fn auto_upgrade_if_enabled(config: &Config) -> anyhow::Result<AutoUpgr
if sudo_requires_manual_intervention(&fallback.stderr, fallback.status)
{
outcome.user_notice = Some(format!(
"Automatic upgrade needs your attention. Run `/upgrade` to finish with `{}`.",
"Automatic upgrade needs your attention. Run `/update` to finish with `{}`.",
command_display
));
}
@@ -245,7 +245,7 @@ pub async fn auto_upgrade_if_enabled(config: &Config) -> anyhow::Result<AutoUpgr
Err(err) => {
warn!("auto-upgrade: sudo retry error: {err}");
outcome.user_notice = Some(format!(
"Automatic upgrade could not escalate permissions. Run `/upgrade` to finish with `{}`.",
"Automatic upgrade could not escalate permissions. Run `/update` to finish with `{}`.",
command_display
));
return Ok(outcome);
@@ -415,13 +415,24 @@ fn sudo_requires_manual_intervention(stderr: &str, status: Option<i32>) -> bool
fn truncate_for_log(text: &str) -> String {
const LIMIT: usize = 256;
const ELLIPSIS_BYTES: usize = '…'.len_utf8();
if text.len() <= LIMIT {
text.replace('\n', " ")
} else {
let mut truncated = text[..LIMIT].to_string();
truncated.push_str("…");
truncated.replace('\n', " ")
return text.replace('\n', " ");
}
let slice_limit = LIMIT.saturating_sub(ELLIPSIS_BYTES);
let safe_boundary = text
.char_indices()
.map(|(idx, _)| idx)
.chain(std::iter::once(text.len()))
.take_while(|idx| *idx <= slice_limit)
.last()
.unwrap_or(0);
let safe_slice = text.get(..safe_boundary).unwrap_or("");
let mut truncated = safe_slice.to_string();
truncated.push('…');
truncated.replace('\n', " ")
}
#[cfg(test)]
@@ -442,6 +453,16 @@ mod unit_tests {
assert!(truncated.len() <= 257);
}
#[test]
fn truncate_for_log_handles_multibyte_characters() {
let long = "漢字".repeat(200);
let truncated = truncate_for_log(&long);
assert!(truncated.ends_with('…'));
let prefix = truncated.trim_end_matches('…');
assert!(prefix.len() <= 256);
assert!(long.starts_with(prefix));
}
#[cfg(any(target_os = "macos", target_os = "linux"))]
#[test]
fn wrap_with_sudo_injects_dash_dash() {