From 144b3a58a0fceae316edf54a644f4c034026d886 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A0=20Arrufat?= Date: Wed, 17 Jun 2026 16:26:43 +0200 Subject: [PATCH] terminal: support positional args for single-field schemas Allows tools like `/getEnv` with a single optional field to accept positional arguments. Also adds autocompletion and ghost hints for live `LP_*` environment variables. Additionally, fixes a memory leak in `settings.zig` when parsing invalid ZON configurations. --- src/agent/Agent.zig | 1 + src/agent/Terminal.zig | 62 ++++++++++++++++++++++++++++++++++-------- src/agent/settings.zig | 25 ++++++++++++++++- src/script/Schema.zig | 17 ++++++++++-- src/script/command.zig | 11 ++++++++ 5 files changed, 100 insertions(+), 16 deletions(-) diff --git a/src/agent/Agent.zig b/src/agent/Agent.zig index 6924f536a..17f935866 100644 --- a/src/agent/Agent.zig +++ b/src/agent/Agent.zig @@ -1810,6 +1810,7 @@ fn completionModels(context: *anyopaque, _: std.mem.Allocator) []const []const u test { _ = save; + _ = settings; } test "capToolOutput: passes through when under cap" { diff --git a/src/agent/Terminal.zig b/src/agent/Terminal.zig index 47580c154..a81716bca 100644 --- a/src/agent/Terminal.zig +++ b/src/agent/Terminal.zig @@ -303,12 +303,12 @@ fn analyzeBody(schema: *const Schema, body: []const u8, ends_ws: bool) BodyAnaly a.markUsed(tok[0..eq]); continue; } - // Single-required schemas bind the first arg positionally to their - // lone required field (`/goto https://example.com`). - if (i == 0 and schema.required.len == 1) { - a.markUsed(schema.required[0]); + // The first bare arg binds positionally to the schema's positional + // field (`/goto https://example.com`, `/getEnv LP_TOKEN`). + if (i == 0) if (schema.leadingPositionalField()) |pos| { + a.markUsed(pos); continue; - } + }; if (i == last and !ends_ws) a.partial_key = tok; } return a; @@ -354,11 +354,12 @@ fn valueAt(schema: *const Schema, body: []const u8, ends_ws: bool) ?ValueAt { const field = schema.findField(active[0..eq]) orelse return null; return .{ .field = field, .partial = active[eq + 1 ..], .kv = true }; } - // The leading bare token binds to a single-required schema's lone field. - if (active_index == 0 and schema.required.len == 1) { - const field = schema.findField(schema.required[0]) orelse return null; + // The leading bare token binds to the schema's positional field (lone + // required, or sole optional field like getEnv's `name`). + if (active_index == 0) if (schema.leadingPositionalField()) |pos| { + const field = schema.findField(pos) orelse return null; return .{ .field = field, .partial = active, .kv = false }; - } + }; return null; } @@ -372,8 +373,14 @@ fn addValueCompletions( ) bool { const ends_ws = input[input.len - 1] == ' '; const v = valueAt(schema, body, ends_ws) orelse return false; - if (v.field.enum_values.len == 0) return false; const prefix = input[0 .. input.len - v.partial.len]; + if (schema.tool == .getEnv) { + var name_buf: [2048]u8 = undefined; + const names = lpEnvNameList(&name_buf) orelse return true; + for (names) |name| addPrefixedCompletion(cenv, buf, input, prefix, name, "", v.partial); + return true; + } + if (v.field.enum_values.len == 0) return false; for (v.field.enum_values) |val| addPrefixedCompletion(cenv, buf, input, prefix, val, "", v.partial); return true; } @@ -461,6 +468,13 @@ fn addPathCompletions( } } +/// LP_* env var names (sorted) written into `buf`; null on enumeration failure. +/// Returned slices borrow `buf`, which must outlive them. +fn lpEnvNameList(buf: []u8) ?[]const []const u8 { + var fba: std.heap.FixedBufferAllocator = .init(buf); + return browser_tools.lpEnvNames(fba.allocator()) catch null; +} + /// Completes `$LP_*` against the live process environment. fn addEnvVarCompletions( cenv: ?*c.ic_completion_env_t, @@ -474,8 +488,7 @@ fn addEnvVarCompletions( } var name_buf: [2048]u8 = undefined; - var fba: std.heap.FixedBufferAllocator = .init(&name_buf); - const names = browser_tools.lpEnvNames(fba.allocator()) catch return; + const names = lpEnvNameList(&name_buf) orelse return; if (names.len == 0) return; const head = input[0 .. dollar + 1]; @@ -661,6 +674,14 @@ fn renderSchemaHint(schema: *const Schema, body: []const u8, ends_ws: bool) [*c] if (v.field.enum_values.len > 0 and (v.kv or v.partial.len > 0)) { return ghostFirstMatch(v.field.enum_values, v.partial, ""); } + // getEnv's `name` ghosts a live LP_* var, like /provider ghosts a provider. + if (schema.tool == .getEnv) { + var name_buf: [2048]u8 = undefined; + if (lpEnvNameList(&name_buf)) |names| { + const lead: []const u8 = if (v.partial.len == 0 and !ends_ws) " " else ""; + if (ghostFirstMatch(names, v.partial, lead)) |hint| return hint; + } + } } const a = analyzeBody(schema, body, ends_ws); @@ -1271,6 +1292,23 @@ test "valueAt: enum field via positional and kv, partial and empty" { } } +test "valueAt: getEnv binds the leading positional to its optional name field" { + const schema = Schema.findByName("getEnv").?; + // `/getEnv ` — fresh positional, even though `name` is optional (0 required). + { + const v = valueAt(schema, "", true).?; + try std.testing.expectEqualStrings("name", v.field.name); + try std.testing.expectEqualStrings("", v.partial); + try std.testing.expect(!v.kv); + } + // `/getEnv LP_H` — partial value bound to `name`. + { + const v = valueAt(schema, "LP_H", false).?; + try std.testing.expectEqualStrings("name", v.field.name); + try std.testing.expectEqualStrings("LP_H", v.partial); + } +} + test "renderSchemaHint: ghosts enum value once typing, keeps template when empty" { const schema = Schema.findByName("waitForState").?; diff --git a/src/agent/settings.zig b/src/agent/settings.zig index 1aadd8008..b323596b6 100644 --- a/src/agent/settings.zig +++ b/src/agent/settings.zig @@ -137,7 +137,15 @@ pub const Remembered = struct { pub fn loadRemembered(allocator: std.mem.Allocator) ?Remembered { const data = std.fs.cwd().readFileAllocOptions(allocator, remembered_path, 1024, null, .of(u8), 0) catch return null; defer allocator.free(data); - const remembered = std.zon.parse.fromSlice(Remembered, allocator, data, null, .{}) catch return null; + return parseRemembered(allocator, data); +} + +fn parseRemembered(allocator: std.mem.Allocator, data: [:0]const u8) ?Remembered { + // A real Diagnostics, not null: a type-check failure allocates an owned + // error note that leaks unless a Diagnostics owns it to free on deinit. + var diag: std.zon.parse.Diagnostics = .{}; + defer diag.deinit(allocator); + const remembered = std.zon.parse.fromSlice(Remembered, allocator, data, &diag, .{}) catch return null; // An empty model is corrupt only when a provider is set; a null provider // (LLM disabled) legitimately has no model to remember. if (remembered.provider != null and remembered.model.len == 0) { @@ -239,3 +247,18 @@ pub fn reconcileModel( } return .abort; } + +const testing = @import("../testing.zig"); + +test "parseRemembered: invalid enum is rejected without leaking" { + // A bad enum builds an owned error note; the leak detector fails here if + // the Diagnostics doesn't free it. + try testing.expect(parseRemembered(testing.allocator, ".{ .provider = .not_a_provider, .model = \"x\" }") == null); +} + +test "parseRemembered: valid file round-trips" { + const remembered = parseRemembered(testing.allocator, ".{ .provider = null, .model = \"some-model\" }").?; + defer std.zon.parse.free(testing.allocator, remembered); + try testing.expect(remembered.provider == null); + try testing.expectString("some-model", remembered.model); +} diff --git a/src/script/Schema.zig b/src/script/Schema.zig index b447696e3..0fdc22a15 100644 --- a/src/script/Schema.zig +++ b/src/script/Schema.zig @@ -183,6 +183,16 @@ pub fn isBarePositional(self: Schema, args: std.json.ObjectMap) bool { return v == .string; } +/// The field a single leading positional binds to: the lone required field, +/// or — when nothing is required — the tool's only field (e.g. getEnv binds to +/// `name`). Null when there's no required field and more than one optional +/// field, since the target would be ambiguous (click's selector/backendNodeId). +pub fn leadingPositionalField(self: Schema) ?[]const u8 { + if (self.required.len == 1) return self.required[0]; + if (self.required.len == 0 and self.fields.len == 1) return self.fields[0].name; + return null; +} + /// Parse `rest` (args portion of a slash command) into a `std.json.Value`. /// Returns null when the schema takes no args and `rest` is empty. /// @@ -217,19 +227,20 @@ pub fn parseValueDiag(self: Schema, arena: std.mem.Allocator, rest_raw: []const const tokens = try tokenize(arena, rest); const leading_positional = tokens.len >= 1 and !looksLikeKv(tokens[0]); - if (leading_positional and self.required.len != 1) return error.PositionalNotAllowed; + const positional_field = self.leadingPositionalField(); + if (leading_positional and positional_field == null) return error.PositionalNotAllowed; var list = try std.ArrayList(KvPair).initCapacity(arena, tokens.len + self.required.len); const kv_start: usize = if (leading_positional) 1 else 0; if (leading_positional) { - list.appendAssumeCapacity(.{ .key = self.required[0], .value = stripQuotes(tokens[0]) }); + list.appendAssumeCapacity(.{ .key = positional_field.?, .value = stripQuotes(tokens[0]) }); } for (tokens[kv_start..]) |tok| { const eq = std.mem.indexOfScalar(u8, tok, '=') orelse { // `/extract save=x '{…}'` — the value would have bound fine as a // leading positional, so point at the ordering instead of the // generic kv complaint. - if (self.required.len == 1 and !leading_positional) return error.PositionalMustComeFirst; + if (positional_field != null and !leading_positional) return error.PositionalMustComeFirst; return error.MalformedKv; }; if (eq == 0 or eq == tok.len - 1) return error.MalformedKv; diff --git a/src/script/command.zig b/src/script/command.zig index 0902a3b77..6ee19a79c 100644 --- a/src/script/command.zig +++ b/src/script/command.zig @@ -385,6 +385,17 @@ test "parse: /click rejects positional (zero required fields)" { try testing.expectString("Login", cmd.tool_call.args.?.object.get("selector").?.string); } +test "parse: /getEnv positional binds to optional name" { + var arena: std.heap.ArenaAllocator = .init(testing.allocator); + defer arena.deinit(); + const cmd = try Command.parse(arena.allocator(), "/getEnv LP_HN_USERNAME"); + try testing.expectString("getEnv", cmd.tool_call.name()); + try testing.expectString("LP_HN_USERNAME", cmd.tool_call.args.?.object.get("name").?.string); + // No arg still lists names (null args). + const list = try Command.parse(arena.allocator(), "/getEnv"); + try testing.expect(list.tool_call.args == null); +} + test "parse: /extract rejects positional after key=value" { var arena: std.heap.ArenaAllocator = .init(testing.allocator); defer arena.deinit();