pipe: a filter that fails says so, and `| head -1` stops failing

Two things made the selection pipe feel like it had never worked. It runs —
test/snapshots/pipe.snap drives the real binary through a pty and filters
`alpha beta` to `ALPHA BETA` — but it had no way to tell you when it did not,
and one of its failure conditions was not a failure at all.

IT NOW SAYS WHY. `runOne` read the command's stderr into memory and freed it
two lines later, unread; every caller answered a failed filter with a bare
`return`; `pipeResponse` had eight more silent exits under that. So `| trr`
(a typo), `| grep nomatch` (exit 1), `| jq .` on bad JSON — all did nothing,
said nothing, and left the text alone with no way to find out why. The runner
carries a `Failure` home instead: which selection, what became of the command,
and its own stderr. The core turns that into an `+Errors` buffer — acme's name
for output that came from the program rather than from a word anybody clicked:

    | trr

    exit status 127
    sh: line 1: trr: command not found

An output buffer rather than the message row because the useful half of a
shell failure is the text the shell wrote, and a 256-byte row would keep the
label and throw away the reason. Focus stays with the file: `openRead` moves
`p.active` to what it opens, which is right for a Grep you asked to read and
wrong for a report you did not — you want to fix the command and press `|`
again. A host with no `pull_pipe` at all (the detached daemon, the browser,
the board) now says that too, instead of answering failure into the void.

`| head -1` NOW WORKS. `writer_context.ok` was part of the success condition,
so a command that stopped reading its stdin failed the filter even though it
had done exactly its job: `head` takes the line it wants and closes the pipe,
the write gets EPIPE, and a selection bigger than the 64 KiB pipe buffer was
enough to trigger it. helix joins its input task and ignores the result for
this reason; the exit status is the whole verdict. Also reported rather than
swallowed: the ten-second timeout, the output ceilings, and a file edited
while the filter ran — one keystroke during a slow command used to discard the
result in a way indistinguishable from the filter doing nothing.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_016Q4RATpafkwahrovHQLKRf

1 files changed, 133 insertions, 26 deletions

diff --git a/src/selection_pipe.zig b/src/selection_pipe.zig
index c1104010..8e721751 100644
--- a/src/selection_pipe.zig
+++ b/src/selection_pipe.zig
@@ -65,19 +65,60 @@ pub const Job = struct {
     }
 };
 
-/// One worker answer. `outputs` owns each slice; a failure owns an empty list.
+/// WHY a filter produced nothing, carried home so somebody can be told.
+///
+/// Until this existed the runner read the command's stderr into memory and
+/// then FREED IT UNREAD — the one artifact that explains a failure, discarded
+/// two lines after it arrived — and every caller answered a failed filter with
+/// a bare `return`. `| trr a-z A-Z` (a typo), `| grep nomatch` (exit 1),
+/// `| jq .` on bad JSON: all of them did nothing, said nothing, and left the
+/// text alone with no way to find out why.
+pub const Failure = struct {
+    pub const Kind = enum {
+        /// The command never started: no `/bin/sh`, a cwd that is gone, a NUL
+        /// in the command, a fork that failed.
+        spawn,
+        /// Still running at `command_timeout_seconds`.
+        timeout,
+        /// Past `max_stdout_bytes` / `max_stderr_bytes` / the job total.
+        too_large,
+        /// Ran, and exited nonzero. `code` says which.
+        exit,
+        /// Killed by a signal.
+        signal,
+        /// A read, a write or an allocation failed under us.
+        io,
+    };
+
+    kind: Kind = .io,
+    /// Which selection this was, so a report over several cursors can say.
+    index: u32 = 0,
+    /// The exit status, when `kind` is `.exit`.
+    code: u8 = 0,
+    /// stderr exactly as the command wrote it, OWNED by the response. Empty
+    /// when the command said nothing, which is why `kind` and `code` exist.
+    stderr: []u8 = &.{},
+};
+
+/// One worker answer. `outputs` owns each slice; a failure owns an empty list
+/// and, usually, the command's own account of itself in `failure`.
 pub const Response = struct {
     id: u32,
     success: bool,
     outputs: [][]u8,
+    failure: ?Failure = null,
 
     pub fn deinit(response: *Response, gpa: std.mem.Allocator) void {
         for (response.outputs) |output| gpa.free(output);
         if (response.outputs.len > 0) gpa.free(response.outputs);
+        if (response.failure) |f| if (f.stderr.len > 0) gpa.free(f.stderr);
         response.* = undefined;
     }
 };
 
