fix(core): keep whitespace-led Bash comments on the conservative split

`splitCommandForRules` only refused to collapse a `#` sitting at index 0, so any leading whitespace still put the whole line on the comment fast path: `' # noop ; rm -rf /tmp/x'` became one segment that no `Bash(...)` rule can match, and an explicit `deny` rule silently stopped applying even though the index-0 spelling of the same text kept matching. Bash executes nothing for either spelling, so the defect is the permission layer certifying against the user's own rule on incidental whitespace. Require non-whitespace code before the `#`, and keep both design-doc twins (EN + zh-CN, per AGENTS.md) stating the widened precondition and its acceptance criterion. Test-only follow-ups in the same file: - pin the three other Bash-rule consumers on the same segmentation decision (`findMatchingDenyRule`, `hasRelevantRules`, `hasMatchingAskRule`), each bash arm paired with the `cmd` arm that still expects the split, so reverting any one call site to `splitCompoundCommand` goes red; - title each row of the comment table by its command string, so a regression names the flipped input instead of repeating one label 13 times; - hoist the `shellTypeMock` reset to file scope, covering the five later top-level describes that build real PermissionManagers from the same file-global singleton. Co-authored-by: Qwen-Coder <[email protected]> Patrol-Run: qwen-pr-closeout/jmu6a0kjc2c
yiliang114 · Sep 19, 2026 · Sep 17, 2026 · Sep 17, 2026 · Sep 17, 2026 · Sep 17, 2026
commit f17f500f556defc9e4047c0282d321ae92e2fa74
diff --git a/docs/design/safe-bash-comment-splitting.md b/docs/design/safe-bash-comment-splitting.md
@@ -34,7 +34,7 @@ The wrapper keeps the original command as one segment only when all of these are
 - the tool is `run_shell_command`, so the scanned string is literally the text the shell will execute;
 - the active shell is `bash`;
 - the command is one physical line;
-- a `#` outside quotes starts after a space or tab, never at index 0 — a segment that begins with `#` can no longer match any `Bash(...)` rule, so collapsing it would silently drop an explicit user rule;
+- a `#` outside quotes starts after a space or tab and has non-whitespace code before it — a segment whose only content before that `#` is whitespace is entirely a comment, so it can no longer match any `Bash(...)` rule and collapsing it would silently drop an explicit user rule;
 - the code before that `#` contains no shell operator, escape, expansion, substitution, grouping, or redirection syntax.
 
 Every other input uses the existing splitter unchanged. Unsupported syntax can therefore retain an extra prompt, but it cannot gain a broader allow decision from this change.
@@ -48,6 +48,6 @@ The supported subset is intentionally narrow. Widening it requires evidence agai
 - The #11815 command is one segment under Bash and an allowed `echo` resolves to `allow`.
 - The same text remains split for `cmd` and PowerShell.
 - Multi-line commands, commands containing substitution syntax, and commands with an operator before the comment retain the old conservative split.
-- A command whose first character is `#` also retains it, so an explicit `deny` rule still matches the text after the comment.
+- A command whose first non-whitespace character is `#` — at index 0 or behind leading spaces/tabs — also retains it, so an explicit `deny` rule still matches the text after the comment.
 - A `monitor` command whose `#` only exists inside the wrapper's inner quotes still splits, so a separator the spawned command executes is never swallowed as comment text.
 - Existing permission-manager tests, formatting, lint, typecheck, and build checks pass.
diff --git a/docs/design/safe-bash-comment-splitting.zh-CN.md b/docs/design/safe-bash-comment-splitting.zh-CN.md
@@ -34,7 +34,7 @@
 - 工具是 `run_shell_command`，即被扫描的字符串正是 shell 将执行的文本；
 - 当前 shell 是 `bash`；
 - 命令只有一个物理行；
-- 引号外的 `#` 位于空格或制表符之后，且不在下标 0——以 `#` 开头的 segment 无法再匹配任何 `Bash(...)` 规则，折叠它会静默丢掉用户显式配置的规则；
+- 引号外的 `#` 位于空格或制表符之后，且其前面存在非空白代码——若 `#` 之前只有空白，整个 segment 就是一条注释，无法再匹配任何 `Bash(...)` 规则，折叠它会静默丢掉用户显式配置的规则；
 - `#` 之前的代码不包含 shell operator、转义、展开、substitution、分组或重定向语法。
 
 其他所有输入均原样使用现有切分器。因此，不支持的语法可以继续多弹一次确认，但不会因为本次改动获得更宽松的 allow 判定。
@@ -48,6 +48,6 @@
 - #11815 的命令在 Bash 下只有一个 segment，允许的 `echo` 判定为 `allow`。
 - 同一段文本在 `cmd` 和 PowerShell 下仍会切分。
 - 多行命令、包含 substitution 语法的命令，以及注释前存在 operator 的命令保持旧的保守切分。
-- 首字符是 `#` 的命令同样保持旧的保守切分，因此显式 `deny` 规则仍能匹配注释之后的文本。
+- 首个非空白字符是 `#` 的命令（无论位于下标 0 还是在前导空格/制表符之后）同样保持旧的保守切分，因此显式 `deny` 规则仍能匹配注释之后的文本。
 - `monitor` 命令中只存在于 wrapper 内层引号里的 `#` 仍会切分，因此 spawned 命令真正执行的分隔符不会被当成注释吞掉。
 - 现有 permission-manager 测试、格式化、lint、typecheck 和 build 检查通过。
@@ -60,6 +60,15 @@ vi.mock('../utils/shell-utils.js', async (importOriginal) => {
   };
 });
 
