diff --git a/CHANGELOG.md b/CHANGELOG.md index 635e4a0..598e75f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,13 @@ All notable changes to the claude-plugins project will be documented in this fil 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`. +### code-review v3.8.1 + +#### Fixed +- `/code-review --github ` 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/...origin/`. 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: `), 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. +- 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`. +- 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/`, 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. + ### code-review v3.8.0 #### Changed diff --git a/plugins/code-review/.claude-plugin/plugin.json b/plugins/code-review/.claude-plugin/plugin.json index 5d7a7ca..a82e8e0 100644 --- a/plugins/code-review/.claude-plugin/plugin.json +++ b/plugins/code-review/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "code-review", "description": "Code review plugin", - "version": "3.8.0", + "version": "3.8.1", "author": { "name": "ClosedLoop", "email": "support@closedloop.ai" diff --git a/plugins/code-review/commands/start.md b/plugins/code-review/commands/start.md index 24b51dd..382605d 100644 --- a/plugins/code-review/commands/start.md +++ b/plugins/code-review/commands/start.md @@ -374,7 +374,7 @@ These notes annotate the run-plan stages with anything not obvious from the plan - **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. - **stage_02_prep_assets**: copies `shared_prompt.txt` and `bha_suffix.txt` from `/tools/prompts/` to ``. Both cache and non-cache paths use these assets. -- **stage_03_resolve_scope**: writes `/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 `/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/...origin/`); 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 `/pr_head_worktree` and records its absolute path in `review_root`/`worktree_path`; reviewer prompts and verifier inputs then read **source** under ``. **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 `/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. +- **stage_03_resolve_scope**: writes `/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 `/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/...origin/`); 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 `/pr_head_worktree` and records its absolute path in `review_root`/`worktree_path`; reviewer prompts and verifier inputs then read **source** under ``. **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/`, 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 `/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. - **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 `` 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 `/auto_incremental.json` with optional `diff_scope` (override) and `review_mode_line`. If `diff_scope` is non-null, update the cached `` token. Print `review_mode_line` (always) and, if `pr_auto_detected` was true in `scope.json`, print `"Auto-detected PR # for branch ."`. - **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. - **stage_09_detect_injection** (PLN-720): scores PR title/body/commits against the canonical 9-pattern catalogue and writes `/injection_report.json`. On severity ≥ Medium (score ≥ 30), rewrites `/intent_context.json` in place with `quarantine: true` and redacted fields. On severity ≥ High (score ≥ 70), also writes `/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. diff --git a/plugins/code-review/tools/python/code_review_helpers.py b/plugins/code-review/tools/python/code_review_helpers.py index 84e9431..ab38bd8 100644 --- a/plugins/code-review/tools/python/code_review_helpers.py +++ b/plugins/code-review/tools/python/code_review_helpers.py @@ -240,7 +240,7 @@ def _resolve_pr_scope( current_branch: str, *, allow_guess_fallback: bool, -) -> dict[str, str | int]: +) -> dict[str, str | int | bool]: """Resolve diff scope fields for a given PR number. When *allow_guess_fallback* is ``True`` (explicit ``--pr-number``), a @@ -248,6 +248,12 @@ def _resolve_pr_scope( default branch / ``head_ref=current_branch``. When ``False`` (auto-detect path), errors propagate so the caller can revert to branch scope. + + ``head_ref_from_metadata`` is ``True`` only when ``gh pr view`` returned + both the base and the head branch names, and ``False`` whenever + ``head_ref`` is the ``current_branch`` guess. A guess names whatever + branch is checked out, not this PR, so github mode refuses on it rather + than verify a checkout against it (ISS-8769). """ try: result = subprocess.run( @@ -258,11 +264,13 @@ def _resolve_pr_scope( lines = result.stdout.strip().splitlines() base_ref = lines[0].strip() if len(lines) > 0 else _resolve_default_base_ref() head_ref = lines[1].strip() if len(lines) > 1 else current_branch + head_ref_from_metadata = len(lines) > 1 except subprocess.CalledProcessError: if not allow_guess_fallback: raise base_ref = _resolve_default_base_ref() head_ref = current_branch + head_ref_from_metadata = False return { "diff_scope": f"origin/{base_ref}...origin/{head_ref}", @@ -273,16 +281,19 @@ def _resolve_pr_scope( "path_filter": "", "scope_kind": "pr", "pr_number": pr_number, + "head_ref_from_metadata": head_ref_from_metadata, } def _git_rev_parse(ref: str) -> str | None: """Resolve *ref* to a commit SHA, or ``None`` if it does not exist. - Used to compare the PR head commit against the working-tree HEAD when - deciding whether local PR review needs a worktree. Never raises — an - unresolvable ref (e.g. ``origin/`` that was never fetched) - returns ``None`` so the caller falls back to reading the working tree. + Never raises: an unresolvable ref (``origin/`` that was never + fetched, an unborn ``HEAD``) is a ``None``, not an error. + What ``None`` MEANS is the caller's to decide and they differ — + ``_resolve_default_base_ref`` reads it as "try the next candidate", + ``_tree_holds_pr_head`` as "not this shape", ``cmd_resolve_scope`` as + "cannot establish the PR head", which it refuses on. """ try: result = subprocess.run( @@ -462,6 +473,73 @@ def _working_tree_clean() -> bool: return False +def _head_is_pr_merge(pr_head: str, pr_base: str) -> bool: + """True when HEAD is a merge of *pr_head* into the PR's base branch. + + ``HEAD^2 == pr_head`` alone does not identify ``refs/pull/N/merge``: any + merge that takes the head as its second parent passes it, including one + whose first parent is an unrelated branch and an octopus that folds a + third branch in, and reviewers would then read code this PR's diff does + not contain. GitHub's merge ref has exactly two parents, the base tip it + merged into and then the head, so all of these must hold: + + - exactly two parents (no octopus), + - the second parent is *pr_head*, and + - the first parent is on the PR's base branch: *pr_base* + (``origin/``) is it or descends from it. + + Ancestry, not equality, on the first parent: the merge ref records the + base tip at the moment GitHub computed it, and the runner fetches the base + later, so ``origin/`` is routinely ahead of it. The PR metadata's + ``baseRefOid`` is no better a pin — it would be read at resolve time, + after the checkout, so equality with it refuses every run in which the + base moved in between, + and ancestry against it needs a commit the runner may never have fetched. + ``origin/`` is the ref the review diff is computed from, and the + walk to the first parent stays inside history that diff already needs. + + Fails closed: any git error (unresolvable base, shallow history, git + missing) is ``False``. + """ + try: + # Plumbing, so user log config cannot reshape the output: + # " ...". + parents = _run_git(["rev-list", "--parents", "-n", "1", "HEAD"]).split()[1:] + except (subprocess.CalledProcessError, FileNotFoundError, OSError): + return False + if len(parents) != 2 or parents[1] != pr_head: + return False + return _is_ancestor(parents[0], pr_base) + + +def _tree_holds_pr_head(pr_head: str, pr_base: str) -> bool: + """True when the working tree holds *pr_head*'s content (ISS-8769). + + Two checkout shapes legitimately hold a PR's source, and BOTH occur in + GitHub Actions, so neither may be treated as the only one: + + - the head commit itself (``actions/checkout`` with an explicit + ``ref: ``), and + - the PR's merge ref ``refs/pull/N/merge`` — the DEFAULT checkout for a + ``pull_request`` event — a two-parent merge of the head into a commit + on *pr_base* (see ``_head_is_pr_merge`` for why each part is needed). + + Anything else (another branch, a stale head, an unresolvable ref, a merge + that is not this PR's) is not established, and a caller must refuse rather + than review it. Uncommitted changes to tracked files disqualify either + shape: the tree then holds content that is in no commit this PR contains. + Untracked files do not, matching ``_working_tree_clean``. + + Costs one ``rev-parse`` plus one ``status`` on the head checkout, and two + more git calls (parents, ancestry) on the merge-ref shape. + """ + if not pr_head: + return False + if _git_rev_parse("HEAD") != pr_head and not _head_is_pr_merge(pr_head, pr_base): + return False + return _working_tree_clean() + + def _git_worktree_quiet(args: list[str]) -> None: """Run a best-effort ``git worktree`` cleanup command, swallowing exec errors. @@ -4927,6 +5005,11 @@ def cmd_resolve_scope(args: argparse.Namespace) -> int: scope_kind = "branch" pr_auto_detected = False + # Github mode verifies the checkout against the PR's OWN metadata, so it + # needs to know whether the head was read from ``gh pr view`` or guessed, + # and which base the PR targets before ``--base`` overrides ``base_ref``. + pr_head_from_metadata = False + pr_base_ref = "" if pr_number is not None: # Explicit --pr-number: use _resolve_pr_scope with guess fallback. @@ -4939,6 +5022,8 @@ def cmd_resolve_scope(args: argparse.Namespace) -> int: review_branch = str(pr_scope["review_branch"]) path_filter = str(pr_scope["path_filter"]) scope_kind = str(pr_scope["scope_kind"]) + pr_head_from_metadata = bool(pr_scope["head_ref_from_metadata"]) + pr_base_ref = base_ref # Fetch origin head (allow failure for explicit PR) subprocess.run( @@ -5021,9 +5106,12 @@ def cmd_resolve_scope(args: argparse.Namespace) -> int: # the PR head with no uncommitted modifications. In every other case we # MUST isolate; if isolation cannot be established (head unresolvable, or # ``git worktree add`` fails) we abort rather than silently review the - # wrong source against a remote PR diff. GitHub CI mode already checks - # out the PR head, so this is local-only. Hygiene-only runs read no - # source (and Gate A exits before the footer teardown), so they skip it. + # wrong source against a remote PR diff. Isolation stays LOCAL-ONLY: + # ``review_root`` moves where a whole agent fleet reads from, and github + # mode has prompts that do not resolve it. Github mode instead VERIFIES + # the tree it was handed and refuses — see the ISS-8769 block below. + # Hygiene-only runs read no source (and Gate A exits before the footer + # teardown), so they skip it. head_sha = "" review_root = "" worktree_path = "" @@ -5062,6 +5150,58 @@ def cmd_resolve_scope(args: argparse.Namespace) -> int: review_root = created worktree_path = created + # ISS-8769 — github mode VERIFIES the tree it was handed, and refuses. + # + # ``--github`` selects file-based handoff output. It does NOT declare "I am + # running inside GitHub Actions", and running it from a developer machine is + # supported — so the old code, which skipped every check whenever the mode + # was github, silently reviewed whatever branch happened to be checked out + # while holding a diff computed from the PR's remote refs. Nothing in the + # run said which tree was read, and the deep tier's cross-file reviewers + # (Impact Analyzer, Design Critic, Bug Hunter B) are exactly the ones a + # wrong tree makes confidently wrong. + # + # This block ADDS a refusal and changes nothing else: no worktree, no + # ``review_root``, and ``head_sha`` stays out of the emitted scope so no + # downstream consumer of it changes behavior in github mode. Either the + # tree is established as this PR's source, or the run stops. + # + # "This PR" means the PR's own metadata, checked FIRST. When ``gh pr view`` + # fails, ``_resolve_pr_scope`` guesses ``head_ref`` from the checked-out + # branch, and a clean checkout of that branch trivially holds the guessed + # head — so verifying the tree against it would pass and review that + # branch against the requested PR's diff. + if mode == "github" and scope_kind == "pr" and not hygiene_only: + if not pr_head_from_metadata: + print( + f"Error: cannot resolve PR #{pr_number}'s head from its " + f"metadata (`gh pr view {pr_number}` failed or returned no " + "head branch), so the only head available is a guess " + f"({diff_tip}, from the checked-out branch). A tree that " + "matches a guess proves nothing about this PR, and continuing " + "could review the wrong code against its diff. Check " + "`gh auth status` (GH_TOKEN on a CI runner) and that PR " + f"#{pr_number} exists in this repository, then retry.", + file=sys.stderr, + ) + return 1 + pr_head = _git_rev_parse(diff_tip) or "" # diff_tip == origin/ + if not _tree_holds_pr_head(pr_head, f"origin/{pr_base_ref}"): + print( + "Error: cannot establish that the checked-out tree is PR " + f"#{pr_number}'s source ({diff_tip} = " + f"{pr_head or 'unresolvable'}). Reviewers read the working " + "tree, so continuing would review the wrong code against this " + "PR's diff — the exact failure this check exists to stop. A " + "CI runner satisfies it by checking out the PR head, or the " + "PR's merge ref (exactly the head merged into a commit on " + f"origin/{pr_base_ref}), with a clean tree; locally, check out " + "the PR branch (or drop --github and run the local flow, which " + "isolates the head into a worktree for you).", + file=sys.stderr, + ) + return 1 + result_out = { "diff_scope": diff_scope, "base_ref": base_ref, diff --git a/plugins/code-review/tools/python/test_code_review_helpers.py b/plugins/code-review/tools/python/test_code_review_helpers.py index 23fb276..32e136c 100644 --- a/plugins/code-review/tools/python/test_code_review_helpers.py +++ b/plugins/code-review/tools/python/test_code_review_helpers.py @@ -6857,15 +6857,22 @@ def test_pr_auto_detected_respects_base_override(self, tmp_path: Path) -> None: class TestResolveScopeWorktree: - """PR-head worktree isolation for local PR review (cmd_resolve_scope). - - Local ``/code-review `` computes its diff from the fetched remote - refs but reviewer/verifier agents read source via Read/Grep against the - working tree. When the operator is on a different branch, those reads - see the wrong content and the verifier rejects every finding on the - existence check. These tests pin that resolve-scope materializes a - detached worktree at the PR head SHA (surfaced as ``review_root``) - exactly when it is needed — and degrades cleanly otherwise. + """PR-head source handling for PR review (cmd_resolve_scope). + + ``/code-review `` computes its diff from the fetched remote refs but + reviewer/verifier agents read source via Read/Grep against the working + tree. When that tree is not the PR head, those reads see the wrong + content and the verifier rejects every finding on the existence check. + + The two modes answer that differently and the cases below are split + accordingly. LOCAL mode ISOLATES: it materializes a detached worktree at + the PR head SHA (surfaced as ``review_root``) exactly when it is needed, + and aborts when it cannot. GITHUB mode VERIFIES AND REFUSES (ISS-8769): + it emits no ``review_root`` and no ``head_sha``, so nothing downstream + changes, but it will not proceed against a tree it cannot establish as + the PR's source. Coverage is deliberately NOT symmetric — the local-only + cases (hygiene-only, worktree-add failure) have no github analogue + because github mode creates no worktree. """ def _invoke( @@ -6879,6 +6886,11 @@ def _invoke( hygiene_only: str = "false", worktree_add_rc: int = 0, dirty: bool = False, + merge_parent: str = "", + first_parent: str = "main000", + extra_parent: str = "", + gh_stdout: str | None = "main\nfeat-x\n", + base_ref_override: str | None = None, ) -> tuple[int, str, list[list[str]]]: """Run cmd_resolve_scope with mocked git; return (rc, stdout, git_calls).""" import io @@ -6896,17 +6908,51 @@ def _side_effect(cmd, **_kwargs): # noqa: ANN001, ANN202 calls.append(cl) joined = " ".join(cl) if cl[:2] == ["gh", "pr"] and "baseRefName" in joined: + # `None` models `gh pr view` failing (auth, network, no such + # PR); `_resolve_pr_scope` then guesses head_ref from + # setup.json's `other-branch`. + if gh_stdout is None: + raise subprocess.CalledProcessError(1, cl) return subprocess.CompletedProcess( - args=cl, returncode=0, stdout="main\nfeat-x\n", + args=cl, returncode=0, stdout=gh_stdout, ) if cl[:2] == ["git", "fetch"]: return subprocess.CompletedProcess(args=cl, returncode=0, stdout="") if cl[:3] == ["git", "rev-parse", "--verify"]: ref = cl[-1] - sha = {"origin/feat-x": head_sha, "HEAD": work_head}.get(ref, "") + # The operator's `other-branch` is pushed and checked out, so a + # guessed `origin/other-branch` head resolves to HEAD itself. + sha = { + "origin/feat-x": head_sha, + "origin/other-branch": work_head, + "HEAD": work_head, + }.get(ref, "") return subprocess.CompletedProcess( - args=cl, returncode=0, stdout=(sha + "\n") if sha else "", + args=cl, + returncode=0 if sha else 1, + stdout=(sha + "\n") if sha else "", + ) + if cl[:3] == ["git", "rev-list", "--parents"]: + # " ". HEAD is a merge only when + # `merge_parent` (its second parent) is set: the PR's merge ref + # shape (refs/pull/N/merge, the DEFAULT `actions/checkout` for a + # `pull_request` event), with `first_parent` as the base tip it + # merged into and `extra_parent` making it an octopus. + tokens = ( + [work_head, first_parent, merge_parent, extra_parent] + if merge_parent else [work_head, first_parent] + ) + return subprocess.CompletedProcess( + args=cl, returncode=0, + stdout=" ".join(t for t in tokens if t) + "\n", ) + if cl[:3] == ["git", "merge-base", "--is-ancestor"]: + # Only `main000` is on the PR's base branch (`gh` says `main`). + # `_run_git` relies on check=True raising for git's non-zero + # exit, which a mocked subprocess.run does not do by itself. + if cl[3:] == ["main000", "origin/main"]: + return subprocess.CompletedProcess(args=cl, returncode=0, stdout="") + raise subprocess.CalledProcessError(1, cl) if cl[:2] == ["git", "status"]: return subprocess.CompletedProcess( args=cl, returncode=0, @@ -6929,7 +6975,7 @@ def _side_effect(cmd, **_kwargs): # noqa: ANN001, ANN202 try: ns = argparse.Namespace( mode=mode, pr_number=pr_number, scope_args="", - base_ref_override=None, setup_json=str(setup_path), + base_ref_override=base_ref_override, setup_json=str(setup_path), hygiene_only=hygiene_only, ) with patch( @@ -6990,8 +7036,9 @@ def test_no_worktree_for_hygiene_only(self, tmp_path: Path) -> None: assert result["worktree_path"] == "" def test_no_worktree_in_github_mode_no_pr(self, tmp_path: Path) -> None: - # GitHub CI without a PR number → scope_kind "github_pending"; the - # guard fails on both mode and scope_kind. + # GitHub mode without a PR number -> scope_kind "github_pending". There + # is no PR head to check a tree against, so neither the local isolation + # block nor the github verification block applies. result = self._run( head_sha="aaa111", work_head="bbb222", mode="github", pr_number=None, tmp_path=tmp_path, @@ -6999,19 +7046,186 @@ def test_no_worktree_in_github_mode_no_pr(self, tmp_path: Path) -> None: assert result["review_root"] == "" assert result["worktree_path"] == "" - def test_no_worktree_github_mode_with_pr_scope(self, tmp_path: Path) -> None: - # GitHub mode WITH a PR number → scope_kind "pr", so the no-worktree - # behavior is carried solely by the ``mode == "local"`` guard. This - # isolates the mode check from the scope_kind check, so a regression - # that dropped the mode guard would surface here (the runner already - # checks out the PR head in CI). - result = self._run( + def test_github_mode_refuses_a_tree_that_is_not_the_pr_head( + self, tmp_path: Path, + ) -> None: + """ISS-8769 — `--github` outside CI must not review the wrong tree. + + This case previously asserted the opposite: no worktree, rc 0, on a + tree whose HEAD is not the PR head. That expectation was wrong rather + than merely outdated — it encoded "github mode means a CI runner + already holding the PR head" while setting up the exact state where + that premise fails, so it pinned the defect. `--github` selects + file-based handoff output; it does not declare "I am inside GitHub + Actions", and running it from a developer machine is supported. + + The refusal is the whole behavior change: no worktree is created and + no `review_root` is emitted in github mode, so nothing downstream + moves. The two shapes a real runner produces are accepted by the two + cases below, which is what keeps this from breaking CI. + """ + rc, _out, calls = self._invoke( head_sha="aaa111", work_head="bbb222", mode="github", pr_number=42, tmp_path=tmp_path, ) + assert rc == 1 + assert not [c for c in calls if c[:3] == ["git", "worktree", "add"]] + + def test_github_mode_accepts_a_head_checkout(self, tmp_path: Path) -> None: + # Runner shape 1: `actions/checkout` with an explicit `ref: ` + # (what closedloop-ai/symphony-alpha's claude-code-review.yml does). + # The first positive control for the refusal above. + rc, out, calls = self._invoke( + head_sha="same999", work_head="same999", + mode="github", pr_number=42, tmp_path=tmp_path, + ) + assert rc == 0 + result = json.loads(out) assert result["scope_kind"] == "pr" + assert not [c for c in calls if c[:3] == ["git", "worktree", "add"]] + # Github mode emits NEITHER field. `head_sha` in particular re-routes + # `_file_content_hash` and the inline-comment `commit_id`, so leaving + # it empty is what makes this change additive. assert result["review_root"] == "" assert result["worktree_path"] == "" + assert result["head_sha"] == "" + + def test_github_mode_accepts_the_prs_merge_ref(self, tmp_path: Path) -> None: + # Runner shape 2, and the DEFAULT one: `actions/checkout` on a + # `pull_request` event checks out `refs/pull/N/merge`, a two-parent + # merge of the PR head (second parent) into the base tip GitHub merged + # into (first parent: the mock's default `main000`, on origin/main). + # HEAD therefore never equals the head SHA, and a check that only + # compared HEAD would refuse every such run — this case is what stops + # that. + rc, out, calls = self._invoke( + head_sha="aaa111", work_head="merge777", merge_parent="aaa111", + mode="github", pr_number=42, tmp_path=tmp_path, + ) + assert rc == 0 + assert json.loads(out)["review_root"] == "" + assert not [c for c in calls if c[:3] == ["git", "worktree", "add"]] + + def test_github_mode_refuses_a_merge_ref_for_a_different_head( + self, tmp_path: Path, + ) -> None: + # The merge-ref arm must check WHICH head it merged, not merely that + # HEAD has a second parent. Without this, any merge commit at all would + # be accepted as this PR's source. + rc, _out, _calls = self._invoke( + head_sha="aaa111", work_head="merge777", merge_parent="cccc33", + mode="github", pr_number=42, tmp_path=tmp_path, + ) + assert rc == 1 + + def test_github_mode_refuses_a_merge_whose_first_parent_is_not_on_the_base( + self, tmp_path: Path, + ) -> None: + # Taking the PR head as second parent does not make a merge the PR's + # merge ref: a local `git merge` of the head into an unrelated branch + # has the same second parent, and its tree carries that branch's code, + # which this PR's diff does not contain. The first parent must be on + # the PR's base branch. + rc, _out, calls = self._invoke( + head_sha="aaa111", work_head="merge777", merge_parent="aaa111", + first_parent="side444", + mode="github", pr_number=42, tmp_path=tmp_path, + ) + assert rc == 1 + assert not [c for c in calls if c[:3] == ["git", "worktree", "add"]] + + def test_github_mode_refuses_an_octopus_merge_of_the_head( + self, tmp_path: Path, + ) -> None: + # Base tip first, PR head second, and a third branch folded in. GitHub's + # merge ref has exactly two parents; a second-parent check alone would + # accept this and reviewers would read the third branch's code as the + # PR's. + rc, _out, calls = self._invoke( + head_sha="aaa111", work_head="merge777", merge_parent="aaa111", + extra_parent="side444", + mode="github", pr_number=42, tmp_path=tmp_path, + ) + assert rc == 1 + assert not [c for c in calls if c[:3] == ["git", "worktree", "add"]] + + def test_github_mode_merge_check_uses_the_pr_base_under_a_base_override( + self, tmp_path: Path, + ) -> None: + # `--base develop` re-points the review diff, but GitHub built the merge + # ref against the PR's own base (`main` in its metadata), so that is the + # branch the first parent must be on. `main000` is on origin/main and + # NOT on origin/develop; checking against the overridden base would + # refuse every genuine merge-ref run that passes `--base`. + rc, out, calls = self._invoke( + head_sha="aaa111", work_head="merge777", merge_parent="aaa111", + base_ref_override="develop", + mode="github", pr_number=42, tmp_path=tmp_path, + ) + assert rc == 0 + result = json.loads(out) + assert result["base_ref"] == "develop" + assert result["review_root"] == "" + assert not [c for c in calls if c[:3] == ["git", "worktree", "add"]] + + def test_github_mode_refuses_a_dirty_tree_at_the_head( + self, tmp_path: Path, + ) -> None: + # HEAD is the PR head but tracked files are modified, so the tree holds + # content that is in no commit this PR contains. Local mode isolates + # here (test_dirty_tree_isolates_even_when_head_matches); github mode + # has no worktree to isolate into, so it refuses. + rc, _out, _calls = self._invoke( + head_sha="same999", work_head="same999", dirty=True, + mode="github", pr_number=42, tmp_path=tmp_path, + ) + assert rc == 1 + + def test_github_mode_refuses_when_the_pr_head_is_unresolvable( + self, tmp_path: Path, + ) -> None: + # The head branch name came from the PR's metadata, but + # `origin/` was never fetched (resolve-scope lets that fetch + # fail), so the PR head is not established. An unresolvable ref must + # refuse rather than fall through to "review whatever is here". + rc, _out, calls = self._invoke( + head_sha="", work_head="bbb222", + mode="github", pr_number=42, tmp_path=tmp_path, + ) + assert rc == 1 + assert not [c for c in calls if c[:3] == ["git", "worktree", "add"]] + + @pytest.mark.parametrize( + "gh_stdout", [None, "main\n"], + ids=["gh_pr_view_fails", "gh_pr_view_returns_no_head"], + ) + def test_github_mode_refuses_a_guessed_head_even_when_the_tree_matches_it( + self, tmp_path: Path, gh_stdout: str | None, + ) -> None: + # Without a head from `gh pr view`, the guess fallback takes head_ref + # from the checked-out branch. That branch is pushed, checked out, and + # clean here, so a tree check against the guess would pass and review + # `other-branch` against PR #42's diff. The head must come from the + # PR's own metadata before any checkout is verified against it. + rc, _out, calls = self._invoke( + head_sha="aaa111", work_head="bbb222", gh_stdout=gh_stdout, + mode="github", pr_number=42, tmp_path=tmp_path, + ) + assert rc == 1 + assert not [c for c in calls if c[:3] == ["git", "worktree", "add"]] + + def test_github_mode_hygiene_only_reads_no_source_and_is_not_checked( + self, tmp_path: Path, + ) -> None: + # Gate A runs deterministic hygiene checks and exits before any agent + # reads source, so there is no wrong tree to read and nothing to refuse. + # Without this case the verification block could be tightened to fire on + # hygiene-only runs and every case above would stay green. + result = self._run( + head_sha="aaa111", work_head="bbb222", hygiene_only="true", + mode="github", pr_number=42, tmp_path=tmp_path, + ) + assert result["review_root"] == "" def test_fail_closed_when_worktree_add_fails(self, tmp_path: Path) -> None: # Head differs (isolation required) but ``git worktree add`` failed →