diff --git a/docs/REVIEW-2026-09-07.md b/docs/REVIEW-2026-09-07.md index 4215fa4..aaa1ed1 100644 --- a/docs/REVIEW-2026-09-07.md +++ b/docs/REVIEW-2026-09-07.md @@ -26,6 +26,21 @@ 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 @@ -62,6 +77,8 @@ 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 @@ -83,6 +100,8 @@ host's disk fills and every other component's log goes with it. `: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` @@ -103,6 +122,8 @@ the module doc for `dropped` claims it is "reported rather than inferred" 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 @@ -127,6 +148,8 @@ every later event. `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 @@ -148,6 +171,8 @@ 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 @@ -171,6 +196,8 @@ fault e922b73 was written to fix. 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 @@ -186,6 +213,8 @@ testing applies the shape") is design, not code. 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 @@ -205,6 +234,8 @@ 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()` @@ -225,6 +256,8 @@ upload fails with a TLS error nobody can attribute. `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 @@ -236,6 +269,8 @@ 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 @@ -253,6 +288,8 @@ on characters the platform might not have", stated and then accepted. 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 @@ -262,6 +299,8 @@ 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 @@ -281,6 +320,8 @@ settles on the first tick passes this test. -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` @@ -290,6 +331,8 @@ 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` @@ -301,6 +344,8 @@ 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 @@ -339,6 +384,32 @@ slot. 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 @@ -351,6 +422,13 @@ slot. --- +**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 ```