mirror of
https://github.com/openai/codex.git
synced 2026-11-13 06:45:07 +00:00
Route rollout reads through the canonical JSON decoder (#42378)
## Why Directly deserializing the flattened `RolloutLine` envelope can reject nested decimal values, preventing affected paginated sessions from resuming. ## What changed - Add canonical string, byte, and reverse-scanner helpers that decode rollout records through `serde_json::Value` before decoding the flattened item. - Route rollout readers across session discovery, history, migration, search, thread storage, and transcript previews through those helpers. - Remove `Deserialize` from `RolloutLine` so new readers cannot bypass the canonical persistence decoder. ## Testing Add coverage that resumes a paginated rollout after a token-count record with a decimal rate-limit value and verifies that ordinal sequencing continues. GitOrigin-RevId: 49abac1e0751c073daa5a93a840d8a483fd2d013
This commit is contained in:
1 parent
e1d0ef995f
commit
69cebb5d15
49 files changed
+184
-114
No files matched your search
@@ -46,7 +46,9 @@ pub(crate) use codex_protocol::protocol;
|
||||
/// Remove it once Serde supports format-specific buffering.
|
||||
pub fn decode_rollout_line(value: Value) -> serde_json::Result<RolloutLine> {
|
||||
let Value::Object(mut fields) = value else {
|
||||
return serde_json::from_value(value);
|
||||
return Err(serde_json::Error::custom(
|
||||
"rollout line must be a JSON object",
|
||||
));
|
||||
};
|
||||
let timestamp = fields
|
||||
.remove("timestamp")
|
||||
@@ -66,6 +68,16 @@ pub fn decode_rollout_line(value: Value) -> serde_json::Result<RolloutLine> {
|
||||
})
|
||||
}
|
||||
|
||||
/// Parses a persisted JSONL rollout record through the canonical JSON decoder.
|
||||
pub fn parse_rollout_line(line: &str) -> serde_json::Result<RolloutLine> {
|
||||
serde_json::from_str::<Value>(line).and_then(decode_rollout_line)
|
||||
}
|
||||
|
||||
/// Parses persisted JSONL rollout record bytes through the canonical JSON decoder.
|
||||
pub fn parse_rollout_line_bytes(bytes: &[u8]) -> serde_json::Result<RolloutLine> {
|
||||
serde_json::from_slice::<Value>(bytes).and_then(decode_rollout_line)
|
||||
}
|
||||
|
||||
pub const SESSIONS_SUBDIR: &str = "sessions";
|
||||
pub const ARCHIVED_SESSIONS_SUBDIR: &str = "archived_sessions";
|
||||
pub static INTERACTIVE_SESSION_SOURCES: LazyLock<Vec<SessionSource>> = LazyLock::new(|| {
|
||||
|
||||
@@ -20,7 +20,6 @@ use super::SESSIONS_SUBDIR;
|
||||
use super::compression;
|
||||
use super::rollout_file_name::RolloutFileName;
|
||||
use crate::RolloutItem;
|
||||
use crate::RolloutLine;
|
||||
use crate::protocol::EventMsg;
|
||||
use crate::state_db;
|
||||
use codex_file_search as file_search;
|
||||
@@ -1128,7 +1127,7 @@ async fn read_head_summary(path: &Path, head_limit: usize) -> io::Result<HeadTai
|
||||
}
|
||||
lines_scanned += 1;
|
||||
|
||||
let parsed: Result<RolloutLine, _> = serde_json::from_str(trimmed);
|
||||
let parsed = crate::parse_rollout_line(trimmed);
|
||||
let rollout_line = match parsed {
|
||||
Ok(rollout_line) => rollout_line,
|
||||
Err(_) => {
|
||||
@@ -1241,7 +1240,7 @@ pub async fn read_head_for_summary(path: &Path) -> io::Result<Vec<serde_json::Va
|
||||
if trimmed.is_empty() {
|
||||
continue;
|
||||
}
|
||||
if let Ok(rollout_line) = serde_json::from_str::<RolloutLine>(trimmed) {
|
||||
if let Ok(rollout_line) = crate::parse_rollout_line(trimmed) {
|
||||
match rollout_line.item {
|
||||
RolloutItem::SessionMeta(session_meta_line) => {
|
||||
if let Ok(value) = serde_json::to_value(session_meta_line) {
|
||||
@@ -1300,7 +1299,7 @@ pub async fn read_session_meta_line(path: &Path) -> io::Result<SessionMetaLine>
|
||||
if trimmed.is_empty() {
|
||||
continue;
|
||||
}
|
||||
let Ok(rollout_line) = serde_json::from_str::<RolloutLine>(trimmed) else {
|
||||
let Ok(rollout_line) = crate::parse_rollout_line(trimmed) else {
|
||||
if let Ok(value) = serde_json::from_str::<serde_json::Value>(trimmed) {
|
||||
crate::recorder::reject_unknown_thread_history_mode(&value)?;
|
||||
}
|
||||
|
||||
@@ -10,7 +10,6 @@ use codex_protocol::protocol::HistoryPosition;
|
||||
use codex_protocol::protocol::ThreadHistoryMode;
|
||||
|
||||
use crate::RolloutItem;
|
||||
use crate::RolloutLine;
|
||||
use crate::reverse_jsonl_scanner::ReverseJsonlScanner;
|
||||
use crate::reverse_jsonl_scanner::ScanOutcome;
|
||||
|
||||
@@ -67,7 +66,7 @@ pub(crate) fn ordinal_state_for_rollout(
|
||||
|
||||
let mut scanner = ReverseJsonlScanner::new(file)?;
|
||||
let record = loop {
|
||||
match scanner.scan_next::<RolloutLine>()? {
|
||||
match scanner.scan_next_rollout_line()? {
|
||||
Some(ScanOutcome::Parsed(record)) => break record,
|
||||
Some(ScanOutcome::Rejected(_)) => continue,
|
||||
None => {
|
||||
@@ -111,7 +110,7 @@ fn read_history_metadata(
|
||||
if line.trim().is_empty() {
|
||||
continue;
|
||||
}
|
||||
let record: RolloutLine = serde_json::from_str(line.as_str()).map_err(|error| {
|
||||
let record = crate::parse_rollout_line(line.as_str()).map_err(|error| {
|
||||
io::Error::other(format!(
|
||||
"failed to parse first rollout record at {}: {error}",
|
||||
path.display()
|
||||
|
||||
@@ -14,11 +14,14 @@ use codex_protocol::protocol::AgentMessageEvent;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::HistoryPosition;
|
||||
use codex_protocol::protocol::RateLimitSnapshot;
|
||||
use codex_protocol::protocol::RateLimitWindow;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_protocol::protocol::SessionMeta;
|
||||
use codex_protocol::protocol::SessionMetaLine;
|
||||
use codex_protocol::protocol::SessionSource;
|
||||
use codex_protocol::protocol::ThreadHistoryMode;
|
||||
use codex_protocol::protocol::TokenCountEvent;
|
||||
use codex_protocol::protocol::TurnContextItem;
|
||||
use codex_protocol::protocol::UserMessageEvent;
|
||||
use codex_protocol::security_risk::SecurityRiskScore;
|
||||
@@ -103,7 +106,7 @@ fn read_rollout_lines(path: &Path) -> std::io::Result<Vec<RolloutLine>> {
|
||||
fs::read_to_string(path)?
|
||||
.lines()
|
||||
.filter(|line| !line.trim().is_empty())
|
||||
.map(|line| serde_json::from_str(line).map_err(std::io::Error::other))
|
||||
.map(|line| crate::parse_rollout_line(line).map_err(std::io::Error::other))
|
||||
.collect()
|
||||
}
|
||||
|
||||
@@ -687,7 +690,7 @@ async fn recorder_materializes_on_flush_with_pending_items() -> std::io::Result<
|
||||
vec![Some(0), Some(1), Some(2)]
|
||||
);
|
||||
let first_line = text.lines().next().expect("session metadata line");
|
||||
let session_meta: RolloutLine = serde_json::from_str(first_line)?;
|
||||
let session_meta = crate::parse_rollout_line(first_line)?;
|
||||
let RolloutItem::SessionMeta(session_meta) = session_meta.item else {
|
||||
panic!("expected session metadata in rollout");
|
||||
};
|
||||
@@ -995,6 +998,68 @@ async fn resumed_paginated_rollout_continues_after_ordinal_gap() -> std::io::Res
|
||||
recorder.shutdown().await
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn resumed_paginated_rollout_continues_after_decimal_token_count() -> std::io::Result<()> {
|
||||
let home = TempDir::new().expect("temp dir");
|
||||
let config = test_config(home.path());
|
||||
let thread_id = ThreadId::new();
|
||||
let recorder = RolloutRecorder::new(
|
||||
&config,
|
||||
RolloutRecorderParams::new(
|
||||
thread_id,
|
||||
/*forked_from_id*/ None,
|
||||
/*parent_thread_id*/ None,
|
||||
SessionSource::Exec,
|
||||
/*thread_source*/ None,
|
||||
"test_originator".to_string(),
|
||||
BaseInstructions::default(),
|
||||
Vec::new(),
|
||||
)
|
||||
.with_history_mode(ThreadHistoryMode::Paginated),
|
||||
)
|
||||
.await?;
|
||||
let rollout_path = recorder.rollout_path().to_path_buf();
|
||||
recorder
|
||||
.record_canonical_items(&[RolloutItem::EventMsg(EventMsg::TokenCount(
|
||||
TokenCountEvent {
|
||||
info: None,
|
||||
rate_limits: Some(RateLimitSnapshot {
|
||||
limit_id: None,
|
||||
limit_name: None,
|
||||
primary: Some(RateLimitWindow {
|
||||
used_percent: 0.0,
|
||||
window_minutes: Some(60),
|
||||
resets_at: Some(1_800_000_000),
|
||||
}),
|
||||
secondary: None,
|
||||
credits: None,
|
||||
individual_limit: None,
|
||||
spend_control_reached: None,
|
||||
plan_type: None,
|
||||
rate_limit_reached_type: None,
|
||||
normal_model_slug: None,
|
||||
}),
|
||||
},
|
||||
))])
|
||||
.await?;
|
||||
recorder.persist().await?;
|
||||
recorder.shutdown().await?;
|
||||
|
||||
let resumed =
|
||||
RolloutRecorder::new(&config, RolloutRecorderParams::resume(rollout_path.clone())).await?;
|
||||
resumed
|
||||
.record_canonical_items(&[agent_message_item("after-resume")])
|
||||
.await?;
|
||||
resumed.shutdown().await?;
|
||||
|
||||
let ordinals = read_rollout_lines(&rollout_path)?
|
||||
.into_iter()
|
||||
.map(|line| line.ordinal)
|
||||
.collect::<Vec<_>>();
|
||||
assert_eq!(ordinals, vec![Some(0), Some(1), Some(2)]);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn resumed_paginated_rollout_repairs_unsafe_tail() -> std::io::Result<()> {
|
||||
let valid_unterminated = serde_json::to_string(&RolloutLine {
|
||||
@@ -1034,7 +1099,7 @@ async fn resumed_paginated_rollout_repairs_unsafe_tail() -> std::io::Result<()>
|
||||
assert!(contents.ends_with('\n'), "{name} tail should be terminated");
|
||||
let ordinals = contents
|
||||
.lines()
|
||||
.filter_map(|line| serde_json::from_str::<RolloutLine>(line).ok())
|
||||
.filter_map(|line| crate::parse_rollout_line(line).ok())
|
||||
.map(|line| line.ordinal)
|
||||
.collect::<Vec<_>>();
|
||||
assert_eq!(
|
||||
|
||||
@@ -4,6 +4,9 @@ use std::io::Seek;
|
||||
use std::io::SeekFrom;
|
||||
|
||||
use serde::de::DeserializeOwned;
|
||||
use serde_json::Value;
|
||||
|
||||
use crate::RolloutLine;
|
||||
|
||||
const READ_CHUNK_SIZE: usize = 64 * 1024;
|
||||
|
||||
@@ -128,6 +131,17 @@ where
|
||||
}
|
||||
}
|
||||
|
||||
/// Scans the next rollout record through the canonical persisted JSON decoder.
|
||||
pub fn scan_next_rollout_line(&mut self) -> io::Result<Option<ScanOutcome<RolloutLine>>> {
|
||||
Ok(self.scan_next::<Value>()?.map(|outcome| match outcome {
|
||||
ScanOutcome::Parsed(value) => match crate::decode_rollout_line(value) {
|
||||
Ok(line) => ScanOutcome::Parsed(line),
|
||||
Err(error) => ScanOutcome::Rejected(error),
|
||||
},
|
||||
ScanOutcome::Rejected(error) => ScanOutcome::Rejected(error),
|
||||
}))
|
||||
}
|
||||
|
||||
fn finish_record<T>(&mut self) -> Option<ScanOutcome<T>>
|
||||
where
|
||||
T: DeserializeOwned,
|
||||
|
||||
@@ -18,7 +18,6 @@ use super::SESSIONS_SUBDIR;
|
||||
use super::compression;
|
||||
use crate::ResponseItemEnvelope;
|
||||
use crate::RolloutItem;
|
||||
use crate::RolloutLine;
|
||||
|
||||
const MATCH_CONTEXT_BEFORE_CHARS: usize = 48;
|
||||
const MATCH_CONTEXT_AFTER_CHARS: usize = 96;
|
||||
@@ -248,7 +247,7 @@ fn case_insensitive_literal_regex(search_term: impl AsRef<str>) -> io::Result<Re
|
||||
}
|
||||
|
||||
fn content_match_snippet(jsonl_line: &str, search_term: &Regex) -> Option<String> {
|
||||
let rollout_line = serde_json::from_str::<RolloutLine>(jsonl_line.trim()).ok()?;
|
||||
let rollout_line = crate::parse_rollout_line(jsonl_line.trim()).ok()?;
|
||||
let text = conversation_text_from_item(&rollout_line.item)?;
|
||||
excerpt_around_match(text.as_str(), search_term)
|
||||
}
|
||||
|
||||
@@ -59,7 +59,7 @@ fn rollout_line_decoder_preserves_canonical_json_compatibility() -> Result<()> {
|
||||
|
||||
for encoded in cases {
|
||||
let value = serde_json::from_str::<serde_json::Value>(encoded)?;
|
||||
let decoded = crate::decode_rollout_line(value.clone())?;
|
||||
let decoded = crate::parse_rollout_line(encoded)?;
|
||||
let mut expected = value;
|
||||
if expected["type"] != "response_item" {
|
||||
expected
|
||||
|
||||
Reference in new issue
Block a user