From 1b450c79126cc56df29b833110a8c702f82de882 Mon Sep 17 00:00:00 2001 From: joeytrasatti-openai Date: Wed, 19 Aug 2026 17:45:15 +0000 Subject: [PATCH] Persist thread section moves before the first turn (#39523) ## Why New non-ephemeral threads have no persisted rollout or preview until their first turn, so moving them into a section could leave them absent from section-filtered thread lists. ## What changed - Materialize and flush a non-ephemeral thread before applying an explicit section move. - Include explicitly sectioned threads with empty previews in section-filtered queries, and update the partial indexes to cover them. ## Testing - Add an app-server test that moves a newly started thread into and out of the pinned section before its first turn. - Cover empty-preview threads in recency and section-position query tests. GitOrigin-RevId: aa3c4daf9890f06c08b93cd60f146099cbc6cfb2 --- codex-rs/app-server/README.md | 2 +- .../tests/suite/v2/thread_metadata_update.rs | 88 +++++++++++++++++++ codex-rs/core/src/thread_manager.rs | 17 ++-- ..._threads_section_empty_preview_indexes.sql | 10 +++ codex-rs/state/src/runtime/threads.rs | 17 ++-- 5 files changed, 119 insertions(+), 15 deletions(-) create mode 100644 codex-rs/state/migrations/0050_threads_section_empty_preview_indexes.sql diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 20dc3e6be1..483924a572 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -649,7 +649,7 @@ to load the containing turn. ### Example: Update stored thread metadata -Use `thread/metadata/update` to patch sqlite-backed `gitInfo` without resuming a thread. Omitted fields are left unchanged, while explicit `null` clears a stored value. Use `thread/section/move` to enter, reorder, or leave a section; section positions remain server-owned, and `thread/list` returns threads in their manual order when `sortKey` is `section_position`. +Use `thread/metadata/update` to patch sqlite-backed `gitInfo` without resuming a thread. Omitted fields are left unchanged, while explicit `null` clears a stored value. Use `thread/section/move` to enter, reorder, or leave a section; an explicit move persists a newly started non-ephemeral thread even before its first turn. Section positions remain server-owned, and `thread/list` returns threads in their manual order when `sortKey` is `section_position`. A non-null `sectionId` filter includes explicitly placed threads whose preview is still empty. ```json { "method": "thread/metadata/update", "id": 24, "params": { diff --git a/codex-rs/app-server/tests/suite/v2/thread_metadata_update.rs b/codex-rs/app-server/tests/suite/v2/thread_metadata_update.rs index 172715a1e5..d82ed2c4f8 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_metadata_update.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_metadata_update.rs @@ -48,6 +48,94 @@ use tokio::time::timeout; const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); const INVALID_REQUEST_ERROR_CODE: i64 = -32600; +#[tokio::test] +async fn thread_section_move_pins_before_first_turn() -> Result<()> { + let server = create_mock_responses_server_repeating_assistant("Done").await; + let codex_home = TempDir::new()?; + mock_responses_config(&server.uri()).write(codex_home.path())?; + let mut mcp = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .build() + .await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let start_id = mcp + .send_thread_start_request_with_auto_env(ThreadStartParams { + model: Some("mock-model".to_string()), + ..Default::default() + }) + .await?; + let started: ThreadStartResponse = + timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(start_id)).await??; + let thread_id = started.thread.id; + assert_eq!(started.thread.preview, ""); + + let move_id = mcp + .send_thread_section_move_request(ThreadSectionMoveParams { + thread_id: thread_id.clone(), + section_id: Some(PINNED_THREAD_SECTION_ID.to_string()), + before_thread_id: None, + }) + .await?; + let _: ThreadSectionMoveResponse = + timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(move_id)).await??; + + let list_params = ThreadListParams { + cursor: None, + limit: Some(100), + sort_key: None, + sort_direction: None, + model_providers: None, + source_kinds: None, + archived: None, + section_id: Some(Some(PINNED_THREAD_SECTION_ID.to_string())), + project_id: None, + cwd: None, + use_state_db_only: true, + search_term: None, + parent_thread_id: None, + ancestor_thread_id: None, + }; + for sort_key in [ThreadSortKey::SectionPosition, ThreadSortKey::RecencyAt] { + let list_id = mcp + .send_thread_list_request(ThreadListParams { + sort_key: Some(sort_key), + ..list_params.clone() + }) + .await?; + let listed: ThreadListResponse = + timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(list_id)).await??; + assert_eq!( + listed + .data + .iter() + .map(|thread| thread.id.as_str()) + .collect::>(), + vec![thread_id.as_str()] + ); + } + + let move_id = mcp + .send_thread_section_move_request(ThreadSectionMoveParams { + thread_id, + section_id: None, + before_thread_id: None, + }) + .await?; + let _: ThreadSectionMoveResponse = + timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(move_id)).await??; + let list_id = mcp + .send_thread_list_request(ThreadListParams { + section_id: None, + ..list_params + }) + .await?; + let listed: ThreadListResponse = + timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(list_id)).await??; + assert_eq!(listed.data, vec![]); + Ok(()) +} + #[tokio::test] async fn thread_section_move_pins_and_unpins_with_filtered_recency_pagination() -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; diff --git a/codex-rs/core/src/thread_manager.rs b/codex-rs/core/src/thread_manager.rs index b8960c8544..6b1e5db805 100644 --- a/codex-rs/core/src/thread_manager.rs +++ b/codex-rs/core/src/thread_manager.rs @@ -835,19 +835,22 @@ impl ThreadManager { } } - /// Moves a persisted thread to, within, or out of a server-ordered section. + /// Moves a thread to, within, or out of a server-ordered section. pub async fn move_thread_to_section( &self, thread_id: ThreadId, section: Option<&str>, before_thread_id: Option, ) -> CodexResult<()> { - if let Ok(thread) = self.get_thread(thread_id).await - && thread.config_snapshot().await.ephemeral - { - return Err(CodexErr::InvalidRequest(format!( - "ephemeral thread does not support section moves: {thread_id}" - ))); + if let Ok(thread) = self.get_thread(thread_id).await { + if thread.config_snapshot().await.ephemeral { + return Err(CodexErr::InvalidRequest(format!( + "ephemeral thread does not support section moves: {thread_id}" + ))); + } + // Explicit placement must work before the first turn materializes the thread. + thread.ensure_rollout_materialized().await; + thread.flush_rollout().await?; } self.state diff --git a/codex-rs/state/migrations/0050_threads_section_empty_preview_indexes.sql b/codex-rs/state/migrations/0050_threads_section_empty_preview_indexes.sql new file mode 100644 index 0000000000..5ca988f779 --- /dev/null +++ b/codex-rs/state/migrations/0050_threads_section_empty_preview_indexes.sql @@ -0,0 +1,10 @@ +DROP INDEX idx_threads_section_recency_at_ms; +DROP INDEX idx_threads_section_position; + +CREATE INDEX idx_threads_section_recency_at_ms + ON threads(archived, thread_section_id, recency_at_ms DESC, id DESC) + WHERE thread_section_id IS NOT NULL; + +CREATE INDEX idx_threads_section_position + ON threads(archived, thread_section_id, section_position ASC, id ASC) + WHERE thread_section_id IS NOT NULL; diff --git a/codex-rs/state/src/runtime/threads.rs b/codex-rs/state/src/runtime/threads.rs index 035f76ba7c..4bb83cd169 100644 --- a/codex-rs/state/src/runtime/threads.rs +++ b/codex-rs/state/src/runtime/threads.rs @@ -1382,7 +1382,7 @@ fn push_thread_filters_with_preview<'a>( } else { builder.push(" AND threads.archived = 0"); } - if !include_empty_preview { + if !include_empty_preview && !matches!(section, Some(Some(_))) { builder.push(" AND threads.preview <> ''"); } match section { @@ -1674,6 +1674,10 @@ mod tests { ] { let mut metadata = test_thread_metadata(&codex_home, thread_id, codex_home.clone()); metadata.recency_at = DateTime::::from_timestamp(recency_at, 0).unwrap(); + if thread_id == oldest_pinned { + metadata.preview = Some(String::new()); + metadata.first_user_message = None; + } metadata.section = section.map(|id| crate::ThreadSection { id: id.to_string(), name: crate::PINNED_THREAD_SECTION_NAME.to_string(), @@ -1748,12 +1752,7 @@ mod tests { .iter() .map(|thread| thread.id) .collect::>(), - vec![ - newest_unpinned, - newest_pinned, - oldest_pinned, - oldest_unpinned, - ] + vec![newest_unpinned, newest_pinned, oldest_unpinned,] ); let mut builder = QueryBuilder::::new("EXPLAIN QUERY PLAN "); @@ -1806,6 +1805,10 @@ mod tests { for (thread_id, position) in [(first, 1_000_000), (tied, 1_000_000), (last, 2_000_000)] { let mut metadata = test_thread_metadata(&codex_home, thread_id, codex_home.clone()); + if thread_id == tied { + metadata.preview = Some(String::new()); + metadata.first_user_message = None; + } metadata.section = Some(crate::ThreadSection { id: CUSTOM_THREAD_SECTION_ID.to_string(), name: "Custom section".to_string(),