52e5cdc1

scenario: plug capture leaks + thread CellGeom + distinct errors

a73x   2026-04-19 09:42

Critical fixes:
  - Capture branch now OOM-safe: all allocations complete before
    committing to the captures map; errdefers clean up on failure.
  - Duplicate-label capture frees the prior image + reuses old key.
  - last_capture_label UAF closed (dupe before freeing old).

Important fixes:
  - AssertCellWithoutCapture is a distinct error from AssertFailed.
  - CellGeom threads through ScenarioState.init for Plan 3 use.

Minor fix:
  - assert-cell-at bounds-check diagnostic now says the right name.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---

diff --git a/src/scenario.zig b/src/scenario.zig
index 1f6007a..e0dbfad 100644
--- a/src/scenario.zig
+++ b/src/scenario.zig
@@ -434,7 +434,7 @@ pub fn parse(
                    return error.ParseFailed;
                };
                if (row >= rows or col >= cols) {
                    diag.* = .{ .line = line_num, .message = "assert-cell row/col out of bounds for scenario size" };
                    diag.* = .{ .line = line_num, .message = "assert-cell-at row/col out of bounds for scenario size" };
                    return error.ParseFailed;
                }
                try directives.append(alloc, .{ .assert_cell_at = .{
@@ -781,6 +781,7 @@ pub const TickError = error{
    ScenarioTimeout,
    SleepUntilFlipTimeout,
    AssertFailed,
    AssertCellWithoutCapture,
    PredicateOnMissingLabel,
    CallbackFailed,
} || std.mem.Allocator.Error;
@@ -799,12 +800,14 @@ pub const ScenarioState = struct {
    sleep_until_flip_started_ns: ?i128, // set when entering a sleep-until-flip directive
    captures: std.StringHashMapUnmanaged(png.Image), // label → captured PNG
    last_capture_label: ?[]const u8, // most recently captured label (owned by alloc)
    cell_geom: CellGeom, // renderer cell dimensions for assert predicates
    alloc: std.mem.Allocator,

    pub fn init(
        alloc: std.mem.Allocator,
        scenario: *const Scenario,
        origin_ns: i128,
        cell_geom: CellGeom,
    ) ScenarioState {
        return .{
            .scenario = scenario,
@@ -815,6 +818,7 @@ pub const ScenarioState = struct {
            .sleep_until_flip_started_ns = null,
            .captures = .{},
            .last_capture_label = null,
            .cell_geom = cell_geom,
            .alloc = alloc,
        };
    }
@@ -883,23 +887,39 @@ pub const ScenarioState = struct {
                        std.log.warn("scenario: capture callback failed: {s}", .{@errorName(err)});
                        return error.CallbackFailed;
                    };
                    // Dupe the label into ScenarioState.alloc so captures outlive the arena.
                    const label_copy = try self.alloc.dupe(u8, label);
                    errdefer self.alloc.free(label_copy);
                    try self.captures.put(self.alloc, label_copy, img);
                    // Update last_capture_label: free previous, dupe new from original label.
                    if (self.last_capture_label) |old| self.alloc.free(old);
                    self.last_capture_label = try self.alloc.dupe(u8, label);
                    errdefer self.alloc.free(img.pixels);

                    // Dupe label for both the map key and last_capture_label. Do both
                    // allocs BEFORE committing to the map so any OOM unwinds cleanly.
                    const key_dup = try self.alloc.dupe(u8, label);
                    errdefer self.alloc.free(key_dup);
                    const new_last = try self.alloc.dupe(u8, label);
                    errdefer self.alloc.free(new_last);

                    // Reserve the map slot. getOrPut may OOM; errdefers above handle it.
                    const gop = try self.captures.getOrPut(self.alloc, key_dup);

                    // Past here: no more OOM-able operations — commit everything atomically.
                    if (gop.found_existing) {
                        // Free old pixel buffer and our freshly-allocated key_dup;
                        // the map retains the existing key slot.
                        self.alloc.free(gop.value_ptr.pixels);
                        self.alloc.free(key_dup);
                    }
                    gop.value_ptr.* = img;

                    if (self.last_capture_label) |old_last| self.alloc.free(old_last);
                    self.last_capture_label = new_last;

                    self.cursor += 1;
                },
                .assert_cell => |ac| {
                    const lbl = self.last_capture_label orelse {
                        std.debug.print("scenario: assert-cell with no prior capture\n", .{});
                        return error.AssertFailed;
                        return error.AssertCellWithoutCapture;
                    };
                    const img = self.captures.get(lbl).?;
                    const geom = CellGeom{ .cell_w_px = 8, .cell_h_px = 16 };
                    const result = evalPredicate(img, ac.row, ac.col, geom, ac.pred, null);
                    const result = evalPredicate(img, ac.row, ac.col, self.cell_geom, ac.pred, null);
                    if (!result.pass) {
                        std.debug.print("scenario: assert-cell ({d},{d}) {s} failed: {s}\n", .{
                            ac.row, ac.col, @tagName(ac.pred), result.reason,
@@ -912,8 +932,7 @@ pub const ScenarioState = struct {
                    const img = self.captures.get(aca.label) orelse {
                        return error.PredicateOnMissingLabel;
                    };
                    const geom = CellGeom{ .cell_w_px = 8, .cell_h_px = 16 };
                    const result = evalPredicate(img, aca.row, aca.col, geom, aca.pred, null);
                    const result = evalPredicate(img, aca.row, aca.col, self.cell_geom, aca.pred, null);
                    if (!result.pass) {
                        std.debug.print("scenario: assert-cell-at {s} ({d},{d}) {s} failed: {s}\n", .{
                            aca.label, aca.row, aca.col, @tagName(aca.pred), result.reason,
@@ -986,7 +1005,7 @@ test "tick: empty scenario is immediately done" {
        \\timeout 1000ms
    );
    defer s.deinit();
    var state = ScenarioState.init(std.testing.allocator, &s, 0);
    var state = ScenarioState.init(std.testing.allocator, &s, 0, .{ .cell_w_px = 8, .cell_h_px = 16 });
    defer state.deinit();

    try std.testing.expect(state.isDone());
@@ -999,7 +1018,7 @@ test "tick: bytes directive fires write_bytes immediately" {
        \\bytes "abc"
    );
    defer s.deinit();
    var state = ScenarioState.init(std.testing.allocator, &s, 0);
    var state = ScenarioState.init(std.testing.allocator, &s, 0, .{ .cell_w_px = 8, .cell_h_px = 16 });
    defer state.deinit();

    var tio = TestIO{ .alloc = std.testing.allocator };
@@ -1019,7 +1038,7 @@ test "tick: sleep holds advancement until time passes" {
        \\bytes "x"
    );
    defer s.deinit();
    var state = ScenarioState.init(std.testing.allocator, &s, 0);
    var state = ScenarioState.init(std.testing.allocator, &s, 0, .{ .cell_w_px = 8, .cell_h_px = 16 });
    defer state.deinit();

    var tio = TestIO{ .alloc = std.testing.allocator };
@@ -1048,7 +1067,7 @@ test "tick: capture calls io.capture and stores the result under the label" {
        \\capture snap1
    );
    defer s.deinit();
    var state = ScenarioState.init(std.testing.allocator, &s, 0);
    var state = ScenarioState.init(std.testing.allocator, &s, 0, .{ .cell_w_px = 8, .cell_h_px = 16 });
    defer state.deinit();

    var tio = TestIO{ .alloc = std.testing.allocator };
@@ -1069,7 +1088,7 @@ test "tick: scenario timeout fires" {
        \\bytes "x"
    );
    defer s.deinit();
    var state = ScenarioState.init(std.testing.allocator, &s, 0);
    var state = ScenarioState.init(std.testing.allocator, &s, 0, .{ .cell_w_px = 8, .cell_h_px = 16 });
    defer state.deinit();

    var tio = TestIO{ .alloc = std.testing.allocator };
@@ -1088,7 +1107,7 @@ test "tick: sleep-until-flip holds until flip callback returns true" {
        \\bytes "x"
    );
    defer s.deinit();
    var state = ScenarioState.init(std.testing.allocator, &s, 0);
    var state = ScenarioState.init(std.testing.allocator, &s, 0, .{ .cell_w_px = 8, .cell_h_px = 16 });
    defer state.deinit();

    var tio = TestIO{ .alloc = std.testing.allocator };
@@ -1114,7 +1133,7 @@ test "tick: sleep-until-flip times out after 2x blink_period_ns of real wait" {
        \\sleep-until-flip
    );
    defer s.deinit();
    var state = ScenarioState.init(std.testing.allocator, &s, 0);
    var state = ScenarioState.init(std.testing.allocator, &s, 0, .{ .cell_w_px = 8, .cell_h_px = 16 });
    defer state.deinit();

    var tio = TestIO{ .alloc = std.testing.allocator };
@@ -1152,7 +1171,7 @@ test "tick: write_bytes callback failure surfaces as CallbackFailed" {
        \\bytes "x"
    );
    defer s.deinit();
    var state = ScenarioState.init(std.testing.allocator, &s, 0);
    var state = ScenarioState.init(std.testing.allocator, &s, 0, .{ .cell_w_px = 8, .cell_h_px = 16 });
    defer state.deinit();

    var ctx: u8 = 0;
@@ -1534,7 +1553,7 @@ test "tick+eval: assert-cell evaluates against last capture" {
        \\assert-cell 0 0 cursor-block-at
    );
    defer s.deinit();
    var state = ScenarioState.init(std.testing.allocator, &s, 0);
    var state = ScenarioState.init(std.testing.allocator, &s, 0, .{ .cell_w_px = 8, .cell_h_px = 16 });
    defer state.deinit();

    var tio = TestIO{ .alloc = std.testing.allocator };
@@ -1553,7 +1572,7 @@ test "tick+eval: assert-cell-at on missing label errors" {
        \\assert-cell-at nope 0 0 cell-empty
    );
    defer s.deinit();
    var state = ScenarioState.init(std.testing.allocator, &s, 0);
    var state = ScenarioState.init(std.testing.allocator, &s, 0, .{ .cell_w_px = 8, .cell_h_px = 16 });
    defer state.deinit();

    var tio = TestIO{ .alloc = std.testing.allocator };
@@ -1562,3 +1581,39 @@ test "tick+eval: assert-cell-at on missing label errors" {
    const r = state.tick(0, tio.io());
    try std.testing.expectError(error.PredicateOnMissingLabel, r);
}

test "tick: duplicate capture label frees the prior image" {
    var s = try parseOk(
        \\size 80 24
        \\timeout 5000ms
        \\capture snap
        \\capture snap
    );
    defer s.deinit();
    var state = ScenarioState.init(std.testing.allocator, &s, 0, .{ .cell_w_px = 8, .cell_h_px = 16 });
    defer state.deinit();

    var tio = TestIO{ .alloc = std.testing.allocator };
    defer tio.deinit();

    _ = try state.tick(0, tio.io());
    try std.testing.expect(state.isDone());
    // Testing allocator will catch any leak here on deinit.
}

test "tick: assert-cell with no prior capture errors distinctly" {
    var s = try parseOk(
        \\size 80 24
        \\timeout 5000ms
        \\assert-cell 0 0 cell-empty
    );
    defer s.deinit();
    var state = ScenarioState.init(std.testing.allocator, &s, 0, .{ .cell_w_px = 8, .cell_h_px = 16 });
    defer state.deinit();

    var tio = TestIO{ .alloc = std.testing.allocator };
    defer tio.deinit();

    const r = state.tick(0, tio.io());
    try std.testing.expectError(error.AssertCellWithoutCapture, r);
}