Skip to content

Commit 9e923ed

Browse files
wongkclaude
andauthored
fix(code-review): base local diff on the branch fork point, not a stale ref (#181)
* fix(code-review): base local diff on the branch fork point, not a stale ref Local branch review hardcoded its 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. Since `A...B` diffs from `merge-base(A, B)`, a lagging local ref drags the merge base back past the branch's real fork point and folds unrelated landed commits into the review diff. Preferring `origin/<base>` unconditionally would only mirror the bug: that ref lags whenever the base has unpushed local commits, and branching off those puts the fork point ahead of it. Both merge bases are ancestors of HEAD along the base branch, so pick the ref producing the later one — it is the true fork point under either kind of staleness. A remote-less repo has no second view and keeps its local ref. The base branch is now detected (origin/HEAD, then main/master) rather than assumed to be `main`, so master-default repos resolve correctly too, and fetch-intent reads commit subjects from the same fork point instead of attributing someone else's landed commit to the branch under review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(code-review): changelog + README for v3.6.1 diff-base fix Document the fork-point base selection and default-branch detection. The README's scope table, --base flag, and examples described the old hardcoded `main` base; a new Base ref resolution section covers detection and the later-merge-base rule. Also corrects README staleness surfaced by the inventory check: the architecture tree omitted the skills/ directory, three of four commands, four of seven reviewer prompts, and scripts/, and the verifier-stats section named an output artifact as GitHub mode's presenter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(code-review): address review findings on the diff-base fix Three findings from the cr-64914 review, all confirmed against the code: - _resolve_default_base_ref's docstring named a `compute-cache-keys` subcommand that does not exist; the consumer that origin-qualifies base_ref is `compute-hashes` (cmd_compute_hashes). - /start's help hardcoded the base as `(origin/main)`, contradicting the detection and local-ref fallback this same change added. - The hermetic git fixture runner was reached by importing a private helper out of prefix_golden_harness. It is shared test env setup used by two modules now, so it moves to conftest.py as GIT_IDENTITY_ENV + git_fixture and both modules import it from there. The harness's deliberate duplication is its independent walk wrapper (the A/B parity oracle), not its git plumbing, so sharing this does not weaken it — the seven byte-parity golden fixtures still pass unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(code-review): pin diff-base selection under local/origin divergence Coverage previously exercised the two staleness kinds separately (stale local ref; unpushed local commits). When local main and origin/main diverge, both happen at once: the branch is cut from one of the two tips, and _base_rev must pick that tip. Two complementary tests — branch off the local tip, branch off the remote tip — each pinned by mutation checks: an always-local base fails the off-remote case, always-origin fails the off-local case, only the per-run later-merge-base selection passes both. The v3.6.1 changelog bullet gains one clause stating the divergence case is handled, since that is a real property of the shipped selection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 1f07321 commit 9e923ed

8 files changed

Lines changed: 514 additions & 79 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.6.1
8+
9+
#### Fixed
10+
- **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/<base>`: 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/<base>` 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/<base>` 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.
11+
- 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/<ref>` that git cannot resolve.
12+
- `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.
13+
714
### code-review v3.6.0
815

916
#### Fixed

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.6.0",
4+
"version": "3.6.1",
55
"author": {
66
"name": "ClosedLoop",
77
"email": "support@closedloop.ai"

plugins/code-review/README.md

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,27 @@ plugins/code-review/
2424
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
2525
commands/
2626
start.md Main /start command (orchestrator)
27+
shallow.md /shallow wrapper — `/start --depth shallow`
28+
deep.md /deep wrapper — `/start --depth deep`
29+
cost.md /cost command — token-cost attribution from session transcripts
30+
skills/
31+
spawn-reviewers/SKILL.md Reviewer-fleet spawn/collection contract at stage_20_spawn_reviewers
32+
verify-findings/SKILL.md Finding-verifier fleet dispatch at stage_23_verify_findings (PLN-722)
33+
singleton-dispatch/SKILL.md Single-agent dispatch for stage_11_extract_signals / stage_15_coverage_critic (PLN-725)
34+
present-local/SKILL.md Local-mode presenter at stage_29_present
35+
fix/SKILL.md Verifies and fixes BLOCKING/HIGH findings from a prior review session
2736
prompts/
2837
github-review.md GitHub-mode constraints and output steps (loaded conditionally)
38+
scripts/
39+
dist/cost-report.mjs Bundled Node cost analyzer for /cost (sources at tools/code-review-cost/)
2940
tools/
3041
prompts/shared_prompt.txt Shared reviewer constraints injected into every agent prompt
3142
prompts/bha_suffix.txt Bug Hunter A reviewer persona and focus areas
3243
prompts/design_critic_suffix.txt Design Critic reviewer role (software-design craftsmanship; always-on at deep tier)
44+
prompts/impact_analyzer_prompt.txt Impact Analyzer reviewer role (FEA-1401 cross-file blast radius; deep tier, signal-gated)
45+
prompts/coverage_critic_prompt.txt Coverage critic role (standard/deep tiers)
46+
prompts/signal_extraction_prompt.txt Signal extraction role (standard/deep tiers)
47+
prompts/verifier_prompt.txt Finding-verifier role (falsify-oriented; PLN-722)
3348
python/code_review_schema.py Canonical Finding + ResultEnvelope schema + validators (PLN-719)
3449
python/test_code_review_schema.py Schema tests + round-trips
3550
python/code_review_helpers.py Deterministic helper CLI (parse-diff, hygiene, partition, route, validate, cache, finalize-result, arbitrate-budget, prepare-run, etc.)
@@ -90,34 +105,36 @@ Runs a comprehensive code review. Invokes the full pipeline: diff parsing, hygie
90105

91106
| Argument | Behavior |
92107
|---|---|
93-
| _(none)_ | Diff current branch vs `main` |
108+
| _(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 |
94109
| `staged` | Diff only staged (index) changes |
95-
| `file1 file2 ...` | Diff specific files against `main` |
110+
| `file1 file2 ...` | Diff specific files from the fork point off the default branch |
96111
| `123` | Use PR #123's diff (local output, no posting) |
97112

113+
**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 `<base>` and an `origin/<base>`, 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.
114+
98115
**Mode flags:**
99116

100117
| Flag | Description |
101118
|---|---|
102119
| `--github` | GitHub CI mode: auto-detect PR from branch or accept explicit PR number, post inline comments via file-based handoff |
103120
| `--github 123` | GitHub CI mode: review PR #123 specifically |
104121
| `--hygiene-only` | Run only the deterministic hygiene checks. Zero LLM tokens consumed. Fast. |
105-
| `--base <ref>` | Override the base branch for diffing (default: `main`) |
122+
| `--base <ref>` | Override the base branch for diffing (default: the repository's detected default branch) |
106123
| `--since-last-review` | Review only commits added since the last successful review (local mode only) |
107124
| `--full-review` | Force a full diff even when auto-incremental mode would narrow the scope |
108125
| `--depth shallow\|standard\|deep` | Reviewer-fleet tier. Default `standard`. See **Depth Tiers** below |
109126

110127
**Examples:**
111128

112129
```bash
113-
/start # All changes on current branch vs main
130+
/start # Open PR diff, else changes on current branch since its fork point
114131
/start staged # Only staged changes
115132
/start src/auth.ts src/user.ts # Specific files
116133
/start 123 # PR #123 diff locally
117134
/start --github # CI: auto-detect PR, post comments
118135
/start --github 123 # CI: PR #123, post comments
119136
/start --hygiene-only # Hygiene checks only
120-
/start --base develop # Diff against develop instead of main
137+
/start --base develop # Diff against develop instead of the default branch
121138
/start --since-last-review # Only new commits since last review
122139
/start --full-review # Disable incremental narrowing
123140
```
@@ -309,7 +326,7 @@ Overrides survive across runs while the file content matches and the 90-day TTL
309326

310327
**Re-assert is best-effort against finding_id drift.** Finding IDs are assigned as `<reviewer>_f<index>` where `<index>` 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.
311328

312-
The presenter (local mode `start.md`, GitHub mode `code-review-verifier-stats.md`) surfaces:
329+
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:
313330

314331
- Per-reviewer FP rate (`stats.verification.by_reviewer[*].fp_rate`)
315332
- Override count per reviewer (`stats.verification.by_reviewer[*].re_asserted`)

plugins/code-review/commands/start.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ Run a multi-agent code review with partitioned deep review, deterministic hygien
1313
## Usage
1414

1515
```
16-
/start # Review open PR diff for current branch, or main...HEAD if no PR
16+
/start # Review open PR diff for current branch, or the diff since the branch forked from the default branch if no PR
1717
/start staged # Review only staged changes
1818
/start file1 file2 # Review specific files
1919
/start 123 # Review PR #123 diff locally (no posting)

plugins/code-review/tools/python/code_review_helpers.py

Lines changed: 102 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -244,8 +244,8 @@ def _resolve_pr_scope(
244244
"""Resolve diff scope fields for a given PR number.
245245

246246
When *allow_guess_fallback* is ``True`` (explicit ``--pr-number``), a
247-
``CalledProcessError`` from ``gh pr view`` falls back to
248-
``base_ref="main"`` / ``head_ref=current_branch``. When ``False``
247+
``CalledProcessError`` from ``gh pr view`` falls back to the repo's
248+
default branch / ``head_ref=current_branch``. When ``False``
249249
(auto-detect path), errors propagate so the caller can revert to branch
250250
scope.
251251
"""
@@ -256,12 +256,12 @@ def _resolve_pr_scope(
256256
capture_output=True, text=True, check=True,
257257
)
258258
lines = result.stdout.strip().splitlines()
259-
base_ref = lines[0].strip() if len(lines) > 0 else "main"
259+
base_ref = lines[0].strip() if len(lines) > 0 else _resolve_default_base_ref()
260260
head_ref = lines[1].strip() if len(lines) > 1 else current_branch
261261
except subprocess.CalledProcessError:
262262
if not allow_guess_fallback:
263263
raise
264-
base_ref = "main"
264+
base_ref = _resolve_default_base_ref()
265265
head_ref = current_branch
266266

267267
return {
@@ -295,6 +295,92 @@ def _git_rev_parse(ref: str) -> str | None:
295295
return None
296296

297297

298+
def _resolve_default_base_ref() -> str:
299+
"""Return the repository's default branch *name* (e.g. ``main``, ``master``).
300+
301+
Probes, in order: the ``origin/HEAD`` symbolic ref (what the remote
302+
reports as its default), then well-known remote branches, then the
303+
same names locally for repos with no ``origin``. Falls back to
304+
``main`` when nothing resolves, preserving the historical default.
305+
306+
Returns a bare branch name, not a ref — callers pair it with
307+
:func:`_base_rev` to get the revision to diff against. ``base_ref``
308+
travels through ``scope.json`` as a name because consumers such as
309+
``compute-hashes`` origin-qualify it themselves.
310+
"""
311+
try:
312+
symbolic = _run_git(
313+
["symbolic-ref", "--short", "refs/remotes/origin/HEAD"],
314+
).strip()
315+
except (subprocess.CalledProcessError, FileNotFoundError, OSError):
316+
symbolic = ""
317+
if symbolic:
318+
# "origin/main" -> "main"
319+
_, _, name = symbolic.partition("/")
320+
if name:
321+
return name
322+
for candidate in ("main", "master"):
323+
if _git_rev_parse(f"origin/{candidate}"):
324+
return candidate
325+
for candidate in ("main", "master"):
326+
if _git_rev_parse(candidate):
327+
return candidate
328+
return "main"
329+
330+
331+
def _merge_base(a: str, b: str) -> str | None:
332+
"""Return the merge base of *a* and *b*, or ``None`` if there isn't one.
333+
334+
``None`` covers both "no common ancestor" and "a ref does not resolve",
335+
which callers treat alike: neither yields a usable fork point.
336+
"""
337+
try:
338+
return _run_git(["merge-base", a, b]).strip() or None
339+
except (subprocess.CalledProcessError, FileNotFoundError, OSError):
340+
return None
341+
342+
343+
def _is_ancestor(commit: str, descendant: str) -> bool:
344+
"""Whether *commit* is *descendant* or one of its ancestors."""
345+
try:
346+
_run_git(["merge-base", "--is-ancestor", commit, descendant])
347+
return True
348+
except (subprocess.CalledProcessError, FileNotFoundError, OSError):
349+
return False
350+
351+
352+
def _base_rev(base_ref: str, head_rev: str = "HEAD") -> str:
353+
"""Return the revision to diff *head_rev* against for branch *base_ref*.
354+
355+
``<base>...<head>`` diffs from ``merge-base(<base>, <head>)``, so the base
356+
ref matters only through the fork point it produces. A clone holds two
357+
views of the same base branch, and either one can lag the other:
358+
359+
* The local ``<base_ref>`` is shared by every worktree of a clone, so it
360+
carries whatever commit the primary checkout last left it on. When it
361+
sits behind the fork point, the merge base walks backwards and folds
362+
every commit that landed on the base in between into the review diff.
363+
* ``origin/<base_ref>`` lags whenever the base has unpushed local commits.
364+
Branch off those and the fork point is ahead of the remote ref, which
365+
folds the unpushed base commits into the diff instead.
366+
367+
Both merge bases are ancestors of *head_rev* along the base branch, so the
368+
later of the two is the true fork point — take the ref that produces it,
369+
which is correct under either kind of staleness. Falls back to whichever
370+
ref resolves when only one does (e.g. a remote-less repo, where the local
371+
ref is the only truth).
372+
"""
373+
remote_rev = f"origin/{base_ref}"
374+
local_mb = _merge_base(base_ref, head_rev)
375+
remote_mb = _merge_base(remote_rev, head_rev)
376+
if remote_mb is None:
377+
return base_ref
378+
if local_mb is None:
379+
return remote_rev
380+
# Equal bases resolve to the remote ref; the two ranges are identical.
381+
return remote_rev if _is_ancestor(local_mb, remote_mb) else base_ref
382+
383+
298384
# Startup-GC age guard: a PR-head worktree directory is reclaimed as an
299385
# abort-orphan only once it is older than this. Set far above any real
300386
# 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:
47914877
current_branch = "HEAD"
47924878

47934879
diff_scope = ""
4794-
base_ref = "main"
4880+
base_ref = _resolve_default_base_ref()
47954881
head_ref = current_branch
47964882
review_branch = current_branch
47974883
diff_tip = "HEAD"
@@ -4844,21 +4930,23 @@ def cmd_resolve_scope(args: argparse.Namespace) -> int:
48444930
pr_auto_detected = True
48454931
except (subprocess.CalledProcessError, FileNotFoundError,
48464932
OSError, ValueError):
4847-
# Any failure: fall back to branch scope
4933+
# Any failure: fall back to branch scope. base_ref is
4934+
# untouched here — every pr_scope assignment below the
4935+
# fetch is unreachable once either raise-point fires.
48484936
pr_number = None
48494937
pr_auto_detected = False
4850-
diff_scope = "main...HEAD"
4938+
diff_scope = f"{_base_rev(base_ref)}...HEAD"
48514939
scope_kind = "branch"
48524940
else:
4853-
diff_scope = "main...HEAD"
4941+
diff_scope = f"{_base_rev(base_ref)}...HEAD"
48544942
scope_kind = "branch"
48554943
elif scope_args.strip() == "staged":
48564944
diff_scope = "--cached"
48574945
scope_kind = "staged"
48584946
else:
48594947
# Treat scope_args as file paths
48604948
files = scope_args.strip()
4861-
diff_scope = f"main...HEAD -- {files}"
4949+
diff_scope = f"{_base_rev(base_ref)}...HEAD -- {files}"
48624950
path_filter = f"-- {files}"
48634951
scope_kind = "file_paths"
48644952

@@ -4870,11 +4958,12 @@ def cmd_resolve_scope(args: argparse.Namespace) -> int:
48704958
# Apply base-ref override if provided
48714959
if base_ref_override:
48724960
if scope_kind == "pr":
4873-
diff_scope = f"origin/{base_ref_override}...origin/{head_ref}"
4961+
override_head = f"origin/{head_ref}"
4962+
diff_scope = f"{_base_rev(base_ref_override, override_head)}...{override_head}"
48744963
elif path_filter:
4875-
diff_scope = f"origin/{base_ref_override}...HEAD {path_filter}"
4964+
diff_scope = f"{_base_rev(base_ref_override)}...HEAD {path_filter}"
48764965
else:
4877-
diff_scope = f"origin/{base_ref_override}...HEAD"
4966+
diff_scope = f"{_base_rev(base_ref_override)}...HEAD"
48784967
base_ref = base_ref_override
48794968

48804969
# Worktree isolation for local PR review. The diff is computed from the
@@ -4987,7 +5076,7 @@ def cmd_fetch_intent(args: argparse.Namespace) -> int:
49875076
elif scope_kind == "branch":
49885077
try:
49895078
result = subprocess.run(
4990-
["git", "log", f"{base_ref}..{diff_tip}",
5079+
["git", "log", f"{_base_rev(base_ref, diff_tip)}..{diff_tip}",
49915080
"--oneline", "--no-merges", "--format=%s"],
49925081
capture_output=True, text=True, check=True,
49935082
)

0 commit comments

Comments
 (0)