Separate interactive and non-interactive sessions #4612
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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
…ive-sessions # Conflicts: # codex-rs/cli/src/proto.rs # codex-rs/core/src/rollout/list.rs
| pub fn new( | ||
| conversation_id: ConversationId, | ||
| instructions: Option<String>, | ||
| interactive: bool, |
There was a problem hiding this comment.
wdut of having this as enum?
| codex_home: &Path, | ||
| page_size: usize, | ||
| cursor: Option<&Cursor>, | ||
| interactive_only: bool, |
There was a problem hiding this comment.
what do you think of having a struct for this?
There was a problem hiding this comment.
Just for this parameter of for all of them? I think struct makes sense when we get more filters.
There was a problem hiding this comment.
I meant for all parameters but yeah agreed
…ive-sessions # Conflicts: # codex-rs/app-server/src/message_processor.rs # codex-rs/exec/src/lib.rs # codex-rs/mcp-server/src/message_processor.rs
…ter test Replace strict path equality assertions with filename-based checks to avoid dependency on exact directory structure. This makes the test more robust to changes in session path generation. Also update assertions for session counts accordingly.
Introduce SessionSource to indicate the origin of a session (Cli, VSCode, Exec, Mcp, Unknown) instead of the interactive boolean. Update session metadata and listing/filtering to use SessionSource, including in ConversationManager, RolloutRecorder, apps, and tests. Adjust list_conversations APIs and migration points to accept allowed_sources for filtering, and update all callsites and test factories accordingly. Also, update serialization and default logic for SessionMeta and
…tly in session filtering - Define INTERACTIVE_SESSION_SOURCES (&[SessionSource::Cli, SessionSource::Vscode]) in core/rollout, export for reuse. - Replace local INTERACTIVE_SOURCES constants/usages in core and tui, and update tests/production code to use INTERACTIVE_SESSION_SOURCES. - Fix enum variant name VSCode -> Vscode for consistency. - Update sample session creation in tests to specify
…onSource Update naming of SessionSource::Vscode to SessionSource::VSCode everywhere for consistency with protocol definition, including in tests and session source filters.
| pub enum SessionSource { | ||
| Cli, | ||
| #[default] | ||
| VSCode, |
There was a problem hiding this comment.
how about app-server instead of vscode?
There was a problem hiding this comment.
vscode is the name of the product, I'd like external customers to be able to understand what we're talking about in their session file.
There was a problem hiding this comment.
It's internal code tho. Also, vscode may confuse other what about cursor and windsurf. I think people reading the code will have easier time referring to a crate/dir name
| #[derive(Serialize, Deserialize, Copy, Clone, Debug, PartialEq, Eq, TS, Default)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum SessionSource { | ||
| Cli, |
There was a problem hiding this comment.
how about tui because we have a cli crate
There was a problem hiding this comment.
codex cli is the name of the product
There was a problem hiding this comment.
It's confusing with cli.rs tho. I feel it's not clear which one to me.
| let summary = read_head_and_tail(&path, HEAD_RECORD_LIMIT, TAIL_RECORD_LIMIT) | ||
| .await | ||
| .unwrap_or_default(); | ||
| if !allowed_sources.is_empty() |
|
Tested in CLI and VSCode. |
pakrym-oai
deleted the
pakrym/separate-interactive-and-non-interactive-sessions
branch
Do not show exec session in VSCode/TUI selector.