From eb863e0b16148a95a4b2bae33315919dc5dbbd95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A0=20Arrufat?= Date: Thu, 14 May 2026 09:23:54 +0200 Subject: [PATCH] script: unify tool call conversion to use json values --- src/agent/Agent.zig | 4 +-- src/agent/CommandExecutor.zig | 6 ++-- src/mcp/tools.zig | 16 +++++----- src/script/Command.zig | 59 +++++++++++------------------------ 4 files changed, 31 insertions(+), 54 deletions(-) diff --git a/src/agent/Agent.zig b/src/agent/Agent.zig index d4341291..4d5adae1 100644 --- a/src/agent/Agent.zig +++ b/src/agent/Agent.zig @@ -796,7 +796,7 @@ fn runHealTurn(self: *Agent, arena: std.mem.Allocator, prompt: []const u8) ![]Co for (result.tool_calls_made) |tc| { if (tc.is_error) continue; const args = tc.arguments orelse continue; - const cmd = Command.fromToolCallValue(tc.name, args) orelse continue; + const cmd = Command.fromToolCall(tc.name, args) orelse continue; if (!isHealAllowed(cmd)) { self.terminal.printInfoFmt( "self-heal: ignoring {s} (navigation and eval are not allowed during heal)", @@ -953,7 +953,7 @@ fn processUserMessage(self: *Agent, input: TurnInput) !?[]const u8 { for (result.tool_calls_made) |tc| { if (tc.is_error) continue; const args = tc.arguments orelse continue; - const cmd = Command.fromToolCallValue(tc.name, args) orelse continue; + const cmd = Command.fromToolCall(tc.name, args) orelse continue; if (!recorded_any) { if (input.record_comment) |c| r.recordComment(c); recorded_any = true; diff --git a/src/agent/CommandExecutor.zig b/src/agent/CommandExecutor.zig index cc797ca8..c4d25ec4 100644 --- a/src/agent/CommandExecutor.zig +++ b/src/agent/CommandExecutor.zig @@ -46,13 +46,13 @@ pub const ExecResult = struct { pub fn executeWithResult(self: *Self, arena: std.mem.Allocator, cmd: Command.Command) ExecResult { if (cmd == .extract) return self.execExtract(arena, cmd.extract); - const tcv = (Command.toToolCallValue(arena, cmd, browser_tools.substituteEnvVars) catch + const tc = (Command.toToolCall(arena, cmd, browser_tools.substituteEnvVars) catch return .{ .output = "out of memory", .failed = true }) orelse return .{ .output = "internal: command has no tool mapping", .failed = true }; - if (self.tool_executor.callValue(arena, tcv.name, tcv.args)) |output| + if (self.tool_executor.callValue(arena, tc.name, tc.args)) |output| return .{ .output = output, .failed = false } else |err| - return .{ .output = std.fmt.allocPrint(arena, "{s} failed: {s}", .{ tcv.name, @errorName(err) }) catch "tool failed", .failed = true }; + return .{ .output = std.fmt.allocPrint(arena, "{s} failed: {s}", .{ tc.name, @errorName(err) }) catch "tool failed", .failed = true }; } /// Data-producing commands (EXTRACT/EVAL/MARKDOWN/TREE) go to stdout so shell diff --git a/src/mcp/tools.zig b/src/mcp/tools.zig index 6f5d7da2..89524ddb 100644 --- a/src/mcp/tools.zig +++ b/src/mcp/tools.zig @@ -191,7 +191,7 @@ fn dispatchBrowserTool( fn recordIfActive(server: *Server, name: []const u8, arguments: ?std.json.Value) void { if (server.recorder == null) return; const args_value = arguments orelse return; - const cmd = Command.fromToolCallValue(name, args_value) orelse return; + const cmd = Command.fromToolCall(name, args_value) orelse return; server.recorder.?.record(cmd); } @@ -276,21 +276,21 @@ fn handleScriptStep(server: *Server, arena: std.mem.Allocator, id: std.json.Valu // Map the Command to its underlying browser tool and dispatch through // the same path as a direct MCP call. Recording is intentionally NOT // applied to script_step lines: replay shouldn't double-record. - const tcv = (try Command.toToolCallValue(arena, cmd, Command.noSubstitute)) orelse { + const tc = (try Command.toToolCall(arena, cmd, Command.noSubstitute)) orelse { return sendErrorContent(server, id, "command has no browser-tool mapping"); }; - const action = std.meta.stringToEnum(browser_tools.Action, tcv.name) orelse { - return sendErrorContent(server, id, "internal: unknown action from Command.toToolCallValue"); + const action = std.meta.stringToEnum(browser_tools.Action, tc.name) orelse { + return sendErrorContent(server, id, "internal: unknown action from Command.toToolCall"); }; if (action == .eval) { - return sendEvalOutcome(server, id, browser_tools.callEval(arena, server.session, &server.node_registry, tcv.args)); + return sendEvalOutcome(server, id, browser_tools.callEval(arena, server.session, &server.node_registry, tc.args)); } - const result = browser_tools.call(arena, server.session, &server.node_registry, tcv.name, tcv.args) catch |err| { + const result = browser_tools.call(arena, server.session, &server.node_registry, tc.name, tc.args) catch |err| { const url = browser_tools.currentUrlOrPlaceholder(server.session); - const msg = std.fmt.allocPrint(arena, "{s} failed at line `{s}` (url: {s}): {s}", .{ tcv.name, args.line, url, @errorName(err) }) catch @errorName(err); + const msg = std.fmt.allocPrint(arena, "{s} failed at line `{s}` (url: {s}): {s}", .{ tc.name, args.line, url, @errorName(err) }) catch @errorName(err); return sendErrorContent(server, id, msg); }; @@ -301,7 +301,7 @@ fn handleScriptStep(server: *Server, arena: std.mem.Allocator, id: std.json.Valu if (verification.result == .failed) { const url = browser_tools.currentUrlOrPlaceholder(server.session); const reason = verification.reason orelse "verification failed"; - const msg = std.fmt.allocPrint(arena, "{s} executed at line `{s}` but verification failed (url: {s}): {s}", .{ tcv.name, args.line, url, reason }) catch reason; + const msg = std.fmt.allocPrint(arena, "{s} executed at line `{s}` but verification failed (url: {s}): {s}", .{ tc.name, args.line, url, reason }) catch reason; return sendErrorContent(server, id, msg); } diff --git a/src/script/Command.zig b/src/script/Command.zig index 947b3bc9..88d28bda 100644 --- a/src/script/Command.zig +++ b/src/script/Command.zig @@ -545,13 +545,6 @@ fn quote(s: []const u8) Quoted { return .{ .s = s }; } -/// A serialized LLM tool call: tool name plus JSON arguments. -/// `args_json` is empty for no-arg tools (e.g. tree, markdown). -pub const ToolCall = struct { - name: []const u8, - args_json: []const u8, -}; - /// Callback for resolving placeholder strings (typically `$LP_*` env vars) /// inside selector-like fields before serialization. Pass `noSubstitute` /// when raw output is desired (e.g. in tests). @@ -561,10 +554,9 @@ pub fn noSubstitute(_: std.mem.Allocator, input: []const u8) std.mem.Allocator.E return input; } -/// Same shape as `ToolCall` but the args are an already-built `std.json.Value`, -/// so callers can hand them straight to `lp.tools.call` without a stringify/ -/// reparse round-trip on the hot replay path. -pub const ToolCallValue = struct { +/// A tool call: the action name plus its already-built JSON arguments, +/// ready to hand to `lp.tools.call` without a stringify/reparse round-trip. +pub const ToolCall = struct { name: []const u8, args: ?std.json.Value, }; @@ -578,7 +570,7 @@ pub const ToolCallValue = struct { /// `type_cmd` is intentionally NOT substituted: `execFill` in /// `browser/tools.zig` substitutes it itself so the secret never appears /// in the result text echoed back to the LLM/terminal. -pub fn toToolCallValue(arena: std.mem.Allocator, cmd: Command, substitute: SubstituteFn) std.mem.Allocator.Error!?ToolCallValue { +pub fn toToolCall(arena: std.mem.Allocator, cmd: Command, substitute: SubstituteFn) std.mem.Allocator.Error!?ToolCall { const Action = lp.tools.Action; var obj: std.json.ObjectMap = .init(arena); switch (cmd) { @@ -628,29 +620,10 @@ pub fn toToolCallValue(arena: std.mem.Allocator, cmd: Command, substitute: Subst } } -/// Stringified flavor of `toToolCallValue` — used by the recorder/diagnostic -/// paths (and tests) that want a JSON string. Hot dispatch should use -/// `toToolCallValue` instead and skip the stringify+reparse. -pub fn toToolCall(arena: std.mem.Allocator, cmd: Command, substitute: SubstituteFn) std.mem.Allocator.Error!?ToolCall { - const tcv = (try toToolCallValue(arena, cmd, substitute)) orelse return null; - return .{ - .name = tcv.name, - .args_json = if (tcv.args) |v| try std.json.Stringify.valueAlloc(arena, v, .{}) else "", - }; -} - -/// Inverse of `toToolCall`: parse an LLM tool call into a Command, or -/// return null if the tool name doesn't correspond to a PandaScript -/// command. Variants emitted by `toToolCall` round-trip through this. -pub fn fromToolCall(arena: std.mem.Allocator, tool_name: []const u8, arguments: []const u8) ?Command { - const parsed = std.json.parseFromSliceLeaky(std.json.Value, arena, arguments, .{}) catch return null; - return fromToolCallValue(tool_name, parsed); -} - -/// Like `fromToolCall` but takes the already-parsed JSON value directly, -/// skipping the string round-trip when the caller already has it (e.g. the -/// MCP server, which dispatches off `std.json.Value`). -pub fn fromToolCallValue(tool_name: []const u8, arguments: std.json.Value) ?Command { +/// Inverse of `toToolCall`: map an LLM tool call into a Command, or return +/// null if the tool name doesn't correspond to a PandaScript command. +/// Variants emitted by `toToolCall` round-trip through this. +pub fn fromToolCall(tool_name: []const u8, arguments: std.json.Value) ?Command { const Action = lp.tools.Action; const action = std.meta.stringToEnum(Action, tool_name) orelse return null; const obj = switch (arguments) { @@ -1296,8 +1269,8 @@ fn expectRoundTrip(cmd: Command) !void { defer arena.deinit(); const a = arena.allocator(); const tc = (try toToolCall(a, cmd, noSubstitute)) orelse return error.NoToolMapping; - const back = fromToolCall(a, tc.name, if (tc.args_json.len == 0) "{}" else tc.args_json) orelse - return error.RoundTripFailed; + const args = tc.args orelse return error.RoundTripFailed; + const back = fromToolCall(tc.name, args) orelse return error.RoundTripFailed; try std.testing.expectEqualDeep(cmd, back); } @@ -1334,13 +1307,15 @@ test "toToolCall: variants without tool mapping return null" { test "fromToolCall: unknown tool returns null" { var arena: std.heap.ArenaAllocator = .init(std.testing.allocator); defer arena.deinit(); - try std.testing.expect(fromToolCall(arena.allocator(), "no_such_tool", "{}") == null); + const empty = try std.json.parseFromSliceLeaky(std.json.Value, arena.allocator(), "{}", .{}); + try std.testing.expect(fromToolCall("no_such_tool", empty) == null); } test "fromToolCall: missing required field returns null" { var arena: std.heap.ArenaAllocator = .init(std.testing.allocator); defer arena.deinit(); - try std.testing.expect(fromToolCall(arena.allocator(), "click", "{}") == null); + const empty = try std.json.parseFromSliceLeaky(std.json.Value, arena.allocator(), "{}", .{}); + try std.testing.expect(fromToolCall("click", empty) == null); } test "toToolCall: substitute callback applied to selector fields" { @@ -1350,7 +1325,8 @@ test "toToolCall: substitute callback applied to selector fields" { const tc = (try toToolCall(a, .{ .click = "abc" }, testUpcase)).?; try std.testing.expectEqualStrings("click", tc.name); - try std.testing.expectEqualStrings("{\"selector\":\"ABC\"}", tc.args_json); + const args_json = try std.json.Stringify.valueAlloc(a, tc.args.?, .{}); + try std.testing.expectEqualStrings("{\"selector\":\"ABC\"}", args_json); } test "toToolCall: type_cmd value is NOT substituted" { @@ -1361,7 +1337,8 @@ test "toToolCall: type_cmd value is NOT substituted" { const tc = (try toToolCall(a, .{ .type_cmd = .{ .selector = "abc", .value = "$LP_PASSWORD" } }, testUpcase)).?; try std.testing.expectEqualStrings("fill", tc.name); // selector substituted, value preserved as $LP_* reference - try std.testing.expectEqualStrings("{\"selector\":\"ABC\",\"value\":\"$LP_PASSWORD\"}", tc.args_json); + const args_json = try std.json.Stringify.valueAlloc(a, tc.args.?, .{}); + try std.testing.expectEqualStrings("{\"selector\":\"ABC\",\"value\":\"$LP_PASSWORD\"}", args_json); } fn expectBody(body: []const u8, complete_args: usize, at_boundary: bool) !void {