diff --git a/CHANGELOG.md b/CHANGELOG.md
index ddd0f67..04dc442 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.6.1
+
+#### Fixed
+- **Local branch review no longer bases its diff on a stale ref, which folded unrelated commits into the review.** `resolve-scope` hardcoded the diff scope to `main...HEAD`. That `main` is the local branch ref, which every worktree of a clone shares, so a worktree inherits whatever commit the primary checkout last left it on. Because `A...B` diffs from `merge-base(A, B)`, a local ref sitting behind the branch's fork point dragged the merge base backwards and pulled every commit that landed on the base in between into the diff — reviewing other people's work as if it were the branch's. The base is now chosen per run between the local ref and `origin/`: both merge bases are ancestors of HEAD along the base branch, so the ref producing the later one is the true fork point. This is correct under either kind of staleness — a local ref behind the fork point, or an `origin/` behind it because the base has unpushed local commits (where preferring the remote ref would fold those commits in instead) — and under both at once, when the local ref and `origin/` have diverged: whichever tip the branch was cut from yields the deeper merge base, so it is selected regardless of which ref that is. A repo with no remote has no second view and keeps its local ref, so remote-less reviews are unchanged.
+- The base branch is now detected — `origin/HEAD`, then `main`/`master` remotely, then locally — rather than assumed to be `main`, so repositories whose default branch is `master` (or any name `origin/HEAD` reports) resolve their scope correctly instead of failing against a nonexistent `main`. `--base-ref-override` runs through the same selection, and falls back to the local branch when the named base has no remote-tracking ref rather than emitting an `origin/[` that git cannot resolve.
+- `fetch-intent` now reads branch commit subjects from the same fork point instead of the raw local base ref, so intent classification and injection detection no longer receive commits that landed on the base branch and were never part of the change under review.
+
### code-review v3.6.0
#### Fixed
diff --git a/plugins/code-review/.claude-plugin/plugin.json b/plugins/code-review/.claude-plugin/plugin.json
index 7d2c66e..f55e775 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.6.0",
+ "version": "3.6.1",
"author": {
"name": "ClosedLoop",
"email": "support@closedloop.ai"
diff --git a/plugins/code-review/README.md b/plugins/code-review/README.md
index 8436d24..7d41425 100644
--- a/plugins/code-review/README.md
+++ b/plugins/code-review/README.md
@@ -24,12 +24,27 @@ plugins/code-review/
code-review-worker-graph.md Graph-aware variant for the cross-file and design reviewers (Impact Analyzer, Bug Hunter B, fast-path, Design Critic); adds read-only codebase-memory-mcp tools — cross-file usage discovery for the cross-file roles, project-structure/dependency-graph analysis (get_architecture, query_graph) for the Design Critic
commands/
start.md Main /start command (orchestrator)
+ shallow.md /shallow wrapper — `/start --depth shallow`
+ deep.md /deep wrapper — `/start --depth deep`
+ cost.md /cost command — token-cost attribution from session transcripts
+ skills/
+ spawn-reviewers/SKILL.md Reviewer-fleet spawn/collection contract at stage_20_spawn_reviewers
+ verify-findings/SKILL.md Finding-verifier fleet dispatch at stage_23_verify_findings (PLN-722)
+ singleton-dispatch/SKILL.md Single-agent dispatch for stage_11_extract_signals / stage_15_coverage_critic (PLN-725)
+ present-local/SKILL.md Local-mode presenter at stage_29_present
+ fix/SKILL.md Verifies and fixes BLOCKING/HIGH findings from a prior review session
prompts/
github-review.md GitHub-mode constraints and output steps (loaded conditionally)
+ scripts/
+ dist/cost-report.mjs Bundled Node cost analyzer for /cost (sources at tools/code-review-cost/)
tools/
prompts/shared_prompt.txt Shared reviewer constraints injected into every agent prompt
prompts/bha_suffix.txt Bug Hunter A reviewer persona and focus areas
prompts/design_critic_suffix.txt Design Critic reviewer role (software-design craftsmanship; always-on at deep tier)
+ prompts/impact_analyzer_prompt.txt Impact Analyzer reviewer role (FEA-1401 cross-file blast radius; deep tier, signal-gated)
+ prompts/coverage_critic_prompt.txt Coverage critic role (standard/deep tiers)
+ prompts/signal_extraction_prompt.txt Signal extraction role (standard/deep tiers)
+ prompts/verifier_prompt.txt Finding-verifier role (falsify-oriented; PLN-722)
python/code_review_schema.py Canonical Finding + ResultEnvelope schema + validators (PLN-719)
python/test_code_review_schema.py Schema tests + round-trips
python/code_review_helpers.py Deterministic helper CLI (parse-diff, hygiene, partition, route, validate, cache, finalize-result, arbitrate-budget, prepare-run, etc.)
@@ -90,11 +105,13 @@ Runs a comprehensive code review. Invokes the full pipeline: diff parsing, hygie
| Argument | Behavior |
|---|---|
-| _(none)_ | Diff current branch vs `main` |
+| _(none)_ | Review the open PR's diff for the current branch; with no open PR, diff the current branch from its fork point off the default branch |
| `staged` | Diff only staged (index) changes |
-| `file1 file2 ...` | Diff specific files against `main` |
+| `file1 file2 ...` | Diff specific files from the fork point off the default branch |
| `123` | Use PR #123's diff (local output, no posting) |
+**Base ref resolution.** The default base branch is *detected*, not assumed: the helper reads the `origin/HEAD` symbolic ref, then probes `origin/main` / `origin/master`, then the same names locally, falling back to `main` only when nothing resolves. The diff runs from the fork point rather than a fixed ref — a clone holds both a local `` and an `origin/`, and either can lag the other (a stale local checkout, or unpushed local base commits). The helper takes the merge base of each against `HEAD` and uses whichever ref yields the *later* one, since that is the true fork point. This keeps commits that landed on the base branch after the fork out of the review diff under either kind of staleness.
+
**Mode flags:**
| Flag | Description |
@@ -102,7 +119,7 @@ Runs a comprehensive code review. Invokes the full pipeline: diff parsing, hygie
| `--github` | GitHub CI mode: auto-detect PR from branch or accept explicit PR number, post inline comments via file-based handoff |
| `--github 123` | GitHub CI mode: review PR #123 specifically |
| `--hygiene-only` | Run only the deterministic hygiene checks. Zero LLM tokens consumed. Fast. |
-| `--base ][` | Override the base branch for diffing (default: `main`) |
+| `--base ][` | Override the base branch for diffing (default: the repository's detected default branch) |
| `--since-last-review` | Review only commits added since the last successful review (local mode only) |
| `--full-review` | Force a full diff even when auto-incremental mode would narrow the scope |
| `--depth shallow\|standard\|deep` | Reviewer-fleet tier. Default `standard`. See **Depth Tiers** below |
@@ -110,14 +127,14 @@ Runs a comprehensive code review. Invokes the full pipeline: diff parsing, hygie
**Examples:**
```bash
-/start # All changes on current branch vs main
+/start # Open PR diff, else changes on current branch since its fork point
/start staged # Only staged changes
/start src/auth.ts src/user.ts # Specific files
/start 123 # PR #123 diff locally
/start --github # CI: auto-detect PR, post comments
/start --github 123 # CI: PR #123, post comments
/start --hygiene-only # Hygiene checks only
-/start --base develop # Diff against develop instead of main
+/start --base develop # Diff against develop instead of the default branch
/start --since-last-review # Only new commits since last review
/start --full-review # Disable incremental narrowing
```
@@ -309,7 +326,7 @@ Overrides survive across runs while the file content matches and the 90-day TTL
**Re-assert is best-effort against finding_id drift.** Finding IDs are assigned as `_f` where `` is the reviewer's emission position. Across re-runs the LLM may reorder or drop findings, so an override written against `bha_f3` on run N may map to a different finding (or no finding) on run N+1. The content-hash anchor prevents promoting an unrelated finding at a different line — but the common drift case is the override silently no-ops. Two mitigations: (1) re-assert and re-run immediately so the override is honored against the same emission set, and (2) inspect the verify-prepare manifest for `override_hits` / `override_invalidated` to confirm the override landed.
-The presenter (local mode `start.md`, GitHub mode `code-review-verifier-stats.md`) surfaces:
+The presenter (local mode: the `present-local` skill, GitHub mode: `github-review.md` Step 6e, which writes `.closedloop-ai/code-review-verifier-stats.md`) surfaces:
- Per-reviewer FP rate (`stats.verification.by_reviewer[*].fp_rate`)
- Override count per reviewer (`stats.verification.by_reviewer[*].re_asserted`)
diff --git a/plugins/code-review/commands/start.md b/plugins/code-review/commands/start.md
index fc27177..8d0852b 100644
--- a/plugins/code-review/commands/start.md
+++ b/plugins/code-review/commands/start.md
@@ -13,7 +13,7 @@ Run a multi-agent code review with partitioned deep review, deterministic hygien
## Usage
```
-/start # Review open PR diff for current branch, or main...HEAD if no PR
+/start # Review open PR diff for current branch, or the diff since the branch forked from the default branch if no PR
/start staged # Review only staged changes
/start file1 file2 # Review specific files
/start 123 # Review PR #123 diff locally (no posting)
diff --git a/plugins/code-review/tools/python/code_review_helpers.py b/plugins/code-review/tools/python/code_review_helpers.py
index 47bdba6..0540b8f 100644
--- a/plugins/code-review/tools/python/code_review_helpers.py
+++ b/plugins/code-review/tools/python/code_review_helpers.py
@@ -244,8 +244,8 @@ def _resolve_pr_scope(
"""Resolve diff scope fields for a given PR number.
When *allow_guess_fallback* is ``True`` (explicit ``--pr-number``), a
- ``CalledProcessError`` from ``gh pr view`` falls back to
- ``base_ref="main"`` / ``head_ref=current_branch``. When ``False``
+ ``CalledProcessError`` from ``gh pr view`` falls back to the repo's
+ default branch / ``head_ref=current_branch``. When ``False``
(auto-detect path), errors propagate so the caller can revert to branch
scope.
"""
@@ -256,12 +256,12 @@ def _resolve_pr_scope(
capture_output=True, text=True, check=True,
)
lines = result.stdout.strip().splitlines()
- base_ref = lines[0].strip() if len(lines) > 0 else "main"
+ 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
except subprocess.CalledProcessError:
if not allow_guess_fallback:
raise
- base_ref = "main"
+ base_ref = _resolve_default_base_ref()
head_ref = current_branch
return {
@@ -295,6 +295,92 @@ def _git_rev_parse(ref: str) -> str | None:
return None
+def _resolve_default_base_ref() -> str:
+ """Return the repository's default branch *name* (e.g. ``main``, ``master``).
+
+ Probes, in order: the ``origin/HEAD`` symbolic ref (what the remote
+ reports as its default), then well-known remote branches, then the
+ same names locally for repos with no ``origin``. Falls back to
+ ``main`` when nothing resolves, preserving the historical default.
+
+ Returns a bare branch name, not a ref — callers pair it with
+ :func:`_base_rev` to get the revision to diff against. ``base_ref``
+ travels through ``scope.json`` as a name because consumers such as
+ ``compute-hashes`` origin-qualify it themselves.
+ """
+ try:
+ symbolic = _run_git(
+ ["symbolic-ref", "--short", "refs/remotes/origin/HEAD"],
+ ).strip()
+ except (subprocess.CalledProcessError, FileNotFoundError, OSError):
+ symbolic = ""
+ if symbolic:
+ # "origin/main" -> "main"
+ _, _, name = symbolic.partition("/")
+ if name:
+ return name
+ for candidate in ("main", "master"):
+ if _git_rev_parse(f"origin/{candidate}"):
+ return candidate
+ for candidate in ("main", "master"):
+ if _git_rev_parse(candidate):
+ return candidate
+ return "main"
+
+
+def _merge_base(a: str, b: str) -> str | None:
+ """Return the merge base of *a* and *b*, or ``None`` if there isn't one.
+
+ ``None`` covers both "no common ancestor" and "a ref does not resolve",
+ which callers treat alike: neither yields a usable fork point.
+ """
+ try:
+ return _run_git(["merge-base", a, b]).strip() or None
+ except (subprocess.CalledProcessError, FileNotFoundError, OSError):
+ return None
+
+
+def _is_ancestor(commit: str, descendant: str) -> bool:
+ """Whether *commit* is *descendant* or one of its ancestors."""
+ try:
+ _run_git(["merge-base", "--is-ancestor", commit, descendant])
+ return True
+ except (subprocess.CalledProcessError, FileNotFoundError, OSError):
+ return False
+
+
+def _base_rev(base_ref: str, head_rev: str = "HEAD") -> str:
+ """Return the revision to diff *head_rev* against for branch *base_ref*.
+
+ ``...`` diffs from ``merge-base(, )``, so the base
+ ref matters only through the fork point it produces. A clone holds two
+ views of the same base branch, and either one can lag the other:
+
+ * The local ```` is shared by every worktree of a clone, so it
+ carries whatever commit the primary checkout last left it on. When it
+ sits behind the fork point, the merge base walks backwards and folds
+ every commit that landed on the base in between into the review diff.
+ * ``origin/`` lags whenever the base has unpushed local commits.
+ Branch off those and the fork point is ahead of the remote ref, which
+ folds the unpushed base commits into the diff instead.
+
+ Both merge bases are ancestors of *head_rev* along the base branch, so the
+ later of the two is the true fork point — take the ref that produces it,
+ which is correct under either kind of staleness. Falls back to whichever
+ ref resolves when only one does (e.g. a remote-less repo, where the local
+ ref is the only truth).
+ """
+ remote_rev = f"origin/{base_ref}"
+ local_mb = _merge_base(base_ref, head_rev)
+ remote_mb = _merge_base(remote_rev, head_rev)
+ if remote_mb is None:
+ return base_ref
+ if local_mb is None:
+ return remote_rev
+ # Equal bases resolve to the remote ref; the two ranges are identical.
+ return remote_rev if _is_ancestor(local_mb, remote_mb) else base_ref
+
+
# Startup-GC age guard: a PR-head worktree directory is reclaimed as an
# abort-orphan only once it is older than this. Set far above any real
# review wall-time (runs are minutes, not hours) so a concurrent in-flight
@@ -4791,7 +4877,7 @@ def cmd_resolve_scope(args: argparse.Namespace) -> int:
current_branch = "HEAD"
diff_scope = ""
- base_ref = "main"
+ base_ref = _resolve_default_base_ref()
head_ref = current_branch
review_branch = current_branch
diff_tip = "HEAD"
@@ -4844,13 +4930,15 @@ def cmd_resolve_scope(args: argparse.Namespace) -> int:
pr_auto_detected = True
except (subprocess.CalledProcessError, FileNotFoundError,
OSError, ValueError):
- # Any failure: fall back to branch scope
+ # Any failure: fall back to branch scope. base_ref is
+ # untouched here — every pr_scope assignment below the
+ # fetch is unreachable once either raise-point fires.
pr_number = None
pr_auto_detected = False
- diff_scope = "main...HEAD"
+ diff_scope = f"{_base_rev(base_ref)}...HEAD"
scope_kind = "branch"
else:
- diff_scope = "main...HEAD"
+ diff_scope = f"{_base_rev(base_ref)}...HEAD"
scope_kind = "branch"
elif scope_args.strip() == "staged":
diff_scope = "--cached"
@@ -4858,7 +4946,7 @@ def cmd_resolve_scope(args: argparse.Namespace) -> int:
else:
# Treat scope_args as file paths
files = scope_args.strip()
- diff_scope = f"main...HEAD -- {files}"
+ diff_scope = f"{_base_rev(base_ref)}...HEAD -- {files}"
path_filter = f"-- {files}"
scope_kind = "file_paths"
@@ -4870,11 +4958,12 @@ def cmd_resolve_scope(args: argparse.Namespace) -> int:
# Apply base-ref override if provided
if base_ref_override:
if scope_kind == "pr":
- diff_scope = f"origin/{base_ref_override}...origin/{head_ref}"
+ override_head = f"origin/{head_ref}"
+ diff_scope = f"{_base_rev(base_ref_override, override_head)}...{override_head}"
elif path_filter:
- diff_scope = f"origin/{base_ref_override}...HEAD {path_filter}"
+ diff_scope = f"{_base_rev(base_ref_override)}...HEAD {path_filter}"
else:
- diff_scope = f"origin/{base_ref_override}...HEAD"
+ diff_scope = f"{_base_rev(base_ref_override)}...HEAD"
base_ref = base_ref_override
# Worktree isolation for local PR review. The diff is computed from the
@@ -4987,7 +5076,7 @@ def cmd_fetch_intent(args: argparse.Namespace) -> int:
elif scope_kind == "branch":
try:
result = subprocess.run(
- ["git", "log", f"{base_ref}..{diff_tip}",
+ ["git", "log", f"{_base_rev(base_ref, diff_tip)}..{diff_tip}",
"--oneline", "--no-merges", "--format=%s"],
capture_output=True, text=True, check=True,
)
diff --git a/plugins/code-review/tools/python/conftest.py b/plugins/code-review/tools/python/conftest.py
index 5f141cc..081cb01 100644
--- a/plugins/code-review/tools/python/conftest.py
+++ b/plugins/code-review/tools/python/conftest.py
@@ -7,6 +7,8 @@
import argparse
import json
+import os
+import subprocess
from pathlib import Path
from typing import Any
@@ -14,6 +16,51 @@
from code_review_schema import SCHEMA_VERSION, empty_telemetry
+# Pinned git identity for every test-side fixture repo. Fixed values →
+# deterministic commit SHAs, which matters for any artifact that embeds a
+# commit hash (context_key, PR-head SHAs) and for the golden fixtures that
+# assert on them byte-for-byte. Pair with a pinned GIT_AUTHOR_DATE /
+# GIT_COMMITTER_DATE (see git_fixture's *env*) wherever the SHA itself is
+# pinned; identity alone is not enough to fix a SHA.
+GIT_IDENTITY_ENV = {
+ "GIT_AUTHOR_NAME": "Fixture Author",
+ "GIT_AUTHOR_EMAIL": "fixture@example.com",
+ "GIT_COMMITTER_NAME": "Fixture Author",
+ "GIT_COMMITTER_EMAIL": "fixture@example.com",
+ # Fully isolate git config so the host's config can neither break commits
+ # (commit.gpgsign, hooks) nor make them host-dependent. GIT_CONFIG_NOSYSTEM
+ # blocks /etc/gitconfig, but the operator's ~/.gitconfig is *global*, not
+ # system — so it also needs GIT_CONFIG_GLOBAL. Pointing GLOBAL and SYSTEM at
+ # os.devnull is unambiguous and order-independent: a fixture repo may be
+ # built BEFORE a hermetic HOME redirect, so relying on HOME alone would
+ # leave those commits exposed to the real ~/.gitconfig.
+ "GIT_CONFIG_NOSYSTEM": "1",
+ "GIT_CONFIG_GLOBAL": os.devnull,
+ "GIT_CONFIG_SYSTEM": os.devnull,
+}
+
+
+def git_fixture(repo: Path, *args: str, env: dict[str, str] | None = None) -> str:
+ """Run a git command inside *repo* and return stdout (raises on failure).
+
+ The shared builder for every test-side git fixture repo: it pins commit
+ identity and neutralizes host git config via :data:`GIT_IDENTITY_ENV`, so
+ fixture history is byte-stable across runs and hosts. *env* overlays extra
+ variables (e.g. ``GIT_AUTHOR_DATE``) onto that base.
+ """
+ full_env = {**os.environ, **GIT_IDENTITY_ENV}
+ if env:
+ full_env.update(env)
+ result = subprocess.run(
+ ["git", *args],
+ cwd=str(repo),
+ env=full_env,
+ capture_output=True,
+ text=True,
+ check=True,
+ )
+ return result.stdout
+
def pytest_addoption(parser: pytest.Parser) -> None:
"""Register code-review-specific CLI options.
diff --git a/plugins/code-review/tools/python/prefix_golden_harness.py b/plugins/code-review/tools/python/prefix_golden_harness.py
index 2070b35..59babc3 100644
--- a/plugins/code-review/tools/python/prefix_golden_harness.py
+++ b/plugins/code-review/tools/python/prefix_golden_harness.py
@@ -67,6 +67,7 @@
from typing import Any
import code_review_helpers
+from conftest import GIT_IDENTITY_ENV, git_fixture
from golden_fixture_harness import run_with_stdout_capture
# The plugin root (…/plugins/code-review) — this module lives at
@@ -74,27 +75,6 @@
# so it is independent of the harness's chdir into the fixture repo.
PLUGIN_ROOT = Path(__file__).resolve().parents[2]
-# Pinned git identity + author/committer dates. Fixed values → deterministic
-# commit SHAs, which matters for any prefix artifact that embeds a commit hash
-# (context_key, PR-head SHAs in later fixtures). Even where the plain-branch
-# prefix does not embed a SHA, pinning is free insurance.
-_GIT_IDENTITY_ENV = {
- "GIT_AUTHOR_NAME": "Fixture Author",
- "GIT_AUTHOR_EMAIL": "fixture@example.com",
- "GIT_COMMITTER_NAME": "Fixture Author",
- "GIT_COMMITTER_EMAIL": "fixture@example.com",
- # Fully isolate git config so the host's config can neither break commits
- # (commit.gpgsign, hooks) nor make them host-dependent. GIT_CONFIG_NOSYSTEM
- # blocks /etc/gitconfig, but the operator's ~/.gitconfig is *global*, not
- # system — so it also needs GIT_CONFIG_GLOBAL. Pointing GLOBAL and SYSTEM at
- # os.devnull is unambiguous and order-independent: the fixture repo is built
- # (via _git) BEFORE the hermetic HOME redirect, so relying on HOME alone
- # would leave those commits exposed to the real ~/.gitconfig.
- "GIT_CONFIG_NOSYSTEM": "1",
- "GIT_CONFIG_GLOBAL": os.devnull,
- "GIT_CONFIG_SYSTEM": os.devnull,
-}
-
# The reviewer fleet — the first non-deterministic (LLM) stage. The prefix walk
# stops *before* this id; everything earlier is deterministic (the two PLN-725
# singletons are stubbed).
@@ -147,6 +127,12 @@ class FixtureRepoSpec:
computes), which is why ``head`` lands on a separate branch — so the
symmetric diff is non-empty. ``head`` may be ``None`` for the degenerate
empty-diff fixture (base commit only; HEAD stays on ``main``).
+
+ resolve-scope prefers ``origin/main`` as the diff base and only falls back
+ to the local ``main`` ref when no remote-tracking ref exists. These repos
+ carry no ``origin``, so the base stays local and ``diff_scope`` stays
+ ``main...HEAD`` — keep it that way, since adding a remote here would
+ repoint every fixture's expected ``scope.json``.
"""
base: Commit
@@ -154,32 +140,16 @@ class FixtureRepoSpec:
feature_branch: str = "feature"
-def _git(repo: Path, *args: str, env: dict[str, str] | None = None) -> str:
- """Run a git command inside ``repo`` and return stdout (raises on failure)."""
- full_env = {**os.environ, **_GIT_IDENTITY_ENV}
- if env:
- full_env.update(env)
- result = subprocess.run(
- ["git", *args],
- cwd=str(repo),
- env=full_env,
- capture_output=True,
- text=True,
- check=True,
- )
- return result.stdout
-
-
def _apply_commit(repo: Path, commit: Commit) -> None:
for rel, content in commit.writes.items():
path = repo / rel
path.parent.mkdir(parents=True, exist_ok=True)
path.write_text(content)
for rel in commit.deletes:
- _git(repo, "rm", "--quiet", rel)
- _git(repo, "add", "-A")
+ git_fixture(repo, "rm", "--quiet", rel)
+ git_fixture(repo, "add", "-A")
date_env = {"GIT_AUTHOR_DATE": commit.date, "GIT_COMMITTER_DATE": commit.date}
- _git(repo, "commit", "--quiet", "-m", commit.message, env=date_env)
+ git_fixture(repo, "commit", "--quiet", "-m", commit.message, env=date_env)
def build_fixture_repo(repo: Path, spec: FixtureRepoSpec) -> None:
@@ -192,10 +162,10 @@ def build_fixture_repo(repo: Path, spec: FixtureRepoSpec) -> None:
empty.
"""
repo.mkdir(parents=True, exist_ok=True)
- _git(repo, "init", "--quiet", "-b", "main")
+ git_fixture(repo, "init", "--quiet", "-b", "main")
_apply_commit(repo, spec.base)
if spec.head is not None:
- _git(repo, "checkout", "--quiet", "-b", spec.feature_branch)
+ git_fixture(repo, "checkout", "--quiet", "-b", spec.feature_branch)
_apply_commit(repo, spec.head)
@@ -219,7 +189,7 @@ def hermetic_prefix_env(repo: Path, home: Path) -> Iterator[None]:
mutated = {
"HOME": str(home),
"CR_GLOBAL_CACHE": "0",
- **_GIT_IDENTITY_ENV,
+ **GIT_IDENTITY_ENV,
}
saved: dict[str, str | None] = {k: os.environ.get(k) for k in mutated}
original_detect = code_review_helpers._detect_open_pr
@@ -1387,7 +1357,7 @@ def _seed_review_state(home: Path, repo: Path, cr_dir: Path) -> None:
ancestor of HEAD — so ``--since-last-review`` yields ``...HEAD``.
``completed_at`` is a cache input, never snapshotted.
"""
- base_sha = _git(repo, "rev-parse", "main").strip()
+ base_sha = git_fixture(repo, "rev-parse", "main").strip()
cache_dir = cache_dir_for(home)
cache_dir.mkdir(parents=True, exist_ok=True)
state = {
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 7e99d39..6d9022c 100644
--- a/plugins/code-review/tools/python/test_code_review_helpers.py
+++ b/plugins/code-review/tools/python/test_code_review_helpers.py
@@ -16,6 +16,7 @@
import pytest
from conftest import (
+ git_fixture,
invoke_prepare_run,
minimal_diff_finding,
minimal_envelope,
@@ -6300,6 +6301,265 @@ def test_corrupt_manifest_not_first_run(self) -> None:
# ---------------------------------------------------------------------------
+def _commit_file(repo: Path, name: str, content: str) -> str:
+ """Commit *content* as *name* inside *repo*; return the new commit SHA."""
+ (repo / name).write_text(content)
+ git_fixture(repo, "add", "-A")
+ git_fixture(repo, "commit", "--quiet", "-m", f"add {name}")
+ return git_fixture(repo, "rev-parse", "HEAD").strip()
+
+
+def _build_stale_base_repo(
+ repo: Path, *, default_branch: str = "main", with_origin: bool = True,
+) -> dict[str, str]:
+ """Materialize a repo whose local base branch lags the branch's fork point.
+
+ Reproduces the worktree hazard without a network remote. History::
+
+ A ── B <- B is the commit feat-x forked from
+ ╰── C <- feat-x (HEAD)
+
+ The local ```` ref is rewound to ``A`` while
+ ``refs/remotes/origin/`` is pinned at ``B``, standing in
+ for a clone whose shared local base ref sits behind the remote. ``B``
+ adds ``UNRELATED.txt`` (someone else's landed work) and ``C`` adds
+ ``MINE.txt`` (the change under review), so a diff based on the stale
+ local ref is detectable by file name alone.
+
+ With ``with_origin=False`` no remote-tracking ref is created, modelling a
+ remote-less repo where the local ref is the only truth. Returns the
+ ``a``/``b``/``c`` SHAs; HEAD is left on ``feat-x``.
+ """
+ repo.mkdir(parents=True, exist_ok=True)
+ git_fixture(repo, "init", "--quiet", "-b", default_branch)
+ a = _commit_file(repo, "base.txt", "base\n")
+ b = _commit_file(repo, "UNRELATED.txt", "landed on the base after the fork\n")
+ git_fixture(repo, "checkout", "--quiet", "-b", "feat-x")
+ c = _commit_file(repo, "MINE.txt", "the change under review\n")
+ # Rewind the local base ref behind the fork point; pin the remote at it.
+ git_fixture(repo, "branch", "--force", default_branch, a)
+ if with_origin:
+ git_fixture(repo, "update-ref", f"refs/remotes/origin/{default_branch}", b)
+ return {"a": a, "b": b, "c": c}
+
+
+class TestResolveDiffBase:
+ """Base resolution for local branch review (stale-ref hazard).
+
+ ``...HEAD`` diffs from ``merge-base(, HEAD)``. A clone's local
+ base ref is shared by every worktree, so it carries whatever commit the
+ primary checkout last left it on; when it lags, the merge base walks
+ backwards past the fork point and folds unrelated landed commits into the
+ review diff. ``origin/`` only moves forward, so it pins the fork
+ point.
+ """
+
+ def test_local_branch_diffs_from_fork_point_not_stale_local_ref(
+ self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ repo = tmp_path / "repo"
+ _build_stale_base_repo(repo)
+ monkeypatch.chdir(repo)
+
+ # Guard the fixture: the stale local ref really does over-collect, so
+ # the assertion below is proving the fix and not a no-op repo shape.
+ stale = git_fixture(repo, "diff", "--name-only", "main...HEAD").split()
+ assert sorted(stale) == ["MINE.txt", "UNRELATED.txt"]
+
+ with patch("code_review_helpers._detect_open_pr", return_value=None):
+ result = TestResolveScope()._run("local", tmp_path=tmp_path)
+
+ assert result["diff_scope"] == "origin/main...HEAD"
+ assert result["base_ref"] == "main"
+ # The payoff: the resolved scope reviews only the branch's own commit.
+ reviewed = git_fixture(
+ repo, "diff", "--name-only", result["diff_scope"],
+ ).split()
+ assert reviewed == ["MINE.txt"]
+
+ def test_file_paths_scope_also_uses_fork_point(
+ self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ repo = tmp_path / "repo"
+ _build_stale_base_repo(repo)
+ monkeypatch.chdir(repo)
+ result = TestResolveScope()._run(
+ "local", scope_args="MINE.txt", tmp_path=tmp_path,
+ )
+ assert result["diff_scope"] == "origin/main...HEAD -- MINE.txt"
+ assert result["scope_kind"] == "file_paths"
+
+ def test_remoteless_repo_falls_back_to_local_ref(
+ self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ # No origin means no stale-ref hazard: the local ref is the only truth.
+ repo = tmp_path / "repo"
+ _build_stale_base_repo(repo, with_origin=False)
+ monkeypatch.chdir(repo)
+ with patch("code_review_helpers._detect_open_pr", return_value=None):
+ result = TestResolveScope()._run("local", tmp_path=tmp_path)
+ assert result["diff_scope"] == "main...HEAD"
+ assert result["base_ref"] == "main"
+
+ def test_unpushed_base_commits_keep_the_local_ref_as_base(
+ self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ # Mirror image of the stale-local-ref hazard: when the base branch has
+ # commits that were never pushed and the branch forks off those, it is
+ # origin/main that lags. Preferring the remote ref unconditionally
+ # would fold the unpushed base commits into the diff.
+ repo = tmp_path / "repo"
+ repo.mkdir()
+ git_fixture(repo, "init", "--quiet", "-b", "main")
+ a = _commit_file(repo, "base.txt", "base\n")
+ git_fixture(repo, "update-ref", "refs/remotes/origin/main", a)
+ _commit_file(repo, "UNPUSHED.txt", "on main, never pushed\n")
+ git_fixture(repo, "checkout", "--quiet", "-b", "feat-x")
+ _commit_file(repo, "MINE.txt", "the change under review\n")
+ monkeypatch.chdir(repo)
+
+ with patch("code_review_helpers._detect_open_pr", return_value=None):
+ result = TestResolveScope()._run("local", tmp_path=tmp_path)
+
+ assert result["diff_scope"] == "main...HEAD"
+ reviewed = git_fixture(
+ repo, "diff", "--name-only", result["diff_scope"],
+ ).split()
+ assert reviewed == ["MINE.txt"]
+
+ def _build_diverged_base_repo(self, repo: Path) -> None:
+ """Local ``main`` and ``origin/main`` diverge from a common ancestor.
+
+ Both kinds of staleness at once::
+
+ A1 ── A2 <- origin/main (pushed by someone else)
+ /
+ ... ── B0 ─┤
+ \\
+ L1 ── L2 <- local main (unpushed)
+
+ Neither ref alone can base every branch cut from this repo: a branch
+ off ``L2`` needs the local ref, a branch off ``A2`` needs the remote
+ one. The later merge base with HEAD identifies the correct tip.
+ """
+ repo.mkdir()
+ git_fixture(repo, "init", "--quiet", "-b", "main")
+ _commit_file(repo, "base.txt", "B0\n") # common ancestor
+ git_fixture(repo, "checkout", "--quiet", "-b", "origin-work")
+ _commit_file(repo, "A1.txt", "pushed by someone else\n")
+ a2 = _commit_file(repo, "A2.txt", "pushed by someone else\n")
+ git_fixture(repo, "update-ref", "refs/remotes/origin/main", a2)
+ git_fixture(repo, "checkout", "--quiet", "main")
+ _commit_file(repo, "L1.txt", "unpushed local base commit\n")
+ _commit_file(repo, "L2.txt", "unpushed local base commit\n")
+
+ def test_diverged_base_branch_off_local_tip_uses_local_ref(
+ self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ # Cut from the unpushed local tip: origin/main lags, so basing on it
+ # would fold the unpushed L1/L2 into the diff.
+ repo = tmp_path / "repo"
+ self._build_diverged_base_repo(repo)
+ git_fixture(repo, "checkout", "--quiet", "-b", "feat-x", "main")
+ _commit_file(repo, "MINE.txt", "the change under review\n")
+ monkeypatch.chdir(repo)
+ with patch("code_review_helpers._detect_open_pr", return_value=None):
+ result = TestResolveScope()._run("local", tmp_path=tmp_path)
+ assert result["diff_scope"] == "main...HEAD"
+ reviewed = git_fixture(
+ repo, "diff", "--name-only", result["diff_scope"],
+ ).split()
+ assert reviewed == ["MINE.txt"]
+
+ def test_diverged_base_branch_off_remote_tip_uses_remote_ref(
+ self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ # Cut from origin/main while local main is also ahead with its own
+ # unpushed commits: basing on the local ref would fold A1/A2 in.
+ repo = tmp_path / "repo"
+ self._build_diverged_base_repo(repo)
+ git_fixture(repo, "checkout", "--quiet", "-b", "feat-x", "origin/main")
+ _commit_file(repo, "MINE.txt", "the change under review\n")
+ monkeypatch.chdir(repo)
+ with patch("code_review_helpers._detect_open_pr", return_value=None):
+ result = TestResolveScope()._run("local", tmp_path=tmp_path)
+ assert result["diff_scope"] == "origin/main...HEAD"
+ reviewed = git_fixture(
+ repo, "diff", "--name-only", result["diff_scope"],
+ ).split()
+ assert reviewed == ["MINE.txt"]
+
+ def test_identical_base_views_resolve_to_the_remote_ref(
+ self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ # The common case: local and remote agree, so both fork points match
+ # and either ref yields the same range.
+ repo = tmp_path / "repo"
+ repo.mkdir()
+ git_fixture(repo, "init", "--quiet", "-b", "main")
+ a = _commit_file(repo, "base.txt", "base\n")
+ git_fixture(repo, "update-ref", "refs/remotes/origin/main", a)
+ git_fixture(repo, "checkout", "--quiet", "-b", "feat-x")
+ _commit_file(repo, "MINE.txt", "the change under review\n")
+ monkeypatch.chdir(repo)
+ with patch("code_review_helpers._detect_open_pr", return_value=None):
+ result = TestResolveScope()._run("local", tmp_path=tmp_path)
+ assert result["diff_scope"] == "origin/main...HEAD"
+ reviewed = git_fixture(
+ repo, "diff", "--name-only", result["diff_scope"],
+ ).split()
+ assert reviewed == ["MINE.txt"]
+
+ def test_master_default_branch_is_detected(
+ self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ repo = tmp_path / "repo"
+ _build_stale_base_repo(repo, default_branch="master")
+ monkeypatch.chdir(repo)
+ with patch("code_review_helpers._detect_open_pr", return_value=None):
+ result = TestResolveScope()._run("local", tmp_path=tmp_path)
+ assert result["diff_scope"] == "origin/master...HEAD"
+ assert result["base_ref"] == "master"
+
+ def test_origin_head_symbolic_ref_wins_over_name_probing(
+ self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ # A default branch named neither main nor master is only discoverable
+ # through origin/HEAD, which is why it is probed first.
+ repo = tmp_path / "repo"
+ _build_stale_base_repo(repo, default_branch="develop")
+ git_fixture(
+ repo, "symbolic-ref", "refs/remotes/origin/HEAD",
+ "refs/remotes/origin/develop",
+ )
+ monkeypatch.chdir(repo)
+ with patch("code_review_helpers._detect_open_pr", return_value=None):
+ result = TestResolveScope()._run("local", tmp_path=tmp_path)
+ assert result["diff_scope"] == "origin/develop...HEAD"
+ assert result["base_ref"] == "develop"
+
+ def test_fetch_intent_reads_commits_from_fork_point(
+ self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ # fetch-intent feeds commit subjects to intent classification off the
+ # same base, so a stale local ref would attribute someone else's
+ # landed commit to the branch under review.
+ from code_review_helpers import cmd_fetch_intent
+
+ repo = tmp_path / "repo"
+ _build_stale_base_repo(repo)
+ monkeypatch.chdir(repo)
+ cr_dir = tmp_path / "cr"
+ cr_dir.mkdir()
+ rc = cmd_fetch_intent(argparse.Namespace(
+ pr_number=None, base_ref="main", diff_tip="HEAD",
+ scope_kind="branch", cr_dir=str(cr_dir),
+ ))
+ assert rc == 0
+ intent = json.loads((cr_dir / "intent_context.json").read_text())
+ assert intent["commits"] == "add MINE.txt"
+
+
class TestResolveScope:
def _run(self, mode: str, scope_args: str = "", pr_number: int | None = None,
base_ref_override: str | None = None, setup_json: str | None = None,
@@ -6327,10 +6587,15 @@ def _run(self, mode: str, scope_args: str = "", pr_number: int | None = None,
finally:
_sys.stdout = old_stdout
- def test_local_branch(self, tmp_path: Path) -> None:
+ def test_local_branch(
+ self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ repo = tmp_path / "repo"
+ _build_stale_base_repo(repo)
+ monkeypatch.chdir(repo)
with patch("code_review_helpers._detect_open_pr", return_value=None):
result = self._run("local", tmp_path=tmp_path)
- assert result["diff_scope"] == "main...HEAD"
+ assert result["diff_scope"] == "origin/main...HEAD"
assert result["scope_kind"] == "branch"
assert result["pr_auto_detected"] is False
@@ -6355,17 +6620,48 @@ def test_file_paths(self, tmp_path: Path) -> None:
assert result["scope_kind"] == "file_paths"
assert result["path_filter"] == "-- file1.ts file2.ts"
- def test_base_override(self, tmp_path: Path) -> None:
+ def _repo_with_origin_develop(
+ self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+ ) -> Path:
+ """Hermetic repo carrying an ``origin/develop`` for the override to find."""
+ repo = tmp_path / "repo"
+ shas = _build_stale_base_repo(repo)
+ git_fixture(repo, "update-ref", "refs/remotes/origin/develop", shas["b"])
+ monkeypatch.chdir(repo)
+ return repo
+
+ def test_base_override(
+ self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ self._repo_with_origin_develop(tmp_path, monkeypatch)
with patch("code_review_helpers._detect_open_pr", return_value=None):
result = self._run("local", base_ref_override="develop", tmp_path=tmp_path)
- assert "origin/develop" in result["diff_scope"]
+ assert result["diff_scope"] == "origin/develop...HEAD"
assert result["base_ref"] == "develop"
- def test_base_override_preserves_path_filter(self, tmp_path: Path) -> None:
+ def test_base_override_preserves_path_filter(
+ self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ self._repo_with_origin_develop(tmp_path, monkeypatch)
result = self._run("local", scope_args="file1.ts", base_ref_override="develop", tmp_path=tmp_path)
assert "origin/develop" in result["diff_scope"]
assert "-- file1.ts" in result["path_filter"]
+ def test_base_override_falls_back_to_local_ref_when_unfetched(
+ self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ # An override naming a branch with no remote-tracking ref has no
+ # origin-qualified alternative — use the local ref rather than
+ # emitting an origin/][ that git cannot resolve.
+ repo = tmp_path / "repo"
+ shas = _build_stale_base_repo(repo)
+ git_fixture(repo, "branch", "develop", shas["b"])
+ monkeypatch.chdir(repo)
+ with patch("code_review_helpers._detect_open_pr", return_value=None):
+ result = self._run("local", base_ref_override="develop", tmp_path=tmp_path)
+ assert result["diff_scope"] == "develop...HEAD"
+ assert result["base_ref"] == "develop"
+
# -- PR auto-detection tests ------------------------------------------------
def _gh_pr_view_detect(self, pr_number: str = "675") -> subprocess.CompletedProcess[str]:
@@ -6405,15 +6701,24 @@ def _side_effect(cmd, **_kwargs): # noqa: ANN001, ANN202
if isinstance(fetch_result, Exception):
raise fetch_result
return fetch_result
- # PR-head worktree isolation probes (these tests focus on scope
- # field resolution, not isolation): make the working tree already
- # be the PR head with a clean tree so resolve-scope takes the
- # no-worktree path. Same constant SHA for every ref → HEAD ==
- # origin/; empty status → clean; empty worktree list → no GC.
+ # Ref probes. These tests focus on scope field resolution, so both
+ # downstream readers get a repo that looks maximally ordinary: one
+ # constant SHA for every ref → HEAD == origin/, so
+ # resolve-scope takes the no-worktree path; one constant merge base
+ # for every pair → the local and remote views of the base agree, so
+ # base resolution settles on the remote ref. Empty status → clean;
+ # empty worktree list → no GC. Base-selection behavior itself is
+ # covered against real repos in TestResolveDiffBase.
if cmd_list[:3] == ["git", "rev-parse", "--verify"]:
return subprocess.CompletedProcess(
args=cmd, returncode=0, stdout="deadbeefcafe\n",
)
+ # Covers both `merge-base ` and `merge-base --is-ancestor`,
+ # which signals through its exit code and ignores stdout.
+ if cmd_list[:2] == ["git", "merge-base"]:
+ return subprocess.CompletedProcess(
+ args=cmd, returncode=0, stdout="deadbeefcafe\n",
+ )
if cmd_list[:2] == ["git", "status"]:
return subprocess.CompletedProcess(args=cmd, returncode=0, stdout="")
if cmd_list[:3] == ["git", "worktree", "list"]:
@@ -6445,7 +6750,7 @@ def test_pr_auto_detect_falls_back_on_no_pr(self, tmp_path: Path) -> None:
with patch("code_review_helpers.subprocess.run", side_effect=side_effect):
result = self._run("local", tmp_path=tmp_path)
assert result["scope_kind"] == "branch"
- assert result["diff_scope"] == "main...HEAD"
+ assert result["diff_scope"] == "origin/main...HEAD"
assert result["pr_auto_detected"] is False
def test_explicit_pr_number_resolves_pr_scope_without_auto_detect(self, tmp_path: Path) -> None:
@@ -6489,7 +6794,7 @@ def test_pr_auto_detect_falls_back_when_gh_missing(self, tmp_path: Path) -> None
with patch("code_review_helpers.subprocess.run", side_effect=side_effect):
result = self._run("local", tmp_path=tmp_path)
assert result["scope_kind"] == "branch"
- assert result["diff_scope"] == "main...HEAD"
+ assert result["diff_scope"] == "origin/main...HEAD"
assert result["pr_auto_detected"] is False
def test_pr_auto_detect_falls_back_on_malformed_number(self, tmp_path: Path) -> None:
@@ -6511,7 +6816,7 @@ def test_pr_auto_detect_falls_back_on_fetch_failure(self, tmp_path: Path) -> Non
assert result["pr_auto_detected"] is False
assert result["pr_number"] is None
assert result["scope_kind"] == "branch"
- assert result["diff_scope"] == "main...HEAD"
+ assert result["diff_scope"] == "origin/main...HEAD"
assert result["diff_tip"] == "HEAD"
def test_pr_auto_detect_succeeds_but_resolve_fails(self, tmp_path: Path) -> None:
@@ -6523,7 +6828,7 @@ def test_pr_auto_detect_succeeds_but_resolve_fails(self, tmp_path: Path) -> None
result = self._run("local", tmp_path=tmp_path)
assert result["pr_auto_detected"] is False
assert result["scope_kind"] == "branch"
- assert result["diff_scope"] == "main...HEAD"
+ assert result["diff_scope"] == "origin/main...HEAD"
def test_pr_auto_detected_when_scope_args_branch_literal(self, tmp_path: Path) -> None:
side_effect = self._mock_subprocess_side_effect(
]