Prune commentary and stale Rust port notes
This commit is contained in:
1 parent
5428cd75c9
commit
25370731d0
193 files changed
+693
-16219
No files matched your search
+55
-202
@@ -1,221 +1,74 @@
|
||||
# `app-rust`'s `client` module
|
||||
|
||||
**Renamed 2026-09-08.** This was the `client-core` crate; it is now
|
||||
`app-rust/src/client/`, a module of the one app crate rather than a crate
|
||||
of its own (docs/RUST.md's "One app crate"). Nothing about what it *is*
|
||||
changed: it is still the app's pure logic held once instead of twice, per
|
||||
RUST.md's recommendation item 1, and still has **no UI framework
|
||||
dependency of any kind** -- the `iris` dependency sits behind the `screens`
|
||||
feature and nothing under `src/client` may reach it. That independence is
|
||||
what lets it outlive whichever framework the app draws with, and it is now
|
||||
an invariant of a module rather than of a manifest, so it is worth stating
|
||||
plainly: a `use iris::` under `src/client/` is a defect.
|
||||
`app-rust/src/client` contains platform- and UI-independent client logic. It
|
||||
must not depend on iris; a `use iris::` below this directory is a layering
|
||||
defect. `event-model` remains a separate crate because the server and client
|
||||
both depend on that wire contract.
|
||||
|
||||
`event-model/` stayed a crate, and is the one split in the port that was
|
||||
never optional: it is the wire shape both this app and `server/` depend on,
|
||||
extracted from `server/src/session/driver.rs` and `session/transcript.rs`
|
||||
on 2026-09-04, so a crate is what makes the two agree by construction.
|
||||
## Contents
|
||||
|
||||
Paths below are written as `src/client/…`, relative to `app-rust/`.
|
||||
- `api.rs`: REST client over the injectable `Transport` trait.
|
||||
- `sse.rs` and `event_stream.rs`: SSE framing and session-event following.
|
||||
- `transcript_cache.rs`: bounded, persistent transcript chunks.
|
||||
- `transcript_source.rs`: cache/server selection and live cache updates.
|
||||
- `transcript_fold.rs`: event folding, tool grouping, and page healing.
|
||||
- `markdown_blocks.rs`, `ansi.rs`, and `highlight/`: display-independent text
|
||||
parsing and spans.
|
||||
- `config.rs`: enrollment-link parsing and the shared `EnrolledServer` value.
|
||||
- `log_ring.rs`: bounded process-local diagnostics.
|
||||
|
||||
## What's here, and what Kotlin file it replaces
|
||||
## API coverage
|
||||
|
||||
| `src/client/…` | 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 |
|
||||
`ApiClient` covers session list/read, messages, unqueue, answers, interrupt,
|
||||
stop/start, rename, working directory, model, permission mode, notification
|
||||
setting, commands, compaction, deletion, and transcript pages.
|
||||
|
||||
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).
|
||||
Still missing are setups and discovery, file operations, usage, models and
|
||||
downloads, attachments, imports, and the global notifications stream.
|
||||
`server/src/routes.rs` is the authoritative route table.
|
||||
|
||||
## Correspondence notes worth knowing before touching either side
|
||||
## Transcript invariants
|
||||
|
||||
- **`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`.
|
||||
`join_pages` heals messages and tool runs split across page boundaries. It
|
||||
asserts that a tool id does not survive in both halves. It must not assert
|
||||
sequence ordering across the seam: a peer note carries the sequence of the
|
||||
turn it belongs above and can legitimately interleave with the page where it
|
||||
arrived.
|
||||
|
||||
## What `api.rs` covers, and what it does not yet
|
||||
`Event` has no catch-all variant. A newer server adding an event type will
|
||||
make an older client reject that line rather than draw a placeholder. Fixing
|
||||
that requires a shared wire-model decision, not a client-only workaround.
|
||||
|
||||
`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).
|
||||
`TranscriptSource::page(0, ..)` returns `OlderPage::NothingLoaded` without
|
||||
touching cache or network. This is deliberately distinct from
|
||||
`OlderPage::Events(vec![])`, which means the start of the conversation was
|
||||
actually reached. Network and cache parse failures are errors for the same
|
||||
reason: none of these states may latch a caller's “no more history” flag.
|
||||
|
||||
Covered: session list/read, message send, unqueue, answer, interrupt,
|
||||
stop, start, rename, cwd, model, permission-mode, notify, command,
|
||||
compact, delete, and one transcript page.
|
||||
Fetched transcript lines retain the server's exact JSON bytes through
|
||||
`RawValue`. Re-serializing parsed JSON can change floating-point text, causing
|
||||
the cached and streamed forms of one event to disagree byte-for-byte.
|
||||
|
||||
**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.
|
||||
The reconnect/backoff loop and cancellation of a live stream belong to the
|
||||
embedding runtime. `TranscriptSource::follow` only guarantees that each frame
|
||||
is cached before the caller receives it.
|
||||
|
||||
## What `transcript_fold.rs` covers, and what it does not yet
|
||||
## Enrollment
|
||||
|
||||
`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`.
|
||||
`EnrolledServer` and `parse_link` understand the same
|
||||
`aiapp://enroll?host=H&port=P&token=T[&ca=B]` value used by Android. Storage
|
||||
is caller-specific: Android uses its platform storage and the desktop writes a
|
||||
0600 file under its XDG config directory.
|
||||
|
||||
`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.
|
||||
## Markdown scope
|
||||
|
||||
**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`-only
|
||||
fix, so it is recorded here rather than silently worked around.
|
||||
`markdown_blocks` splits top-level headings, paragraphs, fences, lists,
|
||||
tables, and quotes. It intentionally does not build a full nested CommonMark
|
||||
AST; inline styling and nested presentation remain renderer concerns until a
|
||||
shared non-UI consumer needs them.
|
||||
|
||||
## `config.rs`: `EnrolledServer`
|
||||
## Verification
|
||||
|
||||
`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, decided 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, `src/desktop/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 `src/desktop`; the Android entry point is 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
|
||||
`src/desktop/app.rs` and `src/android/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
|
||||
|
||||
- **A full markdown AST.** `markdown_blocks` (2026-09-06) splits a message
|
||||
into its *top-level* blocks -- heading, paragraph, fence, list, table,
|
||||
quote -- with each block's own source, which is what a renderer needs to
|
||||
lay out prose versus code and what lets a streamed delta re-lay out one
|
||||
block instead of the message (docs/RUST.md's Task B). What it
|
||||
deliberately does **not** build is the tree below that: nested list
|
||||
items, table cells, inline spans. Inline styling is still the renderer's
|
||||
own job per block (`src/ui/markdown.rs`), and nothing
|
||||
has needed the rest yet. `CodeFence.kt`'s use of `org.intellij.markdown`
|
||||
for a full CommonMark AST is Compose rendering plumbing, not something
|
||||
to port as-is.
|
||||
- **`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
|
||||
|
||||
`./scripts/run-tests.sh` from the repo root runs `event-model`, `server` and
|
||||
`app-rust` in that order (each `cargo test`, forwarding arguments the same
|
||||
way it always has). From `app-rust/` directly: `cargo test`, `cargo clippy
|
||||
--all-targets`, `cargo fmt` -- all clean as of 2026-09-08, 229 tests across
|
||||
the crate and its headless harness suites.
|
||||
Run `./scripts/run-tests.sh` from the repository root. For this crate alone,
|
||||
run `cargo test`, `cargo clippy --all-targets`, and `cargo fmt --check` from
|
||||
`app-rust/`.
|
||||
Reference in new issue
Block a user