From 02d4529f55342dee025a5c13f612b72488dfa627 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Mon, 7 Sep 2026 00:44:16 +0000 Subject: [PATCH] Resolve session labels uniquely before acting on them (#43315) ## Why Selecting the first matching session name can target the wrong conversation when labels are duplicated. Session commands also need to accept preview text when no explicit name is set. ## What changed - Share app-server label lookup across resume, fork, archive, unarchive, delete, and queue commands, matching the trimmed name or fallback preview. - Reject distinct matching sessions across collections and source filters, and require a UUID when pagination prevents verifying uniqueness. - Revalidate listed candidates with `thread/read`, skip unusable local rollouts, and retain compatibility with older servers that cannot read unloaded threads. - Show ambiguous resume errors in the TUI without replacing the current conversation. - Disable implicit local daemon reuse with `--oss`. ## Testing Add regression coverage for name and preview lookup, provider filtering, duplicate labels across pages and collections, stale rollouts, older-server compatibility, queue ambiguity, and preserving the current conversation after an ambiguous resume. GitOrigin-RevId: e2766276a16140fb674c9640f7050c11ac0d0f6f --- codex-rs/cli/tests/worktree.rs | 6 +- codex-rs/tui/src/app/event_dispatch.rs | 14 +- codex-rs/tui/src/app/tests.rs | 91 +++ codex-rs/tui/src/lib.rs | 13 +- codex-rs/tui/src/named_session_lookup.rs | 414 +++++-------- .../tui/src/named_session_lookup_tests.rs | 570 +++++++++--------- codex-rs/tui/src/session_archive_commands.rs | 139 +---- .../tui/src/session_archive_commands_tests.rs | 197 +++--- codex-rs/tui/src/session_queue_commands.rs | 18 +- ...__tests__duplicate_named_resume_error.snap | 8 + codex-rs/tui/src/startup_orchestration.rs | 1 + 11 files changed, 699 insertions(+), 772 deletions(-) create mode 100644 codex-rs/tui/src/snapshots/codex_tui__app__tests__duplicate_named_resume_error.snap diff --git a/codex-rs/cli/tests/worktree.rs b/codex-rs/cli/tests/worktree.rs index 12058440fe..593569493f 100644 --- a/codex-rs/cli/tests/worktree.rs +++ b/codex-rs/cli/tests/worktree.rs @@ -307,7 +307,11 @@ trust_level = "trusted" "--add-dir".into(), "extra".into(), "fork".into(), - "managed-source".to_owned(), + if explicit_cd { + owner.clone().context("previous fork owner")? + } else { + "managed-source".to_owned() + }, ]); } else { args.extend(["--cd".into(), source.display().to_string()]); diff --git a/codex-rs/tui/src/app/event_dispatch.rs b/codex-rs/tui/src/app/event_dispatch.rs index 48b0dbad22..ce5e56c931 100644 --- a/codex-rs/tui/src/app/event_dispatch.rs +++ b/codex-rs/tui/src/app/event_dispatch.rs @@ -314,18 +314,26 @@ impl App { &self.config, &id_or_name, ) - .await? + .await { - Some(target_session) => { + Ok(Some(target_session)) => { return self .resume_target_session(tui, app_server, target_session) .await; } - None => { + Ok(None) => { self.chat_widget.add_error_message(format!( "No saved chat found matching '{id_or_name}'." )); } + Err(err) + if err + .downcast_ref::() + .is_some() => + { + self.chat_widget.add_error_message(err.to_string()); + } + Err(err) => return Err(err), } } AppEvent::ArchiveCurrentThread => { diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index 1cf1caf9be..4e0c3bcae0 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -218,6 +218,97 @@ async fn pasted_text_normalizes_mixed_line_endings_at_app_boundary() -> Result<( Ok(()) } +#[tokio::test] +async fn duplicate_named_resume_renders_error_without_replacing_current_session() -> Result<()> { + let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await; + let label = format!("duplicate-{}", ThreadId::new()); + let runtime = codex_state::StateRuntime::init( + app.config.sqlite.clone(), + app.config.model_provider_id.clone(), + ) + .await + .map_err(std::io::Error::other)?; + runtime + .mark_backfill_complete(/*last_watermark*/ None) + .await + .map_err(std::io::Error::other)?; + let mut thread_ids = Vec::new(); + for _ in 0..2 { + let filename_ts = "2025-02-01T10-00-00"; + let timestamp = "2025-02-01T10:00:00Z"; + let thread_id = app_test_support::create_fake_rollout( + app.config.codex_home.as_path(), + filename_ts, + timestamp, + &label, + Some(&app.config.model_provider_id), + /*git_info*/ None, + ) + .expect("materialized rollout should be created"); + let path = app_test_support::rollout_path( + app.config.codex_home.as_path(), + filename_ts, + &thread_id, + ); + let thread_id = ThreadId::from_string(&thread_id)?; + thread_ids.push(thread_id); + let created_at = + chrono::DateTime::parse_from_rfc3339(timestamp)?.with_timezone(&chrono::Utc); + let mut metadata = codex_state::ThreadMetadataBuilder::new( + thread_id, + path.to_path_buf(), + created_at, + serde_json::from_value(serde_json::json!("cli"))?, + ) + .build(&app.config.model_provider_id); + metadata.title = label.clone(); + metadata.first_user_message = Some(label.clone()); + metadata.preview = Some(label.clone()); + runtime + .upsert_thread(&metadata) + .await + .map_err(std::io::Error::other)?; + } + + let current = ThreadId::new(); + app.active_thread_id = Some(current); + app.transcript_cells = vec![plain_line_cell("Existing conversation")]; + let mut app_server = crate::start_embedded_app_server_for_picker(&app.config).await?; + let mut tui = crate::tui::test_support::make_test_tui()?; + app.handle_event( + &mut tui, + &mut app_server, + AppEvent::ResumeSessionByIdOrName(label.clone()), + ) + .await?; + while let Ok(event) = app_event_rx.try_recv() { + if let AppEvent::InsertHistoryCell(cell) = event { + app.transcript_cells.push(Arc::from(cell)); + } + } + let rendered = app + .render_transcript_lines_for_reflow(/*width*/ 80) + .lines + .iter() + .map(rendered_line_text) + .collect::>() + .join("\n"); + assert!( + thread_ids + .iter() + .all(|id| rendered.contains(&id.to_string())) + ); + let rendered = thread_ids + .iter() + .fold(rendered.replace(&label, "duplicate-label"), |text, id| { + text.replace(&id.to_string(), "") + }); + assert_app_snapshot!("duplicate_named_resume_error", rendered); + assert_eq!(app.active_thread_id, Some(current)); + app_server.shutdown().await?; + Ok(()) +} + #[tokio::test] async fn chat_widget_frame_reuses_active_cell_height_across_frame_passes() { #[derive(Debug)] diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 5514c66907..b512fd4323 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -716,7 +716,18 @@ async fn lookup_session_target_with_app_server( }; } - named_session_lookup::lookup(app_server, config, id_or_name).await + let model_provider = + (!app_server.uses_remote_workspace()).then_some(config.model_provider_id.as_str()); + Ok(named_session_lookup::lookup( + app_server, + config.codex_home.as_path(), + id_or_name, + &[named_session_lookup::SessionCollection::Active], + &[resume_source_kinds(/*include_non_interactive*/ false)], + model_provider, + ) + .await? + .and_then(session_target_from_app_server_thread)) } async fn lookup_latest_session_target_with_app_server( diff --git a/codex-rs/tui/src/named_session_lookup.rs b/codex-rs/tui/src/named_session_lookup.rs index 741c847abc..c9ee458924 100644 --- a/codex-rs/tui/src/named_session_lookup.rs +++ b/codex-rs/tui/src/named_session_lookup.rs @@ -1,301 +1,175 @@ +//! Resolve a displayed session label through the app server before acting on its thread ID. + use std::path::Path; use crate::app_server_session::AppServerSession; -use crate::legacy_core::config::Config; -use crate::resume_picker::SessionTarget; -use codex_app_server_protocol::Thread as AppServerThread; +use codex_app_server_client::TypedRequestError; +use codex_app_server_protocol::Thread; use codex_app_server_protocol::ThreadHistoryMode; use codex_app_server_protocol::ThreadListParams; use codex_app_server_protocol::ThreadSortKey; use codex_app_server_protocol::ThreadSourceKind; use codex_protocol::ThreadId; -use codex_protocol::protocol::SessionMeta; -use codex_protocol::protocol::SessionSource; +use color_eyre::eyre::Result; +use color_eyre::eyre::WrapErr; -// Keep this aligned with `resume_source_kinds(/*include_non_interactive*/ false)`. -const RESUME_SESSION_SOURCES: [SessionSource; 2] = [SessionSource::Cli, SessionSource::VSCode]; - -/// Selects whether a local lookup can repair SQLite metadata from rollout files. -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub(super) enum SessionNameLookupMode { - StateDbOnly, - ScanAndRepair, -} - -/// Selects the rollout collection that a local candidate must belong to. -#[derive(Clone, Copy, Debug, Eq, PartialEq)] +#[derive(Clone, Copy)] pub(super) enum SessionCollection { Active, Archived, } -/// An exact-name match and the rollout metadata verified for local candidates. -pub(super) struct NamedSessionCandidate { - pub(super) thread: AppServerThread, - /// Remote candidates never inspect local rollout files and therefore have no session metadata. - pub(super) session_meta: Option, +#[derive(Debug, thiserror::Error)] +pub(crate) enum AmbiguousSessionName { + #[error( + "Multiple sessions match '{name}' (including {first_id} and {second_id}); use a session UUID to disambiguate." + )] + Multiple { + name: String, + first_id: String, + second_id: String, + }, + #[error( + "Cannot verify a unique session label across server pages; matching session UUID: {0}. Use it only if this is the session you want." + )] + Paginated(String), } -/// Lazily pages through exact-name matches and skips unusable local rollouts. -pub(super) struct NamedSessionCandidates<'a> { - name: &'a str, - codex_home: &'a Path, - collection: SessionCollection, - mode: SessionNameLookupMode, - search_term: Option<&'a str>, - source_kinds: Vec, - cursor: Option, - pending: std::vec::IntoIter, - has_more: bool, +pub(super) fn display_label(thread: &Thread) -> &str { + thread + .name + .as_deref() + .map(str::trim) + .filter(|name| !name.is_empty()) + .unwrap_or_else(|| thread.preview.trim()) } -/// Resolves an active session by exact name. -/// -/// Local lookups treat a usable SQLite match as authoritative and consult legacy rollout metadata -/// only to recover a missing or unusable row. Remote workspaces retain the backend's scan behavior. +/// Resolve server-listed labels and reject distinct matches before selecting an ID. pub(super) async fn lookup( app_server: &mut AppServerSession, - config: &Config, + codex_home: &Path, name: &str, -) -> color_eyre::Result> { - if app_server.uses_remote_workspace() { - return Ok(lookup_from_app_server( - app_server, - config, - name, - SessionNameLookupMode::ScanAndRepair, - ) - .await? - .and_then(super::session_target_from_app_server_thread)); + collections: &[SessionCollection], + source_kind_filters: &[Vec], + model_provider: Option<&str>, +) -> Result> { + if name.trim().is_empty() { + return Ok(None); } - - if let Some(thread) = - lookup_from_app_server(app_server, config, name, SessionNameLookupMode::StateDbOnly).await? - { - return Ok(super::session_target_from_app_server_thread(thread)); - } - - if let Some(target) = lookup_legacy_index_target(app_server, config, name).await? { - return Ok(Some(target)); - } - - Ok(lookup_from_app_server( - app_server, - config, - name, - SessionNameLookupMode::ScanAndRepair, - ) - .await? - .and_then(super::session_target_from_app_server_thread)) -} - -/// Recovers a local target from legacy metadata without overriding an explicit SQLite name. -/// -/// Rewriting the recovered name into SQLite is best effort and cannot invalidate a usable target. -async fn lookup_legacy_index_target( - app_server: &mut AppServerSession, - config: &Config, - name: &str, -) -> color_eyre::Result> { - // Older rollouts omitted the provider and were historically assigned to the current provider - // during scans, so only reject an explicit mismatch here. - let candidates = codex_rollout::find_thread_meta_candidates_by_name_str( - config.codex_home.as_path(), - name, - /*state_db_ctx*/ None, - &RESUME_SESSION_SOURCES, - std::slice::from_ref(&config.model_provider_id), - ) - .await?; - for (path, session_meta) in candidates { - if !path.starts_with(config.codex_home.join(codex_rollout::SESSIONS_SUBDIR)) { - continue; - } - - // `thread/read` resolves the candidate's current SQLite title. A conflicting title means - // the sidecar is stale, while a missing legacy title is the partial-write case it recovers. - let thread = app_server - .thread_read(session_meta.meta.id, /*include_turns*/ false) - .await?; - if !current_name_is_compatible(&thread, name) { - continue; - } - if let Err(err) = app_server - .thread_set_name(session_meta.meta.id, name.to_string()) - .await - { - tracing::warn!( - thread_id = %session_meta.meta.id, - %err, - "failed to repair recovered thread name" - ); - } - return Ok(Some(SessionTarget { - path: Some(path), - thread_id: session_meta.meta.id, - history_mode: Some(thread.history_mode), - })); - } - Ok(None) -} - -/// Allows a missing SQLite name only for legacy threads, whose name may exist only in the index. -pub(super) fn current_name_is_compatible(thread: &AppServerThread, name: &str) -> bool { - match thread.history_mode { - ThreadHistoryMode::Legacy => thread.name.as_deref().is_none_or(|current| current == name), - ThreadHistoryMode::Paginated => thread.name.as_deref() == Some(name), - } -} - -/// Returns an eligible active thread for one SQLite-only or scan-and-repair lookup pass. -/// -/// Local resume candidates also honor source/provider filters and reject stale scanned titles. -async fn lookup_from_app_server( - app_server: &mut AppServerSession, - config: &Config, - name: &str, - mode: SessionNameLookupMode, -) -> color_eyre::Result> { - let mut candidates = NamedSessionCandidates::new( - name, - config.codex_home.as_path(), - SessionCollection::Active, - mode, - Some(name), - super::resume_source_kinds(/*include_non_interactive*/ false), - ); - while let Some(candidate) = candidates.next(app_server).await? { - if let Some(session_meta) = candidate.session_meta - && (!RESUME_SESSION_SOURCES.contains(&session_meta.source) - || session_meta - .model_provider - .as_deref() - .is_some_and(|provider| provider != config.model_provider_id.as_str())) - { - continue; - } - if mode == SessionNameLookupMode::ScanAndRepair && !app_server.uses_remote_workspace() { - // Scans can surface stale sidecar names, so verify the candidate's current title. - let thread = app_server - .thread_read( - ThreadId::from_string(&candidate.thread.id)?, - /*include_turns*/ false, - ) - .await?; - if !current_name_is_compatible(&thread, name) { - continue; - } - return Ok(Some(thread)); - } - return Ok(Some(candidate.thread)); - } - Ok(None) -} - -impl<'a> NamedSessionCandidates<'a> { - pub(super) fn new( - name: &'a str, - codex_home: &'a Path, - collection: SessionCollection, - mode: SessionNameLookupMode, - search_term: Option<&'a str>, - source_kinds: Vec, - ) -> Self { - Self { - name, - codex_home, - collection, - mode, - search_term, - source_kinds, - cursor: None, - pending: Vec::new().into_iter(), - has_more: true, - } - } - - /// Returns the next exact-name match, advancing through server pages as needed. - /// - /// Local candidates must have a rollout in the requested collection with a matching session - /// header; remote candidates preserve server-side lookup without touching local files. - pub(super) async fn next( - &mut self, - app_server: &mut AppServerSession, - ) -> color_eyre::Result> { - loop { - if let Some(thread) = self.pending.next() { - if thread.name.as_deref() != Some(self.name) { - continue; - } - let session_meta = if app_server.uses_remote_workspace() { - None - } else { - let (Ok(thread_id), Some(path)) = - (ThreadId::from_string(&thread.id), thread.path.as_deref()) - else { - continue; - }; - let Some(path) = codex_rollout::existing_rollout_path(path).await else { - continue; - }; - let expected_root = self.codex_home.join(match self.collection { - SessionCollection::Active => codex_rollout::SESSIONS_SUBDIR, - SessionCollection::Archived => codex_rollout::ARCHIVED_SESSIONS_SUBDIR, - }); - if !path.starts_with(expected_root) { - continue; - } - let Ok(session_meta) = - codex_rollout::read_session_meta_line(path.as_path()).await - else { - continue; - }; - if session_meta.meta.id != thread_id { - continue; - } - Some(session_meta.meta) - }; - return Ok(Some(NamedSessionCandidate { - thread, - session_meta, - })); - } - - if !self.has_more { - return Ok(None); - } - // Only embedded SQLite supports recency sorting; scanners and older daemons use mtime. - let sort_key = if self.mode == SessionNameLookupMode::StateDbOnly - && app_server.uses_embedded_app_server() - { + let mut matched: Option = None; + let mut paginated = false; + for collection in collections { + for source_kinds in source_kind_filters { + let mut cursor = None; + let sort_key = if app_server.uses_embedded_app_server() { ThreadSortKey::RecencyAt } else { ThreadSortKey::UpdatedAt }; - let response = app_server - .thread_list(ThreadListParams { - originators: None, - cursor: self.cursor.clone(), - limit: Some(100), - sort_key: Some(sort_key), - sort_direction: None, - model_providers: None, - source_kinds: Some(self.source_kinds.clone()), - archived: Some(self.collection == SessionCollection::Archived), - section_id: None, - project_id: None, - parent_thread_id: None, - ancestor_thread_id: None, - cwd: None, - use_state_db_only: self.mode == SessionNameLookupMode::StateDbOnly, - search_term: self.search_term.map(str::to_string), - }) - .await?; - self.cursor = response.next_cursor; - self.has_more = self.cursor.is_some(); - self.pending = response.data.into_iter(); + loop { + let response = app_server + .thread_list(ThreadListParams { + originators: None, + cursor, + limit: Some(100), + sort_key: Some(sort_key), + sort_direction: None, + model_providers: model_provider.map(|provider| vec![provider.to_string()]), + source_kinds: Some(source_kinds.clone()), + archived: Some(matches!(collection, SessionCollection::Archived)), + section_id: None, + project_id: None, + parent_thread_id: None, + ancestor_thread_id: None, + cwd: None, + use_state_db_only: false, + search_term: None, + }) + .await + .wrap_err("failed to list sessions while resolving session label")?; + paginated |= response.next_cursor.is_some(); + for thread in response.data { + if display_label(&thread) != name { + continue; + } + if !app_server.uses_remote_workspace() + && let Some(path) = thread.path.as_ref() + { + let expected_root = codex_home.join(match collection { + SessionCollection::Active => codex_rollout::SESSIONS_SUBDIR, + SessionCollection::Archived => codex_rollout::ARCHIVED_SESSIONS_SUBDIR, + }); + if !path.starts_with(expected_root) + || (thread.history_mode == ThreadHistoryMode::Legacy + && codex_rollout::existing_rollout_path(path).await.is_none()) + { + continue; + } + } + let thread_id = ThreadId::from_string(&thread.id).wrap_err_with(|| { + format!("app server returned invalid session id `{}`", thread.id) + })?; + let current = match app_server + .thread_read(thread_id, /*include_turns*/ false) + .await + { + Ok(current) => current, + Err(err) => { + let Some(TypedRequestError::Server { source, .. }) = + err.downcast_ref::() + else { + return Err(err); + }; + if source.message == format!("thread not loaded: {thread_id}") { + if app_server.uses_embedded_app_server() { + continue; + } + thread.clone() + } else if (source.message.starts_with("failed to read thread: thread-store internal error: session metadata ") + && source.message.contains(" belongs to thread ") + && source.message.ends_with(&format!(", expected {thread_id}"))) + || thread.path.as_ref().is_some_and(|path| { + source.message.starts_with(&format!( + "failed to read thread: thread-store internal error: failed to read session metadata {}: ", + path.display() + )) + }) + { + continue; + } else { + return Err(err); + } + } + }; + if current.id != thread.id || display_label(¤t) != name { + continue; + } + if let Some(previous) = matched.as_ref() + && previous.id != current.id + { + return Err(AmbiguousSessionName::Multiple { + name: name.to_string(), + first_id: previous.id.clone(), + second_id: current.id, + } + .into()); + } + matched = Some(current); + } + let Some(next_cursor) = response.next_cursor else { + break; + }; + cursor = Some(next_cursor); + } } } + // Older server cursors can skip equal timestamps at a page boundary. + if let Some(thread) = matched.as_ref() + && paginated + { + return Err(AmbiguousSessionName::Paginated(thread.id.clone()).into()); + } + Ok(matched) } #[cfg(test)] diff --git a/codex-rs/tui/src/named_session_lookup_tests.rs b/codex-rs/tui/src/named_session_lookup_tests.rs index eb6e01ad93..06805b0f76 100644 --- a/codex-rs/tui/src/named_session_lookup_tests.rs +++ b/codex-rs/tui/src/named_session_lookup_tests.rs @@ -1,6 +1,10 @@ use std::path::PathBuf; use std::sync::Arc; +use codex_app_server_client::RemoteAppServerClient; +use codex_app_server_client::RemoteAppServerConnectArgs; +use codex_app_server_client::RemoteAppServerEndpoint; +use codex_app_server_protocol::JSONRPCMessage; use codex_protocol::ThreadId; use codex_protocol::protocol::SessionMeta; use codex_protocol::protocol::SessionMetaLine; @@ -8,14 +12,20 @@ use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::ThreadHistoryMode; use codex_state::ThreadMetadataBuilder; use codex_utils_absolute_path::test_support::PathExt; +use futures::SinkExt; +use futures::StreamExt; use pretty_assertions::assert_eq; +use serde_json::json; use tempfile::TempDir; +use tokio_tungstenite::tungstenite::Message; +use super::SessionCollection; use super::lookup; use crate::app_server_session::AppServerSession; use crate::app_server_session::ThreadParamsMode; use crate::legacy_core::config::Config; use crate::legacy_core::config::ConfigBuilder; +use crate::resume_source_kinds; use crate::tests::start_test_embedded_app_server; async fn build_config(temp_dir: &TempDir) -> std::io::Result { @@ -42,14 +52,25 @@ async fn state_runtime(config: &Config) -> std::io::Result color_eyre::Result> { + collections: &[SessionCollection], + mode: ThreadParamsMode, + model_provider: Option<&str>, +) -> color_eyre::Result> { let mut app_server = AppServerSession::new( codex_app_server_client::AppServerClient::InProcess( start_test_embedded_app_server(config.clone()).await?, ), - ThreadParamsMode::Embedded, + mode, ); - let target = lookup(&mut app_server, config, name).await?; + let target = lookup( + &mut app_server, + config.codex_home.as_path(), + name, + collections, + &[resume_source_kinds(/*include_non_interactive*/ false)], + model_provider, + ) + .await?; app_server.shutdown().await?; Ok(target) } @@ -131,6 +152,15 @@ fn write_rollout( "content": [{"type": "input_text", "text": preview}], }, }), + serde_json::json!({ + "timestamp": timestamp, + "type": "event_msg", + "payload": { + "type": "user_message", + "message": preview, + "kind": "plain", + }, + }), ]; std::fs::write( &rollout_path, @@ -144,228 +174,181 @@ fn write_rollout( Ok(rollout_path.to_path_buf()) } -async fn add_scan_sentinel(config: &Config, search_term: &str) -> color_eyre::Result { - let thread_id = ThreadId::new(); - write_rollout( - config, - thread_id, - "2025-02-01T11:00:00Z", - "scan sentinel", - SessionSource::Cli, - ThreadHistoryMode::Legacy, - )?; - codex_rollout::append_thread_name( - config.codex_home.as_path(), - thread_id, - &format!("{search_term}-scan-sentinel"), - ) - .await?; - Ok(thread_id) -} - #[tokio::test] -async fn uses_sqlite_title_without_scanning_rollouts() -> color_eyre::Result<()> { +async fn resolves_name_and_preview_from_server_list() -> color_eyre::Result<()> { let temp_dir = TempDir::new()?; let config = build_config(&temp_dir).await?; let runtime = state_runtime(&config).await?; - let thread_id = ThreadId::new(); - let rollout_path = write_rollout( + let named_id = ThreadId::new(); + let named_path = write_rollout( &config, - thread_id, + named_id, "2025-02-01T10:00:00Z", - "preview", + "preview text", SessionSource::Cli, ThreadHistoryMode::Legacy, )?; upsert_thread( &runtime, - thread_metadata(&config, thread_id, rollout_path.clone(), "saved-session"), + thread_metadata(&config, named_id, named_path, "saved-session"), ) .await?; - // Scan-and-repair would discover this matching rollout and insert it into SQLite. - let scan_sentinel_id = add_scan_sentinel(&config, "saved-session").await?; - let duplicate_id = ThreadId::new(); - write_rollout( + let preview_id = ThreadId::new(); + let preview_path = write_rollout( &config, - duplicate_id, - "2025-02-01T11:00:00Z", - "newer duplicate", + preview_id, + "2025-02-02T10:00:00Z", + "preview text", SessionSource::Cli, ThreadHistoryMode::Legacy, )?; - codex_rollout::append_thread_name(config.codex_home.as_path(), duplicate_id, "saved-session") - .await?; - - let target = lookup_name(&config, "saved-session").await?; + upsert_thread( + &runtime, + thread_metadata(&config, preview_id, preview_path, "preview text"), + ) + .await?; + let other_id = ThreadId::new(); + let mut other_config = config.clone(); + other_config.model_provider_id = "another-provider".to_string(); + let other_path = write_rollout( + &other_config, + other_id, + "2025-02-03T10:00:00Z", + "preview text", + SessionSource::Cli, + ThreadHistoryMode::Legacy, + )?; + upsert_thread( + &runtime, + thread_metadata(&other_config, other_id, other_path, "saved-session"), + ) + .await?; + let named = lookup_name( + &config, + "saved-session", + &[SessionCollection::Active], + ThreadParamsMode::Embedded, + Some(&config.model_provider_id), + ) + .await?; + let preview = lookup_name( + &config, + "preview text", + &[SessionCollection::Active], + ThreadParamsMode::Embedded, + Some(&config.model_provider_id), + ) + .await?; + let switched_provider = lookup_name( + &config, + "saved-session", + &[SessionCollection::Active], + ThreadParamsMode::Embedded, + Some(&other_config.model_provider_id), + ) + .await?; + let remote = lookup_name( + &other_config, + "saved-session", + &[SessionCollection::Active], + ThreadParamsMode::Remote, + /*model_provider*/ None, + ) + .await?; assert_eq!( ( - target.map(|target| (target.path, target.thread_id)), - runtime - .get_thread(scan_sentinel_id) - .await - .map_err(std::io::Error::other)?, - runtime - .get_thread(duplicate_id) - .await - .map_err(std::io::Error::other)?, + named.map(|thread| thread.id), + preview.map(|thread| thread.id), + switched_provider.map(|thread| thread.id), + remote.map(|thread| thread.id), + ), + ( + Some(named_id.to_string()), + Some(preview_id.to_string()), + Some(other_id.to_string()), + Some(other_id.to_string()), ), - (Some((Some(rollout_path), thread_id)), None, None), ); Ok(()) } #[tokio::test] -async fn skips_stale_sqlite_candidate_for_valid_duplicate() -> color_eyre::Result<()> { +async fn rejects_duplicate_labels_across_server_pages() -> color_eyre::Result<()> { let temp_dir = TempDir::new()?; let config = build_config(&temp_dir).await?; let runtime = state_runtime(&config).await?; - let valid_id = ThreadId::new(); - let valid_path = write_rollout( - &config, - valid_id, - "2025-02-01T10:00:00Z", - "valid preview", - SessionSource::Cli, - ThreadHistoryMode::Legacy, - )?; - upsert_thread( - &runtime, - thread_metadata(&config, valid_id, valid_path.clone(), "saved-session"), - ) - .await?; - - let stale_id = ThreadId::new(); - let stale_path = write_rollout( - &config, - ThreadId::new(), - "2025-02-01T11:00:00Z", - "stale preview", - SessionSource::Cli, - ThreadHistoryMode::Legacy, - )?; - upsert_thread( - &runtime, - thread_metadata(&config, stale_id, stale_path, "saved-session"), - ) - .await?; - - let target = lookup_name(&config, "saved-session").await?; - - assert_eq!( - target.map(|target| (target.path, target.thread_id)), - Some((Some(valid_path), valid_id)), - ); - Ok(()) -} - -#[tokio::test] -async fn validates_sqlite_candidate_filters_against_rollout() -> color_eyre::Result<()> { - let mut targets = Vec::new(); - for (source, model_provider) in [ - (SessionSource::Exec, None), - (SessionSource::Cli, Some("other-provider")), - ] { - let temp_dir = TempDir::new()?; - let config = build_config(&temp_dir).await?; - let runtime = state_runtime(&config).await?; + let mut relevant_ids = Vec::new(); + for index in 0..102 { let thread_id = ThreadId::new(); - let mut rollout_config = config.clone(); - if let Some(model_provider) = model_provider { - rollout_config.model_provider_id = model_provider.to_string(); + if matches!(index, 0 | 1 | 101) { + relevant_ids.push(thread_id); } let rollout_path = write_rollout( - &rollout_config, + &config, thread_id, "2025-02-01T10:00:00Z", - "preview", - source, + "preview text", + SessionSource::Cli, ThreadHistoryMode::Legacy, )?; - upsert_thread( - &runtime, - thread_metadata(&config, thread_id, rollout_path, "saved-session"), - ) - .await?; - - targets.push( - lookup_name(&config, "saved-session") - .await? - .map(|target| target.thread_id), - ); + let name = if index == 0 || index == 101 { + "same-label".to_string() + } else { + format!("other-{index}") + }; + let mut metadata = thread_metadata(&config, thread_id, rollout_path, &name); + metadata.recency_at += chrono::Duration::seconds(index); + metadata.updated_at += chrono::Duration::seconds(index); + upsert_thread(&runtime, metadata).await?; } - assert_eq!(targets, vec![None, None]); - Ok(()) -} - -#[tokio::test] -async fn recovers_legacy_index_name_without_requiring_sidecar_rewrite() -> color_eyre::Result<()> { - let temp_dir = TempDir::new()?; - let config = build_config(&temp_dir).await?; - let runtime = state_runtime(&config).await?; - let stale_id = ThreadId::new(); - upsert_thread( - &runtime, - thread_metadata( - &config, - stale_id, - config - .codex_home - .join("missing-rollout.jsonl") - .to_path_buf(), - "saved-session", + let mut app_server = AppServerSession::new( + codex_app_server_client::AppServerClient::InProcess( + start_test_embedded_app_server(config.clone()).await?, ), + ThreadParamsMode::Embedded, + ); + let error = lookup( + &mut app_server, + config.codex_home.as_path(), + "same-label", + &[SessionCollection::Active], + &[resume_source_kinds(/*include_non_interactive*/ false)], + Some(&config.model_provider_id), ) - .await?; - - let thread_id = ThreadId::new(); - let rollout_path = write_rollout( - &config, - thread_id, - "2025-02-01T11:00:00Z", - "preview", - SessionSource::Cli, - ThreadHistoryMode::Legacy, - )?; - codex_rollout::append_thread_name(config.codex_home.as_path(), thread_id, "saved-session") - .await?; - // The direct index lookup should not scan and repair other matching rollouts. - let scan_sentinel_id = add_scan_sentinel(&config, "saved-session").await?; - - let index_path = config.codex_home.join("session_index.jsonl"); - let original_permissions = std::fs::metadata(&index_path)?.permissions(); - let mut read_only_permissions = original_permissions.clone(); - read_only_permissions.set_readonly(true); - std::fs::set_permissions(&index_path, read_only_permissions)?; - let target = lookup_name(&config, "saved-session").await; - std::fs::set_permissions(&index_path, original_permissions)?; - let target = target?; - + .await + .expect_err("duplicate labels should require an ID"); + let unverified = lookup( + &mut app_server, + config.codex_home.as_path(), + "other-1", + &[SessionCollection::Active], + &[resume_source_kinds(/*include_non_interactive*/ false)], + Some(&config.model_provider_id), + ) + .await + .expect_err("paginated listings cannot prove uniqueness on older servers"); + app_server.shutdown().await?; assert_eq!( - ( - target.map(|target| (target.path, target.thread_id)), - runtime - .get_thread(thread_id) - .await - .map_err(std::io::Error::other)? - .map(|metadata| metadata.title), - runtime - .get_thread(scan_sentinel_id) - .await - .map_err(std::io::Error::other)?, - ), - ( - Some((Some(rollout_path), thread_id)), - Some("saved-session".to_string()), - None, - ), + error.to_string(), + format!( + "Multiple sessions match 'same-label' (including {} and {}); use a session UUID to disambiguate.", + relevant_ids[2], relevant_ids[0] + ) + ); + assert_eq!( + unverified.to_string(), + format!( + "Cannot verify a unique session label across server pages; matching session UUID: {}. Use it only if this is the session you want.", + relevant_ids[1] + ) ); Ok(()) } #[tokio::test] -async fn skips_stale_legacy_index_candidate() -> color_eyre::Result<()> { +async fn skips_stale_listed_thread_before_valid_label() -> color_eyre::Result<()> { let temp_dir = TempDir::new()?; let config = build_config(&temp_dir).await?; let runtime = state_runtime(&config).await?; @@ -374,140 +357,161 @@ async fn skips_stale_legacy_index_candidate() -> color_eyre::Result<()> { &config, stale_id, "2025-02-01T10:00:00Z", - "preview", + "same-label", SessionSource::Cli, ThreadHistoryMode::Legacy, )?; upsert_thread( &runtime, - thread_metadata(&config, stale_id, stale_path.clone(), "current-session"), + thread_metadata(&config, stale_id, stale_path.clone(), "same-label"), ) .await?; - codex_rollout::append_thread_name(config.codex_home.as_path(), stale_id, "saved-session") - .await?; + std::fs::remove_file(stale_path)?; - let fallback_id = ThreadId::new(); - let fallback_path = write_rollout( + let corrupt_id = ThreadId::new(); + let corrupt_path = write_rollout( &config, - fallback_id, - "2025-02-01T11:00:00Z", - "fallback", + corrupt_id, + "2025-02-01T10:00:00Z", + "same-label", SessionSource::Cli, ThreadHistoryMode::Legacy, )?; - codex_rollout::append_thread_name(config.codex_home.as_path(), fallback_id, "saved-session") - .await?; - for (path, timestamp) in [ - (&stale_path, "2025-02-04T10:00:00Z"), - (&fallback_path, "2025-02-03T10:00:00Z"), - ] { - let modified = chrono::DateTime::parse_from_rfc3339(timestamp)?.into(); - std::fs::OpenOptions::new() - .append(true) - .open(path)? - .set_times(std::fs::FileTimes::new().set_modified(modified))?; - } + upsert_thread( + &runtime, + thread_metadata(&config, corrupt_id, corrupt_path.clone(), "same-label"), + ) + .await?; + std::fs::write(corrupt_path, "invalid session header\n")?; - let target = lookup_name(&config, "saved-session").await?; + let valid_id = ThreadId::new(); + let valid_path = write_rollout( + &config, + valid_id, + "2025-02-01T10:00:01Z", + "same-label", + SessionSource::Cli, + ThreadHistoryMode::Legacy, + )?; + upsert_thread( + &runtime, + thread_metadata(&config, valid_id, valid_path.clone(), "same-label"), + ) + .await?; + let wrong_id = ThreadId::new(); + upsert_thread( + &runtime, + thread_metadata(&config, wrong_id, valid_path, "same-label"), + ) + .await?; - assert_eq!( - ( - target.map(|target| (target.path, target.thread_id)), - runtime - .get_thread(stale_id) - .await - .map_err(std::io::Error::other)? - .map(|metadata| metadata.title), - ), - ( - Some((Some(fallback_path), fallback_id)), - Some("current-session".to_string()), - ), - ); + let found = lookup_name( + &config, + "same-label", + &[SessionCollection::Active], + ThreadParamsMode::Embedded, + Some(&config.model_provider_id), + ) + .await?; + assert_eq!(found.map(|thread| thread.id), Some(valid_id.to_string())); Ok(()) } #[tokio::test] -async fn keeps_newer_paginated_sqlite_name_when_index_is_stale() -> color_eyre::Result<()> { +async fn uses_listed_thread_when_older_server_cannot_read_it() -> color_eyre::Result<()> { let temp_dir = TempDir::new()?; let config = build_config(&temp_dir).await?; let runtime = state_runtime(&config).await?; let thread_id = ThreadId::new(); - let rollout_path = write_rollout( + let path = write_rollout( &config, thread_id, "2025-02-01T10:00:00Z", - "preview", + "preview text", SessionSource::Cli, - ThreadHistoryMode::Paginated, + ThreadHistoryMode::Legacy, )?; - let mut metadata = thread_metadata(&config, thread_id, rollout_path, "preview text"); - metadata.history_mode = ThreadHistoryMode::Paginated; - upsert_thread(&runtime, metadata).await?; - runtime - .update_thread_name(thread_id, Some("current-session")) - .await - .map_err(std::io::Error::other)?; - codex_rollout::append_thread_name(config.codex_home.as_path(), thread_id, "old-session") - .await?; - - let target = lookup_name(&config, "old-session").await?; - - assert_eq!( - ( - target.map(|target| target.thread_id), - runtime - .get_thread(thread_id) - .await - .map_err(std::io::Error::other)? - .and_then(|metadata| metadata.name), + upsert_thread( + &runtime, + thread_metadata(&config, thread_id, path, "saved-session"), + ) + .await?; + let mut embedded = AppServerSession::new( + codex_app_server_client::AppServerClient::InProcess( + start_test_embedded_app_server(config.clone()).await?, ), - (None, Some("current-session".to_string())), + ThreadParamsMode::Embedded, ); - Ok(()) -} + let listed = embedded + .thread_read(thread_id, /*include_turns*/ false) + .await?; + embedded.shutdown().await?; -#[tokio::test] -async fn prefers_most_recent_duplicate_legacy_name() -> color_eyre::Result<()> { - let temp_dir = TempDir::new()?; - let config = build_config(&temp_dir).await?; - let _runtime = state_runtime(&config).await?; - let recent_id = ThreadId::new(); - let recent_path = write_rollout( - &config, - recent_id, - "2025-02-01T10:00:00Z", - "recent", - SessionSource::Cli, - ThreadHistoryMode::Legacy, - )?; - let older_id = ThreadId::new(); - let older_path = write_rollout( - &config, - older_id, - "2025-02-02T10:00:00Z", - "older", - SessionSource::Cli, - ThreadHistoryMode::Legacy, - )?; - codex_rollout::append_thread_name(config.codex_home.as_path(), recent_id, "same-name").await?; - codex_rollout::append_thread_name(config.codex_home.as_path(), older_id, "same-name").await?; - for (path, timestamp) in [ - (&recent_path, "2025-02-04T10:00:00Z"), - (&older_path, "2025-02-03T10:00:00Z"), - ] { - let modified = chrono::DateTime::parse_from_rfc3339(timestamp)?.into(); - std::fs::OpenOptions::new() - .append(true) - .open(path)? - .set_times(std::fs::FileTimes::new().set_modified(modified))?; + for mode in [ThreadParamsMode::Remote, ThreadParamsMode::Embedded] { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await?; + let url = format!("ws://{}", listener.local_addr()?); + let listed_for_server = listed.clone(); + let server = tokio::spawn(async move { + let (stream, _) = listener.accept().await.unwrap(); + let mut socket = tokio_tungstenite::accept_async(stream).await.unwrap(); + while let Some(Ok(Message::Text(text))) = socket.next().await { + let JSONRPCMessage::Request(request) = serde_json::from_str(&text).unwrap() else { + continue; + }; + let response = match request.method.as_str() { + "initialize" => { + json!({"id": request.id, "result": {"userAgent": "legacy-test"}}) + } + "thread/list" => json!({ + "id": request.id, + "result": {"data": [listed_for_server.clone()], "nextCursor": null} + }), + "thread/read" => json!({ + "id": request.id, + "error": { + "code": -32603, + "message": format!("thread not loaded: {thread_id}") + } + }), + other => panic!("unexpected request: {other}"), + }; + socket + .send(Message::Text(response.to_string().into())) + .await + .unwrap(); + if request.method == "thread/read" { + break; + } + } + }); + let client = RemoteAppServerClient::connect(RemoteAppServerConnectArgs { + endpoint: RemoteAppServerEndpoint::WebSocket { + websocket_url: url, + auth_token: None, + }, + client_name: "legacy-test".to_string(), + client_version: "0.0.0".to_string(), + experimental_api: true, + mcp_server_openai_form_elicitation: false, + opt_out_notification_methods: Vec::new(), + channel_capacity: 8, + }) + .await?; + let mut remote = AppServerSession::new( + codex_app_server_client::AppServerClient::Remote(client), + mode, + ); + let found = lookup( + &mut remote, + config.codex_home.as_path(), + "saved-session", + &[SessionCollection::Active], + &[resume_source_kinds(/*include_non_interactive*/ false)], + /*model_provider*/ None, + ) + .await?; + assert_eq!(found, Some(listed.clone())); + server.await?; } - - let target = lookup_name(&config, "same-name").await?; - - assert_eq!( - target.map(|target| (target.path, target.thread_id)), - Some((Some(recent_path), recent_id)), - ); Ok(()) } diff --git a/codex-rs/tui/src/session_archive_commands.rs b/codex-rs/tui/src/session_archive_commands.rs index bdc62ee83e..69f3f4a7d5 100644 --- a/codex-rs/tui/src/session_archive_commands.rs +++ b/codex-rs/tui/src/session_archive_commands.rs @@ -16,10 +16,9 @@ use crate::legacy_core::config::ConfigOverrides; use crate::legacy_core::config::load_config_toml_with_layer_stack; use crate::legacy_core::config::resolve_oss_provider; use crate::legacy_core::config::resolve_profile_v2_config_path; -use crate::named_session_lookup::NamedSessionCandidates; use crate::named_session_lookup::SessionCollection; -use crate::named_session_lookup::SessionNameLookupMode; -use crate::named_session_lookup::current_name_is_compatible; +use crate::named_session_lookup::display_label; +use crate::named_session_lookup::lookup; use codex_app_server_protocol::Thread as AppServerThread; use codex_arg0::Arg0DispatchPaths; use codex_config::CloudConfigBundleLoader; @@ -56,12 +55,6 @@ pub struct SessionArchiveCommandOptions { pub explicit_remote_endpoint: Option, } -#[derive(Clone, Copy, PartialEq, Eq)] -pub(super) enum SessionNameMatch { - First, - FirstIncludingNonInteractive, -} - fn success_message( action: SessionArchiveAction, session_id: ThreadId, @@ -161,125 +154,39 @@ async fn resolve_session_target( }); } - let (search_scope, archived_values): (&str, &[bool]) = match action { - SessionArchiveAction::Archive => ("active", &[false]), - SessionArchiveAction::Delete(_) => ("active or archived", &[false, true]), - SessionArchiveAction::Unarchive => ("archived", &[true]), + let (search_scope, collections): (&str, &[SessionCollection]) = match action { + SessionArchiveAction::Archive => ("active", &[SessionCollection::Active]), + SessionArchiveAction::Delete(_) => ( + "active or archived", + &[SessionCollection::Active, SessionCollection::Archived], + ), + SessionArchiveAction::Unarchive => ("archived", &[SessionCollection::Archived]), }; - for &archived in archived_values { - if let Some(thread) = lookup_session_by_exact_name( - app_server, - codex_home, - target, - archived, - SessionNameMatch::First, - ) - .await? - { - return session_target_from_app_server_thread(thread); - } + if let Some(thread) = lookup( + app_server, + codex_home, + target, + collections, + &[super::resume_source_kinds( + /*include_non_interactive*/ false, + )], + /*model_provider*/ None, + ) + .await? + { + return session_target_from_app_server_thread(thread); } Err(eyre!( "No {search_scope} session found matching '{target}'." )) } -pub(super) async fn lookup_session_by_exact_name( - app_server: &mut AppServerSession, - codex_home: &Path, - name: &str, - archived: bool, - match_policy: SessionNameMatch, -) -> Result> { - // Remote workspaces stay on their existing server-side path. Local workspaces trust SQLite - // names, then scan and repair only after a miss or an unusable rollout path. - let lookup_modes = if app_server.uses_remote_workspace() { - &[SessionNameLookupMode::ScanAndRepair][..] - } else { - &[ - SessionNameLookupMode::StateDbOnly, - SessionNameLookupMode::ScanAndRepair, - ][..] - }; - let source_kind_filters = if match_policy == SessionNameMatch::FirstIncludingNonInteractive { - // An empty filter includes Atlas/ChatGPT sessions; explicit kinds additionally include exec. - vec![ - super::resume_source_kinds(/*include_non_interactive*/ true), - Vec::new(), - ] - } else { - vec![super::resume_source_kinds( - /*include_non_interactive*/ false, - )] - }; - for &lookup_mode in lookup_modes { - let sort_by_recency = lookup_mode == SessionNameLookupMode::StateDbOnly - && app_server.uses_embedded_app_server(); - // Search is the fast path, but legacy stores attach renamed titles after filtering. - for search_term in [Some(name), None] { - let mut first_match: Option = None; - for source_kinds in &source_kind_filters { - let mut candidates = NamedSessionCandidates::new( - name, - codex_home, - if archived { - SessionCollection::Archived - } else { - SessionCollection::Active - }, - lookup_mode, - search_term, - source_kinds.clone(), - ); - while let Some(candidate) = candidates - .next(app_server) - .await - .wrap_err("failed to list sessions while resolving session name")? - { - let thread = if match_policy == SessionNameMatch::FirstIncludingNonInteractive - && lookup_mode == SessionNameLookupMode::ScanAndRepair - && !app_server.uses_remote_workspace() - { - let thread = app_server - .thread_read( - ThreadId::from_string(&candidate.thread.id)?, - /*include_turns*/ false, - ) - .await?; - if !current_name_is_compatible(&thread, name) { - continue; - } - thread - } else { - candidate.thread - }; - if first_match.as_ref().is_none_or(|existing| { - if sort_by_recency { - thread.recency_at.unwrap_or(thread.updated_at) - > existing.recency_at.unwrap_or(existing.updated_at) - } else { - thread.updated_at > existing.updated_at - } - }) { - first_match = Some(thread); - } - break; - } - } - if first_match.is_some() { - return Ok(first_match); - } - } - } - Ok(None) -} - fn session_target_from_app_server_thread(thread: AppServerThread) -> Result { let session_id = ThreadId::from_string(&thread.id) .wrap_err_with(|| format!("app server returned invalid session id `{}`", thread.id))?; Ok(ResolvedSessionTarget { session_id, - session_name: thread.name, + session_name: Some(display_label(&thread).to_string()), }) } diff --git a/codex-rs/tui/src/session_archive_commands_tests.rs b/codex-rs/tui/src/session_archive_commands_tests.rs index 54841fa63e..f715257094 100644 --- a/codex-rs/tui/src/session_archive_commands_tests.rs +++ b/codex-rs/tui/src/session_archive_commands_tests.rs @@ -252,78 +252,122 @@ async fn unarchives_by_sqlite_name() -> color_eyre::Result<()> { } #[tokio::test] -async fn deletes_valid_duplicate_after_stale_sqlite_hit() -> color_eyre::Result<()> { +async fn delete_refuses_same_label_in_active_and_archived_sessions() -> color_eyre::Result<()> { let temp_dir = TempDir::new()?; let config = build_config(&temp_dir).await?; let runtime = state_runtime(&config).await?; - let stale_id = ThreadId::new(); - let stale_rollout_path = write_rollout( - &config, - stale_id, - /*archived*/ true, - "2025-02-01T10:00:00Z", - "stale preview", - SessionSource::Cli, - )?; - runtime - .upsert_thread(&thread_metadata( + let mut paths = Vec::new(); + let mut thread_ids = Vec::new(); + for archived in [false, true] { + let thread_id = ThreadId::new(); + thread_ids.push(thread_id); + let path = write_rollout( &config, - stale_id, - stale_rollout_path.clone(), - "saved-session", - /*archived*/ false, - )) - .await - .map_err(std::io::Error::other)?; - - let thread_id = ThreadId::new(); - let rollout_path = write_rollout( - &config, - thread_id, - /*archived*/ false, - "2025-02-01T10:00:00Z", - "preview", - SessionSource::Cli, - )?; - codex_rollout::append_thread_name(config.codex_home.as_path(), thread_id, "saved-session") - .await?; + thread_id, + archived, + "2025-02-01T10:00:00Z", + "preview text", + SessionSource::Cli, + )?; + runtime + .upsert_thread(&thread_metadata( + &config, + thread_id, + path.clone(), + "preview text", + archived, + )) + .await + .map_err(std::io::Error::other)?; + paths.push(path); + } let mut app_server = start_app_server(config.clone()).await?; - let message = run_session_archive_action_with_app_server( + let error = run_session_archive_action_with_app_server( &mut app_server, config.codex_home.as_path(), SessionArchiveAction::Delete(DeleteConfirmation::Skip), - "saved-session", + "preview text", ) - .await?; + .await + .expect_err("duplicate previews must not pick a deletion target"); app_server.shutdown().await?; - assert_eq!( + (error.to_string(), paths.iter().all(|path| path.exists())), ( - message, - rollout_path.exists(), - stale_rollout_path.exists(), - runtime - .get_thread(thread_id) - .await - .map_err(std::io::Error::other)?, - runtime - .get_thread(stale_id) - .await - .map_err(std::io::Error::other)? - .is_some(), - ), - ( - format!("Deleted session saved-session ({thread_id})."), - false, - true, - None, + format!( + "Multiple sessions match 'preview text' (including {} and {}); use a session UUID to disambiguate.", + thread_ids[0], thread_ids[1] + ), true, ), ); Ok(()) } +#[tokio::test] +async fn refuses_action_with_stale_sqlite_collection() -> color_eyre::Result<()> { + let temp_dir = TempDir::new()?; + let config = build_config(&temp_dir).await?; + let runtime = state_runtime(&config).await?; + let cases = [ + (false, SessionArchiveAction::Archive, "active"), + (true, SessionArchiveAction::Unarchive, "archived"), + ]; + let mut paths = Vec::new(); + for (listed_archived, _, _) in cases { + let stale_id = ThreadId::new(); + let stale_rollout_path = write_rollout( + &config, + stale_id, + /*archived*/ !listed_archived, + "2025-02-01T10:00:00Z", + "stale preview", + SessionSource::Cli, + )?; + runtime + .upsert_thread(&thread_metadata( + &config, + stale_id, + stale_rollout_path.clone(), + &format!("stale-{listed_archived}"), + listed_archived, + )) + .await + .map_err(std::io::Error::other)?; + paths.push((stale_id, stale_rollout_path)); + } + + let mut app_server = start_app_server(config.clone()).await?; + for (listed_archived, action, scope) in cases { + let name = format!("stale-{listed_archived}"); + let error = run_session_archive_action_with_app_server( + &mut app_server, + config.codex_home.as_path(), + action, + &name, + ) + .await + .expect_err("a stale row must not select a rollout in the wrong collection"); + assert_eq!( + error.to_string(), + format!("No {scope} session found matching '{name}'.") + ); + } + app_server.shutdown().await?; + for (id, path) in paths { + assert!(path.exists()); + assert!( + runtime + .get_thread(id) + .await + .map_err(std::io::Error::other)? + .is_some() + ); + } + Ok(()) +} + #[tokio::test] async fn trusts_sqlite_name_over_legacy_index_for_delete() -> color_eyre::Result<()> { let temp_dir = TempDir::new()?; @@ -393,8 +437,7 @@ async fn trusts_sqlite_name_over_legacy_index_for_delete() -> color_eyre::Result } #[tokio::test] -async fn queues_non_interactive_and_custom_sessions_without_scanning_rollouts() --> color_eyre::Result<()> { +async fn queues_non_interactive_and_custom_sessions_by_server_label() -> color_eyre::Result<()> { let temp_dir = TempDir::new()?; let config = build_config(&temp_dir).await?; let runtime = state_runtime(&config).await?; @@ -466,21 +509,6 @@ async fn queues_non_interactive_and_custom_sessions_without_scanning_rollouts() .upsert_thread(&custom_metadata) .await .map_err(std::io::Error::other)?; - let legacy_thread_id = ThreadId::new(); - write_rollout( - &config, - legacy_thread_id, - /*archived*/ false, - "2025-02-01T10:00:00Z", - "preview", - SessionSource::Cli, - )?; - codex_rollout::append_thread_name( - config.codex_home.as_path(), - legacy_thread_id, - "saved-session", - ) - .await?; let (resolved_custom_thread_id, _) = run_session_queue_action_with_app_server( &mut app_server, config.codex_home.as_path(), @@ -489,40 +517,27 @@ async fn queues_non_interactive_and_custom_sessions_without_scanning_rollouts() "custom-client-message-id", ) .await?; - assert_eq!( - ( - resolved_custom_thread_id, - runtime - .get_thread(legacy_thread_id) - .await - .map_err(std::io::Error::other)?, - ), - (custom_thread_id, None), - ); + assert_eq!(resolved_custom_thread_id, custom_thread_id); runtime .update_thread_title(custom_thread_id, "saved-session") .await .map_err(std::io::Error::other)?; - let (resolved_duplicate_id, _) = run_session_queue_action_with_app_server( + let duplicate_error = run_session_queue_action_with_app_server( &mut app_server, config.codex_home.as_path(), "saved-session", "do the thing", "duplicate-client-message-id", ) - .await?; + .await + .expect_err("different sources with the same label need a thread ID"); app_server.shutdown().await?; - assert_eq!( - ( - resolved_duplicate_id, - runtime - .get_thread(legacy_thread_id) - .await - .map_err(std::io::Error::other)?, - ), - (custom_thread_id, None), + duplicate_error.to_string(), + format!( + "Multiple sessions match 'saved-session' (including {thread_id} and {custom_thread_id}); use a session UUID to disambiguate." + ) ); Ok(()) } diff --git a/codex-rs/tui/src/session_queue_commands.rs b/codex-rs/tui/src/session_queue_commands.rs index 46ba56e9f0..485c8077a5 100644 --- a/codex-rs/tui/src/session_queue_commands.rs +++ b/codex-rs/tui/src/session_queue_commands.rs @@ -1,11 +1,9 @@ //! Queue user messages through a local or remote app server. -use std::path::Path; - use crate::app_server_session::AppServerSession; +use crate::named_session_lookup::SessionCollection; +use crate::named_session_lookup::lookup; use crate::session_archive_commands::SessionArchiveCommandOptions; -use crate::session_archive_commands::SessionNameMatch; -use crate::session_archive_commands::lookup_session_by_exact_name; use crate::session_archive_commands::start_app_server_for_session_command; use codex_app_server_client::TypedRequestError; use codex_app_server_protocol::ClientRequest; @@ -19,6 +17,7 @@ use color_eyre::Report; use color_eyre::eyre::Result; use color_eyre::eyre::WrapErr; use color_eyre::eyre::eyre; +use std::path::Path; use uuid::Uuid; const JSONRPC_INVALID_REQUEST: i64 = -32600; @@ -87,12 +86,17 @@ pub(super) async fn run_session_queue_action_with_app_server( let thread_id = if let Ok(thread_id) = ThreadId::from_string(target) { thread_id } else { - let thread = lookup_session_by_exact_name( + let thread = lookup( app_server, codex_home, target, - /*archived*/ false, - SessionNameMatch::FirstIncludingNonInteractive, + &[SessionCollection::Active], + &[ + super::resume_source_kinds(/*include_non_interactive*/ true), + // An empty filter includes Atlas/ChatGPT sessions. + Vec::new(), + ], + /*model_provider*/ None, ) .await? .ok_or_else(|| eyre!("No active session found matching '{target}'."))?; diff --git a/codex-rs/tui/src/snapshots/codex_tui__app__tests__duplicate_named_resume_error.snap b/codex-rs/tui/src/snapshots/codex_tui__app__tests__duplicate_named_resume_error.snap new file mode 100644 index 0000000000..00eec8aae2 --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__app__tests__duplicate_named_resume_error.snap @@ -0,0 +1,8 @@ +--- +source: tui/src/app/tests.rs +assertion_line: 293 +expression: rendered +--- +Existing conversation + +■ Multiple sessions match 'duplicate-label' (including and ); use a session UUID to disambiguate. diff --git a/codex-rs/tui/src/startup_orchestration.rs b/codex-rs/tui/src/startup_orchestration.rs index 783a12f5e4..c9fed10be0 100644 --- a/codex-rs/tui/src/startup_orchestration.rs +++ b/codex-rs/tui/src/startup_orchestration.rs @@ -149,6 +149,7 @@ pub(super) async fn run_main_inner( } let reuse_implicit_local_daemon = !cli.shared.worktree + && !cli.oss && !workload_identity_selected && (cli.agents_overview || can_reuse_implicit_local_daemon(