From bf3479f5c44a6e0315f6f1e671022e35e31cd28b Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Sun, 6 Sep 2026 13:00:37 -0400 Subject: [PATCH] client-core: an unasked page is not an empty one, and two guarded invariants Review of 73251d6's port of TranscriptSource/joinPages. `TranscriptSource::page` answered `before == 0` with an empty `Vec`, which is the same value it answers "this conversation has no more history" with. That is the state the Kotlin keeps apart: `loadOlderPage` returns false at `oldestSeq == 0` *without* touching `moreHistory`, and returns false on an empty page *by latching it*. Collapsing the two moved AGENTS.md's paging bug one layer down rather than fixing it. `page` returns `OlderPage` now -- `Events(vec![])` is the start of the conversation, `NothingLoaded` is not an answer about the conversation at all. `join_pages`' `debug_assert!` on seq ordering across the boundary is not a true invariant: a peer note carries the seq its turn began at, which can be older than the page it arrived in, so an ordinary transcript would have panicked a debug build there. Replaced with the one the function exists to enforce -- no tool id surviving in both halves. `fetch_transcript_lines` stores `RawValue`'s exact server bytes, so the "neither source can produce a newline" comment in `SessionCache::append` now rests on the server's serializer staying compact rather than on a local normalization. Checked with a `debug_assert!` in `append` and `store_page` rather than trusted. Tests for the failure half, which the port had none of: a 500 mid-page, a cached line this build cannot read, and the `after` bound in the case that actually carries one (the existing test asserted only the case with no bound). `cargo fmt`, `cargo clippy --all-targets`, `cargo test` (112) clean in client-core; `cargo check -p desktop-app` clean. --- client-core/src/transcript_cache.rs | 16 +++- client-core/src/transcript_fold.rs | 22 +++-- client-core/src/transcript_source.rs | 135 +++++++++++++++++++++------ docs/CLIENT_CORE.md | 36 +++++-- 4 files changed, 164 insertions(+), 45 deletions(-) diff --git a/client-core/src/transcript_cache.rs b/client-core/src/transcript_cache.rs index a0ec342..7702989 100644 --- a/client-core/src/transcript_cache.rs +++ b/client-core/src/transcript_cache.rs @@ -361,6 +361,10 @@ impl SessionCache { { return Ok(false); } + debug_assert!( + lines.iter().all(|l| !l.contains('\n')), + "a stored page's lines must each be one line" + ); fs::create_dir_all(&this.dir)?; let kind = if rows { "rows" } else { "raw" }; let mut content = lines.join("\n"); @@ -389,8 +393,16 @@ impl SessionCache { return Ok(()); }; // Written as it arrived. A newline inside it would split one - // event into two unreadable halves, but neither source can - // produce one. + // event into two unreadable halves. No source here can produce + // one -- an SSE `data:` field cannot hold a raw newline, and a + // fetched line is one element of a compact JSON array -- but + // that is a fact about the *server's* serializer rather than + // anything this file controls, so it is checked rather than + // trusted. + debug_assert!( + !line.contains('\n'), + "a cached transcript line must be one line: {line}" + ); use std::io::Write; writer.write_all(line.as_bytes())?; writer.write_all(b"\n")?; diff --git a/client-core/src/transcript_fold.rs b/client-core/src/transcript_fold.rs index d7c0192..a4cf03c 100644 --- a/client-core/src/transcript_fold.rs +++ b/client-core/src/transcript_fold.rs @@ -384,15 +384,23 @@ pub fn join_pages(earlier: &[TranscriptItem], later: &[TranscriptItem]) -> Vec true, }) .collect(); - debug_assert!( - match (healed.last(), kept.first()) { - (Some(last), Some(first)) => last.seq() <= first.seq(), - _ => true, - }, - "a joined page must stay ordered by seq at the boundary" - ); let mut out = adopt_run(&healed, &kept); out.extend(kept); + // What this function exists to prevent, checked rather than assumed: the same + // call drawn twice, once from the page that saw its start and once from the page + // that saw its end. Not a seq-ordering check -- a peer note is stamped with the + // seq its turn began at, which can be older than the page it arrived in, so the + // two pages' seqs legitimately interleave at the boundary. + debug_assert!( + { + let mut ids: Vec<&str> = out.iter().filter_map(TranscriptItem::as_tool_run).collect(); + let before = ids.len(); + ids.sort_unstable(); + ids.dedup(); + ids.len() == before + }, + "join_pages left the same tool call in both halves" + ); out } diff --git a/client-core/src/transcript_source.rs b/client-core/src/transcript_source.rs index 296df84..1bb0ee9 100644 --- a/client-core/src/transcript_source.rs +++ b/client-core/src/transcript_source.rs @@ -70,6 +70,26 @@ impl From for PageError { } } +/// What [`TranscriptSource::page`] found, kept as two states rather than +/// one possibly-empty list. +/// +/// The difference is the whole of AGENTS.md's `loadOlderPage` incident: an +/// empty [`Self::Events`] means "this conversation has no more history", +/// which a caller is meant to latch, and [`Self::NothingLoaded`] means the +/// question could not be asked yet, which it must not. Collapsing the two +/// into an empty `Vec` puts the bug back, because the caller cannot tell +/// them apart -- and `unwrap_or_default()` on an `Option` would do the +/// same silently. +#[derive(Debug, Clone, PartialEq)] +pub enum OlderPage { + /// The events before the cursor, oldest first. Empty means the start + /// of the conversation has been reached. + Events(Vec), + /// Nothing is loaded, so there was no cursor to page back from + /// (`before == 0`). Not an answer about the conversation at all. + NothingLoaded, +} + fn parse_line(line: &str) -> Result { serde_json::from_str(line).map_err(|e| ParseError(format!("{e}"))) } @@ -174,35 +194,30 @@ impl TranscriptSource { /// The page before `before`: from the cache when it holds it, /// otherwise from the server bounded by what the cache already has. /// - /// `before == 0` always answers an empty page without asking the - /// cache or the server anything -- see AGENTS.md's "things that have - /// bitten": there is no event before the first one, so a page request - /// there is not a harmless no-op, it is indistinguishable from having - /// reached the start of history and would latch a caller's "there is - /// more" flag false forever. Guarded here rather than left to every - /// caller, because it is a fact about the question, not about who is - /// asking it. - /// /// The server bound (`after`) is what keeps the cache worth having. A /// coalesced page reaches back as far as its row count takes it -- a /// single reply is hundreds of lines -- so a page fetched after the /// reader has been away could run straight past the cached run and /// overlap it, and an overlapping page cannot be stored. Told where /// this phone's copy starts, the server stops there instead. - pub fn page( - &self, - before: u64, - limit: u32, - coalesce: bool, - ) -> Result, PageError> { + /// + /// `before == 0` answers [`OlderPage::NothingLoaded`] without asking + /// the cache or the server anything -- see AGENTS.md's "things that + /// have bitten": there is no event before the first one, so the + /// request is not a harmless no-op, and its empty answer is + /// indistinguishable from having reached the start of history. + /// Guarded here rather than left to every caller, because it is a fact + /// about the question, not about who is asking it. + pub fn page(&self, before: u64, limit: u32, coalesce: bool) -> Result { if before == 0 { - return Ok(Vec::new()); + return Ok(OlderPage::NothingLoaded); } if let Some(lines) = self.cache.page(before, limit as usize, coalesce) { - return lines + let events: Vec = lines .iter() .map(|l| parse_line(l).map_err(PageError::from)) - .collect(); + .collect::>()?; + return Ok(OlderPage::Events(events)); } let after = self.cache.covered_up_to(before).map(|v| v - 1); let page = self.api.fetch_transcript_lines( @@ -220,7 +235,9 @@ impl TranscriptSource { self.cache .store_page(&lines, first_event.seq, before, coalesce); } - Ok(page.into_iter().map(|(_, event)| event).collect()) + Ok(OlderPage::Events( + page.into_iter().map(|(_, event)| event).collect(), + )) } /// [`event_stream::follow_session_events`], with every frame written to @@ -421,8 +438,7 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let transport = ScriptedTransport::default(); let source = source(transport, dir.path()); - let page = source.page(0, 80, true).unwrap(); - assert!(page.is_empty()); + assert_eq!(source.page(0, 80, true).unwrap(), OlderPage::NothingLoaded); assert_eq!(source.api.transport().call_count(), 0); } @@ -435,7 +451,9 @@ mod tests { source.fetch_opening().unwrap(); let calls_before = source.api.transport().call_count(); - let page = source.page(2, 10, true).unwrap(); + let OlderPage::Events(page) = source.page(2, 10, true).unwrap() else { + panic!("a cursor of 2 is a real question about the conversation"); + }; assert_eq!(page.len(), 1); assert_eq!(page[0].seq, 1); assert_eq!( @@ -445,17 +463,16 @@ mod tests { ); } + /// With nothing older cached there is no floor to give the server, so + /// the request carries no `after` at all. #[test] - fn a_server_page_is_bounded_by_what_the_cache_already_covers() { + fn a_server_page_with_nothing_older_cached_carries_no_bound() { let dir = tempfile::tempdir().unwrap(); let transport = ScriptedTransport::default(); transport.respond(200, format!("[{}]", status_line(5))); let source = source(transport, dir.path()); source.fetch_opening().unwrap(); - // The cache now covers seq 5 onward with nothing older, so covered_up_to(5) - // is None (nothing stored below it) -- fetch a page further back and confirm - // the request the cache-less path makes carries no `after` in that case, then - // a second page that the cache *does* bound. + let transport2 = ScriptedTransport::default(); transport2.respond(200, format!("[{}]", status_line(3))); let cache = crate::transcript_cache::TranscriptCache::new(dir.path()).session("s1"); @@ -467,6 +484,70 @@ mod tests { ); } + /// The half the test above cannot show: when the cache *does* hold an + /// older run, the fetch is floored at its end, or the page would run + /// straight past it and overlap -- which `store_page` then refuses, + /// silently costing the phone the page it just paid for. + #[test] + fn a_server_page_is_floored_at_the_end_of_the_cached_run() { + let dir = tempfile::tempdir().unwrap(); + let cache = crate::transcript_cache::TranscriptCache::new(dir.path()).session("s1"); + // A stored page covering [3, 6) and two live events above it, so the run this + // phone holds is [3, 8) -- the newest chunk has to be an appended one, or the + // cache reads the directory as damaged and discards it. + let lines: Vec = (3..6).map(status_line).collect(); + assert!(cache.store_page(&lines, 3, 6, true)); + cache.append(&status_line(6), 6); + cache.append(&status_line(7), 7); + cache.flush(); + + let transport = ScriptedTransport::default(); + transport.respond(200, format!("[{}]", status_line(9))); + let source = TranscriptSource::new(ApiClient::new(transport), "s1", cache); + source.page(10, 10, true).unwrap(); + assert_eq!( + source.api.transport().calls.lock().unwrap()[0], + "/sessions/s1/transcript?limit=10&before=10&coalesce=true&after=7", + "the fetch must stop one seq below where this phone's copy ends" + ); + } + + /// A page the server could not answer is an error, never an empty + /// page: the caller would read the second as "this conversation has no + /// more history" and stop paging for good. + #[test] + fn a_failing_server_page_is_an_error_rather_than_an_empty_one() { + let dir = tempfile::tempdir().unwrap(); + let transport = ScriptedTransport::default(); + transport.respond(500, "server on fire"); + let source = source(transport, dir.path()); + assert!(matches!(source.page(9, 10, true), Err(PageError::Api(_)),)); + } + + /// A cached line this build cannot read is told apart from the network + /// failing, for the same reason: neither is "no more history". + #[test] + fn an_unreadable_cached_page_is_a_parse_error_rather_than_an_empty_one() { + let dir = tempfile::tempdir().unwrap(); + let cache = crate::transcript_cache::TranscriptCache::new(dir.path()).session("s1"); + cache.store_page( + &[r#"{"seq":3,"but":"not an event"}"#.to_string()], + 3, + 4, + true, + ); + cache.append(&status_line(4), 4); + cache.flush(); + let transport = ScriptedTransport::default(); + let source = TranscriptSource::new(ApiClient::new(transport), "s1", cache); + assert!(matches!(source.page(4, 10, true), Err(PageError::Parse(_)),)); + assert_eq!( + source.api.transport().call_count(), + 0, + "a cache hit that cannot be read must not fall through to the server unnoticed" + ); + } + #[test] fn a_bad_cached_opening_line_purges_rather_than_panicking() { let dir = tempfile::tempdir().unwrap(); diff --git a/docs/CLIENT_CORE.md b/docs/CLIENT_CORE.md index 9dabd7e..d2bc7eb 100644 --- a/docs/CLIENT_CORE.md +++ b/docs/CLIENT_CORE.md @@ -104,7 +104,14 @@ landing cleanly between two already-finished calls (most of them) leaves one run drawn as two. `a_call_split_across_the_boundary_merges_into_one_row`, `a_message_split_across_the_boundary_is_rejoined_with_the_newer_halfs_identity`, and `adopt_run_never_renames_into_a_question_row` cover the other three -edges the Kotlin doc calls out. +edges the Kotlin doc calls out. `join_pages` ends in a `debug_assert!` +that no tool id survives in both halves -- the duplicate row it exists to +prevent, checked rather than assumed. What it deliberately does *not* +assert is seq ordering across the boundary: a peer note carries the seq +its turn began at (`place_peer_note`), which can be older than the page +it arrived in, so the two pages' seqs legitimately interleave there. An +earlier draft asserted it and would have panicked in debug builds on an +ordinary transcript. **Known gap, and a decision for whoever closes it:** `event_model::Event` has no `Unknown`/catch-all variant, unlike `Events.kt`'s hand-kept mirror. @@ -142,14 +149,25 @@ wrong") and `page`'s cache-vs-server split bounded by `covered_up_to`. Two additions beyond a literal port, both load-bearing: - **`page(before, ..)` refuses `before == 0` before touching the cache or - the network**, returning an empty page immediately. This is - AGENTS.md's `loadOlderPage` incident (`before = 0` is "no event before - the first one," indistinguishable from "reached the start of history" - if a caller ever asks it) moved out of the Kotlin screen and into this - layer, so every future caller gets the guard rather than having to - remember it. `paging_before_the_first_event_makes_no_request_at_all` - asserts zero transport calls, not just an empty result, since a request - that happens to answer empty is exactly what caused the original bug. + the network**, answering `OlderPage::NothingLoaded`. This is AGENTS.md's + `loadOlderPage` incident (`before = 0` is "no event before the first + one," indistinguishable from "reached the start of history" if a caller + ever asks it) moved out of the Kotlin screen and into this layer, so + every future caller gets the guard rather than having to remember it. + **The return type is `OlderPage`, not a `Vec`, and that is the guard.** + The Kotlin's two falses are different answers -- `oldestSeq == 0` + returns without touching `moreHistory`, an empty page latches it false + -- so a port that answered both with an empty list would have moved the + bug rather than fixed it, one layer down and out of sight of the screen + that used to hold the check. `OlderPage::Events(vec![])` means the start + of the conversation; `OlderPage::NothingLoaded` is not an answer about + the conversation at all. Reviewed 2026-09-06. + `paging_before_the_first_event_makes_no_request_at_all` asserts zero + transport calls, not just the variant, since a request that happens to + answer empty is exactly what caused the original bug, and + `a_failing_server_page_is_an_error_rather_than_an_empty_one` plus + `an_unreadable_cached_page_is_a_parse_error_rather_than_an_empty_one` + are the same rule for the two ways a page can fail. - **`fetch_transcript_lines`** (new in `api.rs`) hands back each line paired with the exact server bytes it came from, via `serde_json::value::RawValue` rather than re-serializing a parsed