[askb]

Problem

Pull requests raised against a stable branch produce Gerrit changes on
master.

Reported by Robert Varga for OpenDaylight:
openflowplugin PR #39
targets stable/vanadium, but it landed as
change 127193
on master, where it conflicts with the legitimate master-targeted bump
(127192).

This surfaced now because ODL recently added Dependabot target-branch
entries for active stable branches
(topic dependabot-stable-branches),
so stable-branch PRs reach g2g for the first time.

Root cause

_push_to_gerrit resolves the target branch correctly (GITHUB_BASE_REF
→ stable/vanadium), logs it, and then never passes it to git-review.
The branch was deliberately omitted and .gitreview used instead — but on
Gerrit-mirrored repos defaultbranch is master on every branch:

$ git show origin/stable/vanadium:.gitreview
[gerrit]
host=git.opendaylight.org
project=openflowplugin.git
defaultbranch=master

So every change went to master, regardless of PR base.

Why simply appending the branch does not work

git-review declares branch as a trailing positional and --reviewers
with nargs='+' (git_review/cmd.py). A trailing branch is swallowed into
the reviewer list:

branch AFTER  reviewers: branch=None   reviewers=['b@x', 'main']
branch BEFORE reviewers: branch='main' reviewers=['b@x']

That is the original "git-review adds the branch name as a reviewer" bug
the old comment referred to — the previous code worked around the symptom
by dropping the branch entirely, which caused this issue.

The same output exposes a second, pre-existing bug: repeated
--reviewer flags make argparse keep only the last occurrence, so every
earlier reviewer was silently discarded.

Fix

Emit the branch first, then a single --reviewers flag. The
normalized-email retry path now reuses the same helper instead of trying to
unpick the reviewer flags — it previously did args[:-1] # Remove branch
against a command that had no trailing branch, so it stripped a reviewer.

Net result is less code than before.

Validation

• Full suite green (1,326 tests), pre-commit run clean on changed files.
• New tests parse the generated argv through git-review's own grammar, so
  the ordering contract fails loudly if it regresses.
• Verified the new tests fail against the old implementation:
  AssertionError: assert None == 'stable/vanadium', with reviewers
  reduced to a single address.

Follow-up (not in this PR)

Consumers pin this action by SHA, so a release is needed before ODL picks
it up. The already-misrouted Gerrit changes need abandoning and
re-pushing.

[Copilot]

🟢 Approval recommended

The focused changes address branch routing and reviewer preservation with targeted regression coverage and no unresolved findings.

No open issues 🎉

What changed in this PR

This PR fixes Gerrit submissions for pull requests targeting stable branches by explicitly passing the PR base branch to git-review, rather than relying on .gitreview defaults.

Changes:

• Places the target branch before a single --reviewers flag, preserving all reviewers.
• Reuses argument construction for normalized-email retries.
• Adds regression tests for branch routing, reviewer ordering, and retry behavior.

File	Description
tests/​test_ghe_and_gitreview_args.py	Updates argument assertions and adds branch-parsing and retry regression tests.
src/​github2gerrit/​core.py	Centralizes branch and reviewer arguments for initial pushes and retries.

_{Review effort: Balanced}

[askb]

Field evidence from the OpenDaylight Gerrit

Scope check against all 22 currently-open changes owned by the gh2gerrit
service user (account 9341) on git.opendaylight.org, cross-referencing each
change's GH-<repo>-<pr> topic back to the GitHub PR's real baseRefName:

12 of 22 are misrouted — every one of them landed on master while the
originating PR targeted a stable branch.

Gerrit	Project	Gerrit branch	PR base branch
127057	netconf	master	11.0.x
127175	controller	master	11.0.x
127176	controller	master	12.0.x
127177	odlparent	master	14.4.x
127179	aaa	master	0.21.x
127182	yangtools	master	15.1.x
127185	bgpcep	master	0.23.x
127186	openflowplugin	master	stable/titanium
127187	bgpcep	master	1.0.x
127189	aaa	master	0.22.x
127190	mdsal	master	15.0.x
127193	openflowplugin	master	stable/vanadium

Why three stable-branch changes did route correctly

Because the branch was never handed to git-review, routing silently fell
through to defaultbranch in each branch's .gitreview. That predicts the
outcome exactly, with no exceptions across the sample:

