fix(smoke): make the screenshot-regression suite deterministic (pixel compare + blank/partial retry + Termina seam fixes)#1451
Merged
Aaronontheweb merged 18 commits intoJun 23, 2026
Conversation
…rs in config-search screenshot tape The three config-search screenshot frames (selection, brave-entry, saved) were failing consistently across unrelated Dependabot PRs because the tape used Sleep 1s before each Screenshot instead of deterministic screen anchors. Root cause (per Akka.NET async state machine and .NET concurrency analysis): Frames 1 and 2: Sleep 1s was dead weight. SelectBackendForEditing and the ProviderSelection navigation are both fully synchronous — they complete on the Termina event loop thread before the next render. Wait+Screen on the heading text is the only gate needed. Frame 3: Sleep 1s was insufficient. SaveWithoutProbeOverride fires four synchronous ReactiveProperty cascade invalidations (two Revalidate calls, an ActiveDialog reset, and the CurrentScreen=Saved assignment) plus a ReloadPersistedDraft disk read. Each invalidation re-evaluates DynamicLayoutNode._factory(), queuing an additional RedrawRequested event. A single Wait+Screen on 'validated and saved' matches after the first stable render but before all queued redraws have settled. Adding a second anchor on '[Enter] Settings Areas' — text emitted by BuildKeyBindings only when CurrentScreen==Saved and ActiveDialog==None (SearchConfigEditorPage.cs:212) — guarantees both reactive properties have committed to their final state in a single stable rendered frame before the screenshot fires.
…condary anchors to mcp-permissions All screenshot tapes used Sleep 1s before and after Screenshot commands as a timing guard. For the screens affected, Wait+Screen is sufficient: - help.tape: static CLI output; nothing renders after the text appears - provider-manager.tape: synchronous list render on navigation - wizard-screens.tape (frames 1 and 2): synchronous wizard step transitions The Sleep 1s after Ctrl+Q in wizard-screens.tape is intentionally kept — that is the alt-screen restore guard (CSI ?1049l), not a screenshot guard. mcp-permissions.tape gets secondary Wait+Screen anchors instead of Sleep: - Frame 1: Wait+Screen@5s /3 tools/ confirms tool count has loaded - Frame 2: Wait+Screen@5s /Server default/ confirms header rows are present alongside the tool rows (direct regression guard for netclaw-dev#1424) Updated stale header comments in provider-manager.tape and wizard-screens.tape that still described the removed Sleep 1s approach.
…onfig-search baselines Two separate fixes: help.tape and provider-manager.tape: restored Sleep 1s before the Screenshot command. These tapes fire their screenshot right at the start of the tape (not mid-flow), and VHS may not have flushed all terminal output into its buffer by the time Wait+Screen matches the first visible line. The Sleep is a genuine render-settle guard, not timing slop. Removed the post-screenshot Sleep (that was pure pacing and is not needed). config-search baselines (approved.png x3): the three config-search screenshot frames had stale baselines that predated a UI change — they were failing consistently on every recent Dependabot PR regardless of what code was touched. Promoted the actual screenshots from CI run 27842198884 (captured with the new deterministic Wait+Screen anchors from the previous commits) as the new approved baselines.
…cp-permissions settle config-search.tape: add secondary Wait+Screen on DuckDuckGo description text to guard against leaked Down key from Settings navigation moving the cursor to Brave before the screenshot fires. mcp-permissions.tape: add Sleep 1s settle guard between server-list screenshot and Enter navigation (daemon can push a tool re-index update in this window, causing Enter to hit the TUI mid-transition and produce a blank tool-grid screenshot); increase tool-grid anchor timeouts to 15s/10s.
…-byte cmp VHS's PNG zlib encoder produces different byte streams for visually identical renders across process invocations (filter-heuristic jitter). cmp -s false- fails on such runs even though zero pixels differ. Switch compare_shot_frame to use ImageMagick's 'compare -metric AE', which counts differing pixels and exits 0 when none differ. This tolerates encoder jitter while still catching real visual regressions (cursor on wrong row, missing UI element, etc.). Falls back to cmp -s when ImageMagick is absent. ImageMagick is already present in CI (used for the existing diff PNG path).
The pixel-comparison fix in run-smoke.sh uses 'compare -metric AE' but ImageMagick was not present in the Screenshot Regression (Linux) job, causing it to fall back to cmp -s and false-fail on PNG encoder jitter.
…500) Set CursorBlink false freezes the terminal cursor but not its on/off state. The cursor block (~160px) can be visible or hidden non-deterministically between runs even with blinking disabled. A hard AE=0 requirement false-fails on this noise. Allow up to 500 differing pixels — covers the cursor block with margin, while still catching real regressions: a misplaced cursor row differs by thousands of pixels, a blank screen by millions.
…-refresh artifact Root cause: under VHS, Termina emits a full-screen clear (\e[2J) + repaint as one atomic write when a startup ResizeEvent fires (ConsoleInputSource polls Console.WindowWidth every 10ms; the PTY size jitters as it initialises). VHS can sample the screenshot between the clear and the repaint half of that same write, capturing a blank frame (AE~68000, 1 unique color). The write is atomic from the app side — real users never see it; only VHS's mid-write PTY sampling does. Amplified by the Termina 0.12.1->0.14.0-beta.1 bump, which routes spinners through RenderFrameProvider and raises render cadence. Deterministic-in-outcome fix (test harness, production code is correct): - run-smoke.sh: detect a transient blank capture via unique-color count (identify %k < 16; blank=1 color, sparsest real frame=7627) and re-run that tape up to SHOT_BLANK_RETRIES times. A blank can no longer cause a false fail; it was never a false pass (blank != baseline). - provider-manager.tape: re-assert the list anchor immediately before the screenshot (was Sleep-then-capture, which reopened the blank window). Also widen the cursor-cell tolerance 500->1000 (AE): the shell-prompt cursor block measures ~493px (one cell); 500 left only 7px margin. 1000 (~2 cells) clears a single cursor cell while still failing on real text changes.
macOS runners ship bash 3.2 (no associative arrays). The SHOT_TAPE_FRAMES `declare -A` parsed its [help]=... entries as indexed-array assignments and aborted under `set -u` with 'line 94: help: unbound variable', failing the Native Smoke (macOS) job (which loads run-smoke.sh for the light/full modes, not just screenshots). Replace the associative array with a portable shot_tape_frames() case function. No behavior change on Linux.
…inism # Conflicts: # tests/smoke/screenshots/config-search-brave-entry.approved.png
The Termina full-refresh artifact also manifests as a PARTIAL frame: VHS samples mid-repaint so only the top rows land (seen on mcp-permissions-tool-grid — header + Audience row present, but Server enabled/default and the add/echo/ record-tasks rows missing; AE=19047 vs baseline, yet 4467 colors so the color-count blank detector misses it). Generalize the retry trigger from frame_is_blank to frame_needs_retry: blank (unique-color<16) OR differs from baseline by > SHOT_AE_TOLERANCE. A real regression also trips the AE branch but reproduces every attempt, so it still fails at compare time — only latency differs. Tolerance is now a shared global used by both the retry trigger and the compare gate.
Picks up the upstream fix for Aaronontheweb/termina#311: skip the full-screen ForceFullRefresh on a no-op resize. This eliminates the blank/partial screenshot frames at the source (a startup ResizeEvent no longer triggers a [2J clear + repaint that VHS could sample mid-write). The harness blank/partial retry stays as defense-in-depth.
…igation fix, termina#314)
Aaronontheweb
enabled auto-merge (squash)
June 23, 2026 01:05
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
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.
Summary
Makes the VHS screenshot-regression smoke suite deterministic. What started as
one flaky
config-searchtape turned out to be several independent sources ofnon-determinism — some in the test harness, some genuine timing seams in the
Termina TUI framework. This PR fixes them at the source and adds a defense-in-depth
retry so a transient capture can never produce a false fail.
Validated by 10 consecutive green "Screenshot Regression (Linux)" runs on the
final HEAD (full reruns, not single-job), with
config-search-selection— the lastholdout — passing at AE=0 every time.
Test-harness changes (
scripts/smoke/run-smoke.sh,.github/workflows/smoke.yml)deterministic across processes (same pixels, different byte stream), so
cmp -sfalse-failed. Now compares with ImageMagick
compare -metric AE; ImageMagick isinstalled in the screenshot CI job.
Set CursorBlink falsefreezes the shell-prompt cursorbut not its on/off state; the block is ~1 character cell (~493 px). The AE
tolerance clears a single cursor cell while still failing on real content changes.
< 16) or differs from baseline by more than the tolerance is treated as atransient capture and the tape is re-run (bounded). A real regression reproduces
every attempt and still fails at compare time — only latency differs.
the tape→frames map is a
casefunction instead ofdeclare -A, which wasaborting the Native Smoke (macOS) job under
set -u.Tape changes (
tests/smoke/tapes/screenshots/*.tape)Sleep-based timing guards with deterministicWait+Screenanchorsacross
config-search,mcp-permissions,provider-manager,wizard-screens,and
help.picker's last list row; the
Ollamarow before the provider-manager capture).help,config-search-brave-entry).Root-cause fixes in Termina (dependency bumps)
Two of the failures were genuine framework timing seams, fixed upstream and pulled
in here. The harness retries are now pure defense-in-depth:
0.14.0-beta.2— termina#311:a no-op resize triggered a full-screen clear + repaint (
\e[2J), which VHS couldsample mid-write → blank/partial frames.
0.14.0-beta3— termina#314:deferred navigation rendered the old page one extra time on the navigation seam,
so a screenshot taken right after
Entercould capture the page you just left(the
config-search-selectionwrong-page capture).Validation
dotnet slopwatch analyze— no new violations (no production code changed).