fix(smoke): anchor mcp-permissions screenshot on TUI-only text#1574
Merged
Aaronontheweb merged 5 commits intoJul 3, 2026
Merged
Conversation
The mcp-permissions-server-list frame intermittently captured the raw shell transcript instead of the rendered `netclaw mcp permissions` TUI (CI run 28680686756, job "Screenshot Regression (Linux)" on PR netclaw-dev#1573). Root cause: both Wait+Screen anchors preceding the Frame 1 Screenshot matched text that is already sitting in the shell scrollback BEFORE the TUI paints, so both waits returned instantly: - `Wait+Screen@15s /smoke-math/` matched the setup output "Added MCP server 'smoke-math' (stdio)" and "...adjust approvals for 'smoke-math'." printed by `netclaw mcp add`. - `Wait+Screen@5s /3 tools/` matched the tape's OWN typed readiness loop, still visible on screen: `until netclaw mcp list 2>/dev/null | grep -q 'connected (3 tools)'; do sleep 2; done`. With both anchors pre-satisfied, the only remaining delay was the documented `Sleep 3s` settle guard. On a loaded CI runner the TUI took longer than 3s to paint, so Screenshot fired while the screen still showed the shell transcript, failing the byte-for-byte baseline comparison. This violates the tapes/README.md rule to anchor every step on stable substrings from the NEXT view. Fix: anchor Frame 1 on strings that only the rendered TUI produces: - `MCP Permissions` — the page title from McpToolPermissionsPage.BuildHeader; proves the alternate screen buffer painted. Nothing in the setup transcript prints this string. - `Connected, 3 tools` — the exact server-row text from McpToolPermissionsPage.BuildServerList, rendered as "{Name} ({Status}, {ToolCount} tools)". The daemon reports the state as PascalCase "Connected" (McpConnectionState.ToString()); the CLI transcript form is lowercase and comma-less ("connected (3 tools)"), so the two can never collide. The `Sleep 3s` settle guard is kept: it protects against a different hazard (daemon state refresh immediately after first render) and the committed baseline was captured with it in place. The tool-grid frame's anchors (/record-tasks/, /Server default:/) and the other screenshot tapes were audited for the same hazard: all of their pre-capture anchors match only post-TUI content, so no other changes were needed. Validation: `scripts/smoke/run-smoke.sh screenshots` passed 3 consecutive runs locally, all 9 frames matching baselines (mcp-permissions-server-list at AE=0 each run).
Aaronontheweb
added
bug
Aaronontheweb
enabled auto-merge (squash)
July 3, 2026 20:28
…ferred-capture race)
The Frame-1 anchor fix on this branch exposed a second, distinct defect on
Frame 2 (mcp-permissions-tool-grid) in CI run 28682629818: the capture showed
the restored shell transcript even though the tool grid had rendered and its
Wait+Screen anchors matched.
Root cause: VHS Screenshot is not synchronous — it flushes the current
framebuffer on a later render tick. Frame 2's Screenshot is the only capture in
the tape immediately followed by a screen-destroying key (Ctrl+Q); the TUI
teardown restores the main buffer before the deferred capture lands, so VHS
writes the transcript instead of the tool grid. Proven with a minimal `less`
alt-screen repro (Screenshot->immediate-q captures the main buffer;
Screenshot->Sleep->q captures the alt screen). This is exactly the hazard the
tapes README already documents ("one after so the next keystroke cannot leak
into the capture") — Frame 2 had the before-sleep but was missing the after.
- mcp-permissions.tape: add Sleep 2s between the Frame 2 Screenshot and Ctrl+Q.
- provider-manager / config-search / wizard-screens: same latent
Screenshot->Ctrl+Q pattern with no settle; add Sleep 1s before the teardown
key to end the class (wizard-screens had its Sleep on the wrong side of Ctrl+Q).
- README: clarify the after-sleep must sit between Screenshot and the next
screen-changing key, with the VHS async-capture rationale.
Validation: 3 consecutive green `run-smoke.sh screenshots` runs (VHS v0.11.0);
mcp-permissions-tool-grid now AE=0 on the first attempt with zero retries.
Tape/doc-only change; no .cs touched.
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.
Summary
Fixes a flaky screenshot-regression smoke frame:
mcp-permissions-server-listintests/smoke/tapes/screenshots/mcp-permissions.tapeintermittently captured the raw shell transcript instead of the renderednetclaw mcp permissionsTUI, failing the byte-for-byte baseline comparison.Found via artifact analysis of the "Screenshot Regression (Linux)" failure on PR #1573 (CI run 28680686756). The failure is independent of that PR — the hazard is latent in the tape on
devand can fire on any sufficiently loaded runner.Diagnosis (Frame 1)
The uploaded
mcp-permissions-server-list.actual.pngshowed the shell with thenetclaw mcp permissionscommand just typed — the TUI had not painted yet. BothWait+Screenanchors guarding the Frame 1Screenshotmatched text already present in the shell scrollback before the TUI ever launches, so both returned instantly:Wait+Screen@15s /smoke-math/— pre-matched by the setup output printed bynetclaw mcp add:Added MCP server 'smoke-math' (stdio)andNext: run `netclaw mcp permissions` to grant tools and adjust approvals for 'smoke-math'.Wait+Screen@5s /3 tools/— pre-matched by the tape's own typed readiness loop, still visible on screen:until netclaw mcp list 2>/dev/null | grep -q 'connected (3 tools)'; do sleep 2; doneWith both anchors pre-satisfied, the only remaining delay before
Screenshotwas the documentedSleep 3ssettle guard. On a loaded CI runner the TUI took longer than 3s to paint, so the capture fired on the raw shell. The later frame in the same tape (mcp-permissions-tool-grid) passed AE=0 because its anchors (/record-tasks/,/Server default:/) only match post-TUI content.This violates the authoring conventions in
tests/smoke/tapes/README.md(rule 5: anchor every state change on stable substrings from the next view; everyScreenshotmust be preceded by an anchoredWait+Screenproving we are on the right screen).Fix (Frame 1)
Replace the two ambiguous Frame 1 anchors with strings only the rendered TUI produces (timeouts preserved):
Wait+Screen@15s /smoke-math/Wait+Screen@15s /MCP Permissions/McpToolPermissionsPage.BuildHeader()(new TextNode("MCP Permissions")). Nothing in the setup transcript prints this string; matching it proves the TUI painted.Wait+Screen@5s /3 tools/Wait+Screen@5s /Connected, 3 tools/McpToolPermissionsPage.BuildServerList():$"{s.Name} ({s.Status}, {s.ToolCount} tools)", whereStatusis the daemon's PascalCaseMcpConnectionState.Connected.ToString()(via the/api/mcp/statusesDTO). The transcript form emitted bynetclaw mcp listis lowercase and comma-less —connected (3 tools)(McpCommand.cs) — so the two can never collide. This also anchors the frame on the settled state the baseline expects (server row rendered as Connected with all 3 tools indexed).The
Sleep 3ssettle guard is deliberately kept: it protects against a different hazard (a daemon state refresh repaint immediately after first render — the documented reason it exists) and the committed baseline was captured with it in place.Frame 2: the deferred-capture race (root cause of run 28682629818)
After the Frame 1 fix went green, CI run 28682629818 (this branch) failed on Frame 2,
mcp-permissions-tool-grid. Itsactual.pngshowed the shell transcript (main buffer) withnetclaw mcp permissionstyped and nothing after it — even though Frame 1 in the same tape run passed AE=0, proving the TUI had painted on the alternate screen. So the terminal was back on the main buffer by the time Frame 2 was captured.This is not a TUI crash and not the same anchor bug. Evidence, from the collected artifacts of that run:
run-screenshots.logshowsWait 30s Screen record-tasksandWait 10s Screen Server default:both matched (no timeout), vhs exited 0, and the post-tape assertion logged OK — on all 5 attempts. So the tool grid genuinely rendered and the anchors correctly matched TUI-only text.tape-shot-mcp-permissions.gifshows the tool grid (server: smoke-math,Audience,Server enabled,Server default: [Auto], theadd/echo/record-tasksrows) rendered and stable for ~17 frames, then a clean transition to the shell transcript at the exact frameCtrl+Qfires. Pixel-diffingactual.pngagainst the GIF frames: it is far closer to the post-Ctrl+Q transcript frame (AE≈88k) than to the stable tool-grid frames (AE≈131k).cli-stop, exit 0) with no exception/stack trace, and the transcript ends cleanly with no error text — ruling out a crash.run_shot_tape_with_retryclears both PNGs before each retry and re-ran 5×; the transcript reproduced every time — it is deterministic, not stale mixing.Root cause: VHS's
Screenshotdoes not write the PNG synchronously at the tape line. It hands the request to the render loop, which flushes the current framebuffer on a later tick. Frame 2'sScreenshotis the only capture in the tape immediately followed by a screen-destroying key (Ctrl+Q), with no settle between them.Ctrl+Qtears the TUI off the alternate screen buffer and restores the shell transcript on the main buffer before the deferred capture fires — so VHS writes the transcript, not the tool grid. Frame 1'sScreenshotis followed by moreWait+Screensteps that keep the same screen up, so its deferred capture is safe — which is exactly why fixing Frame 1 "moved" the failure onto Frame 2's latent race (before the fix, Frame 1 broke the retry loop first).Mechanically proven with a minimal isolated repro using
less(same alt-screen buffer, pinned VHS v0.11.0):Screenshot→ immediateq(quit less) → PNG captured the main-buffer scrollback (the bug).Screenshot→Sleep 1s→q→ PNG correctly captured the alt screen (the fix).This is precisely the hazard
tests/smoke/tapes/README.mdalready documents — "[Screenshot is] bracketed bySleep 1s… one after so the next keystroke cannot leak into the capture." Frame 2 had the before-sleep but was missing the after-sleep. A prior fix iteration had dropped it.Fix (Frame 2 + class hardening)
mcp-permissions.tape— addSleep 2sbetween the Frame 2ScreenshotandCtrl+Q, holding the tool grid on screen until the deferred capture lands. (The primary fix.)provider-manager.tape,config-search.tape,wizard-screens.tape— these have the identicalScreenshot→Ctrl+Qteardown pattern with no settle between (wizard-screenseven had itsSleep 1son the wrong side ofCtrl+Q). They pass today only on timing luck. Added aSleep 1sbefore their final teardown key to end the whole class rather than let the next runner move the failure again.tests/smoke/tapes/README.md— clarified that the after-sleep must sit between theScreenshotand the next screen-changing key (never after it), with the VHS async-capture reason spelled out.No product/
.cschanges — this is entirely tape/harness authoring correctness.Audit of sibling frames/tapes (Frame 1 anchors)
Checked every other capture's preceding anchors against what the transcript contains at that point:
mcp-permissionsFrame 2 (/record-tasks/,/Server default:/): matched only inside the already-painted TUI, and neither string appears in the setup output. OK.config-search.tape: seeded transcript containsbackend=duckduckgo(lowercase) and jq JSON; anchors like/Choose the backend Netclaw uses for web search/,/DuckDuckGo works without setup/,/Brave Search requires an API key/,/\[Enter\] Settings Areas/never appear in it. OK.provider-manager.tape(/Ollama/): no setup commands precede the launch; the preamble exports contain no "Ollama". OK.wizard-screens.tape(/Choose your LLM provider:/,/7\. Venice\.ai/,/Who will interact with this Netclaw instance/): no transcript collision. OK.help.tape(/Usage:/): the shell output is the intended capture. OK.Validation
scripts/smoke/run-smoke.sh screenshots— 3 consecutive local runs, all green (VHS v0.11.0, the CI-pinned version; reusing this run's CI-built linux-x64 binaries): all 9 frames matched baselines each run.mcp-permissions-tool-gridnow passes at AE=0 on the first attempt with zero retries (previously it retried 5× and then failed the comparison);mcp-permissions-server-listat AE=0 every run.mcp-permissionsruns green,mcp-permissions-tool-gridAE=0 every time — directly exercising the Frame 2 race in isolation.lessalt-screen repro confirms the VHS deferred-capture mechanism and that a settle sleep betweenScreenshotand the teardown key eliminates it.pwsh -File scripts/Add-FileHeaders.ps1 -Verify— all files have headers. No.cschanged, sodotnet slopwatch analyzehas no new surface (tape/doc-only diff).