Skip to content

Stabilize option evidence selection test - #191

Merged
max-sixty merged 3 commits into
mainfrom
codex/fix-option-evidence-selection
Sep 1, 2026
Merged

Stabilize option evidence selection test#191
max-sixty merged 3 commits into
mainfrom
codex/fix-option-evidence-selection

Conversation

@max-sixty

@max-sixty max-sixty commented Sep 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Chromium 151 can collapse a synthetic text range on mouse release, which made the option-evidence interaction test flaky even though Leaf had already opened the passage composer. This changes the test to drag between actual glyph bounds, assert the evidence selection while the pointer is held, and wait for the composer's focus after release before verifying that the option remains unanswered.

The positive control clicks the same evidence paragraph after the composer closes, preserving the production contract that selecting evidence does not answer an option while clicking those words does. The merged tree passed 30 consecutive focused runs and all 43 option-render tests; the review adjustment then passed another 30 consecutive focused runs.

This was written by Codex on behalf of max-sixty

leaf-agent
leaf-agent reviewed Sep 1, 2026
View reviewed changes

@leaf-agent leaf-agent left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The selection rewrite itself holds up: I ran the file on Chrome 151.0.7922.173 and got 43/43, with the changed test green 10/10 in isolation. Dropping removeAllRanges() for Escape is safe too — I instrumented the test and the selection comes back empty and collapsed after the press, because the composer's Escape rung runs dismissFab, which calls pageSelection()?.removeAllRanges().

One thing I'd push back on is the positive control moving to the label. #ro-column-p is the option's evidence prose and <strong> is its label, so the pair no longer reads against the same element: the negative half asserts that a drag across the evidence doesn't answer, and the control that made it non-vacuous was a plain click on that same paragraph answering. With the control on the label, a regression that made every click inside #ro-column-p a non-pick — worksInside widening, or the [data-lf-offer] capture guard reaching further — leaves this test green. tests/CLAUDE.md puts it as "arranging the test so that a green result could only have come from the behavior named by the test", and the behavior named here is the gesture discriminating, not the region.

The suite has nothing else pinning it: #ro-column-p appears only in this test, and every other option press is either the label or a centre-of-element click that lands on prose without naming it. Reverting just that line still passes here — 10/10 on the same Chrome, with the rest of the new arrangement intact — so unless the move was buying stability I couldn't reproduce, I'd keep the control where it was.

Comment thread tests/test_render_options.py Outdated
@max-sixty
max-sixty merged commit 2326a8d into main Sep 1, 2026
9 checks passed
@max-sixty
max-sixty deleted the codex/fix-option-evidence-selection branch September 1, 2026 17:25
This was referenced Sep 1, 2026
Merged
Merged
max-sixty pushed a commit that referenced this pull request Sep 1, 2026
@leaf-agent
Wait for the posted comment's mark before aiming the next diagram box (…
8aa71b7 
…#196)

## Problem

