Fix ten nightly failures the everyday gate never ran - #192
Conversation
leaf-agent
left a comment
There was a problem hiding this comment.
Read the merged tree, regenerated examples/corpus.html with scripts/corpus.py (no drift), and traced each of the ten fixes back to the cause the description names. The Playwright-loop diagnosis, the --diffs-fg-number-*-override names against the Pierre bundle, the .lf-call-toggle.lf-ui cascade race against chrome-style.js's .lf-ui { font-size: var(--t-5) }, and the new hunk's @@ -112,4 +112,3 @@ line counts all check out. Two things, both in the widget and probe halves.
The first is a removed-intent gap rather than a regression: toolbar.dataset.lfUi = "" was inert — data-lf-ui is written twice in this tree and read nowhere — so dropping it is right, but nothing re-establishes what it was reaching for, and .lf-call-group-count (outside this diff, count.dataset.lfUi = "" in the same file) is the same inert write left standing.
Not raised, for the record so a later pass doesn't re-derive it: a changed root with no calls beneath it now has no route into the patch at all, since the link moved to the children. That is the bounded edge case the repo's threshold says to leave.
…layout change The nightly suite on main came back 10 failed. Most trace to #189, whose new pr-walkthrough page replaced the one two nightly-only tests still name and whose new package widget ships chrome the layer's markers never reached; two trace to #190's zero-height ToC anchor, which shortened the release page past a named scroll; and one is an ordering collision between Playwright's sync API and asyncio.run. - Point the two stale heading assertions at the page the example now is. - Route the data-bound diff test's comment through the anchored field, which is where #181 moved the press it still clicked. - Read the sticky sidebar's scroll position off the page instead of naming it. - Give the author's description a surface the layer's muted ink clears, and the changed rows' line numbers an ink that clears their own fill. - Build CallDiff's disclosure with offer, so an exported copy takes the press away, and give the location an href only where following it goes somewhere. - Read a covered run against what it paints, so clipped overflow stops reading as words on words. - Show a rendered diff and a scrolling code line in the corpus again, which is what the two unlit focus rings were missing. - Run the MCP stdio exchange on its own thread, since the session-scoped browser holds an asyncio loop open in the worker's main one.
#188's test_mcp_app.py raises "Already running asyncio in this thread" on the same collision: the session-scoped browser fixture holds an asyncio loop open in the worker's main thread, so an anyio.run there depends on whether that worker has run a browser test yet. One helper in interact_support answers for both modules.
cce658e to
0662e5f
Compare
…adow climb The toolbar summary and each group's count are words the widget writes, not words the page holds. Neither carried a GENERATED marker, so a drag across them quoted text no version of the markup has; both now wear data-lf-gen, the marker lf-diff gives its own injected stat. Not .lf-ui, which would put chrome-style.js's --t-5 in a cascade race with .lf-call-summary's --t-6. coveredWords walks document.body's node tree, which never enters a shadow root, so the getRootNode().host arm of its climb could not be taken.
leaf-agent
left a comment
There was a problem hiding this comment.
One finding, and it is in the half this PR's own earlier review sent it to — that review is where the mistake came from, so this corrects it rather than restating it.
data-lf-gen does not keep the counts out of a passage. passages.js asks two questions with two lists: quotable() — the reading a drag captures — is uiInside, which tests .lf-ui, [data-lf-said] and never looks at data-lf-gen; GENERATED = ".lf-ui, [data-lf-gen]" is read only by authored(), the version-diff reading. leaf.js's header states the pair outright — "data-lf-gen alone keeps it out of the diff and in reach of the anchor pass" — and passages.js says it again a few lines above GENERATED: "generated text the page authored … is diff-invisible and quotable, which is the pair a user expects". So a reader dragging across 1 changed root · 1 added · 0 removed · 2 items still gets a quotable passage. What the attribute did change is dynamicWords: the run is now its own passage cell, so the quote can no longer run out of it into the page's prose.
The marker is still the right one — it is what lf-diff puts on its +N −M stat, and these counts are the same kind of word. lf-diff also shows what the other reading costs: the line numbers a comment must not quote carry .lf-ui and data-lf-gen, and .lf-call-toggle.lf-ui in this same diff is already the answer to the font-size objection the comment raises against .lf-ui. So keep the attribute; the two places that say what it does are what need correcting, and the second is a test's stated purpose — as written the assertion reads as guarding something it does not guard.
Nothing else. The Playwright-loop diagnosis and the shared run_async thread, the --diffs-fg-number-*-override names against the Pierre bundle, the @@ -112,4 +112,3 @@ counts on the new hunk, the painted climb and its stated over-report direction, and the sidebar test's read-off-the-page scroll all check out; scripts/corpus.py regenerates examples/corpus.html with no drift.
quotable() reads .lf-ui and [data-lf-said] only, so data-lf-gen never made the injected counts unquotable. It takes them out of the version diff that authored() parses, and dynamicWords makes each run its own passage cell so a quote cannot run out of it into the page's prose. Both comments said it suppressed the quote; the attribute stays.
|
The eleventh, So |
…#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>
# Conflicts: # skills/leaf/packages/pr-review/widgets/lf-call-diff.js # tests/test_render_anchors.py
Two files conflicted with #194, which rewrote the same PR-review surfaces. `test_render_anchors.py` takes main's side whole. Both branches were removing the same `.lf-fab` click that #181 stopped drawing; main replaced it with `expect(.lf-fab-input).to_be_focused()`, and since the composer's textarea *is* that field (`composerInput = fabInput` in leaf.js), the extra click this branch added did nothing. That nightly failure is already fixed on main. `lf-call-diff.js` keeps main's `navigateToDatum` travel and this branch's chrome markers. The two sides reached the same export-safety concern from opposite ends: main added an `lfPrepareExport` hook that strips the toggle, the counts and the hrefs on the way out; this branch built the toggle with `offer`, whose `data-lf-offer` marker the standalone bake already removes. The marker is the declared mechanism — widget-elements.js says a widget writes none of the three markers by hand — so the hook goes and the flag checks with it. The counts stay in the copy as `data-lf-gen` content rather than being deleted: they are an account of the tree a reader still wants on paper, not a press to take away. The row guard stays too, since it fixes the live page as well as the copy — the header has no location to travel to, and a link inside a `<summary>` is two gestures on one box. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0149R6uAvYxfu8xSjMrbmj2z
|
Merged
Verification
|
Two nightly reds that arrived with #194's review surfaces and were never run against it, since a pull request's CI stops short of the nightly leg. The standing-state fixture is built so that a verb the registry declares with no event on that page fails rather than going unexercised, and `lf-diff`'s `review` was declared without one. It gets a diff and a standing review. `.lf-diff-review` was taking the outward focus ring it shares with `.lf-diff-next`, which stands on its own in the toolbar. This one rides a file's summary row inside a box that clips at the row's bottom edge, so its lower run fell 3px past what the corpus sweep can see. It takes the inset the summary beside it already takes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0149R6uAvYxfu8xSjMrbmj2z
Moving the review press to an inset ring split one rule in two, and the sweep reads each rule for the name it declares. Neither half had one — the toolbar's rule never did, and the geometry fault ahead of that check was what kept it quiet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0149R6uAvYxfu8xSjMrbmj2z
…rule Three things a review turned up. The comment named the exported copy, and the copy was never the loser: the export hook strips every location href before the bake, which then grants the box its own stop. `reachScrollers` is what the hidden anchor fooled, on the live page, because it reads a candidate for a focusable descendant before granting one. `offer` writes `data-lf-offer`, and the runtime's shared sheet already hides a marked press on paper, so the package's own print rule for the toggle was the second mechanism for one guarantee — the same duplicate this branch removed from the export hook. The sidebar's scroll target is halfway along a stretch that has to exist first. A page too short to lift the box off where it was authored parks the scroll behind the near edge, and the ring assertion would then report a position rather than the page that made it meaningless. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0149R6uAvYxfu8xSjMrbmj2z
A macOS build sits inside a `.app` bundle, so its executable is three levels further down than the `chrome-linux/chrome` the fixture counted back from: `parents[1]` is `Contents`, which has no `-` to split, and the fixture raised IndexError before it could look for the shell. Every test wanting the LEAF_BROWSER_EXECUTABLE path errored at setup on a developer machine, while CI's Linux layout passed. The build directory is the one named `chromium-<build>`, so it is found by that name and the root and build number come off it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0149R6uAvYxfu8xSjMrbmj2z
#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. It aligned the render tests with that contract; test_site.py is nightly-only, so its two readings went unread and stayed on the old one. The 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 it rests on was gone, not merely inverted. It reads whether the field was offered at all, which is the difference it was always after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0149R6uAvYxfu8xSjMrbmj2z
The tend bot resolved the same two files against main at 8aa71b7. Its call-diff resolution took the offer/data-lf-gen markers and kept main's preparingExport guards beside them, which is the duplicate the markers exist to remove: the standalone bake already takes a marked press out of the copy. This branch has since merged main five commits further on and run the nightly suite on the result, so the tree here is the tested one and the bot's commit is recorded rather than applied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0149R6uAvYxfu8xSjMrbmj2z
#204's three new MCP exchanges call `asyncio.run` directly, which is the loop start this branch moved off: in an xdist worker that has already opened a browser, Playwright still holds a loop on the thread and the call raises. They go through `run_async` like the rest, which hands the entry point a thread with no loop on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0149R6uAvYxfu8xSjMrbmj2z
leaf-agent
left a comment
There was a problem hiding this comment.
One finding, and it is about which PR owns a hunk rather than about the code inside it.
tests/conftest.py's headless_shell fix is also #203, and #203 got there first — its commit is 47edb74 at 20:44Z, this branch's 77205a3 at 21:38Z — with the same edit to the same line: chromium.parents[2] / parents[1].name replaced by a search up chromium.parents for the chromium-<build> directory. Whichever lands first conflicts the other, and meanwhile the same change is being reviewed twice. #203 is the narrower home for it: it exists only for #200's follow-ups, it states the macOS layout (chromium-<build>/chrome-mac/Chromium.app/Contents/MacOS/Chromium) as its cause, and it is not carrying thirteen other fixes a maintainer may want to land on a different schedule. Dropping the hunk here leaves this branch's four test_render_commands.py setup errors to #203 — which is where the PR body already attributes them.
If this branch keeps it instead, the half to keep from #203 is rsplit("-", 1). startswith("chromium-") also matches chromium-tip-of-tree-<build>, where split("-", 1)[1] yields tip-of-tree-<build>, chromium_headless_shell-tip-of-tree-<build> never exists, and the candidate loop falls back to the full Chromium binary with nothing said. The explicit AssertionError on this side is the better half of the pair — #203's bare next() raises StopIteration with nothing to read — so neither version is the one to take whole.
Nothing else on the six commits since the last review. Checked: dropping .lf-call-toggle's print rule is covered, since offer() writes data-lf-offer and chrome-style.js's @media print { [data-lf-offer]:not([data-lf-said]) } is the shared rule, while data-lf-said is written only by relabel, which this widget never calls. lf-diff's x-state.review detail schema takes exactly {file, reviewed}, the new ab-patch hunk's @@ -1,2 +1,2 @@ counts match its lines, and the standing floor is set equality, so the verb is now covered rather than merely present. .lf-diff-review's new inset is the same calc(-1 * var(--here-ring-w)) the code-summary rule beside it takes, and both halves of the split rule now name a ring. In test_site.py, a drag over the page's own words reaches openComment(anchor, "", { focus: false }), which opens the composer and so sets fabInput.style.display = "block" — so checkVisibility() is a live discriminator where document.activeElement stopped being one. And the sidebar guard reads both edges off the page before the halfway point is taken.
#203 makes the same edit to the same line of `tests/conftest.py`, and it got there first (`47edb74` at 20:44Z against this branch's `77205a3` at 21:38Z). Two open PRs rewriting one line conflict whichever lands first, and put the same change through review twice; #203 is the narrower home, existing only for #200's follow-ups and naming the macOS app-bundle layout as its cause. The four `test_render_commands.py` setup errors this hunk cleared are #203's, which is where the PR body already attributed them.
A bare `next()` over `chromium.parents` raises `StopIteration` inside a fixture, which reads as an exhausted generator rather than as a Playwright layout this code does not know. #192 had made the same fix to this line with the guard on it; that branch has dropped its copy in favour of this one, so the readable failure comes here.
Problem
cirun 33516407327 came back 10 failed, 1614 passed, 6 skipped. Eight belong to #189, whosecirun was cancelled by #190's push before the nightly leg ever ran on it. #188'stest_mcp_app.pycase 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_*ortest_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.mainhas 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 reachedtests/conftest.py'sheadless_shellfirst, so the fourtest_render_commands.pysetup 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.htmlinto 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 asgetComputedStyle(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-descriptionstacked a second translucent accent onto a card that is already a tint of one, and the layer's own--mutedreads 4.32:1 there; the description now sits on the raised-surface token. CallDiff's disclosure was a bare<button>rather than anoffer, so an exported copy kept a hand over a press nothing can take. Its location anchor carried anhrefon the header row, which names no location at all — andreachScrollersreads 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.
coveredWordsmeasured 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-lightandcode-pre-shadoware 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-numberon 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.runandanyio.run.sync_playwright()keeps an asyncio loop running in its thread for its whole lifetime, and thebrowserfixture 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()isNonebeforesync_playwright(), the loop inside it,Noneafter.interact_support.run_asyncruns 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'sreviewto 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.pyis nightly-only. Its label test turned ondocument.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-nightlyandpre-commit run --all-fileson this head. The everyday suite and lint are green on this branch in CI. Every claim about whatmaindoes was taken from a control run of the same tests in a clean checkout ofmain, not from reading.Where each failure landed
test_render_gate.py::test_the_runtime_holds_a_scroller_the_page_wrotetest_render_export.py::test_an_exported_example_stands_on_its_own[pr-walkthrough],[corpus]test_render_controls.py::test_every_ring_the_layer_draws_is_shown_whole_somewhere_in_the_corpus.lf-diff-review's ring and both ring namestest_render_pages.py::test_a_left_sidebar_uses_the_margin_until_the_page_needs_it_backtest_interact_mcp.py::test_stdio_protocol_carries_the_app_resource_and_private_tool_resulttest_mcp_app.py::test_registered_server_prefers_full_page_and_keeps_snapshot_as_fallbacktest_render_projection.py::test_the_render_gate_applies_every_standing_action_a_second_timetest_site.py::test_the_label_is_chrome_rather_than_words_to_quote,::test_a_comment_lands_in_the_thread_with_its_quotetest_render_anchors.py::test_a_data_bound_diff_aims_and_selects_one_source_linetest_site.py,test_render_export.pystale<h1>The two exported-copy cases each carried four faults, three of them masked:
offeringasserts beforecovered, 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.Three this leaves, all of them main's
test_render_controls.py::test_examples_have_no_serious_wcag_a_or_aa_violations[pr-walkthrough]stays red. The.lf-pr-descriptioncontrast above is fixed and confirmed gone. What holds the test red is 46nested-interactivefindings 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.nodeis 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_retryfails whenever anything runs before it. Confirmed on unmodifiedmain: three runs of the file at-n4, three failures, and the same failure at-n0over the whole file. It passes alone every time.assert len(requests) == 2sees 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_sentencesfails 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, becausetheme.cssasks for Charter and this runner'sfc-matchhas none of the serif stack — the same reading #175 made. Left for the authoring machine.Automated fix for failed run