docs/REVIEW-2026-09-07.md: every finding's status after the fix pass
13 fixed, 6 moot or deferred, 2 not done on purpose. Each finding gets its
own Status line in place rather than a summary at the end, so a reader who
arrives at a finding sees what happened to it; the header carries the
counts and the six commits.
The moot ones are all in the phone-logging route 06b8a1f deleted (D2's
unbounded `POST /client-log` body, D3's silently dropped lines, R3's three
copies of one wire contract, R4's `build.rs`, and the `client_log_time`
duplication) -- the app hands its log to Dev Updater through an on-device
ContentProvider now, so there is nothing left to bound or share. Two more
are deferred to the devlog agent because `iris/android-app/**` and
`client-core/src/log_ring.rs` were open under it this pass.
The two left undone are deliberate. R2 (a mask clips drawing but not
hit-testing) waits on docs/LAYOUT.md's mask redesign, since intersecting
a chain in `resolved_region` now would be a second mechanism to unpick.
R6 is a look-at-it-on-the-phone item and no build in this VM is evidence
about her device's font set.
Full checks on the tree as pulled: `cargo fmt --check` clean in `iris/`,
`server/`, `client-core/` and `event-model/`; `cargo clippy --workspace
--all-targets` exit 0 in `iris/` and `server/` (the only line is the
`future-incompatibilities` note about naga/wgpu/winit, which predates
this pass); `cargo test --workspace` 165 in `iris/`, 160 in `server/` and
157 in `client-core/`, no failures. The one thing not run is a real
device build -- `cargo ndk -t x86_64 -P 29 check -p iris` is clean, but
`-p iris-android-app` is the devlog agent's tree.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
1 parent
a6a100edc6
commit
181ba64606
1 file changed
+78
@@ -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
|
Counts: **5 defects, 7 risks, 3 tests that cannot fail in the bug's
|
||||||
direction, 7 rule findings, 2 nits.**
|
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
|
## 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
|
exists for exactly this), and/or record into the ring at `Info` while
|
||||||
leaving `android_logger` at `Debug`.
|
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
|
### 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
|
`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`
|
`: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.
|
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
|
### 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`
|
`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
|
emit one `warn!` when the sequence is not contiguous with the last batch
|
||||||
from that `source`.
|
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
|
### 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
|
`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
|
`event_time`), and seed `previous` from the previous event's last sample
|
||||||
rather than from the anchor.
|
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
|
### 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
|
`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
|
*Fix*: replace 11750 with the script's own 12250 / 12500 in those four
|
||||||
places, or say which run produced 11750.
|
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
|
## Risks
|
||||||
@@ -171,6 +196,8 @@ fault e922b73 was written to fix.
|
|||||||
extents check), consider a plain `assert!` or a one-shot `log::error!`, so
|
extents check), consider a plain `assert!` or a one-shot `log::error!`, so
|
||||||
the guard survives into the build the defect was found in.
|
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
|
### R2 — a straddling row is now invisible above the list and still tappable through the header
|
||||||
|
|
||||||
Masks are applied in the fragment shader
|
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
|
region with its mask chain in `resolved_region`; the chain walk already
|
||||||
exists on the GPU side.
|
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
|
### R3 — three copies of one wire contract, none of them linked
|
||||||
|
|
||||||
`client-core/src/log_upload.rs:28` (`MAX_LINES_PER_BATCH = 500`) and
|
`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
|
*Fix*: move `ClientLogLine`/`ClientLogBody` and the batch constant into a
|
||||||
shared crate.
|
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
|
### 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()`
|
`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
|
`read_pinned_ca`, and move the `AI_APP_CA`/`XDG_CONFIG_HOME` declarations
|
||||||
above the bench early-return.
|
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
|
### R5 — desktop density is read once and never updated
|
||||||
|
|
||||||
`iris/src/default/mod.rs:254` reads `content_scale(window)` at startup and
|
`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 is invisible here (every display on this machine is 1.0), which is why
|
||||||
it needs writing down.
|
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
|
### 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
|
`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
|
chevrons on Iris's device specifically; the emulator's font set is not
|
||||||
evidence for hers.
|
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
|
### 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
|
`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
|
`: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.
|
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
|
## 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
|
-15250 px/s at density 2.55 must run ≥ ~1.4 s and travel ≥ ~6000 px), not
|
||||||
from `FlingCalculator`.
|
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
|
### 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`
|
`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
|
half of it, and a regression that draws 20 rows in the wrong *place* on the
|
||||||
outbound leg passes.
|
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
|
### 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`
|
`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
|
*Fix*: assert that a row primitive's mask chain contains the list's mask
|
||||||
slot.
|
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
|
## Rules
|
||||||
@@ -339,6 +384,32 @@ slot.
|
|||||||
inset while `ime_visible` already says the keyboard is up. Harmless
|
inset while `ime_visible` already says the keyboard is up. Harmless
|
||||||
today; it is the same conflation the comment beside it warns about.
|
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
|
## Nits
|
||||||
|
|
||||||
- `iris/src/sense.rs:798` computes `self.velocity.velocity()` twice on a
|
- `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
|
## Commits reviewed
|
||||||
|
|
||||||
```
|
```
|
||||||
|
|||||||
Reference in new issue
Block a user