+/// What one invocation came to: the bytes, or the reason there are none.
+pub const Outcome = union(enum) { ok: []u8, failed: Failure };
+
 /// The in-flight set a host keeps while pipes run off its loop.
 ///
 /// tty.zig and gui.zig each had this verbatim — same `finish` walk, same
@@ -151,9 +192,14 @@ pub fn runOne(
     command: []const u8,
     cwd: []const u8,
     input: []const u8,
-) ?[]u8 {
+) Outcome {
+    const fail = struct {
+        fn k(kind: Failure.Kind) Outcome {
+            return .{ .failed = .{ .kind = kind } };
+        }
+    };
     if (std.mem.indexOfScalar(u8, command, 0) != null or
-        std.mem.indexOfScalar(u8, cwd, 0) != null) return null;
+        std.mem.indexOfScalar(u8, cwd, 0) != null) return fail.k(.spawn);
 
     var child = std.process.spawn(io, .{
         .argv = &.{ "/bin/sh", "-c", command },
@@ -161,7 +207,7 @@ pub fn runOne(
         .stdin = .pipe,
         .stdout = .pipe,
         .stderr = .pipe,
-    }) catch return null;
+    }) catch return fail.k(.spawn);
 
     var writer_context: WriterContext = .{
         .io = io,
@@ -172,7 +218,7 @@ pub fn runOne(
     var writer: ?std.Thread = std.Thread.spawn(.{}, writeInput, .{&writer_context}) catch {
         writer_context.file.close(io);
         child.kill(io);
-        return null;
+        return fail.k(.spawn);
     };
     // On every early return kill first, unblocking a command which never read
     // stdin, then join the short-lived writer before its borrowed input dies.
@@ -192,33 +238,49 @@ pub fn runOne(
     } }).toDeadline(io);
     while (multi_reader.fill(64, deadline)) |_| {
         if (stdout_reader.buffered().len > max_stdout_bytes or
-            stderr_reader.buffered().len > max_stderr_bytes) return null;
+            stderr_reader.buffered().len > max_stderr_bytes) return fail.k(.too_large);
     } else |err| switch (err) {
         error.EndOfStream => {},
-        else => return null,
+        // The deadline is the only one of these a human is likely to cause,
+        // and it is the one they are least able to guess at: ten seconds of
+        // nothing used to be followed by nothing.
+        error.Timeout => return fail.k(.timeout),
+        else => return fail.k(.io),
     }
     if (stdout_reader.buffered().len > max_stdout_bytes or
-        stderr_reader.buffered().len > max_stderr_bytes) return null;
-    multi_reader.checkAnyError() catch return null;
-    const term = child.wait(io) catch return null;
+        stderr_reader.buffered().len > max_stderr_bytes) return fail.k(.too_large);
+    multi_reader.checkAnyError() catch return fail.k(.io);
+    const term = child.wait(io) catch return fail.k(.io);
     writer.?.join();
     writer = null;
 
-    const stdout = multi_reader.toOwnedSlice(0) catch return null;
+    const stdout = multi_reader.toOwnedSlice(0) catch return fail.k(.io);
     const stderr = multi_reader.toOwnedSlice(1) catch {
         gpa.free(stdout);
-        return null;
-    };
-    defer gpa.free(stderr);
-    const exited_zero = switch (term) {
-        .exited => |code| code == 0,
-        else => false,
+        return fail.k(.io);
     };
-    if (!writer_context.ok or !exited_zero) {
-        gpa.free(stdout);
-        return null;
+    // stderr is NOT freed here any more. It is the command's own account of
+    // what went wrong, and it goes home with the failure.
+    errdefer gpa.free(stderr);
+
+    // A STDIN WRITE THAT ENDED EARLY IS NOT A FAILURE. `writer_context.ok` was
+    // part of this condition, so `| head -1` over a selection bigger than the
+    // pipe buffer failed — the command closed stdin after the line it wanted,
+    // the write got EPIPE, and a filter that had done exactly its job reported
+    // nothing. helix joins its input task and ignores the result for this
+    // reason; the exit status is the whole verdict.
+    switch (term) {
+        .exited => |code| if (code != 0) {
+            gpa.free(stdout);
+            return .{ .failed = .{ .kind = .exit, .code = code, .stderr = stderr } };
+        },
+        else => {
+            gpa.free(stdout);
+            return .{ .failed = .{ .kind = .signal, .stderr = stderr } };
+        },
     }
-    return stdout;
+    gpa.free(stderr);
+    return .{ .ok = stdout };
 }
 
 /// Invoke the command independently for every selection. The response is all
