Merge pull request #2768 from lightpanda-io/terminal-single-positional-arg

terminal: support positional args for single-field schemas

src/agent/Agent.zig

@@ -1813,6 +1813,7 @@ fn completionModels(context: *anyopaque, _: std.mem.Allocator) []const []const u
 
 test {
     _ = save;
+    _ = settings;
 }
 
 test "capToolOutput: passes through when under cap" {

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").?;
 

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

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;

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