Close forbidden-git-command detection gaps in reviewer_worktree.py (checkout HEAD --, checkout ., git switch) #243

New Issue

2026-07-06T10:39:12-05:00

jcwalker3 commented

2026-07-06 10:39:12 -05:00

Problem

The #233 reviewer dirty-worktree wall (reviewer_worktree.py, merged via PR #234 head 4f46655) classifies forbidden local git mutations with _FORBIDDEN_REVIEWER_GIT. Post-merge review found bypassable gaps — these file-restoring / state-mutating commands are not matched:

git checkout HEAD -- <file> — classic restore syntax; the regex only matches checkout immediately followed by --
git checkout <ref> -- <file> — same gap, any ref
git checkout . — restores all files without any --
git switch / git switch - / git switch -C — HEAD mutation not covered (checkout-to-branch is also unmatched, for consistency decide whether branch switching in a shared worktree is allowed at all for reviewers)
git stash store / git stash branch — uncommon stash subcommands outside the alternation (stash bare is caught, so partial)

Currently matched correctly: bare stash + push/pop/drop/apply/clear/list, checkout --, restore, reset (all modes), clean, cherry-pick, rebase, merge, commit, push, branch -d/-D, worktree remove.

Acceptance criteria

is_forbidden_reviewer_git_command() matches checkout <anything> -- <path>, checkout ., and git switch variants.
Consider inverting the model: reviewer git commands should be allowlist-based (_READONLY_REVIEWER_GIT) with everything unmatched treated as forbidden (fail closed), instead of blocklist-based. Decide and document.
Add tests for each bypass above (positive detection) plus read-only regressions (git checkout never appears in the allowlist).
No change to author-side behavior.

Provenance

Found during post-merge verification of PR #234 (reviewer session, 2026-07-06, pinned head 4f46655). Related: #233, mcp-control-plane #91.

## Problem The #233 reviewer dirty-worktree wall (`reviewer_worktree.py`, merged via PR #234 head `4f46655`) classifies forbidden local git mutations with `_FORBIDDEN_REVIEWER_GIT`. Post-merge review found bypassable gaps — these file-restoring / state-mutating commands are **not** matched: - `git checkout HEAD -- <file>` — classic restore syntax; the regex only matches `checkout` immediately followed by `--` - `git checkout <ref> -- <file>` — same gap, any ref - `git checkout .` — restores all files without any `--` - `git switch` / `git switch -` / `git switch -C` — HEAD mutation not covered (checkout-to-branch is also unmatched, for consistency decide whether branch switching in a shared worktree is allowed at all for reviewers) - `git stash store` / `git stash branch` — uncommon stash subcommands outside the alternation (stash bare is caught, so partial) Currently matched correctly: bare `stash` + push/pop/drop/apply/clear/list, `checkout --`, `restore`, `reset` (all modes), `clean`, `cherry-pick`, `rebase`, `merge`, `commit`, `push`, `branch -d/-D`, `worktree remove`. ## Acceptance criteria 1. `is_forbidden_reviewer_git_command()` matches `checkout <anything> -- <path>`, `checkout .`, and `git switch` variants. 2. Consider inverting the model: reviewer git commands should be allowlist-based (`_READONLY_REVIEWER_GIT`) with everything unmatched treated as forbidden (fail closed), instead of blocklist-based. Decide and document. 3. Add tests for each bypass above (positive detection) plus read-only regressions (`git checkout` never appears in the allowlist). 4. No change to author-side behavior. ## Provenance Found during post-merge verification of PR #234 (reviewer session, 2026-07-06, pinned head `4f46655`). Related: #233, mcp-control-plane #91.

jcwalker3 added the status:in-progress label 2026-07-06 10:41:53 -05:00

jcwalker3 referenced this issue from a commit

2026-07-06 10:42:17 -05:00

fix(reviewer): close forbidden-git detection gaps via allowlist (#243)

jcwalker3 commented

2026-07-06 10:42:22 -05:00

Claimed and implemented on feat/issue-243-forbidden-git-gaps.

Approach: Replaced blocklist with fail-closed allowlist — any git command not in _READONLY_REVIEWER_GIT is forbidden. Closes checkout HEAD/--, checkout ., switch, and uncommon stash bypasses.

Tests: 17 reviewer worktree tests pass including new bypass coverage.

PR opened for review.

Claimed and implemented on `feat/issue-243-forbidden-git-gaps`. **Approach:** Replaced blocklist with fail-closed allowlist — any `git` command not in `_READONLY_REVIEWER_GIT` is forbidden. Closes checkout HEAD/--, checkout ., switch, and uncommon stash bypasses. **Tests:** 17 reviewer worktree tests pass including new bypass coverage. PR opened for review.

jcwalker3 referenced a pull request that will close this issue

2026-07-06 10:42:29 -05:00

fix(reviewer): close forbidden-git detection gaps via allowlist (closes #243) #247

jcwalker3 referenced this issue

2026-07-06 10:43:14 -05:00

Issue-lock gate binds to the shared dev worktree, blocking all scratch-clone author sessions while any session is mid-branch #249

jcwalker3 referenced this issue

2026-07-06 10:46:52 -05:00

Issue-lock gate binds to the shared dev worktree, blocking all scratch-clone author sessions while any session is mid-branch #249

sysadmin referenced this issue

2026-07-06 11:18:22 -05:00

fix(reviewer): close forbidden-git detection gaps via allowlist (closes #243) #247