[`ci` run
33537688138](https://github.com/max-sixty/leaf/actions/runs/33537688138)
on
[`2326a8d`](2326a8d)
came back **11 failed, 1618 passed, 6 skipped**. Ten of those eleven are
already fixed by #192. The eleventh is not, and is nobody else's:
`test_render_aim.py::test_a_declared_box_takes_its_comment_on_every_type_that_carries_an_id`
aims at the next diagram box while the comment it just posted is still
arriving, and the arrival takes the response surface down under the
gesture.

## Solution

`round_trip` ends when the page has heard back what it sent, which is
before it has drawn what came back. Applying the comment repaints the
diagram's marks and hangs its `lf-mark-note` on the element, and that
repaint takes an open response surface down with it — so an aim placed
in the gap opens a composer the arriving comment then closes. The test
now waits for that note before the next aim, which is the same ordering
`test_a_declared_flowchart_node_keeps_its_comment_across_renderings`
already states for the same widget.

Instrumented on `main`, the failing run reads (`performance.now()` ms,
from a probe on the fab bar and on `#life`):

```
2723  second Alt-click — the Queued composer closes
2744  composer reopens on Build, focused; bar placed at (634, 472)
2803  ['life', childList, LF-DIAGRAM]  + ['life', childList, BUTTON lf-mark-note]
2803  .lf-composer display:none, .lf-fab-bar display:none, .lf-fab-input display:none
```

The aim at 2744 was correct — the bar's `aria-label` read `Respond to
diagram · Build` — and the projection landing at 2803 closed it. That is
why the same test failed as `Locator expected to be focused` in CI and
as `Locator expected to be visible` locally: `showFab(null)` hides the
composer, then the bar, so which assertion in `open_compact_comment`
catches it depends on where the 5s budget lands.

This is the gesture-ahead-of-the-page class `running-tend` names, so the
fix is the ordering statement rather than a retry.

## Testing

Reproduced on `main` at roughly 1 in 25–40 runs under four-worker
contention (failures at iterations 4, 38, 7, and 65 of separate loops);
`#life > .lf-mark-note` was absent at the aim in every captured failure.
With the wait, **180 consecutive runs** of the test under the same
contention passed, plus `uv run pytest tests/test_render_aim.py
--run-nightly` (34 passed), the everyday `uv run pytest tests` (764
passed, 6 skipped), and `pre-commit run --files
tests/test_render_aim.py`.

<details><summary>Why this is a separate PR from #192</summary>

#192 fixes the other ten failures in the same run and does not touch
`tests/test_render_aim.py`; the two changes share no file and either
could be reverted without the other. #192's own
`tests/interact_support.py` helper already covers
`test_mcp_app.py::test_registered_server_prefers_full_page_and_keeps_snapshot_as_fallback`,
which is new in this run because #188's `ci` was cancelled by #191's
push before its nightly leg ran, so `d55e574` never had a nightly
result.

</details>

<details><summary>One thing this leaves</summary>

A reader can do what the test did: Alt-click a box in the window between
sending a comment and its projection landing, and have the composer they
just opened close under them. `refreshFab` tears the surface down when
`placeFab` cannot resolve the anchor's box during that repaint, which is
a product question about whether an arriving projection may dismiss a
response surface opened on a different anchor — not something to settle
inside a test fix.

</details>

---

Automated fix for [failed
run](https://github.com/max-sixty/leaf/actions/runs/33537688138)

Co-authored-by: leaf-agent <318509791+leaf-agent@users.noreply.github.com>
max-sixty added a commit that referenced this pull request Sep 1, 2026
@leaf-agent @max-sixty @claude
Fix ten nightly failures the everyday gate never ran (#192)
8018cba 
## Problem

[`ci` run
33516407327](https://github.com/max-sixty/leaf/actions/runs/33516407327)
came back **10 failed, 1614 passed, 6 skipped**. Eight belong to #189,
whose `ci` run was cancelled by #190's push before the nightly leg ever
ran on it. #188's `test_mcp_app.py` case arrived on the same collision
as the MCP test below and is carried here rather than left to red the
next nightly.

The everyday suite that gates a pull request does not run
`test_render_*` or `test_site.py`, so a PR can land its nightly half
unread — which is how these arrived, and how five more have arrived
since, from #193, #194 and #200. `main` has moved a long way under this
branch; the ones it now owns are taken on merge, and three of the new
ones are cleared here. #203 reached `tests/conftest.py`'s
`headless_shell` first, so the four `test_render_commands.py` setup
errors are its.

## Solution

Grouped by cause rather than by test, since several tests share one.

**The example the tests still name.** #189 rewrote
`examples/pr-walkthrough.html` into the Worktrunk review packet, and the
shipped patch is now a collapsed manifest that builds no lines until a
reader opens a file, which the scroller gate read as
`getComputedStyle(null)`. The gate reads the diffs that have drawn
lines, with its own non-vacuity floor kept.

**The new package's chrome and surfaces.** `.lf-pr-description` stacked
a second translucent accent onto a card that is already a tint of one,
and the layer's own `--muted` reads 4.32:1 there; the description now
sits on the raised-surface token. CallDiff's disclosure was a bare
`<button>` rather than an `offer`, so an exported copy kept a hand over
a press nothing can take. Its location anchor carried an `href` on the
header row, which names no location at all — and `reachScrollers` reads
a candidate for a focusable descendant before granting it a stop, so
that hidden anchor answered "there is already a way in here" with a link
nobody can reach, for a box whose words run off the side.

**A reading that counted clipped text as painted.** `coveredWords`
measured each run's whole rect, so an ellipsised name in a narrow column
read as covering its neighbour. It now intersects each run with the
boxes that clip it, stopping at an out-of-flow ancestor where a hidden
overflow further out need not reach. The one test that plants this fault
plants it properly now: the runtime's note is parked in a one-pixel box
with hidden overflow, so opacity alone never put its characters on the
screen.

**Three focus rings the corpus stopped painting.** `code-pre-light` and
`code-pre-shadow` are the tab stops on a scrolling code block and on a
diff's lines, and with the old example gone no example held either. The
page gets one rendered hunk and one code line long enough to scroll.
Then `.lf-diff-review`: it took the outward ring it shared with
`.lf-diff-next`, which stands on its own in the toolbar, while this one
rides a file's summary row inside a box that clips at the row's bottom
edge — its lower run fell 3px past what the sweep can see. It takes the
inset the summary beside it already takes, and splitting the rule made
each half declare the ring name neither had.

**`--diffs-fg-number` on changed rows.** Surfaced by that rendered hunk:
a changed row's number is drawn in the base its own fill is mixed from,
green on green at 4.03:1. Pierre's number overrides now take the layer's
tinted-surface inks.

**A sidebar the page got shorter under.** #190's zero-height ToC anchor
shortened the release page 58px, putting a named `scrollTo(0, 900)` past
the stretch where the box stands on its own offset. Both edges of that
stretch are the page's, so they are read off it, with a floor asserting
the stretch exists before a point halfway along it says anything.

**Playwright's loop against `asyncio.run` and `anyio.run`.**
`sync_playwright()` keeps an asyncio loop running in its thread for its
whole lifetime, and the `browser` fixture is session-scoped per xdist
worker — so an MCP test's own loop start raises in any worker that has
opened a browser and passes in one that has not, leaving the schedule to
decide. Verified directly: `asyncio.events._get_running_loop()` is
`None` before `sync_playwright()`, the loop inside it, `None` after.
`interact_support.run_async` runs the entry point on a thread with no
loop on it, and both MCP modules go through it — #204's three new
exchanges included, since they landed on the direct call while this was
in flight.

**A declared verb no page replayed.** #194 added `lf-diff`'s `review` to
the registry without an event for it on the standing-state page — the
one fixture built so that exactly this fails rather than going
unexercised. It gets a diff and a standing review.

**A focus contract two site tests never heard about.** #193 kept the
response field passive so a drag leaves the browser's own selection
alone and a native copy still has something to take, and aligned the
render tests with it. `test_site.py` is nightly-only. Its label test
turned on `document.activeElement === field`, which is now false for the
page's own words and the site's label alike — the discriminator was
gone, not inverted — so it reads whether the field was offered at all,
which is the difference it was always after.

## Testing

`uv run pytest tests --run-nightly` and `pre-commit run --all-files` on
this head. The everyday suite and lint are green on this branch in CI.
Every claim about what `main` does was taken from a control run of the
same tests in a clean checkout of `main`, not from reading.

<details><summary>Where each failure landed</summary>

| test | fix |
| --- | --- |
|
`test_render_gate.py::test_the_runtime_holds_a_scroller_the_page_wrote`
| reads the diffs that drew lines |
|
`test_render_export.py::test_an_exported_example_stands_on_its_own[pr-walkthrough]`,
`[corpus]` | widget, probe, theme |
|
`test_render_controls.py::test_every_ring_the_layer_draws_is_shown_whole_somewhere_in_the_corpus`
| example, then `.lf-diff-review`'s ring and both ring names |
|
`test_render_pages.py::test_a_left_sidebar_uses_the_margin_until_the_page_needs_it_back`
| scroll position read off the page |
|
`test_interact_mcp.py::test_stdio_protocol_carries_the_app_resource_and_private_tool_result`
| own thread |
|
`test_mcp_app.py::test_registered_server_prefers_full_page_and_keeps_snapshot_as_fallback`
| own thread |
|
`test_render_projection.py::test_the_render_gate_applies_every_standing_action_a_second_time`
| standing fixture (#194's) |
| `test_site.py::test_the_label_is_chrome_rather_than_words_to_quote`,
`::test_a_comment_lands_in_the_thread_with_its_quote` | the field is
offered, not entered (#193's) |
|
`test_render_anchors.py::test_a_data_bound_diff_aims_and_selects_one_source_line`
| **#194's and #193's, taken on merge** |
| `test_site.py`, `test_render_export.py` stale `<h1>` | **#202's, taken
on merge** |

The two exported-copy cases each carried four faults, three of them
masked: `offering` asserts before `covered`, which asserts before the
copy's axe run, so each fix uncovered the next. The ring sweep did the
same — the geometry fault stood in front of a naming check that had been
silent about a rule missing its name since it was written.

</details>

<details><summary>Three this leaves, all of them main's</summary>


**`test_render_controls.py::test_examples_have_no_serious_wcag_a_or_aa_violations[pr-walkthrough]`
stays red.** The `.lf-pr-description` contrast above is fixed and
confirmed gone. What holds the test red is 46 `nested-interactive`
findings from #194: each file's "Mark reviewed" `<button>` sits inside
that file's `<summary>`, and a disclosure with a focusable descendant is
a serious WCAG failure. Clearing it means taking the press out of the
disclosure and re-laying the row — `entry.node` is the `<details>` in
ten places, and the theme, print and export rules key on it — which is a
change to that widget's shape with visual verification of its own, not a
line this branch should slip in.


**`test_render_anchors.py::test_a_failed_fragment_hydration_waits_for_a_reader_retry`
fails whenever anything runs before it.** Confirmed on unmodified
`main`: three runs of the file at `-n4`, three failures, and the same
failure at `-n0` over the whole file. It passes alone every time.
`assert len(requests) == 2` sees one — the reopened disclosure's second
fragment fetch never reaches the route. Arrived with #189.


**`test_render_anchors.py::test_a_drag_released_mid_word_hugs_words_and_sentences`
fails over the whole file and passes alone.** `assert 'inside' ==
'inside it'` — the selection did not grow to the word boundary. It looks
like the next instance of what #191 diagnosed, Chromium 151 collapsing a
synthetic range on mouse release.

Also still behind: the gallery's `docs/example-pr-walkthrough.jpg`.
Regenerating here rewrites all nine stills in substituted faces, because
`theme.css` asks for Charter and this runner's `fc-match` has none of
the serif stack — the same reading #175 made. Left for the authoring
machine.

</details>

---

Automated fix for [failed
run](https://github.com/max-sixty/leaf/actions/runs/33516407327)

> _This was written by Claude Code on behalf of @max-sixty_

---------

Co-authored-by: leaf-agent <318509791+leaf-agent@users.noreply.github.com>
Co-authored-by: Maximilian Roos <m@maxroos.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@leaf-agent leaf-agent mentioned this pull request Sep 2, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leaf-agent leaf-agent leaf-agent approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@max-sixty @leaf-agent