From 1abf05aec7ab67b0d4e24711aab5167616a0798f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Adri=C3=A0=20Arrufat?= <adria.arrufat@gmail.com>
Date: Mon, 11 May 2026 14:53:17 +0200
Subject: [PATCH] agent: unify command keywords and simplify env listing

Centralizes PandaScript keywords in Command.zig as a single source of
truth for the parser and REPL. Simplifies environment variable listing
and formatting in tools.zig.
---
 src/agent/Agent.zig    |  2 +-
 src/agent/Command.zig  | 44 ++++++++++++++++------------
 src/agent/Terminal.zig | 44 ++++++++--------------------
 src/browser/tools.zig  | 66 ++++++++++++++----------------------------
 4 files changed, 61 insertions(+), 95 deletions(-)

diff --git a/src/agent/Agent.zig b/src/agent/Agent.zig
index 52d081b9..ab7b3fdf 100644
--- a/src/agent/Agent.zig
+++ b/src/agent/Agent.zig
@@ -319,7 +319,7 @@ fn runRepl(self: *Self) void {
         // PandaScript command.
         if (std.meta.activeTag(cmd) == .natural_language) {
             if (Command.keywordSyntax(line)) |kc| {
-                self.terminal.printErrorFmt("Usage: {s} {s}", .{ kc.name, kc.args });
+                self.terminal.printErrorFmt("Usage: {s} {s}", .{ kc.name, kc.args.? });
                 continue;
             }
         }
diff --git a/src/agent/Command.zig b/src/agent/Command.zig
index 6ca26895..5b7a7ac4 100644
--- a/src/agent/Command.zig
+++ b/src/agent/Command.zig
@@ -219,34 +219,42 @@ pub fn parse(line: []const u8) Command {
 
 pub const KeywordSyntax = struct {
     name: []const u8,
-    args: []const u8,
+    /// Null for argless commands (TREE, MARKDOWN, LOGIN, ACCEPT_COOKIES).
+    args: ?[]const u8,
+};
+
+/// Single source of truth for PandaScript keyword names — consumed by the
+/// parser, the REPL highlighter, and Tab completion. Order is REPL-facing
+/// (action verbs first), so completions feel natural.
+pub const keywords = [_]KeywordSyntax{
+    .{ .name = "GOTO", .args = "<url>" },
+    .{ .name = "CLICK", .args = "'<selector>'" },
+    .{ .name = "TYPE", .args = "'<selector>' '<value>'" },
+    .{ .name = "WAIT", .args = "'<selector>'" },
+    .{ .name = "SCROLL", .args = "[x] [y]" },
+    .{ .name = "HOVER", .args = "'<selector>'" },
+    .{ .name = "SELECT", .args = "'<selector>' '<value>'" },
+    .{ .name = "CHECK", .args = "'<selector>' [true|false]" },
+    .{ .name = "TREE", .args = null },
+    .{ .name = "MARKDOWN", .args = null },
+    .{ .name = "EXTRACT", .args = "'<selector>'" },
+    .{ .name = "EVAL", .args = "'<script>'" },
+    .{ .name = "LOGIN", .args = null },
+    .{ .name = "ACCEPT_COOKIES", .args = null },
 };
 
 /// If the first word of `line` matches a recognized PandaScript keyword that
 /// takes arguments, returns its expected shape. Lets the REPL distinguish
 /// "you mistyped args for a known command" from "this is natural language" —
 /// the latter goes to the LLM, the former gets a syntax error. Argless
-/// commands (TREE, MARKDOWN, LOGIN, ACCEPT_COOKIES) are intentionally absent
-/// because they always parse successfully when typed alone, so they never
-/// fall through to natural_language.
+/// commands return null because they always parse successfully when typed
+/// alone, so they never fall through to natural_language.
 pub fn keywordSyntax(line: []const u8) ?KeywordSyntax {
     const trimmed = std.mem.trim(u8, line, &std.ascii.whitespace);
     const end = std.mem.indexOfAny(u8, trimmed, &std.ascii.whitespace) orelse trimmed.len;
     const word = trimmed[0..end];
-    const table = [_]KeywordSyntax{
-        .{ .name = "GOTO", .args = "<url>" },
-        .{ .name = "CLICK", .args = "'<selector>'" },
-        .{ .name = "TYPE", .args = "'<selector>' '<value>'" },
-        .{ .name = "WAIT", .args = "'<selector>'" },
-        .{ .name = "SCROLL", .args = "[x] [y]" },
-        .{ .name = "HOVER", .args = "'<selector>'" },
-        .{ .name = "SELECT", .args = "'<selector>' '<value>'" },
-        .{ .name = "CHECK", .args = "'<selector>' [true|false]" },
-        .{ .name = "EXTRACT", .args = "'<selector>'" },
-        .{ .name = "EVAL", .args = "'<script>'" },
-    };
-    for (table) |kc| {
-        if (std.mem.eql(u8, word, kc.name)) return kc;
+    for (keywords) |kc| {
+        if (kc.args != null and std.mem.eql(u8, word, kc.name)) return kc;
     }
     return null;
 }
diff --git a/src/agent/Terminal.zig b/src/agent/Terminal.zig
index 43876c5a..2d81d3c7 100644
--- a/src/agent/Terminal.zig
+++ b/src/agent/Terminal.zig
@@ -2,6 +2,7 @@ const std = @import("std");
 const lp = @import("lightpanda");
 const browser_tools = lp.tools;
 const Config = lp.Config;
+const Command = @import("Command.zig");
 const SlashCommand = @import("SlashCommand.zig");
 const Spinner = @import("Spinner.zig");
 const c = @cImport({
@@ -39,25 +40,6 @@ spinner: Spinner,
 /// `setSlashSchemas` is called.
 slash_schemas: []const SlashCommand.SchemaInfo = &.{},
 
-const CommandInfo = struct { name: [:0]const u8 };
-
-const commands = [_]CommandInfo{
-    .{ .name = "GOTO" },
-    .{ .name = "CLICK" },
-    .{ .name = "TYPE" },
-    .{ .name = "WAIT" },
-    .{ .name = "SCROLL" },
-    .{ .name = "HOVER" },
-    .{ .name = "SELECT" },
-    .{ .name = "CHECK" },
-    .{ .name = "TREE" },
-    .{ .name = "MARKDOWN" },
-    .{ .name = "EXTRACT" },
-    .{ .name = "EVAL" },
-    .{ .name = "LOGIN" },
-    .{ .name = "ACCEPT_COOKIES" },
-};
-
 // Meta slash commands handled directly by the agent (not by ToolExecutor).
 // Kept in sync with `handleSlash` in `Agent.zig`. Only the names matter for
 // completion; arg hints are not surfaced separately (the menu is enough).
@@ -83,7 +65,7 @@ pub fn init(allocator: std.mem.Allocator, history_path: ?[:0]const u8, verbosity
     _ = c.ic_enable_multiline(true);
     _ = c.ic_enable_hint(true);
     _ = c.ic_enable_inline_help(true);
-    // Match linenoise's instant ghost behavior; default is 400 ms.
+    // Show ghost completions instantly; isocline's default is 400 ms.
     _ = c.ic_set_hint_delay(0);
     _ = c.ic_enable_brace_insertion(true);
     // `ps-*` namespace avoids colliding with isocline's built-in `ic-*` styles.
@@ -119,8 +101,6 @@ pub fn deinit(self: *Self) void {
     if (self.repl_arena) |*a| a.deinit();
 }
 
-// Shared between the spinner-emit path (writes to an arena buffer) and the
-// non-spinner TTY path (writes to stderr via std.debug.print).
 const bullet_line_fmt = "{s}●{s} {s}[tool: {s}]{s} {s}\n";
 
 /// Called after the tool returns.
@@ -326,9 +306,9 @@ fn completionCallback(cenv: ?*c.ic_completion_env_t, prefix: [*c]const u8) callc
     } else if (!has_space) {
         // Case-insensitive here so Tab also rewrites mistyped lowercase
         // (`goto` → `GOTO`); the highlighter stays case-sensitive.
-        for (commands) |cmd| {
-            if (std.ascii.startsWithIgnoreCase(cmd.name, input)) {
-                const text = std.fmt.bufPrintZ(&buf, "{s}", .{cmd.name}) catch continue;
+        for (Command.keywords) |kw| {
+            if (std.ascii.startsWithIgnoreCase(kw.name, input)) {
+                const text = std.fmt.bufPrintZ(&buf, "{s}", .{kw.name}) catch continue;
                 _ = c.ic_add_completion_prim(cenv, text.ptr, null, null, @intCast(input.len), 0);
             }
         }
@@ -368,16 +348,16 @@ fn highlighterCallback(henv: ?*c.ic_highlight_env_t, input: [*c]const u8, _: ?*a
 }
 
 fn isAllUpper(s: []const u8) bool {
-    for (s) |ch| switch (ch) {
-        'A'...'Z', '_', '0'...'9' => {},
-        else => return false,
-    };
-    return s.len > 0;
+    if (s.len == 0) return false;
+    for (s) |ch| {
+        if (!std.ascii.isUpper(ch) and !std.ascii.isDigit(ch) and ch != '_') return false;
+    }
+    return true;
 }
 
 fn isKnownCommand(name: []const u8) bool {
-    for (commands) |cmd| {
-        if (std.mem.eql(u8, cmd.name, name)) return true;
+    for (Command.keywords) |kw| {
+        if (std.mem.eql(u8, kw.name, name)) return true;
     }
     return false;
 }
diff --git a/src/browser/tools.zig b/src/browser/tools.zig
index 585e85ac..c3b69d7c 100644
--- a/src/browser/tools.zig
+++ b/src/browser/tools.zig
@@ -919,27 +919,26 @@ fn execGetEnv(arena: std.mem.Allocator, arguments: ?std.json.Value) ToolError![]
         if (lookupLpEnv(name)) |value| return value;
         return std.fmt.allocPrint(arena, "Environment variable '{s}' is not set", .{name}) catch ToolError.InternalError;
     }
-    return listLpEnvNames(arena);
+
+    const names = lpEnvNames(arena) catch return ToolError.InternalError;
+    return formatLpEnvNames(arena, names);
 }
 
-fn lpNameLessThan(_: void, a: []const u8, b: []const u8) bool {
-    return std.mem.lessThan(u8, a, b);
-}
-
-fn listLpEnvNames(arena: std.mem.Allocator) ToolError![]const u8 {
-    var lines: std.ArrayList([]const u8) = .empty;
-    lines.ensureTotalCapacity(arena, std.os.environ.len) catch return ToolError.InternalError;
-    for (std.os.environ) |entry| {
-        lines.appendAssumeCapacity(std.mem.span(entry));
+fn formatLpEnvNames(arena: std.mem.Allocator, names: []const []const u8) ToolError![]const u8 {
+    if (names.len == 0) return "No LP_* environment variables are set.";
+    var aw: std.Io.Writer.Allocating = .init(arena);
+    aw.writer.print("LP_* environment variables ({d}):\n", .{names.len}) catch return ToolError.InternalError;
+    for (names) |n| {
+        aw.writer.print("  {s}\n", .{n}) catch return ToolError.InternalError;
     }
-    return formatLpEnvNames(arena, lines.items);
+    return aw.written();
 }
 
 /// Sorted `LP_*`-prefixed environment-variable names from the current
 /// process. Returned slices point into `std.os.environ`, which is stable for
 /// the process lifetime; the outer slice is allocated from `arena`. Used by
-/// the agent REPL completer to offer `$LP_*` Tab completions — same data
-/// source as the `getEnv` tool (no-name variant), just unformatted.
+/// the agent REPL completer to offer `$LP_*` Tab completions and by
+/// `execGetEnv` for the no-name variant.
 pub fn lpEnvNames(arena: std.mem.Allocator) error{OutOfMemory}![]const []const u8 {
     var names: std.ArrayList([]const u8) = .empty;
     try names.ensureTotalCapacity(arena, std.os.environ.len);
@@ -950,29 +949,14 @@ pub fn lpEnvNames(arena: std.mem.Allocator) error{OutOfMemory}![]const []const u
         if (!std.ascii.startsWithIgnoreCase(name, "LP_")) continue;
         names.appendAssumeCapacity(name);
     }
-    std.mem.sort([]const u8, names.items, {}, lpNameLessThan);
+    std.mem.sort([]const u8, names.items, {}, struct {
+        fn lt(_: void, a: []const u8, b: []const u8) bool {
+            return std.mem.lessThan(u8, a, b);
+        }
+    }.lt);
     return names.items;
 }
 
-fn formatLpEnvNames(arena: std.mem.Allocator, env_lines: []const []const u8) ToolError![]const u8 {
-    var names: std.ArrayList([]const u8) = .empty;
-    for (env_lines) |line| {
-        const eq_idx = std.mem.indexOfScalar(u8, line, '=') orelse continue;
-        const name = line[0..eq_idx];
-        if (!std.ascii.startsWithIgnoreCase(name, "LP_")) continue;
-        names.append(arena, name) catch return ToolError.InternalError;
-    }
-    if (names.items.len == 0) return "No LP_* environment variables are set.";
-    std.mem.sort([]const u8, names.items, {}, lpNameLessThan);
-
-    var aw: std.Io.Writer.Allocating = .init(arena);
-    aw.writer.print("LP_* environment variables ({d}):\n", .{names.items.len}) catch return ToolError.InternalError;
-    for (names.items) |n| {
-        aw.writer.print("  {s}\n", .{n}) catch return ToolError.InternalError;
-    }
-    return aw.written();
-}
-
 /// Resolve an LP_-prefixed environment variable, or `null` for any other name.
 /// Only the LP_ namespace is readable from the model; everything else
 /// (provider API keys, system env, third-party secrets) is hidden so the LLM
@@ -1210,31 +1194,25 @@ test "execGetEnv hides non-LP_ values even when set" {
     );
 }
 
-test "formatLpEnvNames lists names without values" {
+test "formatLpEnvNames renders names without values" {
     var arena: std.heap.ArenaAllocator = .init(std.testing.allocator);
     defer arena.deinit();
     const aa = arena.allocator();
 
-    const lines = [_][]const u8{
-        "LP_FOO=secret",
-        "HOME=/home/x",
-        "LP_BAR=other-secret",
-    };
-    const r = try formatLpEnvNames(aa, &lines);
+    const names = [_][]const u8{ "LP_BAR", "LP_FOO" };
+    const r = try formatLpEnvNames(aa, &names);
 
     try std.testing.expect(std.mem.indexOf(u8, r, "LP_FOO") != null);
     try std.testing.expect(std.mem.indexOf(u8, r, "LP_BAR") != null);
-    try std.testing.expect(std.mem.indexOf(u8, r, "HOME") == null);
     try std.testing.expect(std.mem.indexOf(u8, r, "secret") == null);
 }
 
-test "formatLpEnvNames reports empty when no LP_* entries" {
+test "formatLpEnvNames reports empty when no names" {
     var arena: std.heap.ArenaAllocator = .init(std.testing.allocator);
     defer arena.deinit();
     const aa = arena.allocator();
 
-    const lines = [_][]const u8{"HOME=/home/x"};
-    const r = try formatLpEnvNames(aa, &lines);
+    const r = try formatLpEnvNames(aa, &.{});
     try std.testing.expectEqualStrings("No LP_* environment variables are set.", r);
 }