From d1cf2b967c7f675594abc3b576a87b64d653a9dc Mon Sep 17 00:00:00 2001 From: jif-oai Date: Wed, 19 Nov 2025 11:50:42 +0000 Subject: [PATCH] One fix --- codex-rs/cli/src/main.rs | 49 --- codex-rs/core/src/codex.rs | 11 +- codex-rs/core/src/rollout/recorder.rs | 1 - codex-rs/seed.md | 283 ------------------ codex-rs/tui/src/bottom_pane/chat_composer.rs | 7 +- 5 files changed, 14 insertions(+), 337 deletions(-) delete mode 100644 codex-rs/seed.md diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index 1afd573af6..f0c4b2e34c 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -103,9 +103,6 @@ enum Subcommand { /// Fork an existing session into a new conversation. Fork(ForkCommand), - /// Manage saved sessions. - Session(SessionCommand), - /// [EXPERIMENTAL] Browse tasks from Codex Cloud and apply changes locally. #[clap(name = "cloud", alias = "cloud-tasks")] Cloud(CloudTasksCli), @@ -158,18 +155,6 @@ struct ForkCommand { config_overrides: TuiCli, } -#[derive(Debug, Parser)] -struct SessionCommand { - #[command(subcommand)] - action: SessionSubcommand, -} - -#[derive(Debug, clap::Subcommand)] -enum SessionSubcommand { - /// List saved sessions created via `/save `. - List, -} - #[derive(Debug, Parser)] struct SandboxArgs { #[command(subcommand)] @@ -507,11 +492,6 @@ async fn cli_main(codex_linux_sandbox_exe: Option) -> anyhow::Result<() let exit_info = codex_tui::run_main(interactive, codex_linux_sandbox_exe).await?; handle_app_exit(exit_info)?; } - Some(Subcommand::Session(SessionCommand { action })) => match action { - SessionSubcommand::List => { - list_saved_sessions_cli().await?; - } - }, Some(Subcommand::Login(mut login_cli)) => { prepend_config_flags( &mut login_cli.config_overrides, @@ -752,35 +732,6 @@ fn print_completion(cmd: CompletionCommand) { generate(cmd.shell, &mut app, name, &mut std::io::stdout()); } -async fn list_saved_sessions_cli() -> anyhow::Result<()> { - let codex_home = codex_core::config::find_codex_home()?; - let entries = codex_core::list_saved_sessions(&codex_home).await?; - if entries.is_empty() { - println!("No saved sessions. Run `/save ` inside Codex to create one."); - return Ok(()); - } - println!( - "{:<20} {:<36} {:<20} {:<12} CWD", - "NAME", "CONVERSATION ID", "SAVED AT", "MODEL" - ); - for entry in entries { - let cid = entry.conversation_id.to_string(); - println!( - "{:<20} {:<36} {:<20} {:<12} {}", - entry.name, - cid, - entry.saved_at, - entry.model, - entry.cwd.display() - ); - let created = entry.created_at.as_deref().unwrap_or("-"); - println!("{:<20} created: {}", "", created); - println!("{:<20} path: {}", "", entry.rollout_path.display()); - println!(); - } - Ok(()) -} - #[cfg(test)] mod tests { use super::*; diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index c954ae0b6f..03d0e63e62 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -710,7 +710,16 @@ impl Session { // If persisting, persist all rollout items as-is (recorder filters) if persist && !rollout_items.is_empty() { - self.persist_rollout_items(&rollout_items).await; + // Drop legacy SessionMeta lines from the source rollout so the forked + // session only contains its own SessionMeta written by the recorder. + let filtered: Vec<_> = rollout_items + .iter() + .cloned() + .filter(|item| !matches!(item, RolloutItem::SessionMeta(_))) + .collect(); + if !filtered.is_empty() { + self.persist_rollout_items(&filtered).await; + } } // Flush after seeding history and any persisted rollout copy. if let Err(e) = self.flush_rollout().await { diff --git a/codex-rs/core/src/rollout/recorder.rs b/codex-rs/core/src/rollout/recorder.rs index fb00cbc3f0..9743137b08 100644 --- a/codex-rs/core/src/rollout/recorder.rs +++ b/codex-rs/core/src/rollout/recorder.rs @@ -359,7 +359,6 @@ fn create_log_file( let path = dir.join(filename); let file = std::fs::OpenOptions::new() .read(true) - .append(true) .create(true) .open(&path)?; diff --git a/codex-rs/seed.md b/codex-rs/seed.md deleted file mode 100644 index 263b276e87..0000000000 --- a/codex-rs/seed.md +++ /dev/null @@ -1,283 +0,0 @@ -# Saved Session / Fork Design (Closer to `codex resume`) - -## Overview - -This design keeps as close as possible to the existing `codex resume` behavior, but adds: - -- In‑session slash command: - - `/save ` – snapshot the current session so it can be resumed/forked later. -- CLI commands: - - `codex resume ID|NAME` – existing behavior, extended so `` aliases a saved session. - - `codex fork ID|NAME` – new: start a new session as a copy of a saved session or existing id. - - `codex session list` – list all **saved** sessions (by `` and underlying id). - -The underlying persistence remains the existing rollout files under `~/.codex/sessions/...`. “Saved sessions” are thin, named pointers to snapshots of those rollouts. - ---- - -## Behavior Summary - -- `/save ` (in-session) - - Takes the **current state of the conversation** (as if you hit Ctrl‑C and then later did `codex resume `). - - Writes or updates a saved entry with user‑chosen ``. - - If `` already exists, it is **overwritten** to point at the new snapshot (this matches “override the list of sessions” semantics). - -- `codex resume ID|NAME` (CLI) - - Behaves like `codex resume ` today. - - Additionally, if `ID|NAME` matches a saved ``, it is resolved to the underlying saved snapshot and resumed from there. - -- `codex fork ID|NAME` (CLI) - - Like `codex resume`, but starts a **new session** with a **new conversation id**, using the same snapshot as the source. - - `ID|NAME` can be either: - - A saved name created via `/save`, or - - A raw conversation id, in which case the fork is created from that rollout. - -- `codex session list` - - Lists all **saved** sessions: - - ``, underlying conversation id, created_at/saved_at, cwd, model, and rollout path. - - The list is entirely driven by `/save`; unsaved transient sessions do not appear here. - ---- - -## Data Model - -We build entirely on top of the **existing session persistence** (rollout files under `~/.codex/sessions/...`) and add a **name field on the session itself** instead of a separate registry. - -### Session name - -Conceptually, a session has: - -```rust -struct SessionMeta { - id: ConversationId, // existing UUID - // ... - name: Option, // NEW: user-supplied ``, if any -} -``` - -Concretely: - -- We extend the existing `SessionMeta` (the struct written in the first JSONL record of each rollout) with an optional `name: Option`. -- The app server’s `Thread` view can surface this name so the TUI has easy access to it. - -**Storage** - -- No new files are introduced. -- The session name is persisted **with the session rollout itself**: - - Either directly in `SessionMeta.name`, or - - Via a small additional rollout item that updates the name (implementation detail). -- Listing / resolving sessions uses the existing rollout listing mechanisms (`RolloutRecorder::list_conversations`, app‑server `ThreadList`) and simply reads the name field. - -Notes: - -- **Name uniqueness** is a logical constraint, not a separate registry: - - Saving with the same `` again just updates the name on the current session. - - For resolution, we treat `` as an alias for the “most recent” session with that name (older sessions with the same name effectively fall out of the saved list). -- Underlying rollout files remain in the existing `~/.codex/sessions/YYYY/MM/DD/rollout-*.jsonl` structure; we just annotate them with an optional name. - -### Relationship to existing types - -- We reuse: - - `RolloutRecorder` for persistence. - - `RolloutRecorder::list_conversations` / `find_conversation_path_by_id_str` for discovery. - - `InitialHistory::Resumed` + `ConversationManager::resume_conversation_from_rollout` and `resume_conversation_with_history` for resuming. - - The existing “resume by id” flow; `` just resolves to the underlying conversation id/path using the stored name. - ---- - -## `/save ` - -### Semantics - -- User issues `/save ` from within a Codex session (TUI). -- Codex: - - Flushes the current rollout so all events up to this point are durably written (as if the process were about to exit). - - Records a **saved snapshot** pointing to this rollout file and conversation id. - - If a saved entry with the same `` already exists, it is **replaced**: - - The session list now reflects only the latest snapshot for that name. - -This matches the mental model of “if I hit Ctrl‑C right now, I can later `codex resume ` from this point”, just with a human‑friendly name instead of a raw UUID. - -### Implementation sketch - -TUI: - -- Recognize `/save ` as a local command (do not send it to the model). -- Call a new app‑server RPC, e.g. `session/save`: - -```rust -struct SessionSaveParams { - thread_id: String, // current thread (maps to ConversationId) - name: String, // user-specified `` -} -``` - -App server: - -- Resolve `(ConversationId, CodexConversation)` from `thread_id`. -- Ask the underlying session to flush the rollout: - - e.g. `session.flush_rollout().await` (or equivalent). -- Update the session metadata to set `SessionMeta.name = Some()`, and ensure that change is persisted to the rollout. - - If another session already has the same name, the system will treat the **most recent** session with that name as canonical when resolving ``. - -No new persistence format is required beyond adding the `name` field to existing session metadata. - ---- - -## `codex resume ID|NAME` - -### Semantics - -- `codex resume ` (CLI) keeps existing behavior, but with an extra resolution step: - - If `` matches a saved `` in `saved_sessions.json`, resume from that saved session. - - Otherwise, treat `` as a raw conversation id and use the current resume behavior. - -This means: - -- Existing workflows (`codex resume `, `codex resume --last`) continue to work. -- A user can type `codex resume codex-core` after doing `/save codex-core` in a session. - -### Implementation sketch - -CLI: - -- Extend the existing `codex resume` subcommand resolution: - - 1. Try to resolve `` as a saved name: - - Use rollout listing APIs (`RolloutRecorder::list_conversations` or app‑server `ThreadList`) to find sessions whose `SessionMeta.name == `, pick the most recent one, and obtain `(conversation_id, rollout_path)`. - 2. If found, resume from that rollout path (see below). - 3. If not found, fall back to the existing id‑based lookup using `find_conversation_path_by_id_str`. - -App server: - -- The CLI already uses app‑server APIs to resume conversations; the new behavior only needs to: - - Inject the correct `path` or `conversation_id` into the existing `ResumeConversationParams`, based on the resolution above. - - Existing logic in `handle_resume_conversation` (which uses `RolloutRecorder::get_rollout_history` and `ConversationManager::resume_conversation_with_history`) remains unchanged. - ---- - -## `codex fork ID|NAME` - -### Semantics - -- `codex fork ` (CLI) creates a **new** conversation whose initial state is copied from an existing one: - - If `` is a saved name, resolve it to the saved rollout path and conversation id. - - If `` is a raw id, locate its rollout using `find_conversation_path_by_id_str`. - - In both cases, a new conversation is spawned with: - - A **new conversation id**. - - Initial history loaded from the source rollout. - -This is the “explore once, fork many times” workflow driven from the CLI: - -- Explore `codex-core` in TUI. -- `/save codex-core`. -- `codex fork codex-core` for “feature A”. -- `codex fork codex-core` again for “feature B`. - -### Implementation sketch - -CLI: - -- Add a new subcommand: - -```text -codex fork -``` - -- Resolution: - 1. Try `` as a saved name by scanning sessions whose `SessionMeta.name == ` and picking the most recent one. - 2. If not found, treat it as a raw id and call `find_conversation_path_by_id_str`. - 3. If still not found, print a helpful error. - -- Once a `rollout_path` is known, call an app‑server method (or use the existing resume machinery) that: - - Loads `InitialHistory` via `RolloutRecorder::get_rollout_history(&rollout_path)`. - - Spawns a new conversation with `ConversationManager::resume_conversation_with_history`. - -App server: - -- We can either: - - Add a dedicated `session/fork` RPC that accepts `path` or `conversation_id`, or - - Reuse the existing `ResumeConversationParams` and add a small wrapper on the server that: - - Loads history from the given rollout. - - Spawns a new conversation with that history (instead of attaching to an existing id). - -In both cases, the “fork” operation is implemented entirely in terms of existing rollout + `InitialHistory` plumbing; the only new behavior is that it always uses a fresh conversation id. - ---- - -## `codex session list` - -### Semantics - -`codex session list` prints the list of **saved** sessions, one per line: - -- `` (user‑chosen) -- Underlying conversation id -- `cwd` -- `model` -- `created_at` and/or `updated_at` - -This gives users a quick way to discover what names they can use with `/resume`, `/fork`, or `codex resume`. - -### Implementation sketch - -CLI: - -- Add a new subcommand, conceptually: - -```text -codex session list -``` - -- The CLI loads configuration, locates `codex_home`, then: - - Uses `RolloutRecorder::list_conversations` (or app‑server `ThreadList`) to iterate over sessions. - - Filters down to those with a non‑empty `SessionMeta.name`. - - Prints entries in a compact table format, grouping by name if multiple sessions share the same one (showing only the most recent for each name). - -No separate registry file is needed; we derive the list directly from persisted session metadata. - ---- - -## UX Notes & Edge Cases - -- **Overwriting names** - - `/save ` **always overwrites** any existing saved entry with that name. - - This matches the “override the list of sessions” intent: the list reflects the latest saved snapshots for each name. - -- **Continuing after `/save`** - - After `/save`, the current conversation continues as normal. - - The saved entry points at the rollout **as it existed at the time of save**: - - On resume/fork we read the rollout file; because we flushed on save, the state is consistent with how the session looked at that moment. - - Additional events recorded after `/save` will also end up in the same rollout file; conceptually this makes the “saved snapshot” move forward in time as you keep using the same session. If we want a truly frozen snapshot, we can later add an option to write to a dedicated copy of the rollout. - -- **Name vs id collisions** - - If a user picks a `` that happens to look like a UUID, name resolution still prefers explicit saved entries: - - First look up saved name. - - If not found, treat as id. - -- **Listing behavior** - - `codex session list` only shows saved sessions, not all historical rollouts. - - This keeps the list small and intentional; users explicitly control it via `/save`. - -- **Deletion** - - Initial version can omit deletion; names can be overwritten by re‑saving. - - A future `codex session delete ` and/or `/delete-session ` command could remove entries from `saved_sessions.json`. - ---- - -## Implementation Phasing - -1. **Saved registry + resolution** - - Implement `SavedSessionEntry` and `saved_sessions.json` read/write helpers. - - Implement name → entry resolution and fallback to id → rollout path using existing `find_conversation_path_by_id_str`. - -2. **App server RPCs + TUI slash command** - - Add `session/save` RPC. - - Wire `/save ` in TUI to `session/save`. - -3. **CLI integration** - - Extend `codex resume` to accept `` as well as ``. - - Add `codex fork ` that uses the same resolution and history loading, but always spawns a new conversation. - - Add `codex session list` to show saved sessions. - -This keeps the design very close to today’s `codex resume` behavior while adding the ergonomics you described: explicit `/save `, `/fork `, `/resume `, and a simple `codex session list` view. diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index a70bf20328..531d5a9227 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -335,9 +335,10 @@ impl ChatComposer { fn command_args_from_line(line: &str, command: SlashCommand) -> String { if let Some((name, rest)) = parse_slash_name(line) - && name == command.command() { - return rest.to_string(); - } + && name == command.command() + { + return rest.to_string(); + } String::new() }