From 1309407e01c85bf2e270278d3a70d66c2f2e46bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A0=20Arrufat?= Date: Fri, 22 May 2026 16:19:46 +0200 Subject: [PATCH] script: unify heal formatting and move TTY helpers - Merge `formatHealReplacement` and `formatHealReplacementLines` using a new `HealBody` union. - Move `interactiveTty` and `promptNumberedChoice` to `Terminal.zig`. - Relocate and consolidate tests for `canHeal` and `isRecorded`. - Make internal `Schema` functions private. --- src/agent/Agent.zig | 76 ++---------------------------- src/agent/Terminal.zig | 40 ++++++++++++++++ src/mcp/tools.zig | 2 +- src/script.zig | 103 ++++++++++++++++------------------------- src/script/Schema.zig | 8 ++-- src/script/command.zig | 95 ++++++++++++++++++------------------- 6 files changed, 135 insertions(+), 189 deletions(-) diff --git a/src/agent/Agent.zig b/src/agent/Agent.zig index c5317cf1..991334cd 100644 --- a/src/agent/Agent.zig +++ b/src/agent/Agent.zig @@ -730,7 +730,7 @@ fn runActionEntry(self: *Agent, sa: std.mem.Allocator, entry: script.Iterator.En // opener alone is useless to the LLM — feed it the full block body. const failed_text = std.mem.trimRight(u8, entry.raw_span, &std.ascii.whitespace); if (self.attemptSelfHeal(sa, failed_text, reason, last_comment)) |healed_cmds| { - const replacement = script.formatHealReplacement(sa, entry.raw_span, entry.opener_line, healed_cmds) catch |err| { + const replacement = script.formatHealReplacement(sa, entry.raw_span, entry.opener_line, .{ .cmds = healed_cmds }) catch |err| { self.terminal.printErrorFmt( "line {d}: failed to record heal: {s} (script left unchanged)", .{ entry.line_num, @errorName(err) }, @@ -1258,7 +1258,7 @@ fn defaultModel(p: Config.AiProvider) []const u8 { } fn pickProvider(found: []const Credentials) !Credentials { - if (!interactiveTty()) { + if (!Terminal.interactiveTty()) { log.fatal(.app, "multiple API keys detected", .{ .hint = "Pass --provider explicitly when running non-interactively", }); @@ -1268,7 +1268,7 @@ fn pickProvider(found: []const Credentials) !Credentials { var labels: [@typeInfo(Config.AiProvider).@"enum".fields.len][]const u8 = undefined; for (found, 0..) |f, i| labels[i] = @tagName(f.provider); - const idx = promptNumberedChoice("Multiple API keys detected. Pick provider:", labels[0..found.len], null) catch { + const idx = Terminal.promptNumberedChoice("Multiple API keys detected. Pick provider:", labels[0..found.len], null) catch { std.debug.print("Cancelled — pass --provider to skip the picker.\n", .{}); return error.UserCancelled; }; @@ -1280,7 +1280,7 @@ fn pickProvider(found: []const Credentials) !Credentials { /// heap buffer (including for the default case) so the caller has one /// uniform free path. fn pickModel(allocator: std.mem.Allocator, llm: Credentials, base_url: ?[:0]const u8) ![]u8 { - if (!interactiveTty()) { + if (!Terminal.interactiveTty()) { log.fatal(.app, "pick-model needs a TTY", .{ .hint = "rerun in a terminal or pass --model explicitly", }); @@ -1314,75 +1314,9 @@ fn pickModel(allocator: std.mem.Allocator, llm: Credentials, base_url: ?[:0]cons const header = std.fmt.bufPrint(&header_buf, "Pick model for {s}{s}:", .{ @tagName(llm.provider), enter_hint }) catch "Pick model:"; - const idx = promptNumberedChoice(header, ids, default_idx) catch { + const idx = Terminal.promptNumberedChoice(header, ids, default_idx) catch { std.debug.print("Cancelled — pass --model to skip the picker.\n", .{}); return error.UserCancelled; }; return try allocator.dupe(u8, ids[idx]); } - -fn interactiveTty() bool { - return std.posix.isatty(std.posix.STDIN_FILENO) and std.posix.isatty(std.posix.STDERR_FILENO); -} - -/// Numbered TTY picker. `default` (if set) marks that row "(default)" and -/// makes Enter return that index. Errors with NoChoice after 3 invalid -/// attempts. -fn promptNumberedChoice(header: []const u8, items: []const []const u8, default: ?usize) !usize { - var stdin_buf: [128]u8 = undefined; - var stdin = std.fs.File.stdin().reader(&stdin_buf); - - var attempt: u8 = 0; - while (attempt < 3) : (attempt += 1) { - std.debug.print("{s}\n", .{header}); - for (items, 0..) |item, idx| { - const marker: []const u8 = if (default) |d| (if (d == idx) " (default)" else "") else ""; - std.debug.print(" {d:>3}) {s}{s}\n", .{ idx + 1, item, marker }); - } - std.debug.print("> ", .{}); - - const line = stdin.interface.takeDelimiterInclusive('\n') catch |err| switch (err) { - error.EndOfStream, error.StreamTooLong, error.ReadFailed => return error.UserCancelled, - }; - const trimmed = std.mem.trim(u8, line, " \t\r\n"); - if (trimmed.len == 0) { - if (default) |d| return d; - std.debug.print("Invalid input — type a number.\n", .{}); - continue; - } - const choice = std.fmt.parseInt(usize, trimmed, 10) catch { - const hint: []const u8 = if (default != null) " (or press Enter for default)" else ""; - std.debug.print("Invalid input — type a number{s}.\n", .{hint}); - continue; - }; - if (choice >= 1 and choice <= items.len) return choice - 1; - std.debug.print("Out of range.\n", .{}); - } - return error.NoChoice; -} - -// --- Tests --- - -test "canHeal: only page-local DOM commands are allowed" { - // Table-driven over the live tool flags so adding a new tool can't - // silently drift from the heal allow-list. - const allow = [_]BrowserTool{ .click, .hover, .waitForSelector, .fill, .selectOption, .setChecked, .scroll, .extract, .press }; - const deny = [_]BrowserTool{ .goto, .eval, .tree, .markdown, .search, .links }; - - for (allow) |action| { - const cmd = Command.fromToolCall(action, null); - try std.testing.expect(cmd.canHeal()); - } - for (deny) |action| { - const cmd = Command.fromToolCall(action, null); - try std.testing.expect(!cmd.canHeal()); - } - - try std.testing.expect(!(Command{ .login = {} }).canHeal()); - try std.testing.expect(!(Command{ .acceptCookies = {} }).canHeal()); - try std.testing.expect(!(Command{ .comment = {} }).canHeal()); -} - -test { - _ = @import("SlashCommand.zig"); -} diff --git a/src/agent/Terminal.zig b/src/agent/Terminal.zig index f5e2f5a9..706c94ca 100644 --- a/src/agent/Terminal.zig +++ b/src/agent/Terminal.zig @@ -610,6 +610,46 @@ pub fn freeLine(line: []const u8) void { c.ic_free(@ptrCast(@constCast(line.ptr))); } +pub fn interactiveTty() bool { + return std.posix.isatty(std.posix.STDIN_FILENO) and std.posix.isatty(std.posix.STDERR_FILENO); +} + +/// Numbered TTY picker. `default` (if set) marks that row "(default)" and +/// makes Enter return that index. Errors with NoChoice after 3 invalid +/// attempts. +pub fn promptNumberedChoice(header: []const u8, items: []const []const u8, default: ?usize) !usize { + var stdin_buf: [128]u8 = undefined; + var stdin = std.fs.File.stdin().reader(&stdin_buf); + + var attempt: u8 = 0; + while (attempt < 3) : (attempt += 1) { + std.debug.print("{s}\n", .{header}); + for (items, 0..) |item, idx| { + const marker: []const u8 = if (default) |d| (if (d == idx) " (default)" else "") else ""; + std.debug.print(" {d:>3}) {s}{s}\n", .{ idx + 1, item, marker }); + } + std.debug.print("> ", .{}); + + const line = stdin.interface.takeDelimiterInclusive('\n') catch |err| switch (err) { + error.EndOfStream, error.StreamTooLong, error.ReadFailed => return error.UserCancelled, + }; + const trimmed = std.mem.trim(u8, line, " \t\r\n"); + if (trimmed.len == 0) { + if (default) |d| return d; + std.debug.print("Invalid input — type a number.\n", .{}); + continue; + } + const choice = std.fmt.parseInt(usize, trimmed, 10) catch { + const hint: []const u8 = if (default != null) " (or press Enter for default)" else ""; + std.debug.print("Invalid input — type a number{s}.\n", .{hint}); + continue; + }; + if (choice >= 1 and choice <= items.len) return choice - 1; + std.debug.print("Out of range.\n", .{}); + } + return error.NoChoice; +} + pub fn printAssistant(_: *Terminal, text: []const u8) void { const fd = std.posix.STDOUT_FILENO; _ = std.posix.write(fd, text) catch {}; diff --git a/src/mcp/tools.zig b/src/mcp/tools.zig index 22bca84b..a0c249db 100644 --- a/src/mcp/tools.zig +++ b/src/mcp/tools.zig @@ -335,7 +335,7 @@ fn handleScriptHeal(server: *Server, arena: std.mem.Allocator, id: std.json.Valu return sendErrorContent(server, id, msg); } - splices[i] = script.formatHealReplacementLines(arena, entry.span, spec.original_line, spec.replacement_lines) catch |err| + splices[i] = script.formatHealReplacement(arena, entry.span, spec.original_line, .{ .lines = spec.replacement_lines }) catch |err| return sendErrorContent(server, id, @errorName(err)); } diff --git a/src/script.zig b/src/script.zig index b5c93797..79ab9693 100644 --- a/src/script.zig +++ b/src/script.zig @@ -186,47 +186,36 @@ pub fn writeAtomic( try af.finish(); } +/// Replacement body: either parsed Commands (agent self-heal) or pre-rendered +/// lines (MCP `script_heal`, where the LLM driver supplies raw PandaScript). +pub const HealBody = union(enum) { + cmds: []const Command, + lines: []const []const u8, +}; + /// Build the standard `# [Auto-healed] Original: ` header followed by -/// the serialized replacement commands. Caller owns the returned slice. +/// the body. Caller owns the returned slice. pub fn formatHealReplacement( arena: std.mem.Allocator, original_span: []const u8, opener_line: []const u8, - cmds: []const Command, + body: HealBody, ) !Replacement { - std.debug.assert(cmds.len > 0); var aw: std.Io.Writer.Allocating = .init(arena); try aw.writer.print("# [Auto-healed] Original: {s}\n", .{opener_line}); - for (cmds) |cmd| { - try cmd.format(&aw.writer); - try aw.writer.writeByte('\n'); + switch (body) { + .cmds => |cmds| for (cmds) |cmd| { + try cmd.format(&aw.writer); + try aw.writer.writeByte('\n'); + }, + .lines => |lines| for (lines) |line| { + try aw.writer.writeAll(line); + try aw.writer.writeByte('\n'); + }, } return .{ .original_span = original_span, .new_text = aw.written() }; } -/// Same shape as `formatHealReplacement` but for callers that already have -/// rendered replacement lines (no Command round-trip). Used by the MCP -/// `script_heal` tool where the LLM driver supplies raw PandaScript lines. -pub fn formatHealReplacementLines( - arena: std.mem.Allocator, - original_span: []const u8, - opener_line: []const u8, - replacement_lines: []const []const u8, -) !Replacement { - var aw: std.Io.Writer.Allocating = .init(arena); - - try aw.writer.print("# [Auto-healed] Original: {s}\n", .{opener_line}); - for (replacement_lines) |line| { - try aw.writer.writeAll(line); - try aw.writer.writeByte('\n'); - } - - return .{ - .original_span = original_span, - .new_text = aw.written(), - }; -} - /// Reject paths that an untrusted MCP client could use to escape the /// working directory: empty paths, absolute paths, and any path with a /// `..` segment. Operator-controlled symlinks already inside CWD are out @@ -375,45 +364,31 @@ fn buildToolCall(arena: std.mem.Allocator, name: []const u8, kvs: []const struct return .{ .tool_call = .{ .tool = tool, .args = .{ .object = obj } } }; } -test "formatHealReplacement: single command produces one-line replacement" { - var arena: std.heap.ArenaAllocator = .init(std.testing.allocator); - defer arena.deinit(); - - const cmds = [_]Command{buildToolCall(arena.allocator(), "click", &.{.{ "selector", "#submit-v2" }})}; - const replacement = try formatHealReplacement( - arena.allocator(), - "/click selector='#submit'\n", - "/click selector='#submit'", - &cmds, - ); - - try std.testing.expectEqualStrings("/click selector='#submit'\n", replacement.original_span); - try std.testing.expectEqualStrings( - "# [Auto-healed] Original: /click selector='#submit'\n/click selector='#submit-v2'\n", - replacement.new_text, - ); -} - -test "formatHealReplacement: multiple commands produce multi-line replacement" { +test "formatHealReplacement: single and multiple commands" { var arena: std.heap.ArenaAllocator = .init(std.testing.allocator); defer arena.deinit(); const aa = arena.allocator(); - const cmds = [_]Command{ - buildToolCall(aa, "click", &.{.{ "selector", ".cookie-accept" }}), - buildToolCall(aa, "click", &.{.{ "selector", "#submit-v2" }}), - }; - const replacement = try formatHealReplacement( - aa, - "/click selector='#submit'\n", - "/click selector='#submit'", - &cmds, - ); - - try std.testing.expectEqualStrings( - "# [Auto-healed] Original: /click selector='#submit'\n/click selector='.cookie-accept'\n/click selector='#submit-v2'\n", - replacement.new_text, - ); + { + const cmds = [_]Command{buildToolCall(aa, "click", &.{.{ "selector", "#submit-v2" }})}; + const r = try formatHealReplacement(aa, "/click selector='#submit'\n", "/click selector='#submit'", .{ .cmds = &cmds }); + try std.testing.expectEqualStrings("/click selector='#submit'\n", r.original_span); + try std.testing.expectEqualStrings( + "# [Auto-healed] Original: /click selector='#submit'\n/click selector='#submit-v2'\n", + r.new_text, + ); + } + { + const cmds = [_]Command{ + buildToolCall(aa, "click", &.{.{ "selector", ".cookie-accept" }}), + buildToolCall(aa, "click", &.{.{ "selector", "#submit-v2" }}), + }; + const r = try formatHealReplacement(aa, "/click selector='#submit'\n", "/click selector='#submit'", .{ .cmds = &cmds }); + try std.testing.expectEqualStrings( + "# [Auto-healed] Original: /click selector='#submit'\n/click selector='.cookie-accept'\n/click selector='#submit-v2'\n", + r.new_text, + ); + } } test "writeAtomic: writes content and creates .bak" { diff --git a/src/script/Schema.zig b/src/script/Schema.zig index ed077690..5e81c83a 100644 --- a/src/script/Schema.zig +++ b/src/script/Schema.zig @@ -244,7 +244,7 @@ pub fn parseSlashCommand(input: []const u8) ?Split { return splitNameRest(input[1..]); } -pub fn find(schemas: []const Schema, name: []const u8) ?*const Schema { +fn find(schemas: []const Schema, name: []const u8) ?*const Schema { if (std.meta.stringToEnum(BrowserTool, name)) |tool| { const idx = @intFromEnum(tool); if (idx < schemas.len) return &schemas[idx]; @@ -452,7 +452,7 @@ pub const QuoteType = enum { return if (s.len == 3) fromPrefix(s) else null; } - pub fn fromPrefix(s: []const u8) ?QuoteType { + fn fromPrefix(s: []const u8) ?QuoteType { if (std.mem.startsWith(u8, s, "\"\"\"")) return .triple_double; if (std.mem.startsWith(u8, s, "'''")) return .triple_single; return null; @@ -467,7 +467,7 @@ pub const QuoteType = enum { /// Pick a triple-quote delimiter not appearing in `body`. Null when /// both appear and neither can wrap unambiguously. - pub fn pickFor(body: []const u8) ?QuoteType { + fn pickFor(body: []const u8) ?QuoteType { const has_single = std.mem.indexOf(u8, body, "'''") != null; const has_double = std.mem.indexOf(u8, body, "\"\"\"") != null; if (has_single and has_double) return null; @@ -515,7 +515,7 @@ pub fn writeInlineValue(writer: *std.Io.Writer, v: std.json.Value) (std.Io.Write /// Caller must filter via `quotableInline` first; remaining ambiguous /// cases trap as `WriteFailed` so a stray path can't emit a broken line. -pub fn writeQuoted(writer: *std.Io.Writer, s: []const u8) (std.Io.Writer.Error || error{AmbiguousQuoting})!void { +fn writeQuoted(writer: *std.Io.Writer, s: []const u8) (std.Io.Writer.Error || error{AmbiguousQuoting})!void { if (std.mem.indexOfScalar(u8, s, '\n') != null) return error.WriteFailed; const has_single = std.mem.indexOfScalar(u8, s, '\'') != null; diff --git a/src/script/command.zig b/src/script/command.zig index 16ea854b..370e7775 100644 --- a/src/script/command.zig +++ b/src/script/command.zig @@ -285,24 +285,22 @@ test "format: /eval emits triple-quote block for multi-line script" { try testing.expectString("/eval '''\nconst x = 1;\nreturn x;\n'''", aw.written()); } -test "format: /setChecked omits checked=true (matches default)" { +test "format: /setChecked omits checked=true (default), keeps checked=false" { var arena: std.heap.ArenaAllocator = .init(testing.allocator); defer arena.deinit(); - const cmd = try Command.parse(arena.allocator(), "/setChecked selector='#agree' checked=true"); - var aw: std.Io.Writer.Allocating = .init(testing.allocator); - defer aw.deinit(); - try cmd.format(&aw.writer); - try testing.expectString("/setChecked selector='#agree'", aw.written()); -} + const aa = arena.allocator(); -test "format: /setChecked keeps checked=false (non-default)" { - var arena: std.heap.ArenaAllocator = .init(testing.allocator); - defer arena.deinit(); - const cmd = try Command.parse(arena.allocator(), "/setChecked selector='#x' checked=false"); - var aw: std.Io.Writer.Allocating = .init(testing.allocator); - defer aw.deinit(); - try cmd.format(&aw.writer); - try testing.expectString("/setChecked selector='#x' checked=false", aw.written()); + const cases = [_]struct { input: []const u8, expected: []const u8 }{ + .{ .input = "/setChecked selector='#agree' checked=true", .expected = "/setChecked selector='#agree'" }, + .{ .input = "/setChecked selector='#x' checked=false", .expected = "/setChecked selector='#x' checked=false" }, + }; + for (cases) |case| { + const cmd = try Command.parse(aa, case.input); + var aw: std.Io.Writer.Allocating = .init(testing.allocator); + defer aw.deinit(); + try cmd.format(&aw.writer); + try testing.expectString(case.expected, aw.written()); + } } test "format: /login and /acceptCookies" { @@ -317,6 +315,26 @@ test "format: /login and /acceptCookies" { try testing.expectString("/acceptCookies", aw2.written()); } +test "canHeal: only page-local DOM commands are allowed" { + // Table-driven over the live tool flags so adding a new tool can't + // silently drift from the heal allow-list. + const allow = [_]BrowserTool{ .click, .hover, .waitForSelector, .fill, .selectOption, .setChecked, .scroll, .extract, .press }; + const deny = [_]BrowserTool{ .goto, .eval, .tree, .markdown, .search, .links }; + + for (allow) |action| { + const cmd = Command.fromToolCall(action, null); + try testing.expect(cmd.canHeal()); + } + for (deny) |action| { + const cmd = Command.fromToolCall(action, null); + try testing.expect(!cmd.canHeal()); + } + + try testing.expect(!(Command{ .login = {} }).canHeal()); + try testing.expect(!(Command{ .acceptCookies = {} }).canHeal()); + try testing.expect(!(Command{ .comment = {} }).canHeal()); +} + test "isRecorded / canHeal / producesData via tool flags" { var arena: std.heap.ArenaAllocator = .init(testing.allocator); defer arena.deinit(); @@ -335,22 +353,22 @@ test "isRecorded / canHeal / producesData via tool flags" { try testing.expect(!login.canHeal()); } -test "isRecorded: null args on a required-fields tool are not recorded" { - // A provider that hands back `arguments: null` for `/click` would - // otherwise produce a bare `/click` line that can't be replayed. - const click_null = Command.fromToolCall(.click, null); - try testing.expect(click_null.isRecorded()); // click has zero required fields - const goto_null = Command.fromToolCall(.goto, null); - try testing.expect(!goto_null.isRecorded()); // goto requires url - const fill_null = Command.fromToolCall(.fill, null); - try testing.expect(!fill_null.isRecorded()); // fill requires value -} - -test "isRecorded and format: backendNodeId stripped, selector preserved" { +test "isRecorded: args shape and locator semantics" { var arena: std.heap.ArenaAllocator = .init(testing.allocator); defer arena.deinit(); const aa = arena.allocator(); + // Null args: recorded iff the tool has zero required fields. A provider + // that hands back `arguments: null` for `/click` would otherwise produce + // a bare `/click` line that can't be replayed. + try testing.expect(Command.fromToolCall(.click, null).isRecorded()); + try testing.expect(!Command.fromToolCall(.goto, null).isRecorded()); + try testing.expect(!Command.fromToolCall(.fill, null).isRecorded()); + + // Non-object args: recorded iff the tool doesn't need a locator. + try testing.expect(Command.fromToolCall(.goto, .{ .string = "https://x" }).isRecorded()); + try testing.expect(!Command.fromToolCall(.click, .{ .string = "#submit" }).isRecorded()); + // selector + backendNodeId: keep the call, drop the backendNodeId. { var obj: std.json.ObjectMap = .init(aa); @@ -365,7 +383,7 @@ test "isRecorded and format: backendNodeId stripped, selector preserved" { try testing.expectString("/click selector='#submit'", aw.written()); } - // backendNodeId only: still skipped — no replayable identifier. + // backendNodeId only: skipped — no replayable identifier. { var obj: std.json.ObjectMap = .init(aa); try obj.put("backendNodeId", .{ .integer = 42 }); @@ -373,24 +391,3 @@ test "isRecorded and format: backendNodeId stripped, selector preserved" { try testing.expect(!cmd.isRecorded()); } } - -test "fromToolCall: builds a tool_call Command" { - var arena: std.heap.ArenaAllocator = .init(testing.allocator); - defer arena.deinit(); - - var obj: std.json.ObjectMap = .init(arena.allocator()); - try obj.put("url", .{ .string = "https://x" }); - const cmd = Command.fromToolCall(.goto, .{ .object = obj }); - try testing.expect(cmd == .tool_call); - try testing.expectString("goto", cmd.tool_call.name()); -} - -test "isRecorded: non-object args check locator presence" { - // goto does not need a locator: isRecorded returns true even if args is not object - const goto_non_obj = Command.fromToolCall(.goto, .{ .string = "https://x" }); - try testing.expect(goto_non_obj.isRecorded()); - - // click needs a locator: isRecorded returns false if args is not object - const click_non_obj = Command.fromToolCall(.click, .{ .string = "#submit" }); - try testing.expect(!click_non_obj.isRecorded()); -}