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.
This commit is contained in:
Adrià Arrufat
2026-05-25 17:51:46 +02:00
parent 4ad59b07b3
commit 4bc5baf7ee
12 changed files with 114 additions and 25 deletions

View File

@@ -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 });

View File

@@ -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

View File

@@ -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

View File

@@ -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);
}

View File

@@ -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 } };
}

View File

@@ -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();

View File

@@ -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());

View File

@@ -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 {

View File

@@ -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);
};

View File

@@ -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 `/<tool> '''<body>'''` —
@@ -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 => {},
}

View File

@@ -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..];
}

View File

@@ -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 } };
}