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.

Fix pass, 2026-09-07 evening

Every finding below carries a Status line. In summary: 13 fixed (D1, D4, D5, R1, R5, R7, T1, T2, T3 and four of the rule findings and both nits), 6 moot or deferred (D2, D3, R3, R4 and two rule findings, all of them in the phone-logging route that 06b8a1f deleted or in files the devlog agent held open), and 2 not done on purpose (R2, which waits on docs/LAYOUT.md's mask redesign, and R6, which needs Iris's own phone).

The commits are 2ec0fee (D4), 7e79ec1 (D5), 551c013 (R1), e10582a (T1-T3), ff1d6ea (R5, R7) and a6a100e (the rename and the nits). Each fix that the rig can express carries a test, and each of those was confirmed by breaking its subject on purpose -- the break is recorded beside the assertion, so the next reader does not have to re-derive it.

---

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.

Status: fixed in 992c472 (verified 2026-09-07: all four callsites, plus sense.rs's drag-release samples line, now sit behind iris::diagnostics::trace_enabled, and input_log_roundtrip proves both directions).

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.

Status: moot -- POST /client-log was deleted with the whole upload route (06b8a1f), the app hands its log to Dev Updater through an on-device ContentProvider instead. Nothing to bound.

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.

Status: moot -- client-core/src/log_upload.rs was deleted with the route (06b8a1f). Whatever the ContentProvider does about eviction is that design's question, not this one's.

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.

Status: fixed in 2ec0fee. The arithmetic moved into sense::PointerClock, which anchors at now - (event_time - oldest_sample) and carries the last sample seen across events, so the ordering assert compares against the previous event's last sample rather than the anchor. It lives in sense because iris::android is cfg'd out everywhere but the device: sense_tests.rs's the_first_events_batched_samples_are_dated_apart reports [0ns, 0ns, 0ns] against the old anchoring.

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.

Status: fixed in 7e79ec1. All four places now say 12250 / 12500, the 1.30x ratio becomes 1.24x, and RUST.md records where 11750 half came from (196 px over a 16.68 ms 60 Hz frame rather than the recording's own 16 ms -- which explains the flick row and not the other one, so that one was copied).

---

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.

Status: fixed in 551c013. is_masked, the extents check, set_mask's doubled-call check, Painter::glyphs's atlas generation and List::fling's finiteness are assert!/assert_eq! now; List::place's slot precondition, poly_fit_least_squares's two, and PointerClock::sample's ordering stay debug_assert! and say in a comment why. The layer-1 suites pass in --release as well as debug, which is what says the promoted ones do not fire on a real replayed flick.

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.

Status: not done, deliberately -- docs/LAYOUT.md's mask redesign ("masks reference a drawn primitive instead of copying a shape", 1121d7c) is where hit-testing gets the shape, and intersecting a chain in resolved_region now would be a second mechanism to unpick. Pointer left here rather than a fix.

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.

Status: moot -- both copies went with the route (06b8a1f). If a client/server contract comes back, event-model is still the answer.

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.

Status: moot -- iris/android-app/build.rs was deleted (06b8a1f/d8562d9): the destination comes from the enrolment link now, so nothing is baked in at build time and there is nothing for Cargo to watch.

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.

Status: fixed in ff1d6ea. WindowEvent::ScaleFactorChanged re-reads content_scale -- through that function, so IRIS_SCALE still pins --phone's density instead of following the monitor -- and UiRenderState::set_density marks the tree for a full redraw when the value actually changes, since Text::shape keys its cache on (attrs, width, density).

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.

Status: not done here -- it is a look at it on Iris's phone item, and no build in this VM is evidence about her device's font set. Carried forward as the review said: before the next phone build, look at a bold run and at CLOSED_MARK/OPEN_MARK/UP_MARK (U+25B8/BE/B4) on her device specifically.

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.

Status: fixed in ff1d6ea. poly_fit_least_squares returns Option and bails at DEGENERATE_NORM (Compose's 0.000001f) instead of clamping; velocity() answers 0 on None. a_fit_through_linearly_dependent_points_has_no_solution reports Some([NaN, NaN, NaN]) with the clamp back in place.

---

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.

Status: fixed in e10582a. Both bounds come from fling_spline_reference.py, which gained this case's own line (density=2.55 v=15250.0: distance=11057.424px duration=2.0716s), and travel is measured in pixels from a row's own on-screen extent (10527px measured). Scaling tick_fling's elapsed by 1000 reports "stopped after 8ms"; scaling its delta by 0.01 reports "travelled 111px".

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.

Status: fixed in e10582a. The first leg still cannot assert the box (an unmeasured row has to be drawn to be measured), so there is a third leg -- back again, every height known. Widening intersects_viewport downwards passes all 40 forward steps and fails at "back 6".

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.

Status: fixed in e10582a. It walks every row primitive's mask chain and requires the list's own slot on it, and rejects a chain that loops. Forcing Painter::set_mask's parent to NONE fails it with "clips to [Id(1)], a chain that never reaches the list's own mask Id(0)".

---

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.

Status of the rule findings, 2026-09-07 evening.

• list.rs:576's ungated per-frame line -- fixed in 992c472 with the rest of D1.
• routes.rs:1518's client_log_time duplicating log_ring.rs's clock_time -- moot: the route was deleted (06b8a1f).
• log_ring.rs:301's "the caller is named in the error" -- deferred to the devlog agent; client-core/src/log_ring.rs is its file this pass, and app_log.rs no longer repeats the claim.
• view.rs:624's stale VelocityTracker::add_sample -- fixed in 2ec0fee; the paragraph was rewritten for the anchoring change and now names PointerClock rather than a method that no longer exists.
• MOVE_CHAIN_LIMIT naming two chains -- fixed in a6a100e: renamed to PARENT_CHAIN_LIMIT in render_state.rs and shader.wgsl at once (it had no other users), with the doc naming both chains it governs.
• sense.rs:1434's unreproducible negative control -- fixed in 7e79ec1. Rerun with velocity reverted to (newest - oldest) / span: seven fail in -p iris (the flick recording, the accelerating flick, the horizon, the stopped finger, the minimum sample count, both drag_gesture flick tests) plus phone_screen.rs's flick. RUST.md's "exactly seven" was right; the doc comment's "exactly this one, the flick recording, and phone_screen.rs" was not, and now says the same thing RUST.md does.
• bench_client.rs:393's set_bottom_inset(.., max(..)) -- deferred to the devlog agent; iris/android-app/** was open under it this pass.

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.

---

Status of the nits, both fixed in a6a100e. DragGesture's release computes velocity() once into a local both the outcome and the iris drag release: line read. selection.rs's animate(id) is behind is_scrolling(), which is the same answer List::fling itself reached -- and phone_screen.rs's recorded flick still flings, which is the half that says the guard did not turn a working release off.

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