From a8bcd57e3efcb126dc90c3ff4734c2c2f1bc398a Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 18 Jul 2026 10:24:13 +0800 Subject: [PATCH 1/2] fix(core): resolve a parameter expansion in command position to its command root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `getCommandRoot` had no answer for a command that begins with a parameter expansion, and the shell tool treats no-answer as a hard refusal — "Could not identify command root to obtain permission from user" — issued from validateToolParamValues, before any approval mode is consulted, YOLO included. That form is not exotic: the bundled /review skill invokes every one of its ~27 commands as `"${QWEN_CODE_CLI:-qwen}" review …`, so every review run opened with a refused command until the model hand-resolved the variable and retried with the explicit path. Observed on every dogfooded run since the variable landed. The resolver mirrors what the spawned shell will do, against the environment it will inherit: `:-` substitutes the default when the variable is unset OR empty, `-` only when unset, and a bare `$VAR` that resolves to nothing still yields no root — such a command has nothing to name and stays refusable. Leading env assignments are skipped first, exactly as the plain-token path skips them. With the variable set, the root becomes the real entry's basename (`dev.js`, `qwen`), so permission prompts and allowlists name the binary that actually runs. Verified end-to-end: the exact skill form now executes through the real shell tool in Auto mode, where it was previously refused in YOLO. Six tests pin the POSIX semantics both ways; disabling the resolver turns all six red; the shell.ts and monitor.ts caller suites pass unchanged. --- packages/core/src/utils/shell-utils.test.ts | 56 +++++++++++++++++++++ packages/core/src/utils/shell-utils.ts | 51 +++++++++++++++++++ 2 files changed, 107 insertions(+) diff --git a/packages/core/src/utils/shell-utils.test.ts b/packages/core/src/utils/shell-utils.test.ts index e330e251e81..187ee904e0f 100644 --- a/packages/core/src/utils/shell-utils.test.ts +++ b/packages/core/src/utils/shell-utils.test.ts @@ -12,6 +12,7 @@ import { COMMAND_SUBSTITUTION_WARNING, detectSelfKillCommand, escapeShellArg, + getCommandRoot, getCommandRoots, getShellConfiguration, hasNonFinalTopLevelBackgroundOperator, @@ -401,6 +402,61 @@ describe('checkCommandPermissions', () => { }); }); +describe('getCommandRoot — parameter expansion in command position', () => { + // The bundled /review skill invokes every command as + // `"${QWEN_CODE_CLI:-qwen}" review …`. Before this resolver, such a command + // had NO identifiable root — the shell tool hard-refused it ("Could not + // identify command root to obtain permission from user") before any approval + // mode was consulted, YOLO included. Dogfooded live on every /review run. + const NAME = 'SHELL_UTILS_TEST_ENTRY'; + afterEach(() => { + delete process.env[NAME]; + }); + + it('resolves ${VAR:-default} to the variable when set and non-empty', () => { + process.env[NAME] = '/repo/scripts/dev.js'; + expect(getCommandRoot(`"\${${NAME}:-qwen}" review foo`)).toBe('dev.js'); + expect(getCommandRoot(`\${${NAME}:-qwen} review foo`)).toBe('dev.js'); + }); + + it('resolves ${VAR:-default} to the default when unset OR empty — POSIX :-', () => { + expect(getCommandRoot(`"\${${NAME}:-qwen}" review foo`)).toBe('qwen'); + process.env[NAME] = ''; + expect(getCommandRoot(`"\${${NAME}:-qwen}" review foo`)).toBe('qwen'); + }); + + it('resolves ${VAR-default} to the default only when unset — POSIX -', () => { + expect(getCommandRoot(`"\${${NAME}-qwen}" review foo`)).toBe('qwen'); + process.env[NAME] = ''; + // Empty-but-set: `-` keeps the empty value; nothing to name, no root. + expect(getCommandRoot(`"\${${NAME}-qwen}" review foo`)).toBeUndefined(); + }); + + it('resolves a bare "$VAR" head, and yields no root when it is unset', () => { + process.env[NAME] = '/usr/local/bin/qwen'; + expect(getCommandRoot(`"$${NAME}" review foo`)).toBe('qwen'); + delete process.env[NAME]; + // Unset with no default resolves to nothing: the command stays refusable, + // exactly as an empty command would be — there is nothing to name. + expect(getCommandRoot(`"$${NAME}" review foo`)).toBeUndefined(); + }); + + it('skips leading env assignments before the expansion, like the plain path', () => { + process.env[NAME] = '/repo/scripts/dev.js'; + expect(getCommandRoot(`FOO=1 "\${${NAME}:-qwen}" review foo`)).toBe( + 'dev.js', + ); + }); + + it('feeds getCommandRoots, so the shell tool no longer hard-refuses the skill form', () => { + expect( + getCommandRoots( + `"\${${NAME}:-qwen}" review fetch-pr 7 --out x.json && echo done`, + ), + ).toEqual(['qwen', 'echo']); + }); +}); + describe('getCommandRoots', () => { it('should return a single command', async () => { expect(getCommandRoots('ls -l')).toEqual(['ls']); diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index 6e5476fb97a..6a5051f7b8f 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -324,6 +324,52 @@ export function splitCommands(command: string): string[] { return commands.filter(Boolean); // Filter out any empty strings } +/** + * A parameter expansion standing in command position — `$VAR`, `"$VAR"`, + * `${VAR}`, `"${VAR:-default}"`, `${VAR-default}` — resolved the way the shell + * will resolve it at execution time. + * + * Without this, such a command had NO identifiable root: the tokenizer turns + * the expansion into a non-string (or empty) token, `getCommandRoot` returns + * undefined, and the shell tool hard-refuses the command before any approval + * mode is consulted — including YOLO. Dogfooded live: the bundled /review + * skill invokes every command as `"${QWEN_CODE_CLI:-qwen}" review …`, and each + * run opened with "Could not identify command root to obtain permission from + * user" until the model hand-resolved the variable itself. + * + * Resolution mirrors POSIX: `:-` substitutes the default when the variable is + * unset OR empty; `-` only when unset; a bare `$VAR` that resolves to nothing + * yields no root, and the command stays refusable — there is nothing to name. + * The environment consulted is this process's own, which is what the spawned + * shell inherits. + */ +const PARAMETER_EXPANSION_COMMAND = + /^"?\$(?:\{([A-Za-z_][A-Za-z0-9_]*)(?:(:?-)([^}]*))?\}|([A-Za-z_][A-Za-z0-9_]*))"?$/; + +function resolveLeadingParameterExpansion(command: string): string | undefined { + let rest = command; + // Leading env assignments come first (`FOO=1 "${BAR:-baz}" …`), exactly as + // the plain-token path below skips them. + while (true) { + const t = takeLeadingToken(rest); + if (!t || !isEnvAssignmentToken(t.token)) break; + rest = t.rest; + } + const head = takeLeadingToken(rest); + if (!head) return undefined; + const m = PARAMETER_EXPANSION_COMMAND.exec(head.token); + if (!m) return undefined; + const name = (m[1] ?? m[4]) as string; + const op = m[2]; + const value = process.env[name]; + const useDefault = + op === undefined ? false : op === ':-' ? !value : value === undefined; + const resolved = useDefault + ? stripSymmetricQuotes(m[3] ?? '').value + : (value ?? ''); + return resolved || undefined; +} + /** * Extracts the root command from a given shell command string. * Skips leading env var assignments (VAR=value) so that @@ -335,6 +381,11 @@ export function getCommandRoot(command: string): string | undefined { return undefined; } + const expanded = resolveLeadingParameterExpansion(trimmedCommand); + if (expanded !== undefined) { + return expanded.split(/[\\/]/).pop(); + } + try { const tokens = parse(trimmedCommand).filter( (token): token is string => typeof token === 'string', From 917ebee64076d62963c7986f2c264a3d5fe93d5f Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 18 Jul 2026 11:26:47 +0800 Subject: [PATCH 2/2] fix(core): field-split an unquoted expansion the way the shell does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review caught the claimed-but-unmodeled half: an UNQUOTED expansion field-splits before command lookup. With the variable unset, the shell removes the empty expansion and runs the next word — this helper returned no root and the valid command stayed hard-refused; with a multi-word value, the shell's command is the first field — this helper reported the whole value as the root. Both verified against real bash. Quoting now decides, as in the shell: a quoted expansion is one word (empty = an empty command name, refusable); an unquoted one splits, an empty result falls through to the next token, and the first field names the root. The regex backreference also stops accepting asymmetric quoting. Both branches are mutation-checked: dropping the empty-removal or the split turns the new test red. --- packages/core/src/utils/shell-utils.test.ts | 19 ++++++++++++ packages/core/src/utils/shell-utils.ts | 34 +++++++++++++++------ 2 files changed, 44 insertions(+), 9 deletions(-) diff --git a/packages/core/src/utils/shell-utils.test.ts b/packages/core/src/utils/shell-utils.test.ts index 187ee904e0f..180ddca1a50 100644 --- a/packages/core/src/utils/shell-utils.test.ts +++ b/packages/core/src/utils/shell-utils.test.ts @@ -441,6 +441,25 @@ describe('getCommandRoot — parameter expansion in command position', () => { expect(getCommandRoot(`"$${NAME}" review foo`)).toBeUndefined(); }); + it('field-splits an UNQUOTED expansion the way the shell does', () => { + // Both cases verified against real bash. Unset: `$VAR printf OK` removes + // the empty expansion and runs `printf` — returning no root here would + // hard-refuse a command the shell executes fine. Multi-word: with + // VAR='/usr/bin/env printf', the shell's command is `env` after splitting; + // reporting 'env printf' would show the wrong permission root. + expect(getCommandRoot(`$${NAME} printf OK`)).toBe('printf'); + expect(getCommandRoot(`\${${NAME}} printf OK`)).toBe('printf'); + process.env[NAME] = '/usr/bin/env printf'; + expect(getCommandRoot(`$${NAME} OK`)).toBe('env'); + // Quoting suppresses splitting: the whole value is one (unrunnable) word, + // and the root is its basename — faithful to what the shell would exec. + expect(getCommandRoot(`"$${NAME}" OK`)).toBe('env printf'); + }); + + it('an empty unquoted expansion with nothing after it still has no root', () => { + expect(getCommandRoot(`$${NAME}`)).toBeUndefined(); + }); + it('skips leading env assignments before the expansion, like the plain path', () => { process.env[NAME] = '/repo/scripts/dev.js'; expect(getCommandRoot(`FOO=1 "\${${NAME}:-qwen}" review foo`)).toBe( diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index 6a5051f7b8f..76073f83345 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -338,13 +338,17 @@ export function splitCommands(command: string): string[] { * user" until the model hand-resolved the variable itself. * * Resolution mirrors POSIX: `:-` substitutes the default when the variable is - * unset OR empty; `-` only when unset; a bare `$VAR` that resolves to nothing - * yields no root, and the command stays refusable — there is nothing to name. - * The environment consulted is this process's own, which is what the spawned - * shell inherits. + * unset OR empty; `-` only when unset. Quoting then decides what happens, + * exactly as it does in the shell. A QUOTED expansion is one word: empty means + * an empty command name, which has nothing to name and stays refusable. An + * UNQUOTED expansion field-splits: an empty result is REMOVED and the next + * token is the command (`$VAR printf OK` with VAR unset runs `printf`), and a + * multi-word value's FIRST field is the command (`$VAR OK` with + * VAR='/usr/bin/env printf' runs `env`). The environment consulted is this + * process's own, which is what the spawned shell inherits. */ const PARAMETER_EXPANSION_COMMAND = - /^"?\$(?:\{([A-Za-z_][A-Za-z0-9_]*)(?:(:?-)([^}]*))?\}|([A-Za-z_][A-Za-z0-9_]*))"?$/; + /^("?)\$(?:\{([A-Za-z_][A-Za-z0-9_]*)(?:(:?-)([^}]*))?\}|([A-Za-z_][A-Za-z0-9_]*))\1$/; function resolveLeadingParameterExpansion(command: string): string | undefined { let rest = command; @@ -359,15 +363,27 @@ function resolveLeadingParameterExpansion(command: string): string | undefined { if (!head) return undefined; const m = PARAMETER_EXPANSION_COMMAND.exec(head.token); if (!m) return undefined; - const name = (m[1] ?? m[4]) as string; - const op = m[2]; + const quoted = m[1] === '"'; + const name = (m[2] ?? m[5]) as string; + const op = m[3]; const value = process.env[name]; const useDefault = op === undefined ? false : op === ':-' ? !value : value === undefined; const resolved = useDefault - ? stripSymmetricQuotes(m[3] ?? '').value + ? stripSymmetricQuotes(m[4] ?? '').value : (value ?? ''); - return resolved || undefined; + if (quoted) { + // One word, splitting suppressed — empty is an empty command name. + return resolved || undefined; + } + // Unquoted: the shell field-splits the expansion before command lookup. + const fields = resolved.split(/[ \t\n]+/).filter(Boolean); + if (fields.length === 0) { + // The empty expansion is removed; the command is whatever follows it. + const next = head.rest.trim(); + return next ? getCommandRoot(next) : undefined; + } + return fields[0]; } /**