Skip to content

Commit fbc4f0c

Browse files
peterulsteenclaudewongk
authored
fix(code-review): refuse a --github review of a tree that is not the PR head (ISS-8769) (#198)
* ISS-8769: Refuse a --github review of a tree that is not the PR head `/code-review --github <PR>` run outside GitHub Actions reviewed whatever tree happened to be checked out, silently and with exit 0. `cmd_resolve_scope` skipped every PR-head check when the mode was github, but `--github` selects file-based handoff output - it does not declare "I am inside GitHub Actions" - and running it from a developer machine is supported. Such a run left `review_root` empty and every reviewer read the operator's branch while holding a diff from origin/<base>...origin/<head>. GitHub mode now VERIFIES the tree it was handed and REFUSES with a non-zero exit when it cannot establish it as the PR's source. Two checkout shapes are accepted because both occur in Actions: HEAD is the PR head (explicit `ref: <head sha>`), or HEAD is the PR's merge ref refs/pull/N/merge - the DEFAULT checkout for a pull_request event - whose second parent is the head. Either way tracked files must be unmodified. The block ADDS a refusal and changes nothing else: no worktree, no `review_root`, and `head_sha` stays out of the emitted scope so no consumer of it moves. Isolation stays local-only on purpose - it redirects a whole agent fleet, and github-review.md's own steps do not resolve `review_root`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(code-review): bind github-mode checkout check to PR metadata and base * fix(code-review): lock github-mode merge check to the PR base under --base --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Kris Wong <kris.wong@closedloop.ai>
1 parent 9e97927 commit fbc4f0c

5 files changed

Lines changed: 393 additions & 32 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,13 @@ All notable changes to the claude-plugins project will be documented in this fil
44

55
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Entries are listed newest-first; each plugin section is treated as released when merged to `main`.
66

7+
### code-review v3.8.1
8+
9+
#### Fixed
10+
- `/code-review --github <PR>` run outside GitHub Actions no longer reviews whatever tree happens to be checked out. `cmd_resolve_scope` skipped every PR-head check when the mode was github, but `--github` selects file-based handoff output and is a supported invocation from a developer machine — so such a run left `review_root` empty and every reviewer read the operator's branch while holding a diff computed from `origin/<base>...origin/<head>`. Worst on the deep tier, whose cross-file reviewers grep for callsites the wrong tree does not have, and silent in both directions: nothing in the output said which tree was read. GitHub mode now VERIFIES the tree it was handed and REFUSES with a non-zero exit when it cannot establish it as the PR's source. Two checkout shapes are accepted, because both occur in GitHub Actions: HEAD is the PR head (`actions/checkout` with an explicit `ref: <head sha>`), or HEAD is the PR's merge ref `refs/pull/N/merge` — the DEFAULT checkout for a `pull_request` event. Either way the tree must have no uncommitted changes to tracked files. The check costs at most five local git calls and changes nothing else: github mode still creates no worktree, still emits an empty `review_root`, and still emits an empty `head_sha`, so no consumer of those fields moves. Local mode is untouched — it continues to isolate the PR head into a detached worktree, which github mode deliberately does not do because `review_root` redirects a whole agent fleet and `github-review.md`'s own steps do not resolve it.
11+
- GitHub mode refuses when the PR head did not come from the PR's own metadata. When `gh pr view` fails or returns no head branch, `_resolve_pr_scope` falls back to guessing the head from the checked-out branch, and a clean checkout of that branch would pass any tree check and be reviewed against the requested PR's diff. `_resolve_pr_scope` now reports `head_ref_from_metadata`, and `cmd_resolve_scope` refuses in github mode before verifying the checkout whenever it is false. This also covers a CI runner on a detached HEAD, where the guess degrades to `origin/HEAD`.
12+
- GitHub mode accepts a merge checkout only when it is shaped like the PR's merge ref. Checking that HEAD's second parent is the PR head was not enough: a merge of the head into an unrelated branch, or an octopus merge that folds a third branch in, passed it and put code the PR's diff does not contain in front of reviewers. `_head_is_pr_merge` now requires exactly two parents, the second being the PR head and the first being on the PR's base branch (an ancestor of `origin/<baseRefName>`, taken from the PR metadata rather than a `--base` override). Ancestry rather than equality keeps a genuine merge ref accepted after the base branch advances past the commit GitHub merged into. Any git error during these checks refuses.
13+
714
### code-review v3.8.0
815

916
#### Changed

‎plugins/code-review/.claude-plugin/plugin.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"name": "code-review",
33
"description": "Code review plugin",
4-
"version": "3.8.0",
4+
"version": "3.8.1",
55
"author": {
66
"name": "ClosedLoop",
77
"email": "support@closedloop.ai"

‎plugins/code-review/commands/start.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -374,7 +374,7 @@ These notes annotate the run-plan stages with anything not obvious from the plan
374374

375375
- **stage_01_setup**: already executed in stage 0b (which captured stdout and wrote `setup.json` itself). The walker treats this as a no-op; the run plan's `stdout` field is `None` for this stage because no shell redirect is correct here.
376376
- **stage_02_prep_assets**: copies `shared_prompt.txt` and `bha_suffix.txt` from `<PLUGIN_ROOT>/tools/prompts/` to `<CR_DIR>`. Both cache and non-cache paths use these assets.
377-
- **stage_03_resolve_scope**: writes `<CR_DIR>/scope.json` with `diff_scope`, `base_ref`, `head_ref`, `review_branch`, `diff_tip`, `pr_number`, `path_filter`, `scope_kind`, `pr_auto_detected`, `head_sha`, `review_root`, `worktree_path`. After this stage, run `finalize-cache` to populate `<CR_DIR>/cache_config.json`. The walker uses these for token resolution downstream. **PR-head worktree (local PR review):** for `MODE=local` PR reviews (`scope_kind == "pr"`, not hygiene-only) resolve-scope isolates source reads so reviewer/verifier agents read the PR head, not the operator's working tree. The diff is computed from the fetched remote refs (`origin/<base>...origin/<head>`); reading the working tree is safe **only** when it already IS the PR head with a clean tree. Otherwise resolve-scope materializes a detached git worktree at the PR head SHA under `<CR_DIR>/pr_head_worktree` and records its absolute path in `review_root`/`worktree_path`; reviewer prompts and verifier inputs then read **source** under `<REVIEW_ROOT>`. **Fail-closed:** if isolation is required but cannot be established (PR head unresolvable, or `git worktree add` fails), resolve-scope returns non-zero and the run aborts (`on_failure: abort`) rather than silently review the wrong branch — the operator is told to check out the PR branch or fix the git error. `review_root` is empty (agents read the working tree) only for the already-at-head-and-clean case, staged/file/branch scope, hygiene-only, and GitHub mode (where the runner already checks out the head). **Worktree lifecycle:** resolve-scope runs a startup GC (`_gc_stale_pr_head_worktrees`) that reclaims orphaned `cr-*/pr_head_worktree` checkouts from prior runs that aborted before teardown; `stage_30_footer` tears down the current run's worktree (validating the path equals the canonical `<CR_DIR>/pr_head_worktree` before the destructive removal). Because the walker can abort before the footer, the next run's startup GC is the backstop — a leaked worktree is never silently reused. Code-intelligence-aware reviewers run grep-only (`CODE_INTEL_ALLOWED=false`) whenever `review_root` is set, since any external index covers the operator checkout, not the PR head.
377+
- **stage_03_resolve_scope**: writes `<CR_DIR>/scope.json` with `diff_scope`, `base_ref`, `head_ref`, `review_branch`, `diff_tip`, `pr_number`, `path_filter`, `scope_kind`, `pr_auto_detected`, `head_sha`, `review_root`, `worktree_path`. After this stage, run `finalize-cache` to populate `<CR_DIR>/cache_config.json`. The walker uses these for token resolution downstream. **PR-head worktree (local PR review):** for `MODE=local` PR reviews (`scope_kind == "pr"`, not hygiene-only) resolve-scope isolates source reads so reviewer/verifier agents read the PR head, not the operator's working tree. The diff is computed from the fetched remote refs (`origin/<base>...origin/<head>`); reading the working tree is safe **only** when it already IS the PR head with a clean tree. Otherwise resolve-scope materializes a detached git worktree at the PR head SHA under `<CR_DIR>/pr_head_worktree` and records its absolute path in `review_root`/`worktree_path`; reviewer prompts and verifier inputs then read **source** under `<REVIEW_ROOT>`. **Fail-closed:** if isolation is required but cannot be established (PR head unresolvable, or `git worktree add` fails), resolve-scope returns non-zero and the run aborts (`on_failure: abort`) rather than silently review the wrong branch — the operator is told to check out the PR branch or fix the git error. `review_root` is empty (agents read the working tree) only for the already-at-head-and-clean case, staged/file/branch scope, hygiene-only, and GitHub mode. **GitHub mode verifies instead of isolating (ISS-8769):** for `MODE=github` PR reviews (`scope_kind == "pr"`, not hygiene-only) resolve-scope first requires the PR head to come from the PR's own metadata — when `gh pr view` fails, the only head left is a guess (the checked-out branch), and a tree matching a guess proves nothing, so it refuses — then checks that the checked-out tree actually holds that head: either HEAD is that commit, or HEAD is the PR's merge ref (`refs/pull/N/merge`, the default `actions/checkout` for a `pull_request` event), a merge with exactly two parents whose second parent is that commit and whose first parent is on the PR's base branch (an ancestor of `origin/<baseRefName>`, which tolerates the base advancing after GitHub computed the merge) — with no uncommitted changes to tracked files. Both runner shapes pass and cost at most five local git calls; nothing else changes, and `review_root`/`head_sha` stay empty so no downstream consumer moves. Anything else **refuses** with a non-zero exit. `--github` is a supported invocation from a developer machine (it selects file-based handoff output; it does not declare "I am inside GitHub Actions"), and before this check such a run silently reviewed the operator's branch against the PR's diff. It refuses rather than isolating because `review_root` redirects a whole agent fleet and `github-review.md`'s own steps do not resolve it; the remedy is to check out the PR branch, or drop `--github` and use the local flow, which isolates for you. **Worktree lifecycle:** resolve-scope runs a startup GC (`_gc_stale_pr_head_worktrees`) that reclaims orphaned `cr-*/pr_head_worktree` checkouts from prior runs that aborted before teardown; `stage_30_footer` tears down the current run's worktree (validating the path equals the canonical `<CR_DIR>/pr_head_worktree` before the destructive removal). Because the walker can abort before the footer, the next run's startup GC is the backstop — a leaked worktree is never silently reused. Code-intelligence-aware reviewers run grep-only (`CODE_INTEL_ALLOWED=false`) whenever `review_root` is set, since any external index covers the operator checkout, not the PR head.
378378
- **stage_07_auto_incremental**: runs **before** `stage_05_parse_diff` (its array position is between `stage_04_finalize_cache` and `stage_05_parse_diff`). This ordering matters: any `diff_scope` override must be applied to the cached `<DIFF_SCOPE>` token BEFORE parse-diff and extract-patches materialize `diff_data.json` and `patches_all.txt`, otherwise downstream stages see full-PR diff data alongside a narrowed token. The stage retains its `_07_` id as a stable label; execution order follows array position. Writes `<CR_DIR>/auto_incremental.json` with optional `diff_scope` (override) and `review_mode_line`. If `diff_scope` is non-null, update the cached `<DIFF_SCOPE>` token. Print `review_mode_line` (always) and, if `pr_auto_detected` was true in `scope.json`, print `"Auto-detected PR #<PR_NUMBER> for branch <REVIEW_BRANCH>."`.
379379
- **stage_08_fetch_intent**: the helper writes `intent_context.json` into `cr_dir` itself; its stdout is a small `{path, source}` summary that the walker discards. The run plan's `stdout` field is `None` here because redirecting stdout to `intent_context.json` would corrupt the file by overwriting the helper's structured payload with the summary.
380380
- **stage_09_detect_injection** (PLN-720): scores PR title/body/commits against the canonical 9-pattern catalogue and writes `<CR_DIR>/injection_report.json`. On severity ≥ Medium (score ≥ 30), rewrites `<CR_DIR>/intent_context.json` in place with `quarantine: true` and redacted fields. On severity ≥ High (score ≥ 70), also writes `<CR_DIR>/agent_injection-detector.json` containing a canonical `InjectionAttempt` finding — the `agent_*.json` naming makes `cmd_collect_findings` pick it up via the standard glob with no extra wiring. Always appends one JSONL entry to `.closedloop-ai/injection-log.jsonl` (90-day TTL, swept on read). `on_failure: continue` is intentional — a detector crash must never abort the pipeline.

0 commit comments

Comments
 (0)