From db8a902834f8e0a476cc586d004b4b09027d1f91 Mon Sep 17 00:00:00 2001 From: Sol Kennedy Date: Thu, 17 Sep 2026 14:10:27 -0700 Subject: [PATCH 1/2] fix(codex): discover archived sessions CodexStore::discover() only walked sessions_dir. Codex's own /archive (TUI) and codex archive / codex unarchive (CLI) move a rollout out of that dated tree into a flat sibling directory, archived_sessions, which discover() never looked at. An archived Codex session was therefore invisible to txcript list --from codex / view / MCP list_sessions until it was unarchived. Give CodexStore an optional archived_sessions_dir, set by default_root() to the sibling archived_sessions directory, and walk it alongside sessions_dir in discover(). new() keeps its existing single-argument shape since it's also used to construct a store for writing a new session, which should always land in sessions_dir. Verified on a real machine: `txcript list --from codex` went from 671 to 1202 sessions (+531), matching the file count in ~/.codex/archived_sessions exactly. Fixes #54. Co-Authored-By: Claude Sonnet 5 --- docs/formats/codex.md | 8 +++ src/harness/codex.rs | 114 ++++++++++++++++++++++--------------- tests/integration/codex.rs | 59 +++++++++++++++++++ 3 files changed, 136 insertions(+), 45 deletions(-) diff --git a/docs/formats/codex.md b/docs/formats/codex.md index 5516f18..7278000 100644 --- a/docs/formats/codex.md +++ b/docs/formats/codex.md @@ -32,6 +32,14 @@ are not followed, guarding against cycles; symlinked files still list). A file o session if it contains a `session_meta` line carrying an `id`; discovery parses just those lines and skips message payloads entirely. On load, a missing id falls back to the filename's uuid. +Codex's own `/archive` (TUI) and `codex archive`/`codex unarchive` (CLI) move a rollout out of +this dated tree into a flat sibling directory, `archived_sessions` (no `YYYY/MM/DD` sharding). +`CodexStore::default_root` sets `archived_sessions_dir` to that sibling, and discovery walks it +alongside `sessions_dir`, so an archived rollout is still listed — Codex's own session picker just +won't show it until it's unarchived back into `sessions/`. A `CodexStore` built directly from a +custom `sessions_dir` has no archived directory unless one is set with +`with_archived_sessions_dir`. + ## Dissection of a transcript Every line shares one envelope — upstream's `RolloutLine`: a `timestamp` (RFC 3339, millisecond diff --git a/src/harness/codex.rs b/src/harness/codex.rs index d2c5cc3..0cecba7 100644 --- a/src/harness/codex.rs +++ b/src/harness/codex.rs @@ -688,23 +688,43 @@ fn meta_line_str(ts: &str, kind: &str, payload: Value) -> Line { #[derive(Debug, Clone)] pub struct CodexStore { pub sessions_dir: PathBuf, + /// Codex's own `/archive` (TUI) and `codex archive`/`codex unarchive` + /// (CLI) move a rollout out of the dated `sessions_dir` tree into this + /// flat sibling directory, `archived_sessions`. `None` when unknown, as + /// for a `sessions_dir` built by hand that isn't under a Codex home. + pub archived_sessions_dir: Option, } impl CodexStore { pub fn new(sessions_dir: impl Into) -> Self { Self { sessions_dir: sessions_dir.into(), + archived_sessions_dir: None, } } + /// Also discover rollouts Codex has archived into `dir`. New sessions + /// are always written to `sessions_dir`; this only widens discovery. + #[must_use] + pub fn with_archived_sessions_dir(mut self, dir: impl Into) -> Self { + self.archived_sessions_dir = Some(dir.into()); + self + } + /// The default sessions root: `$CODEX_HOME/sessions` when set (Codex /// honors that override before its home lookup), else `~/.codex/sessions`. + /// `archived_sessions_dir` is set to the matching sibling + /// `archived_sessions` directory. #[must_use] pub fn default_root() -> Option { std::env::var_os("CODEX_HOME") .filter(|v| !v.is_empty()) - .map(|codex_home| Self::new(PathBuf::from(codex_home).join("sessions"))) - .or_else(|| home().map(|h| Self::new(h.join(".codex").join("sessions")))) + .map(PathBuf::from) + .or_else(|| home().map(|h| h.join(".codex"))) + .map(|codex_home| { + Self::new(codex_home.join("sessions")) + .with_archived_sessions_dir(codex_home.join("archived_sessions")) + }) } } @@ -713,52 +733,56 @@ impl Store for CodexStore { type Ref = PathBuf; fn discover(&self) -> Result>> { + let mut files = Vec::new(); if self.sessions_dir.is_dir() { - let mut files = Vec::new(); collect_rollouts(&self.sessions_dir, &mut files); - Ok(super::filter_map_parallel(&files, |path| { - // A rollout that fails to read, or lacks a session_meta with - // an id, is not a resumable session. Only session_meta lines - // are parsed — message payloads are skipped whole — and the - // read stops at the first session_meta carrying the id, which - // is line one of a well-formed rollout. Reading the rest would - // mean pulling every byte of every rollout on the machine - // through a JSON probe to learn nothing more. - let has_id = |l: &Line| l.payload.get("id").and_then(Value::as_str).is_some(); - let file = fs::File::open(path).ok()?; - let mut first: Option = None; - let mut found_id = false; - for line in BufReader::new(file).lines().map_while(std::io::Result::ok) { - if line.trim().is_empty() || !is_session_meta(&line) { - continue; - } - let Ok(parsed) = serde_json::from_str::(&line) else { - continue; - }; - found_id = has_id(&parsed); - if first.is_none() { - first = Some(parsed); - } - if found_id { - break; - } - } - let first = first?; - found_id.then(|| { - let mut meta = meta_from_lines(std::slice::from_ref(&first)); - if meta.id.is_empty() { - meta.id = jsonl::file_id(path); - } - Discovered { - meta, - reference: path.clone(), - } - }) - })) - } else { - // A missing sessions root means no sessions, not an error. - Ok(Vec::new()) } + // A missing sessions root or archived directory means no sessions + // there, not an error; `files` is simply left short. + if let Some(archived) = self.archived_sessions_dir.as_deref() + && archived.is_dir() + { + collect_rollouts(archived, &mut files); + } + Ok(super::filter_map_parallel(&files, |path| { + // A rollout that fails to read, or lacks a session_meta with + // an id, is not a resumable session. Only session_meta lines + // are parsed — message payloads are skipped whole — and the + // read stops at the first session_meta carrying the id, which + // is line one of a well-formed rollout. Reading the rest would + // mean pulling every byte of every rollout on the machine + // through a JSON probe to learn nothing more. + let has_id = |l: &Line| l.payload.get("id").and_then(Value::as_str).is_some(); + let file = fs::File::open(path).ok()?; + let mut first: Option = None; + let mut found_id = false; + for line in BufReader::new(file).lines().map_while(std::io::Result::ok) { + if line.trim().is_empty() || !is_session_meta(&line) { + continue; + } + let Ok(parsed) = serde_json::from_str::(&line) else { + continue; + }; + found_id = has_id(&parsed); + if first.is_none() { + first = Some(parsed); + } + if found_id { + break; + } + } + let first = first?; + found_id.then(|| { + let mut meta = meta_from_lines(std::slice::from_ref(&first)); + if meta.id.is_empty() { + meta.id = jsonl::file_id(path); + } + Discovered { + meta, + reference: path.clone(), + } + }) + })) } fn load(&self, reference: &PathBuf) -> Result> { diff --git a/tests/integration/codex.rs b/tests/integration/codex.rs index 9875507..980bbf5 100644 --- a/tests/integration/codex.rs +++ b/tests/integration/codex.rs @@ -182,6 +182,65 @@ fn discover_extracts_metadata() { assert_eq!(meta.cli_version.as_deref(), Some("0.104.0")); } +/// Codex's own `/archive` (TUI) / `codex archive` (CLI) moves a rollout out +/// of the dated `sessions_dir` tree into a flat sibling `archived_sessions` +/// directory. Discovery must still find it there. +#[test] +fn discover_includes_archived_sessions() { + let dir = tempfile::tempdir().unwrap(); + let sessions = dir.path().join("sessions"); + let nested = sessions.join("2026").join("01").join("02"); + std::fs::create_dir_all(&nested).unwrap(); + std::fs::write( + nested.join("rollout-2026-01-02T03-04-05-sess-active.jsonl"), + format!( + "{}\n", + r#"{"timestamp":"2026-01-02T03:04:05.000Z","type":"session_meta","payload":{"id":"sess-active"}}"#, + ), + ) + .unwrap(); + + let archived = dir.path().join("archived_sessions"); + std::fs::create_dir_all(&archived).unwrap(); + std::fs::write( + archived.join("rollout-2025-12-01T00-00-00-sess-archived.jsonl"), + format!( + "{}\n", + r#"{"timestamp":"2025-12-01T00:00:00.000Z","type":"session_meta","payload":{"id":"sess-archived"}}"#, + ), + ) + .unwrap(); + + let store = codex::CodexStore::new(&sessions).with_archived_sessions_dir(&archived); + let mut ids: Vec<_> = store + .discover() + .unwrap() + .into_iter() + .map(|d| d.meta.id) + .collect(); + ids.sort(); + assert_eq!(ids, vec!["sess-active", "sess-archived"]); +} + +/// A store with no known archived directory (the common case for a +/// hand-built `sessions_dir`) only discovers the active tree. +#[test] +fn discover_without_archived_dir_only_finds_active_sessions() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write( + dir.path().join("rollout-2026-01-02T03-04-05-sess-1.jsonl"), + format!( + "{}\n", + r#"{"timestamp":"2026-01-02T03:04:05.000Z","type":"session_meta","payload":{"id":"sess-1"}}"#, + ), + ) + .unwrap(); + + let found = codex::CodexStore::new(dir.path()).discover().unwrap(); + assert_eq!(found.len(), 1); + assert_eq!(found[0].meta.id, "sess-1"); +} + /// Shaped at codex's granularity: each assistant block is its own message /// (codex stores one `response_item` per line), every assistant turn carries a /// model, only the final text turn carries usage, and `stop_reason` is None From daa40f926d1428151791e28a5dc1a67c58f78331 Mon Sep 17 00:00:00 2001 From: Nishant Joshi Date: Fri, 25 Sep 2026 14:31:15 -0700 Subject: [PATCH 2/2] fix(codex): allow safe archived session deletion --- docs/formats/codex.md | 9 +++++ src/harness/codex.rs | 22 +++++++---- tests/integration/path_safety.rs | 61 +++++++++++++++++++++++++++++++ tests/integration/store_delete.rs | 43 ++++++++++++++++++++++ 4 files changed, 128 insertions(+), 7 deletions(-) diff --git a/docs/formats/codex.md b/docs/formats/codex.md index d25f5cf..8ac8872 100644 --- a/docs/formats/codex.md +++ b/docs/formats/codex.md @@ -40,6 +40,15 @@ won't show it until it's unarchived back into `sessions/`. A `CodexStore` built custom `sessions_dir` has no archived directory unless one is set with `with_archived_sessions_dir`. +Deletion accepts rollout files in either configured directory, including when the active +directory is missing. It resolves symlinks before checking containment and refuses paths +outside both directories. Saving always writes into `sessions_dir`. + +For Rust callers, the new public field changes struct-literal construction. Replace +`CodexStore { sessions_dir }` with `CodexStore::new(sessions_dir)`, or include +`archived_sessions_dir: None` in the literal. Use `with_archived_sessions_dir` to enable +archives for a custom store. + ## Dissection of a transcript Every line shares one envelope — upstream's `RolloutLine`: a `timestamp` (RFC 3339, millisecond diff --git a/src/harness/codex.rs b/src/harness/codex.rs index 26b72e0..31ddbe4 100644 --- a/src/harness/codex.rs +++ b/src/harness/codex.rs @@ -881,8 +881,8 @@ impl CodexStore { } } - /// Also discover rollouts Codex has archived into `dir`. New sessions - /// are always written to `sessions_dir`; this only widens discovery. + /// Also discover and delete rollouts Codex has archived into `dir`. + /// New sessions are always written to `sessions_dir`. #[must_use] pub fn with_archived_sessions_dir(mut self, dir: impl Into) -> Self { self.archived_sessions_dir = Some(dir.into()); @@ -991,8 +991,8 @@ impl Store for CodexStore { } /// Removes a Codex rollout log. Guarded on shape and containment: - /// the reference must be a `.jsonl` file resolving within `sessions_dir`, - /// so a foreign or stale reference never removes files outside the sessions root. + /// the reference must be a `.jsonl` file resolving within `sessions_dir` + /// or the configured `archived_sessions_dir`. Neither root itself is deletable. fn delete(&self, reference: &PathBuf) -> Result<()> { if reference.extension().is_none_or(|ext| ext != "jsonl") { return Err(Error::Malformed { @@ -1001,12 +1001,20 @@ impl Store for CodexStore { }); } let canon = reference.canonicalize()?; - let sessions = self.sessions_dir.canonicalize()?; - if canon.strip_prefix(&sessions).is_err() || canon == sessions { + // Either directory can be absent, including the active tree when + // every session has been archived. Only an existing, resolved root + // can authorize deletion; symlink escapes fail this same check. + let contained = std::iter::once(&self.sessions_dir) + .chain(self.archived_sessions_dir.iter()) + .any(|root| { + root.canonicalize() + .is_ok_and(|root| canon.starts_with(&root) && canon != root) + }); + if !contained { return Err(Error::Malformed { harness: Codex::NAME, detail: format!( - "refusing to delete outside the sessions root: {}", + "refusing to delete outside the configured sessions roots: {}", reference.display() ), }); diff --git a/tests/integration/path_safety.rs b/tests/integration/path_safety.rs index 362e80c..71b8d11 100644 --- a/tests/integration/path_safety.rs +++ b/tests/integration/path_safety.rs @@ -344,3 +344,64 @@ fn discovery_survives_a_symlink_loop() { .is_empty() ); } + +#[test] +fn codex_delete_refuses_paths_outside_both_configured_roots() { + let dir = tempfile::tempdir().unwrap(); + let sessions = dir.path().join("sessions"); + let archived = dir.path().join("archived_sessions"); + std::fs::create_dir_all(&sessions).unwrap(); + std::fs::create_dir_all(&archived).unwrap(); + let store = codex::CodexStore::new(&sessions).with_archived_sessions_dir(&archived); + + for name in ["sessions-other", "archived_sessions-other"] { + let foreign_dir = dir.path().join(name); + std::fs::create_dir_all(&foreign_dir).unwrap(); + let foreign = foreign_dir.join("rollout.jsonl"); + std::fs::write(&foreign, b"{}").unwrap(); + for reference in [ + foreign.clone(), + sessions.join("..").join(name).join("rollout.jsonl"), + archived.join("..").join(name).join("rollout.jsonl"), + ] { + assert!(store.delete(&reference).is_err()); + assert!(foreign.is_file(), "the outside file must survive"); + } + } +} + +#[test] +fn codex_delete_requires_archive_directory_to_be_configured() { + let dir = tempfile::tempdir().unwrap(); + let sessions = dir.path().join("sessions"); + let archived = dir.path().join("archived_sessions"); + std::fs::create_dir_all(&sessions).unwrap(); + std::fs::create_dir_all(&archived).unwrap(); + let reference = archived.join("rollout.jsonl"); + std::fs::write(&reference, b"{}").unwrap(); + let store = codex::CodexStore::new(&sessions); + assert!(store.delete(&reference).is_err()); + assert!(reference.is_file()); +} + +#[cfg(unix)] +#[test] +fn codex_delete_refuses_symlink_escapes_from_both_roots() { + let dir = tempfile::tempdir().unwrap(); + let sessions = dir.path().join("sessions"); + let archived = dir.path().join("archived_sessions"); + let foreign = dir.path().join("rollout.jsonl"); + std::fs::write(&foreign, b"{}").unwrap(); + let store = codex::CodexStore::new(&sessions).with_archived_sessions_dir(&archived); + for root in [&sessions, &archived] { + std::fs::create_dir_all(root).unwrap(); + let link = root.join("rollout-link.jsonl"); + std::os::unix::fs::symlink(&foreign, &link).unwrap(); + assert!(store.delete(&link).is_err()); + assert!(foreign.is_file(), "the symlink target must survive"); + assert!( + link.is_symlink(), + "a rejected reference must remain untouched" + ); + } +} diff --git a/tests/integration/store_delete.rs b/tests/integration/store_delete.rs index 7aa1d70..4412c79 100644 --- a/tests/integration/store_delete.rs +++ b/tests/integration/store_delete.rs @@ -273,3 +273,46 @@ mod opencode_archive { assert!(store.delete(&"nope".to_string()).is_err()); } } + +#[test] +fn codex_delete_archived_session_with_or_without_active_directory() { + for keep_active_dir in [true, false] { + let dir = tempfile::tempdir().unwrap(); + let sessions = dir.path().join("sessions"); + let archived = dir.path().join("archived_sessions"); + let store = codex::CodexStore::new(&sessions).with_archived_sessions_dir(&archived); + let native = codex::Codex::from_common(&small_common("archived-session")).unwrap(); + let saved = store.save(&native).unwrap(); + assert!(saved.reference.starts_with(&sessions)); + + std::fs::create_dir_all(&archived).unwrap(); + let reference = archived.join(saved.reference.file_name().unwrap()); + std::fs::rename(&saved.reference, &reference).unwrap(); + if !keep_active_dir { + std::fs::remove_dir_all(&sessions).unwrap(); + } + + let found = store.discover().unwrap(); + assert_eq!(found.len(), 1); + assert_eq!(found[0].reference, reference); + assert_eq!(store.load(&reference).unwrap().meta.id, saved.id); + store.delete(&reference).unwrap(); + assert!(!reference.exists()); + assert!(store.discover().unwrap().is_empty()); + assert!( + archived.is_dir(), + "deleting a rollout must keep its directory" + ); + assert!(store.delete(&reference).is_err()); + } +} + +#[test] +fn codex_active_delete_works_when_archive_directory_is_missing() { + let dir = tempfile::tempdir().unwrap(); + let sessions = dir.path().join("sessions"); + let archived = dir.path().join("archived_sessions"); + let store = codex::CodexStore::new(&sessions).with_archived_sessions_dir(&archived); + roundtrip(&store); + assert!(!archived.exists()); +}