From 349b4ea79874413957d866ec7bcfa82dff8bdbc5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A0=20Arrufat?= Date: Thu, 21 May 2026 21:41:28 +0200 Subject: [PATCH] agent: clean up dead code and optimize schema parsing - Remove unused `callEval` and `extract` from `ToolExecutor`. - Optimize `parseValue` in `schema.zig` to allocate directly instead of using `std.ArrayList` when filling missing defaults. - Simplify redundant checks and clean up comments. --- src/agent/Agent.zig | 7 +----- src/agent/ToolExecutor.zig | 11 ---------- src/browser/tools.zig | 5 ++--- src/script/command.zig | 19 +++++----------- src/script/schema.zig | 45 ++++++++++++++++++++++---------------- 5 files changed, 35 insertions(+), 52 deletions(-) diff --git a/src/agent/Agent.zig b/src/agent/Agent.zig index 80305165..90cc89d8 100644 --- a/src/agent/Agent.zig +++ b/src/agent/Agent.zig @@ -411,10 +411,7 @@ fn runRepl(self: *Agent) void { if (line.len == 0) continue; - // Meta slash commands (/help, /quit, /verbosity) aren't part of - // PandaScript — they're REPL-only and never recorded. Intercept - // before Command.parse so they don't surface as UnknownTool. - if (line.len > 0 and line[0] == '/') { + if (line[0] == '/') { const split = SlashCommand.splitNameRest(line[1..]) orelse continue :repl; if (SlashCommand.findMeta(split.name) != null) { if (self.handleMeta(split.name, split.rest)) break :repl; @@ -453,8 +450,6 @@ fn runRepl(self: *Agent) void { _ = self.runTurn(.{ .prompt = prompt, .record_comment = line, .label = label }); }, .tool_call => |tc| { - // We just parsed `line` as `.tool_call` — it started with `/`, - // so the name+rest split is guaranteed to succeed. const split = SlashCommand.splitNameRest(line[1..]) orelse unreachable; self.terminal.beginTool(tc.name, split.rest); const result = self.cmd_runner.executeWithResult(aa, cmd); diff --git a/src/agent/ToolExecutor.zig b/src/agent/ToolExecutor.zig index ab2e6c7a..26eb37f8 100644 --- a/src/agent/ToolExecutor.zig +++ b/src/agent/ToolExecutor.zig @@ -96,13 +96,6 @@ pub fn getCurrentUrl(self: *ToolExecutor) []const u8 { return browser_tools.currentUrlOrPlaceholder(self.session); } -/// Run a JavaScript expression. Operational failures (OOM, missing page) -/// come back as `ToolError`; JS errors are returned in-band as -/// `ToolResult.is_error = true`. -pub fn callEval(self: *ToolExecutor, arena: std.mem.Allocator, script: []const u8) browser_tools.ToolError!browser_tools.ToolResult { - return browser_tools.evalScript(arena, self.session, &self.node_registry, script); -} - pub fn call(self: *ToolExecutor, arena: std.mem.Allocator, tool_name: []const u8, arguments_json: []const u8) CallError!browser_tools.ToolResult { const arguments: ?std.json.Value = if (arguments_json.len > 0) std.json.parseFromSliceLeaky(std.json.Value, arena, arguments_json, .{}) catch @@ -119,7 +112,3 @@ pub fn call(self: *ToolExecutor, arena: std.mem.Allocator, tool_name: []const u8 pub fn callValue(self: *ToolExecutor, arena: std.mem.Allocator, tool_name: []const u8, arguments: ?std.json.Value) browser_tools.ToolError!browser_tools.ToolResult { return browser_tools.call(arena, self.session, &self.node_registry, tool_name, arguments); } - -pub fn extract(self: *ToolExecutor, arena: std.mem.Allocator, schema_json: []const u8) browser_tools.ToolError!browser_tools.ToolResult { - return browser_tools.extract(arena, self.session, &self.node_registry, schema_json); -} diff --git a/src/browser/tools.zig b/src/browser/tools.zig index 9d42818b..09acc978 100644 --- a/src/browser/tools.zig +++ b/src/browser/tools.zig @@ -526,9 +526,8 @@ pub fn call( }; } -/// 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. +/// Run JavaScript against the current page. The script need not be +/// 0-terminated; a copy is made internally. pub fn evalScript( arena: std.mem.Allocator, session: *lp.Session, diff --git a/src/script/command.zig b/src/script/command.zig index ae6c0402..5f448378 100644 --- a/src/script/command.zig +++ b/src/script/command.zig @@ -106,8 +106,6 @@ pub const Command = union(enum) { const split = schema.splitNameRest(trimmed[1..]) orelse return error.MissingName; - // LLM-trigger meta commands. They live in the language (recordable) - // but execution happens in the REPL/runScript layer. if (std.ascii.eqlIgnoreCase(split.name, "login")) { if (split.rest.len > 0) return error.MalformedKv; return .{ .login = {} }; @@ -135,19 +133,15 @@ pub const Command = union(enum) { } } - /// Construct a Command for a tool call. Used by recording paths that - /// already have the `(name, args)` shape (MCP dispatch, LLM tool calls). - /// The `name` slice must live as long as the Command (typically a - /// `tool_defs`-owned slice, which is process-lifetime). Args borrow - /// from the caller — use `fromToolCallOwned` when the Command outlives - /// the args' arena. + /// `name` and `arguments` must outlive the returned Command. Use + /// `fromToolCallOwned` when that guarantee doesn't hold. pub fn fromToolCall(tool_name: []const u8, arguments: ?std.json.Value) Command { return .{ .tool_call = .{ .name = tool_name, .args = arguments } }; } - /// Same as `fromToolCall` but deep-copies `arguments` into `arena`. - /// Use when the Command must outlive the original args buffer (e.g. the - /// self-heal path returns Commands across an arena deinit). + /// Deep-copies `arguments` into `arena` so the Command can outlive the + /// caller's args buffer (e.g. the self-heal path returns Commands across + /// an arena deinit). pub fn fromToolCallOwned(arena: std.mem.Allocator, tool_name: []const u8, arguments: ?std.json.Value) std.mem.Allocator.Error!Command { const owned_name = if (schema.findSchemaCanonical(schema.globalSchemas(), tool_name)) |s| s.tool_name else try arena.dupe(u8, tool_name); const owned_args = if (arguments) |v| try dupeJsonValue(arena, v) else null; @@ -293,8 +287,7 @@ fn dupeJsonValue(a: std.mem.Allocator, value: std.json.Value) std.mem.Allocator. // --- Formatting --- fn formatToolCall(tc: Command.ToolCall, writer: *std.Io.Writer) std.Io.Writer.Error!void { - const schemas = schema.globalSchemas(); - const s_opt = schema.findSchema(schemas, tc.name); + const s_opt = schema.findSchemaCanonical(schema.globalSchemas(), tc.name); try writer.writeByte('/'); try writer.writeAll(tc.name); diff --git a/src/script/schema.zig b/src/script/schema.zig index 1dbf6a2f..a0e240c2 100644 --- a/src/script/schema.zig +++ b/src/script/schema.zig @@ -275,28 +275,38 @@ pub fn parseValue(arena: std.mem.Allocator, schema: *const SchemaInfo, rest: []c // Default-true required booleans (e.g. setChecked.checked) are filled in // when omitted, so `/setChecked selector='#a'` works without `checked=true`. - var with_defaults: std.ArrayList(KvPair) = .empty; - try with_defaults.ensureTotalCapacity(arena, pairs.len + schema.required.len); - with_defaults.appendSliceAssumeCapacity(pairs); - + var missing_defaults: usize = 0; for (schema.required) |req| { var found = false; - for (with_defaults.items) |p| if (std.mem.eql(u8, p.key, req)) { + for (pairs) |p| if (std.mem.eql(u8, p.key, req)) { found = true; break; }; if (found) continue; - for (schema.fields) |f| { - if (std.mem.eql(u8, f.name, req) and f.default_true) { - with_defaults.appendAssumeCapacity(.{ .key = req, .value = "true" }); - found = true; - break; - } - } - if (!found) return error.MissingRequired; + const has_default = blk: for (schema.fields) |f| { + if (std.mem.eql(u8, f.name, req) and f.default_true) break :blk true; + } else false; + if (!has_default) return error.MissingRequired; + missing_defaults += 1; } - return try buildValue(arena, schema, with_defaults.items); + if (missing_defaults == 0) return try buildValue(arena, schema, pairs); + + const with_defaults = try arena.alloc(KvPair, pairs.len + missing_defaults); + @memcpy(with_defaults[0..pairs.len], pairs); + var next = pairs.len; + for (schema.required) |req| { + var found = false; + for (pairs) |p| if (std.mem.eql(u8, p.key, req)) { + found = true; + break; + }; + if (found) continue; + with_defaults[next] = .{ .key = req, .value = "true" }; + next += 1; + } + + return try buildValue(arena, schema, with_defaults); } const KvPair = struct { @@ -384,11 +394,8 @@ fn coerce(arena: std.mem.Allocator, schema: *const SchemaInfo, key: []const u8, // --- Global lazy schema cache --- // -// `Command.parse` and `Command.format` need schemas to decide between -// positional and kv form, but most callers (recorder, MCP, tests) don't have -// an agent-scoped cache to thread through. Lazily build a process-wide cache -// here so those paths don't have to. Single-threaded REPL only — if -// multi-threaded usage emerges, swap the guard for `std.Once` semantics. +// Single-threaded REPL only — if multi-threaded usage emerges, swap the guard +// for `std.Once` semantics. var global_failed: bool = false; var global_schemas_storage: [browser_tools.tool_defs.len]SchemaInfo = undefined;