Merge pull request #3649 from lightpanda-io/cli-suggest-threshold

cli: scale the "did you mean" distance with the input's length

src/Config.zig

@@ -69,8 +69,7 @@ fn logFilterValidator(allocator: Allocator, args: *std.process.Args.Iterator, li
         }
 
         const v = std.meta.stringToEnum(log.Scope, name) orelse {
-            log.fatal(.app, "invalid option choice", .{ .arg = "--log-filter", .value = part });
-            return error.InvalidOption;
+            return cli.invalidChoice("--log-filter", part[0 .. part.len - name.len], name, comptime tagNames(log.Scope) ++ &[_][]const u8{"all"});
         };
 
         try list.append(allocator, .{ .scope = v, .enable = enable });
@@ -85,10 +84,7 @@ fn logLevelValidator(_: Allocator, args: *std.process.Args.Iterator, target: *?l
         return;
     }
 
-    target.* = std.meta.stringToEnum(log.Level, str) orelse {
-        log.fatal(.app, "invalid option choice", .{ .arg = "--log-level", .value = str });
-        return error.InvalidArgument;
-    };
+    target.* = std.meta.stringToEnum(log.Level, str) orelse return cli.invalidChoice("--log-level", "", str, tagNames(log.Level));
     log.opts.level = target.*.?;
 }
 