Project	Branch	.gitreview defaultbranch	Outcome
yangtools	14.0.x	14.0.x	routed correctly
netconf	10.0.x	10.0.x	routed correctly
releng/autorelease	stable/chromium	stable/chromium	routed correctly
yangtools	15.1.x	master	misrouted
netconf	11.0.x	master	misrouted
openflowplugin	stable/vanadium	master	misrouted

Branches whose .gitreview was refreshed at branch-cut happened to work;
branches that kept the inherited defaultbranch=master did not. The action
had already resolved the correct branch from github.base_ref in every case
and then discarded it — which is what this PR fixes.

The misrouted change that triggered the report (127193) conflicts with the
legitimate master-targeted bump (127192) in the same repo.

[ModeSevenIndustrialSolutions]

Review: fix verified ✅ (gaps remain; follow-up to come)

Verdict: the fix is correct, minimal and well tested, and fine to merge as it stands. It fixes the routing bug on every path that handles one PR at a time: pull_request_target events, @github2gerrit comments, workflow_dispatch with a PR_NUMBER, and the fanned-out legs of a reusable-workflow sweep. The gaps listed below are pre-existing or adjacent, and should not block this. They will be tracked in a single issue and fixed in a follow-up PR based on main once this merges.

---

Verification of this PR

The root cause is confirmed against the real git-review 2.5.0 binary, not only a parser model. I ran git-review --dry-run against a local repo whose .gitreview sets defaultbranch=master, with HEAD on stable/vanadium:

argv shape	push git-review would run
main today (no branch, repeated --reviewer)	HEAD:refs/for/master%topic=GH-demo-39,r=b@x.org
Branch appended after --reviewers	HEAD:refs/for/master%topic=GH-demo-39,r=a@x.org,r=b@x.org,r=stable/vanadium
This PR (branch, then one --reviewers)	HEAD:refs/for/stable/vanadium%topic=GH-demo-39,r=a@x.org,r=b@x.org ✅

Notes from the production run (openflowplugin run 40228174305):

• Only the push target was wrong. The run was PyPI github2gerrit 2.4.4 under action v2.4.3. I checked the misrouted commits on 127193, 127186, 127057 and 127175, and each one's parent is on the intended stable branch. The squash base was already correct; only the git-review argv was missing the branch.
• This also fixes the missing back-links. gerrit review --branch <stable> <sha> found nothing on misrouted changes, so the GHPR: link is missing on every one of them. 127192 has one; 127193, 127186 and 127057 have none.
• The "earlier reviewers dropped" bug is real. On main, only the last reviewer reaches Gerrit.

Tests and CI:

• The 3 new or updated tests in tests/test_ghe_and_gitreview_args.py fail on main with the assertions quoted in the description, and pass here.
• Full suite: 1976 passed. ruff and mypy are clean.
• Every ruleset workflow, DCO and signature check is green.
• Copilot's 🟢 review was on 93ecc08, whose tree is identical to this head (f874064).

Release note (correction to the follow-up section). Consumers run uvx --from github2gerrit unpinned, so the PyPI release is what reaches ODL. It does so on their next run, whatever action SHA they pin, and it reaches every consumer at once. To gauge the blast radius I checked 83 consumer repositories (ODL, ONAP, O-RAN-SC, FD.io, OpenCORD, LFIT). None has a .gitreview defaultbranch different from its GitHub default branch, so PRs to default branches keep pushing exactly where they do today.

---

Gaps not covered here (for the follow-up issue)

1. High: in-process bulk mode still misroutes, and does more damage.

• _resolve_target_branch() reads only environment variables and never reads gh.base_ref.
• _build_bulk_pr_tasks builds a correct per-PR base_ref. The PRs then run in a ThreadPoolExecutor, where GITHUB_BASE_REF is empty and can't differ per thread.
• Reproduced: with gh.base_ref='stable/vanadium' and only that branch fetched, the resolver returns master.
• The same resolver picks the squash base, so the commit is built on origin/master (dragging in stable-only history or conflicting) and then pushed to master.
• Dependabot heads are trusted, so _sweep_can_skip won't skip them.
• ODL's composite-action caller has a bare workflow_dispatch:. action.yaml normalises an empty PR_NUMBER to "0", which means sync_all, so one click on "Run workflow" takes this path. Reusable-workflow sweeps fan out to single-PR legs and are unaffected.
• Fix shape: resolve the branch from the per-PR context first, as _resolve_push_topic already does for the same threading reason. Keep honouring GERRIT_BRANCH and GITHUB_BASE_REF for compatibility.
• G2G_TMP_BRANCH has the same cross-thread problem: it's written at L4471 and L4605, and read at L5207 and L5466.

