fix(code-review): base local diff on the branch fork point, not a stale ref - #181
Merged
Conversation
…le 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>
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>
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>
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Local branch review hardcoded its diff scope to
main...HEAD. Thatmainis the local branch ref, which every worktree of a clone shares — so a worktree inherits whatever commit the primary checkout last leftmainon. BecauseA...Bdiffs frommerge-base(A, B), a localmainsitting behind the branch's fork point drags the merge base backwards and folds every commit that landed on the base in between into the review diff. The result: unrelated changes appear in the review whenever the worktree'smainhas advanced (or lagged) past the localmain.Why not just use
origin/mainPreferring
origin/mainunconditionally only mirrors the bug.origin/<base>lags whenever the base has unpushed local commits; branch off those and the fork point is ahead of the remote ref, so it folds the unpushed base commits in instead. Both directions are reproducible:main...HEADorigin/main...HEADmainstale (the reported bug)mainFix
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 merge base is the true fork point — correct under either kind of staleness. A repo with no remote has no second view and keeps its local ref (remote-less reviews unchanged). Nogit fetchis added, so this stays offline-safe.Two things came along:
origin/HEAD, thenorigin/main/origin/master, then local names, falling back tomain) instead of assumed to bemain, somaster-default repos resolve correctly.--base-ref-overrideruns through the same selection.fetch-intentwas reading commit subjects off the same stale base, attributing other people's landed commits to the branch under review; it now reads from the fork point too.Tests
TestResolveDiffBasebuilds real hermetic git repos (the pre-existingTestResolveScopetests asserted literal strings and were silently ambient-dependent). Mutation-checked both directions: reverting to the old local-ref behavior fails 6 tests; the naive always-prefer-origin fix fails the unpushed-commits test. Full gate green — 2088 passed, ruff + pyright clean. The seven byte-parity golden prefix fixtures pass unchanged.Note: the last two commits address a self-review (findings from
cr-64914, which reviewed this branch using the fixed base resolution) — a hallucinated subcommand name in a docstring, a stale(origin/main)in/starthelp, and moving the shared git-fixture runner intoconftest.pyper convention.plugins/code-reviewversion bumped 3.6.0 → 3.6.1.🤖 Generated with Claude Code