fix(core): treat a word-initial # as a comment when splitting shell commands

packages/core/src/permissions/permission-manager.test.ts

@@ -704,6 +704,232 @@ describe('splitCompoundCommand', () => {
       expect(splitCompoundCommand(command)).toEqual(['echo hi']);
     },
   );
+
+  // A word-initial `#` outside quotes opens a comment that runs to the end of
+  // the physical line, so every operator inside it is inert. Confirmed against
+  // bash: each of these traces a single `+ echo a`, and
+  // `bash -xc "echo 'a' # comment ; echo B"` traces a single `+ echo a`.
+  //
+  // The `'a\'` rows passed before the comment state existed too, but only by
+  // accident — the backslash inside the quotes was read as an escape that
+  // swallowed the closing `'`, so the scanner sat in an unterminated string
+  // that masked the operator. They keep passing once #11765 makes that
+  // backslash literal, because the comment then masks the operator for the
+  // right reason.
+  it.each([
+    [`echo 'a' # comment ; echo B`],
+    [`echo 'a\\' # note: use ; carefully`],
+    [`echo 'a\\' # trailing && touch /tmp/x`],
+    [`echo 'a\\' # trailing | touch /tmp/x`],
+    ['echo hi # nothing here'],
+    [`git status # don't ; echo B`],
+    ['echo a # c && rm -rf /tmp/x'],
+    ['echo a # c || rm -rf /tmp/x'],
+    ['echo a # c | rm -rf /tmp/x'],
+    ['echo a # c & rm -rf /tmp/x'],
+  ])(
+    'does not split %s, where the operator is inside a comment',
+    async (command) => {
+      expect(splitCompoundCommand(command)).toEqual([command]);
+    },
+  );
+
+  // Over-correction guard: `#` starts a comment only at the start of a word, so
+  // bash really does run two commands here.
+  it('still splits echo a#b ; echo B, where # is not at a word start', async () => {
+    expect(splitCompoundCommand('echo a#b ; echo B')).toEqual([
+      'echo a#b',
+      'echo B',
+    ]);
+  });
+
+  // Same guard for the whitespace side. bash's word separators are the three
+  // default `IFS` characters — space, tab, newline — and not JavaScript's `\s`,
+  // which also matches `\r`, `\v`, `\f` and `\u00a0`. After any of those four the
+  // `#` is mid-word and literal, so the `;` stays a real boundary and bash runs
+  // both commands: `bash -xc "$(printf 'echo a\r#c ; echo DANGER')"` traces
+  // `+ echo $'a\r#c'` and then `+ echo DANGER`. Reading the `#` as a comment
+  // folded the line into one segment, so the second command lost its rule check.
+  it.each([
+    [
+      'a carriage return',
+      'echo a\r#c ; echo DANGER',
+      ['echo a\r#c', 'echo DANGER'],
+    ],
+    [
+      'a vertical tab',
+      'echo a\v#c ; rm -rf /tmp/x',
+      ['echo a\v#c', 'rm -rf /tmp/x'],
+    ],
+    [
+      'a form feed',
+      'echo a\f#c ; rm -rf /tmp/x',
+      ['echo a\f#c', 'rm -rf /tmp/x'],
+    ],
+    [
+      'a no-break space',
+      'echo a\u00a0#c ; rm -rf /tmp/x',
+      ['echo a\u00a0#c', 'rm -rf /tmp/x'],
+    ],
+  ])(
+    'still splits after %s, which does not end a bash word',
+    async (_separator, command, expected) => {
+      expect(splitCompoundCommand(command)).toEqual(expected);
+    },
+  );
+
+  // An escaped boundary does not end a word: `bash -xc 'echo a\ #b ; echo B'`
+  // traces `+ echo a #b` then `+ echo B` — the `#` is mid-word and literal.
+  // Reading it as a comment would swallow the `;` boundary and drop the second
+  // command's rule check.
+  it('does not read a comment after an escaped space', async () => {
+    expect(splitCompoundCommand('echo a\\ #b ; rm -rf /tmp/x')).toEqual([
+      'echo a\\ #b',
+      'rm -rf /tmp/x',
+    ]);
+  });
+
+  it('does not read a comment after an escaped operator', async () => {
+    expect(splitCompoundCommand('echo a\\;#c ; rm -rf /tmp/x')).toEqual([
+      'echo a\\;#c',
+      'rm -rf /tmp/x',
+    ]);
+  });
+
+  // A word starts after an operator as well as after whitespace:
+  // `bash -xc "echo a;#c ; rm -rf /tmp/x"` traces a single `+ echo a`. The
+  // leftover `#c …` segment is pre-existing behaviour — bash runs one command
+  // here, not two — and is harmless in the fail-closed direction.
+  it('reads a # straight after an operator as a comment', async () => {
+    expect(splitCompoundCommand('echo a;#c ; rm -rf /tmp/x')).toEqual([
+      'echo a',
+      '#c ; rm -rf /tmp/x',
+    ]);
+  });
+
+  // Inside quotes a `#` is literal, so the operator after the closing quote is
+  // a real boundary.
+  it('does not treat a quoted # as a comment', async () => {
+    expect(splitCompoundCommand(`echo '# c' ; rm -rf /tmp/x`)).toEqual([
+      `echo '# c'`,
+      'rm -rf /tmp/x',
+    ]);
+  });
+
+  // The row above pins nothing about branch *order*: its `#` is preceded by `'`,
+  // which is not a word boundary, so `isCommentStart` returns false there no
+  // matter where the comment block sits. A quoted `#` preceded by a space stays
+  // literal only because the quote check runs first — hoisting the comment block
+  // above `if (inSingle || inDouble) { continue; }` (a reorder that leaves this
+  // whole file green) folds these two into one segment that `Bash(echo *)`
+  // covers, while `bash -xc 'echo "a # b" ; echo DANGER'` runs both commands.
+  it.each([
+    ['echo "a # b" ; rm -rf /tmp/x', 'echo "a # b"'],
+    ["echo 'a # b' ; rm -rf /tmp/x", "echo 'a # b'"],
+  ])(
+    'does not read the space-preceded quoted # in %s as a comment',
+    async (command, head) => {
+      expect(splitCompoundCommand(command)).toEqual([head, 'rm -rf /tmp/x']);
+    },
+  );
+
+  // `#` at index 0 is at a word start, so a shebang line is a comment and the
+  // newline after it still bounds the real command.
+  it('treats a leading # as a comment', async () => {
+    expect(splitCompoundCommand('#!/bin/sh\necho hi')).toEqual([
+      '#!/bin/sh',
+      'echo hi',
+    ]);
+  });
+
+  // The comment ends at the *physical* newline, and that newline stays a
+  // boundary: `bash -xc $'echo hi # c\nrm -rf /tmp/x'` traces both commands, so
+  // the `rm` must keep its own segment and its own rule check.
+  it('keeps the newline that ends a comment as a boundary', async () => {
+    expect(splitCompoundCommand('echo hi # c\nrm -rf /tmp/x')).toEqual([
+      'echo hi # c',
+      'rm -rf /tmp/x',
+    ]);
+  });
+
+  // …and a backslash does not reach past that newline. bash does not continue a
+  // line inside a comment, so it runs the `rm` here; letting the existing
+  // `escaped` flag find the newline folded both commands into one segment and
+  // the `rm` lost its rule check.
+  it('does not let a trailing backslash extend a comment', async () => {
+    expect(splitCompoundCommand('echo hi # foo \\\nrm -rf /tmp/x')).toEqual([
+      'echo hi # foo \\',
+      'rm -rf /tmp/x',
+    ]);
+  });
+
+  // A space-preceded `#` inside a `${ … }` parameter expansion is literal: bash
+  // does no comment recognition there, so the `;` stays a real boundary and the
+  // tail runs — `bash -xc 'x=""; echo ${x:- a #b} ; touch m ; echo SECOND'`
+  // traces `+ echo a '#b'`, `+ touch m` and `+ echo SECOND`. Folding the line
+  // here cost the tail its own rule check and hid it from the virtual-op pass.
+  it('does not read a # inside a parameter expansion as a comment', async () => {
+    expect(splitCompoundCommand('echo ${x:- a #b} ; rm -rf /tmp/x')).toEqual([
+      'echo ${x:- a #b}',
+      'rm -rf /tmp/x',
+    ]);
+  });
+
+  // Over-correction guard for the same depth: it is scoped to the expansion.
+  // After the closing `}` the `#` is at a word start again, while bash keeps the
+  // `#` in `${x#pre}` and `${#x}` literal; both split today and must keep doing
+  // so, or the guard would over-split every `#` that follows a `${ … }`.
+  it.each([
+    ['echo ${x} # c ; rm -rf /tmp/x', ['echo ${x} # c ; rm -rf /tmp/x']],
+    ['echo ${x#pre} ; rm -rf /tmp/x', ['echo ${x#pre}', 'rm -rf /tmp/x']],
+    ['echo ${#x} ; rm -rf /tmp/x', ['echo ${#x}', 'rm -rf /tmp/x']],
+  ])(
+    'keeps the # semantics of %s around a parameter expansion',
+    async (command, expected) => {
+      expect(splitCompoundCommand(command)).toEqual(expected);
+    },
+  );
+
+  // bash performs no comment recognition inside `(( … ))` / `$(( … ))`, so the
+  // `#` stays literal and `))` still closes the expansion. Swallowing the `))`
+  // additionally stranded the arithmetic depth at 1, after which no later bare
+  // `&` was a boundary. `bash -xc '(( 1 #c )) ; touch m'` prints an arithmetic
+  // error and still runs the `touch`, and `bash -xc 'echo $(( 1 #c )) & touch
+  // m'` creates the marker in the backgrounded subshell.
+  it('does not read a # inside an arithmetic expansion as a comment', async () => {
+    expect(splitCompoundCommand('echo $(( 1 #c )) & rm -rf /tmp/x')).toEqual([
+      'echo $(( 1 #c ))',
+      'rm -rf /tmp/x',
+    ]);
+  });
+
+  it('does not read a # inside an arithmetic command as a comment', async () => {
+    expect(splitCompoundCommand('(( 1 #c )) ; rm -rf /tmp/x')).toEqual([
+      '(( 1 #c ))',
+      'rm -rf /tmp/x',
+    ]);
+  });
+
+  // Over-correction guard: a comment must never be able to leave the arithmetic
+  // depth raised, or every later bare `&` stops being a boundary — including on
+  // the following physical line.
+  it('keeps a later & a boundary after a # inside arithmetic', async () => {
+    expect(
+      splitCompoundCommand('echo $(( 1 # c ))\necho a & rm -rf /tmp/x'),
+    ).toEqual(['echo $(( 1 # c ))', 'echo a', 'rm -rf /tmp/x']);
+  });
+
+  // bash finds a backtick body's closing delimiter with a raw scan that does not
+  // honour an outer-level `#`, so the tail really runs: the oracle
+  // `bash -xc 'echo `date # c` ; echo SECOND'` traces `++ date`, `+ echo …` and
+  // `+ echo SECOND`. The comment skip therefore has to stop at the backtick so
+  // the operator after it keeps its own segment and its own rule check.
+  it('does not let a comment swallow a closing backtick', async () => {
+    expect(splitCompoundCommand('echo `date # c` ; rm -rf /tmp/x')).toEqual([
+      'echo `date # c`',
+      'rm -rf /tmp/x',
+    ]);
+  });
 });
 
 // ─── splitCompoundCommandSegments ────────────────────────────────────────────
