ai-app/docs/REVIEW-2026-09-07.md

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