ci: automate v1 backports from the v1-needed label

[baptmont]

Link to Issue or Description of Change

2. Or, if no issue exists, describe the change:

Problem:

A fix that applies to both lines needs two pull requests, and the second one is
hand-made every time. Six of the fifteen commits on v1 are backports, several
rolling up five main PRs each, so the work was already being batched to keep
the cost bearable.

The obstacle is not the cherry-pick. main is google.golang.org/adk/v2 and
v1 is google.golang.org/adk, so every Go file's import block differs between
the branches and any patch touching imports conflicts.

Solution:

A v1-needed label says a change on main still owes a 1.x equivalent. This
makes it do something: merging a labelled PR replays its squash commit onto a
branch cut from v1 and opens the backport PR. v1 and v2 stay
informational, recording which branch a PR targets.

That module path difference is a pure string rewrite, so the patch is rewritten
before it is applied and the common case lands untouched. Genuine drift still
conflicts and still goes to a human — the run comments on the original PR asking
for a manual backport rather than pushing something half-applied.

One backport PR per original PR. An earlier revision batched the whole queue
onto one branch, which made a single conflict fatal to the run and put the
"which PRs are already done" bookkeeping into parsed prose. Both problems went
away with the batching; see the review thread. A conflict now costs its own
backport and nothing else.

Two decisions worth surfacing for review:

• It runs on the built-in GITHUB_TOKEN; no secret. The usual objection is
  that a pull request opened with GITHUB_TOKEN never triggers workflow runs,
  so the backport PR would arrive with no CI and could never be merged. That
  stopped being true in June
  2026:
  a PR opened by github-actions[bot] does trigger its pull_request
  workflows, in an approval-required state — the runs are created and held
  until someone with write access clicks Approve workflows to run.

  So the choice is one click per backport, against a personal access token bound
  to one person's account, outliving every job it is used in, and needing an org
  approval before it works at all. That click lands on a pull request a human
  has to review and merge anyway. What it does need is the "Allow GitHub
  Actions to create and approve pull requests" repository setting (Settings →
  Actions → General, possibly inherited from the org). A workflow token cannot
  read that setting, so it is not checked up front: gh pr create fails on it
  and the script recognises that specific failure and names it, rather than
  leaving a pushed branch and a bare 403 to interpret.

• It triggers on push, not pull_request_target. The job holds a token
  that can push and open pull requests, and pull_request_target would grant
  that to a workflow running in a pull request's context. Since the work is
  driven off the label queue rather than the event payload, push costs
  nothing: a nightly schedule catches PRs labelled after they merged, and
  workflow_dispatch takes PR numbers or all.

The queue clears itself. A PR leaves it when its change reaches v1 — matched
on the (cherry picked from commit <sha>) trailer the script writes, so the
test is an exact match on something the automation owns rather than a number
scraped out of a title — or while its backport/v1/pr-<n> branch exists.
Deleting that branch re-queues the PR, which is how a stale backport is
regenerated.

Testing Plan

No Go code changes, so there are no unit tests to add. The shell and workflow
logic was verified against real repository history rather than by inspection.

Replay correctness, against backports that were already done by hand. Each
was replayed onto the v1 tree as it stood before the hand-made backport
landed, and the result compared to it:

• #1301 → reproduces 95a40be6 byte for byte (git diff against it is
  empty). This is the regression test for the whole apply path.
• #1252 → rejects exactly the hunks touching IsolationScope, a field v1
  never had. Correct outcome: 4 of 7 hunks applied, .rej files left behind.
• #1195 → needs --skip-gomod, and then applies. Correcting an earlier
  claim in this description: I previously wrote that fix(session): enforce user ownership in VertexAI DeleteSession #1195 "fails as a raw
  cherry-pick, applies cleanly once rewritten". That is wrong. It does not apply
  cleanly with or without the module rewrite — the rejecting hunk is a
  dependency line in go.mod, which the rewrite does not touch, because the 1.x
  dependency set differs from main's. I re-ran the previous revision of the
  script to confirm this was never true rather than a regression here. With
  --skip-gomod the rest applies, though the result still differs from the
  hand-made backport, which also adapted a test — the case for "a clean apply is
  not a correct backport".

