iris
bf3479f5c4
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.
214 lines
13 KiB
Markdown
214 lines
13 KiB
Markdown
# client-core
|
|
|
|
`client-core/` is the app's pure logic held once instead of twice, per
|
|
RUST.md's recommendation item 1. It is a plain Rust library crate with no UI
|
|
framework dependency of any kind, so it can outlive whichever one the app
|
|
ends up drawing with (Masonry, iris, or something else -- see RUST.md).
|
|
`event-model/` is its sibling: the wire shape both this crate and `server/`
|
|
share, extracted from `server/src/session/driver.rs` and
|
|
`session/transcript.rs` on 2026-09-04.
|
|
|
|
Neither crate is wired into anything yet. `server/` re-exports `event-model`
|
|
so its own behaviour is unchanged (`./run-tests.sh` covers it); `client-core`
|
|
has no caller -- it exists for whichever experiment in RUST.md picks it up
|
|
next (a Masonry or iris transcript screen, most likely).
|
|
|
|
## What's here, and what Kotlin file it replaces
|
|
|
|
| `client-core/src/…` | Kotlin original | Status |
|
|
|------------------------------------------|-------------------------------------------|--------|
|
|
| `event-model/src/lib.rs` (shared crate) | `Events.kt` (the enum mirror) | Done |
|
|
| `ansi.rs` | `Ansi.kt` | Done, ported test-for-test |
|
|
| `highlight/mod.rs`, `languages.rs` | `Highlighter.kt`, `Languages.kt` | Done, ported test-for-test |
|
|
| `highlight/markdown.rs` | `MarkdownSyntax.kt` | Done, ported test-for-test |
|
|
| `transcript_cache.rs` | `TranscriptCache.kt` | Done, ported test-for-test |
|
|
| `sse.rs` | `Sse.kt` (the framing half) | Done, new tests (Kotlin had none of its own beyond integration) |
|
|
| `api.rs` | `Api.kt` | Partial -- see below |
|
|
| `event_stream.rs` | `EventStream.kt` | Done |
|
|
| `transcript_fold.rs` | `TranscriptItems.kt`, `ToolRows.kt` | Done -- see below |
|
|
| `config.rs` | `ServerConfig.kt`'s `handleEnrollment` | New, desktop-only so far -- see below |
|
|
| `transcript_source.rs` | `TranscriptSource.kt` | Done -- see below |
|
|
| *(not ported, and may never be)* | `TranscriptUnits.kt` | Out of scope -- see below |
|
|
|
|
Every file above whose Kotlin counterpart had a JVM unit test (`AnsiTest`,
|
|
`HighlighterTest`, `TranscriptCacheTest`) has had every one of those test
|
|
cases ported alongside it, plus new tests for the pieces that had none
|
|
(`sse.rs`, `api.rs`, `event_stream.rs`, `transcript_fold.rs`,
|
|
`transcript_source.rs` -- the Kotlin `TranscriptSource.kt`/`TranscriptItems.kt`
|
|
had no JVM unit tests of their own, so these were written fresh against the
|
|
Kotlin source and AGENTS.md's paging incidents as the spec). Test count by
|
|
crate as of this writing: **109 in `client-core`**, 0 in `event-model` (its
|
|
types carry no logic of their own to test -- `server/`'s own tests exercise
|
|
them via `session::transcript`'s round-trip coverage).
|
|
|
|
## Correspondence notes worth knowing before touching either side
|
|
|
|
- **`ansi.rs`'s `StyledText`/`Style`/`Rgb`** stand in for Compose's
|
|
`AnnotatedString`/`SpanStyle`/`Color`, since this crate has no Compose.
|
|
`StyledText` is plain text plus a `Vec<(Range<usize>, Style)>` of
|
|
non-overlapping spans. Whatever UI framework ends up consuming this
|
|
crate maps `Style` onto its own text-styling type; nothing here should
|
|
change to accommodate a particular one.
|
|
- **`highlight`'s `Span`/`Kind`** use **char indices, not byte offsets**
|
|
(`Vec<char>` internally), mirroring the Kotlin original's `Char`-indexed
|
|
strings. `highlight::span_text` turns a `Span` back into text for a
|
|
caller working the same way; a caller that wants byte offsets into a
|
|
`&str` has to convert.
|
|
- **`transcript_cache.rs`'s `SessionCache::guard`** found a real
|
|
translation bug while it was being written: an early draft let a
|
|
*damaged* chunk (one file unreadable, discard just this session) and a
|
|
genuine I/O failure (disk gone, disable the whole cache) both surface as
|
|
the same `Err` from one closure, which would have disabled every
|
|
session's cache over a single corrupt chunk. Fixed by checking a
|
|
thread-local "was this damage" flag before deciding which failure mode
|
|
it was -- see the comment on `guard` and the commit message for
|
|
`transcript_cache.rs`.
|
|
|
|
## What `api.rs` covers, and what it does not yet
|
|
|
|
`ApiClient` wraps a `Transport` trait (network I/O kept out from behind, so
|
|
`ApiClient` and `event_stream::follow_session_events` are tested with a
|
|
fake transport and no server). `UreqTransport` is the only real
|
|
implementation, backed by `ureq` -- see its Cargo.toml comment for why
|
|
(blocking, already a project dependency, no extra TLS crate needed since
|
|
`ureq::tls::Certificate::from_pem` reads the pinned CA directly).
|
|
|
|
Covered: session list/read, message send, unqueue, answer, interrupt,
|
|
stop, start, rename, cwd, model, permission-mode, notify, command,
|
|
compact, delete, and one transcript page.
|
|
|
|
**Not covered, and each is real work rather than a stub to fill in:**
|
|
setups (`/setups*`, machine and provider discovery), the file explorer
|
|
(`/setups/{id}/dir|file`), usage (`/usage`), models
|
|
(`/models*`, HuggingFace browsing and downloads), attachments
|
|
(`/sessions/{id}/attachments`), importing (`/setups/{id}/importable*`),
|
|
and the `/notifications` stream. `server/src/routes.rs`'s module doc is
|
|
the full table to work from when one of these is next.
|
|
|
|
## What `transcript_fold.rs` covers, and what it does not yet
|
|
|
|
`fold_event` covers every `Event` variant server/ can produce today,
|
|
including tool-call/question/image attachment and peer-message placement.
|
|
`group_tool_runs` groups adjacent calls into `TranscriptRow::Tools`.
|
|
|
|
`join_pages` (with `heal_split_message` and `adopt_run`, both private) is
|
|
now ported too, 2026-09-06 -- the page-boundary healing that merges a tool
|
|
call split across two fetched pages, rejoins a message a boundary cut
|
|
through, and renames a run of tool calls onto whichever name is already on
|
|
screen. Ported with AGENTS.md's "things that have bitten" incidents as the
|
|
spec rather than a JVM test file (`TranscriptItems.kt` had none of its
|
|
own): `a_clean_boundary_between_two_finished_runs_is_still_healed_into_one_run`
|
|
is the regression test for the bug that shipped -- `adopt_run` must run on
|
|
*every* join, not only the one where a split call was found, or a boundary
|
|
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. `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.
|
|
A server newer than this build that adds an event type will fail to parse
|
|
that line rather than degrading to a placeholder row. Closing this means
|
|
deciding how `event_model` itself represents "a shape I don't recognise"
|
|
-- a shared-model decision affecting `server/` too, not a `client-core`-only
|
|
fix, so it is recorded here rather than silently worked around.
|
|
|
|
## `config.rs`: `EnrolledServer`
|
|
|
|
`EnrolledServer` (host, port, bearer token) plus `parse_link`, which reads
|
|
the exact `aiapp://enroll?host=H&port=P&token=T` deep link
|
|
`wg-app-link`'s `enroll` mints and `ServerConfig.kt`'s `handleEnrollment`
|
|
parses on the phone -- so any Rust client enrols from the same text a
|
|
phone would scan as a QR, with no second format invented for it (RUST.md's
|
|
E4, DECISIONS.md 2026-09-05). Deliberately does not decide where it is
|
|
persisted or under what file permissions -- a phone seals its token in the
|
|
Android Keystore, `iris/desktop-app/src/config.rs` writes it to
|
|
`$XDG_CONFIG_HOME/ai-app-desktop/enrollment.json` at 0600 -- since that is
|
|
caller-specific (the code rules' "ask for the least you need"). Its only
|
|
caller today is `desktop-app`; a future Android build of this crate would
|
|
be a second one, not a reason to move the type.
|
|
|
|
## What `transcript_source.rs` covers, and what it does not
|
|
|
|
`TranscriptSource<T: Transport>` is the seam a session screen asks for a
|
|
page, ported test-for-test against the Kotlin doc rather than a JVM test
|
|
file (there wasn't one): `cached_opening`, `probe`, `fetch_opening`,
|
|
`page` and `follow`, each matching its Kotlin namesake's contract --
|
|
including `probe`'s three-way outcome (matches / cache purged /
|
|
unreachable, told apart so a caller never treats "couldn't ask" as "was
|
|
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**, 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
|
|
`Value` -- the cache and a live SSE frame for the same event have to
|
|
agree byte-for-byte, which is exactly what the `serde_json`
|
|
float-rounding bug (AGENTS.md) was about. The existing
|
|
`fetch_transcript_page` is untouched (other callers under `iris/`
|
|
depend on its signature); the two share a `transcript_path` helper so
|
|
the query string is written in one place.
|
|
|
|
**Not ported:** `EventStream.kt`'s reconnect-with-backoff loop, and
|
|
`TranscriptSource.close`'s ability to cancel a live stream from another
|
|
thread. Both are wall-clock/thread-lifetime policy that belongs to
|
|
whichever runtime embeds this crate (iris's own timers, a Tokio task, a
|
|
Kotlin coroutine scope), not to this pure logic -- `follow` is the same
|
|
"write to the cache, then hand the frame to the caller" decorator
|
|
`iris/desktop-app/src/app.rs` and `iris/android-app/src/transcript_client.rs`
|
|
already hand-wrote around `event_stream::follow_session_events` before this
|
|
existed; the cache write moved into one shared place so a third caller
|
|
does not repeat it again by hand.
|
|
|
|
## What is not started at all
|
|
|
|
- **The markdown *block* model beyond syntax spans** -- `highlight/markdown.rs`
|
|
colours a `.md` file or fence for the highlighter, but does not build the
|
|
block tree (headings, lists, tables, fences as distinct nodes) that a
|
|
renderer walks to lay out prose versus code versus a table.
|
|
`CodeFence.kt`'s use of `org.intellij.markdown` for that full CommonMark
|
|
AST is Compose rendering plumbing, not something to port as-is; a Rust
|
|
UI layer will want its own block parser or a crate for it, decided
|
|
alongside the framework choice in RUST.md.
|
|
- **`TranscriptUnits.kt`** (see above) -- deliberately out of scope, since
|
|
it flattens a row into bounded units for a *specific* lazy-list
|
|
framework's composition cost, which is a fact about that framework
|
|
rather than about the transcript.
|
|
|
|
## Verifying
|
|
|
|
`./run-tests.sh` from the repo root now runs `event-model`, `client-core`
|
|
and `server` in that order (each `cargo test`, forwarding arguments the
|
|
same way it always has). From `client-core/` directly: `cargo test`
|
|
(109 tests), `cargo clippy --all-targets`, `cargo fmt` -- all clean as of
|
|
this writing (2026-09-06).
|