2. High: the follow-up plan "abandon and re-push" would close the PRs instead.

• cleanup_abandoned_prs_bulk is on by default and runs after every successful single-PR run. It closes any open PR whose first Gerrit URL in its comments points at an abandoned change, without checking that change's branch.
• On UPDATE-mode re-runs (synchronize, recheck), _find_change_by_github_hash matches with no status or branch filter, so it finds the abandoned change. _reconcile_final_state_changes then sees ABANDONED and closes the PR.
• Use Gerrit's Move Change instead. @rovarga already moved 127193, 127177 and 127190 this morning. It works on ODL's Gerrit 3.7.2 and keeps the change number, Change-Id and topic.
• The code version and each change's branch need to match. With the old code, a moved change gets a new duplicate on master at its next re-run; the three moved changes are in that state right now. With the new code, an unmoved change gets a duplicate on the stable branch.
• So: release, move the remaining 9, and avoid rechecks on those PRs in between. The 9 still on master are 127057 (netconf→11.0.x), 127175 and 127176 (controller→11.0.x / 12.0.x), 127179 and 127189 (aaa→0.21.x / 0.22.x), 127182 (yangtools→15.1.x), 127185 and 127187 (bgpcep→0.23.x / 1.0.x), and 127186 (openflowplugin→stable/titanium).

3. Medium: Change-Id lookups don't specify a branch.

• Affected:
  • _lookup_change_state (change:I… project:X, n=1)
  • _query_gerrit_for_results (limit:1)
  • the bare /changes/I… calls in _get_gerrit_change_details, _update_gerrit_change_metadata and _verify_single_change
• In ODL the same Change-Id on several branches is routine, because Gerrit cherry-picks keep it. One openflowplugin Change-Id spans master, stable/vanadium and stable/titanium.
• For that Change-Id, ODL's Gerrit answers 404 Multiple changes found.
• Worst case: a merged cherry-pick on another branch makes the state lookup report MERGED, and g2g closes a PR whose own change is still open.
• Fix shape: give state and detail lookups a branch scope (branch: predicate, or project~branch~Change-Id).

4. Medium (not seen in production yet): CLEANUP_GERRIT can abandon human cherry-picks.

• cleanup_closed_github_prs is on by default. It abandons any open change in the project whose GitHub-PR trailer points at a closed PR, without checking owner or branch.
• abandon_gerrit_change_for_closed_pr, which runs on PR closed events, takes the first match by trailer and has the same blind spot.
• Cherry-picks copy the trailer. There are at least 20 human-owned picks since June, e.g. mdsal 126368 on 14.0.x, and g2g's own parse_trailers extracts GitHub-PR from them.
• Zero incidents so far.

5. Low (only with ALLOW_DUPLICATES: false): duplicate detection doesn't filter by branch.

• _build_duplicate_query_path queries project:X status:open after:… and compares subjects with versions normalised to x.y.z.
• So the same Dependabot bump on two branches counts as a duplicate. The default is true, which is why ODL wasn't blocked.

6. Low: docs and smaller items.

• docs/features.md#L1141 still says the branch "defaults to master".
• GERRIT_BRANCH is documented as dispatch-only (README, docs/cli.md). In practice it's the top-priority branch override for forward pushes, and it's also accepted from config files. With this PR it now steers the push as well as the squash base.
• Nit: the _push_target_args docstring says defaultbranch is "always master" on stable branches. Per the field evidence (yangtools 14.0.x, netconf 10.0.x and others route correctly), "often" is accurate.
• Optional: git-review is a runtime dependency, so a test that runs the real git-review --dry-run, as above, would catch future grammar changes better than the re-implemented parser.

Already branch-correct, nothing needed:

• Strategy 5 dependency reuse and the post-push supersession sweep (both filtered by branch:; the sweep abandons by change number).
• The CI synthetic push (refs/for/{branch}).
• Reverse-flow closure: ODL's stable-branch PRs are being closed by g2g's "Automated PR Closure", e.g. opendaylight/netconf#60, opendaylight/docs#25, opendaylight/gnmi#32.

Deployment FYI. ODL's stable-branch caller workflows (pinned to v1.2.0, branches: [master, main]) are dead config for PR events. The run for PR #39 used the default-branch workflow (action v2.4.3), so ODL's master caller is the only control point.