test: stop asserting a 200ms wall-clock budget when opening Help - #142
Merged
Conversation
`open_help_uses_the_composers_single_200ms_budget` asserted that opening Help with a stalled markdown renderer completed in under 300ms. That is a coin flip on a shared CI runner: it failed twice in one day on unrelated PRs (314ms and 340ms, both green on re-run), taxing every PR that happened to land on a slow macOS machine. The tight bound was trying to prove two different things at once. Split them: - The 200ms budget's arithmetic is already pinned deterministically, with no clock, by tests/whats_new_composer.rs — one_absolute_deadline_is_shared_and_ observed_remaining_decreases asserts every document receives the single `opened_at + WHATS_NEW_COMPOSE_TIMEOUT` instant rather than a fresh timeout each, and already_expired_open_uses_precomputed_fallbacks_without_delegation asserts an expired deadline skips delegation entirely. - What the controller test uniquely proves is that Help does not WAIT on the renderer. The fixture stalls for 60s, so a bound of a tenth of that separates "fell back on the deadline" from "waited for the renderer" with two orders of magnitude of headroom, and cannot flake on a slow runner. Renamed to what it now proves, and the stall duration is a named constant so the bound is derived from it rather than an unexplained literal. Verified live: with the composer's budget perturbed to 45s the test fails (45.005s > bound); reverted, it passes in 0.21s. AGENTS.md gains the standing rule this came from — don't assert a tight time budget, don't prove a negative by sleeping, and where a wait is the point, pin the arithmetic in a clock-free unit test and let the behavioural test assert only that the code did not wait. Generous timeout assertions (src/proc.rs, src/update/gateway.rs) stay legitimate and are called out as such. Also records the two other determinism rules this repo learned the hard way: never send a key that assumes state a pty journey has not observed, and reproduce a Linux-only failure in a Linux container before claiming a fix.
…cked assertions Stale facts an agent would copy: Intent::ALL was documented as '39 today' when it is 41 on main, and the stack line pinned ratatui 0.30.1 while the lock is on 0.30.2. Replace the intent count with an instruction to read it from the source (a number in prose goes stale silently) and loosen the ratatui pin to 0.30.x, which is the claim that actually matters. Adds the rule today's near-miss called for: never weaken a spec-backed assertion to make a change pass. An agent fixing an unreachable! arm removed TreeScrollRight from focus_policy::unavailable_from_pinned AND deleted it from the AC-31/AC-32 test, turning a spec-required notice into a silent no-op; nothing in this file forbade it. The existing 'spec is the contract' bullet governs authoring specs, not quietly editing away the tests that encode them.
…tant Fixes a hole this PR introduced, found by review: relaxing the controller test's bound to 10s meant NOTHING caught a budget widened to 5s. The claim that the composer tests already pinned 200ms was wrong — one_absolute_deadline_is_shared_ and_observed_remaining_decreases proved every delegate got the SAME deadline but never compared it to opened_at, and already_expired_open_uses_precomputed_ fallbacks_without_delegation subtracts the same constant, so both were self-referential about the value. - The composer test now captures `opened_at`, asserts WHATS_NEW_COMPOSE_TIMEOUT is 200ms, and asserts the shared deadline is exactly one budget after the Help-open instant. Clock-free, and it fails the moment the budget moves. - The controller test bounds the WAIT on its own named constant (HELP_STALLED_RENDERER_MAX_WAIT = 2s, ~10x the budget, 30x below the 60s stall) instead of STALLED_RENDERER_WAIT / 6. Those two numbers answer different questions; deriving one from the other silently changed the tolerated latency whenever the fixture's lifetime moved. Verified with the perturbation that previously slipped through: with the budget at 5s, and again at 45s, BOTH tests now fail. Clean, both pass (0.39s / 0.03s). AGENTS.md corrections from the same review: - The timing rule cited src/proc.rs as a generous-timeout example; it asserts a 100ms timeout completes within 250ms, which is the tight shape the rule condemns. Names the real good shape (src/update/gateway.rs, tests/render_delegate.rs) and lists proc.rs, render.rs and controller_async.rs's sleep-based negatives as offenders to fix rather than copy. - Records the honest exception: AC-22/AC-23 mandate a 300ms latency criterion, so help_open_switch_scroll_each_within_300ms must hold a stopwatch. That is the spec, not a testing choice. - Names the two guard tests behind the spec-backed-assertion rule. - Fixes a stale line: the Editor Launcher runs the editor in-process, it does not open a herdr pane (line 67 contradicted the herdr integration section).
CI failed this PR on `render::tests::general_renderer_timeout_has_one_bounded_reap_tail` (macOS 1.96.0) — one of the exact sites the review flagged, flaking on cue while the PR was busy documenting it as a known offender. Both it and `proc::tests::wait_bounded_times_out_and_reaps_a_stalled_child` asserted that a 100ms timeout completed within 250ms. That is the tight timeout-plus-slack shape, not a generous bound: 2.5x the measured operation is a coin flip on a loaded runner. Their actual claim is BOUNDED vs UNBOUNDED — both fixtures stall for 60s — so a 5s bound proves it with 12x headroom below the stall and 50x above the timeout, and cannot flake. Verified the assertions still bite: replacing `wait_bounded` with a plain `child.wait()` makes the test FAIL at 60.01s. (The `ok` line in that run is the stall fixture, which is itself a #[test].) AGENTS.md's offender list shrinks accordingly: proc.rs and render.rs now join gateway.rs and render_delegate.rs as the shape to copy, leaving controller_async.rs's sleep-based negatives as the one outstanding case.
…e tells The last outstanding case from the review. Three tests proved "no render was dispatched" by sleeping 50ms and hoping a wrong render would have landed by then; two more slept 50ms to give a superseded render a chance to arrive. - Adds Controller::render_seq (a read-only accessor; both dispatch_render and dispatch_reflow bump latest_seq synchronously BEFORE spawning the worker), and the three absence tests now assert an unchanged seq. That is a real proof, not a race: verified by dropping the `width_changed` gate so a height-only change reflows — the test fails. - The two superseded-render tests wait on an in-flight counter reaching zero instead of a fixed sleep, so they wait for the worker actually finishing rather than guessing how long that takes. HONEST LIMIT, recorded in the code and in AGENTS.md rather than glossed: making that wait deterministic does not make the detection reliable. Renders run a thread each and the polling loop drains earlier results while waiting for the newest, so perturbing poll's `seq == latest_seq` guard does NOT fail those two tests. They remain end-to-end sanity checks; the guard's real proof would be a unit test feeding poll a stale-seq result directly. Better to write the limit down than to leave a future reader trusting them past it. No 50ms blind sleeps remain in the file. Note for PR #137: it adds an identical render_seq on its branch, so it will need its copy dropped when it rebases onto main.
…itations
Round-2 review findings (gpt-5.6-sol). The earlier blocker was confirmed closed;
these are the three should-fixes.
1. My concurrency description was simply wrong. There is ONE long-lived render
worker (Controller::spawn_worker) taking jobs over a channel in order and
collapsing a backlog — not a thread per render. So results cannot overtake
each other, and the superseded-render tests need no wait at all: c.rs's result
landing already means every earlier job finished or was collapsed. The
in-flight counter added last commit is therefore unnecessary, and its general
claim ("reading zero is permanent") was unsound anyway, since a queued job may
not have entered the provider yet. Removed it, and the comments now describe
the real worker. render_seq's doc no longer says "before spawning the worker".
2. The 5s bounds on the two bounded-vs-unbounded assertions let a real regression
through: the reviewer inserted a 4s sleep between kill and wait and both tests
still passed. Tightened to 2s — 20x the requested 100ms timeout, far above the
314-340ms loaded-runner flakes this PR removed, and now failing that 4s reap
tail at 4.11s (verified).
3. AGENTS.md cited two tests that exist only on the unmerged pinned-preview
branch; standing instructions on main must be true in a fresh clone of main.
Replaced with an on-main example (src/intent.rs's exhaustive AC-N3 read-only
matrix). Also corrected the claim that every named fixture stalls 60s —
tests/render_delegate.rs uses 30s and an endless loop — and dropped the
reference to the helper this commit deletes.
Verified every code symbol AGENTS.md cites exists in this tree.
The one limit this PR had been documenting rather than closing. The end-to-end
superseded-render tests look like they prove `poll`'s `seq == latest_seq` guard
and do not: their polling loop drains the earlier result while waiting for the
newest, so removing the guard leaves them green.
Adds GatedContent, a renderer that reports when each render STARTS and blocks
until the test releases it. That makes the ordering happen on demand instead of
hoping for it: hold a.rs mid-render, dispatch b.rs behind it so latest_seq moves
on, then release a.rs so its result is produced and sent carrying a stale seq.
b.rs's render starting is the proof that a.rs's result has already been sent —
the single ordered worker takes the next job only after sending the previous
result — so no sleeping is involved anywhere.
Verified both directions: the new test fails ("a superseded result must be
DROPPED by poll") when the guard is removed, while the two end-to-end tests stay
green under the same perturbation, which is precisely the gap it fills.
AGENTS.md's rule is inverted accordingly: force the race with a gate first, and
document a limit only where the race genuinely cannot be forced from outside.
That is the better lesson, and this PR now demonstrates it rather than asserting
it.
smarzban
added a commit
that referenced
this pull request
Aug 4, 2026
Brings v1.15.0, the agent-skill --cwd fix (#141) and the determinism work (#142) onto the branch. Four resolutions worth naming: - src/controller/mod.rs: both sides had added `render_seq` at different offsets, so git merged BOTH copies silently rather than conflicting. Dropped this branch's earlier copy and kept main's, whose doc describes the real single ordered worker (this branch's said "before it spawns the worker", which was wrong — there is one long-lived worker, not a thread per render). - CHANGELOG.md: the merge landed the pinned-preview entry inside the RELEASED 1.15.0 section, which would have claimed an unshipped feature was in that release. The repo's own pinned_preview_docs_cover_the_frozen_reference_contract guard caught it; moved to Unreleased. The advisory-notices entry stays deleted per the release decision. - AGENTS.md: took main's wording in both hunks — it replaced the two facts this branch had merely bumped (Intent::ALL's count, the ratatui patch version) with an instruction to read them from the source. - tests/docs_consistency.rs: kept both sides' additions (this branch's ARCHITECTURE const and main's --cwd drift guard). Green after merge: cargo test, fmt, clippy.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes the wall-clock flake that has been failing unrelated PRs, and records the rule it came from.
The flake
open_help_uses_the_composers_single_200ms_budgetasserted that opening Help with a deliberately stalled markdown renderer finished in under 300ms. On a shared CI runner that is a coin flip: it failed twice in one day on unrelated PRs — 314ms on #141 and 340ms on the v1.15.0 release PR — passing on re-run both times.Why the bound existed, and why it can go
The tight bound was proving two things at once, and only one of them needs a clock:
tests/whats_new_composer.rs:one_absolute_deadline_is_shared_and_observed_remaining_decreasesasserts every document receives the singleopened_at + WHATS_NEW_COMPOSE_TIMEOUTinstant rather than a fresh timeout each, andalready_expired_open_uses_precomputed_fallbacks_without_delegationasserts an expired deadline skips delegation. Neither needs wall-clock tolerance.So the test keeps its real claim, loses the flaky one, and is renamed to
open_help_falls_back_instead_of_waiting_for_a_stalled_renderer. The wait is bounded by its own named constant (HELP_STALLED_RENDERER_MAX_WAIT= 2s, ~10x Help's budget and 30x below the 60s stall), separate fromSTALLED_RENDERER_WAITwhich is only the fixture's lifetime — two numbers answering different questions should not be derived from each other.Every other assertion in the test is unchanged: the fixture handshake,
help_open(), and the non-empty plain-text fallback body.Verification
open_help: the controller test fails. The two halves catch a widened budget and a blocking Help independently.render_seqabsence assertions were each perturbed (unconditional height reflow, reflow for syntax content, wrap-toggle on code): each fails.child.kill()andchild.wait()fails the two reap-tail tests at 4.11s, which the earlier 5s bound allowed.cargo test,cargo fmt --check,cargo clippy --all-targets -- -D warningsgreen.AGENTS.md
Adds the standing rule this came from, plus the two other determinism lessons this repo learned the hard way today (the pinned-preview e2e that was red 4/4 on ubuntu because it pressed a toggle against unobserved state, and the "reproduce a Linux-only failure in a Linux container before claiming a fix" rule). Generous timeout assertions —
src/proc.rs,src/update/gateway.rs— are explicitly called out as legitimate, so the rule bans the coin flip rather than all timing tests.No CHANGELOG entry: test and contributor-doc only, no user-facing behaviour.
Beyond the original flake
Reviewed twice by
gpt-5.6-sol. Round 1 found a blocker: the first relaxation left nothing catching a budget widened to 5s, because the composer tests were self-referential about its value. Round 2 confirmed that closed and found three more, all fixed here: a wrong worker model in my comments (there is one ordered worker with backlog collapse, not a thread per render), 5s reap bounds that let a 4s regression through, and AGENTS.md citing tests that exist only on an unmerged branch.The PR now also removes the last blind 50ms sleeps from
tests/controller_async.rs, addsController::render_seqas the synchronous no-dispatch tell, and records in code and in AGENTS.md the one limit that cannot be fixed from outside: the superseded-render tests do not provepoll's seq guard, and the unit test that would is named.Note for PR #137: it adds an identical
render_seqon its branch and will need its copy dropped when it rebases.