Also verified: the queue lists and self-clears against the live API;
--list / explicit-number / bad-input paths; that the (cherry picked from commit <sha>) lookup finds all 27 such trailers already on v1, including
#1301's; that a PR already on v1 is excluded rather than replayed; YAML
validity; and that no run: step interpolates ${{ }} directly (everything
passes through env, per the template-injection fix in #1297).

Found by testing, fixed here: removing a worktree directory without
unregistering it leaves git refusing to re-create it as "missing but already
registered", which broke the second replay of any PR on the same machine. CI
would not have hit it — runners are fresh and each PR gets its own worktree
path — so this was a local-only fault on exactly the manual path the conflict
message tells you to use.

What has not been tested: the assembled workflow has never run against the
live API, because that needs a merged v1-needed PR. Each piece is verified
individually; the end-to-end path is not. The first real merge is the actual
test, and is also where we find out whether "Allow GitHub Actions to create and
approve pull requests" is enabled here — if it is not, the run pushes the branch
and stops with a named error rather than a bare 403.

Manual E2E:

scripts/backport.sh --list          # pending queue
scripts/backport.sh 1301            # no-op: already on v1
scripts/backport.sh 1252            # partial apply + .rej, as expected

Replays happen in a scratch worktree under $TMPDIR, so the current checkout is
never touched, and nothing is pushed without --pr.

Checklist

• I have read the CONTRIBUTING.md document.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have added tests that prove my fix is effective or that my feature works.
  — no Go code here, so there is no unit test to add. Verified against real
  repository history instead; see the Testing Plan.
• New and existing unit tests pass locally with my changes.
  — zero Go files changed, so the suite is unaffected and I did not run it.
  Claiming otherwise would be inaccurate.
• I have manually tested my changes end-to-end — the script, against
  three historical backports. The workflow has not run against the live API;
  that needs a merged v1-needed PR.
• Any dependent changes have been merged and published in downstream modules.
  — not applicable.

Additional context

CONTRIBUTING.md gains a "Backporting to v1" section documenting the three
labels, the workflow, and the two things the tooling deliberately does not do:
a clean apply is not a correct backport (the hand-made #1195 also adapted a
test, so a patch can apply and still not be right), and dependency changes need
judgement because the 1.x dependency set differs.

[karolpiotrowicz]

The trigger choice holds up — push over pull_request_target, the action pinned to a full SHA, and no ${{ }} reaching a run: body. I replayed #1301 through the script against real history and it reproduces the hand-made backport exactly, so the module-path rewrite is doing what it claims. The problems below are all in the failure paths rather than the happy one.

Line references are pinned to d00dbb4.

Three things I think need fixing before this lands.

One conflicting PR stops every backport, and keeps commenting. apply_pr calls exit 1 from inside the apply loop, so the whole run ends at the first conflict. The queue is oldest-first, nothing removes v1-needed, and the push happens after the loop — so the PRs behind the conflicting one are never attempted and the ones that already applied are thrown away with the runner. I ran this with a queue of [#1252 conflicting, #1301 clean] and #1301 was never touched. Because the workflow fires on every push to main plus the nightly cron, and the comment step has no dedup, the same "please backport by hand" message lands on that contributor's PR several times a day until someone intervenes.

The exclusion set matches any #N in any open v1 PR title. The commit-body channel is carefully constrained by the bullet regex, but titles get no equivalent guard — they go straight into grep -oE '#[0-9]+'. An open v1 PR titled fix: address review feedback from #1301 silently drops #1301 from the queue, and --list then reports nothing pending. This is the same class of failure the comment above that code calls the one thing the tool must not have, reached through the channel the repository does not control. Constraining the match to the trailing (#a, #b) shape the script itself writes would close it, or better, key off something the automation owns rather than free text.

--branch is unvalidated and reaches git push directly. The value set at line 496 arrives unchecked at git push --set-upstream "${remote}" "${branch}", with nothing requiring a backport/ prefix — line 378 only supplies a default when the flag is absent. Two things make that reachable rather than theoretical. The workflow's deliberate word-split means a dispatch value of --branch <name> becomes flags rather than data, and line 431 prints --branch to the user as the documented way to finish a two-step run. Naming v1 there is a plausible thing for someone to type. A guard before the push — rejecting anything outside backport/v1/ — covers both, and unlike validating the dispatch input it also covers the local invocation the docs teach.

Worth fixing in the same pass:

• A failed gh pr list does not stop the --all path. The die at line 207 runs on the left of the pipeline at 223, inside a function only ever called from a command substitution (336, 351). shopt -s inherit_errexit is off by default, so the failure is swallowed and the run continues with an empty exclusion set at exit 0. I confirmed it both ways: the explicit-PR-number path at 366 does fail closed, and setting inherit_errexit makes --all fail closed too. The two paths look identical on the page, which is what makes this easy to miss.
• An empty patch is skipped but still advertised. Line 269 returns without removing the number from prs, so refs and the batch title claim a PR that has no commit on the branch. That title is what the next run reads back, so the PR is excluded from the queue permanently having never been backported. Reachable through --skip-gomod, which the script's own conflict message recommends. With a single PR it is worse in a different way: the branch is byte-identical to v1, so the push succeeds and gh pr create then fails on a zero-commit head, leaving an orphan branch behind.
• Branch names are not unique per run. batch-$(date +%Y%m%d) collides on the second batch of a day, and the resume check only looks at local refs, which never exist on a fresh runner. The push is then rejected non-fast-forward and the run fails identically every time after that. The comment step's grep finds nothing in that case, so the only signal is a red badge. A closed backport PR whose branch was kept gets you to the same place.

Smaller things, no need to hold the PR for them:

• confirm_checks returning 1 as the last statement of main makes the script exit non-zero after a backport that fully succeeded, and the workflow then reports "backport failed".
• The failure comment's log scrape matches the phrase anywhere in the log, including inside a PR subject echoed at line 248. A subject containing that wording wins head -1 over the real warning, so the comment goes to the wrong number and the conflicting PR gets none.
• With a body-less squash commit — around 12% of recent main commits — the cherry-pick trailer has no blank line before it, so git folds it into the subject and the single-PR PR title ends in a bare 40-character SHA.
• --force is inert with --all: the guard at 364 skips the block entirely, and pending_queue never consults it.
• --limit 200 on both list calls truncates with no signal, and v1-needed is never removed, so the queried set only grows. Not a problem today, but it fails silently when it does.
• A couple of doc mismatches: the printed verify step uses a bare go work init where AGENTS.md prescribes test -f go.work || go work init, and "re-running is a no-op" holds in CI but errors locally with "branch already exists".

Nothing here needs a redesign — the shape of the tool is right. The theme is that the failure paths assume they will be rare, and a few of them are load-bearing on a branch where mistakes are hard to undo.

[baptmont]

Thanks — this was a genuinely useful review, and the framing at the end ("the failure paths assume they will be rare") turned out to be the actionable part. Pushed 5f9413b.

Five of your six substantive points came from two design choices rather than from the code, so I removed the choices instead of patching the symptoms.

Batching is gone. One backport PR per original PR, each replayed on its own branch in its own worktree.

• One conflicting PR stops every backport — a conflict now costs its own backport. apply_pr no longer exists in a form that can end the run; each PR is an independent iteration and the loop tallies outcomes. Your [#1252, #1301] case now produces a PR for fix(skilltoolset): accept scalar allowed-tools frontmatter #1301 and a comment on fix(session): give Event a consistent JSON encoding #1252.
• Repeated comments — the comment carries a hidden marker and is posted only if that marker is not already on the PR. I kept v1-needed on rather than swapping to a conflict label, so a backport that conflicts today can still land on its own if a later v1 change makes it apply. It retries quietly instead of loudly.
• Empty patch still advertised — an empty patch returns a distinct status and the number never reaches a title or body. There is no batch title left for it to lie in.
• Branch names not unique per run — the name is now backport/v1/pr-<n>, a function of the PR number. It cannot collide, and it doubles as the in-flight check, replacing the local-ref test that could never fire on a fresh runner.

"Already backported" no longer parses prose. You suggested keying off something the automation owns; the script already writes (cherry picked from commit <sha>) on every backport, so that is now the key:

git log "${remote}/v1" --fixed-strings --grep="cherry picked from commit ${sha}"

• The exclusion set matches any #N in any open v1 PR title — no titles are read at all now. fix: address review feedback from #1301 has nothing to match.
• A failed gh pr list does not stop the --all path — the whole backported_prs function is gone, and shopt -s inherit_errexit is on so the next one of these cannot hide the same way. Your diagnosis was exactly right, and it is worth recording why it was invisible: the die is fine, but pending_queue was called from $( ), and errexit does not cross that boundary without the shopt.

I verified the trailer lookup against real history first rather than assuming: all 27 existing cherry picked from commit trailers on v1 are found, including #1301's, so the hand-made backports are correctly recognised.

Kept from your list as straightforward fixes:

• The push refuses any branch outside backport/v1/ — the guard sits immediately before git push, as you suggested, so it covers the local invocation too and not just the dispatch input. The dispatch input is validated as digits-only as well, since the word-split is deliberate.
• A branch with no commits is never pushed (rev-list --count), which removes the orphan-branch-plus-failed-gh pr create outcome.
• confirm_checks returning 1 after a successful backport: fixed in the previous push, before this rewrite.
• The log-scrape that could comment on the wrong PR: gone. The script comments directly, since it knows which PR it is on, and the workflow's failure step is deleted.
• The body-less squash commit folding the trailer into the subject: fixed. Command substitution strips trailing newlines, so the blank line before the trailer is now unconditional.

Failure modes are also distinguished now, which they were not: a conflict is a normal outcome that comments once, keeps the PR queued and leaves the run green, while a failed push or gh pr create fails the run — nobody has been told in that case, so it should be red.

Dropped with the batching: --branch, --worktree, --force, --watch and the two-step resume flow, all of which existed to make a failed batch recoverable. --list, --pr and --skip-gomod remain. Net −121 lines, and rather more than that in concepts.

One correction to my own description, which you should not take on trust from the old testing plan. I had claimed #1195 "fails as a raw cherry-pick, applies cleanly once rewritten", and cited it as the case justifying the module rewrite. That is wrong. #1195 does not apply cleanly with or without the rewrite — the rejecting hunk is a dependency line in go.mod, which the rewrite does not touch. I re-ran the previous revision of the script to confirm this was never true rather than something I broke. It needs --skip-gomod, and even then the result differs from the hand-made backport, which also adapted a test. #1301 remains the honest regression test, and it still reproduces 95a40be6 byte for byte.

One trade-off this introduces, which batching did not have: independent branches are each cut from v1 HEAD, so two backports touching the same lines will conflict at merge time rather than being sequenced onto one branch. GitHub surfaces it, and deleting the stale branch re-queues the PR so the next run regenerates it cleanly. Given the volume — the queue is currently empty and v1 has taken six backports in its life — I think that is the right trade, but it is a real regression versus batching and I would rather flag it than have it surprise someone.

Still unproven, as before: the workflow has never run against the live API, and whether "Allow GitHub Actions to create and approve pull requests" is enabled on this repository is not readable from a workflow token. If it is off, the run pushes the branch and stops with a named error rather than a bare 403.

Two of your smaller items I have deliberately not done: --force is gone along with the flag itself, and --limit 200 is still there — with the queue label-scoped and self-clearing I would rather leave it until it is a real ceiling than add pagination nobody exercises. Say the word if you would prefer it handled now.

[karolpiotrowicz]

The module-path rewrite does what you claim: I replayed #1301 onto v1 as it stood before 95a40be and got exactly the same files, down to the tree hash. The push-instead-of-pull_request_target reasoning holds up, and so does the GITHUB_TOKEN argument — nothing here needs a long-lived credential. Five things need fixing first, and they share a shape: the script does the wrong thing and reports success.

The replay is tied to a named local branch, but the push resolves that name rather than the commit. backport.sh:218 is the only thing binding backport/v1/pr-<n> to the replayed commit, and its exit status is unchecked. If that branch already exists and is checked out anywhere in the clone, checkout -B fails, the worktree stays detached, and the commit is made on a detached HEAD. Both guards then pass while checking something other than what gets pushed: :297 tests a string built from the PR number and can never fail, and :299 counts commits on HEAD. :303 then pushes the ref name, which resolves to the pre-existing branch. The result is a backport PR carrying your own unrelated commit, whose description says it is identical to the original, on a run that reports backported 1 ... failed 0.

Both halves go away if the worktree stays detached: drop the checkout -B, push HEAD:refs/heads/${branch}, and drop the git branch -D. That also makes the :299 guard check the same ref that gets pushed. Neither variant can fire on a fresh runner, so this is a hazard for anyone driving the script by hand.

Reproduction

Needs a PR that is actually pending in the queue, so substitute a live number for <n>.

git switch -c backport/v1/pr-<n> origin/v1
git commit --allow-empty -m "unrelated local work"
git switch -                       # branch stays checked out in a worktree

scripts/backport.sh <n> --pr

==> PR #<n>: <subject>
fatal: 'backport/v1/pr-<n>' is already used by worktree at <path>
==>   applied cleanly
==>   pushing backport/v1/pr-<n>
==>   https://github.com/google/adk-go/pull/<new>
==> backported 1, conflicted 0, empty 0, failed 0

The commit now on the remote branch is the empty one, not the replay. Exit code 0.

The variant where the branch exists but is not checked out is quieter and loses data instead: -B resets it to the replay and git branch -D force-deletes it at the end, leaving whatever was there unreferenced by any ref.

The recovery procedure the bot posts destroys the work it asks for. The comment at backport.sh:184-185 says to resolve the rejects and then "Re-run with --pr once it builds". A re-run does not resume — :213-214 force-removes the worktree and rm -rfs the path before replaying, so the resolution is gone and the same patch conflicts for the same reason. I ran those steps against #1252: the hand-resolved commit ends up reachable only from the reflog, nothing is pushed, and the run exits 0. The terminal text at :256-259 is the procedure that works, and is what the posted comment should say.

An infrastructure failure is reported to the contributor as a conflict in their patch. Because backport_one is invoked as backport_one ... || status=$? at :410, set -e is inert for the whole function body. A failing git worktree add — disk, permissions, a stale registration the prune did not clear — falls through to git apply, which fails because the directory is not there, and comment_conflict posts "this change does not replay cleanly, which usually means 1.x lacks something 2.x already had" on a merged PR whose patch is fine. The run stays green. The same suppression covers the git commit that follows it.

die inside the per-PR path kills the whole run. die is exit 1, which the || status=$? at :410 does not catch — that suppresses errexit, not exit. So :204-205 and the two guards before the push abort mid-drain: every later PR is skipped and the summary line never prints. The documented contract for the function is that these return 3 and main collects them, which already produces a red run after draining the rest.

A branch pushed with no PR behind it silences that backport permanently. in_flight asks only whether the remote branch exists. If the push succeeds and gh pr create then fails, the branch stays and every later run reports "nothing pending" and exits 0 — one red run, then silence, with no backport PR in existence. The likeliest trigger is exactly the condition you handle at :327-331, so this is what the first real run looks like if "Allow GitHub Actions to create and approve pull requests" turns out to be off. Deleting the pushed branch on that return 3 path would make it retryable.

One thing I could not settle, and it is the largest of these if it is real. allow_rebase_merge is enabled on the repository. For a rebase merge, mergeCommit.oid is the last rebased commit rather than a squash of the branch, and it has a single parent either way, so the assumption stated at :228 would not hold. A multi-commit PR merged that way is backported partially, applies cleanly, and then clears itself from the queue on the trailer, permanently. Every one of the last 60 commits on main carries the squash signature, so this has never happened — but it is one button away and nothing asserts against it.

Smaller things:

• --skip-gomod builds a patch relative to the working directory. The pathspecs at :222-223 use . without :(top), so running from a subdirectory silently truncates the patch to that subtree — and the result clears every guard and writes the trailer.
• The trailer match is an unanchored substring. :123-125 greps the whole message, and squash merges carry contributor commit bodies onto v1 verbatim, so a merged v1 PR whose body contains that line suppresses an arbitrary backport. grep -Fxq on the exact line is tighter.
• The workflow input check is line-oriented, so a newline gets past it. grep -qE at backport.yml:96 matches any line, so prs set to $'1301\n*' passes and args=(${PRS}) then globs against the repo root into 28 paths. The script's own option loop rejects them, so nothing executes, but the containment is in a different file from the check. A bash [[ =~ ]] anchors to the whole string.
• The sed at :232 rewrites module paths without their versions, turning google.golang.org/adk/v2 v2.0.0 in plugin/agentanalytics/go.mod into something the go tool rejects with should be v0 or v1, not v2. It surfaces as a red build on the backport PR rather than silently.
• detect_remote matches the repository by substring, so on a fork :106 finds nothing and the job is red on every push to a fork's main once Actions are enabled there. An if: github.repository == 'google/adk-go' on the job avoids that. The same match would also accept a remote pointing at google/adk-go-experimental.
• The script exits immediately on stock macOS bash. shopt -s inherit_errexit at :36 needs 4.4 and macOS ships 3.2, so a contributor following the --list line in CONTRIBUTING gets one line of shopt: noise and exit 1. A BASH_VERSINFO check that names the requirement would turn that into an actionable message.
• Untested, and worth someone checking on a Mac: bare mktemp at :226 and :310 is a GNU extension and BSD mktemp wants a template. I had no macOS to run it on. If it does fail there, patch is empty, the guard above treats it as "empty patch; nothing to backport", and every PR is skipped on a green exit 0.
• Nothing in CI checks either new file. There is no shellcheck or actionlint step in .github/workflows/, and the # shellcheck disable=SC2206 is inert without one. shellcheck -o all is what surfaces the set -e suppression behind three of the findings above.
• CONTRIBUTING.md:79-80 says your current checkout is never touched. It is: the script writes and force-deletes refs/heads/backport/v1/pr-<n> and prunes worktrees in the invoking clone.
• CONTRIBUTING.md:105-108 says authorship works out the same way it does today. The mechanism described is right — I checked #1310, one branch commit by ktsoator, landed as 95a40be authored by wolo with a Co-authored-by. What changes is that the PR owner is now github-actions[bot], so git blame on v1 points at the bot from here on.

[baptmont]

Thanks — the framing ("the script does the wrong thing and reports success") was the right way to group these, and it made them straightforward to work through. Pushed 86e1e2c. All five, plus the rebase-merge question and every smaller item.

The five

The replay is no longer tied to a named local branch. You were right that checkout -B was the only thing binding the name to the commit and that nothing checked it. The worktree now stays detached end to end: no local ref is written, the push sends HEAD:refs/heads/${branch}, and git branch -D is gone from cleanup_worktree. Both guards before the push now check HEAD, which is the thing being pushed.

Reproduced your scenario before and after — a backport/v1/pr-1301 carrying an unrelated commit and checked out in another worktree. After: the replay is byte-identical to 95a40be, and the pre-existing branch is still at its own tip, untouched. The quieter variant is gone too, since nothing resets or deletes local refs any more.

git worktree add and git commit are checked. Your diagnosis of why they weren't is the part I'd missed: || status=$? at the call site makes errexit inert for the entire function body, not just the call. Both now return 3, so an infrastructure failure can no longer fall through to git apply and get posted to a contributor as a conflict in their patch.

die is gone from the per-PR path. exit 1 isn't caught by || status=$? — that suppresses errexit, not exit. The three sites (cat-file, and the two guards before the push) return 3, which main already collects and turns into a red run after draining the rest. Remaining die calls are all in startup or main, where aborting is correct.

A failed gh pr create takes the branch back down. This was the one I'd have been most annoyed to discover in production, given the likeliest trigger is exactly the "Actions cannot create pull requests" case the script already names. It now deletes the remote branch on that path so the PR stays queued, and if the delete itself fails it prints the exact command, because that's the state where silence costs the most.

The posted comment no longer describes a procedure that destroys the work. You're right that the terminal text was already correct and the comment wasn't. The comment now carries the same steps — finish in the worktree, push HEAD — with an explicit "do not re-run the script".

The rebase-merge question

Worth settling rather than leaving, so it's asserted now. Before replaying, the PR's file set (gh pr view --json files) is compared against the merge commit's, and a merge commit that doesn't carry the whole PR is refused with return 3 naming rebase-merge as the likely cause. Checked both directions: a squash merge like #1301 passes cleanly, and feeding a commit that carries only part of a PR is refused rather than half-backported. If it ever fires it's a red run and a human decides, which seems right for something that would otherwise mark a PR permanently done.

Smaller items

All of them:

• :(top) on the --skip-gomod pathspecs. Verified the same 5 files come out from the repository root and from tool/.
• Exact-line trailer match. The --grep still narrows, then each candidate is confirmed with grep -Fxq on the exact line. A commit mentioning the phrase mid-sentence no longer matches; a real trailer still does.
• The module rewrite now puts back <path> v2.x in go.mod require lines, so it produces a hunk that fails to apply rather than a go.mod the go tool rejects. Failing to apply is the honest outcome there anyway.
• detect_remote is anchored to the end of the URL, so google/adk-go-experimental is no longer accepted, and the job carries if: github.repository == 'google/adk-go' so a fork isn't red on every push.
• Workflow input matched with [[ =~ ]]. Confirmed $'1301\n*' is now rejected where the grep -qE accepted it.
• BASH_VERSINFO check naming bash 4.4 and suggesting brew install bash, before the shopt that needs it.
• mktemp given a template at both sites. I still have no macOS to test on, so that one is reasoning rather than evidence — if you or anyone else has a Mac handy, scripts/backport.sh --list is enough to shake out both this and the bash gate.
• Both CONTRIBUTING corrections. It now says the worktree is detached and that the script does add, remove and prune worktrees in the invoking clone; and the authorship paragraph says the PR owner is github-actions[bot], so git blame on v1 points there with the human author in the Co-authored-by: trailer. Thanks for actually checking fix(skilltoolset): accept scalar allowed-tools frontmatter (#1301) #1310 against 95a40be — that mechanism is easy to assert and annoying to verify.

Not done

No shellcheck or actionlint step in CI. You're right that the # shellcheck disable=SC2206 is inert and that shellcheck -o all is what surfaces the set -e suppression behind three of these. I couldn't run it — no shellcheck on this machine and the install is blocked here — so this round is bash -n plus the behavioural tests above, which is weaker than it should be for exactly the class of bug you found. Adding a lint job feels like it belongs in its own PR rather than growing this one further.

[karolpiotrowicz]

Dropping the PAT is the right call and the reasoning behind it checks out — I read the June 2026 changelog and it says what the header says it does, so the backport PRs will get CI on an approval click rather than needing a long-lived credential. I also tried to steer the push target and could not: branch is built from a readonly prefix and an integer that only ever arrives as the API's .number, so there is no input that reaches git push with another ref. The queue no longer parses prose, one conflict costs one backport, and the detached-worktree rewrite closed the whole class from last round. Three things left that I think need fixing, then a handful of small ones.

Line references are pinned to f0aafae.

The rebase-merge guard refuses ordinary squash merges when run locally. :260-261 hands comm -23 a list that jq sorted by codepoint and a list that sort ordered by locale. Those agree under C.UTF-8, which is what the runner uses, so CI is fine — but en_US.UTF-8 is the normal workstation setting and there they disagree. comm then prints comm: file 1 is not in sorted order and reports present files as absent. On this pull request's own three files it reports CONTRIBUTING.md missing, which takes the return 3 path and tells the operator "This is what a rebase merge looks like … Cherry-pick the range by hand instead" about a perfectly good squash merge. Six of the last sixty commits on main trigger it. That matters because :211 and CONTRIBUTING both send people down exactly this path. LC_ALL=C on both sides fixes it. Worth adding git -c core.quotePath=false in the same edit: git show --name-only C-escapes a non-ASCII path where the API returns it raw, which breaks the same comparison on the runner too. No such path exists in the tree today.

The gh pr create error is never captured, so the one diagnosis the workflow promises is dead code. The redirect at :423 sits on the assignment rather than inside the substitution, and bash expands the substitution before applying it, so ${err} is always empty. The grep at :426 can never match and the guidance at :427-429 never prints. What the operator actually gets is warning: could not open the PR for #N: with nothing after the colon. This is the failure you singled out as the likeliest on the first real run, and it is what backport.yml:33-34 promises the script handles. Moving the redirect inside — url="$(gh pr create … 2>"${err}")" — makes it fire, and it closes a second case at the same time: if the mktemp at :409 ever fails, 2>"" is an ambiguous redirect that fails after the substitution has run, so the PR is created and then the failure branch deletes its branch.

Both recovery procedures tell the person to commit the .rej files. :221 in the posted comment and :342 in the terminal text both say git add -A && git commit, and git apply --reject has just written *.rej next to the files it patched. git add -A stages them, so following the instructions verbatim puts reject files on the backport branch and, once it merges, on v1. The review requirement on v1 should catch it, but the instruction should not be creating the opportunity. git add -u avoids it. Both procedures also drop the (cherry picked from commit <sha>) trailer, which is now the exclusion key — that is survivable today because the pushed branch keeps the PR out of the queue via in_flight, but CONTRIBUTING documents deleting that branch as the way to re-queue, so the two interact.

Smaller things:

• :164 — git ls-remote --heads <remote> backport/v1/pr-123 matches on the tail of a ref, so a branch named <anything>/backport/v1/pr-123 makes in_flight a false positive and drops that PR from the queue permanently and silently. The repo already carries <user>/… prefixed branches, so the shape is one habit away. Fully qualifying it as refs/heads/… fixes it and still matches the real branch.
• :194 — the marker is matched with a bare contains over every comment body with no author filter, so anyone can post <!-- adk-backport-conflict --> on a pull request before it ever conflicts and permanently suppress the only signal a human gets.
• :308-321 — the patch pipeline's status is unchecked, and because backport_one is called as || status=$? there is no errexit to catch it. A failed git show or mktemp leaves an empty file, which becomes return 2, which is tallied as "empty patch" and exits green. Of the four return codes, 2 is the one that absorbs tooling failure.
• :501-502 — when explicit numbers are given and the filter matches none of them, the script prints "nothing pending: no merged 'main' PR labelled 'v1-needed' is missing from 'v1'", which is a claim about the whole queue rather than about the numbers asked for. That is the command :211 tells a contributor to run, so it is worth naming which number was unrecognised and why.
• :233-236 — the contract says 3 means the push or the PR call failed, but 3 is also returned from :245, :268, :291, :370, :393 and :398. The behaviour is right in all six, the docstring is not.
• :258-259 — gh pr view --json files caps at 100 and this call does not page. On V2 release #1109 it returns 100 of 547. It does not break the squash case, where a subset still yields an empty comm, and a rebase merge would still be caught because the merge commit's file set is small — but the comparison covers less than it reads as covering.
• :390 — this guard compares branch against the prefix it was built from four lines of context earlier, so it cannot fail. Keeping it as an assertion is reasonable, but the invariant is actually held by the construction, and the comment reads as though the check is what holds it.
• backport.yml:59-61 — no timeout-minutes on a job that holds a serialized concurrency group, so a wedged run blocks every later backport for the six-hour default.

On the guard in the first item: I would fix it rather than drop it. A silent half-backport that then clears itself from the queue on its own trailer is the worst outcome this design has, rebase merges are enabled on the repository even though nothing has used one, and refusing rather than warning is the right way round for that. All three of its problems are one-line changes.

[baptmont]

All eleven fixed, in 2fbb51e, plus two more in 47d8ddd that I found reviewing my own work before asking you to look again.

Thanks in particular for the locale one. I had the bug in front of me and picked the input that hides it: my machine is en_US.UTF-8, but I tested the guard against #1301, whose files are all lowercase under one directory, so codepoint and locale order happened to agree. Reproduced it on this pull request's own three files, confirmed CONTRIBUTING.md reported missing, and confirmed it gone after.

The three

The locale-dependent guard. Both sides sorted and compared under LC_ALL=C, with core.quotePath=false so a non-ASCII path cannot break the same comparison on the runner. Since I was in there, the file list is paginated via gh api --paginate rather than gh pr view --json files, which closes the 100-cap item at the same time — #1109 now returns all 547.

The gh pr create redirect. Moved inside the substitution. Verified the "Allow GitHub Actions to create and approve pull requests" guidance actually fires now, on a simulated failure carrying that exact message. You were right that this was the worst one to have dead, given it is the failure I called most likely on the first real run and the workflow header advertises it. The mktemp-failure ordering hazard goes with it.

Both recovery procedures. They now delete the rejects, stage with git add -u, and carry the (cherry picked from commit <sha>) trailer. The trailer point was the one I would have missed: without it a hand-finished backport is invisible to the exclusion check the moment its branch is deleted, and CONTRIBUTING documents deleting that branch as the way to re-queue.

The four filed as smaller

I moved these up rather than treating them as smaller, since by your own framing all four fail silently and permanently:

• in_flight is fully qualified as refs/heads/…. Confirmed the bare pattern really does match on the tail — ls-remote --heads origin 'backport-tooling' matches refs/heads/baptmont/backport-tooling — and that the qualified form still matches the real branch and no longer matches a prefixed one.
• The patch pipeline's status is checked via PIPESTATUS before the emptiness test, so a failed git show returns 3 instead of being tallied as a benign skip. I verified local -a ps=("${PIPESTATUS[@]}") really does capture through an intervening comment block rather than assuming it.
• The conflict marker is matched only on comments authored by the bot or by whoever is authenticated, so it cannot be pre-posted by a third party to suppress the signal. First attempt at this used gh pr view --jq --arg, which gh does not support — it only surfaced because I ran it against real comments instead of reading it, which is becoming the theme.
• timeout-minutes on the job.

The four cosmetic

Explicit numbers that match nothing now say which and why instead of claiming the whole queue is empty; the return-code docstring lists all six paths that return 3; the push guard's comment says it is an assertion rather than the thing holding the invariant.

Two I found reviewing my own fixes

Worth calling out separately because both are mine, from the commit that fixed your findings:

The merge-shape guard failed open. I wrote if pr_files="$(gh api … | sort -u)" && [[ -n "${pr_files}" ]], so an unreachable API produced an empty list, which read as "nothing to compare" and skipped the check entirely. A guard against a silent half-backport, disabling itself silently. It now refuses: if the file list cannot be fetched, the merge shape is unknown, and unknown is not fine.

The timeout was tuned the wrong way. I picked ten minutes to unwedge the serialized group quickly and did not think about what a kill costs. The job dies mid-drain, and a kill between the push and gh pr create leaves a branch with no pull request behind it — which in_flight then reads as a backport already under way. That is the failure mode two of your findings were about, recreated by my own mitigation. Thirty now, with the reasoning in the comment.

A design question I would rather put to you than decide

Should in_flight check for an open pull request rather than only for the branch? A branch with no PR behind it — from a killed run, or from the gh pr create failure path even after it deletes the branch on the way out — suppresses that backport until someone removes the branch by hand. Checking for an open PR would let orphans re-queue themselves, at the cost of one API call per queue entry. You framed in_flight as branch-existence deliberately, and CONTRIBUTING documents branch deletion as the escape hatch, so I have left it alone rather than quietly changing the contract.

Still not evidence

mktemp and the bash 4.4 gate on macOS. You flagged the mktemp one as worth someone checking and it still is; I have no Mac, so both remain reasoning. scripts/backport.sh --list exercises both in one go if anyone has one to hand.

The end-to-end path has never run. Same as before: it needs a merged v1-needed PR, and whether "Allow GitHub Actions to create and approve pull requests" is on here is not readable from a workflow token. If it is off, the first run pushes a branch, fails to open the PR, deletes the branch again and goes red with the named error — which is at least the shape we want.

Correcting something I said last round

I wrote that I would open a separate PR adding a shell lint job and suggested it land before this one. That is not what happened and it is not the plan. I ran shellcheck --enable=all once as a temporary CI job to answer the point, fixed the two real findings — a git log piped into a loop through a process substitution, where a failure read as "not yet backported" and would open a duplicate, and an unquoted expansion in the guard's message — and then removed the job. A permanent shell lint across the repository turns out to be a much larger decision than it looked: --enable=all produces 36 findings in apidiff.sh alone, almost all brace-and-quote style, and picking a narrower profile is a repo-wide call that should not ride in on this pull request. So the checking happened; the job did not stay. Happy to be overruled if you would rather see it as a permanent job, but I did not want to leave a commitment standing that I had quietly dropped.

[karolpiotrowicz]

All three round-3 fixes hold, and I checked them by measurement rather than by reading: LC_ALL=C on both sides with core.quotePath=false removes the misfire (C.UTF-8 and C match jq's codepoint order, en_US.UTF-8 does not), gh api --paginate returns all 547 files for #1109 where gh pr view --json files returns 100, and the stderr redirect inside the substitution makes the "Allow GitHub Actions to create and approve pull requests" guidance actually fire. The PIPESTATUS capture survives the intervening comment block and a failing git show now returns 3, and the fully-qualified refs/heads/ pattern no longer matches a prefixed branch while still matching the real one. One thing needs fixing before this merges, and it came in with the fix it belongs to.

Line references are pinned to 47d8ddd, the current head.

git add -u drops every file the patch adds. :396 runs git apply --reject without --index, so a file the patch adds lands in the worktree untracked. Both recovery procedures — :405-407 in the terminal text and :235-237 in the posted comment — then say find . -name '*.rej' -delete followed by git add -u, which stages tracked paths only. I ran the printed steps verbatim against a patch that modifies one tracked file and adds another:

apply --reject  ->  added.txt applied cleanly (UNTRACKED), existing.txt rejected
human resolves existing.txt, then runs the printed steps exactly
commit SUCCEEDED
  files in the commit: [existing.txt]
  added.txt present in tree? NO — silently dropped
  trailer present? 1

The commit still carries the cherry-pick trailer, so already_backported matches it and the pull request leaves the queue permanently. A partial change on v1, reached by following the tool's own instructions, and it is the previous git add -A that did this correctly — that form staged the added file, it merely also staged the .rej. Since the find … -delete now runs first, git add -A after it is both safe and complete, which I confirmed on the same fixture. It is one character in two strings.

In the same block: the posted comment's trailer is a placeholder, and the terminal one is not. :240 is literally (cherry picked from commit <sha of the main commit>) where :410 interpolates the real ${sha} — comment_conflict never receives it, its signature is local pr="$1". A reader who follows the bot's comment rather than re-running the script produces a trailer the exclusion check will never match. The failure direction is safe, since a bogus trailer re-queues rather than silently dropping, but it sits inside the one string the whole predicate keys on.

A successful backport can be recorded as a conflict. :525 is the last statement of open_backport_pr, so cleanup_worktree's status becomes the return value. If both git worktree remove and the rm -rf fallback fail, a landed backport returns 1 and is tallied as conflicted at :602, warning "needs a manual backport" for a pull request that is already open. Nothing is lost and the run stays green, so it is one wrong log line, but cleanup_worktree "${worktree}"; return 0 closes it.

Something I could not settle, and you can in one run. :204 is me="$(gh api user --jq '.login' 2>/dev/null || echo '')", interpolated into a jq program at :207. What I measured: on an API error gh api … --jq writes the raw JSON body to stdout, so 2>/dev/null does not suppress it and || echo '' can only append. Feeding that value into the real filter gives failed to parse jq expression (line 1, column 85) … unexpected token "message", and gh pr view exits non-zero into the handler at :209-212, which warns and returns 0 — so the conflict comment would never post. What I could not test is the first link: whether GET /user fails at all under an Actions installation token, since I have no such token. Reviewers reading the same lines split on it, so it is worth settling rather than taking my word: one run with echo "[$(gh api user --jq .login 2>/dev/null || echo '')]" says which it is. Building the filter with a jq variable instead of string interpolation removes the class either way. Note the comment at :202-203 — "an empty login simply matches nothing" — is not guaranteed by the line above it.

Two more, both real and neither urgent. A run killed between the push at :472 and gh pr create at :480 leaves a branch with no pull request, and in_flight then excludes that PR on every later run — the killed run itself is red, so this is quiet rather than invisible, and the documented branch deletion recovers it. And the trailer match at :152 is a whole-line match anywhere in a v1 body, so a commit carrying another PR's sha on its own line would dequeue it — pre-existing, and I would not rework the core exclusion predicate on a fourth revision just for that.

On your in_flight question: I would change it, and I think the contract it documents is the weaker one. Branch-existence conflates "under way" with "attempted and died", and only the second is silent. Checking for an open pull request makes the orphan self-healing, for one API call per queue entry against a queue that is currently empty. Worth costing at the same time: --paginate at :294 turns one request into roughly nineteen for a 547-file pull request, and each can now fail into the fail-closed return 3 — two individually-correct fixes that compose into a new way for a run to go red.

[baptmont]

All fixed in fce6485, and the one you left open is settled by measurement rather than by argument.

git add -u

Reproduced your fixture before touching anything, because I wanted to see it drop the file rather than take it on trust:

apply --reject  ->  added.txt applied cleanly (UNTRACKED), existing.txt rejected
printed steps run verbatim
  files in commit: [existing.txt]
  added.txt in tree? NO
  trailer present? 1

Back to git add -A in both procedures, which is complete now and safe only because the find … -delete I added last round runs first. Same fixture after:

  files in commit: [added.txt existing.txt]
  added.txt in tree? YES
  .rej committed: 0

This one is worth naming for what it was: -u came out of your round-3 suggestion, I took it without testing what apply --reject leaves behind, and the result was a partial change on v1 reached by following the tool's own instructions. The fixture is exactly why "it is one character in two strings" was the wrong way to think about it — the character was easy, knowing which one needed the fixture.

The placeholder trailer

comment_conflict takes the sha now and interpolates it, so the posted comment and the terminal text produce the same trailer. You were right that the failure direction was safe; it was still the one string the whole predicate keys on, written wrong in the copy most people will read.

cleanup_worktree as the return value

return 0 after it. A worktree that refused to go away no longer reports a landed backport as a conflict.

gh api user

Settled, and it is the bad branch. On any API error gh api --jq prints the response body to stdout, so 2>/dev/null does not suppress it and || echo '' can only append:

$ GH_TOKEN=<invalid> gh api user --jq .login 2>/dev/null
{
  "message": "Bad credentials",
  "documentation_url": "https://docs.github.com/rest",
  "status": "401"
}
exit=1

That is the whole first link. The second one does not need a token to answer: GITHUB_TOKEN is an installation access token and GET /user is user-to-server, so it fails in CI — which means the conflict comment would never have posted on the primary path, silently, exactly as you traced it. The login is validated against ^[A-Za-z0-9-]{1,39}$ now and anything else becomes empty, so the class is gone regardless of what any future endpoint does. Your note that the comment above the line was not guaranteed by the line below it was the tell; I had written the reassurance and not the check.

in_flight

Changed, as you suggested. It keys on an open pull request with that head rather than on the branch, so a run killed between the push and gh pr create no longer excludes its PR from every later run. Verified both directions against live data — an open PR on a head reads as in flight, an absent one does not — and CONTRIBUTING now documents the new contract, including that a branch left behind by a dead run suppresses nothing.

The gh pr create failure path still deletes its branch, but for a different reason now: not to avoid exclusion, just to keep the next attempt's push from colliding.

The cost, since you asked for it to be counted

You were right that fail-closed and --paginate compose badly. The guard compares counts first, which is one request, and fetches the full list only when they differ, to name what is missing. A squash carries the whole pull request so the counts match; a rebase merge records one commit so they do not. So the ~19-requests-each-able-to-refuse path only opens when something is already wrong, and the common case went from up to nineteen requests to one.

Residual is in the comment: equal counts over different sets would pass. That needs the last commit of a rebase to touch exactly as many files as the whole branch, and such a patch would almost certainly fail to apply anyway.

Not done, on your steer

The whole-line trailer match at :152 is unchanged. You said you would not rework the core exclusion predicate on a fourth revision for it and I agree — it is pre-existing, the failure needs a v1 commit body carrying another pull request's sha alone on a line, and changing the predicate is the kind of edit that wants its own change and its own fixture.

One thing that changed underneath us

The queue is no longer empty: #1328 merged this morning carrying v1-needed, so --list returns a real entry. That makes in_flight load-bearing rather than theoretical, which is part of why I verified it against live data instead of reasoning about it, and it means the end-to-end run you and I have both been calling untested is now actually available.

Still not evidence, unchanged: mktemp and the bash 4.4 gate on macOS. I have no Mac and both remain reasoning; scripts/backport.sh --list exercises them together if anyone does.

[karolpiotrowicz]

Verified rather than read, and all six hold.

The git add -A change is right, and I checked the thing that makes it right rather than the change itself: find … -delete runs before git add -A in both procedures, at :249/:252 and :441/:444, with no git add -u left anywhere. Running the printed steps verbatim on the same fixture now commits [added.txt existing.txt], no .rej, trailer present, nothing left untracked. Swapping the two lines puts the .rej back in the commit, which is the useful way to see that the ordering is load-bearing and not incidental.

The rest: comment_conflict takes the sha at :203 and the call site passes it, so both procedures now print the same trailer. return 0 after cleanup_worktree at :566. The login pattern drops every error body I threw at it — 401, 403, empty, over-length — to empty, which matches no author, so the filter stays parseable whatever a future endpoint returns. And in_flight behaves in both directions against live data: a head with an open pull request reads as in flight, an absent one does not.

Two things I checked because they were new rather than because you claimed them. in_flight now fails via die inside an && condition, which is the shape that produced the round-three defect, so I tested it in isolation instead of reasoning: a failing gh pr list stops the run with no queue emitted, and the healthy path still filters correctly. And the count comparison holds on real data — changedFiles equals the squash commit's file count on seven of seven merged pull requests I tried, including #1328, so the one-request common case is real rather than hoped for.

On the residual you asked me to weigh: acceptable, no follow-up. One correction to how it is recorded, though. You describe it as equal counts over different sets, but the comparison is over file names, so equal sets with different content pass too — a rebase whose last commit touches the same files as the whole branch clears both the count check and the comm. That is not something this change introduced. It is inherent to comparing file lists rather than content, and it predates the optimisation. Your mitigating argument covers it unchanged: the last commit's patch is written against a parent that is not on v1, so it would almost certainly fail to apply. Worth widening the comment so the recorded residual is the real one.

Two notes, neither blocking. The exclusion moved from a git operation to an API call, so gh pr list --head now costs one request per pending pull request where ls-remote cost none — immaterial at a queue of three, worth knowing before a bulk drain. And the queue is not one entry: #1328, #1394 and #1396 all carry v1-needed. #1396 is chore(deps): bump the go-minor-patch group with 4 updates, which is the case CONTRIBUTING and the conflict text both single out as needing judgement because the 1.x dependency set differs — so the first unattended run will exercise the conflict path rather than the happy one.

One thing worth saying plainly, since you named it yourself: -u came from my round-three suggestion and I gave it to you as "one character in two strings". You were right that the character was the easy part. The fixture is what told either of us which one, and I should have written it before recommending the change rather than after it broke.

The git add -u defect this blocked on is fixed at fce6485, along with everything else raised here. Re-reviewed at that head: #1327 (review)

[baptmont]

Residual comment widened in cc62798 — it now records that the comparison is over file names, so equal sets with different content pass too, and that the count step neither introduced nor widened it. Your framing, since it was more accurate than mine.

Nothing else changed. main is merged in, checks are green on 05aa688.

Both notes taken. The API call per pending PR is worth watching if the queue ever gets long; at three it is noise. And thanks for spotting the queue composition — #1396 being a deps bump means the first unattended run exercises the conflict path, which is the better rehearsal of the two, so I would rather let it than relabel to get a clean first run.

[karolpiotrowicz]

Carried forward to 05aa688, the current head, and I checked the carry rather than assuming it.

The only change to reviewed code since the round-five pass is cc62798, which is seven added and three removed lines entirely inside a comment block. The main merge that followed it leaves all three files the pull request owns byte-identical, so every finding verified at fce6485 holds unchanged on the executable content.

The widened residual reads correctly now. It records that the comparison is over file names, so equal sets with differing content clear both the count check and the comm, and that the count step neither introduced that nor widened it.

Approving. The two notes from the last round stay notes rather than conditions. gh pr list --head costs one request per pending pull request where ls-remote cost none, immaterial at a queue of three and worth knowing before a bulk drain. And letting #1396 be the first unattended run is the right call for the reason you gave, since a dependency bump exercises the conflict path rather than the happy one.