@@ -2396,6 +2622,91 @@ describe('PermissionManager', () => {
         'deny',
       );
     });
+
+    // #11815 — bash runs only the `echo` here, and `Bash(echo *)` covers it, so
+    // the commented-out tail must not drag the whole line into a prompt.
+    it('operator inside a comment: treated as single command', async () => {
+      pm = new PermissionManager(
+        makeConfig({
+          permissionsAllow: ['Bash(echo *)'],
+        }),
+      );
+      pm.initialize();
+      expect(
+        await pm.evaluate({
+          toolName: 'run_shell_command',
+          command: `echo 'a' # comment ; rm -rf /tmp/x`,
+        }),
+      ).toBe('allow');
+    });
+
+    // The comment stops at the physical newline and bash runs the `rm` on the
+    // next line, so the backslash before that newline must not extend the
+    // comment over it and cost the `rm` its deny check.
+    it('command after a comment line: deny still fires', async () => {
+      pm = new PermissionManager(
+        makeConfig({
+          permissionsDeny: ['Bash(rm *)'],
+        }),
+      );
+      pm.initialize();
+      expect(
+        await pm.evaluate({
+          toolName: 'run_shell_command',
+          command: 'echo hi # foo \\\nrm -rf /tmp/x',
+        }),
+      ).toBe('deny');
+    });
+
+    // Only `walkCompoundCommand` strips heredoc bodies; the Bash-rule paths
+    // split the raw command, so this `#` line reaches the comment state. bash
+    // hands that line to `cat` as data and never runs the `rm` —
+    // `bash -xc $'cat <<EOF\n# hi ; rm -rf /\nEOF'` traces only `+ cat` — so the
+    // deny that fired before this change was a false positive on text that is
+    // never executed. Making these paths heredoc-aware is #9417's job, not
+    // this change's.
+    it('heredoc body comment line: no deny on text bash never runs', async () => {
+      pm = new PermissionManager(
+        makeConfig({
+          permissionsDeny: ['Bash(rm *)'],
+        }),
+      );
+      pm.initialize();
+      // `cat` with a heredoc has no rule hitting it; the rm text is data for
+      // cat, not a command. Name the verdict rather than only ruling deny out.
+      expect(
+        await pm.evaluate({
+          toolName: 'run_shell_command',
+          command: 'cat <<EOF\n# hi ; rm -rf /\nEOF',
+        }),
+      ).toBe('ask');
+    });
+
+    // The comment state reads a `#` literal wherever bash does not tokenize a
+    // comment — inside `${ … }`, inside `$(( … ))` / `(( … ))`, and inside a
+    // backtick substitution — so the `;` or `&` after the closing delimiter
+    // stays a real boundary. Folding these lines into one segment let
+    // `Bash(echo *)` cover the tail, turning a configured deny into an allow.
+    it.each([
+      ['a parameter expansion', 'echo ${x:- a #b} ; rm -rf /tmp/x'],
+      ['an arithmetic expansion', 'echo $(( 1 #c )) & rm -rf /tmp/x'],
+      [
+        'an arithmetic command',
+        'echo NPMTEST ; (( failures #c )) ; rm -rf /tmp/x',
+      ],
+      ['a backtick substitution', 'echo `date # c` ; rm -rf /tmp/x'],
+    ])('comment-like # in %s: deny still fires', async (_where, command) => {
+      pm = new PermissionManager(
+        makeConfig({
+          permissionsAllow: ['Bash(echo *)'],
+          permissionsDeny: ['Bash(rm *)'],
+        }),
+      );
+      pm.initialize();
+      expect(
+        await pm.evaluate({ toolName: 'run_shell_command', command }),
+      ).toBe('deny');
+    });
   });
 
   describe('file path evaluation', () => {