+// `shellTypeMock` backs the file-level `vi.mock` above, so it is a mutable
+// file-global that every describe building a real PermissionManager reads.
+// Reset it for the whole file rather than only inside
+// `describe('PermissionManager')`, so the later top-level describes cannot
+// inherit a shell type left behind by an earlier test's execution order.
+beforeEach(() => {
+  shellTypeMock.value = 'bash';
+});
+
 // ─── getToolNameAliases ──────────────────────────────────────────────────────
 
 describe('getToolNameAliases', () => {
@@ -1771,10 +1780,6 @@ function makeConfig(
 describe('PermissionManager', () => {
   let pm: PermissionManager;
 
-  beforeEach(() => {
-    shellTypeMock.value = 'bash';
-  });
-
   describe('basic rule evaluation', () => {
     beforeEach(() => {
       pm = new PermissionManager(
@@ -2376,14 +2381,20 @@ describe('PermissionManager', () => {
       ['bash', 'echo a#b ; rm -rf /tmp/x', 'deny'],
       ['bash', 'echo a\v# comment ; rm -rf /tmp/x', 'deny'],
       ['bash', '# noop ; rm -rf /tmp/x', 'deny'],
+      // Leading whitespace must not turn the line into one comment-only
+      // segment: Bash executes nothing for either spelling, but collapsing it
+      // leaves no text for an explicit `Bash(...)` rule to match.
+      ['bash', ' # noop ; rm -rf /tmp/x', 'deny'],
+      ['bash', '\t# noop ; rm -rf /tmp/x', 'deny'],
       ['bash', "echo 'a # b' ; rm -rf /tmp/x", 'deny'],
       ['bash', 'echo "a # b" ; rm -rf /tmp/x', 'deny'],
       ['bash', 'echo hi > /tmp/o # c ; rm -rf /tmp/x', 'deny'],
       ['bash', 'echo hi | tee /tmp/o # c ; rm -rf /tmp/x', 'deny'],
       ['bash', 'echo hi # c\r; rm -rf /tmp/x', 'deny'],
       ['bash', 'echo hi\t# comment ; rm -rf /tmp/x', 'allow'],
+      ['bash', ' echo hi # comment ; rm -rf /tmp/x', 'allow'],
     ] as const)(
-      'handles comments conservatively for %s',
+      'handles comments conservatively for %s: %s',
       async (shell, command, expected) => {
         shellTypeMock.value = shell;
         pm = new PermissionManager(
@@ -2432,6 +2443,51 @@ describe('PermissionManager', () => {
       },
     );
 
+    // `splitCommandForRules` has to drive every Bash-rule consumer, not just
+    // `evaluate()`. Each of the three below re-splits the command on its own
+    // path, so reverting any one of them to `splitCompoundCommand` would leave
+    // it silently disagreeing with `evaluate()` — citing a deny rule evaluate
+    // never applied, or hiding "Always allow" for a command that is allowed —
+    // while the rest of the suite stayed green. Every assertion pairs the bash
+    // arm (comment recognised → one segment) with the cmd arm (no Bash
+    // comments → the conservative split is expected).
+    const commented = `echo 'a' # comment ; rm -rf /tmp/x`;
+
+    const buildPm = (
+      shell: 'bash' | 'cmd',
+      rules: {
+        permissionsAllow?: string[];
+        permissionsAsk?: string[];
+        permissionsDeny?: string[];
+      },
+    ) => {
+      shellTypeMock.value = shell;
+      const manager = new PermissionManager(makeConfig(rules));
+      manager.initialize();
+      return manager;
+    };
+
+    it('findMatchingDenyRule does not cite a rule the comment hid', () => {
+      const ctx = { toolName: 'run_shell_command', command: commented };
+      const deny = { permissionsDeny: ['Bash(rm *)'] };
+      expect(buildPm('bash', deny).findMatchingDenyRule(ctx)).toBeUndefined();
+      expect(buildPm('cmd', deny).findMatchingDenyRule(ctx)).toBe('Bash(rm *)');
+    });
+
+    it('hasRelevantRules drops the segment the comment hid', () => {
+      const ctx = { toolName: 'run_shell_command', command: commented };
+      const deny = { permissionsDeny: ['Bash(rm *)'] };
+      expect(buildPm('bash', deny).hasRelevantRules(ctx)).toBe(false);
+      expect(buildPm('cmd', deny).hasRelevantRules(ctx)).toBe(true);
+    });
+
+    it('hasMatchingAskRule does not ask for a rule the comment hid', () => {
+      const ctx = { toolName: 'run_shell_command', command: commented };
+      const ask = { permissionsAsk: ['Bash(rm *)'] };
+      expect(buildPm('bash', ask).hasMatchingAskRule(ctx)).toBe(false);
+      expect(buildPm('cmd', ask).hasMatchingAskRule(ctx)).toBe(true);
+    });
+
     it('three-part compound: all must pass', async () => {
       pm = new PermissionManager(
         makeConfig({

@@ -110,10 +110,12 @@ function splitCommandForRules(command: string, toolName: string): string[] {
       ch === '#' &&
       !inSingle &&
       !inDouble &&
-      // Bash only treats ASCII space and tab as word boundaries here. A
-      // leading `#` is deliberately not collapsed: the whole segment would
-      // start with `#`, so no `Bash(...)` rule could match it any more and an
-      // explicit user rule would silently stop applying.
+      // Bash only treats ASCII space and tab as word boundaries here. A `#`
+      // with nothing but whitespace before it is deliberately not collapsed —
+      // whether it sits at index 0 or behind leading spaces/tabs: the whole
+      // segment would be a comment, so no `Bash(...)` rule could match it any
+      // more and an explicit user rule would silently stop applying.
+      command.slice(0, i).trim() !== '' &&
       (command[i - 1] === ' ' || command[i - 1] === '\t')
     ) {
       return [command];