From 4c8217385a41c11bc07e3bb6cf6d49a68f2c919b Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Sat, 5 Sep 2026 14:25:57 +0100 Subject: [PATCH] fix(maestro): scope assignment-skipping to wrappers that accept it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `command FOO=1 claude` and `nohup FOO=1 claude` are invalid shells (they exec `FOO=1` as the program, exit 127), but the resolver skipped the assignment unconditionally after any wrapper. Assignment prefixes are only real at the start of a simple command or after a wrapper that itself interprets them as env vars (env, sudo) or a keyword that restarts the assignment grammar (time) — not after command, nohup, or a package manager's subcommand, which just exec their next argument as the program. Attach an accepts_assignment flag to each wrapper transition in both the Rust and TS resolvers and gate the assignment-skip loop on the flag from the most recently consumed wrapper. Extends the shared fixture with the two reported cases plus every other wrapper transition and several nested combinations (sudo+time valid, sudo+command / command+nohup / time+command invalid, nohup+env valid) to lock the per-wrapper policy in for both languages. --- crates/maestro-protocol/src/display.rs | 83 +++++++++++++++++-------- fixtures/command-resolution.json | 13 ++++ web/src/toolIcon.ts | 84 ++++++++++++++++++++------ 3 files changed, 136 insertions(+), 44 deletions(-) diff --git a/crates/maestro-protocol/src/display.rs b/crates/maestro-protocol/src/display.rs index 3c40b2d..8bf1ff3 100644 --- a/crates/maestro-protocol/src/display.rs +++ b/crates/maestro-protocol/src/display.rs @@ -235,23 +235,34 @@ struct Resolution { } /// Tokenize `command` and walk past leading `FOO=bar` assignments and known -/// wrapper forms (interleaved in any order/count) to find the program. `None` -/// when [`tokenize`] can't safely make sense of the line at all. +/// wrapper forms (interleaved in any order/count) to find the program. An +/// assignment run is only consumed at the very start of the line or right +/// after a wrapper whose own semantics accept one there ([`Wrapper::accepts_assignment`]) — +/// `command FOO=1 claude` and `nohup FOO=1 claude` stop at `FOO=1` rather than +/// skipping to `claude`, since `command`/`nohup` just exec their next argument +/// as the program, they don't setenv from an assignment-shaped one (#199). +/// `None` when [`tokenize`] can't safely make sense of the line at all. fn resolve(command: &str) -> Option { let words = tokenize(command)?; let refs: Vec<&str> = words.iter().map(|w| w.text.as_str()).collect(); let mut i = 0; - while let Some(word) = words.get(i) { - if is_env_assignment(word) { - i += 1; - continue; + let mut assignment_allowed = true; + loop { + if assignment_allowed { + while matches!(words.get(i), Some(word) if is_env_assignment(word)) { + i += 1; + } } + let Some(word) = words.get(i) else { break }; let wrapper = basename(&word.text).to_lowercase(); - let Some(resolver) = wrapper_resolver(&wrapper) else { + let Some(w) = wrapper_resolver(&wrapper) else { break; }; - match resolver(&refs, i) { - Some(next) if next > i => i = next, + match (w.resolve)(&refs, i) { + Some(next) if next > i => { + i = next; + assignment_allowed = w.accepts_assignment; + } // 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. @@ -603,21 +614,45 @@ const BUN_X_OPTS: OptionTable = OptionTable { /// isn't a form we resolve through. type WrapperResolver = fn(&[&str], usize) -> Option; -fn wrapper_resolver(name: &str) -> Option { - match name { - "env" => Some(|words, i| ENV_OPTS.consume(words, i + 1)), - "sudo" => Some(|words, i| SUDO_OPTS.consume(words, i + 1)), - "command" => Some(|words, i| COMMAND_BUILTIN_OPTS.consume(words, i + 1)), - "nohup" => Some(|words, i| NO_OPTIONS.consume(words, i + 1)), - "time" => Some(resolve_time), - "npx" => Some(|words, i| NPX_OPTS.consume(words, i + 1)), - "pnpm" => Some(|words, i| resolve_dlx(&PNPM_OPTS, &PNPM_DLX_OPTS, words, i)), - "yarn" => Some(|words, i| resolve_dlx(&YARN_OPTS, &YARN_DLX_OPTS, words, i)), - "bun" => Some(resolve_bun), - "bunx" => Some(|words, i| BUN_X_OPTS.consume(words, i + 1)), - "uvx" => Some(|_words, i| Some(i + 1)), - _ => None, - } +/// One recognised wrapper form: how to walk past it, and whether an +/// assignment word is a valid follow-on once it's been walked past. +struct Wrapper { + resolve: WrapperResolver, + /// `true` only for wrappers that themselves interpret a following + /// `FOO=bar` as an environment assignment for the command they launch + /// (`env`, `sudo`), or a shell keyword that restarts the assignment-prefix + /// grammar for a brand new simple command (`time`). Every other wrapper + /// here is a plain executable that execs its next argument as the + /// program outright — an assignment-shaped one names a program literally + /// called `FOO=1`, not an env var (#199). + accepts_assignment: bool, +} + +fn wrapper_resolver(name: &str) -> Option { + let (resolve, accepts_assignment): (WrapperResolver, bool) = match name { + "env" => (|words, i| ENV_OPTS.consume(words, i + 1), true), + "sudo" => (|words, i| SUDO_OPTS.consume(words, i + 1), true), + "command" => (|words, i| COMMAND_BUILTIN_OPTS.consume(words, i + 1), false), + "nohup" => (|words, i| NO_OPTIONS.consume(words, i + 1), false), + "time" => (resolve_time, true), + "npx" => (|words, i| NPX_OPTS.consume(words, i + 1), false), + "pnpm" => ( + |words, i| resolve_dlx(&PNPM_OPTS, &PNPM_DLX_OPTS, words, i), + false, + ), + "yarn" => ( + |words, i| resolve_dlx(&YARN_OPTS, &YARN_DLX_OPTS, words, i), + false, + ), + "bun" => (resolve_bun, false), + "bunx" => (|words, i| BUN_X_OPTS.consume(words, i + 1), false), + "uvx" => (|_words, i| Some(i + 1), false), + _ => return None, + }; + Some(Wrapper { + resolve, + accepts_assignment, + }) } /// `pnpm`/`yarn` only look through to the package behind an explicit `dlx` diff --git a/fixtures/command-resolution.json b/fixtures/command-resolution.json index 722e592..cf09407 100644 --- a/fixtures/command-resolution.json +++ b/fixtures/command-resolution.json @@ -79,6 +79,19 @@ { "command": "env -S 'python3 -u' script.py", "program": "env", "tool": null }, { "command": "npx -c \"claude --version\"", "program": "npx", "tool": null }, { "command": "npx --call \"claude --version\"", "program": "npx", "tool": null }, + { "command": "command FOO=1 claude", "program": "FOO=1", "tool": null }, + { "command": "nohup FOO=1 claude", "program": "FOO=1", "tool": null }, + { "command": "npx FOO=1 claude-code", "program": "FOO=1", "tool": null }, + { "command": "pnpm dlx FOO=1 claude-code", "program": "FOO=1", "tool": null }, + { "command": "yarn dlx FOO=1 claude-code", "program": "FOO=1", "tool": null }, + { "command": "bun x FOO=1 claude-code", "program": "FOO=1", "tool": null }, + { "command": "bunx FOO=1 claude-code", "program": "FOO=1", "tool": null }, + { "command": "uvx FOO=1 claude-code", "program": "FOO=1", "tool": null }, + { "command": "sudo time FOO=1 claude", "program": "claude", "tool": "claude" }, + { "command": "nohup env FOO=1 claude", "program": "claude", "tool": "claude" }, + { "command": "sudo command FOO=1 claude", "program": "FOO=1", "tool": null }, + { "command": "command nohup FOO=1 claude", "program": "FOO=1", "tool": null }, + { "command": "time command FOO=1 claude", "program": "FOO=1", "tool": null }, { "command": "\"FOO=1\" claude", "program": "FOO=1", "tool": null }, { "command": "FOO\\=1 claude", "program": "FOO=1", "tool": null }, { "command": "F\"O\"O=1 claude", "program": "FOO=1", "tool": null }, diff --git a/web/src/toolIcon.ts b/web/src/toolIcon.ts index 588b21f..ef13027 100644 --- a/web/src/toolIcon.ts +++ b/web/src/toolIcon.ts @@ -105,24 +105,33 @@ function isEnvAssignment(word: Word): boolean { } /** Walk `words` past leading `FOO=bar` assignments and known wrapper forms - * (interleaved in any order/count) to the index of the actual program. */ + * (interleaved in any order/count) to the index of the actual program. An + * assignment run is only consumed at the very start of the line or right + * after a wrapper whose own semantics accept one there ({@link Wrapper.acceptsAssignment}) — + * `command FOO=1 claude` and `nohup FOO=1 claude` stop at `FOO=1` rather than + * skipping to `claude`, since `command`/`nohup` just exec their next argument + * as the program, they don't setenv from an assignment-shaped one (#199). */ function programIndex(words: Word[]): number { const texts = words.map((w) => w.text); let i = 0; - while (i < words.length) { - if (isEnvAssignment(words[i])) { - i += 1; - continue; + let assignmentAllowed = true; + for (;;) { + if (assignmentAllowed) { + while (i < words.length && isEnvAssignment(words[i])) { + i += 1; + } } + if (i >= words.length) break; const wrapper = basename(texts[i]).toLowerCase(); - const resolver = WRAPPER_RESOLVERS[wrapper]; - if (!resolver) break; - const next = resolver(texts, i); + const w = WRAPPER_RESOLVERS[wrapper]; + if (!w) break; + const next = w.resolve(texts, 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; + assignmentAllowed = w.acceptsAssignment; } return i; } @@ -433,18 +442,53 @@ function resolveBun(words: string[], i: number): number | null { * isn't a form we resolve through. */ type WrapperResolver = (words: string[], i: number) => number | null; -const WRAPPER_RESOLVERS: Record = { - 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: resolveTime, - npx: (words, i) => consumeOptions(NPX_OPTS, words, i + 1), - pnpm: (words, i) => resolveDlx(PNPM_OPTS, PNPM_DLX_OPTS, words, i), - yarn: (words, i) => resolveDlx(YARN_OPTS, YARN_DLX_OPTS, words, i), - bun: resolveBun, - bunx: (words, i) => consumeOptions(BUN_X_OPTS, words, i + 1), - uvx: (_words, i) => i + 1, +/** One recognised wrapper form: how to walk past it, and whether an + * assignment word is a valid follow-on once it's been walked past. */ +interface Wrapper { + resolve: WrapperResolver; + /** `true` only for wrappers that themselves interpret a following + * `FOO=bar` as an environment assignment for the command they launch + * (`env`, `sudo`), or a shell keyword that restarts the assignment-prefix + * grammar for a brand new simple command (`time`). Every other wrapper here + * is a plain executable that execs its next argument as the program + * outright — an assignment-shaped one names a program literally called + * `FOO=1`, not an env var (#199). */ + acceptsAssignment: boolean; +} + +const WRAPPER_RESOLVERS: Record = { + env: { resolve: (words, i) => consumeOptions(ENV_OPTS, words, i + 1), acceptsAssignment: true }, + sudo: { + resolve: (words, i) => consumeOptions(SUDO_OPTS, words, i + 1), + acceptsAssignment: true, + }, + command: { + resolve: (words, i) => consumeOptions(COMMAND_BUILTIN_OPTS, words, i + 1), + acceptsAssignment: false, + }, + nohup: { + resolve: (words, i) => consumeOptions(NO_OPTIONS, words, i + 1), + acceptsAssignment: false, + }, + time: { resolve: resolveTime, acceptsAssignment: true }, + npx: { + resolve: (words, i) => consumeOptions(NPX_OPTS, words, i + 1), + acceptsAssignment: false, + }, + pnpm: { + resolve: (words, i) => resolveDlx(PNPM_OPTS, PNPM_DLX_OPTS, words, i), + acceptsAssignment: false, + }, + yarn: { + resolve: (words, i) => resolveDlx(YARN_OPTS, YARN_DLX_OPTS, words, i), + acceptsAssignment: false, + }, + bun: { resolve: resolveBun, acceptsAssignment: false }, + bunx: { + resolve: (words, i) => consumeOptions(BUN_X_OPTS, words, i + 1), + acceptsAssignment: false, + }, + uvx: { resolve: (_words, i) => i + 1, acceptsAssignment: false }, }; /** Anthropic's burst: tapered rays around a centre, long ones on the axes. */