refactor(core): Classify shell safety as read-only, write, or unknown

docs/design/shell-safety-classification.md

@@ -0,0 +1,47 @@
+# Shell safety classification
+
+## Context and scope
+
+Issue [#6949](https://github.com/QwenLM/qwen-code/issues/6949) requires Plan mode to distinguish commands that are proven read-only from commands whose behavior cannot be established statically. A boolean cannot retain that distinction, so this change introduces a three-state fact layer in `shellAstParser.ts` without changing permission routing.
+
+This change does not modify routing or call-site logic in Shell, Monitor, PermissionManager, speculation, memory-scoped agents, ACP, Plan-mode prompts, or Plan exit behavior. Existing boolean consumers can become more conservative where the classifier is hardened. A follow-up change can route `unknown` commands to one-off approval using the new fact without changing this classifier.
+
+## Contract
+
+`classifyShellCommandSafety(command)` is an internal module API with these results:
+
+| Result      | Meaning                                                                                                                          |
+| ----------- | -------------------------------------------------------------------------------------------------------------------------------- |
+| `read-only` | Every executable path is proven by the current rules not to modify persistent or external state.                                 |
+| `write`     | The syntax contains positive evidence of a file, Git, process, or other state mutation. The command need not ultimately succeed. |
+| `unknown`   | The command cannot be proved safe or mutating by the supported static rules.                                                     |
+
+For a valid AST, results combine in the order `write > unknown > read-only`. A tree containing `ERROR` is classified as `unknown` before evaluating partial syntax. Command and process substitutions impose an `unknown` floor while their executable contents are scanned, so a nested known writer promotes the result to `write`. Redirect analysis owns substitutions inside redirect nodes while command and statement evaluators exclude those nodes from their substitution scans, preventing repeated traversal of nested substitutions. Control flow uses the same unknown floor and scans possible branches. A function definition is not execution and therefore remains `unknown` without classifying its body as an executed write.
+
+A standalone pure assignment and `cd` preserve the existing compatibility behavior. An assignment that prefixes a command or shares a compound sequence with another statement imposes an `unknown` floor because variables such as `LD_PRELOAD`, `PATH`, `PAGER`, or tool-specific configuration can change behavior; explicit write evidence still wins. Subshells and command groups aggregate their executed contents. The API analyzes only the supplied source string; it does not unwrap `sudo` or interpreters, resolve PATH or aliases, or load shell configuration.
+
+## Parser failure and compatibility API
+
+The private classifier may throw while loading or running tree-sitter. The public three-state API maps those failures to `unknown` and never substitutes regex certainty. A parser that throws while parsing is discarded and rebuilt from the already loaded Bash language, because the failed instance may remain poisoned; this does not reload the runtime or language. The existing `isShellCommandReadOnlyAST()` compatibility API returns `true` only for `read-only`, but retains the existing regex fallback when tree-sitter cannot load or throws at runtime. A syntactically invalid tree is a normal `unknown` result, not a parser failure, so it never enters that fallback. Every successfully returned tree is released once in a `finally` block.
+
+This asymmetry is intentional: new consumers need an honest uncertainty fact, while existing boolean consumers keep their parser-availability behavior until they migrate explicitly.
+
+## Supported evidence
+
+The classifier recognizes a bounded, case-sensitive set of direct filesystem writers, process signaling commands, output redirections, Git mutation families, and explicit write modes in `find`, `sed`, `awk`, `sort`, `tree`, `uniq`, `tee`, and `dd`. Sed and AWK use shared linear scanners that distinguish inline programs from option values and file arguments, so escaped, malformed, or highly repetitive input cannot trigger regex backtracking or fabricate write evidence from a filename. Git output files for `diff`, `log`, and `show` are writes. Stateful `printf -v` forms are unknown. Explicit Git helpers and signature verification, including pager/config environment options, diff/text-conversion helpers, grep's external pager, and signature placeholders, are unknown; unsupported Git global options and subcommand help paths also fail closed because help may launch an external viewer. Dynamic execution, external scripts, ambiguous output targets, interpreters and wrappers, `sort --compress-program`, ripgrep preprocessors, hostname helpers, and archive search (`--pre`, `--hostname-bin`, `--search-zip`, and `-z`), and ordinary pager commands remain `unknown`. Option terminators and the supported options' value arity are interpreted so a filename or message literally named `--help` is not mistaken for a help invocation. Differently-cased command names, unlisted package managers, services, and custom executables also remain `unknown`; the classifier is not a sandbox.
+
+The deprecated synchronous checker mirrors every newly rejected pattern needed by synchronous scheduling. It preserves parameter expansions with sentinels instead of allowing `shell-quote` to erase them, rejects malformed trailing pipelines and assignment-bearing compounds, and evaluates wrappers from the original command. It intentionally remains boolean and is more conservative than the AST classifier: `printf`, option-heavy `sort`, `tree`, `uniq`, `rg`, and `ripgrep` commands, and Git branch forms beyond the simplest listing modes, run sequentially.
+
+## Consumers and migration boundary
+
+Current boolean consumers are Shell, Monitor, PermissionManager, the speculation gate, and memory-scoped agent configuration; their call sites do not change in this refactor. The synchronous checker is also used by the core tool scheduler and the legacy shell permission utility. The scheduler now passes the original command to the checker so wrappers remain unknown instead of being unwrapped into an apparently read-only command. `extractCommandRules()` remains independent of safety classification.
+
+The follow-up `fix(core): Route unknown Plan shell commands to one-off approval` should consume `classifyShellCommandSafety()` only at the Plan permission boundary. It must separately define approval provenance, lifetime, ACP behavior, and the interaction with Plan exit; those policies do not belong in the fact layer.
+
+## Claude Code reference
+
+Claude Code's Bash analysis is useful as evidence for two design principles: parsing uncertainty must be represented explicitly, and permission decisions must fail closed when parsing is unavailable or too complex. Its larger Bash parser and policy engine are not copied because Qwen Code needs only a small classifier at the current boundary.
+
+## Verification
+
+Unit coverage uses table-driven matrices for all three states, compound precedence, substitutions, syntax errors, parser initialization and runtime failures, bounded behavior for adversarial nested and escaped input, and compatibility monotonicity. The synchronous checker and scheduler tests prevent newly known unsafe commands from joining concurrent Shell batches.

packages/core/src/core/coreToolScheduler.test.ts

@@ -12020,8 +12020,8 @@ describe('Fire hook functions integration', () => {
         onToolCallsUpdate,
       );
 
-      // "git log" and "ls" are read-only → concurrent
-      // "npm install" is not read-only → sequential, breaks the batch
+      // "git log" and "ls" are read-only → concurrent.
+      // Wrappers, output-writing sort, and npm install are sequential.
       const requests = [
         {
           callId: '1',
@@ -12040,6 +12040,20 @@ describe('Fire hook functions integration', () => {
         {
           callId: '3',
           name: 'run_shell_command',
+          args: { command: "bash -c 'git status'" },
+          isClientInitiated: false,
+          prompt_id: 'p1',
+        },
+        {
+          callId: '4',
+          name: 'run_shell_command',
+          args: { command: 'sort -o output input' },
+          isClientInitiated: false,
+          prompt_id: 'p1',
+        },
+        {
+          callId: '5',
+          name: 'run_shell_command',
           args: { command: 'npm install' },
           isClientInitiated: false,
           prompt_id: 'p1',
@@ -12063,14 +12077,32 @@ describe('Fire hook functions integration', () => {
       expect(gitStart).toBeLessThan(firstReadOnlyEnd);
       expect(lsStart).toBeLessThan(firstReadOnlyEnd);
 
-      // "npm install" should start after both read-only commands complete
+      // The unknown wrapper should start after both reads complete.
       const lastReadOnlyEnd = Math.max(
         executionLog.indexOf('shell:end:git log'),
         executionLog.indexOf('shell:end:ls'),
       );
+      const wrapperStart = executionLog.indexOf(
+        "shell:start:bash -c 'git status'",
+      );
+      expect(wrapperStart).not.toBe(-1);
+      expect(wrapperStart).toBeGreaterThan(lastReadOnlyEnd);
+
+      // The output-writing sort should not overlap the wrapper batch.
+      const wrapperEnd = executionLog.indexOf("shell:end:bash -c 'git status'");
+      const sortStart = executionLog.indexOf(
+        'shell:start:sort -o output input',
+      );
+      expect(wrapperEnd).not.toBe(-1);
+      expect(sortStart).not.toBe(-1);
+      expect(sortStart).toBeGreaterThan(wrapperEnd);
+
+      // npm install should not overlap the sequential sort batch.
+      const sortEnd = executionLog.indexOf('shell:end:sort -o output input');
       const npmStart = executionLog.indexOf('shell:start:npm install');
+      expect(sortEnd).not.toBe(-1);
       expect(npmStart).not.toBe(-1);
-      expect(npmStart).toBeGreaterThan(lastReadOnlyEnd);
+      expect(npmStart).toBeGreaterThan(sortEnd);
     });
   });
 });

packages/core/src/core/coreToolScheduler.ts

@@ -66,7 +66,6 @@ import { unescapePath, PATH_ARG_KEYS } from '../utils/paths.js';
 import type { MemoryPressureMonitor } from '../services/memoryPressureMonitor.js';
 import { CONCURRENCY_SAFE_KINDS, isShellProgressData } from '../tools/tools.js';
 import { isShellCommandReadOnly } from '../utils/shellReadOnlyChecker.js';
-import { stripShellWrapper } from '../utils/shell-utils.js';
 import { parsePositiveIntegerEnv } from '../utils/env.js';
 import {
   isAlreadyTruncated,
@@ -1197,14 +1196,13 @@ export function isToolCallConcurrencySafe(
   if (canonicalToolName(name) === ToolNames.AGENT) return true;
   // Shell commands: check if the command is read-only (e.g., git log, cat).
   // Uses the synchronous regex+shell-quote checker (not the async AST-based
-  // one) because partitioning runs synchronously. The sync checker covers
-  // the same command whitelist and is fail-closed — unknown commands remain
-  // sequential. The AST version is used separately for permission decisions.
+  // one) because partitioning runs synchronously. It is deliberately more
+  // conservative than the AST version used for permission decisions.
   if (kind === Kind.Execute) {
     const command = (args as { command?: string } | undefined)?.command;
     if (typeof command !== 'string') return false;
     try {
-      return isShellCommandReadOnly(stripShellWrapper(command));
+      return isShellCommandReadOnly(command);
     } catch {
       return false; // fail-closed
     }

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

@@ -1486,19 +1486,15 @@ describe('PermissionManager', () => {
       ).toBe('allow');
     });
 
-    it('exact Monitor(...) allow rule matches wrapped fallback commands', async () => {
-      const pm2 = new PermissionManager(
-        makeConfig({
-          permissionsAllow: ['Monitor(FOO="bar baz" tail -f /var/log/app.log)'],
-        }),
-      );
+    it('asks by default for wrapped commands with environment prefixes', async () => {
+      const pm2 = new PermissionManager(makeConfig({}));
       pm2.initialize();
       expect(
         await pm2.evaluate({
           toolName: 'monitor',
           command: String.raw`FOO="bar baz" /bin/bash --noprofile -c 'tail -f /var/log/app.log &'`,
         }),
-      ).toBe('allow');
+      ).toBe('ask');
     });
 
     it('Monitor(...) deny rule sees shell wrapper suffix commands', async () => {

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

@@ -485,14 +485,6 @@ export class PermissionManager {
   private async resolveDefaultPermission(
     command: string,
   ): Promise<'allow' | 'ask'> {
-    // AST-based read-only detection. Commands containing command
-    // substitution are never read-only — `evaluateStatementReadOnly`
-    // (shellAstParser.ts) guards on `containsCommandSubstitutionAST` at
-    // the top so every node type inherits the check, including
-    // `variable_assignment` (`FOO=$(curl ...)`) and `redirected_statement`
-    // (`cat < $(curl ...)`) where earlier versions had blind spots. See
-    // PR #4386 round 4. So substitution-bearing commands fall through
-    // to 'ask' on the line below.
     try {
       const isReadOnly = await isShellCommandReadOnlyAST(command);
       if (isReadOnly) {

packages/core/src/utils/shell-ast-parser-lazy.test.ts

@@ -86,10 +86,23 @@ describe('shellAstParser lazy runtime', () => {
   it('loads web-tree-sitter on first use and deduplicates initialization', async () => {
     const runtimeLoaded = vi.fn();
     const init = vi.fn(async () => undefined);
+    let releaseLanguage!: () => void;
+    const languageReady = new Promise<void>((resolve) => {
+      releaseLanguage = resolve;
+    });
+    const constructed = vi.fn();
+    const loadLanguage = vi.fn(async () => {
+      await languageReady;
+      return {};
+    });
 
     class ParserMock {
       static init = init;
-      static Language = { load: vi.fn(async () => ({})) };
+      static Language = { load: loadLanguage };
+
+      constructor() {
+        constructed();
+      }
 
       setLanguage = vi.fn();
     }
@@ -101,27 +114,129 @@ describe('shellAstParser lazy runtime', () => {
 
     const parser = await import('./shellAstParser.js');
     expect(runtimeLoaded).not.toHaveBeenCalled();
+    const first = parser.initParser();
+    await vi.waitFor(() => expect(loadLanguage).toHaveBeenCalledTimes(1));
+    expect(constructed).not.toHaveBeenCalled();
 
-    await Promise.all([parser.initParser(), parser.initParser()]);
+    let secondResolved = false;
+    const second = parser.initParser().then(() => {
+      secondResolved = true;
+    });
+    await Promise.resolve();
+    expect(secondResolved).toBe(false);
 
+    releaseLanguage();
+    await Promise.all([first, second]);
     expect(runtimeLoaded).toHaveBeenCalledTimes(1);
     expect(init).toHaveBeenCalledTimes(1);
+    expect(loadLanguage).toHaveBeenCalledTimes(1);
+    expect(constructed).toHaveBeenCalledTimes(1);
   });
 
-  it('latches a runtime import failure and falls back without retrying', async () => {
-    const runtimeLoads = vi.fn();
-    vi.doMock('web-tree-sitter', () => {
-      runtimeLoads();
-      throw new Error('runtime chunk unavailable');
+  it('latches a language load failure', async () => {
+    const languageLoads = vi.fn(async () => {
+      throw new Error('bash language unavailable');
     });
 
+    class ParserMock {
+      static init = vi.fn(async () => undefined);
+      static Language = { load: languageLoads };
+
+      setLanguage = vi.fn();
+    }
+
+    vi.doMock('web-tree-sitter', () => ({ default: ParserMock }));
     const parser = await import('./shellAstParser.js');
+
+    expect(await parser.classifyShellCommandSafety('git status')).toBe(
+      'unknown',
+    );
     expect(await parser.isShellCommandReadOnlyAST('git status')).toBe(true);
     expect(await parser.isShellCommandReadOnlyAST('rm -rf temp')).toBe(false);
     await expect(parser.initParser()).rejects.toThrow(
       'tree-sitter WASM failed to initialise',
     );
-    expect(runtimeLoads).toHaveBeenCalledTimes(1);
+    expect(languageLoads).toHaveBeenCalledTimes(1);
+  });
+
+  it('maps parser runtime exceptions without changing the legacy fallback', async () => {
+    const init = vi.fn(async () => undefined);
+    const deleteParser = vi.fn();
+    const parse = vi.fn(() => {
+      throw new Error('parser runtime failure');
+    });
+    const setLanguage = vi
+      .fn()
+      .mockImplementationOnce(() => undefined)
+      .mockImplementationOnce(() => undefined)
+      .mockImplementationOnce(() => {
+        throw new Error('cannot configure replacement');
+      });
+    const constructed = vi.fn();
+
+    class ParserMock {
+      static init = init;
+      static Language = { load: vi.fn(async () => ({})) };
+
+      constructor() {
+        constructed();
+      }
+
+      delete = deleteParser;
+      parse = parse;
+      setLanguage = setLanguage;
+    }
+
+    vi.doMock('web-tree-sitter', () => ({ default: ParserMock }));
+    const parser = await import('./shellAstParser.js');
+
+    expect(await parser.classifyShellCommandSafety('git status')).toBe(
+      'unknown',
+    );
+    expect(await parser.isShellCommandReadOnlyAST('git status')).toBe(true);
+    await expect(parser.initParser()).rejects.toThrow(
+      'tree-sitter WASM failed to initialise',
+    );
+    expect(init).toHaveBeenCalledTimes(1);
+    expect(parse).toHaveBeenCalledTimes(2);
+    expect(constructed).toHaveBeenCalledTimes(3);
+    expect(setLanguage).toHaveBeenCalledTimes(3);
+    expect(deleteParser).toHaveBeenCalledTimes(3);
+  });
+
+  it('releases each parsed tree exactly once', async () => {
+    const deleteTree = vi.fn();
+    const parse = vi
+      .fn()
+      .mockReturnValueOnce({
+        rootNode: { namedChildCount: 0, hasError: false, namedChildren: [] },
+        delete: deleteTree,
+      })
+      .mockReturnValueOnce({
+        get rootNode() {
+          throw new Error('tree evaluation failure');
+        },
+        delete: deleteTree,
+      });
+
+    class ParserMock {
+      static init = vi.fn(async () => undefined);
+      static Language = { load: vi.fn(async () => ({})) };
+
+      parse = parse;
+      setLanguage = vi.fn();
+    }
+
+    vi.doMock('web-tree-sitter', () => ({ default: ParserMock }));
+    const parser = await import('./shellAstParser.js');
+
+    expect(await parser.classifyShellCommandSafety('git status')).toBe(
+      'unknown',
+    );
+    expect(await parser.classifyShellCommandSafety('git status')).toBe(
+      'unknown',
+    );
+    expect(deleteTree).toHaveBeenCalledTimes(2);
   });
 
   it('keeps the packaged runtime deferred and parses from emitted chunks', async () => {
@@ -130,7 +245,7 @@ describe('shellAstParser lazy runtime', () => {
     const entryPath = path.join(tempDir, 'entry.ts');
     writeFileSync(
       entryPath,
-      `export { _resetParser, isShellCommandReadOnlyAST, parseShellCommand } from ${JSON.stringify(
+      `export { _resetParser, classifyShellCommandSafety, isShellCommandReadOnlyAST, parseShellCommand } from ${JSON.stringify(
         path.join(repoRoot, 'packages/core/src/utils/shellAstParser.ts'),
       )};\n`,
     );
@@ -176,6 +291,9 @@ describe('shellAstParser lazy runtime', () => {
       }?test=${Date.now()}`
     )) as {
       _resetParser(): void;
+      classifyShellCommandSafety(
+        command: string,
+      ): Promise<'read-only' | 'write' | 'unknown'>;
       isShellCommandReadOnlyAST(command: string): Promise<boolean>;
       parseShellCommand(command: string): Promise<{
         rootNode: { type: string };
@@ -196,6 +314,15 @@ describe('shellAstParser lazy runtime', () => {
     expect(await packagedParser.isShellCommandReadOnlyAST('rm -rf temp')).toBe(
       false,
     );
+    expect(await packagedParser.classifyShellCommandSafety('rm -rf temp')).toBe(
+      'write',
+    );
+    await packagedParser.classifyShellCommandSafety(
+      'case x in x) rm target;; esac',
+    );
+    expect(await packagedParser.classifyShellCommandSafety('git status')).toBe(
+      'read-only',
+    );
 
     packagedParser._resetParser();
     const recoveredTree = await packagedParser.parseShellCommand('pwd');