Merge pull request 'fix(reviewer): close forbidden-git detection gaps via allowlist (closes #243)' (#247) from feat/issue-243-forbidden-git-gaps into master

reviewer_worktree.py

@@ -4,6 +4,11 @@ Reviewer sessions must never stash, reset, or otherwise manipulate unrelated
 local changes from another session. When the active worktree has dirty tracked
 files outside the PR scope, the workflow must stop or switch to a disposable
 scratch worktree (``scripts/worktree-review``).
+
+Git command policy (#243): reviewers use an allowlist, not a blocklist.
+Any ``git`` invocation that does not match ``_READONLY_REVIEWER_GIT`` is
+forbidden — including ``checkout HEAD --``, ``checkout .``, ``switch``,
+and uncommon ``stash`` subcommands that older blocklists missed.
 """
 
 from __future__ import annotations
@@ -11,25 +16,7 @@ from __future__ import annotations
 import re
 import shlex
 
-# Subcommands that mutate unrelated local state — forbidden for reviewers.
-_FORBIDDEN_REVIEWER_GIT = re.compile(
-    r"\bgit\b(?:\s+(?:-C\s+\S+\s+)?)?"
-    r"(?:stash(?:\s+(?:push|pop|drop|apply|clear|list))?|"
-    r"checkout\s+--|"
-    r"restore\s+|"
-    r"reset(?:\s+(?:--hard|--soft|--mixed|--merge))?|"
-    r"clean(?:\s+(?:-f|-fd|-fdx|-x|-d|-n))*|"
-    r"cherry-pick|"
-    r"rebase|"
-    r"merge|"
-    r"commit(?:\s+(?:--amend|-a|-am))?|"
-    r"push(?:\s+(?:--force|--force-with-lease))?|"
-    r"branch\s+-[dD]|"
-    r"worktree\s+remove)",
-    re.IGNORECASE,
-)
-
-# Read-only git operations reviewers may use for validation.
+# Read-only git operations reviewers may use for validation (#243 allowlist).
 _READONLY_REVIEWER_GIT = re.compile(
     r"\bgit\b(?:\s+(?:-C\s+\S+\s+)?)?"
     r"(?:fetch|status|diff|log|show|rev-parse|branch(?:\s+--show-current)?|"
@@ -37,6 +24,8 @@ _READONLY_REVIEWER_GIT = re.compile(
     re.IGNORECASE,
 )
 
+_GIT_INVOCATION = re.compile(r"\bgit\b", re.IGNORECASE)
+
 
 def parse_dirty_tracked_files(porcelain: str) -> list[str]:
     """Return tracked paths with local modifications from ``git status --porcelain``.
@@ -73,24 +62,26 @@ def files_outside_pr_scope(
     return [path for path in dirty if path not in scope]
 
 
-def is_forbidden_reviewer_git_command(command: str) -> bool:
-    """True when a shell command would mutate unrelated local/remote git state."""
-    text = (command or "").strip()
-    if not text:
-        return False
-    return bool(_FORBIDDEN_REVIEWER_GIT.search(text))
+def _is_git_command(command: str) -> bool:
+    return bool(_GIT_INVOCATION.search((command or "").strip()))
 
 
 def is_readonly_reviewer_git_command(command: str) -> bool:
     """True when the command is an explicitly allowed read-only git operation."""
     text = (command or "").strip()
-    if not text:
-        return False
-    if is_forbidden_reviewer_git_command(text):
+    if not text or not _is_git_command(text):
         return False
     return bool(_READONLY_REVIEWER_GIT.search(text))
 
 
+def is_forbidden_reviewer_git_command(command: str) -> bool:
+    """True when a git command is not on the reviewer readonly allowlist."""
+    text = (command or "").strip()
+    if not text or not _is_git_command(text):
+        return False
+    return not is_readonly_reviewer_git_command(text)
+
+
 def assess_reviewer_git_command_log(commands: list[str] | None) -> dict:
     """Fail closed when reviewer shell history includes forbidden git mutations."""
     forbidden = [

tests/test_reviewer_worktree.py

@@ -65,6 +65,23 @@ class TestForbiddenGitCommands(unittest.TestCase):
         self.assertTrue(is_forbidden_reviewer_git_command("git reset --hard"))
         self.assertTrue(is_forbidden_reviewer_git_command("git clean -fd"))
 
+    def test_blocks_checkout_restore_and_switch_bypasses(self):
+        """Issue #243: blocklist gaps closed via readonly allowlist model."""
+        blocked = (
+            "git checkout HEAD -- review_proofs.py",
+            "git checkout prgs/master -- review_proofs.py",
+            "git checkout .",
+            "git switch -",
+            "git switch -C feat/other-branch",
+            "git switch master",
+            "git stash store",
+            "git stash branch wip-stash",
+        )
+        for cmd in blocked:
+            with self.subTest(cmd=cmd):
+                self.assertTrue(is_forbidden_reviewer_git_command(cmd))
+                self.assertFalse(is_readonly_reviewer_git_command(cmd))
+
     def test_allows_readonly_commands(self):
         for cmd in (
             "git fetch prgs master",