@@ -230,7 +292,17 @@ pub fn runJob(gpa: std.mem.Allocator, io: std.Io, job: *const Job) Response {
     var made: usize = 0;
     var total: usize = 0;
     for (job.inputs, 0..) |input, i| {
-        const output = runOne(gpa, io, job.command, job.cwd, input) orelse break;
+        const output = switch (runOne(gpa, io, job.command, job.cwd, input)) {
+            .ok => |bytes| bytes,
+            .failed => |f| {
+                // WHICH selection, because with several cursors "it failed" is
+                // not enough to go looking with.
+                var owned = f;
+                owned.index = @intCast(i);
+                response.failure = owned;
+                break;
+            },
+        };
         if (std.math.add(usize, total, output.len) catch null) |next_total| {
             if (next_total <= max_total_stdout_bytes) {
                 outputs[i] = output;
@@ -240,6 +312,7 @@ pub fn runJob(gpa: std.mem.Allocator, io: std.Io, job: *const Job) Response {
             }
         }
         gpa.free(output);
+        response.failure = .{ .kind = .too_large, .index = @intCast(i) };
         break;
     }
     if (made != job.inputs.len) {
@@ -252,12 +325,46 @@ pub fn runJob(gpa: std.mem.Allocator, io: std.Io, job: *const Job) Response {
     return response;
 }
 
-test "native pipe runner preserves stdin/stdout bytes and rejects failure" {
+test "native pipe runner preserves stdin/stdout bytes and reports how it failed" {
     const gpa = std.testing.allocator;
     const io = std.testing.io;
-    const output = runOne(gpa, io, "printf 'prefix:'; cat; printf '\\n'", "/tmp", "a\x00b\n") orelse
-        return error.PipeCommandFailed;
+    const output = switch (runOne(gpa, io, "printf 'prefix:'; cat; printf '\\n'", "/tmp", "a\x00b\n")) {
+        .ok => |bytes| bytes,
+        .failed => return error.PipeCommandFailed,
+    };
     defer gpa.free(output);
     try std.testing.expectEqualSlices(u8, "prefix:a\x00b\n\n", output);
-    try std.testing.expect(runOne(gpa, io, "printf ignored; exit 7", "/tmp", "") == null);
+
+    // A NONZERO EXIT COMES HOME WITH ITS REASON. The status and the command's
+    // own stderr are the whole of what a human needs to fix a typo'd filter,
+    // and both used to be freed on the floor.
+    switch (runOne(gpa, io, "echo trouble >&2; exit 7", "/tmp", "")) {
+        .ok => |bytes| {
+            gpa.free(bytes);
+            return error.PipeShouldHaveFailed;
+        },
+        .failed => |f| {
+            defer gpa.free(f.stderr);
+            try std.testing.expectEqual(Failure.Kind.exit, f.kind);
+            try std.testing.expectEqual(@as(u8, 7), f.code);
+            try std.testing.expectEqualSlices(u8, "trouble\n", f.stderr);
+        },
+    }
+
+    // ...and a command that stops reading its stdin SUCCEEDS. `| head -1` over
+    // a selection bigger than the pipe buffer takes the line it wanted and
+    // closes the pipe; the write gets EPIPE, and that used to fail the filter
+    // even though it had done exactly its job.
+    const big = try gpa.alloc(u8, 512 * 1024);
+    defer gpa.free(big);
+    @memset(big, 'x');
+    big[0] = 'a';
+    big[1] = '\n';
+    switch (runOne(gpa, io, "head -1", "/tmp", big)) {
+        .ok => |bytes| {
+            defer gpa.free(bytes);
+            try std.testing.expectEqualSlices(u8, "a\n", bytes);
+        },
+        .failed => return error.EarlyStdinCloseShouldNotFail,
+    }
 }