diff --git a/.clippy.toml b/.clippy.toml index 7ca82d173..70b387ca5 100644 --- a/.clippy.toml +++ b/.clippy.toml @@ -17,6 +17,9 @@ doc-valid-idents = [ allow-unwrap-in-tests = true allow-print-in-tests = true allowed-duplicate-crates = [ + # `ring` uses 0.2; `rand_core` and event IDs use 0.3. Remove this once the + # transitive dependencies converge on one version (T-0jrnyy1). + "getrandom", # `thiserror-impl` 2.0.20 parses with `syn` 3 while the rest of the # proc-macro ecosystem is still on 2, so the graph carries both. Nothing in # the workspace can collapse it: we pin `syn` 2, and moving to 3 would only diff --git a/.config/supply-chain/audits.toml b/.config/supply-chain/audits.toml index fea17ca11..1cf4ca9fa 100644 --- a/.config/supply-chain/audits.toml +++ b/.config/supply-chain/audits.toml @@ -256,6 +256,11 @@ who = "Jean Mertz " criteria = "safe-to-deploy" delta = "0.37.0 -> 0.38.0" +[[audits.rustls]] +who = "Jean Mertz " +criteria = "safe-to-deploy" +delta = "0.23.35 -> 0.23.37" + [[audits.rustls-webpki]] who = "Jean Mertz " criteria = "safe-to-deploy" diff --git a/.config/supply-chain/imports.lock b/.config/supply-chain/imports.lock index 73037a746..301b37e8e 100644 --- a/.config/supply-chain/imports.lock +++ b/.config/supply-chain/imports.lock @@ -1599,12 +1599,28 @@ who = "Alex Crichton " criteria = "safe-to-deploy" delta = "0.1.21 -> 0.1.24" +[[audits.bytecode-alliance.audits.rustls]] +who = "Alex Crichton " +criteria = "safe-to-deploy" +delta = "0.23.37 -> 0.23.45" +notes = """ +A relatively large update, but no new `unsafe` and nothing awry here. Lots of +protocol/etc updates which I'm not personally an expert within but the rustls +maintainers are relatively well trusted as well. +""" + [[audits.bytecode-alliance.audits.rustls-webpki]] who = "Alex Crichton " criteria = "safe-to-deploy" delta = "0.103.12 -> 0.103.13" notes = "Minor fixes for the bug being fixed in this release, nothing awry." +[[audits.bytecode-alliance.audits.rustls-webpki]] +who = "Alex Crichton " +criteria = "safe-to-deploy" +delta = "0.103.13 -> 0.103.15" +notes = "Minor updates and feature shufflings." + [[audits.bytecode-alliance.audits.sha1]] who = "Andrew Brown " criteria = "safe-to-deploy" diff --git a/Cargo.lock b/Cargo.lock index cb88f3395..a26730cd2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2407,6 +2407,7 @@ version = "0.1.0" dependencies = [ "base64", "chrono", + "getrandom 0.3.4", "indexmap", "insta", "jp_attachment", @@ -3908,9 +3909,9 @@ dependencies = [ [[package]] name = "rustls" -version = "0.23.35" +version = "0.23.45" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "533f54bc6a7d4f647e46ad909549eda97bf5afc1585190ef692b4286b198bd8f" +checksum = "0d41d731c7d2f962d1ccc364cec258de3c0e93b38c2fb3ba97ac74513048d634" dependencies = [ "log", "once_cell", @@ -3945,9 +3946,9 @@ dependencies = [ [[package]] name = "rustls-webpki" -version = "0.103.13" +version = "0.103.15" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "61c429a8649f110dddef65e2a5ad240f747e85f7758a6bccc7e5777bd33f756e" +checksum = "f3c3cf1d8b1e7d4927e2d154c3fcb02979afb9939629c62cd9048d4f07b60ac2" dependencies = [ "ring", "rustls-pki-types", diff --git a/Cargo.toml b/Cargo.toml index 49fe5c828..a7ce8abba 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -78,6 +78,7 @@ eventsource-stream = { version = "0.2", default-features = false } fancy-regex = { version = "0.19", default-features = false } futures = { version = "0.3", default-features = false } gemini_client_rs = { git = "https://github.com/JeanMertz/gemini-client", default-features = false } # +getrandom = { version = "0.3", default-features = false } gimli = { version = "0.33" } glob = { version = "0.3", default-features = false } grep-matcher = { version = "0.1", default-features = false } diff --git a/crates/jp_cli/src/cmd/conversation/edit.rs b/crates/jp_cli/src/cmd/conversation/edit.rs index c493bebcb..2957c7395 100644 --- a/crates/jp_cli/src/cmd/conversation/edit.rs +++ b/crates/jp_cli/src/cmd/conversation/edit.rs @@ -73,6 +73,10 @@ pub(crate) struct Edit { no_title: bool, /// Open `events.json` in `$EDITOR`. + /// + /// Each entry carries an `event_id` identifying it. + /// Keep it when you edit an entry's content, and drop it when you add one: + /// a missing, empty, or duplicated ID is replaced on the next load. #[arg(long, short = 'e', group = "file", conflicts_with = "property")] events: bool, diff --git a/crates/jp_cli/src/cmd/conversation/fork_tests.rs b/crates/jp_cli/src/cmd/conversation/fork_tests.rs index 77c143bcc..df67af897 100644 --- a/crates/jp_cli/src/cmd/conversation/fork_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/fork_tests.rs @@ -15,6 +15,7 @@ use jp_conversation::{ use jp_printer::{OutputFormat, Printer}; use jp_storage::backend::{FsStorageBackend, Projection}; use jp_workspace::Workspace; +use serde_json::Value; use tokio::runtime::Runtime; use super::*; @@ -23,6 +24,42 @@ use crate::{ cmd::{compact_flag::CompactFlag, conversation_id::PositionalIds}, }; +/// Assert a fork reproduces its source, entry IDs included. +/// +/// A fork copies the source's entries rather than rebuilding them, so each one +/// keeps its ID and a reference into the source resolves against the fork. +/// +/// The leading `TurnStart` is the exception, and is compared by payload alone: +/// neither stream stored one, so `sanitize` synthesized a separate entry on +/// each side. +/// Two independently created entries are two entries, and their IDs are meant +/// to differ. +fn assert_forked_stream_matches(source: &ConversationStream, fork: &ConversationStream) { + let stored_ids = |stream: &ConversationStream| -> Vec { + stream + .iter() + .filter(|event| !event.is_turn_start()) + .map(|event| event.event_id.to_string()) + .collect() + }; + assert_eq!( + stored_ids(source), + stored_ids(fork), + "a forked entry must keep the ID it had in the source" + ); + + let payloads = |stream: &ConversationStream| -> Vec { + let (_, mut events) = stream.to_parts().unwrap(); + for event in &mut events { + event.as_object_mut().unwrap().shift_remove("event_id"); + } + events + }; + assert_eq!(payloads(source), payloads(fork)); + assert_eq!(source.base_config(), fork.base_config()); + assert_eq!(source.created_at, fork.created_at); +} + /// Parse a [`TurnSelection`] from the flags a user would pass to `jp c fork`. /// /// Going through clap keeps these cases pinned to the real flag surface rather @@ -173,7 +210,7 @@ fn test_conversation_fork() { assert!(convs[0].0.timestamp() < convs[1].0.timestamp()); assert_eq!(convs[0].1, convs[1].1); convs[0].2.sanitize(); - assert_eq!(convs[0].2, convs[1].2); + assert_forked_stream_matches(&convs[0].2, &convs[1].2); }, }), ("no turns keeps config but drops every turn", TestCase { @@ -282,7 +319,7 @@ fn test_conversation_fork() { assert!(convs[0].0.timestamp() < convs[1].0.timestamp()); assert_eq!(convs[0].1, convs[1].1); convs[0].2.sanitize(); - assert_eq!(convs[0].2, convs[1].2); + assert_forked_stream_matches(&convs[0].2, &convs[1].2); }, }), ("with from", TestCase { diff --git a/crates/jp_cli/src/cmd/conversation/grep.rs b/crates/jp_cli/src/cmd/conversation/grep.rs index 9431cbd51..3272ca6ac 100644 --- a/crates/jp_cli/src/cmd/conversation/grep.rs +++ b/crates/jp_cli/src/cmd/conversation/grep.rs @@ -415,7 +415,7 @@ impl Grep { // `iter_events_by_turn` rather than `iter_turns`: the latter resolves and // clones a `PartialAppConfig` per event, which grep never reads. - for (index, event) in events.iter_events_by_turn() { + for event in events.iter_events_by_turn() { if budget.is_exhausted() { return group; } @@ -434,7 +434,7 @@ impl Grep { &mut group.hits, // 1-based to match the `--turn` selector and the headers `print` // renders. - Some(index + 1), + Some(event.turn + 1), scope, Some(event.timestamp), &line_refs, diff --git a/crates/jp_cli/src/cmd/plugin/dispatch_tests.rs b/crates/jp_cli/src/cmd/plugin/dispatch_tests.rs index b11f2b7d7..27d48d4cc 100644 --- a/crates/jp_cli/src/cmd/plugin/dispatch_tests.rs +++ b/crates/jp_cli/src/cmd/plugin/dispatch_tests.rs @@ -145,6 +145,73 @@ fn draft(response: HostToPlugin) -> jp_plugin::message::DraftResponse { } } +/// Unwrap an events response, or say what came back instead. +fn events(response: HostToPlugin) -> jp_plugin::message::EventsResponse { + match response { + HostToPlugin::Events(events) => events, + other => panic!("expected an events response, got {other:?}"), + } +} + +/// Every entry a plugin reads carries the `event_id` its stored form has. +/// +/// This is the one user-visible consequence of stable event IDs: a plugin can +/// name an entry it read and have that name still mean the same entry later. +/// Asserted for every entry rather than the first, because only conversation +/// events reach the iteration views — a compaction is addressable here and +/// nowhere else. +#[test] +fn read_events_gives_a_plugin_each_entrys_id() { + let (ws, id, _tmp) = workspace_with_conversation(); + let handle = ws.acquire_conversation(&id).unwrap(); + let stored_ids = ws.test_lock(handle).as_mut().update_events(|stream| { + stream.start_turn("question"); + stream.add_compaction(jp_conversation::Compaction::new(0, 0)); + stream + .to_parts() + .unwrap() + .1 + .iter() + .map(|event| event["event_id"].as_str().unwrap().to_owned()) + .collect::>() + }); + + let read = events(handle_read_events(&ws, &wire_id(id), None)); + + assert_eq!(read.conversation, wire_id(id)); + let read_ids: Vec<_> = read + .data + .iter() + .map(|event| event["event_id"].as_str().unwrap_or_default().to_owned()) + .collect(); + assert_eq!(read_ids, stored_ids); + assert!(read_ids.iter().all(|id| !id.is_empty())); +} + +/// Decoding content for the plugin must not disturb the entry's identity. +#[test] +fn read_events_decodes_content_without_touching_the_id() { + let (ws, id, _tmp) = workspace_with_conversation(); + let handle = ws.acquire_conversation(&id).unwrap(); + ws.test_lock(handle) + .as_mut() + .update_events(|stream| stream.start_turn("a question")); + + let read = events(handle_read_events(&ws, &wire_id(id), None)); + + let request = read + .data + .iter() + .find(|event| event["type"] == "chat_request") + .expect("the turn's chat request"); + assert_eq!(request["content"], "a question"); + assert!( + request["event_id"] + .as_str() + .is_some_and(|id| !id.is_empty()) + ); +} + /// A conversation with no draft reads back empty rather than as an error: most /// conversations never have one. #[test] diff --git a/crates/jp_conversation/Cargo.toml b/crates/jp_conversation/Cargo.toml index 467f09ab0..df010b2d5 100644 --- a/crates/jp_conversation/Cargo.toml +++ b/crates/jp_conversation/Cargo.toml @@ -19,6 +19,7 @@ jp_id = { workspace = true } base64 = { workspace = true, features = ["std"] } chrono = { workspace = true } +getrandom = { workspace = true } indexmap = { workspace = true, features = ["serde"] } jp_label = { workspace = true } quick-xml = { workspace = true, features = ["serialize"] } diff --git a/crates/jp_conversation/src/compaction.rs b/crates/jp_conversation/src/compaction.rs index 0c57043e3..ba737dc5e 100644 --- a/crates/jp_conversation/src/compaction.rs +++ b/crates/jp_conversation/src/compaction.rs @@ -8,7 +8,7 @@ //! //! See [RFD 064]. //! -//! [RFD 064]: https://github.com/dcdpr/jp/blob/main/docs/rfd/064-non-destructive-conversation-compaction.md +//! [RFD 064]: https://jp.computer/rfd/064 use chrono::{DateTime, Utc}; pub use jp_config::types::{byte_size::ByteSize, policy_spec::PolicySpec}; diff --git a/crates/jp_conversation/src/error.rs b/crates/jp_conversation/src/error.rs index 7da3237d5..cd251e534 100644 --- a/crates/jp_conversation/src/error.rs +++ b/crates/jp_conversation/src/error.rs @@ -16,6 +16,14 @@ pub enum Error { #[error("Invalid ID format: {0}")] InvalidIdFormat(String), + /// An event ID was empty. + /// + /// A stream entry's identity cannot be the empty string: an absent ID is + /// assigned one at load, so an empty one reaching this far is a caller + /// error rather than a legacy file. + #[error("Event ID must not be empty.")] + EmptyEventId, + /// Invalid ID. #[error("Invalid ID: {0}")] Id(#[from] jp_id::Error), diff --git a/crates/jp_conversation/src/event_id.rs b/crates/jp_conversation/src/event_id.rs new file mode 100644 index 000000000..52da318ab --- /dev/null +++ b/crates/jp_conversation/src/event_id.rs @@ -0,0 +1,240 @@ +//! Opaque identifiers for entries in a conversation stream. +//! +//! Two types, because identity has two halves. +//! [`EventId`] is one identifier: a value that is persisted, read back out of +//! hand-edited files, and handed to plugins. +//! [`EventIds`] is the set one stream has issued, and is what makes the +//! identifiers mean anything — "unique within its stream" is a property of the +//! set, not of any value in it. +//! +//! They live together because the rules belong together: the generated format, +//! what counts as a valid identifier, and the promise that an issued identifier +//! is never issued twice are one subject, and splitting them would leave a +//! reader of either half unable to check the other. +//! +//! Only [`EventId`] leaves the crate. + +use std::{collections::HashSet, fmt, str::FromStr}; + +use getrandom::fill; +use serde::{Deserialize, Deserializer, Serialize, de::Error as _}; + +use crate::Error; + +/// Characters a generated ID is built from. +/// +/// Applies only to generation. +/// Deserialization accepts any non-empty string. +const ALPHABET: &[u8; 36] = b"0123456789abcdefghijklmnopqrstuvwxyz"; + +/// Characters in a generated ID. +/// +/// Seven base-36 characters give roughly 78 billion values, which matches Git's +/// short-ref ergonomics and leaves collisions negligible at the ~10k entries a +/// conversation holds. +const GENERATED_LEN: usize = 7; + +/// The smallest byte value that maps unevenly onto [`ALPHABET`]. +/// +/// 252 is the largest multiple of 36 at or below 256, so folding 252..=255 into +/// the alphabet would make four of its characters likelier than the rest. +/// Those bytes are rejected instead. +const BIASED_FROM: u8 = 252; + +/// Bytes drawn per attempt at generating an ID. +/// +/// Each byte has a 4-in-256 chance of being rejected, so 32 bytes yield fewer +/// than [`GENERATED_LEN`] usable ones about once in 10^13 draws. +/// Drawing enough to finish in one pass keeps the retry a formality rather than +/// a path worth reasoning about. +const DRAW_LEN: usize = 32; + +/// An opaque, non-empty identifier for a conversation stream entry. +/// +/// Serialized as a string. +/// Deserialization preserves any non-empty string verbatim, including values +/// outside the generated format. +/// IDs carry identity within a stream, not ordering or content information. +/// The generated format is internal and may change. +/// +/// Once persisted, an entry's ID can be used as a stable reference into the raw +/// conversation stream. +/// Retained entries keep that ID in projected views. +/// +/// Entries synthesized only for a projected view have ephemeral IDs with no +/// corresponding entry in the raw stream. +/// The ID value does not encode this distinction. +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize)] +#[serde(transparent)] +pub struct EventId(String); + +impl EventId { + /// Generate an ID using the OS random source. + /// + /// Collisions are possible; [`EventIds`] is what enforces uniqueness within + /// a stream. + /// + /// # Panics + /// + /// Panics if the OS random source fails. + #[must_use] + pub fn random() -> Self { + let mut id = String::with_capacity(GENERATED_LEN); + + // Terminates because each draw is independent: every pass has an + // overwhelming chance of filling the ID, so the loop is bounded in + // practice by the first one. A pass that rejected too many bytes simply + // draws again. + while id.len() < GENERATED_LEN { + let mut bytes = [0; DRAW_LEN]; + fill(&mut bytes).expect("failed to generate event ID: OS random source failed"); + + for byte in bytes { + if byte >= BIASED_FROM { + continue; + } + + id.push(char::from(ALPHABET[usize::from(byte) % ALPHABET.len()])); + if id.len() == GENERATED_LEN { + return Self(id); + } + } + } + + Self(id) + } + + /// Build an ID from a string, as a hand-edited file or a test fixture would + /// name one. + /// + /// Any non-empty string is accepted, including values outside the generated + /// format: the format is a generation convention, not a parsing constraint. + /// + /// A stream assigns IDs itself, so this is for naming an ID that already + /// exists, not for minting one. + /// Use [`ConversationStream::push_event`] to add an entry and learn the ID + /// it was given. + /// + /// # Errors + /// + /// Returns [`Error::EmptyEventId`] if `value` is empty. + /// + /// [`ConversationStream::push_event`]: crate::ConversationStream::push_event + pub fn new(value: impl Into) -> Result { + let value = value.into(); + if value.is_empty() { + return Err(Error::EmptyEventId); + } + + Ok(Self(value)) + } + + /// Build a non-empty, readable ID for a test fixture. + /// + /// This CANNOT be used in release mode. + /// + /// # Panics + /// + /// Panics if `value` is empty, which in a fixture is a mistake in the test + /// rather than a condition to handle. + #[cfg(debug_assertions)] + #[doc(hidden)] + #[must_use] + pub fn fixed(value: &str) -> Self { + Self::new(value).expect("event ID must not be empty") + } +} + +impl FromStr for EventId { + type Err = Error; + + fn from_str(value: &str) -> Result { + Self::new(value) + } +} + +impl fmt::Display for EventId { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(&self.0) + } +} + +impl<'de> Deserialize<'de> for EventId { + fn deserialize>(deserializer: D) -> Result { + Self::new(String::deserialize(deserializer)?).map_err(D::Error::custom) + } +} + +/// The entry IDs one conversation stream has handed out. +/// +/// This is where "unique within its stream" is enforced: an ID handed out here +/// is never handed out again, not even after the entry holding it is removed. +/// A reference to a removed entry therefore fails to resolve instead of binding +/// to a later, unrelated entry. +/// +/// The set is built from the entries a stream loaded and is never persisted, so +/// this covers one stream's lifetime rather than one conversation's history. +#[expect( + clippy::redundant_pub_crate, + reason = "the module is private today; `pub` would read as public API" +)] +#[derive(Debug, Clone, Default)] +pub(crate) struct EventIds(HashSet); + +impl EventIds { + /// Take `preferred`, or a generated ID when this set already holds it. + /// + /// This is how an entry moving between streams keeps its ID: uniqueness is + /// scoped to a single stream, so the receiving stream only has to replace + /// an ID it has handed out itself. + pub(crate) fn claim(&mut self, preferred: EventId) -> EventId { + if self.0.insert(preferred.clone()) { + return preferred; + } + + self.fresh() + } + + /// A generated ID this set has not handed out. + pub(crate) fn fresh(&mut self) -> EventId { + self.draw(EventId::random) + } + + /// Whether this set has handed out `id`. + #[cfg(test)] + pub(crate) fn contains(&self, id: &EventId) -> bool { + self.0.contains(id) + } + + /// Reserve `ids`, so none of them is ever generated. + /// + /// Used to take in every ID a file carries before any entry is settled, so + /// a generated ID cannot take one belonging to an entry further down the + /// file. + pub(crate) fn reserve(&mut self, ids: impl IntoIterator) { + self.0.extend(ids); + } + + /// Draw from `generate` until it produces an ID this set does not hold. + /// + /// Split out from [`Self::fresh`] so a test can script the generator and + /// force the retry; nothing outside this module supplies one. + fn draw(&mut self, mut generate: impl FnMut() -> EventId) -> EventId { + loop { + let id = generate(); + if self.0.insert(id.clone()) { + return id; + } + } + } +} + +impl FromIterator for EventIds { + fn from_iter>(iter: T) -> Self { + Self(iter.into_iter().collect()) + } +} + +#[cfg(test)] +#[path = "event_id_tests.rs"] +mod tests; diff --git a/crates/jp_conversation/src/event_id_tests.rs b/crates/jp_conversation/src/event_id_tests.rs new file mode 100644 index 000000000..eec7be055 --- /dev/null +++ b/crates/jp_conversation/src/event_id_tests.rs @@ -0,0 +1,221 @@ +use std::str::FromStr as _; + +use serde_json::{from_str, to_string}; + +use super::{EventIds, GENERATED_LEN}; +use crate::{Error, EventId}; + +#[test] +fn new_accepts_a_non_empty_string() { + assert_eq!(EventId::new("k3m9x2a").unwrap(), EventId::fixed("k3m9x2a")); +} + +#[test] +fn new_rejects_an_empty_string() { + assert!(matches!(EventId::new(""), Err(Error::EmptyEventId))); +} + +#[test] +fn new_accepts_a_value_outside_the_generated_format() { + // The format is a generation convention, so a caller naming an ID a + // hand-edited file holds is not held to it. + assert!(EventId::new("Hand-edited ID: 42!").is_ok()); +} + +#[test] +fn parsing_from_a_str_matches_new() { + assert_eq!( + EventId::from_str("k3m9x2a").unwrap(), + EventId::new("k3m9x2a").unwrap() + ); + assert!(matches!(EventId::from_str(""), Err(Error::EmptyEventId))); +} + +#[test] +fn serializes_as_a_bare_string() { + let id = EventId::fixed("k3m9x2a"); + + assert_eq!(to_string(&id).unwrap(), r#""k3m9x2a""#); +} + +#[test] +fn deserializes_from_a_bare_string() { + assert_eq!( + from_str::(r#""k3m9x2a""#).unwrap(), + EventId::fixed("k3m9x2a") + ); +} + +#[test] +fn deserialize_rejects_an_empty_string() { + let error = from_str::(r#""""#).expect_err("empty event ID must be rejected"); + + assert!( + error.to_string().contains("Event ID must not be empty"), + "unexpected message: {error}" + ); +} + +#[test] +fn deserialize_rejects_a_non_string() { + for json in ["null", "7", "true", "[]", "{}"] { + assert!(from_str::(json).is_err(), "accepted {json}"); + } +} + +#[test] +fn a_hand_edited_id_outside_the_generated_format_round_trips() { + // The lowercase-alphanumeric format is a generation convention, not a + // parsing constraint, so a hand-edited file keeps whatever it holds. + let raw = r#""Hand-edited ID: 42!""#; + let id = from_str::(raw).unwrap(); + + assert_eq!(id, EventId::fixed("Hand-edited ID: 42!")); + assert_eq!(to_string(&id).unwrap(), raw); +} + +#[test] +fn an_id_holding_json_significant_characters_round_trips() { + let raw = r#""quote\" backslash\\ brace}""#; + let id = from_str::(raw).unwrap(); + + assert_eq!(to_string(&id).unwrap(), raw); +} + +#[test] +fn a_non_ascii_id_round_trips() { + let id = from_str::(r#""é☃""#).unwrap(); + + assert_eq!(id, EventId::fixed("\u{00e9}\u{2603}")); + assert_eq!(to_string(&id).unwrap(), r#""é☃""#); +} + +#[test] +fn a_whitespace_only_id_round_trips() { + // Non-empty is the only rule, and whitespace satisfies it. + let raw = r#"" \t\n ""#; + let id = from_str::(raw).unwrap(); + + assert_eq!(id, EventId::fixed(" \t\n ")); + assert_eq!(to_string(&id).unwrap(), raw); +} + +#[test] +fn display_writes_the_id_alone() { + assert_eq!(EventId::fixed("k3m9x2a").to_string(), "k3m9x2a"); +} + +#[test] +fn debug_names_the_type() { + assert_eq!( + format!("{:?}", EventId::fixed("k3m9x2a")), + r#"EventId("k3m9x2a")"# + ); +} + +#[test] +fn a_generated_id_has_the_documented_length() { + assert_eq!(EventId::random().to_string().len(), GENERATED_LEN); +} + +#[test] +fn a_generated_id_uses_lowercase_base36() { + // A thousand draws is enough to catch a biased or mis-masked byte leaking + // a character outside the alphabet. + for _ in 0..1_000 { + let id = EventId::random().to_string(); + assert!( + id.bytes() + .all(|byte| byte.is_ascii_lowercase() || byte.is_ascii_digit()), + "generated ID outside the alphabet: {id}" + ); + } +} + +#[test] +fn two_draws_produce_distinct_ids() { + assert_ne!(EventId::random(), EventId::random()); +} + +#[test] +#[should_panic(expected = "event ID must not be empty")] +fn a_fixed_id_must_not_be_empty() { + // `EventId::fixed` is a fixture constructor, so it panics rather than + // returning a `Result`: an empty literal in a test is a mistake in the + // test. + let _id = EventId::fixed(""); +} + +#[test] +fn claim_takes_an_id_the_set_does_not_hold() { + let mut ids = EventIds::default(); + + assert_eq!(ids.claim(EventId::fixed("inherited")), { + EventId::fixed("inherited") + }); +} + +#[test] +fn claim_replaces_an_id_the_set_already_holds() { + let mut ids: EventIds = std::iter::once(EventId::fixed("taken")).collect(); + + let claimed = ids.claim(EventId::fixed("taken")); + + assert_ne!(claimed, EventId::fixed("taken")); + assert_eq!(claimed.to_string().len(), GENERATED_LEN); +} + +#[test] +fn claim_replaces_an_id_it_handed_out_itself() { + // An ID is retired with its entry, so a second claim of the same value + // does not get it back. This is what makes a reference to a removed entry + // fail rather than rebind. + let mut ids = EventIds::default(); + ids.claim(EventId::fixed("once")); + + assert_ne!(ids.claim(EventId::fixed("once")), EventId::fixed("once")); +} + +#[test] +fn a_draw_retries_past_an_id_the_set_already_holds() { + let mut ids: EventIds = [EventId::fixed("taken01"), EventId::fixed("taken02")] + .into_iter() + .collect(); + let mut candidates = ["taken01", "taken02", "fresh01"] + .map(EventId::fixed) + .into_iter(); + + let drawn = ids.draw(|| candidates.next().expect("scripted candidate")); + + assert_eq!(drawn, EventId::fixed("fresh01")); + assert!(candidates.next().is_none(), "stopped before the third draw"); +} + +#[test] +fn a_draw_retries_past_an_id_the_set_handed_out_earlier() { + let mut ids = EventIds::default(); + let mut candidates = ["first01", "first01", "second1"] + .map(EventId::fixed) + .into_iter(); + + let first = ids.draw(|| candidates.next().expect("scripted candidate")); + let second = ids.draw(|| candidates.next().expect("scripted candidate")); + + assert_eq!(first, EventId::fixed("first01")); + assert_eq!(second, EventId::fixed("second1")); +} + +#[test] +fn a_thousand_draws_produce_distinct_ids() { + // The weak half of the pair: a thousand draws from a 36^7 space are almost + // certainly distinct whether or not `fresh` checks the set, so what this + // pins is that `EventId::random` varies at all — a generator returning one + // value forever satisfies every other test in this file. + // `a_draw_retries_past_an_id_the_set_handed_out_earlier` pins the retry + // itself, deterministically. + let mut ids = EventIds::default(); + let drawn: Vec<_> = (0..1_000).map(|_| ids.fresh()).collect(); + + let unique: std::collections::HashSet<_> = drawn.iter().collect(); + assert_eq!(unique.len(), drawn.len()); +} diff --git a/crates/jp_conversation/src/lib.rs b/crates/jp_conversation/src/lib.rs index 907653d01..e8c646364 100644 --- a/crates/jp_conversation/src/lib.rs +++ b/crates/jp_conversation/src/lib.rs @@ -32,6 +32,7 @@ mod compat; pub mod conversation; pub mod error; pub mod event; +mod event_id; pub mod patch; pub(crate) mod storage; pub mod stream; @@ -44,6 +45,7 @@ pub use compaction::{ pub use conversation::{Conversation, ConversationId}; pub use error::Error; pub use event::{ConversationEvent, EventKind}; +pub use event_id::EventId; pub use jp_label::Labels; pub use patch::{EventOverlay, OverlayAction, OverlayMatcher, OverlayPatch}; pub use storage::{decode_event_value, rfc3339}; diff --git a/crates/jp_conversation/src/stream.rs b/crates/jp_conversation/src/stream.rs index e2c5dd843..919997514 100644 --- a/crates/jp_conversation/src/stream.rs +++ b/crates/jp_conversation/src/stream.rs @@ -1,422 +1,55 @@ -//! See [`ConversationStream`]. +//! A conversation's event log. +//! +//! [`ConversationStream`] is the entry point: an ordered list of entries plus +//! the base configuration they layer onto. +//! It owns appending, pruning, repairing, and iterating, and is the only thing +//! that hands out entry IDs. +//! +//! The surrounding modules hold the pieces it is built from: +//! +//! - [`entry`] — what one entry is, and how it is stored. +//! - [`config_delta`] — the entries that change a conversation's config. +//! - [`iter`] — walking the stream, resolving each event's config as it goes. +//! - [`projection`] — the provider-facing view, with overlays applied. +//! - [`turn_iter`] and [`turn_mut`] — reading and writing a turn at a time. use std::{ - collections::{BTreeSet, HashMap}, + collections::{BTreeSet, HashMap, HashSet}, sync::Arc, }; use chrono::{DateTime, Utc}; -use jp_config::{AppConfig, ConfigError, FillDefaults as _, PartialAppConfig, PartialConfig as _}; -use serde::{Deserialize, Serialize, Serializer}; +use jp_config::{AppConfig, FillDefaults as _, PartialAppConfig, PartialConfig as _}; use serde_json::{Map, Value}; -use tracing::{error, warn}; +use tracing::warn; +pub mod config_delta; +mod entry; +pub mod iter; mod projection; pub mod turn_iter; pub mod turn_mut; +pub use config_delta::{ApplyDelta, ConfigDelta, ResetDelta}; +pub use iter::{ + ConversationEventWithConfig, ConversationEventWithConfigMut, ConversationEventWithConfigRef, + EventInTurn, IntoIter, IterMut, +}; pub use projection::{AffectedItem, TurnOrigin}; pub use turn_iter::{IterTurns, Turn}; pub use turn_mut::TurnMut; use crate::{ - Compaction, EventOverlay, OverlayPatch, - compat::deserialize_partial_config, + Compaction, EventId, EventOverlay, OverlayPatch, event::{ChatRequest, ConversationEvent, EventKind, InquiryId, ToolCallResponse, TurnStart}, - storage::{decode_event_value, encode_event}, + event_id::EventIds, + stream::{ + entry::{EventPayload, EventScope, InternalEvent, StoredEvent}, + iter::Iter, + }, }; -/// An internal representation of events in a conversation stream. -/// -/// This type handles base64-encoding of content fields (tool arguments, tool -/// response content, metadata) during serialization, and decoding during -/// deserialization. -/// This keeps the encoding concern isolated to the storage layer — the inner -/// [`ConversationEvent`] types serialize as plain text. -#[derive(Debug, Clone, PartialEq)] -enum InternalEvent { - /// The configuration state of the conversation is updated. - /// - /// When this event is emitted, all subsequent events in the stream are - /// bound to the new configuration. - /// - /// An [`Apply`] delta is merged on top of all previous `ConfigDelta` events - /// in the stream; a [`Reset`] discards the accumulated state, restarting - /// from program defaults. - /// - /// Any non-config events before the first `ConfigDelta` event are - /// considered to have the default configuration. - /// - /// [`Apply`]: ConfigDelta::Apply - /// [`Reset`]: ConfigDelta::Reset - ConfigDelta(ConfigDelta), - /// An event in the conversation stream. - Event(Box), - /// A compaction overlay that modifies how preceding events are projected - /// when building the LLM request. - /// Does not modify or delete any existing events. - Compaction(Compaction), - /// A patch overlay that rewrites how matched events are projected when - /// building the LLM request. - /// Does not modify or delete any existing events. - Overlay(EventOverlay), - /// An event whose `type` tag this build does not recognize. - /// - /// Conversations are an append-only log that a newer `jp` may have written. - /// Rather than fail the entire stream load on an unknown event kind, the - /// raw JSON is retained verbatim so it round-trips losslessly on the next - /// save. - /// Unknown events are invisible to event iteration, config resolution, and - /// providers. - Unknown(Value), -} - -impl Serialize for InternalEvent { - fn serialize(&self, serializer: S) -> Result { - match self { - Self::ConfigDelta(delta) => { - #[derive(Serialize)] - struct Tagged<'a> { - #[serde(rename = "type")] - tag: &'static str, - #[serde(flatten)] - inner: &'a ConfigDelta, - } - - Tagged { - tag: "config_delta", - inner: delta, - } - .serialize(serializer) - } - Self::Event(event) => { - let mut value = - serde_json::to_value(event.as_ref()).map_err(serde::ser::Error::custom)?; - - // Base64-encode storage fields. - encode_event(&mut value, &event.kind); - value.serialize(serializer) - } - Self::Compaction(compaction) => { - #[derive(Serialize)] - struct Tagged<'a> { - #[serde(rename = "type")] - tag: &'static str, - #[serde(flatten)] - inner: &'a Compaction, - } - - Tagged { - tag: "compaction", - inner: compaction, - } - .serialize(serializer) - } - Self::Overlay(overlay) => { - #[derive(Serialize)] - struct Tagged<'a> { - #[serde(rename = "type")] - tag: &'static str, - #[serde(flatten)] - inner: &'a EventOverlay, - } - - Tagged { - tag: "event_overlay", - inner: overlay, - } - .serialize(serializer) - } - Self::Unknown(value) => value.serialize(serializer), - } - } -} - -/// Whether an [`InternalEvent`] belongs to a single turn or applies to the -/// conversation as a whole. -/// -/// This is the single source of truth for which events survive turn-level -/// pruning (`pop`, `trim_chat_request`, `pop_if`, `retain`). -/// Adding a new `InternalEvent` variant forces a classification here — -/// [`InternalEvent::scope`] is an exhaustive match — so no pruning caller can -/// silently mistreat it. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum EventScope { - /// Survives turn pruning: config deltas and compaction overlays apply to - /// the conversation regardless of position. - Global, - /// Belongs to a turn and is removed when that turn is pruned. - Turn, -} - -impl InternalEvent { - /// Convert an internal event into an [`ConversationEvent`]. - /// Returns `None` if the event is a config delta. - #[must_use] - fn into_event(self) -> Option { - match self { - Self::Event(event) => Some(*event), - Self::ConfigDelta(_) | Self::Compaction(_) | Self::Overlay(_) | Self::Unknown(_) => { - None - } - } - } - - /// Get a reference to [`InternalEvent::Event`], if applicable. - #[must_use] - fn as_event(&self) -> Option<&ConversationEvent> { - match self { - Self::Event(event) => Some(event), - Self::ConfigDelta(_) | Self::Compaction(_) | Self::Overlay(_) | Self::Unknown(_) => { - None - } - } - } - - /// Classify the event as turn-scoped or global. - /// See [`EventScope`]. - #[must_use] - const fn scope(&self) -> EventScope { - match self { - Self::ConfigDelta(_) | Self::Compaction(_) | Self::Overlay(_) | Self::Unknown(_) => { - EventScope::Global - } - Self::Event(_) => EventScope::Turn, - } - } -} - -/// A configuration delta. -#[derive(Debug, Clone, PartialEq)] -pub enum ConfigDelta { - /// Merge a partial configuration on top of the accumulated config state. - Apply(ApplyDelta), - - /// Discard the accumulated config state. - /// - /// Config resolution restarts from program defaults; subsequent [`Apply`] - /// events layer on top. - /// - /// [`Apply`]: Self::Apply - Reset(ResetDelta), -} - -impl ConfigDelta { - /// The timestamp of the event, regardless of variant. - #[must_use] - pub const fn timestamp(&self) -> DateTime { - match self { - Self::Apply(delta) => delta.timestamp, - Self::Reset(delta) => delta.timestamp, - } - } -} - -// Hand-rolled so `Apply` keeps the legacy flat shape (no `op` field) and -// `Reset` carries `"op": "reset"`. The variant discriminator must live inside -// the event body: the outer `InternalEvent` envelope already claims the -// top-level `type` key. -impl Serialize for ConfigDelta { - fn serialize(&self, serializer: S) -> Result { - match self { - Self::Apply(delta) => delta.serialize(serializer), - Self::Reset(delta) => { - #[derive(Serialize)] - struct Tagged<'a> { - op: &'static str, - #[serde(flatten)] - inner: &'a ResetDelta, - } - - Tagged { - op: "reset", - inner: delta, - } - .serialize(serializer) - } - } - } -} - -/// A configuration delta that merges on top of the accumulated config state. -#[derive(Debug, Clone, Serialize, PartialEq)] -pub struct ApplyDelta { - /// The timestamp of the event. - #[serde(serialize_with = "crate::serialize_dt")] - pub timestamp: DateTime, - - /// The configuration delta. - pub delta: Box, - - /// Dotted paths of fields cleared before [`delta`] is merged. - /// - /// Merging is per field, so a field that merges by appending cannot reach a - /// value that drops one of its elements: whatever the delta carries is - /// added to what is already there. - /// Clearing the field first leaves the merge nothing to combine with, and - /// the delta's value lands whole. - /// - /// A path that names no field is ignored, so a delta written by a newer - /// version, or naming a field since removed, still replays. - /// - /// [`delta`]: Self::delta - #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub unsets: Vec, -} - -impl ApplyDelta { - /// An apply that merges `delta` and clears nothing. - #[must_use] - pub fn new(timestamp: DateTime, delta: impl Into>) -> Self { - Self { - timestamp, - delta: delta.into(), - unsets: Vec::new(), - } - } - - /// An apply that clears `unsets` before merging `delta`. - #[must_use] - pub fn with_unsets( - timestamp: DateTime, - delta: impl Into>, - unsets: Vec, - ) -> Self { - Self { - timestamp, - delta: delta.into(), - unsets, - } - } -} - -/// A configuration delta that discards the accumulated config state, resetting -/// it to program defaults. -#[derive(Debug, Clone, Serialize, PartialEq, Eq)] -pub struct ResetDelta { - /// The timestamp of the event. - #[serde(serialize_with = "crate::serialize_dt")] - pub timestamp: DateTime, -} - -impl From for ConfigDelta { - fn from(delta: ApplyDelta) -> Self { - Self::Apply(delta) - } -} - -impl From for ConfigDelta { - fn from(delta: ResetDelta) -> Self { - Self::Reset(delta) - } -} - -impl From for ConfigDelta { - fn from(config: PartialAppConfig) -> Self { - Self::Apply(ApplyDelta::new(Utc::now(), config)) - } -} - -/// Extract the stored config subtree from a `config_delta` event. -/// -/// Two on-disk shapes exist. -/// Newer streams nest the config under a `delta` key; older ones carry the -/// config fields as siblings of the envelope keys (`type`, `timestamp`, `op`), -/// which are removed here so only config fields remain. -fn config_delta_subtree(value: &Value) -> Value { - if let Some(delta) = value.get("delta") { - return delta.clone(); - } - - let mut obj = value.as_object().cloned().unwrap_or_default(); - obj.remove("type"); - obj.remove("timestamp"); - obj.remove("op"); - obj.remove("unsets"); - Value::Object(obj) -} - -/// Deserialize a [`ConfigDelta`] from a raw JSON value, tolerating schema -/// changes within the stored config. -/// -/// The `op` field selects the variant: absent (which covers every event written -/// before the reset variant existed) or `"apply"` decodes as -/// [`ConfigDelta::Apply`]; `"reset"` decodes as [`ConfigDelta::Reset`]. -/// Delegates to [`deserialize_partial_config`] for the config subtree and -/// extracts the timestamp separately. -/// -/// # Errors -/// -/// Returns an error for any other `op` value: an op added by a newer version -/// must fail loudly here instead of being misread as an apply and corrupting -/// config resolution. -pub(crate) fn deserialize_config_delta(value: &Value) -> Result { - let timestamp = value - .get("timestamp") - .and_then(Value::as_str) - .and_then(|s| crate::parse_dt(s).ok()) - .unwrap_or_else(Utc::now); - - if let Some(op) = value.get("op") { - if op == "reset" { - return Ok(ConfigDelta::Reset(ResetDelta { timestamp })); - } - - if op != "apply" { - return Err(format!("unknown config delta `op`: {op}")); - } - } - - // The hand-rolled deserializer bypasses the derived one, so the field's - // `#[serde(default)]` never runs and the key has to be read here. - let unsets = value - .get("unsets") - .and_then(Value::as_array) - .map(|paths| { - paths - .iter() - .filter_map(|path| path.as_str().map(str::to_owned)) - .collect() - }) - .unwrap_or_default(); - - let delta = deserialize_partial_config(config_delta_subtree(value)); - - Ok(ConfigDelta::Apply(ApplyDelta { - timestamp, - delta: Box::new(delta), - unsets, - })) -} - -/// Fold a single [`ConfigDelta`] into an accumulated partial config state. -/// -/// [`Apply`] merges the delta on top of `state`. -/// [`Reset`] discards `state`, restarting from the empty partial -/// (`PartialAppConfig::default()`); program defaults are injected when the -/// partial is finalized into an [`AppConfig`]. -/// -/// [`Apply`]: ConfigDelta::Apply -/// [`Reset`]: ConfigDelta::Reset -fn fold_config_delta(state: &mut PartialAppConfig, delta: ConfigDelta) -> Result<(), ConfigError> { - match delta { - ConfigDelta::Apply(apply) => { - for path in &apply.unsets { - if let Err(error) = state.unset(path) { - warn!(%path, %error, "Ignoring a config delta unset for an unknown field."); - } - } - - state.merge(&(), *apply.delta) - } - ConfigDelta::Reset(_) => { - *state = PartialAppConfig::default(); - Ok(()) - } - } -} - /// A stream of events that make up a conversation. -#[derive(Debug, PartialEq, Clone)] +#[derive(Debug, Clone)] pub struct ConversationStream { /// The base configuration for the conversation. /// @@ -431,10 +64,48 @@ pub struct ConversationStream { /// The events in the stream. events: Vec, + /// Every entry ID this stream has handed out, including those whose entry + /// has since been removed. + /// + /// A superset of the IDs in `events`, and the invariant every insertion + /// path relies on: an ID is retired with its entry rather than returned to + /// circulation, so a reference to a deleted entry fails to resolve instead + /// of binding to a later one. + /// Not serialized, so retirement lasts as long as this stream does: a load + /// seeds the set from the entries the file still carries. + /// + /// An entry carrying an ID this set has not seen reaches `events` only + /// through [`Self::append`], [`Self::insert`], or [`Self::adopt`], which is + /// what keeps the two in step. + /// Moving an entry the stream already holds does not go through them, and + /// must not: those take an ID the set has not handed out, so passing one it + /// has would replace the entry's ID rather than preserve it. + event_ids: EventIds, + + /// IDs more than one entry carried when this stream was loaded. + /// + /// Read through [`Self::duplicated_event_ids`], which states the deadline a + /// consumer is held to. + /// Not serialized. + duplicated_event_ids: HashSet, + /// The timestamp of the creation of the stream. pub created_at: DateTime, } +// Hand-rolled to compare what the stream *holds*, not how it came to hold it. +// `event_ids` and `duplicated_event_ids` are load- and history-scoped: two +// streams carrying identical entries would otherwise compare unequal because +// one of them was loaded from a file with duplicate IDs, or because an entry +// was pushed and popped along the way. +impl PartialEq for ConversationStream { + fn eq(&self, other: &Self) -> bool { + self.base_config == other.base_config + && self.events == other.events + && self.created_at == other.created_at + } +} + impl ConversationStream { /// Create a new [`ConversationStream`] with the given base configuration. #[must_use] @@ -442,6 +113,8 @@ impl ConversationStream { Self { base_config, events: Vec::new(), + event_ids: EventIds::default(), + duplicated_event_ids: HashSet::new(), created_at: Utc::now(), } } @@ -460,14 +133,17 @@ impl ConversationStream { self } - /// Returns `true` if the stream is empty. - /// This only considers [`ConversationEvent`]s. + /// Returns `true` if the stream holds no [`ConversationEvent`]s. + /// + /// Entries of other kinds are not counted, so a stream carrying only config + /// deltas and compactions reports `true` here and still writes those + /// entries in [`Self::to_parts`]. #[must_use] pub fn is_empty(&self) -> bool { !self .events .iter() - .any(|e| matches!(e, InternalEvent::Event(_))) + .any(|e| matches!(&e.payload, EventPayload::Event(_))) } /// Returns `true` if the stream contains at least one [`ChatRequest`]. @@ -475,16 +151,19 @@ impl ConversationStream { pub fn has_chat_request(&self) -> bool { self.events .iter() - .any(|e| matches!(e, InternalEvent::Event(event) if event.is_chat_request())) + .any(|e| matches!(&e.payload, EventPayload::Event(event) if event.is_chat_request())) } - /// Returns the number of events in the stream. - /// This only considers [`ConversationEvent`]s. + /// Returns the number of [`ConversationEvent`]s in the stream. + /// + /// Entries of other kinds are not counted, so this is at most the number of + /// entries [`Self::to_parts`] writes, and can be `0` for a stream that + /// stores several. #[must_use] pub fn len(&self) -> usize { self.events .iter() - .filter(|e| matches!(e, InternalEvent::Event(_))) + .filter(|e| matches!(&e.payload, EventPayload::Event(_))) .count() } @@ -539,16 +218,16 @@ impl ConversationStream { /// Returns an error if a delta cannot be folded onto the accumulated state. pub fn config_partial(&self) -> Result { let mut partial = self.base_config.to_partial(); - let iter = self.events.iter().filter_map(|event| match event { - InternalEvent::ConfigDelta(delta) => Some(delta.clone()), - InternalEvent::Event(_) - | InternalEvent::Compaction(_) - | InternalEvent::Overlay(_) - | InternalEvent::Unknown(_) => None, + let iter = self.events.iter().filter_map(|event| match &event.payload { + EventPayload::ConfigDelta(delta) => Some(delta.clone()), + EventPayload::Event(_) + | EventPayload::Compaction(_) + | EventPayload::Overlay(_) + | EventPayload::Unknown(_) => None, }); for delta in iter { - fold_config_delta(&mut partial, delta)?; + config_delta::fold(&mut partial, delta)?; } Ok(partial) @@ -615,6 +294,10 @@ impl ConversationStream { // Re-append preserved overlays in their original order, whether or not // a request was found, so they survive the trim. + // + // Appended directly rather than through `adopt`: these are the entries + // just popped, so `event_ids` still holds their IDs and `adopt` would + // read that as a collision and reassign them. preserved.reverse(); self.events.append(&mut preserved); @@ -654,7 +337,7 @@ impl ConversationStream { return; } - self.events.push(InternalEvent::ConfigDelta(delta)); + self.append(EventPayload::ConfigDelta(delta)); } /// Append a config reset point followed by the state layered on top of it. @@ -690,7 +373,7 @@ impl ConversationStream { /// Add a compaction overlay to the stream. pub fn add_compaction(&mut self, compaction: Compaction) { - self.events.push(InternalEvent::Compaction(compaction)); + self.append(EventPayload::Compaction(compaction)); } /// Append a patch overlay, returning how many events its patches change in @@ -706,7 +389,7 @@ impl ConversationStream { pub fn add_overlay(&mut self, patches: Vec) -> usize { let changed = self.count_overlay_changes(&patches); - self.events.push(InternalEvent::Overlay(EventOverlay { + self.append(EventPayload::Overlay(EventOverlay { timestamp: Utc::now(), patches, })); @@ -720,8 +403,8 @@ impl ConversationStream { let existing: Vec<&OverlayPatch> = self .events .iter() - .filter_map(|e| match e { - InternalEvent::Overlay(overlay) => Some(&overlay.patches), + .filter_map(|e| match &e.payload { + EventPayload::Overlay(overlay) => Some(&overlay.patches), _ => None, }) .flatten() @@ -750,7 +433,7 @@ impl ConversationStream { pub fn remove_compactions(&mut self) -> usize { let before = self.events.len(); self.events - .retain(|e| !matches!(e, InternalEvent::Compaction(_))); + .retain(|e| !matches!(&e.payload, EventPayload::Compaction(_))); before - self.events.len() } @@ -764,36 +447,36 @@ impl ConversationStream { .events .iter() .enumerate() - .filter(|(_, event)| matches!(event, InternalEvent::Compaction(_))) + .filter(|(_, event)| matches!(&event.payload, EventPayload::Compaction(_))) .map(|(position, _)| position) .nth(index)?; - match self.events.remove(position) { - InternalEvent::Compaction(compaction) => Some(compaction), + match self.events.remove(position).payload { + EventPayload::Compaction(compaction) => Some(compaction), _ => unreachable!("position points at a compaction event"), } } /// Returns an iterator over the [`Compaction`] events in the stream. pub fn compactions(&self) -> impl Iterator { - self.events.iter().filter_map(|e| match e { - InternalEvent::Compaction(c) => Some(c), + self.events.iter().filter_map(|e| match &e.payload { + EventPayload::Compaction(c) => Some(c), _ => None, }) } /// Returns an iterator over the [`ConfigDelta`] events in the stream. pub fn config_deltas(&self) -> impl Iterator { - self.events.iter().filter_map(|e| match e { - InternalEvent::ConfigDelta(delta) => Some(delta), + self.events.iter().filter_map(|e| match &e.payload { + EventPayload::ConfigDelta(delta) => Some(delta), _ => None, }) } /// Returns an iterator over the [`EventOverlay`] events in the stream. pub fn overlays(&self) -> impl Iterator { - self.events.iter().filter_map(|e| match e { - InternalEvent::Overlay(o) => Some(o), + self.events.iter().filter_map(|e| match &e.payload { + EventPayload::Overlay(o) => Some(o), _ => None, }) } @@ -807,11 +490,17 @@ impl ConversationStream { /// patch overlays and events this build does not recognize have to survive /// as well. /// + /// Appended entries keep their IDs, so a reference into `other` still + /// resolves against the copy. + /// An ID this stream has already handed out is replaced on the incoming + /// entry. /// `other`'s config deltas are appended verbatim rather than recomputed, so /// the two streams must share a base config for the result to resolve the /// same way. pub fn append_stream(&mut self, other: Self) { - self.events.extend(other.events); + for entry in other.events { + self.adopt(entry); + } } /// Apply projection to the stream. @@ -828,12 +517,15 @@ impl ConversationStream { /// When the stream carries neither patch overlays nor compactions, the /// events are left unchanged and every turn maps to its own index. /// - /// This method is called by [`Thread::into_parts()`] before provider - /// visibility filtering. + /// Apply this to a copy to preserve the raw stream. + /// Retained entries keep their IDs, which reference the original raw + /// entries even when projection changes their content in this view. /// - /// [`Thread::into_parts()`]: crate::thread::Thread::into_parts + /// Synthetic summary entries receive ephemeral IDs and have no + /// corresponding entry in `events.json`. + /// Do not use those synthetic IDs as references into the raw stream. pub fn apply_projection(&mut self) -> Vec { - projection::apply(&mut self.events) + projection::apply(&mut self.events, &mut self.event_ids) } /// List the items `compaction`'s mechanical policies reach, in stream @@ -885,7 +577,7 @@ impl ConversationStream { let has_turn = self .events .iter() - .any(|e| matches!(e, InternalEvent::Event(event) if event.is_turn_start())); + .any(|e| matches!(&e.payload, EventPayload::Event(event) if event.is_turn_start())); if !has_turn { self.push(ConversationEvent::now(TurnStart)); @@ -894,10 +586,89 @@ impl ConversationStream { TurnMut::new(self) } - /// Push a [`ConversationEvent`] onto the stream. - fn push(&mut self, event: impl Into) { + /// Append a payload, returning the ID the stream assigned it. + fn append(&mut self, payload: EventPayload) -> EventId { + let event_id = self.event_ids.fresh(); + self.events.push(InternalEvent { + event_id: event_id.clone(), + payload, + }); + event_id + } + + /// Insert a payload at `index`, returning the ID the stream assigned it. + fn insert(&mut self, index: usize, payload: EventPayload) -> EventId { + let event_id = self.event_ids.fresh(); + self.events.insert(index, InternalEvent { + event_id: event_id.clone(), + payload, + }); + event_id + } + + /// Append an entry from another stream, keeping its ID when this stream has + /// not handed that ID out. + /// + /// IDs are unique within a stream, so an entry arriving from elsewhere can + /// keep the identity it already has, and references into the source stream + /// keep resolving against the copy. + /// Only a collision with an ID this stream has handed out forces a new one. + fn adopt(&mut self, entry: InternalEvent) -> EventId { + let event_id = self.event_ids.claim(entry.event_id); + self.events.push(InternalEvent { + event_id: event_id.clone(), + payload: entry.payload, + }); + event_id + } + + /// Append a [`ConversationEvent`], returning the ID the stream assigned it. + /// + /// The ID identifies this entry for as long as it is in the stream, and is + /// persisted with it. + /// Turn boundaries are not created here; use [`Self::start_turn`]. + /// + /// There is no way back from an [`EventId`] to the entry holding it yet, so + /// a caller keeps the returned ID for what it writes elsewhere rather than + /// to look the entry up again. + /// + /// [`Self::push`] is the same append for a caller with no use for the ID. + pub fn push_event(&mut self, event: impl Into) -> EventId { + self.append(EventPayload::Event(Box::new(event.into()))) + } + + /// Append entries carrying known IDs. + /// + /// IDs are normally assigned by the stream, so a test that needs to name + /// one builds the entries itself. + /// Going through here registers those IDs, which is what the uniqueness + /// invariant on `event_ids` needs; pushing onto `events` directly would + /// leave the stream able to hand out an ID it already holds. + #[cfg(test)] + fn extend_entries(&mut self, entries: impl IntoIterator) { + for entry in entries { + self.adopt(entry); + } + } + + /// Whether `event_ids` still holds every ID the stream's entries carry. + /// + /// The invariant that field documents, checked directly. + /// Watching for a reused ID would not: two generated IDs practically never + /// collide, so the check would hold whether or not the set was in step. + #[cfg(test)] + fn id_set_covers_entries(&self) -> bool { self.events - .push(InternalEvent::Event(Box::new(event.into()))); + .iter() + .all(|entry| self.event_ids.contains(&entry.event_id)) + } + + /// Append a [`ConversationEvent`], discarding the ID it was given. + /// + /// [`Self::push_event`] for the many callers that only want the event in + /// the stream, so they do not each write `let _ =`. + fn push(&mut self, event: impl Into) { + self.push_event(event); } /// Returns the structured output schema for the current turn. @@ -917,7 +688,7 @@ impl ConversationStream { let turn_start = self .events .iter() - .rposition(|e| matches!(e, InternalEvent::Event(ev) if ev.is_turn_start())); + .rposition(|e| matches!(&e.payload, EventPayload::Event(ev) if ev.is_turn_start())); let search_from = turn_start.map_or(0, |pos| pos + 1); @@ -981,10 +752,15 @@ impl ConversationStream { .last() .map_or_else(|| self.base_config.to_partial(), |v| v.config); - self.events - .remove(pos) + let internal = self.events.remove(pos); + let event_id = internal.event_id.clone(); + internal .into_event() - .map(|event| ConversationEventWithConfig { event, config }) + .map(|event| ConversationEventWithConfig { + event_id, + event, + config, + }) } /// Returns the last turn-scoped [`ConversationEvent`] in the stream, @@ -1060,14 +836,14 @@ impl ConversationStream { /// turn-truncation helpers and the `fork` time filter inherit it without /// each tracking overlay validity themselves. /// - /// [RFD 097]: https://github.com/dcdpr/jp/blob/main/docs/rfd/097-stable-event-identifiers.md + /// [RFD 097]: https://jp.computer/rfd/097 pub fn retain(&mut self, mut f: impl FnMut(&ConversationEvent) -> bool) { // Fast path: with no overlays present there's nothing to invalidate, so // skip the turn-index bookkeeping. if !self .events .iter() - .any(|e| matches!(e, InternalEvent::Compaction(_))) + .any(|e| matches!(&e.payload, EventPayload::Compaction(_))) { self.events.retain(|event| match event.scope() { EventScope::Global => true, @@ -1097,8 +873,8 @@ impl ConversationStream { // Drop only overlays the removal could have invalidated: those whose // range reaches the earliest removed turn or beyond. if let Some(threshold) = first_removed_turn { - self.events.retain(|event| match event { - InternalEvent::Compaction(c) => c.to_turn < threshold, + self.events.retain(|event| match &event.payload { + EventPayload::Compaction(c) => c.to_turn < threshold, _ => true, }); } @@ -1152,11 +928,9 @@ impl ConversationStream { /// maintain configuration state, and turn markers are invisible to /// providers but useful for `--last`. fn drop_leading_non_user_events(&mut self) { - let Some(pos) = self - .events - .iter() - .position(|e| matches!(e, InternalEvent::Event(event) if event.is_chat_request())) - else { + let Some(pos) = self.events.iter().position( + |e| matches!(&e.payload, EventPayload::Event(event) if event.is_chat_request()), + ) else { return; }; @@ -1167,12 +941,12 @@ impl ConversationStream { if i >= pos { return true; } - match event { - InternalEvent::ConfigDelta(_) - | InternalEvent::Compaction(_) - | InternalEvent::Overlay(_) - | InternalEvent::Unknown(_) => true, - InternalEvent::Event(e) => e.is_turn_start(), + match &event.payload { + EventPayload::ConfigDelta(_) + | EventPayload::Compaction(_) + | EventPayload::Overlay(_) + | EventPayload::Unknown(_) => true, + EventPayload::Event(e) => e.is_turn_start(), } }); } @@ -1304,17 +1078,16 @@ impl ConversationStream { if self .events .iter() - .all(|e| !matches!(e, InternalEvent::Event(event) if !event.is_turn_start())) + .all(|e| !matches!(&e.payload, EventPayload::Event(event) if !event.is_turn_start())) { // Stream has no non-TurnStart events, nothing to normalize. return; } // Find the position of the first ChatRequest. - let first_chat_pos = self - .events - .iter() - .position(|e| matches!(e, InternalEvent::Event(event) if event.is_chat_request())); + let first_chat_pos = self.events.iter().position( + |e| matches!(&e.payload, EventPayload::Event(event) if event.is_chat_request()), + ); // Remove all but the last TurnStart before the first ChatRequest. This // collapses multiple stale turn markers from filtered turns into a @@ -1323,7 +1096,7 @@ impl ConversationStream { let leading_turn_starts: Vec = self.events[..chat_pos] .iter() .enumerate() - .filter(|(_, e)| matches!(e, InternalEvent::Event(event) if event.is_turn_start())) + .filter(|(_, e)| matches!(&e.payload, EventPayload::Event(event) if event.is_turn_start())) .map(|(i, _)| i) .collect(); @@ -1341,15 +1114,15 @@ impl ConversationStream { } // Ensure there's a TurnStart before the first ChatRequest. - let first_event_is_turn_start = - self.events - .iter() - .any(|e| matches!(e, InternalEvent::Event(event) if event.is_turn_start())) - && self.events.iter().position( - |e| matches!(e, InternalEvent::Event(event) if event.is_turn_start()), - ) < self.events.iter().position( - |e| matches!(e, InternalEvent::Event(event) if event.is_chat_request()), - ); + let first_event_is_turn_start = self + .events + .iter() + .any(|e| matches!(&e.payload, EventPayload::Event(event) if event.is_turn_start())) + && self.events.iter().position( + |e| matches!(&e.payload, EventPayload::Event(event) if event.is_turn_start()), + ) < self.events.iter().position( + |e| matches!(&e.payload, EventPayload::Event(event) if event.is_chat_request()), + ); if !first_event_is_turn_start { // Find where to insert (right before the first ChatRequest, @@ -1357,7 +1130,9 @@ impl ConversationStream { let insert_pos = self .events .iter() - .position(|e| matches!(e, InternalEvent::Event(event) if event.is_chat_request())) + .position( + |e| matches!(&e.payload, EventPayload::Event(event) if event.is_chat_request()), + ) .unwrap_or(0); let timestamp = self @@ -1366,9 +1141,9 @@ impl ConversationStream { .and_then(InternalEvent::as_event) .map_or(DateTime::::UNIX_EPOCH, |e| e.timestamp); - self.events.insert( + self.insert( insert_pos, - InternalEvent::Event(Box::new(ConversationEvent::new(TurnStart, timestamp))), + EventPayload::Event(Box::new(ConversationEvent::new(TurnStart, timestamp))), ); } } @@ -1417,14 +1192,16 @@ impl ConversationStream { // Insert synthetic responses directly after each orphaned request. // Iterate in reverse so earlier indices remain valid. for (pos, id, timestamp) in orphans.into_iter().rev() { - let response = InternalEvent::Event(Box::new(ConversationEvent::new( - ToolCallResponse { - id, - result: Err("Tool call was interrupted.".to_string()), - }, - timestamp, - ))); - self.events.insert(pos + 1, response); + self.insert( + pos + 1, + EventPayload::Event(Box::new(ConversationEvent::new( + ToolCallResponse { + id, + result: Err("Tool call was interrupted.".to_string()), + }, + timestamp, + ))), + ); } } @@ -1441,8 +1218,8 @@ impl ConversationStream { IterTurns::new(self.iter()) } - /// Returns each event paired with the 0-based index of the turn it belongs - /// to. + /// Iterate over the conversation events, each tagged with its 0-based turn + /// index and entry ID. /// /// Turn boundaries match [`Self::iter_turns`]: a [`TurnStart`] opens a new /// turn, and events before the first `TurnStart` form an implicit leading @@ -1455,12 +1232,12 @@ impl ConversationStream { /// Prefer it whenever only event content is needed. /// /// [`TurnStart`]: crate::event::TurnStart - pub fn iter_events_by_turn(&self) -> impl Iterator { + pub fn iter_events_by_turn(&self) -> impl Iterator> { let mut turn = 0; let mut seen_event = false; self.events.iter().filter_map(move |internal| { - let InternalEvent::Event(event) = internal else { + let EventPayload::Event(event) = &internal.payload else { return None; }; @@ -1471,7 +1248,11 @@ impl ConversationStream { } seen_event = true; - Some((turn, &**event)) + Some(EventInTurn { + turn, + event_id: &internal.event_id, + event, + }) }) } @@ -1486,7 +1267,7 @@ impl ConversationStream { pub fn turn_count(&self) -> usize { self.iter_events_by_turn() .last() - .map_or(0, |(turn, _)| turn + 1) + .map_or(0, |event| event.turn + 1) } /// Returns the turn that was active at the given time. @@ -1529,7 +1310,7 @@ impl ConversationStream { let turn_count = self .events .iter() - .filter(|e| matches!(e, InternalEvent::Event(ev) if ev.is_turn_start())) + .filter(|e| matches!(&e.payload, EventPayload::Event(ev) if ev.is_turn_start())) .count(); if turn_count <= n { @@ -1595,8 +1376,8 @@ impl ConversationStream { if let Some(pos) = self .events .iter() - .rposition(|e| matches!(e, InternalEvent::Event(_))) - && let InternalEvent::Event(ref event) = self.events[pos] + .rposition(|e| matches!(&e.payload, EventPayload::Event(_))) + && let EventPayload::Event(event) = &self.events[pos].payload && event.is_turn_start() { self.events.remove(pos); @@ -1637,367 +1418,26 @@ impl ConversationStream { Self { base_config: AppConfig::new_test().into(), events: vec![], + event_ids: EventIds::default(), + duplicated_event_ids: HashSet::new(), created_at: Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), } } } -impl Extend for ConversationStream { - fn extend>(&mut self, iter: T) { - // Cache the running tail config across iterations. Without this, every - // push falls through `push_with_config_delta` → `self.last()`, which - // walks the whole stream and deep-clones `PartialAppConfig` on each - // step — making `extend(n)` O(n²) in clones. - let mut tail = self - .last() - .map_or_else(|| self.base_config.to_partial(), |v| v.config); - - for v in iter { - let ConversationEventWithConfig { event, config } = v; - let config_delta = tail.delta(config.clone()); - - if !config_delta.is_empty() { - self.add_config_delta(ApplyDelta::new(event.timestamp, config_delta)); - } - - tail = config; - self.push(event); - } - } -} - -impl Extend for ConversationStream { - fn extend>(&mut self, iter: T) { - for v in iter { - self.push(v); - } - } -} - -impl IntoIterator for ConversationStream { - type Item = ConversationEventWithConfig; - - type IntoIter = IntoIter; - - fn into_iter(self) -> Self::IntoIter { - IntoIter { - current_config: self.base_config.to_partial(), - inner_iter: self.events.into_iter(), - } - } -} - -/// An owned iterator over the events in a conversation stream. -pub struct IntoIter { - /// The configuration state for the next event in the iterator. - current_config: PartialAppConfig, - - /// The iterator over the events in the stream. - inner_iter: std::vec::IntoIter, -} - -impl Iterator for IntoIter { - type Item = ConversationEventWithConfig; - - fn next(&mut self) -> Option { - loop { - let event = self.inner_iter.next()?; - - match event { - InternalEvent::ConfigDelta(delta) => { - if let Err(error) = fold_config_delta(&mut self.current_config, delta) { - error!(%error, "Failed to merge config delta."); - } - } - InternalEvent::Event(event) => { - return Some(ConversationEventWithConfig { - event: *event, - config: self.current_config.clone(), - }); - } - InternalEvent::Compaction(_) - | InternalEvent::Overlay(_) - | InternalEvent::Unknown(_) => {} - } - } - } -} - -impl DoubleEndedIterator for IntoIter { - fn next_back(&mut self) -> Option { - loop { - let event = self.inner_iter.next_back()?; - - match event { - InternalEvent::ConfigDelta(_) - | InternalEvent::Compaction(_) - | InternalEvent::Overlay(_) - | InternalEvent::Unknown(_) => { - // A delta/compaction at the very end of the list affects - // nothing that follows it, and it doesn't affect previous - // items. We simply discard it. - // event at the tail likewise yields no ConversationEvent. - } - InternalEvent::Event(event) => { - // Start with the state currently at the front of the line - let mut config = self.current_config.clone(); - - // Scan the remaining items in the middle (without consuming - // them) to apply all pending deltas to our temporary - // config. - for internal_event in self.inner_iter.as_slice() { - if let InternalEvent::ConfigDelta(delta) = internal_event - && let Err(error) = fold_config_delta(&mut config, delta.clone()) - { - error!(%error, "Failed to merge config delta."); - } - } - - return Some(ConversationEventWithConfig { - event: *event, - config, - }); - } - } - } - } -} - -/// An iterator over the borrowed events in a conversation stream. -struct Iter<'a> { - /// The stream being iterated over. - stream: &'a ConversationStream, - - /// The configuration state for the first, next event in the iterator. - front_config: PartialAppConfig, - - /// The index of the `next` event in the iterator. - front: usize, - - /// The index of the `next_back` event in the iterator. - back: usize, -} - -impl<'a> Iterator for Iter<'a> { - type Item = ConversationEventWithConfigRef<'a>; - - fn next(&mut self) -> Option { - while self.front < self.back { - let event = &self.stream.events[self.front]; - self.front += 1; - - match event { - InternalEvent::ConfigDelta(delta) => { - if let Err(error) = fold_config_delta(&mut self.front_config, delta.clone()) { - error!(%error, "Failed to merge config delta."); - } - } - InternalEvent::Event(event) => { - return Some(ConversationEventWithConfigRef { - event, - config: self.front_config.clone(), - }); - } - InternalEvent::Compaction(_) - | InternalEvent::Overlay(_) - | InternalEvent::Unknown(_) => {} - } - } - - None - } -} - -impl DoubleEndedIterator for Iter<'_> { - fn next_back(&mut self) -> Option { - while self.back > self.front { - self.back -= 1; - let event = &self.stream.events[self.back]; - - let InternalEvent::Event(event) = event else { - continue; - }; - - let mut config = self.stream.base_config.to_partial(); - for internal_event in &self.stream.events[..self.back] { - if let InternalEvent::ConfigDelta(delta) = internal_event - && let Err(error) = fold_config_delta(&mut config, delta.clone()) - { - error!(%error, "Failed to merge config delta."); - } - } - - return Some(ConversationEventWithConfigRef { event, config }); - } - - None - } -} - -/// An iterator over the mutable events in a conversation stream. -pub struct IterMut<'a> { - /// The configuration state for the first, next event in the iterator. - front_config: PartialAppConfig, - - /// The iterator over the events in the stream. - iter: std::slice::IterMut<'a, InternalEvent>, -} - -impl<'a> Iterator for IterMut<'a> { - type Item = ConversationEventWithConfigMut<'a>; - - fn next(&mut self) -> Option { - for event in self.iter.by_ref() { - match event { - InternalEvent::ConfigDelta(delta) => { - if let Err(error) = fold_config_delta(&mut self.front_config, delta.clone()) { - error!(%error, "Failed to merge config delta."); - } - } - InternalEvent::Event(event) => { - return Some(ConversationEventWithConfigMut { - event, - config: self.front_config.clone(), - }); - } - InternalEvent::Compaction(_) - | InternalEvent::Overlay(_) - | InternalEvent::Unknown(_) => {} - } - } - - None - } -} - -/// A reference to a [`ConversationEvent`] with its configuration. -#[derive(Debug, PartialEq, Clone)] -pub struct ConversationEventWithConfigRef<'a> { - /// The event. - pub event: &'a ConversationEvent, - - /// The configuration. - pub config: PartialAppConfig, -} - -/// A mutable reference to a [`ConversationEvent`] with its configuration. -#[derive(Debug, PartialEq)] -pub struct ConversationEventWithConfigMut<'a> { - /// The event. - pub event: &'a mut ConversationEvent, - - /// The configuration. - pub config: PartialAppConfig, -} - -/// A [`ConversationEvent`] with its configuration. -#[derive(Debug, PartialEq, Clone)] -pub struct ConversationEventWithConfig { - /// The event. - pub event: ConversationEvent, - - /// The configuration at the time the event was added. - /// - /// It should be noted that this is not necessarily the same as the current - /// active configuration of the application, even if this is the latest - /// event in the stream. - /// For one, the event may have been added a while ago, but more - /// importantly, not all configuration changes are automatically applied to - /// a [`ConversationStream`]. - /// For example, if a new tool is added in the configuration, it will not - /// become available in the conversation stream until explicitly added using - /// the CLI flag `--tool` or `--cfg`, while *NEW* conversations *WILL* get - /// the new tool by default. - pub config: PartialAppConfig, -} - -impl ConversationEventWithConfig { - /// Consume the type and return the underlying [`ConversationEvent`]. - #[must_use] - pub fn into_inner(self) -> ConversationEvent { - self.event - } - - /// Consume the type and return the underlying [`EventKind`]. - #[must_use] - pub fn into_kind(self) -> EventKind { - self.event.kind - } - - /// Return a reference to the underlying [`EventKind`]. - #[must_use] - pub const fn kind(&self) -> &EventKind { - &self.event.kind - } -} - -impl From> for ConversationEventWithConfig { - fn from(value: ConversationEventWithConfigRef<'_>) -> Self { - Self { - event: value.event.clone(), - config: value.config, - } - } -} - -impl FromIterator for Result { - fn from_iter>(iter: T) -> Self { - let mut events = iter.into_iter(); - - let Some((config, first_event)) = events.next().map(|e| (e.config, e.event)) else { - return Err(StreamError::FromEmptyIterator); - }; - - let mut stream = ConversationStream::new(jp_config::util::build(config)?.into()); - stream.push(first_event); - stream.extend(events); - - Ok(stream) - } -} - -impl std::ops::Deref for ConversationEventWithConfig { - type Target = ConversationEvent; - - fn deref(&self) -> &Self::Target { - &self.event - } -} - -impl std::ops::Deref for ConversationEventWithConfigRef<'_> { - type Target = ConversationEvent; - - fn deref(&self) -> &Self::Target { - self.event - } -} - -impl std::ops::Deref for ConversationEventWithConfigMut<'_> { - type Target = ConversationEvent; - - fn deref(&self) -> &Self::Target { - self.event - } -} - -impl std::ops::DerefMut for ConversationEventWithConfigMut<'_> { - fn deref_mut(&mut self) -> &mut Self::Target { - self.event - } -} - -impl From for ConversationEventWithConfig { - fn from(event: ConversationEvent) -> Self { - Self { - event, - config: PartialAppConfig::empty(), - } - } -} - +/// The storage boundary: turning a stream into stored JSON and back. +/// +/// This is where a file's shortcomings are dealt with — a legacy layout, an +/// entry with no ID, two entries sharing one. +/// A stream reaching the rest of JP has none of them, so everything above can +/// assume its invariants instead of checking them. impl ConversationStream { /// Construct a stream from a base config and serialized events. /// + /// Duplicate entry IDs are repaired on later occurrences, with a warning + /// for each replacement. + /// Payloads and timestamps are preserved. + /// /// The storage layer reads `base_config.json` as a raw JSON [`Value`] and /// `events.json` as raw JSON values. /// All deserialization, including schema-aware stripping of unknown fields @@ -2025,18 +1465,74 @@ impl ConversationStream { ) -> Result { let base_config = crate::compat::deserialize_partial_config(base_config); - let events = events + let stored = events .into_iter() - .map(|v| serde_json::from_value(v).map_err(StreamError::Json)) + .map(|v| serde_json::from_value::(v).map_err(StreamError::Json)) .collect::, _>>()?; + let mut event_ids = EventIds::default(); + // Every ID the file carries is reserved before any entry is settled, so + // a generated replacement cannot take one belonging to an entry further + // down the file. + event_ids.reserve(stored.iter().filter_map(|e| e.event_id.clone())); + + let mut seen = HashSet::with_capacity(stored.len()); + let mut duplicated_event_ids = HashSet::new(); + let events = stored + .into_iter() + .map(|StoredEvent { event_id, payload }| { + let event_id = match event_id { + // A legacy entry has no identity yet; give it one. + None => event_ids.fresh(), + // The first entry to carry an ID keeps it. + Some(id) if seen.insert(id.clone()) => id, + // A later one cannot, so it is reassigned and the shared + // value recorded: repair restores uniqueness, but it cannot + // say which entry a reference to that value meant. + Some(id) => { + let replacement = event_ids.fresh(); + warn!( + event_id = %id, + replacement_event_id = %replacement, + "Regenerated duplicate conversation event ID.", + ); + duplicated_event_ids.insert(id); + replacement + } + }; + + InternalEvent { event_id, payload } + }) + .collect(); + Ok(Self { base_config: finalize_recovered_config(base_config, fallback)?, events, + event_ids, + duplicated_event_ids, created_at: Utc::now(), }) } + /// The IDs this load found on more than one entry. + /// + /// Repair kept the first entry carrying such an ID and reassigned the rest, + /// which restores uniqueness but cannot say which entry a pre-existing + /// reference to the shared ID meant. + /// A feature that resolves references treats a reference to one of these as + /// unresolved, and must do so within this load cycle: once the repaired + /// stream is saved the file holds unique IDs, and a later load reports + /// nothing here. + /// + /// Scoped to this stream's own load. + /// Entries taken from another stream, through [`Self::append_stream`] or + /// [`Extend`], arrive without that stream's record, so a consumer resolving + /// a reference against a copy asks the stream that read the file. + #[must_use] + pub const fn duplicated_event_ids(&self) -> &HashSet { + &self.duplicated_event_ids + } + /// Decompose the stream into its storable parts. /// /// Returns the base config and the serialized events array as raw JSON. @@ -2093,7 +1589,7 @@ impl ConversationStream { } // Extract the config subtree as the base config value. - let base_config = config_delta_subtree(&events[0]); + let base_config = config_delta::subtree(&events[0]); // Remaining elements are events. from_parts handles compat stripping. let events = events.into_iter().skip(1).collect(); @@ -2207,72 +1703,14 @@ pub enum StreamError { }, } -// A custom deserializer for `InternalEvent` that avoids serde allocations when -// trying to match `untagged` enum variants. -// -// Deserializes to a JSON `Value` first, then dispatches on the `type` tag. This -// avoids the allocation overhead serde incurs when trying each variant of an -// untagged enum. Base64-encoded storage fields are decoded before the final -// deserialization into typed events. -// -// `cargo dhat` had shown the untagged approach to be a hotspot. -impl<'de> Deserialize<'de> for InternalEvent { - fn deserialize(deserializer: D) -> Result - where - D: serde::Deserializer<'de>, - { - let mut value = Value::deserialize(deserializer)?; - - let tag = value - .get("type") - .and_then(Value::as_str) - .unwrap_or_default(); - - if tag == "config_delta" { - return deserialize_config_delta(&value) - .map(Self::ConfigDelta) - .map_err(serde::de::Error::custom); - } - - if tag == "compaction" { - return serde_json::from_value(value) - .map(Self::Compaction) - .map_err(serde::de::Error::custom); - } - - if tag == "event_overlay" { - return serde_json::from_value(value) - .map(Self::Overlay) - .map_err(serde::de::Error::custom); - } - - // Conversations are an append-only log a newer `jp` may have written. - // An unrecognized event kind is preserved verbatim instead of failing - // the whole stream load, so it round-trips on the next save. Corrupt - // *known* events still fail loudly below. - if !EventKind::TYPE_TAGS.contains(&tag) { - #[cfg(debug_assertions)] - { - let mut probe = value.clone(); - decode_event_value(&mut probe); - debug_assert!( - serde_json::from_value::(probe).is_err(), - "event tag `{tag}` is missing from EventKind::TYPE_TAGS", - ); - } - warn!(%tag, "Unknown conversation event kind; preserving raw event."); - return Ok(Self::Unknown(value)); - } - - // Decode base64-encoded storage fields before deserializing. - decode_event_value(&mut value); - - serde_json::from_value(value) - .map(|e| Self::Event(Box::new(e))) - .map_err(serde::de::Error::custom) - } -} - #[cfg(test)] #[path = "stream_tests.rs"] mod tests; + +#[cfg(test)] +#[path = "stream/event_id_tests.rs"] +mod event_id_tests; + +#[cfg(test)] +#[path = "stream/event_id_repair_tests.rs"] +mod event_id_repair_tests; diff --git a/crates/jp_conversation/src/stream/config_delta.rs b/crates/jp_conversation/src/stream/config_delta.rs new file mode 100644 index 000000000..438185ce7 --- /dev/null +++ b/crates/jp_conversation/src/stream/config_delta.rs @@ -0,0 +1,265 @@ +//! Config deltas: the stream entries that change a conversation's +//! configuration. +//! +//! A conversation's config is not a snapshot but a base plus an ordered series +//! of changes, each recorded as a stream entry. +//! [`ConfigDelta`] is one such change, and [`fold`] is what applies it: +//! resolving a conversation's config means folding every delta in the stream +//! onto the base, in order. +//! +//! The on-disk shape is older than the type, so both serialization directions +//! are hand-rolled. +//! [`deserialize`] reads every shape ever written, including entries that +//! predate the [`Reset`] variant and entries whose config fields sat directly +//! in the envelope. +//! +//! [`Reset`]: ConfigDelta::Reset + +use chrono::{DateTime, Utc}; +use jp_config::{ConfigError, PartialAppConfig, PartialConfig as _}; +use serde::{Serialize, Serializer}; +use serde_json::Value; +use tracing::warn; + +use crate::compat::deserialize_partial_config; + +/// A configuration delta. +#[derive(Debug, Clone, PartialEq)] +pub enum ConfigDelta { + /// Merge a partial configuration on top of the accumulated config state. + Apply(ApplyDelta), + + /// Discard the accumulated config state. + /// + /// Config resolution restarts from program defaults; subsequent [`Apply`] + /// events layer on top. + /// + /// [`Apply`]: Self::Apply + Reset(ResetDelta), +} + +impl ConfigDelta { + /// The timestamp of the event, regardless of variant. + #[must_use] + pub const fn timestamp(&self) -> DateTime { + match self { + Self::Apply(delta) => delta.timestamp, + Self::Reset(delta) => delta.timestamp, + } + } +} + +// Hand-rolled so `Apply` keeps the legacy flat shape (no `op` field) and +// `Reset` carries `"op": "reset"`. The variant discriminator must live inside +// the event body: the outer stream entry envelope already claims the top-level +// `type` key. +impl Serialize for ConfigDelta { + fn serialize(&self, serializer: S) -> Result { + match self { + Self::Apply(delta) => delta.serialize(serializer), + Self::Reset(delta) => { + #[derive(Serialize)] + struct Tagged<'a> { + op: &'static str, + #[serde(flatten)] + inner: &'a ResetDelta, + } + + Tagged { + op: "reset", + inner: delta, + } + .serialize(serializer) + } + } + } +} + +/// A configuration delta that merges on top of the accumulated config state. +#[derive(Debug, Clone, Serialize, PartialEq)] +pub struct ApplyDelta { + /// The timestamp of the event. + #[serde(serialize_with = "crate::serialize_dt")] + pub timestamp: DateTime, + + /// The configuration delta. + pub delta: Box, + + /// Dotted paths of fields cleared before [`delta`] is merged. + /// + /// Merging is per field, so a field that merges by appending cannot reach a + /// value that drops one of its elements: whatever the delta carries is + /// added to what is already there. + /// Clearing the field first leaves the merge nothing to combine with, and + /// the delta's value lands whole. + /// + /// A path that names no field is ignored, so a delta written by a newer + /// version, or naming a field since removed, still replays. + /// + /// [`delta`]: Self::delta + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub unsets: Vec, +} + +impl ApplyDelta { + /// An apply that merges `delta` and clears nothing. + #[must_use] + pub fn new(timestamp: DateTime, delta: impl Into>) -> Self { + Self { + timestamp, + delta: delta.into(), + unsets: Vec::new(), + } + } + + /// An apply that clears `unsets` before merging `delta`. + #[must_use] + pub fn with_unsets( + timestamp: DateTime, + delta: impl Into>, + unsets: Vec, + ) -> Self { + Self { + timestamp, + delta: delta.into(), + unsets, + } + } +} + +/// A configuration delta that discards the accumulated config state, resetting +/// it to program defaults. +#[derive(Debug, Clone, Serialize, PartialEq, Eq)] +pub struct ResetDelta { + /// The timestamp of the event. + #[serde(serialize_with = "crate::serialize_dt")] + pub timestamp: DateTime, +} + +impl From for ConfigDelta { + fn from(delta: ApplyDelta) -> Self { + Self::Apply(delta) + } +} + +impl From for ConfigDelta { + fn from(delta: ResetDelta) -> Self { + Self::Reset(delta) + } +} + +impl From for ConfigDelta { + fn from(config: PartialAppConfig) -> Self { + Self::Apply(ApplyDelta::new(Utc::now(), config)) + } +} + +/// Extract the stored config subtree from a `config_delta` event. +/// +/// Two on-disk shapes exist. +/// Newer streams nest the config under a `delta` key; older ones carry the +/// config fields as siblings of the envelope keys (`type`, `timestamp`, `op`), +/// which are removed here so only config fields remain. +/// +/// The value is the entry as stored, so `event_id` may still be among those +/// siblings and is removed with them. +/// The stream-entry deserializer lifts that key out before a payload is read, +/// but this is also reached with a raw entry it never saw: the first element of +/// a legacy file, which [`ConversationStream::from_legacy_events`] reads as the +/// base config rather than as an entry. +/// +/// [`ConversationStream::from_legacy_events`]: crate::ConversationStream::from_legacy_events +pub(super) fn subtree(value: &Value) -> Value { + if let Some(delta) = value.get("delta") { + return delta.clone(); + } + + let mut obj = value.as_object().cloned().unwrap_or_default(); + obj.remove("type"); + obj.remove("timestamp"); + obj.remove("op"); + obj.remove("unsets"); + obj.remove("event_id"); + Value::Object(obj) +} + +/// Deserialize a [`ConfigDelta`] from a raw JSON value, tolerating schema +/// changes within the stored config. +/// +/// The `op` field selects the variant: absent (which covers every event written +/// before the reset variant existed) or `"apply"` decodes as +/// [`ConfigDelta::Apply`]; `"reset"` decodes as [`ConfigDelta::Reset`]. +/// Delegates to [`deserialize_partial_config`] for the config subtree and +/// extracts the timestamp separately. +/// +/// # Errors +/// +/// Returns an error for any other `op` value: an op added by a newer version +/// must fail loudly here instead of being misread as an apply and corrupting +/// config resolution. +pub(crate) fn deserialize(value: &Value) -> Result { + let timestamp = value + .get("timestamp") + .and_then(Value::as_str) + .and_then(|s| crate::parse_dt(s).ok()) + .unwrap_or_else(Utc::now); + + if let Some(op) = value.get("op") { + if op == "reset" { + return Ok(ConfigDelta::Reset(ResetDelta { timestamp })); + } + + if op != "apply" { + return Err(format!("unknown config delta `op`: {op}")); + } + } + + // The hand-rolled deserializer bypasses the derived one, so the field's + // `#[serde(default)]` never runs and the key has to be read here. + let unsets = value + .get("unsets") + .and_then(Value::as_array) + .map(|paths| { + paths + .iter() + .filter_map(|path| path.as_str().map(str::to_owned)) + .collect() + }) + .unwrap_or_default(); + + let delta = deserialize_partial_config(subtree(value)); + + Ok(ConfigDelta::Apply(ApplyDelta { + timestamp, + delta: Box::new(delta), + unsets, + })) +} + +/// Fold a single [`ConfigDelta`] into an accumulated partial config state. +/// +/// [`Apply`] merges the delta on top of `state`. +/// [`Reset`] discards `state`, restarting from the empty partial +/// (`PartialAppConfig::default()`); program defaults are injected when the +/// partial is finalized into an [`AppConfig`]. +/// +/// [`AppConfig`]: jp_config::AppConfig +/// [`Apply`]: ConfigDelta::Apply +/// [`Reset`]: ConfigDelta::Reset +pub(super) fn fold(state: &mut PartialAppConfig, delta: ConfigDelta) -> Result<(), ConfigError> { + match delta { + ConfigDelta::Apply(apply) => { + for path in &apply.unsets { + if let Err(error) = state.unset(path) { + warn!(%path, %error, "Ignoring a config delta unset for an unknown field."); + } + } + + state.merge(&(), *apply.delta) + } + ConfigDelta::Reset(_) => { + *state = PartialAppConfig::default(); + Ok(()) + } + } +} diff --git a/crates/jp_conversation/src/stream/entry.rs b/crates/jp_conversation/src/stream/entry.rs new file mode 100644 index 000000000..e8a695bab --- /dev/null +++ b/crates/jp_conversation/src/stream/entry.rs @@ -0,0 +1,318 @@ +//! What one entry in a conversation stream is, and how it is stored. +//! +//! An entry is an identity plus a payload. +//! [`InternalEvent`] pairs the two and is what a stream holds; [`EventPayload`] +//! is the payload alone, and is where the on-disk `type` tag and the base64 +//! encoding of content fields live. +//! +//! Reading an entry back goes through [`StoredEvent`], whose identity is +//! optional: a file can carry an entry with no ID, or two entries with the same +//! one, and only a stream can settle that. +//! `ConversationStream::from_parts` is what turns a `StoredEvent` into an +//! `InternalEvent`, which is why "unique within its stream" holds of every +//! entry a stream holds. +//! +//! Both serde directions are hand-rolled. +//! Serialization writes `event_id` alongside a flattened payload, so an entry +//! is one JSON object. +//! Deserialization dispatches on the `type` tag rather than trying each variant +//! in turn: `cargo dhat` showed the untagged approach allocating heavily on +//! stream loads. + +use serde::{Deserialize, Deserializer, Serialize, Serializer, de::Error as _}; +use serde_json::{Value, from_value}; +use tracing::warn; + +use super::config_delta::{self, ConfigDelta}; +use crate::{ + Compaction, EventId, EventOverlay, + event::{ConversationEvent, EventKind}, + storage::{decode_event_value, encode_event}, +}; + +/// A stream entry with stream-assigned identity and a flattened storage +/// payload. +/// +/// An `InternalEvent` only exists inside a stream, and its `event_id` is one +/// that stream handed out. +/// Reading one from storage goes through [`StoredEvent`], whose ID is optional +/// until the stream settles it. +#[derive(Debug, Clone, PartialEq, Serialize)] +pub(super) struct InternalEvent { + /// Identity within the raw conversation stream. + pub(super) event_id: EventId, + /// Stored content, including the producer's timestamp. + #[serde(flatten)] + pub(super) payload: EventPayload, +} + +/// Stored payload with type tagging and base64 encoding for content fields. +#[derive(Debug, Clone, PartialEq)] +pub(super) enum EventPayload { + /// The configuration state of the conversation is updated. + /// + /// When this event is emitted, all subsequent events in the stream are + /// bound to the new configuration. + /// + /// An [`Apply`] delta is merged on top of all previous `ConfigDelta` events + /// in the stream; a [`Reset`] discards the accumulated state, restarting + /// from program defaults. + /// + /// Any non-config events before the first `ConfigDelta` event are + /// considered to have the default configuration. + /// + /// [`Apply`]: ConfigDelta::Apply + /// [`Reset`]: ConfigDelta::Reset + ConfigDelta(ConfigDelta), + /// An event in the conversation stream. + Event(Box), + /// A compaction overlay that modifies how preceding events are projected + /// when building the LLM request. + /// Does not modify or delete any existing events. + Compaction(Compaction), + /// A patch overlay that rewrites how matched events are projected when + /// building the LLM request. + /// Does not modify or delete any existing events. + Overlay(EventOverlay), + /// An event whose `type` tag this build does not recognize. + /// + /// Conversations are an append-only log that a newer `jp` may have written. + /// Rather than fail the entire stream load on an unknown event kind, the + /// raw JSON is retained verbatim so it round-trips losslessly on the next + /// save. + /// Unknown events are invisible to event iteration, config resolution, and + /// providers. + Unknown(Value), +} + +impl Serialize for EventPayload { + fn serialize(&self, serializer: S) -> Result { + match self { + Self::ConfigDelta(delta) => { + #[derive(Serialize)] + struct Tagged<'a> { + #[serde(rename = "type")] + tag: &'static str, + #[serde(flatten)] + inner: &'a ConfigDelta, + } + + Tagged { + tag: "config_delta", + inner: delta, + } + .serialize(serializer) + } + Self::Event(event) => { + let mut value = + serde_json::to_value(event.as_ref()).map_err(serde::ser::Error::custom)?; + + // Base64-encode storage fields. + encode_event(&mut value, &event.kind); + value.serialize(serializer) + } + Self::Compaction(compaction) => { + #[derive(Serialize)] + struct Tagged<'a> { + #[serde(rename = "type")] + tag: &'static str, + #[serde(flatten)] + inner: &'a Compaction, + } + + Tagged { + tag: "compaction", + inner: compaction, + } + .serialize(serializer) + } + Self::Overlay(overlay) => { + #[derive(Serialize)] + struct Tagged<'a> { + #[serde(rename = "type")] + tag: &'static str, + #[serde(flatten)] + inner: &'a EventOverlay, + } + + Tagged { + tag: "event_overlay", + inner: overlay, + } + .serialize(serializer) + } + Self::Unknown(value) => value.serialize(serializer), + } + } +} + +// Dispatching on `type` avoids the allocations from trying each untagged +// variant. Base64 content is decoded only for known conversation events. +impl<'de> Deserialize<'de> for EventPayload { + fn deserialize>(deserializer: D) -> Result { + let mut value = Value::deserialize(deserializer)?; + + let tag = value + .get("type") + .and_then(Value::as_str) + .unwrap_or_default(); + + if tag == "config_delta" { + return config_delta::deserialize(&value) + .map(Self::ConfigDelta) + .map_err(D::Error::custom); + } + + if tag == "compaction" { + return serde_json::from_value(value) + .map(Self::Compaction) + .map_err(D::Error::custom); + } + + if tag == "event_overlay" { + return serde_json::from_value(value) + .map(Self::Overlay) + .map_err(D::Error::custom); + } + + // Conversations are an append-only log a newer `jp` may have written. + // An unrecognized event kind is preserved verbatim instead of failing + // the whole stream load, so it round-trips on the next save. Corrupt + // *known* events still fail loudly below. + if !EventKind::TYPE_TAGS.contains(&tag) { + #[cfg(debug_assertions)] + { + let mut probe = value.clone(); + decode_event_value(&mut probe); + debug_assert!( + serde_json::from_value::(probe).is_err(), + "event tag `{tag}` is missing from EventKind::TYPE_TAGS", + ); + } + warn!(%tag, "Unknown conversation event kind; preserving raw event."); + return Ok(Self::Unknown(value)); + } + + // Decode base64-encoded storage fields before deserializing. + decode_event_value(&mut value); + + serde_json::from_value(value) + .map(|e| Self::Event(Box::new(e))) + .map_err(D::Error::custom) + } +} + +/// Whether an [`InternalEvent`] belongs to a single turn or applies to the +/// conversation as a whole. +/// +/// This is the single source of truth for which events survive turn-level +/// pruning (`pop`, `trim_chat_request`, `pop_if`, `retain`). +/// Adding a new `EventPayload` variant forces a classification here: +/// [`InternalEvent::scope`] is an exhaustive match, so no pruning caller can +/// silently mistreat it. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum EventScope { + /// Survives turn pruning: config deltas and compaction overlays apply to + /// the conversation regardless of position. + Global, + /// Belongs to a turn and is removed when that turn is pruned. + Turn, +} + +impl InternalEvent { + /// Consume the entry, returning its conversation event if it has one. + #[must_use] + pub(super) fn into_event(self) -> Option { + match self.payload { + EventPayload::Event(event) => Some(*event), + EventPayload::ConfigDelta(_) + | EventPayload::Compaction(_) + | EventPayload::Overlay(_) + | EventPayload::Unknown(_) => None, + } + } + + /// Get a reference to [`EventPayload::Event`], if applicable. + #[must_use] + pub(super) fn as_event(&self) -> Option<&ConversationEvent> { + match &self.payload { + EventPayload::Event(event) => Some(event), + EventPayload::ConfigDelta(_) + | EventPayload::Compaction(_) + | EventPayload::Overlay(_) + | EventPayload::Unknown(_) => None, + } + } + + /// Classify the event as turn-scoped or global. + /// See [`EventScope`]. + #[must_use] + pub(super) const fn scope(&self) -> EventScope { + match &self.payload { + EventPayload::ConfigDelta(_) + | EventPayload::Compaction(_) + | EventPayload::Overlay(_) + | EventPayload::Unknown(_) => EventScope::Global, + EventPayload::Event(_) => EventScope::Turn, + } + } +} + +/// A stream entry as storage holds it, before a stream settles its identity. +/// +/// `event_id` is `Option` because identity belongs to the stream, not to the +/// file: a legacy entry has none, and a hand-edited file can give two entries +/// the same one. +/// Only `ConversationStream::from_parts` turns this into an [`InternalEvent`], +/// which is what keeps "unique within its stream" true of every entry a stream +/// holds rather than of every entry that happens to have been read. +#[derive(Debug)] +pub(super) struct StoredEvent { + /// The ID the file carried, if any. + pub(super) event_id: Option, + + /// The entry's content. + pub(super) payload: EventPayload, +} + +impl StoredEvent { + /// Settle this entry's identity on its own, outside a stream. + /// + /// A test asserting on one entry's storage round-trip has no stream to take + /// an ID from, and reading a single entry cannot check uniqueness against + /// entries it never saw. + /// Loading a stream goes through `ConversationStream::from_parts`, which + /// settles every entry's ID together. + #[cfg(test)] + pub(super) fn into_entry(self) -> InternalEvent { + InternalEvent { + event_id: self.event_id.unwrap_or_else(EventId::random), + payload: self.payload, + } + } +} + +// `event_id` is lifted out of the stored object before the payload is read, so +// an `Unknown` entry retains its raw JSON without a second copy of the key and +// writes exactly one back out. `shift_remove` keeps the remaining keys in their +// stored order, so a hand-edited entry round-trips unreordered. +// +// A missing, null, or empty `event_id` reads as absent, which is leniency the +// storage boundary owes a legacy file; `EventId` itself stays strict. Any other +// non-string `event_id` is a corrupt entry rather than an absent one, and fails +// loudly. +impl<'de> Deserialize<'de> for StoredEvent { + fn deserialize>(deserializer: D) -> Result { + let mut value = Value::deserialize(deserializer)?; + let object = value + .as_object_mut() + .ok_or_else(|| D::Error::custom("stream entry must be a JSON object"))?; + let event_id = match object.shift_remove("event_id") { + None | Some(Value::Null) => None, + Some(Value::String(id)) if id.is_empty() => None, + Some(id) => Some(from_value(id).map_err(D::Error::custom)?), + }; + let payload = from_value(value).map_err(D::Error::custom)?; + Ok(Self { event_id, payload }) + } +} diff --git a/crates/jp_conversation/src/stream/event_id_repair_tests.rs b/crates/jp_conversation/src/stream/event_id_repair_tests.rs new file mode 100644 index 000000000..607fcd2a0 --- /dev/null +++ b/crates/jp_conversation/src/stream/event_id_repair_tests.rs @@ -0,0 +1,322 @@ +//! Load-time repair of entry IDs: missing, empty, and duplicate values. + +use std::collections::HashSet; + +use jp_config::PartialAppConfig; +use serde_json::{Value, json}; + +use super::ConversationStream; +use crate::EventId; + +/// An entry a build does not recognize, kept verbatim. +/// +/// Used for most fixtures here because repair is indifferent to payload kind, +/// and an unknown entry needs no valid body to be loadable. +fn opaque(id: &str) -> Value { + json!({"event_id": id, "type": "future_event"}) +} + +/// Load raw entries the way the storage layer does, running repair. +fn load(events: Vec) -> ConversationStream { + let (base_config, _) = ConversationStream::new_test().to_parts().unwrap(); + ConversationStream::from_parts(base_config, events, &PartialAppConfig::empty()).unwrap() +} + +/// The IDs a loaded stream holds, in stream order. +fn ids(stream: &ConversationStream) -> Vec { + stream + .events + .iter() + .map(|event| event.event_id.to_string()) + .collect() +} + +#[test] +fn the_first_occurrence_of_a_duplicate_keeps_its_id() { + let stream = load(vec![opaque("shared"), opaque("shared")]); + + assert_eq!(ids(&stream)[0], "shared"); +} + +#[test] +fn a_later_occurrence_of_a_duplicate_is_reassigned() { + let stream = load(vec![opaque("shared"), opaque("shared")]); + + assert_ne!(ids(&stream)[1], "shared"); +} + +#[test] +fn a_duplicated_id_is_recorded_as_ambiguous() { + // The surviving entry keeps the ID, so the ambiguity is not "which entry + // exists" but "which entry a reference meant", which is why the value is + // reported rather than retired. + let stream = load(vec![opaque("shared"), opaque("shared")]); + + assert_eq!( + stream.duplicated_event_ids(), + &HashSet::from([EventId::fixed("shared")]) + ); +} + +#[test] +fn a_replacement_avoids_an_id_the_repair_pass_has_not_reached() { + // `shared` is duplicated before `taken` appears, so a pass that reserved + // IDs as it walked, rather than reading the whole file up front, could + // hand the replacement an ID a later entry already holds. + // + // Generated IDs make a real collision here vanishingly unlikely, so what + // this pins is that the reservation covers the later entry at all: + // `event_ids` is seeded from every loaded entry before repair runs, which + // `EventIds::fresh` then draws against. See + // `a_draw_retries_past_an_id_the_set_already_holds` for the deterministic + // half. + let stream = load(vec![opaque("shared"), opaque("shared"), opaque("taken")]); + let ids = ids(&stream); + + assert_eq!(ids[0], "shared"); + assert_eq!(ids[2], "taken"); + assert_ne!(ids[1], "shared"); + assert_ne!(ids[1], "taken"); +} + +#[test] +fn every_entry_holds_a_distinct_id_after_repair() { + let stream = load(vec![ + opaque("shared"), + opaque("shared"), + opaque("shared"), + opaque("shared"), + ]); + + let unique: std::collections::HashSet<_> = ids(&stream).into_iter().collect(); + assert_eq!(unique.len(), 4); +} + +#[test] +fn repair_reaches_every_payload_kind() { + // Repair works on the wrapper, so each payload kind must be reachable by + // it. Each entry below carries the same ID, so all but the first are + // reassigned. + let stream = load(vec![ + json!({"event_id": "shared", "type": "config_delta", "op": "reset", "timestamp": "1970-01-01 00:00:00.0"}), + json!({"event_id": "shared", "type": "compaction", "from_turn": 0, "to_turn": 0, "timestamp": "1970-01-01 00:00:00.0"}), + json!({"event_id": "shared", "type": "event_overlay", "patches": [], "timestamp": "1970-01-01 00:00:00.0"}), + json!({"event_id": "shared", "type": "future_event"}), + json!({"event_id": "shared", "type": "chat_request", "content": "question", "timestamp": "1970-01-01 00:00:00.0"}), + ]); + + let unique: std::collections::HashSet<_> = ids(&stream).into_iter().collect(); + assert_eq!(unique.len(), 5); +} + +#[test] +fn repair_leaves_payloads_and_timestamps_untouched() { + let original = vec![ + json!({"event_id": "shared", "type": "chat_request", "content": "question", "timestamp": "1970-01-01 00:00:00.0"}), + json!({"event_id": "shared", "type": "chat_response", "message": "answer", "timestamp": "1970-01-01 00:00:01.0"}), + ]; + let stream = load(original.clone()); + + let (_, mut saved) = stream.to_parts().unwrap(); + let mut expected = original; + for event in saved.iter_mut().chain(&mut expected) { + event.as_object_mut().unwrap().shift_remove("event_id"); + } + assert_eq!(saved, expected); +} + +#[test] +fn repair_does_not_sanitize_the_stream() { + // Repair runs before `sanitize`, so a stream that opens without a turn + // marker still has none afterwards. + let stream = load(vec![ + json!({"event_id": "shared", "type": "chat_request", "content": "question", "timestamp": "1970-01-01 00:00:00.0"}), + json!({"event_id": "shared", "type": "chat_response", "message": "answer", "timestamp": "1970-01-01 00:00:01.0"}), + ]); + + assert_eq!(stream.events.len(), 2); +} + +#[test] +fn an_entry_without_an_id_is_assigned_one() { + let stream = load(vec![json!({"type": "future_event"})]); + + assert!(!ids(&stream)[0].is_empty()); +} + +#[test] +fn an_entry_with_an_empty_id_is_assigned_one() { + let stream = load(vec![json!({"type": "future_event", "event_id": ""})]); + + assert!(!ids(&stream)[0].is_empty()); +} + +#[test] +fn assigned_ids_are_written_on_save() { + let stream = load(vec![json!({"type": "future_event"})]); + + let (_, saved) = stream.to_parts().unwrap(); + assert_eq!(saved[0]["event_id"], ids(&stream)[0].as_str()); +} + +#[test] +fn assigning_ids_to_a_legacy_file_reports_no_ambiguity() { + // An absent ID is unambiguous: no reference to it can exist yet. + let stream = load(vec![json!({"type": "future_event"})]); + + assert!(stream.duplicated_event_ids().is_empty()); +} + +#[test] +fn reloading_a_repaired_stream_reports_no_ambiguity() { + let stream = load(vec![opaque("shared"), opaque("shared")]); + let (base_config, saved) = stream.to_parts().unwrap(); + + let reloaded = + ConversationStream::from_parts(base_config, saved, &PartialAppConfig::empty()).unwrap(); + + assert!(reloaded.duplicated_event_ids().is_empty()); +} + +#[test] +fn a_repaired_stream_round_trips_unchanged() { + let stream = load(vec![opaque("shared"), opaque("shared")]); + let (base_config, saved) = stream.to_parts().unwrap(); + + let reloaded = + ConversationStream::from_parts(base_config, saved.clone(), &PartialAppConfig::empty()) + .unwrap(); + + assert_eq!(reloaded.to_parts().unwrap().1, saved); +} + +#[test] +fn sanitize_does_not_repair_ids() { + // Repair belongs to the load path: it reads the IDs a file carried, which + // is knowledge `sanitize` does not have. `sanitize` repairs stream + // structure, and a stream carrying duplicate IDs in memory keeps them. + let mut stream = load(vec![opaque("first"), opaque("second")]); + stream.events[1].event_id = EventId::fixed("first"); + + stream.sanitize(); + + assert_eq!(ids(&stream), ["first", "first"]); + assert!(stream.duplicated_event_ids().is_empty()); +} + +#[test] +fn ambiguity_is_reported_for_every_duplicated_id() { + let stream = load(vec![ + opaque("first"), + opaque("first"), + opaque("second"), + opaque("second"), + ]); + + assert_eq!( + stream.duplicated_event_ids(), + &HashSet::from([EventId::fixed("first"), EventId::fixed("second")]) + ); +} + +#[test] +fn a_hand_edited_id_survives_a_save_and_reload_verbatim() { + // A hand-edited file can hold anything non-empty, including characters JSON + // has to escape and an ID long enough to rule out silent truncation. None + // of it is normalized: what the file holds is what the stream reports and + // writes back. + for id in [ + r#"quote" backslash\ brace}"#, + "newline\nand\ttab", + "é☃", + "Hand-edited ID: 42!", + &"x".repeat(512), + ] { + let stream = load(vec![json!({"event_id": id, "type": "future_event"})]); + assert_eq!(ids(&stream), [id], "loading {id:?}"); + assert!(stream.duplicated_event_ids().is_empty(), "loading {id:?}"); + + let (base_config, saved) = stream.to_parts().unwrap(); + assert_eq!(saved[0]["event_id"], id, "saving {id:?}"); + + let reloaded = + ConversationStream::from_parts(base_config, saved, &PartialAppConfig::empty()).unwrap(); + assert_eq!(ids(&reloaded), [id], "reloading {id:?}"); + } +} + +#[test] +fn two_entries_sharing_a_hand_edited_id_are_repaired() { + // Duplicate detection compares the whole string, so an ID needing JSON + // escaping is matched on what it decodes to rather than on its stored form. + let id = r#"quote" backslash\"#; + let stream = load(vec![ + json!({"event_id": id, "type": "future_event"}), + json!({"event_id": id, "type": "future_event"}), + ]); + + assert_eq!(ids(&stream)[0], id); + assert_ne!(ids(&stream)[1], id); + assert_eq!( + stream.duplicated_event_ids(), + &HashSet::from([EventId::fixed(id)]) + ); +} + +#[test] +fn legacy_events_are_loaded_with_ids_assigned() { + let (base_config, _) = ConversationStream::new_test().to_parts().unwrap(); + let stream = ConversationStream::from_legacy_events( + vec![ + json!({"type": "config_delta", "delta": base_config, "timestamp": "1970-01-01 00:00:00.0"}), + json!({"type": "chat_request", "content": "question", "timestamp": "1970-01-01 00:00:00.0"}), + json!({"type": "future_event", "content": "opaque"}), + ], + &PartialAppConfig::empty(), + ) + .unwrap() + .unwrap(); + + // The first entry became the base config rather than a stream entry. + assert_eq!(stream.events.len(), 2); + assert!(ids(&stream).iter().all(|id| !id.is_empty())); + assert_ne!(ids(&stream)[0], ids(&stream)[1]); +} + +#[test] +fn legacy_events_are_repaired_for_duplicates() { + let (base_config, _) = ConversationStream::new_test().to_parts().unwrap(); + let stream = ConversationStream::from_legacy_events( + vec![ + json!({"type": "config_delta", "delta": base_config, "timestamp": "1970-01-01 00:00:00.0"}), + json!({"event_id": "shared", "type": "chat_request", "content": "question", "timestamp": "1970-01-01 00:00:00.0"}), + opaque("shared"), + ], + &PartialAppConfig::empty(), + ) + .unwrap() + .unwrap(); + + assert_eq!(ids(&stream)[0], "shared"); + assert_ne!(ids(&stream)[1], "shared"); + assert_eq!( + stream.duplicated_event_ids(), + &HashSet::from([EventId::fixed("shared")]) + ); +} + +#[test] +fn load_history_does_not_affect_stream_equality() { + // `duplicated_event_ids` and the handed-out ID set are load-scoped. Two + // streams holding the same entries are equal regardless of whether one of + // them got there through repair. + let repaired = load(vec![opaque("shared"), opaque("shared")]); + let (base_config, saved) = repaired.to_parts().unwrap(); + let clean = ConversationStream::from_parts(base_config, saved, &PartialAppConfig::empty()) + .unwrap() + .with_created_at(repaired.created_at); + + assert!(!repaired.duplicated_event_ids().is_empty()); + assert!(clean.duplicated_event_ids().is_empty()); + assert_eq!(repaired, clean); +} diff --git a/crates/jp_conversation/src/stream/event_id_tests.rs b/crates/jp_conversation/src/stream/event_id_tests.rs new file mode 100644 index 000000000..a79af716c --- /dev/null +++ b/crates/jp_conversation/src/stream/event_id_tests.rs @@ -0,0 +1,892 @@ +//! Entry identity across the stream's storage, insertion, and view surfaces. + +use std::collections::HashSet; + +use chrono::{DateTime, Utc}; +use jp_config::PartialAppConfig; +use serde_json::{Map, Value, from_str, from_value, json, to_string}; + +use super::{ + ConversationEventWithConfig, ConversationStream, EventPayload, InternalEvent, ResetDelta, + StoredEvent, +}; +use crate::{ + Compaction, ConversationEvent, EventId, SummaryPolicy, ToolCallPolicy, + event::{ChatRequest, ChatResponse, ToolCallRequest, TurnStart}, +}; + +/// A stream entry with a known ID, for assertions that name one. +fn entry(id: &str, event: ConversationEvent) -> InternalEvent { + InternalEvent { + event_id: EventId::fixed(id), + payload: EventPayload::Event(Box::new(event)), + } +} + +/// An event at the epoch, so a timestamp never varies between runs. +fn event(kind: impl Into) -> ConversationEvent { + let mut event = kind.into(); + event.timestamp = DateTime::::UNIX_EPOCH; + event +} + +/// One turn: a marker, a question, and an answer, with known IDs. +fn fixture() -> ConversationStream { + let mut stream = ConversationStream::new_test(); + stream.extend_entries([ + entry("turn001", event(TurnStart)), + entry("chat001", event(ChatRequest::from("question"))), + entry("reply01", event(ChatResponse::message("answer"))), + ]); + stream +} + +/// A question followed by a tool call that never got a response. +fn orphaned_tool_call() -> ConversationStream { + let mut stream = ConversationStream::new_test(); + stream.extend_entries([ + entry("chat001", event(ChatRequest::from("question"))), + entry( + "tool001", + event(ToolCallRequest::new( + "call1".into(), + "read".into(), + Map::new(), + )), + ), + ]); + stream +} + +/// The IDs a stream holds, in stream order. +fn ids(stream: &ConversationStream) -> Vec { + stream + .events + .iter() + .map(|event| event.event_id.to_string()) + .collect() +} + +// --------------------------------------------------------------------------- +// Storage shape +// --------------------------------------------------------------------------- + +#[test] +fn a_stored_entry_writes_its_id_before_the_payload() { + let stored = to_string(&entry("chat001", event(ChatRequest::from("question")))).unwrap(); + + assert_eq!( + stored, + r#"{"event_id":"chat001","timestamp":"1970-01-01 00:00:00.0","type":"chat_request","content":"question"}"# + ); +} + +#[test] +fn every_payload_kind_round_trips_byte_for_byte() { + // The wrapper's serde is hand-rolled, so each payload kind is pinned to the + // exact bytes it reads and writes. Base64 fields (`arguments`, `content`, + // `metadata`) confirm the encode hooks still run under the wrapper. + for raw in [ + r#"{"event_id":"turn001","timestamp":"1970-01-01 00:00:00.0","type":"turn_start"}"#, + r#"{"event_id":"chat001","timestamp":"1970-01-01 00:00:00.0","type":"chat_request","content":"question"}"#, + r#"{"event_id":"reset01","type":"config_delta","op":"reset","timestamp":"1970-01-01 00:00:00.0"}"#, + r#"{"event_id":"apply01","type":"config_delta","timestamp":"1970-01-01 00:00:00.0","delta":{}}"#, + r#"{"event_id":"tool001","timestamp":"1970-01-01 00:00:00.0","type":"tool_call_request","id":"tool-id","name":"read","arguments":{"path":"YWJj"},"metadata":{"signature":"YWJj"}}"#, + r#"{"event_id":"tool002","timestamp":"1970-01-01 00:00:00.0","type":"tool_call_response","id":"tool-id","content":"YWJj","is_error":false}"#, + r#"{"event_id":"compact","type":"compaction","timestamp":"1970-01-01 00:00:00.0","from_turn":0,"to_turn":1}"#, + r#"{"event_id":"overlay","type":"event_overlay","timestamp":"1970-01-01 00:00:00.0","patches":[]}"#, + ] { + let entry = from_str::(raw).unwrap().into_entry(); + assert_eq!(to_string(&entry).unwrap(), raw, "round-tripping {raw}"); + } +} + +#[test] +fn an_unknown_entry_round_trips_byte_for_byte() { + // A newer `jp` may have written a kind this build does not know. Its raw + // JSON is kept verbatim, so it survives a load and save unchanged. + let raw = r#"{"event_id":"Future ID!","type":"future_event","id":"payload-id","nested":{"event_id":"not-the-wrapper","bytes":"base64:YWJj"},"extra":[1,null,true]}"#; + let entry = from_str::(raw).unwrap().into_entry(); + + assert_eq!(to_string(&entry).unwrap(), raw); +} + +#[test] +fn an_unknown_entry_keeps_its_id_on_the_wrapper_only() { + // The wrapper's `event_id` is lifted out of the stored object, so the + // payload does not carry a second copy and one key is written back. + let raw = r#"{"event_id":"Future ID!","type":"future_event","nested":{"event_id":"not-the-wrapper"}}"#; + let entry = from_str::(raw).unwrap().into_entry(); + + let EventPayload::Unknown(payload) = &entry.payload else { + panic!("expected an unknown payload"); + }; + assert_eq!(entry.event_id, EventId::fixed("Future ID!")); + assert!(payload.get("event_id").is_none()); + // A payload-owned `event_id` deeper in the object is untouched. + assert_eq!(payload["nested"]["event_id"], "not-the-wrapper"); +} + +#[test] +fn lifting_the_id_out_leaves_the_other_keys_in_order() { + // `shift_remove`, not `remove`: a hand-edited entry keeps its key order so + // a save produces no spurious diff. + let entry = from_str::( + r#"{"type":"future_event","first":1,"event_id":"middle","last":2}"#, + ) + .unwrap() + .into_entry(); + + assert_eq!( + to_string(&entry).unwrap(), + r#"{"event_id":"middle","type":"future_event","first":1,"last":2}"# + ); +} + +#[test] +fn an_entry_that_is_not_an_object_is_rejected() { + for raw in ["null", "7", r#""a string""#, "[]"] { + assert!(from_str::(raw).is_err(), "accepted {raw}"); + } +} + +#[test] +fn an_entry_with_a_non_string_id_is_rejected() { + // A corrupt ID is not an absent one: it fails loudly rather than being + // silently replaced. `null` is the exception, and spells absence. + for id in [json!(1), json!(true), json!([]), json!({})] { + assert!( + from_value::(json!({"type": "future_event", "event_id": id})).is_err(), + "accepted {id}" + ); + } +} + +#[test] +fn an_entry_missing_an_id_reads_as_having_none() { + // Identity belongs to the stream, so the wire form reports what the file + // held and leaves assigning one to whoever is loading the stream. + let stored = from_value::(json!({"type": "future_event"})).unwrap(); + + assert!(stored.event_id.is_none()); +} + +#[test] +fn an_entry_with_an_empty_id_reads_as_having_none() { + let stored = + from_value::(json!({"type": "future_event", "event_id": ""})).unwrap(); + + assert!(stored.event_id.is_none()); +} + +#[test] +fn an_entry_with_a_null_id_reads_as_having_none() { + // `null` is JSON's spelling of "no value", and a hand-edited file is + // invited to clear an ID. Rejecting it would fail the whole stream load + // over the one spelling the format itself suggests. + let stored = + from_value::(json!({"type": "future_event", "event_id": Value::Null})) + .unwrap(); + + assert!(stored.event_id.is_none()); +} + +// --------------------------------------------------------------------------- +// Identity across a save and load +// --------------------------------------------------------------------------- + +#[test] +fn an_id_assigned_to_a_legacy_entry_survives_a_save_and_reload() { + let (base, _) = fixture().to_parts().unwrap(); + let loaded = ConversationStream::from_parts( + base, + vec![json!({ + "type": "chat_request", "content": "question", "timestamp": "1970-01-01 00:00:00.0" + })], + &PartialAppConfig::empty(), + ) + .unwrap(); + + let (base, saved) = loaded.to_parts().unwrap(); + let reloaded = ConversationStream::from_parts(base, saved, &PartialAppConfig::empty()).unwrap(); + + assert_eq!( + loaded.first().unwrap().event_id, + reloaded.first().unwrap().event_id + ); +} + +#[test] +fn every_entry_kind_keeps_its_id_across_a_save_and_reload() { + // The claim this RFD rests on: an entry is addressable by a stable ID + // whatever its payload, not only when it happens to be a conversation + // event. A compaction, a patch overlay, a config delta, and an entry this + // build does not recognize are each as referenceable as a chat request. + // + // Only conversation events are reachable through the iteration views, so + // the others are checked where they *are* addressable: the stored JSON. + let mut stream = fixture(); + stream.add_config_delta(ResetDelta { + timestamp: DateTime::::UNIX_EPOCH, + }); + stream.add_compaction(Compaction::new(0, 0)); + stream.add_overlay(Vec::new()); + stream.extend_entries([from_value::( + json!({"event_id": "future1", "type": "future_event"}), + ) + .unwrap() + .into_entry()]); + let before = ids(&stream); + + let (base_config, saved) = stream.to_parts().unwrap(); + let reloaded = + ConversationStream::from_parts(base_config, saved, &PartialAppConfig::empty()).unwrap(); + + assert_eq!(ids(&reloaded), before); + // Each kind is present, so the assertion above covers all of them rather + // than a list that happens to be all events. + let kinds: Vec<_> = reloaded + .to_parts() + .unwrap() + .1 + .iter() + .map(|event| event["type"].as_str().unwrap_or_default().to_owned()) + .collect(); + assert_eq!(kinds, [ + "turn_start", + "chat_request", + "chat_response", + "config_delta", + "compaction", + "event_overlay", + "future_event", + ]); +} + +#[test] +fn editing_an_entry_keeps_its_id() { + let mut stream = fixture(); + + stream + .iter_mut() + .nth(1) + .unwrap() + .as_chat_request_mut() + .unwrap() + .content = "edited".into(); + + assert_eq!(ids(&stream)[1], "chat001"); + assert_eq!( + stream + .iter() + .nth(1) + .unwrap() + .as_chat_request() + .unwrap() + .content, + "edited" + ); +} + +#[test] +fn filtering_the_stream_keeps_the_ids_of_survivors() { + let mut stream = fixture(); + + stream.retain(|event| !event.is_chat_response()); + + assert_eq!(ids(&stream), ["turn001", "chat001"]); +} + +// --------------------------------------------------------------------------- +// Iteration views +// --------------------------------------------------------------------------- +// +// Every view onto the stream reports the same ID for the same entry. One test +// per view, so a failure names the view that drifted. + +#[test] +fn iter_reports_entry_ids() { + let stream = fixture(); + + let seen: Vec<_> = stream + .iter() + .map(|event| event.event_id.to_string()) + .collect(); + + assert_eq!(seen, ["turn001", "chat001", "reply01"]); +} + +#[test] +fn iter_reports_entry_ids_in_reverse() { + let stream = fixture(); + + let seen: Vec<_> = stream + .iter() + .rev() + .map(|event| event.event_id.to_string()) + .collect(); + + assert_eq!(seen, ["reply01", "chat001", "turn001"]); +} + +#[test] +fn iter_mut_reports_entry_ids() { + let mut stream = fixture(); + + let seen: Vec<_> = stream + .iter_mut() + .map(|event| event.event_id.to_string()) + .collect(); + + assert_eq!(seen, ["turn001", "chat001", "reply01"]); +} + +#[test] +fn into_iter_reports_entry_ids() { + let seen: Vec<_> = fixture() + .into_iter() + .map(|event| event.event_id.to_string()) + .collect(); + + assert_eq!(seen, ["turn001", "chat001", "reply01"]); +} + +#[test] +fn into_iter_reports_entry_ids_in_reverse() { + let seen: Vec<_> = fixture() + .into_iter() + .rev() + .map(|event| event.event_id.to_string()) + .collect(); + + assert_eq!(seen, ["reply01", "chat001", "turn001"]); +} + +#[test] +fn iter_turns_reports_entry_ids() { + let stream = fixture(); + + let seen: Vec<_> = stream + .iter_turns() + .flat_map(|turn| { + turn.iter() + .map(|event| event.event_id.to_string()) + .collect::>() + }) + .collect(); + + assert_eq!(seen, ["turn001", "chat001", "reply01"]); +} + +#[test] +fn iter_events_by_turn_reports_entry_ids() { + let stream = fixture(); + + let seen: Vec<_> = stream + .iter_events_by_turn() + .map(|event| event.event_id.to_string()) + .collect(); + + assert_eq!(seen, ["turn001", "chat001", "reply01"]); +} + +#[test] +fn iter_events_by_turn_pairs_each_id_with_its_turn() { + let mut stream = fixture(); + stream.start_turn("next question"); + + let seen: Vec<_> = stream + .iter_events_by_turn() + .map(|event| (event.turn, event.event_id.to_string())) + .collect(); + + assert_eq!(seen[..3], [ + (0, "turn001".to_owned()), + (0, "chat001".to_owned()), + (0, "reply01".to_owned()), + ]); + assert_eq!(seen[3].0, 1); + assert_eq!(seen[4].0, 1); +} + +#[test] +fn pop_reports_the_id_of_the_entry_it_removed() { + let mut stream = fixture(); + + assert_eq!(stream.pop().unwrap().event_id, EventId::fixed("reply01")); +} + +#[test] +fn converting_a_borrowed_event_keeps_its_id() { + let stream = fixture(); + + let owned = ConversationEventWithConfig::from(stream.first().unwrap()); + + assert_eq!(owned.event_id, EventId::fixed("turn001")); +} + +#[test] +fn an_unknown_entry_stays_invisible_to_event_iteration() { + let mut stream = fixture(); + stream.extend_entries([from_value::( + json!({"event_id": "unknown", "type": "future_event", "content": "opaque"}), + ) + .unwrap() + .into_entry()]); + + assert_eq!(stream.iter().count(), 3); + assert_eq!(stream.iter_mut().count(), 3); + assert_eq!(stream.clone().into_iter().count(), 3); + assert_eq!(stream.iter_events_by_turn().count(), 3); + // Invisible to iteration, but still stored and still addressable. + assert_eq!(stream.to_parts().unwrap().1.len(), 4); +} + +// --------------------------------------------------------------------------- +// Insertion +// --------------------------------------------------------------------------- + +#[test] +fn pushing_an_event_returns_the_id_it_was_given() { + let mut stream = ConversationStream::new_test(); + + let event_id = stream.push_event(event(ChatRequest::from("question"))); + + assert_eq!(ids(&stream), [event_id.to_string()]); +} + +#[test] +fn starting_a_turn_assigns_ids_to_both_entries() { + let mut stream = ConversationStream::new_test(); + + stream.start_turn("question"); + + assert_eq!(ids(&stream).len(), 2); + assert_ne!(ids(&stream)[0], ids(&stream)[1]); +} + +#[test] +fn building_a_turn_assigns_ids_to_its_events() { + let mut stream = fixture(); + + stream + .current_turn_mut() + .add_event(event(ChatResponse::message("reply"))) + .build() + .unwrap(); + + assert_eq!(ids(&stream).len(), 4); + assert!(!ids(&stream)[3].is_empty()); +} + +#[test] +fn adding_a_config_delta_assigns_it_an_id() { + let mut stream = ConversationStream::new_test(); + + stream.add_config_delta(ResetDelta { + timestamp: DateTime::::UNIX_EPOCH, + }); + + assert!(!ids(&stream)[0].is_empty()); +} + +#[test] +fn adding_a_compaction_assigns_it_an_id() { + let mut stream = ConversationStream::new_test(); + + stream.add_compaction(Compaction::new(0, 0)); + + assert!(!ids(&stream)[0].is_empty()); +} + +#[test] +fn adding_an_overlay_assigns_it_an_id() { + let mut stream = ConversationStream::new_test(); + + stream.add_overlay(Vec::new()); + + assert!(!ids(&stream)[0].is_empty()); +} + +#[test] +fn every_stored_entry_carries_a_non_empty_id() { + let mut stream = ConversationStream::new_test(); + stream.start_turn("question"); + stream.add_config_delta(ResetDelta { + timestamp: DateTime::::UNIX_EPOCH, + }); + stream.add_compaction(Compaction::new(0, 0)); + stream.add_overlay(Vec::new()); + + let (_, saved) = stream.to_parts().unwrap(); + assert!( + saved + .iter() + .all(|event| event["event_id"].as_str().is_some_and(|id| !id.is_empty())), + "stored entries: {saved:#?}" + ); +} + +#[test] +fn trimming_a_turn_keeps_the_id_of_an_overlay_it_preserves() { + // The overlay is popped and put back, not recreated, so it is the same + // entry and keeps its identity. Reassigning it would break a reference to + // an overlay that outlived the turn it happened to sit behind. + let mut stream = fixture(); + stream.add_compaction(Compaction::new(0, 0)); + let overlay_id = ids(&stream)[3].clone(); + + let request = stream.trim_chat_request(); + + assert_eq!(request.map(|r| r.content), Some("question".to_owned())); + assert_eq!(ids(&stream), ["turn001".to_owned(), overlay_id]); +} + +#[test] +fn an_id_is_retired_with_the_entry_that_held_it() { + // The removed entry's ID is never handed out again, so a reference to it + // fails to resolve rather than binding to a later, unrelated entry. + let mut stream = ConversationStream::new_test(); + let removed = stream.push_event(event(ChatRequest::from("question"))); + assert_eq!(stream.pop().unwrap().event_id, removed); + + let replacement = stream.push_event(event(ChatRequest::from("another question"))); + + assert_ne!(replacement, removed); +} + +// --------------------------------------------------------------------------- +// Moving entries between streams +// --------------------------------------------------------------------------- + +#[test] +fn appending_a_stream_keeps_the_incoming_ids() { + // Uniqueness is scoped to one stream, so a copy inherits the source's + // identities and a reference into the source resolves against the copy. + let mut destination = ConversationStream::new_test(); + + destination.append_stream(fixture()); + + assert_eq!(ids(&destination), ["turn001", "chat001", "reply01"]); +} + +#[test] +fn appending_a_stream_keeps_the_incoming_payloads() { + let source = fixture(); + let mut destination = ConversationStream::new_test(); + + destination.append_stream(source.clone()); + + let payloads: Vec<_> = destination + .events + .iter() + .map(|event| &event.payload) + .collect(); + let expected: Vec<_> = source.events.iter().map(|event| &event.payload).collect(); + assert_eq!(payloads, expected); +} + +#[test] +fn appending_a_stream_replaces_an_id_the_destination_already_holds() { + // Appending a stream onto itself is the one case where preserving the + // incoming ID would break uniqueness, so the second copy is reassigned. + let mut destination = fixture(); + + destination.append_stream(fixture()); + + assert_eq!(ids(&destination)[..3], ["turn001", "chat001", "reply01"]); + let unique: HashSet<_> = ids(&destination).into_iter().collect(); + assert_eq!(unique.len(), 6, "every entry holds a distinct ID"); +} + +#[test] +fn extending_from_another_stream_keeps_the_incoming_ids() { + let mut destination = ConversationStream::new_test(); + + destination.extend(fixture()); + + assert_eq!(ids(&destination), ["turn001", "chat001", "reply01"]); +} + +#[test] +fn extending_with_bare_events_assigns_fresh_ids() { + // A `ConversationEvent` carries no identity; the stream entry wrapping it + // does. + let mut stream = ConversationStream::new_test(); + + stream.extend([event(ChatRequest::from("question"))]); + + assert!(!ids(&stream)[0].is_empty()); +} + +// --------------------------------------------------------------------------- +// Repair injected by `sanitize` +// --------------------------------------------------------------------------- + +#[test] +fn synthetic_entries_are_assigned_distinct_ids() { + let mut stream = orphaned_tool_call(); + + stream.sanitize(); + + // A leading `TurnStart` and a synthetic response join the two originals. + let unique: HashSet<_> = ids(&stream).into_iter().collect(); + assert_eq!(unique.len(), 4); +} + +#[test] +fn sanitize_keeps_the_ids_of_the_entries_it_did_not_add() { + let mut stream = orphaned_tool_call(); + + stream.sanitize(); + + let seen: Vec<_> = stream + .iter() + .map(|event| event.event_id.to_string()) + .collect(); + assert_eq!(seen[1], "chat001"); + assert_eq!(seen[2], "tool001"); +} + +#[test] +fn a_synthetic_tool_response_keeps_its_requests_timestamp() { + // Only the ID is stream-assigned; shifting the timestamp to "now" would + // change ordering semantics. + let mut stream = orphaned_tool_call(); + + stream.sanitize(); + + assert!( + stream + .iter() + .all(|event| event.timestamp == DateTime::::UNIX_EPOCH) + ); +} + +// --------------------------------------------------------------------------- +// Projection +// --------------------------------------------------------------------------- + +/// A stream whose single turn is replaced by a summary when projected. +fn summarized() -> ConversationStream { + let mut stream = fixture(); + stream.add_compaction(Compaction { + timestamp: DateTime::::UNIX_EPOCH, + from_turn: 0, + to_turn: 0, + summary: Some(SummaryPolicy::generated("summary")), + reasoning: None, + tool_calls: None, + }); + stream +} + +#[test] +fn projecting_does_not_touch_the_raw_stream() { + let raw = summarized(); + let saved = raw.to_parts().unwrap(); + + let mut projected = raw.clone(); + projected.apply_projection(); + + assert_eq!(raw.to_parts().unwrap(), saved); +} + +#[test] +fn a_synthetic_summary_entry_has_no_id_from_the_raw_stream() { + // Summary entries exist only in the projected view, so their IDs are + // ephemeral and must not be used as references into `events.json`. + let raw = summarized(); + let mut projected = raw.clone(); + + projected.apply_projection(); + + let raw_ids: HashSet<_> = ids(&raw).into_iter().collect(); + assert_eq!(raw_ids.len(), 4, "three entries and the compaction"); + assert!( + ids(&projected).iter().all(|id| !raw_ids.contains(id)), + "projected: {:?}, raw: {raw_ids:?}", + ids(&projected) + ); +} + +#[test] +fn synthetic_summary_entries_have_distinct_ids() { + let mut projected = summarized(); + + projected.apply_projection(); + + let unique: HashSet<_> = ids(&projected).into_iter().collect(); + assert_eq!(unique.len(), 3); +} + +/// Two turns where a compaction summarizes only the first. +fn partially_summarized() -> ConversationStream { + let mut stream = ConversationStream::new_test(); + stream.extend_entries([ + entry("turn001", event(TurnStart)), + entry("chat001", event(ChatRequest::from("old question"))), + entry("reply01", event(ChatResponse::message("old answer"))), + entry("turn002", event(TurnStart)), + entry("chat002", event(ChatRequest::from("new question"))), + entry("reply02", event(ChatResponse::message("new answer"))), + ]); + stream.add_compaction(Compaction { + timestamp: DateTime::::UNIX_EPOCH, + from_turn: 0, + to_turn: 0, + summary: Some(SummaryPolicy::generated("summary")), + reasoning: None, + tool_calls: None, + }); + stream +} + +#[test] +fn a_retained_entry_keeps_its_id_alongside_an_injected_summary() { + // The case where both kinds of projected entry coexist: the summary's + // ephemeral IDs and the second turn's stored IDs are in one list, and the + // retained entries must still carry the IDs they have in `events.json`. + let mut projected = partially_summarized(); + + projected.apply_projection(); + + let ids = ids(&projected); + assert_eq!(ids.len(), 6, "three synthetic, three retained: {ids:?}"); + assert_eq!(ids[3..], ["turn002", "chat002", "reply02"]); +} + +#[test] +fn a_summarized_entrys_id_does_not_reappear_in_the_projected_view() { + // A reader holding a reference into `events.json` must not find it + // resolving to a summary entry that stands in for the entry it named. + // + // Generated IDs make an accidental reuse vanishingly unlikely, so this is + // the weaker half of the pair: what makes it hold is that + // `projection::apply` seeds its `EventIds` from every raw entry before any + // synthetic one is drawn. `a_draw_retries_past_an_id_the_set_already_holds` + // pins the drawing itself, deterministically. + let mut projected = partially_summarized(); + + projected.apply_projection(); + + let projected_ids = ids(&projected); + let unique: HashSet<_> = projected_ids.iter().collect(); + assert_eq!( + unique.len(), + projected_ids.len(), + "two projected entries share an ID: {projected_ids:?}" + ); + + let summarized = ["turn001", "chat001", "reply01"]; + assert!( + projected_ids + .iter() + .all(|id| !summarized.contains(&id.as_str())), + "a synthetic entry took a summarized entry's ID: {projected_ids:?}" + ); +} + +#[test] +fn projecting_leaves_the_id_set_covering_the_projected_entries() { + // `apply_projection` replaces the entry list wholesale, synthetic entries + // included. The stream's ID set has to come out of that holding every ID + // its entries carry, or a later insertion can hand out one of them again. + let mut projected = partially_summarized(); + + projected.apply_projection(); + + assert!(projected.id_set_covers_entries()); +} + +#[test] +fn a_cloned_stream_carries_the_ids_its_source_had_handed_out() { + // `PartialEq` deliberately ignores `event_ids`, so a clone that lost the + // set still compares equal to its source and most tests would not notice. + // `append_stream(self.clone())` would: the copy would see no collisions and + // keep every incoming ID, leaving the result holding each one twice. + // The pop retires an ID, so the set holds one the entries no longer carry + // and a clone that rebuilt it from the entries would come out different. + let mut source = fixture(); + assert!(source.pop().is_some()); + + let mut clone = source.clone(); + clone.append_stream(source.clone()); + + assert!(clone.id_set_covers_entries()); + let unique: HashSet<_> = ids(&clone).into_iter().collect(); + assert_eq!(unique.len(), ids(&clone).len(), "{:?}", ids(&clone)); +} + +#[test] +fn a_stream_that_was_never_projected_covers_its_entries() { + // The same invariant on the ordinary paths, so a failure above points at + // projection rather than at insertion. + let mut stream = fixture(); + stream.start_turn("question"); + stream.add_compaction(Compaction::new(0, 0)); + stream.append_stream(fixture()); + stream.sanitize(); + + assert!(stream.id_set_covers_entries()); +} + +/// A stream whose tool call request is blanked, not removed, when projected. +fn stripped_tool_call() -> ConversationStream { + let mut stream = ConversationStream::new_test(); + stream.extend_entries([ + entry("turn001", event(TurnStart)), + entry("chat001", event(ChatRequest::from("question"))), + entry( + "tool001", + event(ToolCallRequest::new( + "call1".into(), + "read".into(), + Map::from_iter([("path".into(), json!("src/main.rs"))]), + )), + ), + ]); + stream.add_compaction(Compaction { + timestamp: DateTime::::UNIX_EPOCH, + from_turn: 0, + to_turn: 0, + summary: None, + reasoning: None, + tool_calls: Some( + ToolCallPolicy::Strip { + request: true, + response: false, + } + .into(), + ), + }); + stream +} + +#[test] +fn a_mechanically_projected_entry_keeps_its_raw_id() { + // A policy that rewrites an entry's content leaves its identity alone: the + // projected entry still points at the raw entry it came from. + let mut projected = stripped_tool_call(); + + projected.apply_projection(); + + assert_eq!(ids(&projected), ["turn001", "chat001", "tool001"]); +} + +#[test] +fn a_mechanically_projected_entry_has_its_content_rewritten() { + let mut projected = stripped_tool_call(); + + projected.apply_projection(); + + assert!( + projected + .last() + .unwrap() + .as_tool_call_request() + .unwrap() + .arguments + .is_empty() + ); +} diff --git a/crates/jp_conversation/src/stream/iter.rs b/crates/jp_conversation/src/stream/iter.rs new file mode 100644 index 000000000..2a7f28afa --- /dev/null +++ b/crates/jp_conversation/src/stream/iter.rs @@ -0,0 +1,444 @@ +//! Walking a conversation stream, and what one step of that walk yields. +//! +//! Every iterator here resolves the configuration each event was recorded +//! under, and yields a `…WithConfig` view carrying it. +//! A stream stores config as a base plus a series of deltas, so an event's +//! config is the base with every delta *before it* folded in: a forward walk +//! carries that state as it goes, and a reverse walk reconstructs it. +//! +//! Entries that are not conversation events are stepped over: a config delta is +//! folded into the running state, and compactions, patch overlays, and entries +//! this build does not recognize are skipped entirely. +//! +//! Three iterators, by what the caller may do with what it gets: +//! +//! - [`Iter`] borrows, and walks from either end. +//! - [`IterMut`] borrows mutably, forwards only. +//! - [`IntoIter`] takes ownership, and walks from either end. +//! +//! Each yields an event's entry ID alongside it. +//! Editing an event through [`IterMut`] does not change its ID: identity +//! belongs to the entry, not to its content. + +use jp_config::{PartialAppConfig, PartialConfig as _}; +use tracing::error; + +use super::{ + ConversationStream, StreamError, + config_delta::{self, ApplyDelta}, + entry::{EventPayload, InternalEvent}, +}; +use crate::{ConversationEvent, EventId, EventKind}; + +/// Append events carrying their own identity and config state. +/// +/// Each event keeps its [`EventId`] unless this stream has already handed that +/// ID out, so moving events between streams preserves references into the +/// source. +/// +/// The config deltas separating them are not copied: they are recomputed +/// against this stream's running config state, which is what suits a +/// destination whose base config differs from the source's. +/// A recomputed delta is a new entry and is assigned a new ID. +impl Extend for ConversationStream { + fn extend>(&mut self, iter: T) { + // Cache the running tail config across iterations. Without this, every + // push falls through `push_with_config_delta` → `self.last()`, which + // walks the whole stream and deep-clones `PartialAppConfig` on each + // step — making `extend(n)` O(n²) in clones. + let mut tail = self + .last() + .map_or_else(|| self.base_config().to_partial(), |v| v.config); + + for v in iter { + let ConversationEventWithConfig { + event_id, + event, + config, + } = v; + let config_delta = tail.delta(config.clone()); + + if !config_delta.is_empty() { + self.add_config_delta(ApplyDelta::new(event.timestamp, config_delta)); + } + + tail = config; + self.adopt(InternalEvent { + event_id, + payload: EventPayload::Event(Box::new(event)), + }); + } + } +} + +/// Append bare events, each receiving an ID assigned by this stream. +/// +/// A [`ConversationEvent`] carries no identity of its own; identity belongs to +/// the stream entry wrapping it. +impl Extend for ConversationStream { + fn extend>(&mut self, iter: T) { + for v in iter { + self.push_event(v); + } + } +} + +impl IntoIterator for ConversationStream { + type IntoIter = IntoIter; + type Item = ConversationEventWithConfig; + + fn into_iter(self) -> Self::IntoIter { + IntoIter { + current_config: self.base_config().to_partial(), + inner_iter: self.events.into_iter(), + } + } +} + +/// An owned iterator over the events in a conversation stream. +pub struct IntoIter { + /// The configuration state for the next event in the iterator. + current_config: PartialAppConfig, + + /// The iterator over the events in the stream. + inner_iter: std::vec::IntoIter, +} + +impl Iterator for IntoIter { + type Item = ConversationEventWithConfig; + + fn next(&mut self) -> Option { + loop { + let InternalEvent { event_id, payload } = self.inner_iter.next()?; + + match payload { + EventPayload::ConfigDelta(delta) => { + if let Err(error) = config_delta::fold(&mut self.current_config, delta) { + error!(%error, "Failed to merge config delta."); + } + } + EventPayload::Event(event) => { + return Some(ConversationEventWithConfig { + event_id, + event: *event, + config: self.current_config.clone(), + }); + } + EventPayload::Compaction(_) + | EventPayload::Overlay(_) + | EventPayload::Unknown(_) => {} + } + } + } +} + +impl DoubleEndedIterator for IntoIter { + fn next_back(&mut self) -> Option { + loop { + let InternalEvent { event_id, payload } = self.inner_iter.next_back()?; + + match payload { + EventPayload::ConfigDelta(_) + | EventPayload::Compaction(_) + | EventPayload::Overlay(_) + | EventPayload::Unknown(_) => { + // A delta/compaction at the very end of the list affects + // nothing that follows it, and it doesn't affect previous + // items. We simply discard it. + // event at the tail likewise yields no ConversationEvent. + } + EventPayload::Event(event) => { + // Start with the state currently at the front of the line + let mut config = self.current_config.clone(); + + // Scan the remaining items in the middle (without consuming + // them) to apply all pending deltas to our temporary + // config. + for internal_event in self.inner_iter.as_slice() { + if let EventPayload::ConfigDelta(delta) = &internal_event.payload + && let Err(error) = config_delta::fold(&mut config, delta.clone()) + { + error!(%error, "Failed to merge config delta."); + } + } + + return Some(ConversationEventWithConfig { + event_id, + event: *event, + config, + }); + } + } + } + } +} + +/// An iterator over the borrowed events in a conversation stream. +pub(super) struct Iter<'a> { + /// The stream being iterated over. + pub(super) stream: &'a ConversationStream, + + /// The configuration state for the first, next event in the iterator. + pub(super) front_config: PartialAppConfig, + + /// The index of the `next` event in the iterator. + pub(super) front: usize, + + /// The index of the `next_back` event in the iterator. + pub(super) back: usize, +} + +impl<'a> Iterator for Iter<'a> { + type Item = ConversationEventWithConfigRef<'a>; + + fn next(&mut self) -> Option { + while self.front < self.back { + let internal = &self.stream.events[self.front]; + let event_id = &internal.event_id; + self.front += 1; + + match &internal.payload { + EventPayload::ConfigDelta(delta) => { + if let Err(error) = config_delta::fold(&mut self.front_config, delta.clone()) { + error!(%error, "Failed to merge config delta."); + } + } + EventPayload::Event(event) => { + return Some(ConversationEventWithConfigRef { + event_id, + event, + config: self.front_config.clone(), + }); + } + EventPayload::Compaction(_) + | EventPayload::Overlay(_) + | EventPayload::Unknown(_) => {} + } + } + + None + } +} + +impl DoubleEndedIterator for Iter<'_> { + fn next_back(&mut self) -> Option { + while self.back > self.front { + self.back -= 1; + let internal = &self.stream.events[self.back]; + let event_id = &internal.event_id; + + let EventPayload::Event(event) = &internal.payload else { + continue; + }; + + let mut config = self.stream.base_config().to_partial(); + for internal_event in &self.stream.events[..self.back] { + if let EventPayload::ConfigDelta(delta) = &internal_event.payload + && let Err(error) = config_delta::fold(&mut config, delta.clone()) + { + error!(%error, "Failed to merge config delta."); + } + } + + return Some(ConversationEventWithConfigRef { + event_id, + event, + config, + }); + } + + None + } +} + +/// An iterator over the mutable events in a conversation stream. +pub struct IterMut<'a> { + /// The configuration state for the first, next event in the iterator. + pub(super) front_config: PartialAppConfig, + + /// The iterator over the events in the stream. + pub(super) iter: std::slice::IterMut<'a, InternalEvent>, +} + +impl<'a> Iterator for IterMut<'a> { + type Item = ConversationEventWithConfigMut<'a>; + + fn next(&mut self) -> Option { + for internal in self.iter.by_ref() { + let event_id = &internal.event_id; + match &mut internal.payload { + EventPayload::ConfigDelta(delta) => { + if let Err(error) = config_delta::fold(&mut self.front_config, delta.clone()) { + error!(%error, "Failed to merge config delta."); + } + } + EventPayload::Event(event) => { + return Some(ConversationEventWithConfigMut { + event_id, + event, + config: self.front_config.clone(), + }); + } + EventPayload::Compaction(_) + | EventPayload::Overlay(_) + | EventPayload::Unknown(_) => {} + } + } + + None + } +} + +/// A [`ConversationEvent`] with the turn it belongs to and its entry ID. +/// +/// Yielded by [`ConversationStream::iter_events_by_turn`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct EventInTurn<'a> { + /// 0-based index of the turn holding this event. + pub turn: usize, + + /// The identity of this entry in its stream. + pub event_id: &'a EventId, + + /// The event. + pub event: &'a ConversationEvent, +} + +impl std::ops::Deref for EventInTurn<'_> { + type Target = ConversationEvent; + + fn deref(&self) -> &Self::Target { + self.event + } +} + +/// A reference to a [`ConversationEvent`] with its configuration. +#[derive(Debug, PartialEq, Clone)] +pub struct ConversationEventWithConfigRef<'a> { + /// The identity of this entry in its stream. + pub event_id: &'a EventId, + + /// The event. + pub event: &'a ConversationEvent, + + /// The configuration. + pub config: PartialAppConfig, +} + +/// A mutable reference to a [`ConversationEvent`] with its configuration. +#[derive(Debug, PartialEq)] +pub struct ConversationEventWithConfigMut<'a> { + /// The identity of this entry, unchanged by payload edits. + pub event_id: &'a EventId, + + /// The event. + pub event: &'a mut ConversationEvent, + + /// The configuration. + pub config: PartialAppConfig, +} + +/// A [`ConversationEvent`] with its configuration. +#[derive(Debug, PartialEq, Clone)] +pub struct ConversationEventWithConfig { + /// The identity of this entry in its source stream. + pub event_id: EventId, + + /// The event. + pub event: ConversationEvent, + + /// The configuration at the time the event was added. + /// + /// It should be noted that this is not necessarily the same as the current + /// active configuration of the application, even if this is the latest + /// event in the stream. + /// For one, the event may have been added a while ago, but more + /// importantly, not all configuration changes are automatically applied to + /// a [`ConversationStream`]. + /// For example, if a new tool is added in the configuration, it will not + /// become available in the conversation stream until explicitly added using + /// the CLI flag `--tool` or `--cfg`, while *NEW* conversations *WILL* get + /// the new tool by default. + pub config: PartialAppConfig, +} + +impl ConversationEventWithConfig { + /// Consume the type and return the underlying [`ConversationEvent`]. + #[must_use] + pub fn into_inner(self) -> ConversationEvent { + self.event + } + + /// Consume the type and return the underlying [`EventKind`]. + #[must_use] + pub fn into_kind(self) -> EventKind { + self.event.kind + } + + /// Return a reference to the underlying [`EventKind`]. + #[must_use] + pub const fn kind(&self) -> &EventKind { + &self.event.kind + } +} + +impl From> for ConversationEventWithConfig { + fn from(value: ConversationEventWithConfigRef<'_>) -> Self { + Self { + event_id: value.event_id.clone(), + event: value.event.clone(), + config: value.config, + } + } +} + +impl FromIterator for Result { + fn from_iter>(iter: T) -> Self { + let mut events = iter.into_iter(); + + let Some((config, first_id, first_event)) = + events.next().map(|e| (e.config, e.event_id, e.event)) + else { + return Err(StreamError::FromEmptyIterator); + }; + + let mut stream = ConversationStream::new(jp_config::util::build(config)?.into()); + stream.adopt(InternalEvent { + event_id: first_id, + payload: EventPayload::Event(Box::new(first_event)), + }); + stream.extend(events); + + Ok(stream) + } +} + +impl std::ops::Deref for ConversationEventWithConfig { + type Target = ConversationEvent; + + fn deref(&self) -> &Self::Target { + &self.event + } +} + +impl std::ops::Deref for ConversationEventWithConfigRef<'_> { + type Target = ConversationEvent; + + fn deref(&self) -> &Self::Target { + self.event + } +} + +impl std::ops::Deref for ConversationEventWithConfigMut<'_> { + type Target = ConversationEvent; + + fn deref(&self) -> &Self::Target { + self.event + } +} + +impl std::ops::DerefMut for ConversationEventWithConfigMut<'_> { + fn deref_mut(&mut self) -> &mut Self::Target { + self.event + } +} diff --git a/crates/jp_conversation/src/stream/projection.rs b/crates/jp_conversation/src/stream/projection.rs index c7eae1274..bd47b79c9 100644 --- a/crates/jp_conversation/src/stream/projection.rs +++ b/crates/jp_conversation/src/stream/projection.rs @@ -17,10 +17,11 @@ use std::collections::{HashMap, HashSet, VecDeque}; use chrono::{DateTime, Utc}; use serde_json::{Map, Value}; -use super::InternalEvent; +use super::{EventPayload, InternalEvent}; use crate::{ ByteSize, Compaction, PolicySpec, ReasoningPolicy, ToolCallPolicy, event::{ChatRequest, ChatResponse, ConversationEvent, TurnStart}, + event_id::EventIds, }; /// Which raw conversation turn(s) a projected turn stands for. @@ -80,8 +81,8 @@ impl TurnOrigin { fn apply_overlays(events: &mut Vec) { let patches: Vec<_> = events .iter() - .filter_map(|e| match e { - InternalEvent::Overlay(overlay) => Some(overlay.patches.clone()), + .filter_map(|e| match &e.payload { + EventPayload::Overlay(overlay) => Some(overlay.patches.clone()), _ => None, }) .flatten() @@ -92,14 +93,14 @@ fn apply_overlays(events: &mut Vec) { } for event in events.iter_mut() { - if let InternalEvent::Event(conv_event) = event { + if let EventPayload::Event(conv_event) = &mut event.payload { for patch in &patches { patch.apply(&mut conv_event.metadata); } } } - events.retain(|e| !matches!(e, InternalEvent::Overlay(_))); + events.retain(|e| !matches!(&e.payload, EventPayload::Overlay(_))); } /// Resolved compaction policies for a single turn. @@ -151,14 +152,20 @@ struct ResolvedSummary { /// Returns one [`TurnOrigin`] per resulting turn, in turn order, mapping each /// projected turn back to the raw turn number(s) it represents. /// +/// `event_ids` is the stream's own ID set, and the synthetic entries are drawn +/// from it. +/// It is taken rather than rebuilt here so the stream still holds every ID its +/// entries carry once this returns: a set that had never seen the synthetic +/// entries could hand one of their IDs to a later insertion. +/// /// [`Compaction`]: crate::Compaction -pub(super) fn apply(events: &mut Vec) -> Vec { +pub(super) fn apply(events: &mut Vec, event_ids: &mut EventIds) -> Vec { apply_overlays(events); let compactions: Vec<_> = events .iter() - .filter_map(|e| match e { - InternalEvent::Compaction(c) => Some(c.clone()), + .filter_map(|e| match &e.payload { + EventPayload::Compaction(c) => Some(c.clone()), _ => None, }) .collect(); @@ -197,24 +204,24 @@ pub(super) fn apply(events: &mut Vec) -> Vec { let mut event_origins: Vec = Vec::with_capacity(events.len()); let mut summaries_injected: HashSet = HashSet::new(); - for (i, event) in std::mem::take(events).into_iter().enumerate() { + for (i, mut event) in std::mem::take(events).into_iter().enumerate() { let turn = turn_indices[i]; - match event { + match &mut event.payload { // Config deltas carry global state; unknown (forward-compat) events // are opaque. Both pass through projection verbatim — the iterators // skip unknown events, so they stay invisible to providers. - InternalEvent::ConfigDelta(_) | InternalEvent::Unknown(_) => { + EventPayload::ConfigDelta(_) | EventPayload::Unknown(_) => { projected.push(event); event_origins.push(TurnOrigin::Kept(turn)); } // Compaction events are consumed by projection — they've been // applied and should not survive into the projected stream. // Patch overlays were consumed by `apply_overlays` above. - InternalEvent::Compaction(_) | InternalEvent::Overlay(_) => {} - InternalEvent::Event(conv_event) => { + EventPayload::Compaction(_) | EventPayload::Overlay(_) => {} + EventPayload::Event(conv_event) => { let Some(policy) = policies.get(turn) else { - projected.push(InternalEvent::Event(conv_event)); + projected.push(event); event_origins.push(TurnOrigin::Kept(turn)); continue; }; @@ -236,6 +243,7 @@ pub(super) fn apply(events: &mut Vec) -> Vec { inject_summary( &mut projected, &mut event_origins, + event_ids, &summary.text, conv_event.timestamp, turn, @@ -246,11 +254,11 @@ pub(super) fn apply(events: &mut Vec) -> Vec { continue; } - let Some(event) = apply_mechanical(*conv_event, policy, tool_calls.get(&i)) else { + if apply_mechanical(conv_event, policy, tool_calls.get(&i)) == Projected::Dropped { continue; - }; + } - projected.push(InternalEvent::Event(Box::new(event))); + projected.push(event); event_origins.push(TurnOrigin::Kept(turn)); } } @@ -359,23 +367,34 @@ pub(super) fn affected_items( items } +/// Whether a mechanical policy left an event in the projected view. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Projected { + /// The event stays, with its content possibly rewritten in place. + Kept, + /// The event is gone from the projected view. + Dropped, +} + /// Apply a turn's mechanical policies (reasoning and tool calls) to one event. /// -/// Returns `None` when the policies drop the event from the projected view. +/// The event is rewritten in place, which is what preserves its `event_id`: a +/// projected entry still refers to the raw entry it came from, even when a +/// policy blanks its content. /// A policy whose spec carries an `over` threshold reaches only the items /// larger than it; without one, every item in range is reached. fn apply_mechanical( - mut event: ConversationEvent, + event: &mut ConversationEvent, policy: &TurnPolicy, info: Option<&ToolCallInfo>, -) -> Option { +) -> Projected { if let Some(spec) = &policy.reasoning && matches!(spec.policy, ReasoningPolicy::Strip) && let Some(response) = event.as_chat_response() && response.is_reasoning() && spec.covers(reasoning_size(response)) { - return None; + return Projected::Dropped; } // A `None` tool-call policy means "no opinion", so the event passes through @@ -394,23 +413,23 @@ fn apply_mechanical( if (event.is_tool_call_request() || event.is_tool_call_response()) && spec.covers(pair) { - return None; + return Projected::Dropped; } } ToolCallPolicy::Strip { request, response } => { // Each half is judged on its own size, so a call with a short // request and a huge response loses only the response. if *request && event.is_tool_call_request() && spec.covers(own) { - strip_tool_request(&mut event); + strip_tool_request(event); } if *response && event.is_tool_call_response() && spec.covers(own) { - strip_tool_response(&mut event, info.map_or("unknown", |i| i.name.as_str())); + strip_tool_response(event, info.map_or("unknown", |i| i.name.as_str())); } } } } - Some(event) + Projected::Kept } /// Group projected events into turns (matching [`IterTurns`]) and return each @@ -460,18 +479,18 @@ pub(super) fn assign_turn_indices(events: &[InternalEvent]) -> Vec { let mut current_has_event = false; for event in events { - match event { - InternalEvent::Event(ev) => { + match &event.payload { + EventPayload::Event(ev) => { if ev.is_turn_start() && current_has_event { turn += 1; } indices.push(turn); current_has_event = true; } - InternalEvent::ConfigDelta(_) - | InternalEvent::Compaction(_) - | InternalEvent::Overlay(_) - | InternalEvent::Unknown(_) => { + EventPayload::ConfigDelta(_) + | EventPayload::Compaction(_) + | EventPayload::Overlay(_) + | EventPayload::Unknown(_) => { indices.push(turn); } } @@ -550,26 +569,27 @@ fn resolve_policies(max_turn: usize, compactions: &[crate::Compaction]) -> Vec, origins: &mut Vec, + event_ids: &mut EventIds, summary: &str, timestamp: DateTime, from: usize, to: usize, ) { let origin = TurnOrigin::Summary { from, to }; - events.push(InternalEvent::Event(Box::new(ConversationEvent::new( - TurnStart, timestamp, - )))); - origins.push(origin); - events.push(InternalEvent::Event(Box::new(ConversationEvent::new( - ChatRequest::from("[Summary of previous conversation]"), - timestamp, - )))); - origins.push(origin); - events.push(InternalEvent::Event(Box::new(ConversationEvent::new( - ChatResponse::message(summary), - timestamp, - )))); - origins.push(origin); + for event in [ + ConversationEvent::new(TurnStart, timestamp), + ConversationEvent::new( + ChatRequest::from("[Summary of previous conversation]"), + timestamp, + ), + ConversationEvent::new(ChatResponse::message(summary), timestamp), + ] { + events.push(InternalEvent { + event_id: event_ids.fresh(), + payload: EventPayload::Event(Box::new(event)), + }); + origins.push(origin); + } } /// Blank a tool call request's arguments. diff --git a/crates/jp_conversation/src/stream/projection_tests.rs b/crates/jp_conversation/src/stream/projection_tests.rs index aa93009ad..109858bb5 100644 --- a/crates/jp_conversation/src/stream/projection_tests.rs +++ b/crates/jp_conversation/src/stream/projection_tests.rs @@ -4,6 +4,7 @@ use chrono::{TimeZone as _, Utc}; use proptest::prelude::*; use serde_json::Map; +use super::assign_turn_indices; use crate::{ ByteSize, Compaction, ConversationEvent, ConversationStream, EventKind, PolicySpec, ReasoningPolicy, SummaryPolicy, ToolCallPolicy, @@ -2019,52 +2020,30 @@ proptest! { #[test] fn turn_indices_basic() { - use super::assign_turn_indices; - use crate::stream::InternalEvent; + let mut stream = ConversationStream::new_test(); + stream.extend([ + ConversationEvent::new(TurnStart, ts(0)), + ConversationEvent::new(ChatRequest::from("q1"), ts(0)), + ConversationEvent::new(TurnStart, ts(1)), + ConversationEvent::new(ChatRequest::from("q2"), ts(1)), + ConversationEvent::new(TurnStart, ts(2)), + ConversationEvent::new(ChatRequest::from("q3"), ts(2)), + ]); - let events = vec![ - InternalEvent::Event(Box::new(ConversationEvent::new(TurnStart, ts(0)))), - InternalEvent::Event(Box::new(ConversationEvent::new( - ChatRequest::from("q1"), - ts(0), - ))), - InternalEvent::Event(Box::new(ConversationEvent::new(TurnStart, ts(1)))), - InternalEvent::Event(Box::new(ConversationEvent::new( - ChatRequest::from("q2"), - ts(1), - ))), - InternalEvent::Event(Box::new(ConversationEvent::new(TurnStart, ts(2)))), - InternalEvent::Event(Box::new(ConversationEvent::new( - ChatRequest::from("q3"), - ts(2), - ))), - ]; - - let indices = assign_turn_indices(&events); + let indices = assign_turn_indices(&stream.events); assert_eq!(indices, vec![0, 0, 1, 1, 2, 2]); } #[test] fn turn_indices_with_implicit_leading_turn() { - use super::assign_turn_indices; - use crate::stream::InternalEvent; - - // Events before the first `TurnStart` form an implicit turn 0, so the first - // explicit turn is turn 1 — matching `IterTurns` (see - // `turn_index_with_implicit_leading_turn` in turn_iter_tests). - let events = vec![ - InternalEvent::Event(Box::new(ConversationEvent::new( - ChatRequest::from("orphan"), - ts(0), - ))), - InternalEvent::Event(Box::new(ConversationEvent::new(TurnStart, ts(1)))), - InternalEvent::Event(Box::new(ConversationEvent::new( - ChatRequest::from("q1"), - ts(1), - ))), - ]; - - let indices = assign_turn_indices(&events); + let mut stream = ConversationStream::new_test(); + stream.extend([ + ConversationEvent::new(ChatRequest::from("orphan"), ts(0)), + ConversationEvent::new(TurnStart, ts(1)), + ConversationEvent::new(ChatRequest::from("q1"), ts(1)), + ]); + + let indices = assign_turn_indices(&stream.events); assert_eq!(indices, vec![0, 1, 1]); } diff --git a/crates/jp_conversation/src/stream/turn_iter_tests.rs b/crates/jp_conversation/src/stream/turn_iter_tests.rs index 6abea1f09..daf182eca 100644 --- a/crates/jp_conversation/src/stream/turn_iter_tests.rs +++ b/crates/jp_conversation/src/stream/turn_iter_tests.rs @@ -26,7 +26,10 @@ fn turn_indices(stream: &ConversationStream) -> (Vec, Vec) { .iter_turns() .flat_map(|turn| turn.iter().map(|_| turn.index()).collect::>()) .collect(); - let direct = stream.iter_events_by_turn().map(|(turn, _)| turn).collect(); + let direct = stream + .iter_events_by_turn() + .map(|event| event.turn) + .collect(); (via_turns, direct) } diff --git a/crates/jp_conversation/src/stream/turn_mut.rs b/crates/jp_conversation/src/stream/turn_mut.rs index d84ba4810..2f2ef0fd6 100644 --- a/crates/jp_conversation/src/stream/turn_mut.rs +++ b/crates/jp_conversation/src/stream/turn_mut.rs @@ -2,7 +2,7 @@ use tracing::warn; -use super::{ConversationStream, InternalEvent, StreamError}; +use super::{ConversationStream, EventPayload, InternalEvent, StreamError}; use crate::{ ConversationEvent, EventKind, event::{ @@ -198,7 +198,7 @@ impl<'a> TurnMut<'a> { let events: &[InternalEvent] = &stream.events; let last_turn_start = events .iter() - .rposition(|e| matches!(e, InternalEvent::Event(ev) if ev.is_turn_start())); + .rposition(|e| matches!(&e.payload, EventPayload::Event(ev) if ev.is_turn_start())); last_turn_start.map_or(events, |pos| &events[pos..]) }; diff --git a/crates/jp_conversation/src/stream_tests.rs b/crates/jp_conversation/src/stream_tests.rs index 2730e802b..78cb4785e 100644 --- a/crates/jp_conversation/src/stream_tests.rs +++ b/crates/jp_conversation/src/stream_tests.rs @@ -18,6 +18,14 @@ use crate::{ resolve_range, }; +/// Wrap a payload with a fixed ID for serialization assertions. +fn fixed_entry(payload: EventPayload) -> InternalEvent { + InternalEvent { + event_id: EventId::fixed("entry01"), + payload, + } +} + /// A fixed timestamp for config delta events. fn delta_timestamp() -> DateTime { Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap() @@ -480,7 +488,34 @@ fn a_delta_carries_its_unsets_through_a_round_trip() { let json = serde_json::to_value(&delta).unwrap(); assert_eq!(json["unsets"], serde_json::json!(["editor.envs"])); - assert_eq!(deserialize_config_delta(&json).unwrap(), delta); + assert_eq!(config_delta::deserialize(&json).unwrap(), delta); +} + +/// A legacy delta carries its config fields beside the envelope keys, so +/// `event_id` sits among them and must not be read back as a config field. +/// +/// Reachable two ways — as the base config of a legacy file, and as a stream +/// entry in one — so both are checked here. +#[test] +fn a_legacy_delta_does_not_read_its_entry_id_as_config() { + let raw = serde_json::json!({ + "event_id": "entry01", + "type": "config_delta", + "timestamp": "2025-01-01 00:00:00.0", + "style": { "code": { "color": false } }, + }); + + let subtree = config_delta::subtree(&raw); + assert!( + subtree.get("event_id").is_none(), + "the wrapper's key leaked into the config: {subtree}" + ); + assert_eq!(subtree["style"]["code"]["color"], false); + + let ConfigDelta::Apply(apply) = config_delta::deserialize(&raw).unwrap() else { + panic!("expected an apply"); + }; + assert_eq!(apply.delta.style.code.color, Some(false)); } /// A delta written before clearing existed loads with nothing to clear. @@ -492,7 +527,7 @@ fn a_delta_without_unsets_loads_with_none() { "delta": { "style": { "code": { "color": false } } }, }); - let ConfigDelta::Apply(apply) = deserialize_config_delta(&json).unwrap() else { + let ConfigDelta::Apply(apply) = config_delta::deserialize(&json).unwrap() else { panic!("expected an apply"); }; @@ -511,13 +546,11 @@ fn an_unset_naming_no_field_does_not_stop_the_replay() { let mut partial = jp_config::PartialAppConfig::empty(); partial.style.code.color = Some(false); - stream - .events - .push(InternalEvent::ConfigDelta(ConfigDelta::Apply( - ApplyDelta::with_unsets(delta_timestamp(), partial, vec![ - "style.code.no_such_field".to_owned(), - ]), - ))); + stream.append(EventPayload::ConfigDelta(ConfigDelta::Apply( + ApplyDelta::with_unsets(delta_timestamp(), partial, vec![ + "style.code.no_such_field".to_owned(), + ]), + ))); assert!(!stream.config().unwrap().style.code.color); } @@ -844,19 +877,15 @@ fn test_to_parts_from_parts_roundtrip() { assert_eq!(stream, stream2); // Add some events and roundtrip again. - stream - .events - .push(InternalEvent::Event(Box::new(ConversationEvent::new( - ChatRequest::from("foo"), - Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), - )))); + stream.push_event(ConversationEvent::new( + ChatRequest::from("foo"), + Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), + )); - stream - .events - .push(InternalEvent::Event(Box::new(ConversationEvent::new( - ChatResponse::message("bar"), - Utc.with_ymd_and_hms(2020, 1, 2, 0, 0, 0).unwrap(), - )))); + stream.push_event(ConversationEvent::new( + ChatResponse::message("bar"), + Utc.with_ymd_and_hms(2020, 1, 2, 0, 0, 0).unwrap(), + )); let (base_config, events) = stream.to_parts().unwrap(); assert_eq!(events.len(), 2); @@ -1457,11 +1486,13 @@ fn test_sanitize_noop_on_healthy_stream() { /// Serialize a [`ConfigDelta`] as an [`InternalEvent`] and deserialize it back. fn roundtrip_delta(delta: ConfigDelta) -> ConfigDelta { - let event = InternalEvent::ConfigDelta(delta); + let event = fixed_entry(EventPayload::ConfigDelta(delta)); let json = serde_json::to_value(&event).unwrap(); - let deserialized: InternalEvent = serde_json::from_value(json).unwrap(); - match deserialized { - InternalEvent::ConfigDelta(d) => d, + let deserialized = serde_json::from_value::(json) + .unwrap() + .into_entry(); + match deserialized.payload { + EventPayload::ConfigDelta(d) => d, _ => panic!("expected ConfigDelta"), } } @@ -1535,12 +1566,14 @@ fn test_roundtrip_delta_strip_unknown_field_preserves_rest() { partial.style.code.color = Some(false); let original = ConfigDelta::from(partial); - let event = InternalEvent::ConfigDelta(original); + let event = fixed_entry(EventPayload::ConfigDelta(original)); let mut json = serde_json::to_value(&event).unwrap(); json["delta"]["style"]["code"]["removed_field"] = serde_json::json!("stale"); - let deserialized: InternalEvent = serde_json::from_value(json).unwrap(); - let InternalEvent::ConfigDelta(ConfigDelta::Apply(result)) = deserialized else { + let deserialized = serde_json::from_value::(json) + .unwrap() + .into_entry(); + let EventPayload::ConfigDelta(ConfigDelta::Apply(result)) = deserialized.payload else { panic!("expected Apply config delta"); }; assert_eq!(result.delta.style.code.color, Some(false)); @@ -1552,20 +1585,23 @@ fn test_internal_event_config_delta_reset_roundtrip() { timestamp: Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), }); - let event = InternalEvent::ConfigDelta(reset.clone()); + let event = fixed_entry(EventPayload::ConfigDelta(reset.clone())); let json = serde_json::to_value(&event).unwrap(); assert_eq!( json, serde_json::json!({ + "event_id": "entry01", "type": "config_delta", "op": "reset", "timestamp": "2020-01-01 00:00:00.0", }) ); - let deserialized: InternalEvent = serde_json::from_value(json).unwrap(); - assert_eq!(deserialized, InternalEvent::ConfigDelta(reset)); + let deserialized = serde_json::from_value::(json) + .unwrap() + .into_entry(); + assert_eq!(deserialized, fixed_entry(EventPayload::ConfigDelta(reset))); } #[test] @@ -1573,16 +1609,15 @@ fn test_internal_event_config_delta_apply_shape_has_no_op_field() { let mut partial = jp_config::PartialAppConfig::empty(); partial.style.code.color = Some(false); - let event = InternalEvent::ConfigDelta(ConfigDelta::Apply(ApplyDelta::new( - Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), - partial, + let event = fixed_entry(EventPayload::ConfigDelta(ConfigDelta::Apply( + ApplyDelta::new(Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), partial), ))); let json = serde_json::to_value(&event).unwrap(); let mut keys: Vec<_> = json.as_object().unwrap().keys().cloned().collect(); keys.sort(); - assert_eq!(keys, ["delta", "timestamp", "type"]); + assert_eq!(keys, ["delta", "event_id", "timestamp", "type"]); assert_eq!(json["type"], "config_delta"); assert_eq!(json["timestamp"], "2020-01-01 00:00:00.0"); assert_eq!(json["delta"]["style"]["code"]["color"], false); @@ -1598,8 +1633,10 @@ fn test_legacy_config_delta_without_op_decodes_as_apply() { "delta": { "style": { "code": { "color": false } } } }); - let internal: InternalEvent = serde_json::from_value(raw).unwrap(); - let InternalEvent::ConfigDelta(ConfigDelta::Apply(apply)) = internal else { + let internal = serde_json::from_value::(raw) + .unwrap() + .into_entry(); + let EventPayload::ConfigDelta(ConfigDelta::Apply(apply)) = internal.payload else { panic!("expected Apply config delta"); }; assert_eq!(apply.delta.style.code.color, Some(false)); @@ -1614,8 +1651,10 @@ fn test_config_delta_with_explicit_apply_op_decodes_as_apply() { "delta": { "style": { "code": { "color": false } } } }); - let internal: InternalEvent = serde_json::from_value(raw).unwrap(); - let InternalEvent::ConfigDelta(ConfigDelta::Apply(apply)) = internal else { + let internal = serde_json::from_value::(raw) + .unwrap() + .into_entry(); + let EventPayload::ConfigDelta(ConfigDelta::Apply(apply)) = internal.payload else { panic!("expected Apply config delta"); }; assert_eq!(apply.delta.style.code.color, Some(false)); @@ -1630,7 +1669,7 @@ fn test_config_delta_with_unknown_op_fails_deserialization() { "op": "unset", "timestamp": "2025-01-01 00:00:00.0", }); - assert!(serde_json::from_value::(raw).is_err()); + assert!(serde_json::from_value::(raw).is_err()); // Non-string values are rejected too. let raw = serde_json::json!({ @@ -1638,7 +1677,7 @@ fn test_config_delta_with_unknown_op_fails_deserialization() { "op": 42, "timestamp": "2025-01-01 00:00:00.0", }); - assert!(serde_json::from_value::(raw).is_err()); + assert!(serde_json::from_value::(raw).is_err()); } #[test] @@ -1724,7 +1763,7 @@ fn test_config_fold_reset_discards_accumulated_state() { fresh, )), ] { - stream.events.push(InternalEvent::ConfigDelta(delta)); + stream.append(EventPayload::ConfigDelta(delta)); } let config = stream.config().unwrap(); @@ -1746,25 +1785,19 @@ fn test_iter_config_reflects_reset() { fresh.user.name = Some("fresh".to_owned()); let mut stream = ConversationStream::new_test(); - stream - .events - .push(InternalEvent::ConfigDelta(ConfigDelta::Apply( - ApplyDelta::new(Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), dev), - ))); + stream.append(EventPayload::ConfigDelta(ConfigDelta::Apply( + ApplyDelta::new(Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), dev), + ))); stream.push(ConversationEvent::new( ChatRequest::from("before reset"), Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 1).unwrap(), )); - stream - .events - .push(InternalEvent::ConfigDelta(ConfigDelta::Reset(ResetDelta { - timestamp: Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 2).unwrap(), - }))); - stream - .events - .push(InternalEvent::ConfigDelta(ConfigDelta::Apply( - ApplyDelta::new(Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 3).unwrap(), fresh), - ))); + stream.append(EventPayload::ConfigDelta(ConfigDelta::Reset(ResetDelta { + timestamp: Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 2).unwrap(), + }))); + stream.append(EventPayload::ConfigDelta(ConfigDelta::Apply( + ApplyDelta::new(Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 3).unwrap(), fresh), + ))); stream.push(ConversationEvent::new( ChatRequest::from("after reset"), Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 4).unwrap(), @@ -1794,7 +1827,7 @@ fn test_deserialize_config_delta_extracts_timestamp_and_delta() { } }); - let delta = deserialize_config_delta(&value).unwrap(); + let delta = config_delta::deserialize(&value).unwrap(); assert_eq!(delta.timestamp().to_string(), "2025-01-01 00:00:00 UTC"); let ConfigDelta::Apply(apply) = delta else { panic!("expected Apply config delta"); @@ -1809,7 +1842,7 @@ fn test_deserialize_config_delta_preserves_timestamp_on_bad_delta() { "delta": "not an object at all" }); - let delta = deserialize_config_delta(&value).unwrap(); + let delta = config_delta::deserialize(&value).unwrap(); assert_eq!(delta.timestamp().to_string(), "2024-12-25 18:30:00 UTC"); let ConfigDelta::Apply(apply) = delta else { panic!("expected Apply config delta"); @@ -2664,7 +2697,7 @@ fn test_resolve_range_clamps_beyond_max() { #[test] fn test_internal_event_compaction_roundtrip() { let compaction = make_compaction(0, 5); - let event = InternalEvent::Compaction(compaction.clone()); + let event = fixed_entry(EventPayload::Compaction(compaction.clone())); let json = serde_json::to_value(&event).unwrap(); assert_eq!(json["type"], "compaction"); @@ -2672,8 +2705,10 @@ fn test_internal_event_compaction_roundtrip() { assert_eq!(json["to_turn"], 5); assert_eq!(json["reasoning"], "strip"); - let deserialized: InternalEvent = serde_json::from_value(json).unwrap(); - let InternalEvent::Compaction(result) = deserialized else { + let deserialized = serde_json::from_value::(json) + .unwrap() + .into_entry(); + let EventPayload::Compaction(result) = deserialized.payload else { panic!("expected Compaction"); }; assert_eq!(result, compaction); @@ -2700,7 +2735,7 @@ fn test_internal_event_overlay_roundtrip() { }], }; - let event = InternalEvent::Overlay(overlay.clone()); + let event = fixed_entry(EventPayload::Overlay(overlay.clone())); let json = serde_json::to_value(&event).unwrap(); assert_eq!(json["type"], "event_overlay"); @@ -2708,8 +2743,10 @@ fn test_internal_event_overlay_roundtrip() { assert_eq!(json["patches"][0]["matcher"]["value"], "stale"); assert_eq!(json["patches"][0]["action"]["action"], "remove_metadata"); - let deserialized: InternalEvent = serde_json::from_value(json).unwrap(); - let InternalEvent::Overlay(result) = deserialized else { + let deserialized = serde_json::from_value::(json) + .unwrap() + .into_entry(); + let EventPayload::Overlay(result) = deserialized.payload else { panic!("expected Overlay"); }; assert_eq!(result, overlay); @@ -2731,8 +2768,10 @@ fn test_internal_event_overlay_timestamp_accepts_storage_format() { }], }); - let deserialized: InternalEvent = serde_json::from_value(json).unwrap(); - let InternalEvent::Overlay(result) = deserialized else { + let deserialized = serde_json::from_value::(json) + .unwrap() + .into_entry(); + let EventPayload::Overlay(result) = deserialized.payload else { panic!("expected Overlay"); }; @@ -2745,7 +2784,7 @@ fn test_internal_event_overlay_timestamp_accepts_storage_format() { // And what it writes is the same format the siblings write, so a hand-edited // conversation round-trips byte-identically. - let json = serde_json::to_value(InternalEvent::Overlay(result)).unwrap(); + let json = serde_json::to_value(fixed_entry(EventPayload::Overlay(result))).unwrap(); assert_eq!(json["timestamp"], "2026-08-27 12:00:00.0"); } @@ -2815,7 +2854,7 @@ fn test_overlay_survives_turn_pruning() { stream .events .iter() - .any(|e| matches!(e, InternalEvent::Overlay(_))), + .any(|e| matches!(&e.payload, EventPayload::Overlay(_))), "pruning a turn must not drop the overlay" ); } @@ -2824,25 +2863,32 @@ fn test_overlay_survives_turn_pruning() { #[test] fn test_internal_event_known_kind_deserializes_as_event() { - let event = InternalEvent::Event(Box::new(ConversationEvent::now(ChatRequest::from("hi")))); + let event = fixed_entry(EventPayload::Event(Box::new(ConversationEvent::now( + ChatRequest::from("hi"), + )))); let json = serde_json::to_value(&event).unwrap(); - let internal: InternalEvent = serde_json::from_value(json).unwrap(); - assert!(matches!(internal, InternalEvent::Event(_))); + let internal = serde_json::from_value::(json) + .unwrap() + .into_entry(); + assert!(matches!(&internal.payload, EventPayload::Event(_))); } #[test] fn test_internal_event_preserves_unknown_kind_verbatim() { // An event written by a newer jp with a kind this build doesn't know. let raw = serde_json::json!({ + "event_id": "future1", "type": "unknown_future_kind", "timestamp": "2025-01-01 00:00:00.0", "summary": "a compacted summary", "metadata": { "foo": "YmFy" } }); - let internal: InternalEvent = serde_json::from_value(raw.clone()).unwrap(); - assert!(matches!(internal, InternalEvent::Unknown(_))); + let internal = serde_json::from_value::(raw.clone()) + .unwrap() + .into_entry(); + assert!(matches!(&internal.payload, EventPayload::Unknown(_))); // The payload round-trips byte-for-byte: no decode/re-encode is applied, // so the newer jp reads back exactly what it wrote. @@ -2859,6 +2905,7 @@ fn test_from_parts_tolerates_unknown_event_kind() { // A newer jp appended an event kind this build doesn't understand. let unknown = serde_json::json!({ + "event_id": "future1", "type": "unknown_future_kind", "timestamp": "2025-01-01 00:01:00.0", "summary": "a compacted summary" @@ -2937,9 +2984,25 @@ fn extend_into_empty_preserves_observed_iter_and_serialized_shape() { let dest_view: Vec<_> = dest.iter().map(|e| (e.event.clone(), e.config)).collect(); assert_eq!(source_view, dest_view); - // 2. The serialized storage shape must match. Extending an empty stream - // from a source must reproduce the source's on-disk form exactly. - let source_parts = source.to_parts().unwrap(); - let dest_parts = dest.to_parts().unwrap(); - assert_eq!(source_parts, dest_parts); + // 2. Each conversation event keeps the ID it had in the source: identity + // is per-stream, so a destination that has handed out no IDs takes the + // source's verbatim. + let source_ids: Vec<_> = source.iter().map(|e| e.event_id.to_string()).collect(); + let dest_ids: Vec<_> = dest.iter().map(|e| e.event_id.to_string()).collect(); + assert_eq!(source_ids, dest_ids); + + // 3. The serialized storage shape must match, apart from the config + // deltas' IDs. `Extend` recomputes a delta against the destination's + // running config state rather than copying the source's, so those + // entries are new entries and are assigned new IDs. + let (source_base, mut source_events) = source.to_parts().unwrap(); + let (dest_base, mut dest_events) = dest.to_parts().unwrap(); + for event in source_events.iter_mut().chain(&mut dest_events) { + let event = event.as_object_mut().unwrap(); + if event["type"] == "config_delta" { + event.shift_remove("event_id"); + } + } + assert_eq!(source_base, dest_base); + assert_eq!(source_events, dest_events); } diff --git a/crates/jp_ffi/src/display.rs b/crates/jp_ffi/src/display.rs index f013f21c8..c0d2eb3f5 100644 --- a/crates/jp_ffi/src/display.rs +++ b/crates/jp_ffi/src/display.rs @@ -92,14 +92,15 @@ pub(crate) fn project_turns(stream: &ConversationStream) -> Vec { // `iter_events_by_turn` rather than `iter_turns`: the latter resolves and // clones the accumulated config for every event and materializes the whole // stream up front, and none of that is read here. - for (index, event) in stream.iter_events_by_turn() { + for in_turn in stream.iter_events_by_turn() { + let index = in_turn.turn; let continues_run = in_message_run; in_message_run = matches!( - &event.kind, + &in_turn.kind, EventKind::ChatResponse(ChatResponse::Message { .. }) ); - let Some(event) = project_event(event) else { + let Some(event) = project_event(in_turn.event) else { continue; }; diff --git a/crates/jp_llm/src/test.rs b/crates/jp_llm/src/test.rs index f83fbe8aa..dfb3524f8 100644 --- a/crates/jp_llm/src/test.rs +++ b/crates/jp_llm/src/test.rs @@ -669,10 +669,10 @@ pub async fn run_chat_completion( all_events[index].push(TestEvent::Flushed(event.clone())); history.push(ConversationEventWithConfig { - event: event.clone(), + event_id: stream.push_event(event.clone()), + event, config: config.clone(), }); - stream.extend(std::iter::once(event)); } } Event::Patch(_) | Event::KeepAlive => {} @@ -684,10 +684,10 @@ pub async fn run_chat_completion( all_events[index].push(TestEvent::Flushed(event.clone())); history.push(ConversationEventWithConfig { - event: event.clone(), + event_id: stream.push_event(event.clone()), + event, config: config.clone(), }); - stream.extend(std::iter::once(event)); } all_events[index].push(TestEvent::Finished(reason)); @@ -727,7 +727,29 @@ pub async fn run_chat_completion( // ConversationStream doesn't implement Serialize directly; // decompose it via to_parts for the snapshot. let snap_value = conversation_stream.as_ref().map(|s| { - let (config, events) = s.to_parts().unwrap(); + let (config, mut events) = s.to_parts().unwrap(); + // An entry ID is host-assigned randomness rather than + // anything the provider returned, so it is dropped + // instead of recorded: a placeholder would add a line + // per entry that only ever reads `[event_id]`. + // + // What the snapshot would have checked is asserted + // here instead, which is stricter than a placeholder + // — the redaction that produced one could not tell a + // missing key from a present one. + for event in &mut events { + let event = + event.as_object_mut().expect("stored entry is an object"); + let event_id = event.shift_remove("event_id"); + assert!( + event_id + .as_ref() + .and_then(|id| id.as_str()) + .is_some_and(|id| !id.is_empty()), + "every stored entry carries a non-empty `event_id`, got \ + {event_id:?}" + ); + } serde_json::json!({ "base_config": config, "events": events }) }); Snap::json(snap_value) diff --git a/docs/.vitepress/rfd-summaries.json b/docs/.vitepress/rfd-summaries.json index 0190b591c..115fbe1c1 100644 --- a/docs/.vitepress/rfd-summaries.json +++ b/docs/.vitepress/rfd-summaries.json @@ -384,7 +384,7 @@ "summary": "Sanitize terminal output by allowlisting safe ANSI sequences (colors) and dropping dangerous ones (cursor, erase, OSC) to prevent escape injection attacks from untrusted content." }, "097-stable-event-identifiers.md": { - "hash": "2310623e9dc1f4d82e3c2ed79d48b2ba6a7ba6a8731b5a658bf17b17a3b6b82d", + "hash": "db6f239dc5efcf5e3f95c0e74f0ff4af81dc86e0d9d5c285954ac871e10f61f0", "summary": "Assign stable, stream-unique identifiers to every conversation event for safe manual editing and future reference features." }, "098-request-response-event-linking.md": { diff --git a/docs/architecture/query-stream-pipeline.md b/docs/architecture/query-stream-pipeline.md index 504878889..4ad82ccd3 100644 --- a/docs/architecture/query-stream-pipeline.md +++ b/docs/architecture/query-stream-pipeline.md @@ -227,7 +227,7 @@ The architecture uses existing types from the codebase: ```rust pub struct ConversationEvent { - pub timestamp: UtcDateTime, + pub timestamp: DateTime, pub kind: EventKind, pub metadata: Map, } @@ -256,11 +256,64 @@ pub enum ChatResponse { ```rust pub struct ConversationStream { base_config: Arc, - events: Vec, // ConfigDelta or ConversationEvent - pub created_at: UtcDateTime, + events: Vec, + event_ids: EventIds, + duplicated_event_ids: HashSet, + pub created_at: DateTime, +} + +struct InternalEvent { + event_id: EventId, + payload: EventPayload, +} + +enum EventPayload { + Event(Box), + ConfigDelta(ConfigDelta), + Compaction(Compaction), + Overlay(EventOverlay), + Unknown(Value), } ``` +The wrapper carries the entry's [Event ID] and the payload carries its +timestamp, and the two serialize into one object: + +```json +{"event_id":"k3m9x2a","timestamp":"2026-05-03 12:00:00.0","type":"chat_request","content":"Hello"} +``` + +Identity is the stream's to assign and the payload's timestamp is its producer's +to report, which is why the two sit on different halves. +`event_ids` holds every ID the stream has handed out, so an ID is retired with +the entry that held it rather than returned to circulation. +`duplicated_event_ids` records the IDs a load found duplicated, which a +reference-bearing feature reads to tell an ambiguous reference from a resolvable +one. +Neither is serialized. + +A stored entry reads as a `StoredEvent`, whose `event_id` is `Option`: a legacy +entry carries none, and a hand-edited file can give two entries the same one. +`from_parts` settles those IDs as it builds the stream — assigning one to every +entry that lacks one, and to every entry after the first to carry a given ID — +and `from_legacy_events` delegates to it. +An `InternalEvent` therefore only exists inside a stream, holding an ID that +stream handed out, which is what makes uniqueness a property of the type rather +than of a pass someone has to remember to run. +This is separate from `sanitize()`, which repairs stream structure rather than +identity, and which sees IDs that are already settled. + +The module is split by concern: `stream::entry` holds `InternalEvent`, +`EventPayload`, `StoredEvent`, and the hand-rolled serde for all three; +`stream::config_delta` holds `ConfigDelta` and the folding that resolves a +conversation's config; `stream::iter` holds the three iterators and the +`…WithConfig` views they yield; `stream::projection` builds the provider-facing +view. +`ConversationStream` itself is in `stream`, in two `impl` blocks: the stream's +own behavior, and the storage boundary that reads and writes stored JSON. + +See [RFD 097] for the rules a consumer of these IDs is held to. + **`Thread`** (`jp_conversation::thread`): ```rust @@ -274,7 +327,8 @@ pub struct Thread { The pipeline builds `ConversationEvent` instances and pushes them to `ConversationStream`. -Persistence serializes `ConversationStream` to disk. +`ConversationStream::to_parts` provides the base configuration and serialized +entries for storage in `base_config.json` and `events.json`, respectively. ----- @@ -1898,4 +1952,6 @@ This architecture addresses the key issues in the current implementation: The migration can be done incrementally, with each phase adding tests and maintaining backward compatibility until the final cleanup. +[Event ID]: ubiquitous-language/conversation.md#event-id +[RFD 097]: ../rfd/097-stable-event-identifiers.md [architecture.md]: architecture.md diff --git a/docs/architecture/ubiquitous-language/conversation.md b/docs/architecture/ubiquitous-language/conversation.md index 97bf4b4af..9741ee802 100644 --- a/docs/architecture/ubiquitous-language/conversation.md +++ b/docs/architecture/ubiquitous-language/conversation.md @@ -6,6 +6,8 @@ Six terms describe its structure — a **Conversation** is the stored entity, a **Turn** is one slice of it, an **Event** is the atomic unit inside a Turn, **Tool Calls** and **Inquiries** are specific event kinds, and a **Thread** is the projection of a Conversation that gets sent to an LLM provider. +An **Event ID** names one entry in the log, so a reference to it survives edits +that move the entry. These terms are tightly coupled — paraphrasing one usually breaks the model for another. @@ -17,8 +19,8 @@ annotated. second. > [!NOTE] -> Cluster status: **Turn**, **Active Conversation**, **Live Conversation**, -> **Archived Conversation**, and **Label** are defined below. +> Cluster status: **Turn**, **Event ID**, **Active Conversation**, **Live +> Conversation**, **Archived Conversation**, and **Label** are defined below. > The remaining terms are placeholders and will land in subsequent passes. > Until then, see the [legacy single-page glossary] for the older definitions of > the unfilled terms. @@ -149,6 +151,47 @@ keeps one in `metadata.json`, a ticket writes one `- **Label**:` line per pair. When you mean a `key=value` pair stored on a conversation or a ticket, the word is **Label**. +### Event ID + +The identity of one entry in a conversation's event log, unique within that +stream. +It belongs to the entry rather than to the entry's position, so editing an +entry's content or moving it leaves its Event ID alone. +Every entry has one, whatever it holds. + +**Implementation.** `EventId` in `jp_conversation`, held by the stream-entry +wrapper and persisted as `event_id`. +A stream hands out its own IDs, and a removed entry takes its ID with it for as +long as that stream is in memory. +Retired IDs are not persisted, so the guarantee is per-load rather than for all +time. + +**In context.** The point of an intrinsic identity is that a reference to an +entry survives the edits a reference to "the third entry" would not. +A duplicate introduced by hand-editing is repaired on load, but the duplicated +value is reported as ambiguous rather than silently bound to whichever entry +kept it: a reference into an edited file resolves to the right entry or to +nothing, never to the wrong one. +Projection builds a provider-facing copy whose synthetic entries carry IDs of +the same type, but those exist only in that copy and are not references into the +stored stream. + +**Not the same as.** The payload `id` on a tool call or inquiry, which pairs a +request with its response and means something to the provider. +An entry can carry both, and they answer different questions. +Also not a **Conversation**'s id, which names the whole log rather than one +entry in it. + +**Scope.** An Event ID carries identity alone: no ordering, no content, and no +uniqueness beyond its own stream. +Two conversations may hold the same value, and code must not read one for +anything but identity-within-a-stream. +See [RFD-097]. + +**Avoid.** *Event key*, *entry hash*, *event index*. +The last is actively wrong — an index is the position an Event ID exists to +replace. + ### Turn A contiguous group of conversation events bracketed by a `TurnStart`: one user @@ -175,5 +218,6 @@ None of these are project terms. When you mean a single user-prompt-to-final-response cycle with the assistant, the word is **Turn**. +[RFD-097]: ../../rfd/097-stable-event-identifiers.md [`jp_label`]: https://github.com/dcdpr/jp/tree/main/crates/jp_label [legacy single-page glossary]: ../ubiquitous-language.md diff --git a/docs/architecture/ubiquitous-language/index.md b/docs/architecture/ubiquitous-language/index.md index 530a75542..39aa9986e 100644 --- a/docs/architecture/ubiquitous-language/index.md +++ b/docs/architecture/ubiquitous-language/index.md @@ -54,9 +54,9 @@ In disagreements between code and this document, the code is authoritative. ## Clusters -- [**Conversation**] — `Conversation`, `Turn`, `Event`, `Tool Call`, `Inquiry`, - `Thread`, `Active Conversation`, `Live Conversation`, `Archived Conversation`, - `Label`. +- [**Conversation**] — `Conversation`, `Turn`, `Event`, `Event ID`, `Tool + Call`, `Inquiry`, `Thread`, `Active Conversation`, `Live Conversation`, + `Archived Conversation`, `Label`. The user-facing notion of "talking to the assistant" and the event log that backs it. @@ -70,6 +70,7 @@ Each entry links to the cluster where it lives. - **Active Conversation** → [Conversation › Active Conversation] - **Archived Conversation** → [Conversation › Archived Conversation] +- **Event ID** → [Conversation › Event ID] - **Label** → [Conversation › Label] - **Live Conversation** → [Conversation › Live Conversation] - **Turn** → [Conversation › Turn] @@ -77,6 +78,7 @@ Each entry links to the cluster where it lives. [**Conversation**]: ./conversation.md [Conversation › Active Conversation]: ./conversation.md#active-conversation [Conversation › Archived Conversation]: ./conversation.md#archived-conversation +[Conversation › Event ID]: ./conversation.md#event-id [Conversation › Label]: ./conversation.md#label [Conversation › Live Conversation]: ./conversation.md#live-conversation [Conversation › Turn]: ./conversation.md#turn diff --git a/docs/rfd/.priority.json b/docs/rfd/.priority.json index 78bc16524..61fe4a6cf 100644 --- a/docs/rfd/.priority.json +++ b/docs/rfd/.priority.json @@ -10,7 +10,6 @@ "D15", "D32", "065", - "097", "098", "070", "026", diff --git a/docs/rfd/097-stable-event-identifiers.md b/docs/rfd/097-stable-event-identifiers.md index f9b082f2e..1ec917a28 100644 --- a/docs/rfd/097-stable-event-identifiers.md +++ b/docs/rfd/097-stable-event-identifiers.md @@ -1,6 +1,6 @@ # RFD 097: Stable Event Identifiers -- **Status**: Discussion +- **Status**: Implemented - **Category**: Design - **Authors**: Jean Mertz - **Date**: 2026-05-03 @@ -221,36 +221,77 @@ The event constructors are unchanged: `ConversationEvent::new(kind, ts)`, `ConversationEvent::now(kind)`, and the `ConfigDelta` constructors keep their current signatures and gain no ID argument. -The mutation entry points that append to the stream generate the ID: +The stream holds the set of IDs it has handed out, and that set is the one place +"unique within its stream" is enforced: ```rust +/// The entry IDs one conversation stream has handed out. +struct EventIds(HashSet); + +impl EventIds { + /// Take `preferred`, or a generated ID when this set already holds it. + fn claim(&mut self, preferred: EventId) -> EventId; + + /// A generated ID this set has not handed out. + fn fresh(&mut self) -> EventId; +} + impl ConversationStream { - fn wrap(&self, payload: EventPayload) -> InternalEvent { - InternalEvent { event_id: self.fresh_event_id(), payload } - } - - /// A random `EventId` that does not collide with any entry already in the - /// stream. - /// The stream knows the existing IDs, so this is where "unique within its - /// stream" is enforced. - fn fresh_event_id(&self) -> EventId { /* retry random() until unused */ } + /// Append a payload, returning the ID the stream assigned it. + fn append(&mut self, payload: EventPayload) -> EventId; + + /// Append an entry from another stream, keeping its ID when free. + fn adopt(&mut self, entry: InternalEvent) -> EventId; } ``` -The invariant: every path that creates or inserts an `InternalEvent` goes -through this wrap constructor; deserialization is the only path that preserves -an existing ID. +Insertion returns the assigned ID, so a caller that needs to refer to the entry +it just wrote does not have to read it back out of the stream. + +The invariant: every path that adds an `InternalEvent` to a stream goes through +the stream, which owns the set of IDs it has handed out. The known call sites are `push`, `add_config_delta`, `add_compaction`, `extend`, -`TurnMut::build`, `start_turn`, the synthetic insertions in +`append_stream`, `TurnMut::build`, `start_turn`, the synthetic insertions in `normalize_turn_starts` and `sanitize_orphaned_tool_calls`, and projection's injected entries (ephemeral; see [Projection views](#projection-views)). Because generation happens where stream context exists, "unique within its stream" holds at insertion, not merely after a load-time pass. -A fixture or test that needs a deterministic ID constructs the `InternalEvent` -wrapper directly with a fixed `EventId`; live and synthetic insertion paths let -the stream assign one. -There is no thread-local RNG and no test/prod plumbing inside the event -constructors. + +An ID is retired with the entry that held it: for as long as the stream is in +memory, it does not hand out an ID again after the entry holding it is removed. +Otherwise a reference to a deleted entry could silently rebind to a later, +unrelated one — the positional aliasing this RFD exists to remove, reintroduced +through the ID. + +Retirement lasts the lifetime of that in-memory stream, and no longer. +Retired IDs are not persisted, so a load rebuilds the set from the entries the +file still carries, and a later draw can in principle land on an ID a previous +session deleted. +The draw would have to hit that one value out of 36^7, so nothing is done about +it: persisting tombstones would grow `events.json` without bound to guard a +collision no input can force. +A feature that resolves references should not read cross-load non-reuse into +this paragraph. + +**An entry arriving from another stream keeps its ID.** Uniqueness is scoped to +a single stream (see [Non-Goals](#non-goals)), so an entry copied between +streams can carry its identity with it, and a reference resolved against the +source resolves against the copy. +Only a collision with an ID the destination has already handed out forces a new +one. +This is what makes `jp conversation fork` produce a stream whose entries are +still addressable by the IDs the user saw in the source. + +One case is not a copy and does not preserve: the config deltas +`Extend` writes are *recomputed* against the +destination's running config state rather than carried over, so they are new +entries and are assigned new IDs. + +A fixture or test that needs a deterministic ID builds the `InternalEvent` +wrappers and hands them to the stream, which registers their IDs; live and +synthetic insertion paths let the stream assign one. +There is no thread-local RNG, and no generator argument on any production +signature. Synthetic stream entries that must keep a specific timestamp (for example, a `TurnStart` that adopts the first chat request's time, or a synthetic @@ -269,14 +310,34 @@ any other ID. ### Projection views `event_id` stability is a property of the persisted raw stream. +Its IDs can be used as stable references to entries in `events.json`. Compaction projection builds an ephemeral provider view by transforming a copy -of the stream, injecting synthetic entries (for example the summary -`ChatRequest` / `ChatResponse` pair) that exist only in that view. -Those entries are wrapped like any other and receive fresh IDs, but the IDs are +of the stream; it does not remove the original entries from the persisted raw +stream. + +An existing entry retained in the projected view keeps its original `event_id`, +including when a mechanical policy changes its projected content. +That ID still identifies the original entry in `events.json`, not a separate +stored version of the projected content. + +Entries synthesized for the projection, such as the summary `TurnStart` and +`ChatRequest` / `ChatResponse` pair, exist only in that view. +They have no corresponding entries in `events.json`. +They are wrapped like any other entry and receive fresh IDs, but those IDs are ephemeral: they carry no stability contract across projections, are never persisted or exposed through storage or plugin APIs, and MUST NOT be used as references into `events.json`. +The synthetic IDs are drawn from the projected stream's own ID set, not from a +set local to projection. +That keeps two properties at once: a synthetic entry cannot take the ID of an +entry projection carried through, and the stream is left holding every ID its +entries carry, so a later insertion cannot hand out one of them again. + +Both stored entries and projection-only entries use `EventId`. +The ID value alone does not indicate whether a corresponding stored entry +exists. + ### Dependency `EventId::random()` requires a source of randomness, which the workspace does @@ -300,18 +361,37 @@ The append APIs stay infallible. ### Storage-layer repair -ID-uniqueness repair runs inside `ConversationStream::from_parts` and -`from_legacy_events`, immediately after deserialization, before the stream is -returned. -It is **not** part of `ConversationStream::sanitize()`, which is reserved for -higher-level stream mutations (orphaned tool responses, orphaned inquiry -responses, leading non-user events, turn-start normalization). +A stored entry's `event_id` is read as *optional*: a legacy entry has none, and +a hand-edited file can give two entries the same one. +The wire form (`StoredEvent`) therefore carries `Option`, and +`ConversationStream::from_parts` settles every entry's identity as it builds the +stream, before it is returned. +`from_legacy_events` delegates to `from_parts`. + +Making the wire form's ID optional is what keeps "unique within its stream" a +property of the type rather than of a pass that has to be remembered: an +`InternalEvent` only exists inside a stream, and its ID is one that stream +handed out. +A future load path cannot skip the step, because there is nothing else that +turns a `StoredEvent` into a stream entry. ```txt -collect ids; for each duplicate id, regenerate it on the later occurrence(s) -so the stream again has unique ids; record which ids were duplicated. +reserve every id the file carries; +for each entry, in order: + no id -> assign a generated one + id not seen yet -> keep it + id already seen -> assign a generated one, and record the id as duplicated ``` +Reserving the whole file first is what stops a generated ID taking one that +belongs to an entry further down the file. + +This is **not** part of `ConversationStream::sanitize()`, which is reserved for +higher-level stream mutations (orphaned tool responses, orphaned inquiry +responses, leading non-user events, turn-start normalization). +`sanitize` also could not do the job: it sees a stream whose IDs are already +settled, not the IDs a file carried. + Repair restores the *uniqueness* invariant, but it cannot restore reference *intent*. A manual edit that duplicates an ID (a copy-paste, or a reorder that moves a @@ -331,13 +411,23 @@ This is what makes the Motivation's claim hold: a copy, reorder, or delete produces a *detectable* mismatch, never a silent positional rebind to the wrong entry. -The set of duplicated IDs is retained as private, load-scoped state on -`ConversationStream`, populated by the repair pass and consulted by future -reference resolution to classify a reference as ambiguous. +The set of duplicated IDs is load-scoped state on `ConversationStream`, recorded +as the IDs are settled and read through `duplicated_event_ids()` by reference +resolution to classify a reference as ambiguous. Once the repaired stream is saved and reloaded, the file has unique IDs and the set is empty. -No public accessor is added until a consumer exists, consistent with the -`event_ids()` stance below. + +The record covers the load that produced it, and does not travel with the +entries. +A stream that takes entries from another, through `append_stream` or `Extend`, +reports only what its own load found: the ambiguity is a property of the file +that was read, and `jp conversation fork` writes a file with unique IDs. +A consumer resolving a reference therefore asks the stream that read the file +the reference names, not a copy made from it. +Unioning the record into the destination is deliberately not done: no consumer +exists yet to say whether inherited ambiguity should read as ambiguous, and +`Extend` recomputes config deltas rather than copying them, so the two paths do +not share one answer. RFDs that introduce reference-bearing entries must consume this recorded ambiguity in the same load cycle, before the stream is persisted: resolve or @@ -361,11 +451,13 @@ if overlay.anchor_id was duplicated at load, or overlay.anchor_id ∉ { id of every stream entry } { drop overlay } ``` -This RFD does not add a public `event_ids()` accessor; there is no consumer yet. -The ID set is an internal notion the repair pass already computes, and the -public API can grow an accessor when a real consumer (compaction, future -sub-agent provenance features, plugin subscriptions, interactive editing) -defines the shape it needs. +This RFD does not add a public accessor for the *set of live entry IDs*; there +is no consumer yet. +The stream computes it to enforce uniqueness, and the public API can grow an +accessor when a real consumer (compaction, future sub-agent provenance features, +plugin subscriptions, interactive editing) defines the shape it needs. +`duplicated_event_ids()` is public because the ambiguity it reports has a +deadline: it must be read in the same load cycle, before a save discards it. ### Storage @@ -535,10 +627,12 @@ Mergeable on its own. Confirm a byte-for-byte round-trip against existing fixtures, modulo the new key. - Assign `event_id` on insertion: route every `InternalEvent` creation through - the wrap constructor (`push`, `add_config_delta`, `add_compaction`, `extend`, - `TurnMut::build`, `start_turn`, and the synthetic insertions in - `normalize_turn_starts` / `sanitize_orphaned_tool_calls`), backed by a - `fresh_event_id()` that avoids collision with IDs already in the stream. + the stream (`push`, `add_config_delta`, `add_compaction`, `extend`, + `append_stream`, `TurnMut::build`, `start_turn`, and the synthetic insertions + in `normalize_turn_starts` / `sanitize_orphaned_tool_calls`), backed by an ID + set the stream owns. + An entry arriving from another stream keeps its ID unless the destination has + already handed that ID out. - Expose `event_id` on the iteration views, for the `ConversationEvent`s they already yield; config deltas, compactions, and unknown entries gain no new programmatic surface (they remain ID-addressable in the persisted JSON). @@ -554,18 +648,19 @@ part. ### Phase 3 — Storage-layer uniqueness repair -- Add `ensure_unique_event_ids` in `ConversationStream`, recording which IDs - were duplicated in a private, load-scoped field on the stream (no public - accessor until a consumer exists). -- Call it from `from_parts` and `from_legacy_events` after deserialization, - before the stream is returned. +- Give the wire form (`StoredEvent`) an `Option`, so an entry read from + storage has no identity until a stream settles it. +- Settle every entry's ID in `from_parts` as the stream is built, recording the + duplicated values in a load-scoped field read through + `duplicated_event_ids()`. + `from_legacy_events` delegates to `from_parts`. **Not** part of `sanitize()`. -- Test: a stream with two entries sharing an explicit fixed ID; repair - regenerates the later occurrence and the in-memory stream has unique IDs. +- Test: a stream with two entries sharing an explicit fixed ID; the first keeps + it, the second is reassigned, and the value is reported as duplicated. - Test: a legacy file with no `event_id` fields; entries get IDs assigned at load. -- Test: a live insertion path never exposes a duplicate ID; force a collision in - `fresh_event_id` and verify it retries. +- Test: a scripted generator forced to repeat an ID the stream holds; the draw + retries. Depends on Phase 2. diff --git a/docs/ticket/0jrnyy1-collapse-the-duplicate-getrandom-versions-in-the-dependency.md b/docs/ticket/0jrnyy1-collapse-the-duplicate-getrandom-versions-in-the-dependency.md new file mode 100644 index 000000000..eceec09e0 --- /dev/null +++ b/docs/ticket/0jrnyy1-collapse-the-duplicate-getrandom-versions-in-the-dependency.md @@ -0,0 +1,26 @@ +# Collapse the duplicate getrandom versions in the dependency graph + +- **Status**: Todo +- **Kind**: Chore +- **Authors**: jp +- **Date**: 2026-09-14 +- **Label**: domain=tooling +- **Label**: package=jp_conversation +- **Label**: type=task + +`.clippy.toml` exempts `getrandom` from `allowed-duplicate-crates`, because the +graph carries both 0.2 and 0.3: + +- `ring` depends on 0.2. +- `rand_core` and `jp_conversation` (for `EventId::random`) depend on 0.3. + +Nothing in the workspace can collapse this today: `ring` reaches us transitively +and pins 0.2 itself. + +Drop the exemption once the transitive dependencies converge on one version. +Checking whether they have is a matter of running `cargo tree --invert --package +getrandom` and seeing whether anything still pulls 0.2. + +The exemption is cheap and correct, so this is bookkeeping rather than a +problem. +It is filed so the entry does not outlive the reason for it.