From 4c9087dfe6cab218f6b7ab45ab5e41f63769d0ee Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 29 Sep 2026 16:50:27 -0700 Subject: [PATCH] Qualify local_review's Remote-Tracking Target So a Local Branch Cannot Shadow It (#2108) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `local_review.py`'s `target_ref` returned a remote-tracking target as its short name, such as `upstream/main` or `origin/develop`, and `merge_base` handed that name to `git merge-base`. Git resolves a short name through `refs/heads/` before `refs/remotes/`, so a local branch literally named like the remote-tracking ref silently defined the review scope. - `target_ref` now looks each remote-tracking candidate up by its exact name with `git show-ref --verify` (a new `remote_tracking_ref` helper) and returns it fully qualified as `refs/remotes/...`. A full-name lookup through `rev-parse` was not enough, since `rev-parse` also resolves `refs/remotes/` through a local branch of that full name. - An exact remote-tracking ref that does not resolve to a commit refuses rather than falling through to the as-written step, where a same-named local branch would win. - The final refusal names the three refs actually tried, and the `--target` help names the remote-tracking check that runs before `origin/`. - Tests pin a local branch shadowing the `upstream/main` form, the `origin/` form, and the full `refs/remotes/...` form, plus the non-commit refusal. Each new test was run against the unfixed `local_review.py` and fails there. The value only reaches `git merge-base`. Receipts store the target as the caller typed it, so no existing receipt changes key. Follow-ups filed from the local review passes rather than widening this change: - #2106: a dangling remote-tracking symref, or one naming a missing object, is rejected by `show-ref` and still falls through. This predates the change. - #2107: the `local-strict-review` Skill's `git merge-base origin/ HEAD` command still resolves the short name, so in the shadowed case it now disagrees with the engine. Closes on promotion: #1235 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Summary by CodeRabbit * **Bug Fixes** * Target selection now checks matching remote-tracking branches before `origin` or the target as written. Targets already qualified as remote-tracking refs are checked only as written, preventing local branches from shadowing remote branches or qualified targets from being reinterpreted under `origin`. * A remote-tracking ref that does not resolve to a commit now produces an error. * **Documentation** * Updated `--target` help text to explain the target resolution order. --------- Co-authored-by: Claude Opus 5.5 --- scripts/local_review.py | 72 ++++++++++++++++++++++-------- tests/test_local_review.py | 91 +++++++++++++++++++++++++++++++++++--- 2 files changed, 139 insertions(+), 24 deletions(-) diff --git a/scripts/local_review.py b/scripts/local_review.py index 38d2bfe9a..3dd660066 100755 --- a/scripts/local_review.py +++ b/scripts/local_review.py @@ -282,35 +282,68 @@ def ref_exists(ref: str, root: Path) -> bool: return True +def remote_tracking_ref(name: str, root: Path) -> str | None: + """The exact `refs/remotes/` ref, or None where `show-ref --verify` rejects that name. + + `rev-parse` runs its whole resolution list on a full name too, so `refs/remotes/` also + matches a local branch literally named that. `show-ref --verify` matches the name exactly. + Raises CannotRun where the ref it accepts does not resolve to a commit. + """ + ref = f"refs/remotes/{name}" + try: + git("show-ref", "--verify", "--quiet", ref, root=root) + except CannotRun: + return None + if not ref_exists(ref, root): + raise CannotRun( + f"{ref} does not resolve to a commit, so the review scope cannot be determined" + ) + return ref + + def target_ref(target: str, root: Path) -> str: """The ref a target name means, preferring the remote-tracking one. - A target that is already a remote-tracking ref as written, such as `upstream/main`, is used - as written before anything else is tried. Without that check first, the `origin/` - preference below is unconditional and can mis-scope this exact case: if a branch literally - named `upstream/main` also exists on `origin`, `origin/upstream/main` would resolve and win, - silently measuring the caller's explicitly named remote against `origin` instead. + A target that is already a remote-tracking ref as written, such as `upstream/main`, is tried + first, in its qualified form, before anything else. Without that check first, the + `origin/` preference below is unconditional and can mis-scope this exact case: if a + branch literally named `upstream/main` also exists on `origin`, `origin/upstream/main` would + resolve and win, silently measuring the caller's explicitly named remote against `origin` + instead. - Otherwise, `origin/` is tried next and used whenever it resolves, so an ordinary - fleet branch name works and so does one holding a slash. Treating any slash as "already a - full ref", which an earlier version did, silently measured a target such as `release/v1` - against the local branch of that name rather than the remote one, and a local branch that - has moved on then defines the review scope with no error at all. + Otherwise, `origin/` is tried next as a remote-tracking ref, so an ordinary fleet + branch name works and so does one holding a slash. Treating any slash as "already a full ref", + which an earlier version did, silently measured a target such as `release/v1` against the + local branch of that name rather than the remote one, and a local branch that has moved on + then defines the review scope with no error at all. A value that resolves only as written is used as written last, which is what lets a fork-based flow name another remote's branch even when it is not already a remote-tracking ref (for example a local-only branch checked out from that remote). + + A remote-tracking result is matched by its exact name and returned fully qualified, as + `refs/remotes/...`. Git resolves a short name such as `upstream/main` against `refs/heads/` + before `refs/remotes/`, so a local branch literally named that would otherwise define the + review scope in place of the remote-tracking ref this function chose. + + A target already written as `refs/remotes/...` is not also tried under `origin/`, where a + branch literally carrying that name would win. """ - if ref_exists(f"refs/remotes/{target}", root): - return target - remote = f"origin/{target}" - if ref_exists(remote, root): - return remote + prefix = "refs/remotes/" + if target.startswith(prefix): + names: tuple[str, ...] = (target.removeprefix(prefix),) + else: + names = (target, f"origin/{target}") + for name in names: + ref = remote_tracking_ref(name, root) + if ref is not None: + return ref if ref_exists(target, root): return target + tried = " or ".join(f"{prefix}{name}" for name in names) raise CannotRun( - f"neither {remote} nor {target} resolves in this checkout," - " so the review scope cannot be determined" + f"no remote-tracking ref of a commit exists at {tried}, and {target} does not resolve" + " in this checkout, so the review scope cannot be determined" ) @@ -1039,8 +1072,9 @@ def main(argv: list[str] | None = None) -> int: "--target", default=None, help=( - f"target branch (default {DEFAULT_TARGET}). Resolved as origin/ where that" - " exists, else as written, so another remote's branch can be named directly" + f"target branch (default {DEFAULT_TARGET}). Resolved as the remote-tracking ref" + " , then as origin/ unless already starts with" + " refs/remotes/, else as written, so another remote's branch can be named directly" ), ) diff --git a/tests/test_local_review.py b/tests/test_local_review.py index 9d1012a88..f8cdf56d6 100755 --- a/tests/test_local_review.py +++ b/tests/test_local_review.py @@ -228,13 +228,19 @@ def test_a_slashed_target_prefers_the_remote_over_a_local_branch(self) -> None: remote = run(self.tmp, "rev-parse", "origin/release/v1").strip() local = run(self.tmp, "rev-parse", "release/v1").strip() self.assertNotEqual(remote, local, "fixture does not distinguish the two") - self.assertEqual(local_review.target_ref("release/v1", self.tmp), "origin/release/v1") + self.assertEqual( + local_review.target_ref("release/v1", self.tmp), "refs/remotes/origin/release/v1" + ) self.assertEqual(local_review.merge_base("release/v1", self.tmp), remote) - def test_a_target_that_only_exists_on_another_remote_is_used_as_written(self) -> None: + def test_a_target_that_only_exists_on_another_remote_is_qualified_from_that_remote( + self, + ) -> None: """This is what lets a fork-based flow name an upstream branch directly.""" run(self.tmp, "update-ref", "refs/remotes/upstream/main", "HEAD") - self.assertEqual(local_review.target_ref("upstream/main", self.tmp), "upstream/main") + self.assertEqual( + local_review.target_ref("upstream/main", self.tmp), "refs/remotes/upstream/main" + ) def test_an_explicit_remote_tracking_target_wins_over_a_same_named_origin_branch(self) -> None: """An explicitly named remote-tracking ref must not lose to the origin/ preference. @@ -256,7 +262,81 @@ def test_an_explicit_remote_tracking_target_wins_over_a_same_named_origin_branch run(self.tmp, "rev-parse", "upstream/main").strip(), "fixture does not distinguish the two", ) - self.assertEqual(local_review.target_ref("upstream/main", self.tmp), "upstream/main") + self.assertEqual( + local_review.target_ref("upstream/main", self.tmp), "refs/remotes/upstream/main" + ) + + def test_a_same_named_local_branch_cannot_shadow_a_remote_tracking_target(self) -> None: + """Git resolves a short name against refs/heads/ first, so the ref must be qualified. + + The remote-tracking `upstream/main` sits at the base commit, and a local branch of the + same name sits at the task tip. Resolving the short name picks the local branch and + yields the task tip as the merge base, while the qualified ref yields the base commit. + """ + base = run(self.tmp, "rev-parse", "HEAD").strip() + run(self.tmp, "update-ref", "refs/remotes/upstream/main", base) + (self.tmp / "moved.txt").write_text("task moved on\n", encoding="utf-8") + run(self.tmp, "add", "moved.txt") + run(self.tmp, "commit", "-m", "task work") + run(self.tmp, "branch", "upstream/main", "HEAD") + tip = run(self.tmp, "rev-parse", "HEAD").strip() + self.assertNotEqual(base, tip, "fixture does not distinguish the two") + self.assertEqual(local_review.merge_base("upstream/main", self.tmp), base) + + def test_a_local_branch_named_origin_target_cannot_shadow_the_origin_ref(self) -> None: + """The `origin/` step is qualified too, since the fleet default takes it. + + `refs/remotes/origin/feat` sits at the base commit, and a local branch literally named + `origin/feat` sits at the task tip, which the short name would resolve to first. + """ + base = run(self.tmp, "rev-parse", "HEAD").strip() + run(self.tmp, "update-ref", "refs/remotes/origin/feat", base) + (self.tmp / "moved.txt").write_text("task moved on\n", encoding="utf-8") + run(self.tmp, "add", "moved.txt") + run(self.tmp, "commit", "-m", "task work") + run(self.tmp, "branch", "origin/feat", "HEAD") + tip = run(self.tmp, "rev-parse", "HEAD").strip() + self.assertNotEqual(base, tip, "fixture does not distinguish the two") + self.assertEqual(local_review.merge_base("feat", self.tmp), base) + + def test_a_qualified_target_is_not_also_tried_under_origin(self) -> None: + """An origin branch literally named `refs/remotes/origin/main` must not replace the ref. + + The requested `refs/remotes/origin/main` sits at the base commit, and the nested + `refs/remotes/origin/refs/remotes/origin/main` sits at the task tip. + """ + base = run(self.tmp, "rev-parse", "HEAD").strip() + run(self.tmp, "update-ref", "refs/remotes/origin/main", base) + (self.tmp / "moved.txt").write_text("task moved on\n", encoding="utf-8") + run(self.tmp, "add", "moved.txt") + run(self.tmp, "commit", "-m", "task work") + run(self.tmp, "update-ref", "refs/remotes/origin/refs/remotes/origin/main", "HEAD") + tip = run(self.tmp, "rev-parse", "HEAD").strip() + self.assertNotEqual(base, tip, "fixture does not distinguish the two") + self.assertEqual(local_review.merge_base("refs/remotes/origin/main", self.tmp), base) + + def test_a_remote_tracking_ref_naming_a_non_commit_refuses_rather_than_falls_through( + self, + ) -> None: + """Falling through would reach the as-written step, where a same-named branch wins.""" + tree = run(self.tmp, "write-tree").strip() + run(self.tmp, "update-ref", "refs/remotes/upstream/main", tree) + run(self.tmp, "branch", "upstream/main", "HEAD") + with self.assertRaises(local_review.CannotRun): + local_review.target_ref("upstream/main", self.tmp) + + def test_a_local_branch_named_like_a_qualified_remote_ref_is_not_remote_tracking( + self, + ) -> None: + """`rev-parse` also resolves a full name through `refs/heads/`, so the match is exact. + + A local branch literally named `refs/remotes/upstream/main`, with no remote-tracking ref + of that name, must leave the target unresolved rather than be taken as remote-tracking. + """ + run(self.tmp, "branch", "refs/remotes/upstream/main", "HEAD") + self.assertIsNone(local_review.remote_tracking_ref("upstream/main", self.tmp)) + with self.assertRaises(local_review.CannotRun): + local_review.target_ref("upstream/main", self.tmp) def test_a_target_resolving_nowhere_is_a_boundary(self) -> None: with self.assertRaises(local_review.CannotRun): @@ -1128,7 +1208,8 @@ def test_the_backend_is_given_the_merge_base_not_the_target_tip(self) -> None: argv = argv_log.read_text(encoding="utf-8").split("\n") self.assertIn("--agent", argv) self.assertIn(base, argv, f"the backend was not given the merge base: {argv}") - self.assertNotIn("origin/develop", argv, "the backend was given the target tip") + for tip in ("origin/develop", "refs/remotes/origin/develop"): + self.assertNotIn(tip, argv, "the backend was given the target tip") # The flag name matters as much as the value. # The CLI documents --base as taking a branch and --base-commit as taking a commit hash. # A sha handed to --base is the wrong call even though the sha itself is right.