Skip to content

Commit f3ffb31

Browse files
wongkclaude
andcommitted
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>
1 parent 7ab9d43 commit f3ffb31

2 files changed

Lines changed: 63 additions & 1 deletion

File tree

CHANGELOG.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
77
### code-review v3.6.1
88

99
#### 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). A repo with no remote has no second view and keeps its local ref, so remote-less reviews are unchanged.
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.
1111
- 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.
1212
- `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.
1313

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

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6427,6 +6427,68 @@ def test_unpushed_base_commits_keep_the_local_ref_as_base(
64276427
).split()
64286428
assert reviewed == ["MINE.txt"]
64296429

6430+
def _build_diverged_base_repo(self, repo: Path) -> None:
6431+
"""Local ``main`` and ``origin/main`` diverge from a common ancestor.
6432+
6433+
Both kinds of staleness at once::
6434+
6435+
A1 ── A2 <- origin/main (pushed by someone else)
6436+
/
6437+
... ── B0 ─┤
6438+
\\
6439+
L1 ── L2 <- local main (unpushed)
6440+
6441+
Neither ref alone can base every branch cut from this repo: a branch
6442+
off ``L2`` needs the local ref, a branch off ``A2`` needs the remote
6443+
one. The later merge base with HEAD identifies the correct tip.
6444+
"""
6445+
repo.mkdir()
6446+
git_fixture(repo, "init", "--quiet", "-b", "main")
6447+
_commit_file(repo, "base.txt", "B0\n") # common ancestor
6448+
git_fixture(repo, "checkout", "--quiet", "-b", "origin-work")
6449+
_commit_file(repo, "A1.txt", "pushed by someone else\n")
6450+
a2 = _commit_file(repo, "A2.txt", "pushed by someone else\n")
6451+
git_fixture(repo, "update-ref", "refs/remotes/origin/main", a2)
6452+
git_fixture(repo, "checkout", "--quiet", "main")
6453+
_commit_file(repo, "L1.txt", "unpushed local base commit\n")
6454+
_commit_file(repo, "L2.txt", "unpushed local base commit\n")
6455+
6456+
def test_diverged_base_branch_off_local_tip_uses_local_ref(
6457+
self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
6458+
) -> None:
6459+
# Cut from the unpushed local tip: origin/main lags, so basing on it
6460+
# would fold the unpushed L1/L2 into the diff.
6461+
repo = tmp_path / "repo"
6462+
self._build_diverged_base_repo(repo)
6463+
git_fixture(repo, "checkout", "--quiet", "-b", "feat-x", "main")
6464+
_commit_file(repo, "MINE.txt", "the change under review\n")
6465+
monkeypatch.chdir(repo)
6466+
with patch("code_review_helpers._detect_open_pr", return_value=None):
6467+
result = TestResolveScope()._run("local", tmp_path=tmp_path)
6468+
assert result["diff_scope"] == "main...HEAD"
6469+
reviewed = git_fixture(
6470+
repo, "diff", "--name-only", result["diff_scope"],
6471+
).split()
6472+
assert reviewed == ["MINE.txt"]
6473+
6474+
def test_diverged_base_branch_off_remote_tip_uses_remote_ref(
6475+
self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
6476+
) -> None:
6477+
# Cut from origin/main while local main is also ahead with its own
6478+
# unpushed commits: basing on the local ref would fold A1/A2 in.
6479+
repo = tmp_path / "repo"
6480+
self._build_diverged_base_repo(repo)
6481+
git_fixture(repo, "checkout", "--quiet", "-b", "feat-x", "origin/main")
6482+
_commit_file(repo, "MINE.txt", "the change under review\n")
6483+
monkeypatch.chdir(repo)
6484+
with patch("code_review_helpers._detect_open_pr", return_value=None):
6485+
result = TestResolveScope()._run("local", tmp_path=tmp_path)
6486+
assert result["diff_scope"] == "origin/main...HEAD"
6487+
reviewed = git_fixture(
6488+
repo, "diff", "--name-only", result["diff_scope"],
6489+
).split()
6490+
assert reviewed == ["MINE.txt"]
6491+
64306492
def test_identical_base_views_resolve_to_the_remote_ref(
64316493
self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
64326494
) -> None:

0 commit comments

Comments
 (0)