From 4bc5baf7eef6cbcc906d2408173b0fe48bfe1d7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A0=20Arrufat?= Date: Mon, 25 May 2026 17:51:46 +0200 Subject: [PATCH] agent: improve command diagnostics and fix various bugs - Add diagnostics to command parsing to report invalid field values. - Fix UTF-8 truncation in spinner to avoid splitting codepoints. - Fix swapped arguments in log error formatting. - Distinguish "null" values from missing elements in verifier. - Handle MCP tool cancellation and timeouts with specific error codes. --- src/agent/Agent.zig | 14 +++++++++++--- src/agent/Spinner.zig | 18 ++++++++++++++++-- src/browser/Session.zig | 9 +++++++-- src/browser/actions.zig | 2 +- src/browser/tools.zig | 5 ++++- src/lightpanda.zig | 9 +++++++++ src/log.zig | 2 +- src/mcp/protocol.zig | 7 +++++++ src/mcp/tools.zig | 12 +++++++++--- src/script/Schema.zig | 40 +++++++++++++++++++++++++++++++++------- src/script/Verifier.zig | 14 ++++++++++---- src/script/command.zig | 7 ++++++- 12 files changed, 114 insertions(+), 25 deletions(-) diff --git a/src/agent/Agent.zig b/src/agent/Agent.zig index e91e22fa..bb12c643 100644 --- a/src/agent/Agent.zig +++ b/src/agent/Agent.zig @@ -468,7 +468,8 @@ fn runRepl(self: *Agent) void { } } - const cmd = Command.parse(aa, line) catch |err| switch (err) { + var diag: Schema.Diag = .{}; + const cmd = Command.parseDiag(aa, line, &diag) catch |err| switch (err) { error.NotASlashCommand => { if (self.ai_client == null) { self.terminal.printError("Basic REPL (--no-llm) accepts only slash commands. Try /help, or drop --no-llm and set an API key (ANTHROPIC_API_KEY, OPENAI_API_KEY, or GOOGLE_API_KEY) to enable natural-language prompts.", .{}); @@ -479,7 +480,7 @@ fn runRepl(self: *Agent) void { }, else => |e| { const name = if (slash_split) |sp| sp.name else line; - self.printSlashParseError(e, name); + self.printSlashParseError(e, name, &diag); continue :repl; }, }; @@ -591,7 +592,13 @@ fn printSlashHelp(self: *Agent, arena: std.mem.Allocator, target: []const u8) vo self.terminal.printInfo("schema:\n{s}", .{aw.written()}); } -fn printSlashParseError(self: *Agent, err: Schema.ParseError, name: []const u8) void { +fn printSlashParseError(self: *Agent, err: Schema.ParseError, name: []const u8, diag: ?*const Schema.Diag) void { + if (err == error.InvalidValue) { + if (diag) |d| if (d.bad_field.len > 0) { + self.terminal.printError("{s}: {s}: expected {s}, got '{s}'. Try /help {s}.", .{ name, d.bad_field, @tagName(d.expected_type), d.bad_value, name }); + return; + }; + } const reason: []const u8 = switch (err) { error.UnknownTool => "unknown tool", error.MissingName => return self.terminal.printError("missing tool name. Try /help.", .{}), @@ -601,6 +608,7 @@ fn printSlashParseError(self: *Agent, err: Schema.ParseError, name: []const u8) error.PositionalNotAllowed => "positional only works for tools with one required field. Use key=value", error.UnterminatedQuote => "unterminated quote", error.UnsupportedEscape => "backslash escapes aren't supported in quoted values; use the other quote style or `'''…'''`", + error.InvalidValue => "invalid value (check argument type)", error.OutOfMemory => return self.terminal.printError("out of memory", .{}), }; self.terminal.printError("{s}: {s}. Try /help {s}.", .{ name, reason, name }); diff --git a/src/agent/Spinner.zig b/src/agent/Spinner.zig index 24d80df4..36612514 100644 --- a/src/agent/Spinner.zig +++ b/src/agent/Spinner.zig @@ -168,12 +168,12 @@ pub fn setTool(self: *Spinner, name: []const u8, args: []const u8) void { const manual = self.state == .idle; self.tool_calls += 1; var tool: ToolState = .{ .set_ns = std.time.nanoTimestamp(), .manual = manual }; - tool.name_len = @min(name.len, tool.name_buf.len); + tool.name_len = utf8FloorTo(name, tool.name_buf.len); @memcpy(tool.name_buf[0..tool.name_len], name[0..tool.name_len]); // Strip control chars: a literal `\n` in args (e.g. /eval """…""" bodies) // breaks the spinner's `\r`-based redraw — the cursor only rewinds to the // start of the last line, leaving prior frames stuck on screen. - tool.args_len = @min(args.len, tool.args_buf.len); + tool.args_len = utf8FloorTo(args, tool.args_buf.len); for (args[0..tool.args_len], 0..) |ch, i| { tool.args_buf[i] = if (ch < 0x20 or ch == 0x7f) ' ' else ch; } @@ -287,6 +287,20 @@ fn renderLocked(self: *Spinner) void { _ = std.posix.write(std.posix.STDERR_FILENO, written) catch {}; } +/// Largest prefix length of `bytes` that fits in `max_bytes` and ends on +/// a UTF-8 codepoint boundary. Invalid sequences are treated as one byte +/// each so the function never loops. +fn utf8FloorTo(bytes: []const u8, max_bytes: usize) usize { + if (bytes.len <= max_bytes) return bytes.len; + var i: usize = 0; + while (i < max_bytes) { + const seq_len = std.unicode.utf8ByteSequenceLength(bytes[i]) catch 1; + if (i + seq_len > max_bytes) break; + i += seq_len; + } + return i; +} + /// Returns the byte length of `bytes` that fits in `max_cells` cells, /// rounded down to a whole UTF-8 codepoint. Multi-cell glyphs (CJK, /// wide emoji) are counted as 1 — args are typically ASCII so the diff --git a/src/browser/Session.zig b/src/browser/Session.zig index 2a3a4c94..720b4636 100644 --- a/src/browser/Session.zig +++ b/src/browser/Session.zig @@ -419,13 +419,18 @@ pub fn processQueuedNavigation(self: *Session) !void { navigations.clearRetainingCapacity(); // Second pass: process synchronous navigations (about:blank) - // These may trigger new navigations which go into queued_navigation + // These may trigger new navigations which go into queued_navigation. + // Mirror the first pass: a failure on one frame must not orphan the + // rest of the queue (the `defer clearRetainingCapacity` would wipe + // siblings whose _queued_navigation stays set). for (about_blank_queue.items) |frame| { const qn = frame._queued_navigation orelse { log.debug(.frame, "skipped null queued nav", .{}); continue; }; - try self.processFrameNavigation(frame, qn); + self.processFrameNavigation(frame, qn) catch |err| { + log.warn(.frame, "frame navigation", .{ .url = qn.url, .err = err }); + }; } // Safety: Remove any about:blank navigations that were queued during diff --git a/src/browser/actions.zig b/src/browser/actions.zig index d646144d..98ab368f 100644 --- a/src/browser/actions.zig +++ b/src/browser/actions.zig @@ -163,7 +163,7 @@ fn implicitFormSubmit(el: *Element, frame: *Frame) !void { return form.requestSubmit(submitter, frame); } if (el.is(Button)) |button| { - if (!std.mem.eql(u8, button.getType(), "submit")) return; + if (!std.ascii.eqlIgnoreCase(button.getType(), "submit")) return; const form = button.getForm(frame) orelse return; return form.requestSubmit(el, frame); } diff --git a/src/browser/tools.zig b/src/browser/tools.zig index e32270e2..fb68759e 100644 --- a/src/browser/tools.zig +++ b/src/browser/tools.zig @@ -1360,7 +1360,10 @@ fn resolveNodeAndPage(session: *lp.Session, registry: *CDPNode.Registry, node_id fn resolveBySelector(session: *lp.Session, selector: []const u8) ToolError!NodeAndPage { const page = try requireFrame(session); - const element = Selector.querySelector(page.document.asNode(), selector, page) catch return ToolError.InvalidParams; + const element = Selector.querySelector(page.document.asNode(), selector, page) catch |err| switch (err) { + error.OutOfMemory => return ToolError.InternalError, + else => return ToolError.InvalidParams, + }; const node = (element orelse return ToolError.NodeNotFound).asNode(); return .{ .node = node, .page = page, .target = .{ .selector = selector } }; } diff --git a/src/lightpanda.zig b/src/lightpanda.zig index 1839faf0..f8d839b2 100644 --- a/src/lightpanda.zig +++ b/src/lightpanda.zig @@ -65,6 +65,15 @@ pub const FetchOpts = struct { writer: ?*std.Io.Writer = null, json: bool = false, }; +/// Loads `url` in a fresh session and waits per `opts`. +/// +/// Errors: +/// - `error.Timeout` if the wait deadline (`opts.wait_ms`) expires. +/// - `error.Cancelled` if the embedder installed a `Session.cancel_hook` +/// that returned true during the wait. The hook is opt-in via +/// `session.cancel_hook = .{...}`; without it, this error never fires. +/// - Other errors from navigation / parsing / I/O surface as their +/// underlying tag. pub fn fetch(app: *App, browser: *Browser, url: [:0]const u8, opts: FetchOpts) !void { const notification = try Notification.init(app.allocator); defer notification.deinit(); diff --git a/src/log.zig b/src/log.zig index 6d629f4d..2c11b332 100644 --- a/src/log.zig +++ b/src/log.zig @@ -125,7 +125,7 @@ pub fn log(comptime scope: Scope, level: Level, comptime msg: []const u8, data: var buf: [4096]u8 = undefined; var w: std.Io.Writer = .fixed(&buf); logTo(scope, level, msg, data, &w) catch |log_err| { - std.debug.print("$time={d} $level=fatal $scope={s} $msg=\"log err\" err={s} log_msg=\"{s}\"\n", .{ timestamp(.clock), @errorName(log_err), @tagName(scope), msg }); + std.debug.print("$time={d} $level=fatal $scope={s} $msg=\"log err\" err={s} log_msg=\"{s}\"\n", .{ timestamp(.clock), @tagName(scope), @errorName(log_err), msg }); return; }; s(w.buffered()); diff --git a/src/mcp/protocol.zig b/src/mcp/protocol.zig index 3153856d..a90f364a 100644 --- a/src/mcp/protocol.zig +++ b/src/mcp/protocol.zig @@ -37,6 +37,13 @@ pub const ErrorCode = enum(i64) { InternalError = -32603, FrameNotLoaded = -32604, NotFound = -32605, + /// Application-range code: tool call was aborted by the caller (SIGINT, + /// session shutdown). Distinct from InternalError so MCP clients don't + /// retry into a loop on intentional cancellation. + Cancelled = -32001, + /// Application-range code: tool call exceeded its deadline. Distinct + /// from Cancelled — Timeout is a tool-state outcome, not a caller signal. + Timeout = -32002, }; pub const Notification = struct { diff --git a/src/mcp/tools.zig b/src/mcp/tools.zig index 3d3b0e04..ad18335c 100644 --- a/src/mcp/tools.zig +++ b/src/mcp/tools.zig @@ -173,7 +173,9 @@ fn dispatchBrowserTool( const code: protocol.ErrorCode = switch (err) { error.FrameNotLoaded => .FrameNotLoaded, error.NodeNotFound, error.InvalidParams => .InvalidParams, - error.NavigationFailed, error.Cancelled, error.Timeout, error.InternalError, error.OutOfMemory => .InternalError, + error.Cancelled => .Cancelled, + error.Timeout => .Timeout, + error.NavigationFailed, error.InternalError, error.OutOfMemory => .InternalError, }; return server.sendError(id, code, @errorName(err)); }; @@ -256,8 +258,12 @@ fn handleScriptStep(server: *Server, arena: std.mem.Allocator, id: std.json.Valu return server.sendError(id, .InvalidParams, "expected { line: string }"); }; - const cmd = Command.parse(arena, args.line) catch |err| { - const msg = std.fmt.allocPrint(arena, "could not parse step `{s}`: {s}", .{ args.line, @errorName(err) }) catch @errorName(err); + var diag: lp.script.Schema.Diag = .{}; + const cmd = Command.parseDiag(arena, args.line, &diag) catch |err| { + const msg = if (err == error.InvalidValue and diag.bad_field.len > 0) + std.fmt.allocPrint(arena, "could not parse step `{s}`: {s}: expected {s}, got '{s}'", .{ args.line, diag.bad_field, @tagName(diag.expected_type), diag.bad_value }) catch @errorName(err) + else + std.fmt.allocPrint(arena, "could not parse step `{s}`: {s}", .{ args.line, @errorName(err) }) catch @errorName(err); return sendErrorContent(server, id, msg); }; diff --git a/src/script/Schema.zig b/src/script/Schema.zig index 05c27b35..d1c14c5b 100644 --- a/src/script/Schema.zig +++ b/src/script/Schema.zig @@ -71,6 +71,7 @@ pub const ParseError = error{ PositionalNotAllowed, UnterminatedQuote, UnsupportedEscape, + InvalidValue, OutOfMemory, }; @@ -79,6 +80,14 @@ pub const Split = struct { rest: []const u8, }; +/// Populated by `parseValueDiag` on `InvalidValue` so callers can surface +/// the offending field/type/value to the user. +pub const Diag = struct { + bad_field: []const u8 = "", + expected_type: FieldType = .other, + bad_value: []const u8 = "", +}; + // --- Per-instance methods --- /// True when the tool can be addressed as `/ ''''''` — @@ -154,6 +163,14 @@ pub fn isSinglePositional(self: Schema, args: std.json.ObjectMap) bool { /// exactly one required. Otherwise positionals error. /// - Everything else is `key=value` with type coercion. pub fn parseValue(self: Schema, arena: std.mem.Allocator, rest: []const u8) ParseError!?std.json.Value { + return self.parseValueDiag(arena, rest, null); +} + +/// Same as `parseValue` but populates `diag` on `error.InvalidValue` so +/// callers can render a user-facing message like +/// "y: expected integer, got 'fast'". +pub fn parseValueDiag(self: Schema, arena: std.mem.Allocator, rest_raw: []const u8, diag: ?*Diag) ParseError!?std.json.Value { + const rest = std.mem.trim(u8, rest_raw, &std.ascii.whitespace); if (rest.len == 0) { if (self.required.len > 0) return error.MissingRequired; return null; @@ -196,7 +213,7 @@ pub fn parseValue(self: Schema, arena: std.mem.Allocator, rest: []const u8) Pars list.appendAssumeCapacity(.{ .key = req, .value = "true" }); } - return try self.buildValue(arena, list.items); + return try self.buildValue(arena, list.items, diag); } fn validateAndFillObject(self: Schema, obj: *std.json.ObjectMap) ParseError!void { @@ -218,27 +235,36 @@ const KvPair = struct { value: []const u8, }; -fn buildValue(self: Schema, arena: std.mem.Allocator, pairs: []const KvPair) error{OutOfMemory}!std.json.Value { +fn buildValue(self: Schema, arena: std.mem.Allocator, pairs: []const KvPair, diag: ?*Diag) ParseError!std.json.Value { var obj: std.json.ObjectMap = .init(arena); try obj.ensureTotalCapacity(pairs.len); for (pairs) |p| { - const v = try self.coerce(arena, p.key, p.value); + const v = try self.coerce(arena, p.key, p.value, diag); try obj.put(p.key, v); } return .{ .object = obj }; } -fn coerce(self: Schema, arena: std.mem.Allocator, key: []const u8, value: []const u8) error{OutOfMemory}!std.json.Value { - switch (self.fieldType(key)) { +fn coerce(self: Schema, arena: std.mem.Allocator, key: []const u8, value: []const u8, diag: ?*Diag) ParseError!std.json.Value { + const ft = self.fieldType(key); + switch (ft) { .integer => { - if (std.fmt.parseInt(i64, value, 10)) |n| return .{ .integer = n } else |_| {} + if (std.fmt.parseInt(i64, value, 10)) |n| return .{ .integer = n } else |_| { + if (diag) |d| d.* = .{ .bad_field = key, .expected_type = ft, .bad_value = value }; + return error.InvalidValue; + } }, .number => { - if (std.fmt.parseFloat(f64, value)) |n| return .{ .float = n } else |_| {} + if (std.fmt.parseFloat(f64, value)) |n| return .{ .float = n } else |_| { + if (diag) |d| d.* = .{ .bad_field = key, .expected_type = ft, .bad_value = value }; + return error.InvalidValue; + } }, .boolean => { if (std.mem.eql(u8, value, "true")) return .{ .bool = true }; if (std.mem.eql(u8, value, "false")) return .{ .bool = false }; + if (diag) |d| d.* = .{ .bad_field = key, .expected_type = ft, .bad_value = value }; + return error.InvalidValue; }, else => {}, } diff --git a/src/script/Verifier.zig b/src/script/Verifier.zig index 461d1b7f..83b24cdf 100644 --- a/src/script/Verifier.zig +++ b/src/script/Verifier.zig @@ -94,7 +94,7 @@ fn verifyFill(self: *Verifier, arena: std.mem.Allocator, selector: []const u8, e // verify the field isn't empty after substitution. if (std.mem.indexOf(u8, expected_value, "$LP_") != null) { const actual = self.queryElementProperty(arena, selector, .value) orelse return .inconclusive; - if (actual.len == 0 or std.mem.eql(u8, actual, "null")) + if (actual.len == 0) return .{ .failed = "element value is empty after fill (expected non-empty for secret)" }; return .passed; } @@ -125,13 +125,19 @@ fn verifyElementValue(self: *Verifier, arena: std.mem.Allocator, selector: []con return .passed; } +/// Returns the property value, or `null` when the element is missing or the +/// eval failed. A single-byte tag (`v` = present, `m` = missing) disambiguates +/// from values that happen to stringify to "null", so `value="null"` after +/// `/fill ... value=null` doesn't look like a missing element. fn queryElementProperty(self: *Verifier, arena: std.mem.Allocator, selector: []const u8, property: ElementProperty) ?[]const u8 { var aw: std.Io.Writer.Allocating = .init(arena); aw.writer.writeAll("(function(){ var el = document.querySelector(") catch return null; std.json.Stringify.value(selector, .{}, &aw.writer) catch return null; - aw.writer.writeAll("); return el ? ") catch return null; + aw.writer.writeAll("); return el ? 'v' + (") catch return null; aw.writer.writeAll(property.jsExpr()) catch return null; - aw.writer.writeAll(" : null; })()") catch return null; + aw.writer.writeAll(") : 'm'; })()") catch return null; const result = browser_tools.evalScript(arena, self.session, self.node_registry, aw.written()) catch return null; - return result.okText(); + const text = result.okText() orelse return null; + if (text.len == 0 or text[0] != 'v') return null; + return text[1..]; } diff --git a/src/script/command.zig b/src/script/command.zig index 6c86d252..f6bc0251 100644 --- a/src/script/command.zig +++ b/src/script/command.zig @@ -148,6 +148,11 @@ pub const Command = union(enum) { } pub fn parse(arena: std.mem.Allocator, line: []const u8) ParseError!Command { + return parseDiag(arena, line, null); + } + + /// Same as `parse` but populates `diag` on `error.InvalidValue`. + pub fn parseDiag(arena: std.mem.Allocator, line: []const u8, diag: ?*Schema.Diag) ParseError!Command { const trimmed = std.mem.trim(u8, line, &std.ascii.whitespace); if (trimmed.len == 0) return .{ .comment = {} }; if (trimmed[0] == '#') return .{ .comment = {} }; @@ -163,7 +168,7 @@ pub const Command = union(enum) { } const s = Schema.findByName(split.name) orelse return error.UnknownTool; - const args = try s.parseValue(arena, split.rest); + const args = try s.parseValueDiag(arena, split.rest, diag); return .{ .tool_call = .{ .tool = s.tool, .args = args } }; }