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_source.rs

@@ -70,6 +70,26 @@ impl From<ParseError> 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<SeqEvent>),
+    /// 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<SeqEvent, ParseError> {
     serde_json::from_str(line).map_err(|e| ParseError(format!("{e}")))
 }
@@ -174,35 +194,30 @@ impl<T: Transport> TranscriptSource<T> {
     /// 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<Vec<SeqEvent>, 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<OlderPage, PageError> {
         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<SeqEvent> = lines
                 .iter()
                 .map(|l| parse_line(l).map_err(PageError::from))
-                .collect();
+                .collect::<Result<_, _>>()?;
+            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<T: Transport> TranscriptSource<T> {
             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<String> = (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();