Reject unsupported history modes when loading rollouts (#34344)

## Why

The first session metadata record defines a rollout's history mode. If that
record uses an unsupported mode, skipping it as a parse error can cause later
metadata copied from fork history to be treated as canonical.

## What changed

- Validate session metadata history modes before deserializing rollout lines,
  while continuing to tolerate unknown modes in copied fork history.
- Simplify thread status registration to pass only the thread ID instead of a
  fully materialized `Thread`.

GitOrigin-RevId: 9de4313d4a76b279d32bb49b2016a4f5d07507c1
This commit is contained in:
Charlie Marsh authored and copyberry committed 2026-07-20 13:39:29 +00:00
1 parent 2deed3fb9c
commit 86102db5a1
4 files changed
+45 -132

No files matched your search

+16 -96
View File
@@ -4,7 +4,6 @@ use crate::outgoing_message::OutgoingEnvelope;
use crate::outgoing_message::OutgoingMessage;
use crate::outgoing_message::OutgoingMessageSender;
use codex_app_server_protocol::ServerNotification;
use codex_app_server_protocol::Thread;
use codex_app_server_protocol::ThreadActiveFlag;
use codex_app_server_protocol::ThreadStatus;
use codex_app_server_protocol::ThreadStatusChangedNotification;
@@ -89,16 +88,18 @@ impl ThreadWatchManager {
}
}
pub(crate) async fn upsert_thread(&self, thread: Thread) {
pub(crate) async fn upsert_thread(&self, thread_id: &str) {
let thread_id = thread_id.to_string();
self.mutate_and_publish(move |state| {
state.upsert_thread(thread.id, /*emit_notification*/ true)
state.upsert_thread(thread_id, /*emit_notification*/ true)
})
.await;
}
pub(crate) async fn upsert_thread_silently(&self, thread: Thread) {
pub(crate) async fn upsert_thread_silently(&self, thread_id: &str) {
let thread_id = thread_id.to_string();
self.mutate_and_publish(move |state| {
state.upsert_thread(thread.id, /*emit_notification*/ false)
state.upsert_thread(thread_id, /*emit_notification*/ false)
})
.await;
}
@@ -453,8 +454,6 @@ fn loaded_thread_status(runtime: &RuntimeFacts) -> ThreadStatus {
#[cfg(test)]
mod tests {
use super::*;
use codex_utils_absolute_path::test_support::PathBufExt;
use codex_utils_absolute_path::test_support::test_path_buf;
use pretty_assertions::assert_eq;
use tokio::time::Duration;
use tokio::time::timeout;
@@ -477,12 +476,7 @@ mod tests {
#[tokio::test]
async fn tracks_non_interactive_thread_status() {
let manager = ThreadWatchManager::new();
manager
.upsert_thread(test_thread(
NON_INTERACTIVE_THREAD_ID,
codex_app_server_protocol::SessionSource::AppServer,
))
.await;
manager.upsert_thread(NON_INTERACTIVE_THREAD_ID).await;
manager.note_turn_started(NON_INTERACTIVE_THREAD_ID).await;
@@ -499,12 +493,7 @@ mod tests {
#[tokio::test]
async fn status_updates_track_single_thread() {
let manager = ThreadWatchManager::new();
manager
.upsert_thread(test_thread(
INTERACTIVE_THREAD_ID,
codex_app_server_protocol::SessionSource::Cli,
))
.await;
manager.upsert_thread(INTERACTIVE_THREAD_ID).await;
manager.note_turn_started(INTERACTIVE_THREAD_ID).await;
assert_eq!(
@@ -612,12 +601,7 @@ mod tests {
#[tokio::test]
async fn system_error_sets_idle_flag_until_next_turn() {
let manager = ThreadWatchManager::new();
manager
.upsert_thread(test_thread(
INTERACTIVE_THREAD_ID,
codex_app_server_protocol::SessionSource::Cli,
))
.await;
manager.upsert_thread(INTERACTIVE_THREAD_ID).await;
manager.note_turn_started(INTERACTIVE_THREAD_ID).await;
manager.note_system_error(INTERACTIVE_THREAD_ID).await;
@@ -643,12 +627,7 @@ mod tests {
#[tokio::test]
async fn shutdown_marks_thread_not_loaded() {
let manager = ThreadWatchManager::new();
manager
.upsert_thread(test_thread(
INTERACTIVE_THREAD_ID,
codex_app_server_protocol::SessionSource::Cli,
))
.await;
manager.upsert_thread(INTERACTIVE_THREAD_ID).await;
manager.note_turn_started(INTERACTIVE_THREAD_ID).await;
manager.note_thread_shutdown(INTERACTIVE_THREAD_ID).await;
@@ -664,12 +643,7 @@ mod tests {
#[tokio::test]
async fn loaded_statuses_default_to_not_loaded_for_untracked_threads() {
let manager = ThreadWatchManager::new();
manager
.upsert_thread(test_thread(
INTERACTIVE_THREAD_ID,
codex_app_server_protocol::SessionSource::Cli,
))
.await;
manager.upsert_thread(INTERACTIVE_THREAD_ID).await;
manager.note_turn_started(INTERACTIVE_THREAD_ID).await;
let statuses = manager
@@ -694,12 +668,7 @@ mod tests {
#[tokio::test]
async fn has_running_turns_tracks_runtime_running_flag_only() {
let manager = ThreadWatchManager::new();
manager
.upsert_thread(test_thread(
INTERACTIVE_THREAD_ID,
codex_app_server_protocol::SessionSource::Cli,
))
.await;
manager.upsert_thread(INTERACTIVE_THREAD_ID).await;
assert_eq!(manager.running_turn_count().await, 0);
@@ -725,12 +694,7 @@ mod tests {
codex_analytics::AnalyticsEventsClient::disabled(),
)));
manager
.upsert_thread(test_thread(
INTERACTIVE_THREAD_ID,
codex_app_server_protocol::SessionSource::Cli,
))
.await;
manager.upsert_thread(INTERACTIVE_THREAD_ID).await;
assert_eq!(
recv_status_changed_notification(&mut outgoing_rx).await,
ThreadStatusChangedNotification {
@@ -768,12 +732,7 @@ mod tests {
codex_analytics::AnalyticsEventsClient::disabled(),
)));
manager
.upsert_thread_silently(test_thread(
INTERACTIVE_THREAD_ID,
codex_app_server_protocol::SessionSource::Cli,
))
.await;
manager.upsert_thread_silently(INTERACTIVE_THREAD_ID).await;
assert_eq!(
manager
@@ -803,18 +762,8 @@ mod tests {
#[tokio::test]
async fn status_watchers_receive_only_their_thread_updates() {
let manager = ThreadWatchManager::new();
manager
.upsert_thread(test_thread(
INTERACTIVE_THREAD_ID,
codex_app_server_protocol::SessionSource::Cli,
))
.await;
manager
.upsert_thread(test_thread(
NON_INTERACTIVE_THREAD_ID,
codex_app_server_protocol::SessionSource::AppServer,
))
.await;
manager.upsert_thread(INTERACTIVE_THREAD_ID).await;
manager.upsert_thread(NON_INTERACTIVE_THREAD_ID).await;
let interactive_thread_id = ThreadId::from_string(INTERACTIVE_THREAD_ID)
.expect("interactive thread id should parse");
let non_interactive_thread_id = ThreadId::from_string(NON_INTERACTIVE_THREAD_ID)
@@ -885,33 +834,4 @@ mod tests {
};
notification
}
fn test_thread(thread_id: &str, source: codex_app_server_protocol::SessionSource) -> Thread {
Thread {
id: thread_id.to_string(),
extra: None,
session_id: thread_id.to_string(),
forked_from_id: None,
parent_thread_id: None,
preview: String::new(),
ephemeral: false,
history_mode: Default::default(),
model_provider: "mock-provider".to_string(),
created_at: 0,
updated_at: 0,
recency_at: Some(0),
status: ThreadStatus::NotLoaded,
path: None,
cwd: test_path_buf("/tmp").abs(),
cli_version: "test".to_string(),
agent_nickname: None,
agent_role: None,
source,
can_accept_direct_input: None,
thread_source: None,
git_info: None,
name: None,
turns: Vec::new(),
}
}
}