fix(maestro): resolve command wrappers with a shell-aware tokenizer (#199)

command_program/command_tool whitespace-split and only skipped whole wrapper words, so quoted paths broke, sudo -u/env -i/nohup/time/npx --yes/pnpm|yarn --silent dlx/bun x were rejected, and pnpm claude / yarn claude (a package.json script, not `dlx claude`) resolved as a Claude launch. Replace it with a tokenizer that honours quoting and escaping without evaluating any expansion, and per-wrapper option tables (env, sudo, command, nohup, time, npx, pnpm, yarn, bun) that require dlx/x before looking through a package-manager wrapper. Ambiguous shell grammar (pipelines, list operators, redirections, substitution, unterminated quotes) is rejected outright rather than guessed through; command_program still degrades to the naive first word for display, but command_tool returns None. Mirror the same resolver in web/src/toolIcon.ts and add fixtures/command-resolution.json as a parity fixture both test suites assert against.
2026-09-05 13:19:28 +01:00
parent e7086512e7
commit 370edfa763
5 changed files with 691 additions and 74 deletions
@@ -1,5 +1,12 @@
 import { describe, expect, it } from "vitest";
 import { splitCommand, toolIconSvg } from "./toolIcon.js";
+import commandResolutionFixture from "../../fixtures/command-resolution.json";
+
+interface CommandResolutionCase {
+  command: string;
+  program: string;
+  tool: string | null;
+}

 describe("tool icons on the command fragment (docs/SIDEBAR.md)", () => {
  it("recognises the agents and shells sessions are spawned with", () => {
@@ -46,6 +53,14 @@ describe("tool icons on the command fragment (docs/SIDEBAR.md)", () => {
    expect(splitCommand("").tool).toBeNull();
  });

+  it("resolves every case in the shared command-resolution fixture (#199)", () => {
+    const cases = commandResolutionFixture.cases as CommandResolutionCase[];
+    expect(cases.length).toBeGreaterThan(0);
+    for (const { command, program, tool } of cases) {
+      expect(splitCommand(command), command).toEqual({ tool, program });
+    }
+  });
+
  it("gives every recognised tool a labelled mark", () => {
    for (const tool of ["claude", "codex", "opencode", "gemini", "aider", "cursor", "pi", "shell"]) {
      const svg = toolIconSvg(tool);
@@ -9,10 +9,11 @@
 * as text, so the fragment never renders a mark that stands for the wrong tool.
 */

-/** Command wrappers that run another program: the tool is what follows. */
-const WRAPPERS = new Set(["sudo", "env", "npx", "bunx", "uvx", "pnpm", "yarn", "command"]);
-
-/** Program name (already basenamed and lowercased) → icon id. */
+/** Program name (already basenamed and lowercased) → icon id.
+ *
+ * `claude-code-acp` — the daemon's actual structured-session child — is
+ * deliberately absent: the daemon launches it directly and ignores the
+ * session's command, so it is never itself a valid user-facing launch form. */
 const BY_PROGRAM: Record<string, string> = {
  claude: "claude",
  "claude-code": "claude",
@@ -53,15 +54,42 @@ export interface CommandParts {
  program: string;
 }

-/** Split a command line into the tool its icon stands for and the program name.
+/** Split a command line into the tool its icon stands for and the program
+ * name — basenamed and stripped of its arguments. Leading `FOO=bar`
+ * assignments and wrapper forms (`sudo`, `env -i`, `npx`, `pnpm dlx`, …) are
+ * looked through, and quoting/escaping is honoured (never evaluated — no
+ * `$var`/`` `cmd` ``/`$(cmd)` expansion). A command a shell would need real
+ * grammar for (pipelines, `&&`/`;`, redirections, substitution, an
+ * unterminated quote) falls back to the naive first word rather than
+ * guessing through it (#199). Mirrors the Rust `command_program`/
+ * `command_tool` in `crates/maestro-protocol/src/display.rs`.
 *
 * `claude --resume` → `{tool: "claude", program: "claude"}`; an unrecognised
 * `hx src/` keeps `hx` as the program with no tool. */
 export function splitCommand(command: string): CommandParts {
-  const words = command.trim().split(/\s+/).filter(Boolean);
+  const words = tokenize(command);
+  if (words === null) {
+    const program = legacyFirstWord(command);
+    return { tool: null, program };
+  }
+  const i = programIndex(words);
+  if (i >= words.length) return { tool: null, program: command.trim() };
+  const program = basename(words[i]);
+  return { tool: BY_PROGRAM[program.toLowerCase()] ?? null, program };
+}
+
+/** The naive "first whitespace word, basenamed" reading used only when
+ * {@link tokenize} rejects the line outright — still something sane to
+ * show, never a wrapper/tool match to act on. */
+function legacyFirstWord(command: string): string {
+  const first = command.trim().split(/\s+/).filter(Boolean)[0];
+  return first === undefined ? command.trim() : basename(first);
+}
+
+/** Walk `words` past leading `FOO=bar` assignments and known wrapper forms
+ * (interleaved in any order/count) to the index of the actual program. */
+function programIndex(words: string[]): number {
  let i = 0;
-  // Leading `FOO=bar` assignments and wrappers stand in front of the real
-  // program; `pnpm dlx x` / `yarn dlx x` need one more word skipped.
  while (i < words.length) {
    const word = words[i];
    if (/^[A-Za-z_][A-Za-z0-9_]*=/.test(word)) {
@@ -69,16 +97,16 @@ export function splitCommand(command: string): CommandParts {
      continue;
    }
    const wrapper = basename(word).toLowerCase();
-    if (WRAPPERS.has(wrapper)) {
-      i += 1;
-      if ((wrapper === "pnpm" || wrapper === "yarn") && words[i] === "dlx") i += 1;
-      continue;
-    }
-    break;
+    const resolver = WRAPPER_RESOLVERS[wrapper];
+    if (!resolver) break;
+    const next = resolver(words, i);
+    // Not a form of the wrapper we recognise (an unknown option, or — for
+    // `pnpm`/`yarn`/`bun` — no `dlx`/`x` subcommand behind it): the wrapper
+    // word itself is the program, not a launcher.
+    if (next === null || next <= i) break;
+    i = next;
  }
-  if (i >= words.length) return { tool: null, program: command.trim() };
-  const program = basename(words[i]);
-  return { tool: BY_PROGRAM[program.toLowerCase()] ?? null, program };
+  return i;
 }

 function basename(word: string): string {
@@ -86,6 +114,208 @@ function basename(word: string): string {
  return cut >= 0 ? word.slice(cut + 1) : word;
 }

+/** Word-split `command` the way a shell would (honouring `'…'`, `"…"` and
+ * `\` quoting/escaping — POSIX quote *removal*, not evaluation) and return
+ * `null` the moment it needs real shell grammar this resolver doesn't
+ * attempt: a pipeline/list operator (`|`, `&`, `;`, `<`, `>`), a subshell or
+ * substitution (`(`, `)`, `` ` ``, `$(`), or an unterminated quote/escape.
+ * Never expands `$var`/`` `cmd` ``/`$(cmd)` — a bare `$` is kept literal. */
+function tokenize(command: string): string[] | null {
+  const words: string[] = [];
+  let current = "";
+  let hasCurrent = false;
+  let i = 0;
+  const n = command.length;
+
+  while (i < n) {
+    const c = command[i];
+    if (c === " " || c === "\t" || c === "\n" || c === "\r") {
+      if (hasCurrent) {
+        words.push(current);
+        current = "";
+        hasCurrent = false;
+      }
+      i += 1;
+    } else if ("|&;<>()`".includes(c)) {
+      return null;
+    } else if (c === "$" && command[i + 1] === "(") {
+      return null;
+    } else if (c === "'") {
+      hasCurrent = true;
+      i += 1;
+      const close = command.indexOf("'", i);
+      if (close === -1) return null; // unterminated single quote
+      current += command.slice(i, close);
+      i = close + 1;
+    } else if (c === '"') {
+      hasCurrent = true;
+      i += 1;
+      for (;;) {
+        if (i >= n) return null; // unterminated double quote
+        const c2 = command[i];
+        if (c2 === '"') {
+          i += 1;
+          break;
+        }
+        if (c2 === "\\") {
+          const escaped = command[i + 1];
+          if (escaped === undefined) return null; // trailing backslash
+          current += '"\\$`'.includes(escaped) ? escaped : "\\" + escaped;
+          i += 2;
+        } else {
+          current += c2;
+          i += 1;
+        }
+      }
+    } else if (c === "\\") {
+      const escaped = command[i + 1];
+      if (escaped === undefined) return null; // trailing backslash
+      hasCurrent = true;
+      current += escaped;
+      i += 2;
+    } else {
+      hasCurrent = true;
+      current += c;
+      i += 1;
+    }
+  }
+  if (hasCurrent) words.push(current);
+  return words;
+}
+
+/** A wrapper's known `-`/`--` options: which take no value, and which
+ * consume either a following word or a `--flag=value` as theirs. A
+ * `-`-prefixed word this table doesn't know halts resolution rather than
+ * guessing past it (#199) — callers read that as {@link consumeOptions}
+ * returning `null`. */
+interface OptionTable {
+  flags: Set<string>;
+  valued: Set<string>;
+}
+
+/** Consume `opts`'s options starting at `words[i]`, returning the index just
+ * past them (which may equal `i` if there are none) — or `null` on an
+ * option this table doesn't recognise, or a valued option missing its
+ * operand. */
+function consumeOptions(opts: OptionTable, words: string[], i: number): number | null {
+  for (;;) {
+    const word = words[i];
+    if (word === undefined) return i;
+    if (word === "--") return i + 1;
+    if (word === "-" || !word.startsWith("-")) return i;
+    const eq = word.indexOf("=");
+    if (eq >= 0) {
+      if (opts.valued.has(word.slice(0, eq))) {
+        i += 1;
+        continue;
+      }
+      return null;
+    }
+    if (opts.flags.has(word)) {
+      i += 1;
+      continue;
+    }
+    if (opts.valued.has(word)) {
+      if (words[i + 1] === undefined) return null;
+      i += 2;
+      continue;
+    }
+    return null;
+  }
+}
+
+const NO_OPTIONS: OptionTable = { flags: new Set(), valued: new Set() };
+
+const ENV_OPTS: OptionTable = {
+  flags: new Set(["-i", "--ignore-environment", "-0", "--null"]),
+  valued: new Set(["-u", "--unset", "-C", "--chdir", "-S", "--split-string"]),
+};
+
+const SUDO_OPTS: OptionTable = {
+  flags: new Set([
+    "-i",
+    "--login",
+    "-n",
+    "--non-interactive",
+    "-E",
+    "--preserve-env",
+    "-H",
+    "--set-home",
+    "-A",
+    "--askpass",
+    "-b",
+    "--background",
+    "-k",
+    "--reset-timestamp",
+    "-K",
+    "--remove-timestamp",
+    "-S",
+    "--stdin",
+  ]),
+  valued: new Set(["-u", "--user", "-g", "--group"]),
+};
+
+const COMMAND_BUILTIN_OPTS: OptionTable = {
+  flags: new Set(["-p", "-v", "-V"]),
+  valued: new Set(),
+};
+
+const TIME_OPTS: OptionTable = {
+  flags: new Set(["-p", "--portability", "-v", "--verbose", "-a", "--append", "-q", "--quiet"]),
+  valued: new Set(["-o", "--output", "-f", "--format"]),
+};
+
+const NPX_OPTS: OptionTable = {
+  flags: new Set(["-y", "--yes", "--no", "-q", "--quiet", "--ignore-existing"]),
+  valued: new Set(["-p", "--package", "-c", "--call", "--shell"]),
+};
+
+const PNPM_OPTS: OptionTable = {
+  flags: new Set(["--silent", "-s", "--stream", "--aggregate-output", "--no-progress"]),
+  valued: new Set(["-C", "--dir", "--filter"]),
+};
+
+const YARN_OPTS: OptionTable = {
+  flags: new Set(["--silent", "-s", "--no-progress", "--non-interactive"]),
+  valued: new Set(["--cwd"]),
+};
+
+/** `pnpm`/`yarn` only look through to the package behind an explicit `dlx`
+ * — without it, the next word is a `package.json` script name, not a
+ * package to run (#199: `pnpm claude` is not `pnpm dlx claude`). */
+function resolveDlx(opts: OptionTable, words: string[], i: number): number | null {
+  const j = consumeOptions(opts, words, i + 1);
+  if (j === null) return null;
+  return words[j] === "dlx" ? j + 1 : null;
+}
+
+/** `bun x <pkg>` is bun's `dlx`/`npx` equivalent; `bun run <script>` is not
+ * (the same package-script ambiguity as bare `pnpm`/`yarn`), so only an `x`
+ * subcommand is looked through. */
+function resolveBun(words: string[], i: number): number | null {
+  return words[i + 1] === "x" ? i + 2 : null;
+}
+
+/** A resolver for one wrapper form: given the tokenized command and the
+ * index of the wrapper word itself, the index of the word after it (past
+ * its options and any subcommand it requires), or `null` if this occurrence
+ * isn't a form we resolve through. */
+type WrapperResolver = (words: string[], i: number) => number | null;
+
+const WRAPPER_RESOLVERS: Record<string, WrapperResolver> = {
+  env: (words, i) => consumeOptions(ENV_OPTS, words, i + 1),
+  sudo: (words, i) => consumeOptions(SUDO_OPTS, words, i + 1),
+  command: (words, i) => consumeOptions(COMMAND_BUILTIN_OPTS, words, i + 1),
+  nohup: (words, i) => consumeOptions(NO_OPTIONS, words, i + 1),
+  time: (words, i) => consumeOptions(TIME_OPTS, words, i + 1),
+  npx: (words, i) => consumeOptions(NPX_OPTS, words, i + 1),
+  pnpm: (words, i) => resolveDlx(PNPM_OPTS, words, i),
+  yarn: (words, i) => resolveDlx(YARN_OPTS, words, i),
+  bun: resolveBun,
+  bunx: (_words, i) => i + 1,
+  uvx: (_words, i) => i + 1,
+};
+
 /** Anthropic's burst: tapered rays around a centre, long ones on the axes. */
 function burst(): string {
  const rays = Array.from({ length: 8 }, (_, k) => {