From 3cc652776bfd05bc9f5a2851f762017c9beef565 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A0=20Arrufat?= Date: Sat, 11 Apr 2026 15:33:06 +0200 Subject: [PATCH] agent: simplify command execution and REPL logic - Use `std.json.Stringify` for safer JS escaping in `execExtract`. - Streamline `CommandExecutor` and remove redundant helpers. - Add "QUIT" as an alias for the "EXIT" command. - Clean up REPL loop control flow and error reporting in `Agent`. - Simplify `Recorder` file operations and logging. --- src/agent/Agent.zig | 53 +++++++++------------ src/agent/Command.zig | 8 ++-- src/agent/CommandExecutor.zig | 89 ++++++++++------------------------- src/agent/Recorder.zig | 31 +++++------- 4 files changed, 63 insertions(+), 118 deletions(-) diff --git a/src/agent/Agent.zig b/src/agent/Agent.zig index 9989a82c..bd1bb6fa 100644 --- a/src/agent/Agent.zig +++ b/src/agent/Agent.zig @@ -80,10 +80,10 @@ self_heal: bool, interactive: bool, pub fn init(allocator: std.mem.Allocator, app: *App, opts: Config.Agent) !*Self { - // Pure replay (positional script, no -i) is the only mode that skips the REPL. + // Pure replay (positional script, no -i) skips the REPL. const will_repl = opts.interactive or opts.script_file == null; - // --self-heal is meaningless without a provider to heal through. + // --self-heal needs a provider to heal through. if (opts.self_heal and opts.provider == null) { log.fatal(.app, "missing --provider", .{ .hint = "--self-heal requires --provider; drop one or add the other", @@ -91,9 +91,8 @@ pub fn init(allocator: std.mem.Allocator, app: *App, opts: Config.Agent) !*Self return error.SelfHealWithoutProvider; } - // An API key is only needed when we're going to open the REPL *and* a - // provider was selected. Without a provider we run the REPL in "dumb" - // Pandascript-only mode. + // An API key is only required when the REPL will run — pure replay with + // a provider is fine without one since no AI turn ever executes. const api_key: ?[:0]const u8 = if (opts.provider) |p| getEnvApiKey(p) orelse if (will_repl) { log.fatal(.app, "missing API key", .{ @@ -127,12 +126,11 @@ pub fn init(allocator: std.mem.Allocator, app: *App, opts: Config.Agent) !*Self return error.ToolInitFailed; }; - // Persist REPL history in a cwd-relative `.lp-history`. Skipped in pure - // replay mode (no REPL is opened). + // Persist REPL history in a cwd-relative `.lp-history`; skipped in pure replay. const history_path: ?[:0]const u8 = if (will_repl) ".lp-history" else null; - // Record REPL commands into the positional script file only when both - // are present — `-i ` means "replay then grow this file". + // `-i ` means "replay then grow this file"; a script path alone is + // pure replay and must not be mutated. const recorder_path: ?[]const u8 = if (opts.interactive) opts.script_file else null; self.* = .{ @@ -173,9 +171,8 @@ pub fn deinit(self: *Self) void { self.allocator.destroy(self); } -/// Returns true on success. In interactive mode the REPL always runs after -/// the (optional) replay phase and the function always returns true; in pure -/// replay mode it returns whatever `runScript` returned. +/// Returns true on success. Interactive mode always returns true; pure +/// replay mirrors `runScript`'s result. pub fn run(self: *Self) bool { if (self.script_file) |path| { const script_ok = self.runScript(path); @@ -194,7 +191,7 @@ fn runRepl(self: *Self) void { self.terminal.printInfo("Dumb REPL (no --provider) — Pandascript only. Pass --provider for natural-language, LOGIN, and ACCEPT_COOKIES."); } - while (true) { + repl: while (true) { const line = self.terminal.readLine("> ") orelse break; defer self.terminal.freeLine(line); @@ -202,24 +199,22 @@ fn runRepl(self: *Self) void { const cmd = Command.parse(line); - if (cmd == .exit or (cmd == .natural_language and std.mem.eql(u8, line, "quit"))) break; - if (cmd == .comment) continue; - if (cmd.needsLlm() and self.ai_client == null) { self.terminal.printError("This command requires --provider. Pandascript commands (GOTO, CLICK, EXTRACT, ...) work without one."); continue; } switch (cmd) { - .exit, .comment => unreachable, // handled above + .exit => break :repl, + .comment => continue :repl, .login => self.processUserMessage(login_prompt, line) catch |err| { - self.printAllocError("LOGIN failed: {s}", .{@errorName(err)}); + self.terminal.printErrorFmt("LOGIN failed: {s}", .{@errorName(err)}); }, .accept_cookies => self.processUserMessage(accept_cookies_prompt, line) catch |err| { - self.printAllocError("ACCEPT_COOKIES failed: {s}", .{@errorName(err)}); + self.terminal.printErrorFmt("ACCEPT_COOKIES failed: {s}", .{@errorName(err)}); }, .natural_language => self.processUserMessage(line, line) catch |err| { - self.printAllocError("Request failed: {s}", .{@errorName(err)}); + self.terminal.printErrorFmt("Request failed: {s}", .{@errorName(err)}); }, else => { self.cmd_executor.execute(cmd); @@ -231,19 +226,15 @@ fn runRepl(self: *Self) void { self.terminal.printInfo("Goodbye!"); } -fn printAllocError(self: *Self, comptime fmt: []const u8, args: anytype) void { - self.terminal.printErrorFmt(fmt, args); -} - fn runScript(self: *Self, path: []const u8) bool { const file = std.fs.cwd().openFile(path, .{}) catch |err| { - self.printAllocError("Failed to open script '{s}': {s}", .{ path, @errorName(err) }); + self.terminal.printErrorFmt("Failed to open script '{s}': {s}", .{ path, @errorName(err) }); return false; }; defer file.close(); const content = file.readToEndAlloc(self.allocator, 10 * 1024 * 1024) catch |err| { - self.printAllocError("Failed to read script: {s}", .{@errorName(err)}); + self.terminal.printErrorFmt("Failed to read script: {s}", .{@errorName(err)}); return false; }; defer self.allocator.free(content); @@ -269,12 +260,12 @@ fn runScript(self: *Self, path: []const u8) bool { continue; }, .natural_language => { - self.printAllocError("line {d}: unrecognized command: {s}", .{ entry.line_num, entry.raw_line }); + self.terminal.printErrorFmt("line {d}: unrecognized command: {s}", .{ entry.line_num, entry.raw_line }); return false; }, .login, .accept_cookies => { if (self.ai_client == null) { - self.printAllocError("line {d}: {s} requires --provider", .{ + self.terminal.printErrorFmt("line {d}: {s} requires --provider", .{ entry.line_num, entry.raw_line, }); @@ -282,7 +273,7 @@ fn runScript(self: *Self, path: []const u8) bool { } const prompt = if (entry.command == .login) login_prompt else accept_cookies_prompt; self.processUserMessage(prompt, "") catch |err| { - self.printAllocError("line {d}: {s} failed: {s}", .{ + self.terminal.printErrorFmt("line {d}: {s} failed: {s}", .{ entry.line_num, entry.raw_line, @errorName(err), @@ -306,7 +297,7 @@ fn runScript(self: *Self, path: []const u8) bool { continue; } } - self.printAllocError("line {d}: command failed: {s}", .{ + self.terminal.printErrorFmt("line {d}: command failed: {s}", .{ entry.line_num, entry.raw_line, }); @@ -338,7 +329,7 @@ fn attemptSelfHeal(self: *Self, intent: ?[]const u8, failed_command: []const u8) var attempt: u8 = 0; while (attempt < self_heal_max_attempts) : (attempt += 1) { self.processUserMessage(prompt, "") catch |err| { - self.printAllocError("self-heal attempt {d}/{d} failed: {s}", .{ + self.terminal.printErrorFmt("self-heal attempt {d}/{d} failed: {s}", .{ attempt + 1, self_heal_max_attempts, @errorName(err), diff --git a/src/agent/Command.zig b/src/agent/Command.zig index 7e1ceba1..1bb0b5a3 100644 --- a/src/agent/Command.zig +++ b/src/agent/Command.zig @@ -213,7 +213,7 @@ pub fn parse(line: []const u8) Command { return .{ .accept_cookies = {} }; } - if (std.ascii.eqlIgnoreCase(cmd_word, "EXIT")) { + if (std.ascii.eqlIgnoreCase(cmd_word, "EXIT") or std.ascii.eqlIgnoreCase(cmd_word, "QUIT")) { return .{ .exit = {} }; } @@ -309,9 +309,9 @@ const QuotedResult = struct { fn extractQuotedWithRemainder(s: []const u8) ?QuotedResult { if (s.len < 2) return null; - const quote = s[0]; - if (quote != '"' and quote != '\'') return null; - const end = std.mem.indexOfScalarPos(u8, s, 1, quote) orelse return null; + const q = s[0]; + if (q != '"' and q != '\'') return null; + const end = std.mem.indexOfScalarPos(u8, s, 1, q) orelse return null; return .{ .value = s[1..end], .remainder = s[end + 1 ..], diff --git a/src/agent/CommandExecutor.zig b/src/agent/CommandExecutor.zig index 32d927ef..5bcd487b 100644 --- a/src/agent/CommandExecutor.zig +++ b/src/agent/CommandExecutor.zig @@ -26,9 +26,15 @@ pub const ExecResult = struct { pub fn executeWithResult(self: *Self, a: std.mem.Allocator, cmd: Command.Command) ExecResult { const Action = browser_tools.Action; return switch (cmd) { - .goto => |url| self.execGoto(a, url), + .goto => |url| self.callTool(a, @tagName(Action.goto), buildJson(a, .{ .url = substituteEnvVars(a, url) })), .click => |sel| self.callTool(a, @tagName(Action.click), buildJson(a, .{ .selector = substituteEnvVars(a, sel) })), - .type_cmd => |args| self.execType(a, args), + // execFill in browser/tools.zig substitutes `value` itself so the + // displayed result keeps the `$LP_*` reference instead of leaking + // the resolved secret back to the terminal. + .type_cmd => |args| self.callTool(a, @tagName(Action.fill), buildJson(a, .{ + .selector = substituteEnvVars(a, args.selector), + .value = args.value, + })), .wait => |selector| self.callTool(a, @tagName(Action.waitForSelector), buildJson(a, .{ .selector = selector })), .scroll => |args| self.callTool(a, @tagName(Action.scroll), buildJson(a, .{ .x = args.x, .y = args.y })), .hover => |sel| self.callTool(a, @tagName(Action.hover), buildJson(a, .{ .selector = substituteEnvVars(a, sel) })), @@ -56,9 +62,8 @@ pub fn execute(self: *Self, cmd: Command.Command) void { self.printResult(cmd, result); } -/// Route a command's output to stdout (for data-producing commands like -/// EXTRACT/EVAL/MARKDOWN/TREE) or stderr (for action commands like -/// GOTO/CLICK/...) so that shell-redirecting stdout captures only data. +/// Data-producing commands (EXTRACT/EVAL/MARKDOWN/TREE) go to stdout so shell +/// redirection captures only their output; action commands go to stderr. pub fn printResult(self: *Self, cmd: Command.Command, result: ExecResult) void { if (cmd.producesData()) { self.terminal.printAssistant(result.output); @@ -74,80 +79,34 @@ fn callTool(self: *Self, arena: std.mem.Allocator, tool_name: []const u8, argume return .{ .output = std.fmt.allocPrint(arena, "{s} failed: {s}", .{ tool_name, @errorName(err) }) catch "tool failed", .failed = true }; } -fn execGoto(self: *Self, arena: std.mem.Allocator, raw_url: []const u8) ExecResult { - const url = substituteEnvVars(arena, raw_url); - return self.callTool(arena, @tagName(browser_tools.Action.goto), buildJson(arena, .{ .url = url })); -} - -fn execType(self: *Self, arena: std.mem.Allocator, args: Command.TypeArgs) ExecResult { - const selector = substituteEnvVars(arena, args.selector); - const value = substituteEnvVars(arena, args.value); - return self.callTool(arena, @tagName(browser_tools.Action.fill), buildJson(arena, .{ .selector = selector, .value = value })); -} - fn execExtract(self: *Self, arena: std.mem.Allocator, raw_selector: []const u8) ExecResult { - const selector = escapeJs(arena, substituteEnvVars(arena, raw_selector)); + const selector = substituteEnvVars(arena, raw_selector); - const script = std.fmt.allocPrint(arena, - \\JSON.stringify(Array.from(document.querySelectorAll("{s}")).map(el => el.textContent.trim())) - , .{selector}) catch return .{ .output = "failed to build extract script", .failed = true }; + // `std.json.Stringify.value` emits a quoted, JS-safe string literal, which + // is also a valid JS string literal — reuse it to splice the selector into + // the querySelectorAll call. + var aw: std.Io.Writer.Allocating = .init(arena); + std.json.Stringify.value(selector, .{}, &aw.writer) catch + return .{ .output = "failed to encode selector", .failed = true }; + const encoded = aw.written(); + + const script = std.fmt.allocPrint( + arena, + "JSON.stringify(Array.from(document.querySelectorAll({s})).map(el => el.textContent.trim()))", + .{encoded}, + ) catch return .{ .output = "failed to build extract script", .failed = true }; return self.callTool(arena, @tagName(browser_tools.Action.eval), buildJson(arena, .{ .script = script })); } const substituteEnvVars = browser_tools.substituteEnvVars; -fn escapeJs(arena: std.mem.Allocator, input: []const u8) []const u8 { - const needs_escape = for (input) |ch| { - if (ch == '"' or ch == '\\' or ch == '\n' or ch == '\r' or ch == '\t') break true; - } else false; - if (!needs_escape) return input; - - var out: std.ArrayListUnmanaged(u8) = .empty; - for (input) |ch| { - switch (ch) { - '\\' => out.appendSlice(arena, "\\\\") catch return input, - '"' => out.appendSlice(arena, "\\\"") catch return input, - '\n' => out.appendSlice(arena, "\\n") catch return input, - '\r' => out.appendSlice(arena, "\\r") catch return input, - '\t' => out.appendSlice(arena, "\\t") catch return input, - else => out.append(arena, ch) catch return input, - } - } - return out.toOwnedSlice(arena) catch input; -} - fn buildJson(arena: std.mem.Allocator, value: anytype) []const u8 { var aw: std.Io.Writer.Allocating = .init(arena); std.json.Stringify.value(value, .{}, &aw.writer) catch return "{}"; return aw.written(); } -// --- Tests --- - -test "escapeJs no escaping needed" { - const result = escapeJs(std.testing.allocator, "hello world"); - try std.testing.expectEqualStrings("hello world", result); -} - -test "escapeJs quotes and backslashes" { - const result = escapeJs(std.testing.allocator, "say \"hello\\world\""); - defer std.testing.allocator.free(result); - try std.testing.expectEqualStrings("say \\\"hello\\\\world\\\"", result); -} - -test "escapeJs newlines and tabs" { - const result = escapeJs(std.testing.allocator, "line1\nline2\ttab"); - defer std.testing.allocator.free(result); - try std.testing.expectEqualStrings("line1\\nline2\\ttab", result); -} - -test "escapeJs injection attempt" { - const result = escapeJs(std.testing.allocator, "\"; alert(1); //"); - defer std.testing.allocator.free(result); - try std.testing.expectEqualStrings("\\\"; alert(1); //", result); -} - test "substituteEnvVars no vars" { const result = substituteEnvVars(std.testing.allocator, "hello world"); try std.testing.expectEqualStrings("hello world", result); diff --git a/src/agent/Recorder.zig b/src/agent/Recorder.zig index d34cb271..a3346ab8 100644 --- a/src/agent/Recorder.zig +++ b/src/agent/Recorder.zig @@ -1,4 +1,6 @@ const std = @import("std"); +const lp = @import("lightpanda"); +const log = lp.log; const Command = @import("Command.zig"); const Self = @This(); @@ -6,17 +8,16 @@ const Self = @This(); file: ?std.fs.File, needs_separator: bool, -/// Open `path` for append. The file is created if missing; if it already has -/// content, a leading newline is written so the appended block starts on a -/// fresh line. A null path disables recording (no-op). +/// Append-open `path`, inserting a leading newline if the file is non-empty. +/// A null path disables recording. pub fn init(path: ?[]const u8) Self { const file: ?std.fs.File = if (path) |p| blk: { const f = std.fs.cwd().createFile(p, .{ .truncate = false }) catch |err| { - std.debug.print("Warning: could not open recording file: {s}\n", .{@errorName(err)}); + log.warn(.app, "could not open recording file", .{ .err = @errorName(err) }); break :blk null; }; f.seekFromEnd(0) catch |err| { - std.debug.print("Warning: could not seek in recording file: {s}\n", .{@errorName(err)}); + log.warn(.app, "could not seek recording file", .{ .err = @errorName(err) }); f.close(); break :blk null; }; @@ -36,25 +37,19 @@ pub fn record(self: *Self, cmd: Command.Command) void { const f = self.file orelse return; if (!cmd.isRecorded()) return; - var buf: [4096]u8 = undefined; - var file_writer = f.writerStreaming(&buf); - const writer = &file_writer.interface; - writer.print("{f}\n", .{cmd}) catch return; - writer.flush() catch return; + var buf: [1024]u8 = undefined; + const line = std.fmt.bufPrint(&buf, "{f}\n", .{cmd}) catch return; + _ = f.write(line) catch return; self.needs_separator = true; } pub fn recordComment(self: *Self, comment: []const u8) void { const f = self.file orelse return; - var buf: [4096]u8 = undefined; - var file_writer = f.writerStreaming(&buf); - const writer = &file_writer.interface; - if (self.needs_separator) writer.writeByte('\n') catch return; + var buf: [1024]u8 = undefined; + const prefix: []const u8 = if (self.needs_separator) "\n# " else "# "; + const line = std.fmt.bufPrint(&buf, "{s}{s}\n", .{ prefix, comment }) catch return; + _ = f.write(line) catch return; self.needs_separator = true; - writer.writeAll("# ") catch return; - writer.writeAll(comment) catch return; - writer.writeByte('\n') catch return; - writer.flush() catch return; } test "record writes state-mutating commands" {