Skip to content

[codex] route thread reads through thread store#18009

Open
wiltzius-openai wants to merge 1 commit intomainfrom
wiltzius/codex/thread-store-read-thread
Open

[codex] route thread reads through thread store#18009
wiltzius-openai wants to merge 1 commit intomainfrom
wiltzius/codex/thread-store-read-thread

Conversation

@wiltzius-openai
Copy link
Copy Markdown
Contributor

@wiltzius-openai wiltzius-openai commented Apr 15, 2026

  • move thread_read and get_thread_summary to use the ThreadStore interface
  • add some additional unit test coverage for the thread store local implementation of thread read
  • in a couple places where the app server interface explicitly accepts a rollout path continue to operate directly on that path; focus on getting the read-by-thread-id case functional first
  • do not yet attempt to migrate other call sites that read threads in app server methods that are about forking/resuming/creating threads -- these require more extensive cleanup to remove path assumptions first

@wiltzius-openai wiltzius-openai force-pushed the wiltzius/codex/thread-store-read-thread branch 7 times, most recently from efc5847 to a32128b Compare April 15, 2026 23:24
@wiltzius-openai wiltzius-openai force-pushed the wiltzius/codex/thread-store-read-thread branch from a32128b to 21c0708 Compare April 15, 2026 23:41
@wiltzius-openai wiltzius-openai marked this pull request as ready for review April 15, 2026 23:43
Copy link
Copy Markdown
Contributor

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 21c0708f90

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +3889 to 3898
if include_turns {
let message = if config_snapshot.ephemeral && loaded_rollout_path.is_none() {
"ephemeral threads do not support includeTurns".to_string()
} else {
format!(
"thread {thread_uuid} is not materialized yet; includeTurns is unavailable before first user message"
)
};
self.send_invalid_request_error(request_id, message).await;
return;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve includeTurns for loaded threads missing from store

This fallback rejects includeTurns unconditionally whenever thread_store.read_thread misses but a live thread exists. That branch is reachable for valid loaded threads whose rollout path is outside codex_home (local store lookup is id-based under codex_home only), so materialized threads resumed by explicit path now return INVALID_REQUEST instead of turns.

Useful? React with 👍 / 👎.

}

let mut thread = if let Some(summary) = db_summary {
summary_to_thread(summary, &self.config.cwd)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we losing the ability to return thread summary directly from sqlite? read_thread.rs seems to use sqlite to extend but not as a fallback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants