test(e2e): harness answers DSR-6n cursor queries - #555
Conversation
The e2e harness was a pure screen scraper — it never replied to a cursor query, so any DSR round-trip (atty's own re-anchoring, or a foreground child like atuin querying the cursor) was untestable. Add an opt-in responder: dsr_reply on|off makes the pump answer ESC[6n with the grid's cursor as ESC[<row>;<col>R. Opt-in per scenario so existing goldens are untouched. NOTE: the accompanying dsr_child_reply scenario documents the child-reply contract but does NOT yet reproduce the atuin hang (it passes on master too) — see PR #553. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Revalidating the cherry-picked commit against master caught two things: - a leftover `env ATTY_TRACE=cursor` from the debugging session, which would have spewed atty's cursor trace into the scenario's terminal; - comments in both the scenario and its config.zig implying the test guards against atty swallowing a child's cursor reply. It does not: it passes on today's atty. Reworded to state plainly that it is a CHARACTERISATION test of the contract "a foreground child receives its cursor reply", not a regression test for the (still unreproduced) atuin hang. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds opt-in terminal-like DSR-6n cursor query answering to the Zig e2e harness so scenarios can observe cursor round-trips (needed for validating atty’s DSR interception behavior and foreground-child interactions like atuin’s Ctrl+R).
Changes:
- Extend the e2e DSL with
dsr_reply on|offand thread the flag through the runner into the session. - Implement a DSR responder in the harness pump loop that replies to
ESC[6nwithESC[<row>;<col>R(1-based) using the VT grid cursor. - Add a new
dsr_child_replycharacterization scenario + per-scenario config to exercise the contract.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/dsr_child_reply/scenario.e2e | New characterization scenario that asserts a foreground child receives a DSR cursor reply. |
| tests/e2e/dsr_child_reply/golden/env.toml | Recorded environment snapshot for the new scenario. |
| tests/e2e/dsr_child_reply/config.zig | Scenario-specific config enabling statusbar to ensure atty issues its own DSR queries. |
| src/test/e2e/runner.zig | Plumbs dsr_reply from DSL into session (pre-spawn and mid-run toggle). |
| src/test/e2e/harness.zig | Adds Session.dsr_reply and emits DSR replies during pumpMs. |
| src/test/e2e/dsl.zig | Adds dsr_reply directive parsing and command kind. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Subagent (fix-and-ship; it mutation-verified the scenario FAILS with `dsr_reply off`, so the test is not vacuous): - bug/med: a `\x1b[6n` split across two reads was never answered — the old per-chunk substring search silently missed it, i.e. a timing-dependent CI flake. Replaced with an incremental matcher whose state carries across reads. - improvement/med: the scenario asserted only that SOME R-terminated blob arrived, which atty leaking its own CPR into the child would also satisfy — the inverse of the contract being tested. The child now validates the shape (`ESC[<row>;<col>`) and reports BAD, which the scenario asserts against. - improvement/low: `dsr_reply` moved into SpawnOpts so it is live at construction; the post-spawn assignment would have missed atty's startup query if `spawn` ever grew a pump. - nit/low: documented the verb in the DSL grammar header + added parse tests (on/off and the reject path). - Reframed the scenario header around the contract it guards rather than its (historical) role in falsifying the #553 fix. Left as noted, not fixed: the reply reports the end-of-chunk cursor, so N queries batched in one chunk share one answer — correct for a liveness check, and nothing asserts the coordinate value. The reply is also not recorded to the cast ('i' events come from writeInput); cast.json is uncommitted for every scenario, so no golden is affected. Re-verified: FAILS with the responder off, PASSES with it on. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…scope - answerDsr retried nothing: a reply hitting EINTR or EAGAIN on the master was silently dropped, stranding whoever queried and surfacing as an intermittent scenario timeout. Now retries both (bounded: 50 × 1ms for EAGAIN) and exits immediately on EPIPE/EIO. - runner pass 1 applied `.dsr_reply` unconditionally while scanning the WHOLE script, so a directive placed after `spawn` — meant as a mid-run toggle — also set the initial state. Pass 1 now only honours pre-spawn directives; pass 2 keeps handling the toggles. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/test/e2e/harness.zig:143
answerDsrwrites synthetic cursor replies directly to the PTY master but does not record them incast.jsonas input events. That makes the asciinema recording incomplete and can hinder replay/debugging ofdsr_reply onscenarios.
var buf: [32]u8 = undefined;
const reply = std.fmt.bufPrint(&buf, "\x1b[{d};{d}R", .{
self.grid.cur_row + 1,
self.grid.cur_col + 1,
}) catch continue;
src/test/e2e/runner.zig:375
- When
dsr_replyis toggled mid-run,session.dsr_matchis not reset. If the toggle happens while a partial\x1b[6nmatch is in progress, re-enabling can mis-detect or miss the next query. Reset the incremental matcher whenever the feature is toggled.
// Also honoured pre-spawn (applied at session creation); allowed
// here so a scenario can toggle the terminal's DSR answering
// mid-run.
.dsr_reply => session.dsr_reply = c.int_arg == 1,
.type_str => try session.typeWith(c.str_arg, @enumFromInt(c.int_arg)),
… reset Subagent (verdict: ship; it re-derived the matcher against 14 vectors — including `ESC ESC [6n`, `ESC[ESC[6n`, byte-splits and partial-then-mismatch — and ran its own negative control confirming the scenario reports LOST with the responder off): - golden/env.toml still carried `ATTY_TRACE = "cursor"` from my debugging run. It can't fail CI (env.toml is written under --update and never compared) but it would have reappeared on the next re-record. Regenerated. - The scenario comment overclaimed: it said the shape check guards against atty leaking its own CPR, but a leaked CPR is itself CPR-shaped and would pass. The check separates "a cursor report" from "anything else" — now stated that way. - Documented that the reply carries the end-of-chunk cursor, so several queries batched into one read share an answer (fine for shape/liveness assertions; revisit if a scenario ever asserts coordinates). - A `dsr_reply` toggle now clears any partial match carried across it. Copilot round 2: no findings. Left as noted: the EAGAIN retry arm is effectively dead (the master is opened blocking) — kept as cheap defence-in-depth; answerDsr doesn't cast.record its reply, so the recording is input-incomplete, but cast.json is uncommitted for every scenario so no golden is affected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Review loop complete — 2 rounds (Copilot + subagent in parallel). Round 1 — 6 findings addressed:
Round 2 — 4 findings addressed:
Verification: full e2e suite 36 pass / 0 fail / 0 updated (twice — before and after the matcher change), and the scenario was mutation-checked to FAIL with Subagent verdicts: fix-and-ship → ship. All threads replied + resolved. |
The gap
The e2e harness is a screen scraper, not a terminal emulator: it spawns atty on a PTY and reads everything atty prints into a VT grid, but it never writes anything back in response. A real terminal answers
ESC[6nwithESC[<row>;<col>R— ours never did.So under test, no cursor replies exist at all, and every DSR round-trip is unobservable: atty's own prompt re-anchoring, and any foreground child (atuin's Ctrl+R) that queries the cursor. That's a structural blind spot over
src/cursor_dsr.zig+ the proxy's stdin interception — the subsystemCLAUDE.mdflags as delicate.What this adds
dsr_reply on|offDSL verb (also settable pre-spawn, and togglable mid-scenario).Session.pumpMs: aftergrid.feed, everyESC[6nin the chunk is answered with the grid's current cursor asESC[<row>;<col>R, 1-based.dsr_child_replyscenario.Opt-in by design. Default is off, so no existing golden changes — verified by a full-suite run: 36 pass, 0 fail, 0 updated (both known flakes included).
Two implementation notes:
writeInput— the latter drains throughpumpMswhen the buffer is full, which would re-enter the pump it's called from (and formed a circular inferred error set).grid.feed, so the position reflects the chunk just drawn.Honest scope of the bundled scenario
dsr_child_replyis a characterisation test of the contract "a foreground child receives its cursor reply." It passes on today's atty and is not a regression test for the atuin hang — that failure is still unreproduced (draft #553).Its real value so far was the opposite of the usual: this harness capability is what let me disprove my own fix in #553. Three separate repro attempts, built on this responder, all passed against unfixed master — which is why #553 is held as a draft instead of merged. A test rig that can falsify a proposed fix is worth having even when it isn't yet catching the bug.
zig fmt,zig build,zig build test, and the full e2e suite are green.