From bf172bd811910bb99dc8d2f785e62bb5368e0d5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A0=20Arrufat?= Date: Thu, 7 May 2026 09:52:43 +0200 Subject: [PATCH] refactor: consolidate browser tool helpers and optimize logic - Extract `awaitQueuedNavigation` and `mapActionError` in `tools.zig`. - Optimize `substituteEnvVars` using `indexOfScalarPos` to reduce copies. - Switch to `parseFromSliceLeaky` for tool arguments parsing. - Improve error handling in `Agent.zig` self-healing and `Recorder.zig`. - Simplify `callEval` by adding `evalScript` helper. --- src/agent/Agent.zig | 4 +- src/agent/Command.zig | 4 +- src/agent/Recorder.zig | 12 +-- src/agent/Terminal.zig | 11 +-- src/agent/ToolExecutor.zig | 4 +- src/browser/tools.zig | 149 +++++++++++++++++-------------------- 6 files changed, 85 insertions(+), 99 deletions(-) diff --git a/src/agent/Agent.zig b/src/agent/Agent.zig index bb61ed7b..a3115de4 100644 --- a/src/agent/Agent.zig +++ b/src/agent/Agent.zig @@ -802,12 +802,12 @@ fn attemptSelfHeal(self: *Self, arena: std.mem.Allocator, failed_command: []cons const ha = self.message_arena.allocator(); const verify_section = if (verify_context) |ctx| - std.fmt.allocPrint(ha, "\n\nVerification detected a problem:\n{s}", .{ctx}) catch "" + std.fmt.allocPrint(ha, "\n\nVerification detected a problem:\n{s}", .{ctx}) catch return null else ""; const comment_section = if (context_comment) |c| - std.fmt.allocPrint(ha, "\n\nThe original user request that generated this command was:\n{s}", .{c}) catch "" + std.fmt.allocPrint(ha, "\n\nThe original user request that generated this command was:\n{s}", .{c}) catch return null else ""; diff --git a/src/agent/Command.zig b/src/agent/Command.zig index a162199c..97aea05e 100644 --- a/src/agent/Command.zig +++ b/src/agent/Command.zig @@ -451,8 +451,8 @@ pub fn toToolCall(arena: std.mem.Allocator, cmd: Command, substitute: Substitute pub fn fromToolCall(arena: std.mem.Allocator, tool_name: []const u8, arguments: []const u8) ?Command { const Action = lp.tools.Action; const action = std.meta.stringToEnum(Action, tool_name) orelse return null; - const parsed = std.json.parseFromSlice(std.json.Value, arena, arguments, .{}) catch return null; - const obj = switch (parsed.value) { + const parsed = std.json.parseFromSliceLeaky(std.json.Value, arena, arguments, .{}) catch return null; + const obj = switch (parsed) { .object => |o| o, else => return null, }; diff --git a/src/agent/Recorder.zig b/src/agent/Recorder.zig index 94806f29..f058afaa 100644 --- a/src/agent/Recorder.zig +++ b/src/agent/Recorder.zig @@ -26,7 +26,7 @@ pub fn init(allocator: std.mem.Allocator, path: ?[]const u8) Self { break :blk null; }; const pos = f.getPos() catch 0; - if (pos > 0) _ = f.write("\n") catch {}; + if (pos > 0) f.writeAll("\n") catch {}; break :blk f; } else null; @@ -45,16 +45,18 @@ pub fn record(self: *Self, cmd: Command.Command) void { self.buf.clearRetainingCapacity(); cmd.format(&self.buf.writer) catch return; self.buf.writer.writeByte('\n') catch return; - _ = f.write(self.buf.written()) catch return; + f.writeAll(self.buf.written()) catch return; self.needs_separator = true; } pub fn recordComment(self: *Self, comment: []const u8) void { const f = self.file orelse return; + self.buf.clearRetainingCapacity(); const prefix: []const u8 = if (self.needs_separator) "\n# " else "# "; - f.writeAll(prefix) catch return; - f.writeAll(comment) catch return; - f.writeAll("\n") catch return; + self.buf.writer.writeAll(prefix) catch return; + self.buf.writer.writeAll(comment) catch return; + self.buf.writer.writeByte('\n') catch return; + f.writeAll(self.buf.written()) catch return; self.needs_separator = true; } diff --git a/src/agent/Terminal.zig b/src/agent/Terminal.zig index 75c1c2f5..57f02d45 100644 --- a/src/agent/Terminal.zig +++ b/src/agent/Terminal.zig @@ -425,13 +425,10 @@ pub fn printToolCall(self: *Self, name: []const u8, args: []const u8) void { std.debug.print("\n{s}{s}[tool: {s}]{s} {s}\n", .{ ansi_dim, ansi_cyan, name, ansi_reset, args }); } -// 2000 keeps stderr readable while exposing the full window the LLM-judge -// actually consumes (SNAPSHOT_MAX_CHARS=900 in benchmarks/llm_judge.py); the -// 500 cap was the binding upstream limit and silently starved the judge of -// grounding evidence on tasks where the agent had observed the answer. -// Does NOT affect the agent's own LLM, which gets up to tool_output_max_bytes -// (1 MiB) via Agent.zig:capToolOutput. Bypassed in REPL: a human just asked -// for the data and would rather scroll than be silently lied to. +// Must exceed the downstream LLM-judge's snapshot window so it has full +// grounding evidence. Does not cap the agent's own LLM, which gets up to +// tool_output_max_bytes (1 MiB) via Agent.zig:capToolOutput. Bypassed in +// REPL where the human can scroll. const max_result_display_len = 2000; pub fn printToolResult(self: *Self, name: []const u8, result: []const u8) void { diff --git a/src/agent/ToolExecutor.zig b/src/agent/ToolExecutor.zig index a9c5d79d..4c3ee01b 100644 --- a/src/agent/ToolExecutor.zig +++ b/src/agent/ToolExecutor.zig @@ -93,9 +93,7 @@ pub fn getCurrentUrl(self: *Self) []const u8 { /// Run a JavaScript expression and return the full result (text + error flag). pub fn callEval(self: *Self, arena: std.mem.Allocator, script: []const u8) browser_tools.EvalResult { - var obj: std.json.ObjectMap = .init(arena); - obj.put("script", .{ .string = script }) catch return .{ .text = "out of memory", .is_error = true }; - return browser_tools.callEval(arena, self.session, &self.node_registry, .{ .object = obj }); + return browser_tools.evalScript(arena, self.session, &self.node_registry, script); } pub fn call(self: *Self, arena: std.mem.Allocator, tool_name: []const u8, arguments_json: []const u8) CallError![]const u8 { diff --git a/src/browser/tools.zig b/src/browser/tools.zig index 5abed3a9..31227943 100644 --- a/src/browser/tools.zig +++ b/src/browser/tools.zig @@ -402,6 +402,20 @@ pub fn callEval( return execEval(arena, session, registry, arguments); } +/// Run JavaScript against the current page, skipping the JSON parameter +/// round-trip that `callEval` requires. The script need not be 0-terminated; +/// a copy is made internally. +pub fn evalScript( + arena: std.mem.Allocator, + session: *lp.Session, + registry: *CDPNode.Registry, + script: []const u8, +) EvalResult { + const z = arena.dupeZ(u8, script) catch return .{ .text = "Error: out of memory", .is_error = true }; + const page = ensurePage(session, registry, null, null, null) catch return .{ .text = "Error: page not loaded", .is_error = true }; + return runEval(arena, page, z); +} + fn execGoto(arena: std.mem.Allocator, session: *lp.Session, registry: *CDPNode.Registry, arguments: ?std.json.Value) ToolError![]const u8 { const args = try parseArgs(GotoParams, arena, arguments); try performGoto(session, registry, args.url, args.timeout, args.waitUntil); @@ -503,12 +517,7 @@ fn execLinks(arena: std.mem.Allocator, session: *lp.Session, registry: *CDPNode. const links_list = lp.links.collectLinks(arena, page.document.asNode(), page) catch return ToolError.InternalError; - var aw: std.Io.Writer.Allocating = .init(arena); - for (links_list, 0..) |href, i| { - if (i > 0) aw.writer.writeByte('\n') catch {}; - aw.writer.writeAll(href) catch {}; - } - return aw.written(); + return std.mem.join(arena, "\n", links_list) catch return ToolError.InternalError; } fn execTree(arena: std.mem.Allocator, session: *lp.Session, registry: *CDPNode.Registry, arguments: ?std.json.Value) ToolError![]const u8 { @@ -613,7 +622,10 @@ fn execEval(arena: std.mem.Allocator, session: *lp.Session, registry: *CDPNode.R .is_error = true, }; const page = ensurePage(session, registry, args.url, args.timeout, args.waitUntil) catch return .{ .text = "Error: page not loaded", .is_error = true }; + return runEval(arena, page, args.script); +} +fn runEval(arena: std.mem.Allocator, page: *lp.Frame, script: [:0]const u8) EvalResult { var ls: lp.js.Local.Scope = undefined; page.js.localScope(&ls); defer ls.deinit(); @@ -622,7 +634,7 @@ fn execEval(arena: std.mem.Allocator, session: *lp.Session, registry: *CDPNode.R try_catch.init(&ls.local); defer try_catch.deinit(); - const js_result = ls.local.compileAndRun(args.script, null) catch |err| { + const js_result = ls.local.compileAndRun(script, null) catch |err| { const caught = try_catch.caughtOrError(arena, err); var aw: std.Io.Writer.Allocating = .init(arena); caught.format(&aw.writer) catch {}; @@ -652,6 +664,19 @@ fn resolveOptionalNode(registry: *CDPNode.Registry, backend_node_id: ?CDPNode.Id return node.dom; } +fn mapActionError(err: anytype) ToolError { + return if (err == error.InvalidNodeType) ToolError.InvalidParams else ToolError.InternalError; +} + +/// If the previous action queued a navigation (form submit, link click, +/// Enter on an input), drive the runner until it completes or times out. +fn awaitQueuedNavigation(session: *lp.Session) ToolError!void { + const page = session.currentPage() orelse return; + if (page.queued_navigation.items.len == 0) return; + var runner = session.runner(.{}) catch return ToolError.InternalError; + runner.wait(.{ .ms = 10000, .until = .done }) catch return ToolError.NavigationFailed; +} + /// Render `"{prefix} ({target}){suffix}. Page url: X, title: Y"`, where target /// is either `"selector: X"` or `"backendNodeId: N"`. fn formatActionResult( @@ -680,19 +705,9 @@ fn execClick(arena: std.mem.Allocator, session: *lp.Session, registry: *CDPNode. const args = try parseArgs(Params, arena, arguments); const resolved = try resolveTarget(session, registry, args.selector, args.backendNodeId); - lp.actions.click(resolved.node, resolved.page) catch |err| { - if (err == error.InvalidNodeType) return ToolError.InvalidParams; - return ToolError.InternalError; - }; + lp.actions.click(resolved.node, resolved.page) catch |err| return mapActionError(err); - // If the click triggered a navigation (e.g. form submission, link click), - // wait for it to complete. - if (session.currentPage()) |page| { - if (page.queued_navigation.items.len != 0) { - var runner = session.runner(.{}) catch return ToolError.InternalError; - runner.wait(.{ .ms = 10000, .until = .done }) catch return ToolError.NavigationFailed; - } - } + try awaitQueuedNavigation(session); const page = session.currentFrame() orelse return ToolError.FrameNotLoaded; return formatActionResult(arena, "Clicked element", args.selector, args.backendNodeId, "", page); @@ -710,10 +725,7 @@ fn execFill(arena: std.mem.Allocator, session: *lp.Session, registry: *CDPNode.R const text = substituteEnvVars(arena, raw_text); const resolved = try resolveTarget(session, registry, args.selector, args.backendNodeId); - lp.actions.fill(resolved.node, text, resolved.page) catch |err| { - if (err == error.InvalidNodeType) return ToolError.InvalidParams; - return ToolError.InternalError; - }; + lp.actions.fill(resolved.node, text, resolved.page) catch |err| return mapActionError(err); // Show the original reference (e.g. $LP_PASSWORD) in the result, not the resolved value const display_text = if (text.ptr != raw_text.ptr) raw_text else text; @@ -731,10 +743,7 @@ fn execScroll(arena: std.mem.Allocator, session: *lp.Session, registry: *CDPNode const page = session.currentFrame() orelse return ToolError.FrameNotLoaded; const target_node = try resolveOptionalNode(registry, args.backendNodeId); - lp.actions.scroll(target_node, args.x, args.y, page) catch |err| { - if (err == error.InvalidNodeType) return ToolError.InvalidParams; - return ToolError.InternalError; - }; + lp.actions.scroll(target_node, args.x, args.y, page) catch |err| return mapActionError(err); const page_title = page.getTitle() catch null; return std.fmt.allocPrint(arena, "Scrolled to x: {d}, y: {d}. Page url: {s}, title: {s}", .{ @@ -773,10 +782,7 @@ fn execHover(arena: std.mem.Allocator, session: *lp.Session, registry: *CDPNode. const args = try parseArgs(Params, arena, arguments); const resolved = try resolveTarget(session, registry, args.selector, args.backendNodeId); - lp.actions.hover(resolved.node, resolved.page) catch |err| { - if (err == error.InvalidNodeType) return ToolError.InvalidParams; - return ToolError.InternalError; - }; + lp.actions.hover(resolved.node, resolved.page) catch |err| return mapActionError(err); return formatActionResult(arena, "Hovered element", args.selector, args.backendNodeId, "", resolved.page); } @@ -791,18 +797,10 @@ fn execPress(arena: std.mem.Allocator, session: *lp.Session, registry: *CDPNode. const page = session.currentFrame() orelse return ToolError.FrameNotLoaded; const target_node = try resolveOptionalNode(registry, args.backendNodeId); - lp.actions.press(target_node, args.key, page) catch |err| { - if (err == error.InvalidNodeType) return ToolError.InvalidParams; - return ToolError.InternalError; - }; + lp.actions.press(target_node, args.key, page) catch |err| return mapActionError(err); // Pressing Enter on a form input triggers implicit form submission. - if (session.currentPage()) |p| { - if (p.queued_navigation.items.len != 0) { - var runner = session.runner(.{}) catch return ToolError.InternalError; - runner.wait(.{ .ms = 10000, .until = .done }) catch return ToolError.NavigationFailed; - } - } + try awaitQueuedNavigation(session); const current_page = session.currentFrame() orelse return ToolError.FrameNotLoaded; const page_title = current_page.getTitle() catch null; @@ -822,10 +820,7 @@ fn execSelectOption(arena: std.mem.Allocator, session: *lp.Session, registry: *C const args = try parseArgs(Params, arena, arguments); const resolved = try resolveTarget(session, registry, args.selector, args.backendNodeId); - lp.actions.selectOption(resolved.node, args.value, resolved.page) catch |err| { - if (err == error.InvalidNodeType) return ToolError.InvalidParams; - return ToolError.InternalError; - }; + lp.actions.selectOption(resolved.node, args.value, resolved.page) catch |err| return mapActionError(err); const prefix = std.fmt.allocPrint(arena, "Selected option '{s}'", .{args.value}) catch return ToolError.InternalError; return formatActionResult(arena, prefix, args.selector, args.backendNodeId, "", resolved.page); @@ -840,10 +835,7 @@ fn execSetChecked(arena: std.mem.Allocator, session: *lp.Session, registry: *CDP const args = try parseArgs(Params, arena, arguments); const resolved = try resolveTarget(session, registry, args.selector, args.backendNodeId); - lp.actions.setChecked(resolved.node, args.checked, resolved.page) catch |err| { - if (err == error.InvalidNodeType) return ToolError.InvalidParams; - return ToolError.InternalError; - }; + lp.actions.setChecked(resolved.node, args.checked, resolved.page) catch |err| return mapActionError(err); const state_str: []const u8 = if (args.checked) "checked" else "unchecked"; const suffix = std.fmt.allocPrint(arena, " to {s}", .{state_str}) catch return ToolError.InternalError; @@ -999,39 +991,36 @@ pub fn substituteEnvVars(arena: std.mem.Allocator, input: []const u8) []const u8 var result: std.ArrayList(u8) = .empty; var i: usize = 0; - while (i < input.len) { - if (input[i] == '$') { - const var_start = i + 1; - var var_end = var_start; - while (var_end < input.len and (std.ascii.isAlphanumeric(input[var_end]) or input[var_end] == '_')) { - var_end += 1; - } - if (var_end > var_start) { - const name = input[var_start..var_end]; - // Same gate as `execGetEnv`: only `LP_*` is resolvable. A - // prompt-injected `fill('$ANTHROPIC_API_KEY')` would otherwise - // leak the resolved value into the page DOM. - var name_buf: [256]u8 = undefined; - const env_val: ?[:0]const u8 = if (std.ascii.startsWithIgnoreCase(name, "LP_") and name.len < name_buf.len) blk: { - @memcpy(name_buf[0..name.len], name); - name_buf[name.len] = 0; - break :blk std.posix.getenv(name_buf[0..name.len :0]); - } else null; - if (env_val) |val| { - result.appendSlice(arena, val) catch return input; - } else { - result.appendSlice(arena, input[i..var_end]) catch return input; - } - i = var_end; - } else { - result.append(arena, '$') catch return input; - i += 1; - } - } else { - result.append(arena, input[i]) catch return input; - i += 1; + var last_copy: usize = 0; + while (std.mem.indexOfScalarPos(u8, input, i, '$')) |dollar| { + const var_start = dollar + 1; + var var_end = var_start; + while (var_end < input.len and (std.ascii.isAlphanumeric(input[var_end]) or input[var_end] == '_')) { + var_end += 1; } + if (var_end == var_start) { + i = dollar + 1; + continue; + } + const name = input[var_start..var_end]; + // Same gate as `execGetEnv`: only `LP_*` is resolvable. A + // prompt-injected `fill('$ANTHROPIC_API_KEY')` would otherwise + // leak the resolved value into the page DOM. + var name_buf: [256]u8 = undefined; + const env_val: ?[:0]const u8 = if (std.ascii.startsWithIgnoreCase(name, "LP_") and name.len < name_buf.len) blk: { + @memcpy(name_buf[0..name.len], name); + name_buf[name.len] = 0; + break :blk std.posix.getenv(name_buf[0..name.len :0]); + } else null; + if (env_val) |val| { + result.appendSlice(arena, input[last_copy..dollar]) catch return input; + result.appendSlice(arena, val) catch return input; + last_copy = var_end; + } + i = var_end; } + if (last_copy == 0) return input; + result.appendSlice(arena, input[last_copy..]) catch return input; return result.toOwnedSlice(arena) catch input; }