From 21583bd1fe6e0af61f8cf85e5bb828e875d244ce Mon Sep 17 00:00:00 2001 From: Jan Guth Date: Thu, 30 Jul 2026 21:20:21 +0200 Subject: [PATCH 1/5] test(e2e): harness answers DSR-6n cursor queries (dsr_reply verb) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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[;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 --- src/test/e2e/dsl.zig | 9 ++++++ src/test/e2e/harness.zig | 34 +++++++++++++++++++++++ src/test/e2e/runner.zig | 8 ++++++ tests/e2e/dsr_child_reply/config.zig | 6 ++++ tests/e2e/dsr_child_reply/golden/env.toml | 25 +++++++++++++++++ tests/e2e/dsr_child_reply/scenario.e2e | 31 +++++++++++++++++++++ 6 files changed, 113 insertions(+) create mode 100644 tests/e2e/dsr_child_reply/config.zig create mode 100644 tests/e2e/dsr_child_reply/golden/env.toml create mode 100644 tests/e2e/dsr_child_reply/scenario.e2e diff --git a/src/test/e2e/dsl.zig b/src/test/e2e/dsl.zig index 05efada2..14d42cc7 100644 --- a/src/test/e2e/dsl.zig +++ b/src/test/e2e/dsl.zig @@ -46,6 +46,7 @@ pub const Kind = enum { set_rows, set_timeout_ms, set_env, + dsr_reply, spawn, type_str, key, @@ -174,6 +175,14 @@ pub fn parse(allocator: Allocator, source: []const u8) ParseError!Script { // never emit a sequence the mouse parser would discard. if (col < 1 or col > 65535 or row < 1 or row > 65535) return ParseError.BadInteger; try cmds.append(allocator, .{ .kind = .click, .line = line_no, .int_arg = col, .int_arg2 = row }); + } else if (eq(head, "dsr_reply")) { + // dsr_reply on|off — make the harness answer DSR-6n cursor queries + // like a real terminal. Off by default (the harness is otherwise a + // pure scraper; replying changes what the child reads and would + // churn every existing golden). Needed for cursor-query round-trips. + const on = eq(tail, "on") or eq(tail, "true") or eq(tail, "1"); + if (!on and !(eq(tail, "off") or eq(tail, "false") or eq(tail, "0"))) return ParseError.UnknownDirective; + try cmds.append(allocator, .{ .kind = .dsr_reply, .line = line_no, .int_arg = if (on) 1 else 0 }); } else if (eq(head, "sleep")) { try cmds.append(allocator, .{ .kind = .sleep, .line = line_no, .int_arg = try parseInt(tail) }); } else if (eq(head, "wait_for")) { diff --git a/src/test/e2e/harness.zig b/src/test/e2e/harness.zig index 4e7921ec..bc4935b2 100644 --- a/src/test/e2e/harness.zig +++ b/src/test/e2e/harness.zig @@ -40,6 +40,13 @@ pub const Session = struct { text_buf: []u8, exited: bool = false, exit_status: u32 = 0, + /// Answer DSR-6n cursor queries like a real terminal would. Off by default: + /// the harness is otherwise a pure screen scraper, and replying changes what + /// the child receives (so it would churn every existing golden). Opt in per + /// scenario with the `dsr_reply on` verb — required to exercise anything + /// that depends on cursor-query round-trips (atty's own re-anchoring, or a + /// foreground child like atuin querying the cursor). + dsr_reply: bool = false, pub fn deinit(self: *Session) void { if (!self.exited) self.terminate(); @@ -102,6 +109,32 @@ pub const Session = struct { } } + /// Reply to every DSR-6n (`\x1b[6n`) in `chunk` with the grid's current + /// cursor as `\x1b[;R`, 1-based — what a real terminal sends back. + /// Called after `grid.feed` so the position reflects the chunk just drawn. + /// Whoever queried (atty, or a foreground child like atuin) reads it off the + /// same stdin, which is exactly the contention this models. + fn answerDsr(self: *Session, chunk: []const u8) void { + var i: usize = 0; + while (std.mem.indexOfPos(u8, chunk, i, "\x1b[6n")) |at| : (i = at + 4) { + 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; + // Write straight to the master — NOT writeInput, which drains via + // pumpMs when the buffer is full and would re-enter the pump we're + // called from. A reply is <= 12 bytes, so one write always suffices; + // best-effort on a (never observed) short write. + var off: usize = 0; + while (off < reply.len) { + const rc = std.c.write(self.master, reply[off..].ptr, reply.len - off); + if (rc <= 0) break; + off += @intCast(rc); + } + } + } + /// Pump output for up to `ms` milliseconds. Returns true if we read /// any bytes, false on timeout with nothing. pub fn pumpMs(self: *Session, ms: i32) !bool { @@ -128,6 +161,7 @@ pub const Session = struct { } self.grid.feed(buf[0..r]); try self.cast.record('o', buf[0..r]); + if (self.dsr_reply) self.answerDsr(buf[0..r]); return true; } if (pfd[0].revents & 0x018 != 0) { diff --git a/src/test/e2e/runner.zig b/src/test/e2e/runner.zig index ce03da0b..a8b71349 100644 --- a/src/test/e2e/runner.zig +++ b/src/test/e2e/runner.zig @@ -307,6 +307,7 @@ fn runScenario(io: std.Io, gpa: Allocator, sc: Scenario, atty_bin: []const u8, u var spawn_argv: []const []const u8 = &.{}; var spawn_seen = false; var first_cmd_after_spawn: usize = 0; + var dsr_reply_on = false; for (script.cmds, 0..) |c, i| { switch (c.kind) { @@ -314,6 +315,7 @@ fn runScenario(io: std.Io, gpa: Allocator, sc: Scenario, atty_bin: []const u8, u .set_rows => rows = @intCast(c.int_arg), .set_timeout_ms => timeout_ms = @intCast(c.int_arg), .set_env => try extra_env.append(gpa, .{ .key = c.str_arg, .value = c.str_arg2 }), + .dsr_reply => dsr_reply_on = c.int_arg == 1, .spawn => { if (spawn_seen) return .{ .fail = "multiple spawn directives" }; spawn_argv = c.argv; @@ -347,6 +349,8 @@ fn runScenario(io: std.Io, gpa: Allocator, sc: Scenario, atty_bin: []const u8, u .extra_env = extra_env.items, }); defer session.deinit(); + // Apply before the first pump so atty's startup cursor query gets answered. + session.dsr_reply = dsr_reply_on; // ── Pass 2: execute commands after spawn. var first_failure: ?[]const u8 = null; @@ -361,6 +365,10 @@ fn runScenario(io: std.Io, gpa: Allocator, sc: Scenario, atty_bin: []const u8, u .set_cols, .set_rows, .set_timeout_ms, .set_env, .spawn => { // Config directives must come before spawn; ignore here. }, + // 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)), .key => { const bytes = dsl.keyBytes(c.str_arg) orelse { diff --git a/tests/e2e/dsr_child_reply/config.zig b/tests/e2e/dsr_child_reply/config.zig new file mode 100644 index 00000000..d341c7ca --- /dev/null +++ b/tests/e2e/dsr_child_reply/config.zig @@ -0,0 +1,6 @@ +//! dsr_child_reply — statusbar on so atty actively tracks the cursor with its +//! own DSR-6n queries; that's the state in which it used to swallow a +//! foreground child's cursor reply (the atuin Ctrl+R hang). +const atty = @import("atty"); + +pub const statusbar: atty.StatusBar = .{ .enabled = true }; diff --git a/tests/e2e/dsr_child_reply/golden/env.toml b/tests/e2e/dsr_child_reply/golden/env.toml new file mode 100644 index 00000000..4bfd121a --- /dev/null +++ b/tests/e2e/dsr_child_reply/golden/env.toml @@ -0,0 +1,25 @@ +# atty e2e — recorded environment +atty_version = "0.8.0" +cols = 80 +rows = 24 + +[argv] +0 = "$ATTY" +1 = "bash" +2 = "--norc" +3 = "--noprofile" +4 = "-i" + +[forced_env] +PATH = "/usr/local/bin:/usr/bin:/bin:/sbin:/usr/sbin" +TERM = "xterm-256color" +LANG = "C.UTF-8" +LC_ALL = "C.UTF-8" +HOME = "/tmp" +SHELL = "/bin/sh" +USER = "test" +PS1 = "$ " + +[extra_env] +ATTY_BIN = ".zig-cache/e2e/dsr_child_reply/bin/atty" +ATTY_TRACE = "cursor" diff --git a/tests/e2e/dsr_child_reply/scenario.e2e b/tests/e2e/dsr_child_reply/scenario.e2e new file mode 100644 index 00000000..de73e676 --- /dev/null +++ b/tests/e2e/dsr_child_reply/scenario.e2e @@ -0,0 +1,31 @@ +cols 80 +rows 24 +timeout_ms 10000 +env ATTY_TRACE=cursor +dsr_reply off + +spawn $ATTY bash --norc --noprofile -i +wait_for "$" +type "__atty_osc133_d() { local __code=$?; printf '\\033]133;D;%s\\007' \"$__code\"; }\r" +wait_for "$" +type "PROMPT_COMMAND='__atty_osc133_d'\r" +wait_for "$" +type "PS1=$'\\[\\033]133;A\\007\\]# \\[\\033]133;B\\007\\]'\r" +wait_for "#" +sleep 300 + +type "echo warm\"\"up\r" +wait_for "warmup" +sleep 500 + +dsr_reply on +sleep 150 + +type "bash -c 'printf \"\\033[6n\"; IFS= read -s -r -d R -t 3 _r && M=OK || M=LOST; echo \"CURSOR\"\"-$M\"'\r" +wait_for "CURSOR-" +sleep 200 +expect_substr "CURSOR-OK" +expect_no_substr "CURSOR-LOST" + +type "exit\r" +exit_code 0 From 2c24658f54f5b59f70a832d79b452085a8e7538e Mon Sep 17 00:00:00 2001 From: Jan Guth Date: Thu, 30 Jul 2026 22:26:34 +0200 Subject: [PATCH 2/5] test(e2e): honest scope for the dsr_child_reply scenario 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 --- tests/e2e/dsr_child_reply/config.zig | 5 ++-- tests/e2e/dsr_child_reply/scenario.e2e | 37 +++++++++++++++----------- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/tests/e2e/dsr_child_reply/config.zig b/tests/e2e/dsr_child_reply/config.zig index d341c7ca..653d7f94 100644 --- a/tests/e2e/dsr_child_reply/config.zig +++ b/tests/e2e/dsr_child_reply/config.zig @@ -1,6 +1,7 @@ //! dsr_child_reply — statusbar on so atty actively tracks the cursor with its -//! own DSR-6n queries; that's the state in which it used to swallow a -//! foreground child's cursor reply (the atuin Ctrl+R hang). +//! own DSR-6n queries; that is the state in which a child's cursor reply could +//! plausibly be consumed by atty's interceptor, so it is the interesting one to +//! pin the contract under. const atty = @import("atty"); pub const statusbar: atty.StatusBar = .{ .enabled = true }; diff --git a/tests/e2e/dsr_child_reply/scenario.e2e b/tests/e2e/dsr_child_reply/scenario.e2e index de73e676..9d299c6e 100644 --- a/tests/e2e/dsr_child_reply/scenario.e2e +++ b/tests/e2e/dsr_child_reply/scenario.e2e @@ -1,26 +1,33 @@ +# A foreground child that queries the cursor must receive the terminal's DSR-6n +# reply. atty issues its own `ESC[6n` queries to track the prompt and strips the +# `ESC[r;cR` answers off stdin so the shell never sees them as keystrokes — so +# there is a real risk of it consuming a reply that belongs to a child (atuin's +# Ctrl+R search is the motivating case). +# +# `dsr_reply on` makes the harness answer cursor queries like a real terminal. +# Without it no replies exist at all and none of this is observable: the harness +# is otherwise a pure screen scraper. +# +# SCOPE: this is a CHARACTERISATION test of the contract "a foreground child gets +# its cursor reply", and it passes on today's atty. It is NOT a regression test +# for the atuin hang — that failure is still unreproduced (see #553). It guards +# the contract against future changes to the DSR interception path. +# +# The child is a separate `bash -c` (its own process group, so it owns the +# terminal foreground) rather than a builtin, matching how atuin runs. The +# marker is assembled at runtime (`"CURSOR""-$M"`) so the result strings never +# appear in the echoed command line — otherwise the assertions would match the +# echo instead of the child's output. + cols 80 rows 24 timeout_ms 10000 -env ATTY_TRACE=cursor -dsr_reply off +dsr_reply on spawn $ATTY bash --norc --noprofile -i wait_for "$" -type "__atty_osc133_d() { local __code=$?; printf '\\033]133;D;%s\\007' \"$__code\"; }\r" -wait_for "$" -type "PROMPT_COMMAND='__atty_osc133_d'\r" -wait_for "$" -type "PS1=$'\\[\\033]133;A\\007\\]# \\[\\033]133;B\\007\\]'\r" -wait_for "#" sleep 300 -type "echo warm\"\"up\r" -wait_for "warmup" -sleep 500 - -dsr_reply on -sleep 150 - type "bash -c 'printf \"\\033[6n\"; IFS= read -s -r -d R -t 3 _r && M=OK || M=LOST; echo \"CURSOR\"\"-$M\"'\r" wait_for "CURSOR-" sleep 200 From 3386f92f3bee216b8ac1f5fb001066a2fc5a7b68 Mon Sep 17 00:00:00 2001 From: Jan Guth Date: Thu, 30 Jul 2026 22:34:41 +0200 Subject: [PATCH 3/5] =?UTF-8?q?fix(e2e):=20address=20review=20round=201=20?= =?UTF-8?q?=E2=80=94=20split=20queries,=20reply=20shape,=20plumbing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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[;`) 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 --- src/test/e2e/dsl.zig | 4 ++++ src/test/e2e/dsl_tests.zig | 10 ++++++++++ src/test/e2e/harness.zig | 24 ++++++++++++++++++++++-- src/test/e2e/runner.zig | 3 +-- tests/e2e/dsr_child_reply/scenario.e2e | 15 ++++++++++----- 5 files changed, 47 insertions(+), 9 deletions(-) diff --git a/src/test/e2e/dsl.zig b/src/test/e2e/dsl.zig index 14d42cc7..aff6dbda 100644 --- a/src/test/e2e/dsl.zig +++ b/src/test/e2e/dsl.zig @@ -6,6 +6,10 @@ //! rows //! timeout_ms # default 5000, caps wait_for + wait_stable //! env KEY=VALUE +//! dsr_reply on|off # answer DSR-6n cursor queries like a real +//! # terminal (default off — the harness is +//! # otherwise a pure screen scraper). Needed for +//! # anything depending on a cursor round-trip. //! spawn # argv0 is the binary (token-split, no quoting yet) //! # if argv0 is "$ATTY", harness substitutes the binary //! type "string" [pattern] # quoted; \n \r \t \\ \" \xNN supported. diff --git a/src/test/e2e/dsl_tests.zig b/src/test/e2e/dsl_tests.zig index 47064b16..5245c889 100644 --- a/src/test/e2e/dsl_tests.zig +++ b/src/test/e2e/dsl_tests.zig @@ -132,3 +132,13 @@ test "DSL string escapes" { defer s.deinit(); try std.testing.expectEqualStrings("a\tb\ncA", s.cmds[0].str_arg); } + +test "DSL dsr_reply parses on/off and rejects anything else" { + var s = try parse(std.testing.allocator, "dsr_reply on\ndsr_reply off\n"); + defer s.deinit(); + try std.testing.expectEqual(@as(usize, 2), s.cmds.len); + try std.testing.expectEqual(Kind.dsr_reply, s.cmds[0].kind); + try std.testing.expectEqual(@as(i64, 1), s.cmds[0].int_arg); + try std.testing.expectEqual(@as(i64, 0), s.cmds[1].int_arg); + try std.testing.expectError(ParseError.UnknownDirective, parse(std.testing.allocator, "dsr_reply maybe\n")); +} diff --git a/src/test/e2e/harness.zig b/src/test/e2e/harness.zig index bc4935b2..2e283a80 100644 --- a/src/test/e2e/harness.zig +++ b/src/test/e2e/harness.zig @@ -27,6 +27,10 @@ pub const SpawnOpts = struct { rows: u16, forced_env: []const KV, extra_env: []const KV, + /// Answer DSR-6n cursor queries like a real terminal. Set at construction so + /// it is live before the first pump — a post-spawn assignment would miss + /// atty's startup query if `spawn` ever grew a pump of its own. + dsr_reply: bool = false, }; pub const Session = struct { @@ -47,6 +51,8 @@ pub const Session = struct { /// that depends on cursor-query round-trips (atty's own re-anchoring, or a /// foreground child like atuin querying the cursor). dsr_reply: bool = false, + /// Bytes of the `\x1b[6n` query matched so far, carried across reads. + dsr_match: usize = 0, pub fn deinit(self: *Session) void { if (!self.exited) self.terminate(); @@ -115,8 +121,21 @@ pub const Session = struct { /// Whoever queried (atty, or a foreground child like atuin) reads it off the /// same stdin, which is exactly the contention this models. fn answerDsr(self: *Session, chunk: []const u8) void { - var i: usize = 0; - while (std.mem.indexOfPos(u8, chunk, i, "\x1b[6n")) |at| : (i = at + 4) { + // Incremental match so a query SPLIT ACROSS READS is still answered — + // `\x1b[6n` can straddle a chunk boundary, and a per-chunk substring + // search would silently miss it (an intermittent, timing-dependent + // no-reply, i.e. a CI flake). + const query = "\x1b[6n"; + for (chunk) |b| { + if (b == query[self.dsr_match]) { + self.dsr_match += 1; + if (self.dsr_match < query.len) continue; + } else { + // Mismatch — restart, but this byte may itself open a new match. + self.dsr_match = if (b == query[0]) 1 else 0; + continue; + } + self.dsr_match = 0; var buf: [32]u8 = undefined; const reply = std.fmt.bufPrint(&buf, "\x1b[{d};{d}R", .{ self.grid.cur_row + 1, @@ -279,5 +298,6 @@ pub fn spawn(allocator: Allocator, opts: SpawnOpts) !Session { .grid = grid, .cast = cast, .text_buf = text_buf, + .dsr_reply = opts.dsr_reply, }; } diff --git a/src/test/e2e/runner.zig b/src/test/e2e/runner.zig index a8b71349..7ac32ffa 100644 --- a/src/test/e2e/runner.zig +++ b/src/test/e2e/runner.zig @@ -347,10 +347,9 @@ fn runScenario(io: std.Io, gpa: Allocator, sc: Scenario, atty_bin: []const u8, u .rows = rows, .forced_env = &forced_env, .extra_env = extra_env.items, + .dsr_reply = dsr_reply_on, }); defer session.deinit(); - // Apply before the first pump so atty's startup cursor query gets answered. - session.dsr_reply = dsr_reply_on; // ── Pass 2: execute commands after spawn. var first_failure: ?[]const u8 = null; diff --git a/tests/e2e/dsr_child_reply/scenario.e2e b/tests/e2e/dsr_child_reply/scenario.e2e index 9d299c6e..0b117898 100644 --- a/tests/e2e/dsr_child_reply/scenario.e2e +++ b/tests/e2e/dsr_child_reply/scenario.e2e @@ -8,10 +8,14 @@ # Without it no replies exist at all and none of this is observable: the harness # is otherwise a pure screen scraper. # -# SCOPE: this is a CHARACTERISATION test of the contract "a foreground child gets -# its cursor reply", and it passes on today's atty. It is NOT a regression test -# for the atuin hang — that failure is still unreproduced (see #553). It guards -# the contract against future changes to the DSR interception path. +# WHAT THIS GUARDS: "a foreground child's cursor reply is never consumed by +# atty's CPR filter." It passes on today's atty — it is a characterisation test +# pinning that contract against future changes to the DSR interception path, NOT +# a regression test for the atuin hang (still unreproduced, see #553). +# +# The child validates the SHAPE of what it received (`ESC[;`), not just +# that some R-terminated blob arrived: atty leaking its own CPR into the child +# would satisfy the weaker check while actually violating the contract. # # The child is a separate `bash -c` (its own process group, so it owns the # terminal foreground) rather than a builtin, matching how atuin runs. The @@ -28,11 +32,12 @@ spawn $ATTY bash --norc --noprofile -i wait_for "$" sleep 300 -type "bash -c 'printf \"\\033[6n\"; IFS= read -s -r -d R -t 3 _r && M=OK || M=LOST; echo \"CURSOR\"\"-$M\"'\r" +type "bash -c 'printf \"\\033[6n\"; re=\"\\[[0-9]+;[0-9]+$\"; IFS= read -s -r -d R -t 3 _r && { [[ $_r =~ $re ]] && M=OK || M=BAD; } || M=LOST; echo \"CURSOR\"\"-$M\"'\r" wait_for "CURSOR-" sleep 200 expect_substr "CURSOR-OK" expect_no_substr "CURSOR-LOST" +expect_no_substr "CURSOR-BAD" type "exit\r" exit_code 0 From 735b6ba74561f7a1379377420bb3ada3cd6d1f33 Mon Sep 17 00:00:00 2001 From: Jan Guth Date: Thu, 30 Jul 2026 22:37:15 +0200 Subject: [PATCH 4/5] =?UTF-8?q?fix(e2e):=20address=20review=20round=201=20?= =?UTF-8?q?(copilot)=20=E2=80=94=20retry=20writes,=20pre-spawn=20scope?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- src/test/e2e/harness.zig | 23 +++++++++++++++++++---- src/test/e2e/runner.zig | 6 +++++- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/test/e2e/harness.zig b/src/test/e2e/harness.zig index 2e283a80..2e767fb9 100644 --- a/src/test/e2e/harness.zig +++ b/src/test/e2e/harness.zig @@ -143,13 +143,28 @@ pub const Session = struct { }) catch continue; // Write straight to the master — NOT writeInput, which drains via // pumpMs when the buffer is full and would re-enter the pump we're - // called from. A reply is <= 12 bytes, so one write always suffices; - // best-effort on a (never observed) short write. + // called from. Retry INTR/AGAIN rather than dropping the reply: a + // dropped reply strands whoever queried, which surfaces as an + // intermittent scenario timeout. Bounded so a wedged fd can't hang + // the run; EPIPE/EIO (child gone) exits immediately. var off: usize = 0; + var retries: usize = 0; while (off < reply.len) { const rc = std.c.write(self.master, reply[off..].ptr, reply.len - off); - if (rc <= 0) break; - off += @intCast(rc); + if (rc > 0) { + off += @intCast(rc); + continue; + } + switch (std.posix.errno(rc)) { + .INTR => continue, + .AGAIN => { + retries += 1; + if (retries > 50) break; + var ts = std.c.timespec{ .sec = 0, .nsec = std.time.ns_per_ms }; + _ = std.c.nanosleep(&ts, null); + }, + else => break, + } } } } diff --git a/src/test/e2e/runner.zig b/src/test/e2e/runner.zig index 7ac32ffa..30431db0 100644 --- a/src/test/e2e/runner.zig +++ b/src/test/e2e/runner.zig @@ -315,7 +315,11 @@ fn runScenario(io: std.Io, gpa: Allocator, sc: Scenario, atty_bin: []const u8, u .set_rows => rows = @intCast(c.int_arg), .set_timeout_ms => timeout_ms = @intCast(c.int_arg), .set_env => try extra_env.append(gpa, .{ .key = c.str_arg, .value = c.str_arg2 }), - .dsr_reply => dsr_reply_on = c.int_arg == 1, + // Only a PRE-spawn directive sets the initial state; one placed after + // `spawn` is a mid-run toggle and is applied by pass 2 instead. + .dsr_reply => if (!spawn_seen) { + dsr_reply_on = c.int_arg == 1; + }, .spawn => { if (spawn_seen) return .{ .fail = "multiple spawn directives" }; spawn_argv = c.argv; From deb7874ecc340bdf88a62121227dc0567efe8118 Mon Sep 17 00:00:00 2001 From: Jan Guth Date: Thu, 30 Jul 2026 22:45:17 +0200 Subject: [PATCH 5/5] =?UTF-8?q?fix(e2e):=20address=20review=20round=202=20?= =?UTF-8?q?=E2=80=94=20stale=20golden,=20honest=20scope,=20toggle=20reset?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/test/e2e/harness.zig | 4 ++++ src/test/e2e/runner.zig | 6 +++++- tests/e2e/dsr_child_reply/golden/env.toml | 1 - tests/e2e/dsr_child_reply/scenario.e2e | 6 ++++-- 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/test/e2e/harness.zig b/src/test/e2e/harness.zig index 2e767fb9..bb78cce2 100644 --- a/src/test/e2e/harness.zig +++ b/src/test/e2e/harness.zig @@ -136,6 +136,10 @@ pub const Session = struct { continue; } self.dsr_match = 0; + // Position is the cursor AFTER the whole chunk was fed, not at the + // query byte — so several queries batched into one read all get the + // same answer. Fine for liveness/shape assertions; revisit if a + // scenario ever asserts the coordinates themselves. var buf: [32]u8 = undefined; const reply = std.fmt.bufPrint(&buf, "\x1b[{d};{d}R", .{ self.grid.cur_row + 1, diff --git a/src/test/e2e/runner.zig b/src/test/e2e/runner.zig index 30431db0..7ed8a35c 100644 --- a/src/test/e2e/runner.zig +++ b/src/test/e2e/runner.zig @@ -371,7 +371,11 @@ fn runScenario(io: std.Io, gpa: Allocator, sc: Scenario, atty_bin: []const u8, u // 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, + .dsr_reply => { + session.dsr_reply = c.int_arg == 1; + // Drop any partial match carried from before the toggle. + session.dsr_match = 0; + }, .type_str => try session.typeWith(c.str_arg, @enumFromInt(c.int_arg)), .key => { const bytes = dsl.keyBytes(c.str_arg) orelse { diff --git a/tests/e2e/dsr_child_reply/golden/env.toml b/tests/e2e/dsr_child_reply/golden/env.toml index 4bfd121a..34c4884d 100644 --- a/tests/e2e/dsr_child_reply/golden/env.toml +++ b/tests/e2e/dsr_child_reply/golden/env.toml @@ -22,4 +22,3 @@ PS1 = "$ " [extra_env] ATTY_BIN = ".zig-cache/e2e/dsr_child_reply/bin/atty" -ATTY_TRACE = "cursor" diff --git a/tests/e2e/dsr_child_reply/scenario.e2e b/tests/e2e/dsr_child_reply/scenario.e2e index 0b117898..d8234fc5 100644 --- a/tests/e2e/dsr_child_reply/scenario.e2e +++ b/tests/e2e/dsr_child_reply/scenario.e2e @@ -14,8 +14,10 @@ # a regression test for the atuin hang (still unreproduced, see #553). # # The child validates the SHAPE of what it received (`ESC[;`), not just -# that some R-terminated blob arrived: atty leaking its own CPR into the child -# would satisfy the weaker check while actually violating the contract. +# that some R-terminated blob arrived — so stray non-CPR bytes can't be mistaken +# for a reply. Note the limit: a CPR that atty leaked is itself CPR-shaped, so +# this separates "a cursor report" from "anything else", not "the child's own +# reply" from "atty's". # # The child is a separate `bash -c` (its own process group, so it owns the # terminal foreground) rather than a builtin, matching how atuin runs. The