fix(core): resolve a parameter expansion in command position to its command root

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']);

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',