Deferred from #1109, raised as a suppressed finding on 11daa23 and confirmed real rather than fixed there, so the merge was not held on it.
The defect
target_ref() in scripts/local_review.py resolves a target by trying origin/<value> first and falling back to the value as written. That ordering was deliberate and fixes a worse bug it replaced: treating any slashed value as already-qualified silently measured an ordinary branch such as release/v1 against the local branch of that name.
But the preference is unconditional, so it mis-scopes the case it was meant to preserve. If a caller names upstream/main and a branch literally called upstream/main also exists on origin, origin/upstream/main resolves and wins. A receipt recorded against upstream/main then scopes to origin instead, contradicting the documented intent that "another remote's branch can be named directly".
The fix
Honour a value that already resolves under refs/remotes/ as a fully-qualified remote-tracking ref before applying the origin/ preference. That keeps release/v1 resolving to origin/release/v1, since bare release/v1 is not a remote-tracking ref, while letting upstream/main win when it genuinely is one.
Why it needs a test, not just a fix
The reviewer's second point is the important one. The ambiguous case needs refs/remotes/upstream/main and refs/remotes/origin/upstream/main both present. Without that fixture the intended behaviour can be silently broken by the origin/ preference with nothing objecting, which is how the original defect got in.
scripts/tests/test_local_review.py already has test_a_target_that_only_exists_on_another_remote_is_used_as_written, which passes today only because the ambiguity is absent from its fixture. That case should be extended rather than duplicated.
Severity
Narrow: it needs a branch on origin whose name collides with a remote name. Filed rather than fixed inline because #1109 had already run eleven review rounds and the content was otherwise settled.
Deferred from #1109, raised as a suppressed finding on
11daa23and confirmed real rather than fixed there, so the merge was not held on it.The defect
target_ref()inscripts/local_review.pyresolves a target by tryingorigin/<value>first and falling back to the value as written. That ordering was deliberate and fixes a worse bug it replaced: treating any slashed value as already-qualified silently measured an ordinary branch such asrelease/v1against the local branch of that name.But the preference is unconditional, so it mis-scopes the case it was meant to preserve. If a caller names
upstream/mainand a branch literally calledupstream/mainalso exists on origin,origin/upstream/mainresolves and wins. A receipt recorded againstupstream/mainthen scopes to origin instead, contradicting the documented intent that "another remote's branch can be named directly".The fix
Honour a value that already resolves under
refs/remotes/as a fully-qualified remote-tracking ref before applying theorigin/preference. That keepsrelease/v1resolving toorigin/release/v1, since barerelease/v1is not a remote-tracking ref, while lettingupstream/mainwin when it genuinely is one.Why it needs a test, not just a fix
The reviewer's second point is the important one. The ambiguous case needs
refs/remotes/upstream/mainandrefs/remotes/origin/upstream/mainboth present. Without that fixture the intended behaviour can be silently broken by theorigin/preference with nothing objecting, which is how the original defect got in.scripts/tests/test_local_review.pyalready hastest_a_target_that_only_exists_on_another_remote_is_used_as_written, which passes today only because the ambiguity is absent from its fixture. That case should be extended rather than duplicated.Severity
Narrow: it needs a branch on origin whose name collides with a remote name. Filed rather than fixed inline because #1109 had already run eleven review rounds and the content was otherwise settled.