diff --git a/src/agent/Agent.zig b/src/agent/Agent.zig index d77433a83..fb30f414e 100644 --- a/src/agent/Agent.zig +++ b/src/agent/Agent.zig @@ -1494,7 +1494,7 @@ fn runCommand(self: *Agent, arena: std.mem.Allocator, cmd: Command) browser_tool .text = switch (err) { error.OutOfMemory => "out of memory", error.FrameNotLoaded => "no page loaded — run /goto first", - else => std.fmt.allocPrint(arena, "{s} failed: {s}", .{ tc.name(), @errorName(err) }) catch "tool failed", + else => std.fmt.allocPrint(arena, "{s} failed: {s}", .{ tc.name(), browser_tools.errorMessage(err) }) catch "tool failed", }, .is_error = true, }; @@ -1927,7 +1927,7 @@ fn handleToolCall(ctx: *anyopaque, allocator: std.mem.Allocator, tool_name: []co const outcome: zenai.provider.Client.ToolHandler.Result = if (browser_tools.call(allocator, self.session, &self.node_registry, tool_name, arguments)) |result| .{ .content = capToolOutput(allocator, tool_name, result.text), .is_error = result.is_error } else |err| - .{ .content = std.fmt.allocPrint(allocator, "Error: {s}", .{@errorName(err)}) catch "Error: tool execution failed", .is_error = true }; + .{ .content = std.fmt.allocPrint(allocator, "Error: {s}", .{browser_tools.errorMessage(err)}) catch "Error: tool execution failed", .is_error = true }; self.terminal.agentToolDone(tool_name, args_str, !outcome.is_error); if (self.terminal.verbosity == .high) self.terminal.printToolOutcome(tool_name, outcome.content, outcome.is_error); diff --git a/src/browser/tools.zig b/src/browser/tools.zig index 903d268b7..a3b989796 100644 --- a/src/browser/tools.zig +++ b/src/browser/tools.zig @@ -370,7 +370,7 @@ pub const Tool = enum { \\ "type": "object", \\ "properties": { \\ "selector": { "type": "string", "description": "Optional CSS selector. Render markdown for just that element's subtree." }, - \\ "backendNodeId": { "type": "integer", "description": "Optional backend node ID. Render markdown for just that node's subtree." }, + \\ "backendNodeId": { "type": "integer", "description": "Optional backend node ID. Render markdown for just that node's subtree. 0 is treated as omitted." }, \\ "maxBytes": { "type": "integer", "description": "Optional soft cap on output size in bytes. Content is truncated at a UTF-8 boundary and a short '[truncated]' marker is appended past the cap." }, \\ "url": { "type": "string", "description": "Optional URL to navigate to before rendering." }, \\ "timeout": { "type": "integer", "description": "Optional timeout in milliseconds. Defaults to 10000." } @@ -386,7 +386,7 @@ pub const Tool = enum { \\ "type": "object", \\ "properties": { \\ "selector": { "type": "string", "description": "Optional CSS selector. When set, dump only that element's outerHTML." }, - \\ "backendNodeId": { "type": "integer", "description": "Optional backend node ID. When set, dump only that node's outerHTML." }, + \\ "backendNodeId": { "type": "integer", "description": "Optional backend node ID. When set, dump only that node's outerHTML. 0 is treated as omitted." }, \\ "url": { "type": "string", "description": "Optional URL to navigate to before dumping." }, \\ "timeout": { "type": "integer", "description": "Optional timeout in milliseconds. Defaults to 10000." } \\ } @@ -454,7 +454,7 @@ pub const Tool = enum { \\ "properties": { \\ "url": { "type": "string", "description": "Optional URL to navigate to before fetching the semantic tree." }, \\ "timeout": { "type": "integer", "description": "Optional timeout in milliseconds. Defaults to 10000." }, - \\ "backendNodeId": { "type": "integer", "description": "Optional backend node ID to get the tree for a specific element instead of the document root." }, + \\ "backendNodeId": { "type": "integer", "description": "Optional backend node ID to get the tree for a specific element instead of the document root. 0 is treated as omitted." }, \\ "maxDepth": { "type": "integer", "description": "Optional maximum depth of the tree to return. Useful for exploring high-level structure first." } \\ } \\} @@ -523,7 +523,7 @@ pub const Tool = enum { \\{ \\ "type": "object", \\ "properties": { - \\ "backendNodeId": { "type": "integer", "description": "Optional: The backend node ID of the element to scroll. If omitted, scrolls the window." }, + \\ "backendNodeId": { "type": "integer", "description": "Optional: The backend node ID of the element to scroll. If omitted (or 0), scrolls the window." }, \\ "x": { "type": "integer", "description": "Optional: The horizontal scroll offset." }, \\ "y": { "type": "integer", "description": "Optional: The vertical scroll offset." } \\ } @@ -596,7 +596,7 @@ pub const Tool = enum { \\ "properties": { \\ "key": { "type": "string", "description": "The key to press (e.g. 'Enter', 'Tab', 'a')." }, \\ "selector": { "type": "string", "description": "Optional CSS selector of the element to target. Preferred over backendNodeId." }, - \\ "backendNodeId": { "type": "integer", "description": "Optional backend node ID of the element to target. Defaults to the document when neither selector nor backendNodeId is provided." } + \\ "backendNodeId": { "type": "integer", "description": "Optional backend node ID of the element to target. Defaults to the document when neither selector nor backendNodeId is provided; 0 is treated as omitted." } \\ }, \\ "required": ["key"] \\} @@ -757,6 +757,16 @@ pub const ToolError = error{ OutOfMemory, }; +/// LLM-facing message for a tool failure. Bare error names leave the model +/// retrying blind; spell out the recovery for errors it can act on. +pub fn errorMessage(err: ToolError) []const u8 { + return switch (err) { + error.NodeNotFound => "NodeNotFound: the selector or backendNodeId matched nothing on the current page. Re-inspect the page (tree/interactiveElements) for fresh node ids, or omit backendNodeId to target the document root.", + error.FrameNotLoaded => "FrameNotLoaded: no page is loaded — call goto (or pass a url) first.", + else => @errorName(err), + }; +} + /// Outcome of running a tool against the page. Operational failures (OOM, /// missing page, invalid params) come out as Zig errors on the enclosing /// `!ToolResult`; `is_error = true` is the in-band signal for a JS-level @@ -2076,13 +2086,23 @@ fn formatEnumError(arena: std.mem.Allocator, field: []const u8, got: []const u8, } pub fn parseValue(comptime T: type, arena: std.mem.Allocator, value: std.json.Value) ParseArgsError!T { - return std.json.parseFromValueLeaky(T, arena, value, .{ .ignore_unknown_fields = true }) catch |err| switch (err) { - error.OutOfMemory => error.OutOfMemory, + var parsed = std.json.parseFromValueLeaky(T, arena, value, .{ .ignore_unknown_fields = true }) catch |err| switch (err) { + error.OutOfMemory => return error.OutOfMemory, else => { log.debug(.browser, "parseValue rejected", .{ .err = @errorName(err), .type = @typeName(T) }); return error.InvalidParams; }, }; + // Schema contract: backendNodeId 0 means omitted — registry ids start at 1, + // and zero-filling models (gpt-5.x) send 0 for "unset". + if (comptime @typeInfo(T) == .@"struct" and @hasField(T, "backendNodeId") and + @typeInfo(@FieldType(T, "backendNodeId")) == .optional) + { + if (parsed.backendNodeId) |nid| { + if (nid == 0) parsed.backendNodeId = null; + } + } + return parsed; } /// For tools where every field is optional. Missing args → default `T`; @@ -2242,6 +2262,35 @@ test "call: unknown tool name surfaces in-band" { try std.testing.expectEqualStrings("Unknown tool: multi_tool_use.parallel", r.text); } +test "parseValue: zero-filled optional backendNodeId treated as omitted" { + var arena: std.heap.ArenaAllocator = .init(std.testing.allocator); + defer arena.deinit(); + const aa = arena.allocator(); + + const Params = struct { + backendNodeId: ?CDPNode.Id = null, + maxDepth: ?u32 = null, + }; + const zeroed = try std.json.parseFromSliceLeaky(std.json.Value, aa, + \\{"backendNodeId":0,"maxDepth":2} + , .{}); + const args = try parseValue(Params, aa, zeroed); + try std.testing.expectEqual(@as(?CDPNode.Id, null), args.backendNodeId); + try std.testing.expectEqual(@as(?u32, 2), args.maxDepth); + + const real = try std.json.parseFromSliceLeaky(std.json.Value, aa, + \\{"backendNodeId":7} + , .{}); + try std.testing.expectEqual(@as(?CDPNode.Id, 7), (try parseValue(Params, aa, real)).backendNodeId); + + // Non-optional ids (nodeDetails) pass through untouched. + const Required = struct { backendNodeId: CDPNode.Id }; + const zero_required = try std.json.parseFromSliceLeaky(std.json.Value, aa, + \\{"backendNodeId":0} + , .{}); + try std.testing.expectEqual(@as(CDPNode.Id, 0), (try parseValue(Required, aa, zero_required)).backendNodeId); +} + test "substituteEnvVars resolves LP_* vars" { var arena: std.heap.ArenaAllocator = .init(std.testing.allocator); defer arena.deinit(); diff --git a/src/mcp/tools.zig b/src/mcp/tools.zig index 5b815077c..47ff1c746 100644 --- a/src/mcp/tools.zig +++ b/src/mcp/tools.zig @@ -139,7 +139,7 @@ fn dispatchBrowserTool( error.Timeout => .Timeout, error.NavigationFailed, error.InternalError, error.OutOfMemory => .InternalError, }; - return server.sendError(id, code, @errorName(err)); + return server.sendError(id, code, browser_tools.errorMessage(err)); }; try sendToolResultText(server, id, result.text, result.is_error); @@ -930,6 +930,34 @@ test "MCP - tree rejects stale backendNodeId instead of dumping whole document" try testing.expect(std.mem.indexOf(u8, written, "NodeNotFound") != null); } +test "MCP - tree treats zero-filled backendNodeId as omitted" { + var out: std.Io.Writer.Allocating = .init(testing.arena_allocator); + const server = try testLoadPage("http://localhost:9582/src/browser/tests/mcp_actions.html", &out.writer); + defer server.deinit(); + + const msg = + \\{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"tree","arguments":{"backendNodeId":0,"maxDepth":3}}} + ; + try router.handleMessage(server, testing.arena_allocator, msg); + const written = out.written(); + try testing.expect(std.mem.indexOf(u8, written, "NodeNotFound") == null); + try testing.expect(std.mem.indexOf(u8, written, "\"isError\":true") == null); +} + +test "MCP - stale backendNodeId surfaces recovery guidance" { + var out: std.Io.Writer.Allocating = .init(testing.arena_allocator); + const server = try testLoadPage("http://localhost:9582/src/browser/tests/mcp_actions.html", &out.writer); + defer server.deinit(); + + const msg = + \\{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"tree","arguments":{"backendNodeId":999999}}} + ; + try router.handleMessage(server, testing.arena_allocator, msg); + const written = out.written(); + try testing.expect(std.mem.indexOf(u8, written, "NodeNotFound") != null); + try testing.expect(std.mem.indexOf(u8, written, "omit backendNodeId") != null); +} + test "MCP - PascalCase argument keys from LLMs are normalized to canonical" { var out: std.Io.Writer.Allocating = .init(testing.arena_allocator); const server = try testLoadPage("http://localhost:9582/src/browser/tests/mcp_actions.html", &out.writer); @@ -1268,7 +1296,7 @@ test "MCP - waitForSelector: timeout" { try router.handleMessage(server, testing.arena_allocator, msg); try testing.expectJson(.{ .id = 1, - .@"error" = .{ .message = "NodeNotFound" }, + .@"error" = .{ .message = browser_tools.errorMessage(error.NodeNotFound) }, }, out.written()); }