From c1afb8815af63c59b67824a73750884aa1a2e363 Mon Sep 17 00:00:00 2001 From: Charley Cunningham Date: Thu, 26 Feb 2026 12:52:31 -0800 Subject: [PATCH] tui: use thread_id for resume/fork cwd resolution (#12727) ## Summary - make resume/fork targets explicit and typed as `SessionTarget { path, thread_id }` (non-optional `thread_id`) - resolve `thread_id` centrally via `resolve_session_thread_id(...)`: - use CLI input directly when it is a UUID (`--resume ` / `--fork `) - otherwise read `thread_id` from rollout `SessionMeta` for path-based selections (picker, `--resume-last`, name-based resume/fork) - use `thread_id` to read cwd from SQLite first during resume/fork cwd resolution - keep rollout fallback for cwd resolution when SQLite is unavailable or does not return thread metadata (`TurnContext` tail, then `SessionMeta`) - keep the resume picker open when a selected row has unreadable session metadata, and show an inline recoverable error instead of aborting the TUI ## Why This removes ad-hoc rollout filename parsing and makes resume/fork target identity explicit. The resume/fork cwd check can use indexed SQLite lookup by `thread_id` in the common path, while preserving rollout-based fallback behavior. It also keeps malformed legacy rows recoverable in the picker instead of letting a selection failure unwind the app. ## Notes - minimal TUI-only change; no schema/protocol changes - includes TUI test coverage for SQLite cwd precedence when `thread_id` is available - includes TUI regression coverage for picker inline error rendering / non-fatal unreadable session rows ## Codex author `codex resume 019c9205-7f8b-7173-a2a2-f082d4df3de3` --- codex-rs/tui/src/app.rs | 59 +++-- codex-rs/tui/src/lib.rs | 230 ++++++++++++++++-- codex-rs/tui/src/resume_picker.rs | 128 +++++++++- ...er__tests__resume_picker_search_error.snap | 5 + 4 files changed, 367 insertions(+), 55 deletions(-) create mode 100644 codex-rs/tui/src/snapshots/codex_tui__resume_picker__tests__resume_picker_search_error.snap diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 490bb0662..99684dded 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -1414,12 +1414,16 @@ impl App { }; ChatWidget::new(init, thread_manager.clone()) } - SessionSelection::Resume(path) => { + SessionSelection::Resume(target_session) => { let resumed = thread_manager - .resume_thread_from_rollout(config.clone(), path.clone(), auth_manager.clone()) + .resume_thread_from_rollout( + config.clone(), + target_session.path.clone(), + auth_manager.clone(), + ) .await .wrap_err_with(|| { - let path_display = path.display(); + let path_display = target_session.path.display(); format!("Failed to resume session from {path_display}") })?; let init = crate::chatwidget::ChatWidgetInit { @@ -1444,13 +1448,18 @@ impl App { }; ChatWidget::new_from_existing(init, resumed.thread, resumed.session_configured) } - SessionSelection::Fork(path) => { + SessionSelection::Fork(target_session) => { otel_manager.counter("codex.thread.fork", 1, &[("source", "cli_subcommand")]); let forked = thread_manager - .fork_thread(usize::MAX, config.clone(), path.clone(), false) + .fork_thread( + usize::MAX, + config.clone(), + target_session.path.clone(), + false, + ) .await .wrap_err_with(|| { - let path_display = path.display(); + let path_display = target_session.path.display(); format!("Failed to fork session from {path_display}") })?; let init = crate::chatwidget::ChatWidgetInit { @@ -1713,12 +1722,14 @@ impl App { } AppEvent::OpenResumePicker => { match crate::resume_picker::run_resume_picker(tui, &self.config, false).await? { - SessionSelection::Resume(path) => { + SessionSelection::Resume(target_session) => { let current_cwd = self.config.cwd.clone(); let resume_cwd = match crate::resolve_cwd_for_resume_or_fork( tui, + &self.config, ¤t_cwd, - &path, + target_session.thread_id, + &target_session.path, CwdPromptAction::Resume, true, ) @@ -1754,7 +1765,7 @@ impl App { .server .resume_thread_from_rollout( resume_config.clone(), - path.clone(), + target_session.path.clone(), self.auth_manager.clone(), ) .await @@ -1788,7 +1799,7 @@ impl App { } } Err(err) => { - let path_display = path.display(); + let path_display = target_session.path.display(); self.chat_widget.add_error_message(format!( "Failed to resume session from {path_display}: {err}" )); @@ -3400,15 +3411,21 @@ mod tests { true ); assert_eq!( - App::should_wait_for_initial_session(&SessionSelection::Resume(PathBuf::from( - "/tmp/restore" - ))), + App::should_wait_for_initial_session(&SessionSelection::Resume( + crate::resume_picker::SessionTarget { + path: PathBuf::from("/tmp/restore"), + thread_id: ThreadId::new(), + } + )), false ); assert_eq!( - App::should_wait_for_initial_session(&SessionSelection::Fork(PathBuf::from( - "/tmp/fork" - ))), + App::should_wait_for_initial_session(&SessionSelection::Fork( + crate::resume_picker::SessionTarget { + path: PathBuf::from("/tmp/fork"), + thread_id: ThreadId::new(), + } + )), false ); } @@ -3444,14 +3461,20 @@ mod tests { #[test] fn startup_waiting_gate_not_applied_for_resume_or_fork_session_selection() { let wait_for_resume = App::should_wait_for_initial_session(&SessionSelection::Resume( - PathBuf::from("/tmp/restore"), + crate::resume_picker::SessionTarget { + path: PathBuf::from("/tmp/restore"), + thread_id: ThreadId::new(), + }, )); assert_eq!( App::should_handle_active_thread_events(wait_for_resume, true), true ); let wait_for_fork = App::should_wait_for_initial_session(&SessionSelection::Fork( - PathBuf::from("/tmp/fork"), + crate::resume_picker::SessionTarget { + path: PathBuf::from("/tmp/fork"), + thread_id: ThreadId::new(), + }, )); assert_eq!( App::should_handle_active_thread_events(wait_for_fork, true), diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index f5d0cab60..cf805d0b9 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -31,8 +31,10 @@ use codex_core::find_thread_path_by_name_str; use codex_core::format_exec_policy_error_with_source; use codex_core::path_utils; use codex_core::read_session_meta_line; +use codex_core::state_db::get_state_db; use codex_core::terminal::Multiplexer; use codex_core::windows_sandbox::WindowsSandboxLevelExt; +use codex_protocol::ThreadId; use codex_protocol::config_types::AltScreenMode; use codex_protocol::config_types::SandboxMode; use codex_protocol::config_types::WindowsSandboxLevel; @@ -665,7 +667,19 @@ async fn run_ratatui_app( find_thread_path_by_name_str(&config.codex_home, id_str).await? }; match path { - Some(path) => resume_picker::SessionSelection::Fork(path), + Some(path) => { + let thread_id = + match resolve_session_thread_id(path.as_path(), is_uuid.then_some(id_str)) + .await + { + Some(thread_id) => thread_id, + None => return missing_session_exit(id_str, "fork"), + }; + resume_picker::SessionSelection::Fork(resume_picker::SessionTarget { + path, + thread_id, + }) + } None => return missing_session_exit(id_str, "fork"), } } else if cli.fork_last { @@ -682,11 +696,37 @@ async fn run_ratatui_app( ) .await { - Ok(page) => page - .items - .first() - .map(|it| resume_picker::SessionSelection::Fork(it.path.clone())) - .unwrap_or(resume_picker::SessionSelection::StartFresh), + Ok(page) => match page.items.first() { + Some(item) => { + match resolve_session_thread_id(item.path.as_path(), None).await { + Some(thread_id) => resume_picker::SessionSelection::Fork( + resume_picker::SessionTarget { + path: item.path.clone(), + thread_id, + }, + ), + None => { + let rollout_path = item.path.display(); + error!( + "Error reading session metadata from latest rollout: {rollout_path}" + ); + restore(); + session_log::log_session_end(); + let _ = tui.terminal.clear(); + return Ok(AppExitInfo { + token_usage: codex_protocol::protocol::TokenUsage::default(), + thread_id: None, + thread_name: None, + update_action: None, + exit_reason: ExitReason::Fatal(format!( + "Found latest saved session at {rollout_path}, but failed to read its metadata. Run `codex fork` to choose from existing sessions." + )), + }); + } + } + } + None => resume_picker::SessionSelection::StartFresh, + }, Err(_) => resume_picker::SessionSelection::StartFresh, } } else if cli.fork_picker { @@ -715,7 +755,21 @@ async fn run_ratatui_app( find_thread_path_by_name_str(&config.codex_home, id_str).await? }; match path { - Some(path) => resume_picker::SessionSelection::Resume(path), + Some(path) => { + let thread_id = match resolve_session_thread_id( + path.as_path(), + is_uuid.then_some(id_str), + ) + .await + { + Some(thread_id) => thread_id, + None => return missing_session_exit(id_str, "resume"), + }; + resume_picker::SessionSelection::Resume(resume_picker::SessionTarget { + path, + thread_id, + }) + } None => return missing_session_exit(id_str, "resume"), } } else if cli.resume_last { @@ -737,7 +791,30 @@ async fn run_ratatui_app( ) .await { - Ok(Some(path)) => resume_picker::SessionSelection::Resume(path), + Ok(Some(path)) => match resolve_session_thread_id(path.as_path(), None).await { + Some(thread_id) => { + resume_picker::SessionSelection::Resume(resume_picker::SessionTarget { + path, + thread_id, + }) + } + None => { + let rollout_path = path.display(); + error!("Error reading session metadata from latest rollout: {rollout_path}"); + restore(); + session_log::log_session_end(); + let _ = tui.terminal.clear(); + return Ok(AppExitInfo { + token_usage: codex_protocol::protocol::TokenUsage::default(), + thread_id: None, + thread_name: None, + update_action: None, + exit_reason: ExitReason::Fatal(format!( + "Found latest saved session at {rollout_path}, but failed to read its metadata. Run `codex resume` to choose from existing sessions." + )), + }); + } + }, _ => resume_picker::SessionSelection::StartFresh, } } else if cli.resume_picker { @@ -761,15 +838,27 @@ async fn run_ratatui_app( let current_cwd = config.cwd.clone(); let allow_prompt = cli.cwd.is_none(); - let action_and_path_if_resume_or_fork = match &session_selection { - resume_picker::SessionSelection::Resume(path) => Some((CwdPromptAction::Resume, path)), - resume_picker::SessionSelection::Fork(path) => Some((CwdPromptAction::Fork, path)), + let action_and_target_session_if_resume_or_fork = match &session_selection { + resume_picker::SessionSelection::Resume(target_session) => { + Some((CwdPromptAction::Resume, target_session)) + } + resume_picker::SessionSelection::Fork(target_session) => { + Some((CwdPromptAction::Fork, target_session)) + } _ => None, }; - let fallback_cwd = match action_and_path_if_resume_or_fork { - Some((action, path)) => { - match resolve_cwd_for_resume_or_fork(&mut tui, ¤t_cwd, path, action, allow_prompt) - .await? + let fallback_cwd = match action_and_target_session_if_resume_or_fork { + Some((action, target_session)) => { + match resolve_cwd_for_resume_or_fork( + &mut tui, + &config, + ¤t_cwd, + target_session.thread_id, + &target_session.path, + action, + allow_prompt, + ) + .await? { ResolveCwdOutcome::Continue(cwd) => cwd, ResolveCwdOutcome::Exit => { @@ -851,12 +940,35 @@ async fn run_ratatui_app( app_result } -pub(crate) async fn read_session_cwd(path: &Path) -> Option { +pub(crate) async fn resolve_session_thread_id( + path: &Path, + id_str_if_uuid: Option<&str>, +) -> Option { + match id_str_if_uuid { + Some(id_str) => ThreadId::from_string(id_str).ok(), + None => read_session_meta_line(path) + .await + .ok() + .map(|meta_line| meta_line.meta.id), + } +} + +pub(crate) async fn read_session_cwd( + config: &Config, + thread_id: ThreadId, + path: &Path, +) -> Option { + if let Some(state_db_ctx) = get_state_db(config, None).await + && let Ok(Some(metadata)) = state_db_ctx.get_thread(thread_id).await + { + return Some(metadata.cwd); + } + // Prefer the latest TurnContext cwd so resume/fork reflects the most recent - // session directory (for the changed-cwd prompt). The alternative would be - // mutating the SessionMeta line when the session cwd changes, but the rollout - // is an append-only JSONL log and rewriting the head would be error-prone. - // When rollouts move to SQLite, we can drop this scan. + // session directory (for the changed-cwd prompt) when DB data is unavailable. + // The alternative would be mutating the SessionMeta line when the session cwd + // changes, but the rollout is an append-only JSONL log and rewriting the head + // would be error-prone. if let Some(cwd) = parse_latest_turn_context_cwd(path).await { return Some(cwd); } @@ -908,12 +1020,14 @@ pub(crate) enum ResolveCwdOutcome { pub(crate) async fn resolve_cwd_for_resume_or_fork( tui: &mut Tui, + config: &Config, current_cwd: &Path, + thread_id: ThreadId, path: &Path, action: CwdPromptAction, allow_prompt: bool, ) -> color_eyre::Result { - let Some(history_cwd) = read_session_cwd(path).await else { + let Some(history_cwd) = read_session_cwd(config, thread_id, path).await else { return Ok(ResolveCwdOutcome::Continue(None)); }; if allow_prompt && cwds_differ(current_cwd, &history_cwd) { @@ -1071,11 +1185,14 @@ mod tests { use codex_core::config::ConfigBuilder; use codex_core::config::ConfigOverrides; use codex_core::config::ProjectConfig; + use codex_core::features::Feature; + use codex_protocol::ThreadId; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::RolloutItem; use codex_protocol::protocol::RolloutLine; use codex_protocol::protocol::SessionMeta; use codex_protocol::protocol::SessionMetaLine; + use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::TurnContextItem; use serial_test::serial; use tempfile::TempDir; @@ -1196,7 +1313,9 @@ mod tests { } std::fs::write(&rollout_path, text)?; - let cwd = read_session_cwd(&rollout_path).await.expect("expected cwd"); + let cwd = read_session_cwd(&config, ThreadId::new(), &rollout_path) + .await + .expect("expected cwd"); assert_eq!(cwd, second); Ok(()) } @@ -1236,7 +1355,9 @@ mod tests { } std::fs::write(&rollout_path, text)?; - let session_cwd = read_session_cwd(&rollout_path).await.expect("expected cwd"); + let session_cwd = read_session_cwd(&config, ThreadId::new(), &rollout_path) + .await + .expect("expected cwd"); assert_eq!(session_cwd, latest); assert!(cwds_differ(¤t, &session_cwd)); Ok(()) @@ -1341,7 +1462,7 @@ trust_level = "untrusted" #[tokio::test] async fn read_session_cwd_falls_back_to_session_meta() -> std::io::Result<()> { let temp_dir = TempDir::new()?; - let _config = build_config(&temp_dir).await?; + let config = build_config(&temp_dir).await?; let session_cwd = temp_dir.path().join("session"); std::fs::create_dir_all(&session_cwd)?; @@ -1363,8 +1484,67 @@ trust_level = "untrusted" ); std::fs::write(&rollout_path, text)?; - let cwd = read_session_cwd(&rollout_path).await.expect("expected cwd"); + let cwd = read_session_cwd(&config, ThreadId::new(), &rollout_path) + .await + .expect("expected cwd"); assert_eq!(cwd, session_cwd); Ok(()) } + + #[tokio::test] + async fn read_session_cwd_prefers_sqlite_when_thread_id_present() -> std::io::Result<()> { + let temp_dir = TempDir::new()?; + let mut config = build_config(&temp_dir).await?; + config.features.enable(Feature::Sqlite); + + let thread_id = ThreadId::new(); + let rollout_cwd = temp_dir.path().join("rollout-cwd"); + let sqlite_cwd = temp_dir.path().join("sqlite-cwd"); + std::fs::create_dir_all(&rollout_cwd)?; + std::fs::create_dir_all(&sqlite_cwd)?; + + let rollout_path = temp_dir.path().join("rollout.jsonl"); + let rollout_line = RolloutLine { + timestamp: "t0".to_string(), + item: RolloutItem::TurnContext(build_turn_context(&config, rollout_cwd)), + }; + std::fs::write( + &rollout_path, + format!( + "{}\n", + serde_json::to_string(&rollout_line).expect("serialize rollout") + ), + )?; + + let runtime = codex_state::StateRuntime::init( + config.codex_home.clone(), + config.model_provider_id.clone(), + None, + ) + .await + .map_err(std::io::Error::other)?; + runtime + .mark_backfill_complete(None) + .await + .map_err(std::io::Error::other)?; + + let mut builder = codex_state::ThreadMetadataBuilder::new( + thread_id, + rollout_path.clone(), + chrono::Utc::now(), + SessionSource::Cli, + ); + builder.cwd = sqlite_cwd.clone(); + let metadata = builder.build(config.model_provider_id.as_str()); + runtime + .upsert_thread(&metadata) + .await + .map_err(std::io::Error::other)?; + + let cwd = read_session_cwd(&config, thread_id, &rollout_path) + .await + .expect("expected cwd"); + assert_eq!(cwd, sqlite_cwd); + Ok(()) + } } diff --git a/codex-rs/tui/src/resume_picker.rs b/codex-rs/tui/src/resume_picker.rs index a827ccab5..ad8156284 100644 --- a/codex-rs/tui/src/resume_picker.rs +++ b/codex-rs/tui/src/resume_picker.rs @@ -39,11 +39,18 @@ use unicode_width::UnicodeWidthStr; const PAGE_SIZE: usize = 25; const LOAD_NEAR_THRESHOLD: usize = 5; + +#[derive(Debug, Clone)] +pub struct SessionTarget { + pub path: PathBuf, + pub thread_id: ThreadId, +} + #[derive(Debug, Clone)] pub enum SessionSelection { StartFresh, - Resume(PathBuf), - Fork(PathBuf), + Resume(SessionTarget), + Fork(SessionTarget), Exit, } @@ -68,10 +75,11 @@ impl SessionPickerAction { } } - fn selection(self, path: PathBuf) -> SessionSelection { + fn selection(self, path: PathBuf, thread_id: ThreadId) -> SessionSelection { + let target_session = SessionTarget { path, thread_id }; match self { - SessionPickerAction::Resume => SessionSelection::Resume(path), - SessionPickerAction::Fork => SessionSelection::Fork(path), + SessionPickerAction::Resume => SessionSelection::Resume(target_session), + SessionPickerAction::Fork => SessionSelection::Fork(target_session), } } } @@ -266,6 +274,7 @@ struct PickerState { action: SessionPickerAction, sort_key: ThreadSortKey, thread_name_cache: HashMap>, + inline_error: Option, } struct PaginationState { @@ -383,6 +392,7 @@ impl PickerState { action, sort_key: ThreadSortKey::CreatedAt, thread_name_cache: HashMap::new(), + inline_error: None, } } @@ -391,6 +401,7 @@ impl PickerState { } async fn handle_key(&mut self, key: KeyEvent) -> Result> { + self.inline_error = None; match key.code { KeyCode::Esc => return Ok(Some(SessionSelection::StartFresh)), KeyCode::Char('c') @@ -402,7 +413,19 @@ impl PickerState { } KeyCode::Enter => { if let Some(row) = self.filtered_rows.get(self.selected) { - return Ok(Some(self.action.selection(row.path.clone()))); + let path = row.path.clone(); + let thread_id = match row.thread_id { + Some(thread_id) => Some(thread_id), + None => crate::resolve_session_thread_id(path.as_path(), None).await, + }; + if let Some(thread_id) = thread_id { + return Ok(Some(self.action.selection(path, thread_id))); + } + self.inline_error = Some(format!( + "Failed to read session metadata from {}", + path.display() + )); + self.request_frame(); } } KeyCode::Up => { @@ -866,12 +889,7 @@ fn draw_picker(tui: &mut Tui, state: &PickerState) -> std::io::Result<()> { frame.render_widget_ref(header_line, header); // Search line - let q = if state.query.is_empty() { - "Type to search".dim().to_string() - } else { - format!("Search: {}", state.query) - }; - frame.render_widget_ref(Line::from(q), search); + frame.render_widget_ref(search_line(state), search); let metrics = calculate_column_metrics(&state.filtered_rows, state.show_all); @@ -904,6 +922,16 @@ fn draw_picker(tui: &mut Tui, state: &PickerState) -> std::io::Result<()> { }) } +fn search_line(state: &PickerState) -> Line<'_> { + if let Some(error) = state.inline_error.as_deref() { + return Line::from(error.red()); + } + if state.query.is_empty() { + return Line::from("Type to search".dim()); + } + Line::from(format!("Search: {}", state.query)) +} + fn render_list( frame: &mut crate::custom_terminal::Frame, area: Rect, @@ -1607,6 +1635,42 @@ mod tests { assert_snapshot!("resume_picker_table", snapshot); } + #[test] + fn resume_search_error_snapshot() { + use crate::custom_terminal::Terminal; + use crate::test_backend::VT100Backend; + + let loader: PageLoader = Arc::new(|_| {}); + let mut state = PickerState::new( + PathBuf::from("/tmp"), + FrameRequester::test_dummy(), + loader, + String::from("openai"), + true, + None, + SessionPickerAction::Resume, + ); + state.inline_error = Some(String::from( + "Failed to read session metadata from /tmp/missing.jsonl", + )); + + let width: u16 = 80; + let height: u16 = 1; + let backend = VT100Backend::new(width, height); + let mut terminal = Terminal::with_options(backend).expect("terminal"); + terminal.set_viewport_area(Rect::new(0, 0, width, height)); + + { + let mut frame = terminal.get_frame(); + let line = search_line(&state); + frame.render_widget_ref(line, frame.area()); + } + terminal.flush().expect("flush"); + + let snapshot = terminal.backend().to_string(); + assert_snapshot!("resume_picker_search_error", snapshot); + } + // TODO(jif) fix // #[tokio::test] // async fn resume_picker_screen_snapshot() { @@ -2102,6 +2166,46 @@ mod tests { assert_eq!(state.selected, 5); } + #[tokio::test] + async fn enter_on_row_without_resolvable_thread_id_shows_inline_error() { + let loader: PageLoader = Arc::new(|_| {}); + let mut state = PickerState::new( + PathBuf::from("/tmp"), + FrameRequester::test_dummy(), + loader, + String::from("openai"), + true, + None, + SessionPickerAction::Resume, + ); + + let row = Row { + path: PathBuf::from("/tmp/missing.jsonl"), + preview: String::from("missing metadata"), + thread_id: None, + thread_name: None, + created_at: None, + updated_at: None, + cwd: None, + git_branch: None, + }; + state.all_rows = vec![row.clone()]; + state.filtered_rows = vec![row]; + + let selection = state + .handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)) + .await + .expect("enter should not abort the picker"); + + assert!(selection.is_none()); + assert_eq!( + state.inline_error, + Some(String::from( + "Failed to read session metadata from /tmp/missing.jsonl" + )) + ); + } + #[tokio::test] async fn up_at_bottom_does_not_scroll_when_visible() { let loader: PageLoader = Arc::new(|_| {}); diff --git a/codex-rs/tui/src/snapshots/codex_tui__resume_picker__tests__resume_picker_search_error.snap b/codex-rs/tui/src/snapshots/codex_tui__resume_picker__tests__resume_picker_search_error.snap new file mode 100644 index 000000000..4f48ef96a --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__resume_picker__tests__resume_picker_search_error.snap @@ -0,0 +1,5 @@ +--- +source: tui/src/resume_picker.rs +expression: snapshot +--- +Failed to read session metadata from /tmp/missing.jsonl