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

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

@@ -704,6 +704,147 @@ 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',
+    ]);
+  });
+
+  // `#` 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',
+    ]);
+  });
 });
 
 // ─── splitCompoundCommandSegments ────────────────────────────────────────────
@@ -2396,6 +2537,65 @@ 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');
+    });
   });
 
   describe('file path evaluation', () => {

packages/core/src/permissions/rule-parser.ts

@@ -863,6 +863,51 @@ function isAsyncOperator(command: string, index: number): boolean {
   return true;
 }
 
+/**
+ * Characters that end a bash word, so a `#` after one of them starts a word and
+ * so a comment: the three default `IFS` whitespace characters plus the operators
+ * that separate commands. The start of the input is handled by
+ * {@link isCommentStart} itself.
+ *
+ * The whitespace half is an explicit list rather than `\s`. JavaScript's `\s`
+ * also matches `\r`, `\v`, `\f` and `\u00a0`, and bash treats none of those as
+ * word separators: `bash -xc "$(printf 'echo a\r#c ; echo DANGER')"` traces two
+ * commands, because the `#` is mid-word and literal while the `;` after it is a
+ * real boundary. Reading that `#` as a comment folded the line into one segment,
+ * so the command bash ran second never got its own rule check.
+ *
+ * Confirmed against bash for the operators: `echo a;#c` and `echo a&#c` each run
+ * one command, and `echo a|#c` is a syntax error because the `#` comments out
+ * the right-hand side of the pipe. Redirection operators and `(` are
+ * deliberately absent: bash starts a word there too, but leaving them literal
+ * only over-splits, which stays fail-closed, and no reported shape needs them.
+ */
+const COMMENT_WORD_BOUNDARIES = [' ', '\t', '\n', ';', '&', '|'];
+
+/**
+ * Whether the `#` at `index` opens a comment rather than being a literal.
+ *
+ * bash only starts a comment at the start of a word, so `echo a#b` prints
+ * `a#b` while `echo a #b` prints `a`. The same word rule keeps the arithmetic
+ * base prefix in `echo $(( 8#17 ))` literal.
+ *
+ * The boundary check is escape-aware: a backslash-escaped space or operator
+ * does not end a word, so the `#` after `a\ ` or `a\;` is mid-word and
+ * literal — treating it as a comment would swallow the rest of the line and
+ * merge a real `;` boundary away, dropping the rule check the following
+ * command would have got.
+ */
+function isCommentStart(command: string, index: number): boolean {
+  if (index === 0) {
+    return true;
+  }
+  const previous = command[index - 1]!;
+  return (
+    COMMENT_WORD_BOUNDARIES.includes(previous) &&
+    precedingBackslashCount(command, index - 1) % 2 === 0
+  );
+}
+
 /**
  * One segment of a compound command, together with the operator that ended it.
  */
@@ -919,6 +964,24 @@ export function splitCompoundCommandSegments(
       continue;
     }
 
+    // A word-initial `#` opens a comment that runs to the end of the physical
+    // line, so an operator inside it is not a boundary: bash runs
+    // `echo 'a' # comment ; echo B` as a single `echo` (#11815).
+    //
+    // This scan deliberately ignores `escaped`. A backslash does not continue a
+    // line inside a comment — bash really does run the `rm` in `echo hi # foo \`
+    // followed by a newline and `rm -rf /` — so honouring the escape would fold
+    // that second command into the comment's segment and cost it its own rule
+    // check. For the same reason the newline is left for the operator scan
+    // below, and stays a boundary.
+    if (ch === '#' && isCommentStart(command, i)) {
+      while (i < command.length && command[i] !== '\n') {
+        i++;
+      }
+      i--; // -1 because the loop will i++, landing back on the newline
+      continue;
+    }
+
     if (ch === '(' && command[i + 1] === '(') {
       arithmeticDepth++;
       i++;
@@ -971,6 +1034,7 @@ export function splitCompoundCommandSegments(
  *   "git status && rm -rf /"  → ["git status", "rm -rf /"]
  *   "ls -la | grep foo"      → ["ls -la", "grep foo"]
  *   "echo 'a && b'"          → ["echo 'a && b'"]  (inside quotes)
+ *   "echo 'a' # c ; echo B"  → ["echo 'a' # c ; echo B"]  (inside a comment)
  *   "a && b || c"            → ["a", "b", "c"]
  *   "git status & rm -rf /"  → ["git status", "rm -rf /"]  (async operator)
  *   "build &> log.txt"       → ["build &> log.txt"]  (redirection, not async)