diff --git a/docs/REVIEW-2026-09-07.md b/docs/REVIEW-2026-09-07.md new file mode 100644 index 0000000..4215fa4 --- /dev/null +++ b/docs/REVIEW-2026-09-07.md @@ -0,0 +1,381 @@ +# Review, 2026-09-07 — `ba2afba..origin/rustify` + +Read-only review of the day's 24 commits: the glyph-atlas fix, the fling +spline and Lsq2 velocity estimator, keyboard/IME insets and `targetSdk`, +historical touch samples and the input clock, list culling / clamp / +anchor re-homing, nested masks and `draw_again`, the headless harness + +`transcript-fixture` + `rig-input`, desktop density, the release profile, +platform fonts + the Android monospace patch, and the client-core log ring +with `POST /client-log`. + +**Verified while reviewing** (working tree, which also carries three other +agents' uncommitted edits — `iris/src/sense.rs`, `iris/core/src/ui/render_state.rs`, +`iris/src/lib.rs`, `iris/core/src/orientation/axis.rs`, and an untracked +`iris/src/diagnostics.rs`): `cargo fmt --check` clean in `iris/`, +`client-core/` and `server/`; `cargo clippy --all-targets` clean in `iris/` +and `client-core/`; `cargo test --lib -p iris` 101 passed, `cargo test -p +transcript-fixture` 10 passed. The `iris` doctest target fails to link +(`extern location for iris_core does not exist`) — a stale build artefact, +not a code fault, but worth knowing before trusting `cargo test -p iris` +as a whole. + +The work is unusually well documented and the two "a test that compared +the code with itself" findings the authors made themselves are real and +were fixed correctly. What follows is what is left. + +Counts: **5 defects, 7 risks, 3 tests that cannot fail in the bug's +direction, 7 rule findings, 2 nits.** + +--- + +## Defects + +### D1 — the app's own log ring is drowned by the same day's per-frame `debug!` lines, so the route built to get Iris's logs to her carries almost none of them + +`iris/android-app/src/lib.rs:132` installs the ring at `LevelFilter::Debug`, +and `client-core/src/log_ring.rs:279` (`RingLogger::enabled`) returns +`true` unconditionally by design, so **every `log::debug!` in the process +lands in a 2000-line / 256 KiB ring**. In the same commit range that ring +became the only way a line reaches Iris, three ungated per-frame `debug!` +callsites are live: + +- `iris/src/android/view.rs:446` and `:509` — two lines *per rendered frame*. +- `iris/src/widget/list.rs:576` — `iris fling tick:`, one line per fling tick. +- `iris/src/widget/text/mod.rs:81` — one per text shape (many per frame while rows compose). + +**Failure scenario.** Iris flicks the transcript on a 120 Hz phone. That is +~240–360 debug lines a second; the ring's 2000-line bound is exhausted in +**under ten seconds**, so by the time she presses `Copy report` every +`log::info!` about what she was actually investigating has been evicted. +The uploader makes it worse: it sends at most the ring per 10 s wake +(2000 lines ≈ 200 lines/s) against ~350 lines/s produced, so it also runs +permanently behind and pushes tens of KB/s of frame spam over the tunnel. + +Note that another agent has already built the right mechanism — the +untracked `iris/src/diagnostics.rs` has `set_trace`/`trace_enabled`, a +default-off gate, and its module doc states this exact problem in as many +words. It gates `iris::input`/`iris::frame`; it does **not** gate the four +callsites above. + +*Fix*: put `List::tick_fling`'s line and `view.rs`'s two `render():` lines +behind `iris::diagnostics::trace_enabled()` (the mechanism that already +exists for exactly this), and/or record into the ring at `Info` while +leaving `android_logger` at `Debug`. + +### D2 — `POST /client-log` can make `ai-server` write an unbounded runtime log at an authenticated client's request + +`server/src/routes.rs:1473` bounds the **line count** (500) and nothing +else. The route sits inside the router that applies +`DefaultBodyLimit::max(32 * 1024 * 1024)` at `server/src/routes.rs:179` +(raised for phone photos), so one request may carry 500 lines of ~64 KiB +each, and each is re-emitted verbatim into `tracing`. There is no +per-message cap on the server, no rate limit, and the runtime log +`ai-server` writes is the file Dev Updater tails and never rotates. +`MAX_MESSAGE_BYTES` (4096) exists only in the *client* +(`client-core/src/log_upload.rs:33`), i.e. the server trusts a value the +attacker controls. + +**Failure scenario.** A buggy client (a `log::debug!` in a loop is enough — +see D1) or one holding a leaked bearer token posts 32 MiB every 10 s; the +host's disk fills and every other component's log goes with it. + +*Fix*: give the route its own `DefaultBodyLimit` (the attachments route at +`:175` is the precedent for a per-route limit) and truncate each `message` +server-side to the same 4096 bytes rather than assuming the client did. + +### D3 — lines the ring drops before the uploader sends them vanish with nothing saying so + +`LogRing::since` (`client-core/src/log_ring.rs:169`) filters `seq >= cursor` +and silently returns fewer lines when eviction has passed the cursor; +`LogUploader::flush_once` (`:94`) then advances to whatever came back. +`dropped` is counted (`log_ring.rs:109`) and shown in the *local* +diagnostics pane, but it is never put in the upload body, and +`ClientLogBody` has no field for it. + +**Failure scenario.** The tunnel is down for two minutes; the ring wraps. +When it comes back, the server log jumps from `#812` to `#5106` with no +line saying anything was lost. This is precisely the "unknown state +sharing a value with the empty state" UI_RULES asks to design first, and +the module doc for `dropped` claims it is "reported rather than inferred" +— it is, but only on the half of the path nobody is reading. + +*Fix*: carry `dropped` (or `firstSeq`) in the batch and have `client_log` +emit one `warn!` when the sequence is not contiguous with the last batch +from that `source`. + +### D4 — the input clock anchors on the first event's *own* time, so that event's historical samples are dated before the anchor: the ordering assert fires, and release silently collapses them onto one instant + +`iris/src/android/view.rs:628` takes the anchor as +`(Instant::now(), event.event_time_nanos())` from the first `MotionEvent` +the view ever sees, and `at()` computes +`anchor_at + (sample_time - anchor_nanos).max(0)`. Historical samples of +that same event are by definition **earlier** than its own `event_time`. + +**Failure scenario.** The first event this view receives is an +`ACTION_MOVE` (the `DOWN` was delivered to another view, or the view was +attached mid-gesture). Its historical samples are, say, 12 ms before +`anchor_nanos`; `at()` clamps all of them to `anchor_at`, so the tracker +receives three samples with identical timestamps, the Lsq2 fit is +degenerate, and the flick reads 0 px/s. In a debug build the +`debug_assert!(ht >= previous)` at `:653` fires first — but `previous` +starts at `anchor_nanos` (`:651`), which is a value from a *different* +event, so that assert is also the wrong comparison for the first sample of +every later event. + +*Fix*: anchor on the earliest sample of the first event +(`historical_event_time_nanos(0)` when `history_size() > 0`, else +`event_time`), and seed `previous` from the previous event's last sample +rather than from the anchor. + +### D5 — the "before" velocity quoted in four places is not what the reference script prints + +`iris/benches/velocity_reference.py`, run today, prints **12250 px/s** for +`flick-120hz.touch`'s average and **12500 px/s** for "press and one move +frame". Four places say 11750 for both: + +- `docs/RUST.md:900` (`flick-120hz.touch | 11750 px/s`) +- `docs/RUST.md:905` (`press + one move frame | 11750 px/s`) +- `docs/IRIS_TODO.md:1026` +- `iris/transcript-fixture/tests/phone_screen.rs:55` + +`iris/src/sense.rs:1406` has the correct 12250, so the two halves of the +same change disagree. The file that carries the wrong number is the one +that says "every number below is printed by `velocity_reference.py` … do +not 'fix' one by running the Rust and copying what it said". One of the +two rows also being 11750 for a completely different sample set is the +tell. + +*Fix*: replace 11750 with the script's own 12250 / 12500 in those four +places, or say which run produced 11750. + +--- + +## Risks + +### R1 — every new invariant guard is a `debug_assert!`, and the phone runs release + +The five guards added today — +`iris/src/widget/list.rs:1156` (a `List` must be inside a `.masked()`), +`:1218` (`extents` holds only on-screen rows), +`iris/src/android/view.rs:653` (historical sample ordering), +`iris/src/sense.rs:1076` (`poly_fit_least_squares` sample count), and +`iris/core/src/ui/painter.rs`'s doubled-`set_mask` check — are all +`debug_assert!`. `docs/RUST.md` records that the bench APK **must** be +installed as `release` on the emulator (the debug `libmain.so` is 325 MB +and will not install) and Iris's phone gets release too. So none of these +can fire on any build anybody actually runs; in release a `List` drawn +without a mask silently paints over its surroundings again — the exact +fault e922b73 was written to fix. + +*Fix*: for the two that are cheap and once-per-draw (`is_masked`, the +extents check), consider a plain `assert!` or a one-shot `log::error!`, so +the guard survives into the build the defect was found in. + +### R2 — a straddling row is now invisible above the list and still tappable through the header + +Masks are applied in the fragment shader +(`iris/core/src/render/shader.wgsl:203`); the CPU hit path +(`UiRenderState::resolved_region`, `iris/core/src/ui/render_state.rs:709`) +does not consult `masks` at all. Before today the top of a straddling row +was drawn over the header *and* hit-testable there; now it is clipped away +but still hit-testable, which is worse — a tap on "Run benchmark" can land +on an invisible link in the row behind it. `docs/LAYOUT.md:1012` ("Hit- +testing applies the shape") is design, not code. + +*Fix*: until LAYOUT.md's mask redesign lands, intersect a widget's hit +region with its mask chain in `resolved_region`; the chain walk already +exists on the GPU side. + +### R3 — three copies of one wire contract, none of them linked + +`client-core/src/log_upload.rs:28` (`MAX_LINES_PER_BATCH = 500`) and +`server/src/routes.rs:1418` (`CLIENT_LOG_MAX_LINES = 500`) must agree, in +different crates, with only a comment saying so; the body itself is built +by hand with `serde_json::json!` on one side and parsed by a +`#[serde(deny_unknown_fields)]` struct on the other. This project already +has the mechanism for exactly this — `event-model`, a crate both `server` +and `client-core` depend on precisely so "the app hand-mirroring it" stops +happening (`server/Cargo.toml:16` says so). + +**Failure scenario.** Somebody raises the client's batch to 1000. Every +upload now returns 400, the uploader retries the *same* batch from the same +cursor forever, and the only sign is one line in a diagnostics pane on a +phone. + +*Fix*: move `ClientLogLine`/`ClientLogBody` and the batch constant into a +shared crate. + +### R4 — `build.rs` bakes in a CA it never asks Cargo to watch, and the bench build now has no rebuild trigger at all + +`emit_log_config` (`iris/android-app/build.rs:92`) calls `read_pinned_ca()` +but emits only `rerun-if-env-changed` for `AI_APP_LOG_HOST/_PORT/_TOKEN` — +no `rerun-if-changed` for the CA *file*, and (because the bench build +returns at `:65`, before the transcript path's declarations) no +`rerun-if-env-changed=AI_APP_CA`/`XDG_CONFIG_HOME` either. Emitting any +`rerun-if-*` directive turns off Cargo's default "rerun when anything in +the package changes" heuristic, so the bench build lost the only trigger it +had. + +**Failure scenario.** `~/.config/ai-app` is wiped (AGENTS.md calls this the +one-way door), `ai-server` mints a new CA, the APK is rebuilt — and +`build.rs` does not re-run, so the APK still pins the dead CA and every +upload fails with a TLS error nobody can attribute. + +*Fix*: `println!("cargo:rerun-if-changed={}", ca_path.display())` inside +`read_pinned_ca`, and move the `AI_APP_CA`/`XDG_CONFIG_HOME` declarations +above the bench early-return. + +### R5 — desktop density is read once and never updated + +`iris/src/default/mod.rs:254` reads `content_scale(window)` at startup and +sets it on both `rsc.ui.text.density` and `render`. `WindowEvent:: +ScaleFactorChanged` is not handled, and `UiRenderer::resize` deliberately +no longer consults `scale_factor`. Dragging the window to a monitor with a +different scale leaves every `dp(...)` and every rasterised glyph at the +old density — the same class of disagreement the commit removed elsewhere. +It is invisible here (every display on this machine is 1.0), which is why +it needs writing down. + +### R6 — removing the bundled fonts removed the guard for a fault that was found on the phone, and the check was run on the desktop + +`iris/core/src/primitive/text.rs`'s `register_bundled_fonts` existed +because "bold spans on a real phone rendered as blank gaps of the correct +advance width" — the deleted doc says so. Its removal is Iris's own call +and is recorded properly in `docs/DECISIONS.md`, but the verification +recorded there is "checked with CJK + emoji **on desktop**", which is the +half that cannot fail: the fault was Android's font enumeration resolving +a weight/style. `iris/transcript-ui/src/tool.rs:110`'s comment is honest +that `CLOSED_MARK`/`OPEN_MARK`/`UP_MARK` (U+25B8/BE/B4) are now "a bet" +that the platform monospace face has them — which is UI_RULES' "don't rely +on characters the platform might not have", stated and then accepted. + +*Fix*: before the next phone build, look at a bold run and the three +chevrons on Iris's device specifically; the emulator's font set is not +evidence for hers. + +### R7 — the least-squares fit clamps a degenerate norm instead of detecting it + +`iris/src/sense.rs:1105`: `1.0 / dot(...).sqrt().max(1e-6)`. Compose's +`polyFitLeastSquares` treats `norm < 1e-6` as "vectors are linearly +dependent, no solution" and bails; clamping instead produces a `q` row of +zeros, a zero on `r`'s diagonal, and a `0/0` that the `is_finite` check at +`:1059` happens to catch. It works, but it works by accident and the escape +is not the one the source it is transcribed from takes. + +--- + +## Tests that cannot fail in the direction the bug would go + +### T1 — `iris/transcript-fixture/tests/phone_screen.rs:64` computes the expected fling duration with the calculator under test, and asserts it one-sidedly + +`let expected = FlingCalculator::new(PHONE_SCALE).duration(velocity);` then +`assert!(ran_for <= expected + 2 frames)`. This is the same +"calculator compared with itself" shape the fling-spline commit +(73f956f) identified and fixed elsewhere, and the direction it can fail in +is "the fling ran too long" — never "the fling stopped dead", which is +literally Iris's reported symptom. The companion +`assert_ne!(before, after)` passes on one pixel of travel. A fling that +settles on the first tick passes this test. + +*Fix*: add a lower bound from `velocity_reference.py`'s number (a fling at +-15250 px/s at density 2.55 must run ≥ ~1.4 s and travel ≥ ~6000 px), not +from `FlingCalculator`. + +### T2 — `top_edge.rs:150` checks a row *count* on the leg where the culling bug appeared, and the box only on the other leg + +`rows_that_have_left_the_viewport_are_not_drawn` asserts `rows.len() <= 24` +on the outbound leg and the per-row `inside the box` predicate only on the +return leg. The doc explains why (an unmeasured row must be drawn to be +measured), which is correct — but it means the test's name is only true of +half of it, and a regression that draws 20 rows in the wrong *place* on the +outbound leg passes. + +### T3 — `top_edge.rs:116` checks that a mask exists and where it is, not that it reaches anything + +`the_list_is_clipped_to_its_own_box` asserts `active.mask != MaskIdx::NONE` +and that the mask's region lies within the list's box. It never checks the +row primitives actually reference that mask, so a broken `Mask::parent` +chain — the thing d507ae4 introduced — would leave this green while a code +fence inside a row drew unclipped again. + +*Fix*: assert that a row primitive's mask chain contains the list's mask +slot. + +--- + +## Rules + +- **`iris/src/widget/list.rs:576` is a second mechanism for per-frame + instrumentation.** `iris::diagnostics::trace_enabled` exists for exactly + "a default-off `debug!` in a hot path" and this line does not use it. + (Cause of D1; the gate is in the untracked `diagnostics.rs`, so at the + reviewed commit the line is simply ungated.) +- **`server/src/routes.rs:1518` (`client_log_time`) duplicates + `client-core/src/log_ring.rs:76` (`clock_time`)** — the same arithmetic + written twice in two crates, with a comment noting they must agree. Same + shared-crate answer as R3. +- **`client-core/src/log_ring.rs:301`'s doc claims more than the code + delivers**: "the caller is named in the error so it is findable" — + `log::SetLoggerError` names nobody. `iris/android-app/src/app_log.rs:44` + repeats the claim. +- **Stale comment: `iris/src/android/view.rs:624`** cites + `VelocityTracker::add_sample`'s debug assert; the method was renamed to + `add_position` in the same commit range. +- **`MOVE_CHAIN_LIMIT` now bounds two different chains** (move offsets and + masks) under a name that says one, in both + `iris/core/src/ui/render_state.rs:63` and `shader.wgsl:97`. The shader's + comment already calls it "the bound on the parent walk"; the constant + should say that too, or masks should get their own. +- **`iris/src/sense.rs:1434`'s stated negative control is not reproducible + as written.** "Reverting `velocity` to `total / span` fails exactly this + one, the flick recording, and `phone_screen.rs`" — but `samples` now + holds *positions*, so `total / span` over them gives 2750 for the steady + drag too, and the commit message for the same change says "exactly seven + tests". Two numbers for one experiment. +- **`iris/android-app/src/bench_client.rs:393`'s `ime_visible` is right and + its sibling one line up is not.** `set_bottom_inset(rsc, + insets.bottom.max(insets.ime_bottom))` still infers "make room" from a + `max`, so during the slide-in the composer is padded by the system-bar + inset while `ime_visible` already says the keyboard is up. Harmless + today; it is the same conflation the comment beside it warns about. + +## Nits + +- `iris/src/sense.rs:798` computes `self.velocity.velocity()` twice on a + release when `info` logging is on (once for the outcome, once for the + log line) — a full Lsq2 fit each. +- `iris/transcript-ui/src/selection.rs:303` calls `ui.ui_mut().animate(id)` + even when `fling()` bailed (`|v| <= 1.0`, or no anchor). Harmless — the + first `tick` unregisters — but it registers an animation that is known + not to exist. + +--- + +## Commits reviewed + +``` +7e4e26a iris: resolve fontique's Android monospace generic family ourselves +84a13e8 iris: a fling starts at Compose's velocity, which is a curve fit and not an average +452c442 docs/RUST.md: queue -- logging landed; iris app enrolment ... +238057a docs: the phone-logging decision, how to use it, and two build-apk traps +896c93a iris: drop bundled Noto Sans, match Compose's platform-font fonts +690161e docs: the transcript's edges were three faults, and what the rig found +e922b73 iris: a transcript row is drawn if it overlaps the viewport, and clipped to it +d507ae4 iris-core: masks nest instead of aborting, and a widget can ask to be drawn again +9ed01e2 docs: phone report 2026-09-07 later -- overscroll, low initial fling velocity ... +5be9f1b iris-android-app: keep the app's own log, put it in Copy report, upload it +977bdb9 client-core: the app's own log ring, and POST /client-log to get it off a phone +9cd1263 docs/RUST.md: queue -- APK size done, the embedded-fonts question left for Iris +42af780 iris android-app: strip+LTO+cgu1+opt-level=s halve libmain.so, no feature trim needed +4274b8b Merge remote-tracking branch 'origin/rustify' into worktree-agent-ace98b0bdaf33ffff +73f956f iris: the fling curve was the identity function, and the keyboard was a targetSdk +038f6a3 docs: the test rig's layers 1 and 2, with their commands and their limits +1121d7c docs/LAYOUT.md: masks reference a drawn primitive instead of copying a shape ... +232de0e iris: a phone-shaped desktop window, driven by the same touch recordings +e430880 docs: phone report 2026-09-07, rows at the transcript's top edge culled early ... +a999bd1 docs: masks with a shape (LAYOUT.md, decided 2026-09-07) and the orchestrator queue +6840edf iris-android-app: the bench's fixture half comes from transcript-fixture +3332201 iris: a headless in-process harness, and the bench fixture as a shared crate +7f4ea7e docs/TODO.md: Compose app crash from Iris's phone log export, reversed AnnotatedString range +591128e AGENTS.md: the phone app and the planned desktop app share widgets and styling +```