@@ -101,10 +97,7 @@ fn mcpLogDefaults() void {
 
 fn logFormatValidator(_: Allocator, args: *std.process.Args.Iterator, target: *?log.Format) !void {
     const str = args.next() orelse return error.MissingArgument;
-    const format = std.meta.stringToEnum(log.Format, str) orelse {
-        log.fatal(.app, "invalid option choice", .{ .arg = "--log-format", .value = str });
-        return error.InvalidArgument;
-    };
+    const format = std.meta.stringToEnum(log.Format, str) orelse return cli.invalidChoice("--log-format", "", str, tagNames(log.Format));
     target.* = format;
     log.opts.format = format;
 }
@@ -323,9 +316,8 @@ fn dumpValidator(_: Allocator, args: *std.process.Args.Iterator, target: *?DumpF
     if (peek_args.next()) |next_arg| {
         const mode = std.meta.stringToEnum(DumpFormat, next_arg) orelse {
             // Anything else is the positional url, unless it is a misspelt format.
-            if (string.closest(next_arg, tagNames(DumpFormat), 2)) |near| {
-                log.fatal(.app, "invalid option choice", .{ .arg = "--dump", .value = log.red(next_arg), .did_you_mean = log.green(near) });
-                return error.InvalidArgument;
+            if (!cli.isUrlLike(next_arg) and string.closest(next_arg, tagNames(DumpFormat)) != null) {
+                return cli.invalidChoice("--dump", "", next_arg, tagNames(DumpFormat));
             }
             target.* = .html;
             return;
@@ -1368,6 +1360,43 @@ test "Config: parseArgs --http-session-timeout" {
     }
 }
 
+test "Config: parseArgs --dump tells a url from a misspelt format" {
+    var arena = std.heap.ArenaAllocator.init(std.testing.allocator);
+    defer arena.deinit();
+
+    {
+        const argv = [_][*:0]const u8{ "lightpanda", "fetch", "--dump", "markdown.com" };
+        const proc_args: std.process.Args = .{ .vector = &argv };
+        const config = try parseArgs(arena.allocator(), proc_args);
+        try std.testing.expectEqual(.html, config.mode.fetch.dump);
+        try std.testing.expectEqualStrings("markdown.com", config.mode.fetch.url.items[0]);
+    }
+    {
+        log.expectLog(&.{.app});
+        const argv = [_][*:0]const u8{ "lightpanda", "fetch", "--dump", "markdwon", "https://example.com" };
+        const proc_args: std.process.Args = .{ .vector = &argv };
+        try std.testing.expectError(error.InvalidArgument, parseArgs(arena.allocator(), proc_args));
+    }
+}
+
+test "Config: parseArgs tells a url from a misspelt command" {
+    var arena = std.heap.ArenaAllocator.init(std.testing.allocator);
+    defer arena.deinit();
+
+    {
+        const argv = [_][*:0]const u8{ "lightpanda", "version.io" };
+        const proc_args: std.process.Args = .{ .vector = &argv };
+        const config = try parseArgs(arena.allocator(), proc_args);
+        try std.testing.expectEqualStrings("version.io", config.mode.fetch.url.items[0]);
+    }
+    {
+        log.expectLog(&.{.app});
+        const argv = [_][*:0]const u8{ "lightpanda", "versoin" };
+        const proc_args: std.process.Args = .{ .vector = &argv };
+        try std.testing.expectError(error.UnknownCommand, parseArgs(arena.allocator(), proc_args));
+    }
+}
+
 test "Config: validateUserAgent" {
     try validateUserAgent("Lightpanda/1.0");
     try std.testing.expectError(error.Reserved, validateUserAgent("mozilla/1.0"));

src/agent/SlashCommand.zig

@@ -103,7 +103,7 @@ const meta_names = blk: {
 /// Every slash-invocable name: browser tools, LLM triggers, meta commands.
 pub const all_names = browser_tools.names ++ llm_names ++ meta_names;
 
-/// Closest command name within two edits, or null — for "did you mean?" on typos.
+/// Closest command name, or null — for "did you mean?" on typos.
 pub fn closestCommand(name: []const u8) ?[]const u8 {
-    return string.closest(name, &all_names, 2);
+    return string.closest(name, &all_names);
 }

src/cli.zig

@@ -200,6 +200,26 @@ pub fn tagNames(comptime E: type) []const []const u8 {
     };
 }
 
+/// No command or choice has a `.`, `/` or `:`, so `markdown.com` is a url
+/// however close it is to `markdown`.
+pub fn isUrlLike(arg: []const u8) bool {
+    return std.mem.indexOfAny(u8, arg, ".:/") != null;
+}
+
+/// `prefix` was stripped from `value` before matching, like `--log-filter`'s sign.
+pub fn invalidChoice(arg: []const u8, prefix: []const u8, value: []const u8, choices: []const []const u8) error{InvalidArgument} {
+    var value_buf: [128]u8 = undefined;
+    const typed = std.fmt.bufPrint(&value_buf, "{s}{s}", .{ prefix, value }) catch value;
+    if (string.closest(value, choices)) |near| {
+        var near_buf: [128]u8 = undefined;
+        const suggestion = std.fmt.bufPrint(&near_buf, "{s}{s}", .{ prefix, near }) catch near;
+        log.fatal(.app, "invalid option choice", .{ .arg = arg, .value = log.red(typed), .did_you_mean = log.green(suggestion) });
+    } else {
+        log.fatal(.app, "invalid option choice", .{ .arg = arg, .value = log.red(typed) });
+    }
+    return error.InvalidArgument;
+}
+
 pub fn Builder(comptime commands: anytype) type {
     return struct {
         const Self = @This();
@@ -442,7 +462,7 @@ pub fn Builder(comptime commands: anytype) type {
             }
 
             // A bare word close to a command name is a typo, not a fetch url.
-            if (std.mem.startsWith(u8, cmd_str, "--") == false and string.closest(cmd_str, command_names, 2) != null) {
+            if (std.mem.startsWith(u8, cmd_str, "--") == false and !isUrlLike(cmd_str) and string.closest(cmd_str, command_names) != null) {
                 return unknownCommand(cmd_str);
             }
 
@@ -474,7 +494,7 @@ pub fn Builder(comptime commands: anytype) type {
 
         fn unknownCommand(name: []const u8) error{UnknownCommand} {
             const arg = log.red(name);
-            if (string.closest(name, command_names, 2)) |near| {
+            if (string.closest(name, command_names)) |near| {
                 log.fatal(.app, "unknown command", .{ .arg = arg, .did_you_mean = log.green(near) });
             } else {
                 log.fatal(.app, "unknown command", .{ .arg = arg });
@@ -688,15 +708,7 @@ pub fn Builder(comptime commands: anytype) type {
                     };
 
                     const str = args.next() orelse return error.MissingArgument;
-                    const v = std.meta.stringToEnum(E, str) orelse {
-                        const value = log.red(str);
-                        if (string.closest(str, tagNames(E), 2)) |near| {
-                            log.fatal(.app, "invalid option choice", .{ .arg = kebab_cased, .value = value, .did_you_mean = log.green(near) });
-                        } else {
-                            log.fatal(.app, "invalid option choice", .{ .arg = kebab_cased, .value = value });
-                        }
-                        return error.InvalidArgument;
-                    };
+                    const v = std.meta.stringToEnum(E, str) orelse return invalidChoice(kebab_cased, "", str, tagNames(E));
 
                     if (is_multiple) {
                         try target.append(allocator, v);
@@ -819,7 +831,7 @@ pub fn Builder(comptime commands: anytype) type {
                 if (std.mem.startsWith(u8, option_name, "--")) {
                     const names = comptime optionNames(options) ++ &[_][]const u8{"--help"};
                     const arg = log.red(option_name);
-                    if (string.closest(option_name, names, 2)) |near| {
+                    if (string.closest(option_name, names)) |near| {
                         log.fatal(.app, "unknown argument", .{ .mode = command.name, .arg = arg, .did_you_mean = log.green(near) });
                     } else {
                         log.fatal(.app, "unknown argument", .{ .mode = command.name, .arg = arg });

src/main.zig

@@ -68,7 +68,6 @@ fn run(allocator: Allocator, main_arena: Allocator, proc_args: std.process.Args)
         // Already logged where they were found.
         error.UnknownCommand,
         error.UnknownOption,
-        error.InvalidOption,
         error.InvalidArgument,
         error.MissingArgument,
         error.TooManyPositionalArguments,

src/string.zig

@@ -417,28 +417,46 @@ pub fn isOneOf(needle: []const u8, haystack: []const []const u8) bool {
     } else false;
 }
 
-/// Case-insensitive. Inputs over 64 bytes return `maxInt`; that fits the
-/// longest CLI flag.
+/// Case-insensitive, and swapping two adjacent characters counts as one
+/// edit. Inputs over 64 bytes return `maxInt`; that fits the longest CLI flag.
 fn editDistance(a: []const u8, b: []const u8) usize {
     const max = 64;
     if (a.len > max or b.len > max) return std.math.maxInt(usize);
+    var a_buf: [max]u8 = undefined;
+    var b_buf: [max]u8 = undefined;
+    const la = std.ascii.lowerString(&a_buf, a);
+    const lb = std.ascii.lowerString(&b_buf, b);
+
+    var prev2: [max + 1]u8 = undefined;
     var prev: [max + 1]u8 = undefined;
     var cur: [max + 1]u8 = undefined;
-    for (0..b.len + 1) |j| prev[j] = @intCast(j);
-    for (a, 1..) |ca, i| {
-        const la = std.ascii.toLower(ca);
+    for (0..lb.len + 1) |j| prev[j] = @intCast(j);
+    for (la, 1..) |ca, i| {
         cur[0] = @intCast(i);
-        for (b, 1..) |cb, j| {
-            const cost: u8 = if (la == std.ascii.toLower(cb)) 0 else 1;
+        for (lb, 1..) |cb, j| {
+            const cost: u8 = if (ca == cb) 0 else 1;
             cur[j] = @min(@min(prev[j] + 1, cur[j - 1] + 1), prev[j - 1] + cost);
+            if (i > 1 and j > 1 and ca == lb[j - 2] and la[i - 2] == cb) {
+                cur[j] = @min(cur[j], prev2[j - 2] + 1);
+            }
         }
+        prev2 = prev;
         prev = cur;
     }
     return prev[b.len];
 }
 
-/// Earlier candidates win ties.
-pub fn closest(name: []const u8, candidates: []const []const u8, max_dist: usize) ?[]const u8 {
+/// The candidate nearest to `name`, within about one edit per three characters
+/// (rustc's rule). A prefix every candidate shares, like a flag's `--`, doesn't
+/// count toward the length. Earlier candidates win ties.
+pub fn closest(name: []const u8, candidates: []const []const u8) ?[]const u8 {
+    if (candidates.len == 0) return null;
+    var shared = candidates[0];
+    for (candidates[1..]) |cand| {
+        shared = shared[0 .. std.mem.indexOfDiff(u8, shared, cand) orelse shared.len];
+    }
+    const typed = if (std.mem.startsWith(u8, name, shared)) name.len - shared.len else name.len;
+    const max_dist = @max(typed, 3) / 3;
     var best: ?[]const u8 = null;
     var best_dist: usize = std.math.maxInt(usize);
     for (candidates) |cand| {
@@ -553,6 +571,8 @@ test "editDistance" {
     try testing.expectEqual(@as(usize, 1), editDistance("wait-m", "wait-ms"));
     try testing.expectEqual(@as(usize, 1), editDistance("wait_ms", "wait-ms"));
     try testing.expectEqual(@as(usize, 3), editDistance("kitten", "sitting"));
+    try testing.expectEqual(@as(usize, 1), editDistance("dmup", "dump"));
+    try testing.expectEqual(@as(usize, 1), editDistance("ab", "ba"));
     try testing.expectEqual(@as(usize, 3), editDistance("", "abc"));
     try testing.expectEqual(@as(usize, 3), editDistance("abc", ""));
 
@@ -562,15 +582,24 @@ test "editDistance" {
 }
 
 test "closest" {
-    const names = [_][]const u8{ "--dump", "--wait-ms", "--wait-until" };
-    try testing.expectEqual("--wait-ms", closest("--wait-mss", &names, 2));
-    try testing.expectEqual("--dump", closest("--dmup", &names, 2));
-    try testing.expectEqual(null, closest("--totally-wrong", &names, 2));
-    try testing.expectEqual(null, closest("--wait-mss", &names, 0));
-    try testing.expectEqual(null, closest("--dump", &.{}, 2));
+    const names = [_][]const u8{ "--dump", "--wait-ms", "--wait-until", "--insecure-disable-tls-host-verification" };
+    try testing.expectEqual("--wait-ms", closest("--wait-mss", &names));
+    try testing.expectEqual("--dump", closest("--dmup", &names));
+    try testing.expectEqual(null, closest("--dmpx", &names));
+    try testing.expectEqual(null, closest("--totally-wrong", &names));
+    try testing.expectEqual(null, closest("--dump", &.{}));
+    try testing.expectEqual("--insecure-disable-tls-host-verification", closest("--insecure-disable-tls-verification", &names));
 
-    const tie = [_][]const u8{ "ab", "ac" };
-    try testing.expectEqual("ab", closest("a", &tie, 1));
+    const formats = [_][]const u8{ "html", "markdown", "pdf", "png" };
+    try testing.expectEqual(null, closest("md", &formats));
+    try testing.expectEqual("pdf", closest("pdg", &formats));
+
+    const commands = [_][]const u8{ "fetch", "mcp", "run" };
+    try testing.expectEqual("run", closest("fun", &commands));
+    try testing.expectEqual(null, closest("ab", &commands));
+
+    const tie = [_][]const u8{ "abcd", "abce" };
+    try testing.expectEqual("abcd", closest("abc", &tie));
 }
 
 test "latin1ToUtf8" {