From 92733763d8330b571e014f7fe66b8e8404b2ebca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A0=20Arrufat?= Date: Tue, 19 May 2026 11:13:15 +0200 Subject: [PATCH] refactor: use explicit type names and encapsulate spinner state - Replace `Self` with `Recorder` and `Verifier` for improved clarity. - Add `Spinner.isEnabled()` to encapsulate atomic state access. - Shorten and refine various comments across the codebase. --- src/agent/Agent.zig | 8 +++----- src/agent/Spinner.zig | 23 +++++++++++++---------- src/agent/Terminal.zig | 2 +- src/script/Recorder.zig | 36 +++++++++++++++++------------------- src/script/Verifier.zig | 24 ++++++++++-------------- 5 files changed, 44 insertions(+), 49 deletions(-) diff --git a/src/agent/Agent.zig b/src/agent/Agent.zig index 9f8d52b1..a089a2b7 100644 --- a/src/agent/Agent.zig +++ b/src/agent/Agent.zig @@ -1194,7 +1194,7 @@ fn handleToolCall(ctx: *anyopaque, allocator: std.mem.Allocator, tool_name: []co const self: *Agent = @ptrCast(@alignCast(ctx)); // The spinner doesn't render args, and `agentToolDone` skips the body // line at low verbosity — don't pay for the stringify when nobody reads it. - const needs_args = self.terminal.spinner.enabled.load(.monotonic) or self.terminal.verbosity != .low; + const needs_args = self.terminal.spinner.isEnabled() or self.terminal.verbosity != .low; const args_str: []const u8 = if (needs_args) (if (arguments) |v| std.json.Stringify.valueAlloc(allocator, v, .{}) catch "" else @@ -1374,10 +1374,8 @@ fn pickModel( var arena: std.heap.ArenaAllocator = .init(allocator); defer arena.deinit(); - // Known limitation: this runs before `SigBridge.attach`, so a Ctrl-C - // pressed during the model listing is dropped silently. The HTTP call - // is synchronous and uninterruptible; press Ctrl-C again at the picker - // prompt (which surfaces it as `error.UserCancelled` via stdin EINTR). + // Runs before `SigBridge.attach` — Ctrl-C during the synchronous HTTP + // fetch is dropped; the picker prompt catches the next press via stdin EINTR. std.debug.print("Fetching models for {s}…\n", .{@tagName(provider)}); const ids = zenai.provider.listChatModelIds(allocator, arena.allocator(), provider, api_key, base_url) catch |err| { log.fatal(.app, "list models failed", .{ .err = @errorName(err) }); diff --git a/src/agent/Spinner.zig b/src/agent/Spinner.zig index 7f0992db..f24475d8 100644 --- a/src/agent/Spinner.zig +++ b/src/agent/Spinner.zig @@ -57,9 +57,8 @@ const State = union(enum) { tool: ToolState, }; -/// Atomic so the agent-thread fast-paths (`if (!self.enabled.load(...))`) and -/// the under-lock write in `ensureWorkerLocked` don't race if a future caller -/// touches the spinner from a non-agent thread. +/// Atomic so the unlocked fast-path reads (`isEnabled`) don't race the +/// under-lock write in `ensureWorkerLocked`. enabled: std.atomic.Value(bool), mu: std.Thread.Mutex = .{}, @@ -80,6 +79,10 @@ pub fn init(is_repl: bool, stderr_is_tty: bool) Spinner { return .{ .enabled = .init(is_repl and stderr_is_tty) }; } +pub inline fn isEnabled(self: *const Spinner) bool { + return self.enabled.load(.monotonic); +} + pub fn deinit(self: *Spinner) void { if (self.thread) |t| { self.mu.lock(); @@ -93,7 +96,7 @@ pub fn deinit(self: *Spinner) void { /// Begin a new agent turn. Spawns the worker thread on first call. pub fn start(self: *Spinner) void { - if (!self.enabled.load(.monotonic)) return; + if (!self.isEnabled()) return; self.mu.lock(); defer self.mu.unlock(); self.state = .thinking; @@ -119,7 +122,7 @@ fn ensureWorkerLocked(self: *Spinner) void { /// End an agent turn cleanly: clear the indicator, commit a one-line summary, /// reset state. Called from a `defer` in the agent code so it always runs. pub fn stop(self: *Spinner) void { - if (!self.enabled.load(.monotonic)) return; + if (!self.isEnabled()) return; self.mu.lock(); defer self.mu.unlock(); if (self.state == .idle) return; @@ -141,7 +144,7 @@ pub fn stop(self: *Spinner) void { /// End a turn with no commit. The caller is responsible for surfacing the /// outcome — tool results, error messages, or summaries. pub fn cancel(self: *Spinner) void { - if (!self.enabled.load(.monotonic)) return; + if (!self.isEnabled()) return; self.mu.lock(); defer self.mu.unlock(); if (self.state == .idle) return; @@ -155,7 +158,7 @@ pub fn cancel(self: *Spinner) void { /// without a preceding `start()` (state `.idle`) the label drops the `agent:` /// prefix — that path is for user-typed REPL commands, not LLM tool calls. pub fn setTool(self: *Spinner, name: []const u8, args: []const u8) void { - if (!self.enabled.load(.monotonic)) return; + if (!self.isEnabled()) return; self.mu.lock(); defer self.mu.unlock(); const manual = self.state == .idle; @@ -178,7 +181,7 @@ pub fn setTool(self: *Spinner, name: []const u8, args: []const u8) void { /// for the rest of the dwell window (`min_tool_display_ns`), then the /// indicator returns to thinking like any other call. pub fn markToolFailed(self: *Spinner) void { - if (!self.enabled.load(.monotonic)) return; + if (!self.isEnabled()) return; self.mu.lock(); defer self.mu.unlock(); switch (self.state) { @@ -194,7 +197,7 @@ pub fn markToolFailed(self: *Spinner) void { /// honors `min_tool_display_ns` — if the current tool label has not been /// up long enough, the flip is deferred until it has. pub fn setThinking(self: *Spinner) void { - if (!self.enabled.load(.monotonic)) return; + if (!self.isEnabled()) return; self.mu.lock(); defer self.mu.unlock(); switch (self.state) { @@ -208,7 +211,7 @@ pub fn setThinking(self: *Spinner) void { /// Print `text` (assumed to include its own newline) above the indicator, /// then leave the indicator to repaint itself on the next tick. pub fn emitAbove(self: *Spinner, text: []const u8) bool { - if (!self.enabled.load(.monotonic)) return false; + if (!self.isEnabled()) return false; self.mu.lock(); defer self.mu.unlock(); if (self.state == .idle) return false; diff --git a/src/agent/Terminal.zig b/src/agent/Terminal.zig index 887c4021..35f4f55f 100644 --- a/src/agent/Terminal.zig +++ b/src/agent/Terminal.zig @@ -151,7 +151,7 @@ pub fn endTool(self: *Terminal, ok: bool) void { /// gated on `medium`+. In non-TTY contexts ANSI is still emitted — /// pipes that strip color see plain text via the bullet character. pub fn agentToolDone(self: *Terminal, name: []const u8, args: []const u8, ok: bool) void { - const spinner_on = self.spinner.enabled.load(.monotonic); + const spinner_on = self.spinner.isEnabled(); if (spinner_on and !ok) self.spinner.markToolFailed(); if (!atLeast(self.verbosity, .medium)) return; diff --git a/src/script/Recorder.zig b/src/script/Recorder.zig index 97605278..4aff75b2 100644 --- a/src/script/Recorder.zig +++ b/src/script/Recorder.zig @@ -22,7 +22,7 @@ const log = lp.log; const testing = @import("../testing.zig"); const Command = @import("Command.zig"); -const Self = @This(); +const Recorder = @This(); allocator: std.mem.Allocator, /// Open append-mode handle while recording is active. Becomes null when a @@ -38,7 +38,7 @@ lines: u32, buf: std.Io.Writer.Allocating, /// Append-open `path`, inserting a leading newline if the file is non-empty. -pub fn init(allocator: std.mem.Allocator, path: []const u8) !Self { +pub fn init(allocator: std.mem.Allocator, path: []const u8) !Recorder { const owned_path = try allocator.dupe(u8, path); errdefer allocator.free(owned_path); const file = try openForAppend(path); @@ -60,23 +60,23 @@ fn openForAppend(path: []const u8) !std.fs.File { return f; } -pub fn deinit(self: *Self) void { +pub fn deinit(self: *Recorder) void { self.buf.deinit(); if (self.file) |f| f.close(); self.allocator.free(self.path); } -pub fn isActive(self: *const Self) bool { +pub fn isActive(self: *const Recorder) bool { return self.file != null; } -pub fn record(self: *Self, cmd: Command.Command) void { +pub fn record(self: *Recorder, cmd: Command.Command) void { if (self.file == null) return; if (!cmd.isRecorded()) return; self.tryRecord(cmd) catch |err| self.disable(err); } -fn tryRecord(self: *Self, cmd: Command.Command) !void { +fn tryRecord(self: *Recorder, cmd: Command.Command) !void { self.buf.clearRetainingCapacity(); try cmd.format(&self.buf.writer); try self.buf.writer.writeByte('\n'); @@ -92,12 +92,12 @@ fn tryRecord(self: *Self, cmd: Command.Command) !void { self.lines += 1; } -pub fn recordComment(self: *Self, comment: []const u8) void { +pub fn recordComment(self: *Recorder, comment: []const u8) void { if (self.file == null) return; self.tryRecordComment(comment) catch |err| self.disable(err); } -fn tryRecordComment(self: *Self, comment: []const u8) !void { +fn tryRecordComment(self: *Recorder, comment: []const u8) !void { self.buf.clearRetainingCapacity(); // Embedded newlines would smuggle an executable line into the script on // replay (e.g. `# foo\nGOTO https://attacker`). Emit each line of the @@ -115,10 +115,8 @@ fn tryRecordComment(self: *Self, comment: []const u8) !void { /// Any failure along the record path — buffer-write OOM, scrub OOM, or file /// write — flips the recorder to inactive so subsequent calls become silent -/// no-ops and `isActive()` reflects the stopped state. Previously a buffer -/// failure would `catch return` while leaving the file open and `isActive()` -/// true, silently dropping the line. -fn disable(self: *Self, err: anyerror) void { +/// no-ops and `isActive()` reflects the stopped state. +fn disable(self: *Recorder, err: anyerror) void { log.warn(.app, "recording disabled", .{ .err = @errorName(err) }); if (self.file) |f| { f.close(); @@ -132,7 +130,7 @@ test "record writes state-mutating commands" { const file = tmp.dir.createFile("test.lp", .{ .read = true }) catch unreachable; - var recorder: Self = .{ + var recorder: Recorder = .{ .allocator = std.testing.allocator, .file = file, .path = try std.testing.allocator.dupe(u8, "test.lp"), @@ -180,7 +178,7 @@ test "record skips empty and comment lines" { const file = tmp.dir.createFile("test2.lp", .{ .read = true }) catch unreachable; - var recorder: Self = .{ + var recorder: Recorder = .{ .allocator = std.testing.allocator, .file = file, .path = try std.testing.allocator.dupe(u8, "test.lp"), @@ -208,7 +206,7 @@ test "lines counter tracks successful appends" { const file = tmp.dir.createFile("count.lp", .{ .read = true }) catch unreachable; - var recorder: Self = .{ + var recorder: Recorder = .{ .allocator = std.testing.allocator, .file = file, .path = try std.testing.allocator.dupe(u8, "test.lp"), @@ -240,7 +238,7 @@ test "init appends to an existing file without truncating" { var path_buf: [std.fs.max_path_bytes]u8 = undefined; const abs_path = tmp.dir.realpath("script.lp", &path_buf) catch unreachable; - var recorder: Self = try .init(std.testing.allocator, abs_path); + var recorder: Recorder = try .init(std.testing.allocator, abs_path); defer recorder.deinit(); recorder.record(Command.parse("CLICK 'Login'")); @@ -267,7 +265,7 @@ test "recordComment splits embedded newlines into separate comment lines" { defer tmp.cleanup(); const file = tmp.dir.createFile("multi.lp", .{ .read = true }) catch unreachable; - var recorder: Self = .{ + var recorder: Recorder = .{ .allocator = std.testing.allocator, .file = file, .path = try std.testing.allocator.dupe(u8, "test.lp"), @@ -302,7 +300,7 @@ test "record disables recorder on write failure" { break :blk tmp.dir.openFile("ro.lp", .{ .mode = .read_only }) catch unreachable; }; - var recorder: Self = .{ + var recorder: Recorder = .{ .allocator = std.testing.allocator, .file = file, .path = try std.testing.allocator.dupe(u8, "test.lp"), @@ -331,7 +329,7 @@ test "init creates the file if missing" { var full_buf: [std.fs.max_path_bytes]u8 = undefined; const abs_path = std.fmt.bufPrint(&full_buf, "{s}/fresh.lp", .{dir_path}) catch unreachable; - var recorder: Self = try .init(std.testing.allocator, abs_path); + var recorder: Recorder = try .init(std.testing.allocator, abs_path); defer recorder.deinit(); recorder.record(Command.parse("GOTO https://example.com")); diff --git a/src/script/Verifier.zig b/src/script/Verifier.zig index 8601c880..4253bdd9 100644 --- a/src/script/Verifier.zig +++ b/src/script/Verifier.zig @@ -22,7 +22,7 @@ const browser_tools = lp.tools; const Command = @import("Command.zig"); const CDPNode = @import("../cdp/Node.zig"); -const Self = @This(); +const Verifier = @This(); session: *lp.Session, node_registry: *CDPNode.Registry, @@ -33,9 +33,8 @@ pub const VerifyResult = union(enum) { inconclusive, }; -/// Closed set of element properties the verifier can probe. Keeps the JS -/// template injection-free (no caller-supplied expression text) and -/// guarantees each variant produces a valid `el`-rooted JS expression. +/// Closed set of element properties the verifier can probe — keeps the JS +/// template injection-free (no caller-supplied expression text). const ElementProperty = enum { value, checked_string, @@ -48,17 +47,14 @@ const ElementProperty = enum { } }; -/// Static fallback used when allocPrint OOMs while formatting the reason — -/// keeps `VerifyResult.failed` non-optional so callers don't have to -/// distinguish "no reason" from "OOM" (which they uniformly couldn't -/// recover from anyway). +/// Fallback when allocPrint OOMs — lets `VerifyResult.failed` stay non-optional. const failed_reason_oom = "verification failed (out of memory while formatting reason)"; /// Verify that a command achieved its intent after execution. Only called /// when the command did not hard-fail (ExecResult.failed == false). /// Commands without a dedicated verifier return `.inconclusive` so callers /// can distinguish "no verification available" from "explicitly verified". -pub fn verify(self: *Self, arena: std.mem.Allocator, cmd: Command.Command) VerifyResult { +pub fn verify(self: *Verifier, arena: std.mem.Allocator, cmd: Command.Command) VerifyResult { return switch (cmd) { .type_cmd => |args| self.verifyFill(arena, args.selector, args.value), .check => |args| self.verifyCheck(arena, args.selector, args.checked), @@ -67,7 +63,7 @@ pub fn verify(self: *Self, arena: std.mem.Allocator, cmd: Command.Command) Verif }; } -fn verifyFill(self: *Self, arena: std.mem.Allocator, selector: []const u8, expected_value: []const u8) VerifyResult { +fn verifyFill(self: *Verifier, arena: std.mem.Allocator, selector: []const u8, expected_value: []const u8) VerifyResult { // Secret env-var references can't be compared literally — just // verify the field isn't empty after substitution. if (std.mem.indexOf(u8, expected_value, "$LP_") != null) { @@ -79,12 +75,12 @@ fn verifyFill(self: *Self, arena: std.mem.Allocator, selector: []const u8, expec return self.verifyElementValue(arena, selector, .{ .property = .value, .expected = expected_value, .label = "value" }); } -fn verifyCheck(self: *Self, arena: std.mem.Allocator, selector: []const u8, expected: bool) VerifyResult { +fn verifyCheck(self: *Verifier, arena: std.mem.Allocator, selector: []const u8, expected: bool) VerifyResult { const expected_str: []const u8 = if (expected) "true" else "false"; return self.verifyElementValue(arena, selector, .{ .property = .checked_string, .expected = expected_str, .label = "checked state" }); } -fn verifySelect(self: *Self, arena: std.mem.Allocator, selector: []const u8, expected_value: []const u8) VerifyResult { +fn verifySelect(self: *Verifier, arena: std.mem.Allocator, selector: []const u8, expected_value: []const u8) VerifyResult { return self.verifyElementValue(arena, selector, .{ .property = .value, .expected = expected_value, .label = "selected value" }); } @@ -94,7 +90,7 @@ const Check = struct { label: []const u8, }; -fn verifyElementValue(self: *Self, arena: std.mem.Allocator, selector: []const u8, check: Check) VerifyResult { +fn verifyElementValue(self: *Verifier, arena: std.mem.Allocator, selector: []const u8, check: Check) VerifyResult { const actual = self.queryElementProperty(arena, selector, check.property) orelse return .inconclusive; if (!std.mem.eql(u8, actual, check.expected)) { const msg = std.fmt.allocPrint(arena, "element {s} is \"{s}\" (expected \"{s}\")", .{ check.label, actual, check.expected }) catch failed_reason_oom; @@ -103,7 +99,7 @@ fn verifyElementValue(self: *Self, arena: std.mem.Allocator, selector: []const u return .passed; } -fn queryElementProperty(self: *Self, arena: std.mem.Allocator, selector: []const u8, property: ElementProperty) ?[]const u8 { +fn queryElementProperty(self: *Verifier, arena: std.mem.Allocator, selector: []const u8, property: ElementProperty) ?[]const u8 { const selector_json = std.json.Stringify.valueAlloc(arena, selector, .{}) catch return null; const script = std.fmt.allocPrint( arena,