Skip to content

fix(skilltoolset): accept scalar allowed-tools frontmatter - #1301

Merged
kdroste-google merged 4 commits into
google:mainfrom
ktsoator:fix/skill-allowed-tools
Aug 11, 2026
Merged

kdroste-google merged 4 commits into
google:mainfrom
ktsoator:fix/skill-allowed-tools

Conversation

@ktsoator

Copy link
Copy Markdown
Contributor

Link to Issue or Description of Change

Closes: #1296

skill.Frontmatter.AllowedTools previously accepted only YAML sequences, while the Agent Skills specification defines allowed-tools as a scalar string.

This change:

  • accepts the specification scalar form;
  • preserves compatibility with YAML sequences;
  • keeps the public AllowedTools []string API unchanged;
  • avoids splitting whitespace and commas inside tool parentheses.

Testing Plan

  • go build -mod=readonly work
  • go test -race -mod=readonly -count=1 -shuffle=on work
  • golangci-lint run in the root and plugin/agentanalytics modules
  • go mod tidy -diff in both modules

@google-cla

google-cla Bot commented Aug 11, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@ktsoator

Copy link
Copy Markdown
Contributor Author

CLA details have been updated. @googlebot rescan

@kdroste-google kdroste-google left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @ktsoator, thank you for this contribution!
It all makes perfect sense, just a few remarks from my side.
At the moment I don't see a need for allowing unbalanced / weird ordered '(', ')' in patterns. WDYT?

Comment thread tool/skilltoolset/skill/frontmatter.go Outdated
Comment thread tool/skilltoolset/skill/frontmatter.go
Comment thread tool/skilltoolset/skill/frontmatter.go
Comment thread tool/skilltoolset/skill/frontmatter_test.go Outdated
@ktsoator

Copy link
Copy Markdown
Contributor Author

Hi @kdroste-google,

Thank you for taking the time to review this PR and for the thoughtful feedback.

I have been using ADK Go for a while, and my team has also used it to deliver a real project at my company. The framework has been genuinely useful to us, so I wanted to contribute some fixes and improvements back to the repository where I can.

Your point about unbalanced parentheses makes sense. Since the parser already treats parentheses structurally when preserving spaces and commas inside tool expressions, silently accepting malformed expressions would be surprising.

I have updated the implementation to:

  • return an error for an unexpected );
  • return an error when an opening ( is left unclosed;
  • document the accepted whitespace/comma-separated Tool and Tool(...) formats in the code and error message;
  • add tests for unbalanced and incorrectly ordered parentheses;
  • rename the existing comma-separated test to make that coverage explicit.

Separator-only input such as ",,,, , ," remains accepted as an empty list, as discussed.

The build, race tests, lint, and module tidy checks all pass locally after the changes. Thanks again for helping improve the implementation.

@kdroste-google kdroste-google left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ktsoator, thank you for those fixes!
I'm glad to hear that you find ADK-GO useful - waiting for your next contributions!

@kdroste-google
kdroste-google merged commit 9584b47 into google:main Aug 11, 2026
13 checks passed
@VDVsx

VDVsx commented Aug 11, 2026

Copy link
Copy Markdown

Thanks both for the fast turnaround on this one!

wolo-lab added a commit that referenced this pull request Aug 11, 2026
…1310)

* fix(skilltoolset): accept scalar allowed-tools frontmatter

* test(skilltoolset): cover allowed-tools source listing

* fix(skilltoolset): reject unbalanced allowed-tools patterns

(cherry picked from commit 9584b47)

Co-authored-by: ktsoator <ktsoator@gmail.com>
baptmont added a commit that referenced this pull request Aug 19, 2026
The review found three blockers and three near-blockers, and five of the
six came from two design choices rather than from the code being wrong.

Batching every pending backport onto one branch made a single conflict
fatal to the whole run: apply_pr exited from inside the loop, so the PRs
behind the conflicting one were never attempted and the ones that had
already applied went away with the runner. It also made the batch title
enumerate PRs that might have contributed no commit, and made the branch
name a date that collides on the second batch of a day.

Working out what was already backported by reading PR numbers out of
commit subjects and PR titles meant parsing prose. An open v1 PR titled
"address review feedback from #1301" silently dropped #1301 from the
queue forever, and the die() protecting that lookup sat behind a pipeline
in a command substitution, where errexit does not reach it.

So: one branch and one pull request per original PR, replayed in its own
worktree. A conflict now costs one backport. The branch name is a
function of the PR number, so it cannot collide and it doubles as the
in-flight check. And "is this already on v1" is answered by searching v1
for the "(cherry picked from commit <sha>)" trailer the script itself
writes -- an exact match on something the automation owns, with no prose
in the loop.

What is left of the review is small and is fixed here: the push refuses
any branch outside backport/v1/, a branch with no commits is never
pushed, inherit_errexit is on, and an empty patch no longer counts as a
backport. Failure modes are now distinguished -- a conflict is a normal
outcome that comments once and keeps the PR queued, while a failed push
or PR call fails the run, because nobody has been told.

Dropped with the batching: --branch, --worktree, --force, --watch and the
two-step resume flow, which existed to make a failed batch recoverable.
--list, --pr and --skip-gomod remain.

Verified against real history: replaying #1301 onto the tree before its
hand-made backport reproduces 95a40be byte for byte, #1252 rejects
exactly the IsolationScope hunks and leaves .rej files, and #1195 needs
--skip-gomod. Note that #1195 does not apply cleanly with or without this
change -- the claim in the original description was wrong, and is
corrected there.
baptmont added a commit that referenced this pull request Aug 20, 2026
Every one of these is the same shape: the script did the wrong thing
and reported success.

The replay is no longer tied to a named local branch. `checkout -B` was
the only thing binding backport/v1/pr-<n> to the replayed commit, its
exit status was unchecked, and the push resolved the name rather than
the commit -- so with that branch already checked out in the clone, the
commit landed on a detached HEAD and the push sent the pre-existing
branch instead, inside a run reporting "backported 1 ... failed 0". The
worktree now stays detached from start to finish, the push sends
HEAD:refs/heads/<branch>, and nothing writes or deletes a local ref. The
guards before the push check HEAD, which is now the thing being pushed.

`git worktree add` and `git commit` are checked. `|| status=$?` at the
call site makes errexit inert for the whole function, so a failing
worktree add fell through to `git apply`, failed because the directory
was not there, and posted "this change does not replay cleanly" on a
merged pull request whose patch was fine.

`die` no longer appears in the per-PR path. It is `exit 1`, which
`|| status=$?` does not catch, so it aborted the drain mid-queue: every
later PR skipped, no summary line. Those paths return 3, which main
already collects into a red run after draining the rest.

A failed `gh pr create` now takes the pushed branch back down. in_flight
treats a pushed branch as a backport already under way, so leaving it
behind removed that PR from the queue permanently -- one red run, then
silence. The likeliest trigger is the "Actions cannot create pull
requests" case the script already handles by name.

The comment the bot posts on a conflict described a recovery procedure
that destroys the work it asks for: "re-run with --pr" starts from a
clean replay and rm -rf's the worktree first. It now says what the
terminal already said -- finish it in the worktree, push HEAD.

Rebase merges are asserted against rather than assumed away. The
repository allows them, and for a rebase merge mergeCommit.oid is only
the last commit of the branch, so a multi-commit PR would be backported
in part, apply cleanly, and clear itself from the queue on the trailer.
The PR's file set is compared against the merge commit's, and a merge
commit that does not carry the whole PR is refused.

Also: the trailer test is an exact line match, so a v1 commit quoting
the phrase in prose no longer suppresses an unrelated backport; the
--skip-gomod pathspec is anchored with :(top), so a run from a
subdirectory no longer silently truncates the patch; the module rewrite
leaves `<path> v2.x` in go.mod require lines alone rather than producing
a go.mod the go tool rejects; the remote match is anchored, so it no
longer accepts google/adk-go-experimental; the workflow input is matched
with [[ =~ ]] rather than a line-oriented grep a newline gets past; the
job is skipped outside google/adk-go; bash >= 4.4 is checked by name
rather than failing on `shopt: invalid option`; and mktemp is given a
template so it works on BSD.

CONTRIBUTING no longer claims the current checkout is untouched, and
notes that the backport PR owner is now the bot, so git blame on v1
points there with the human author in the trailer.

Verified: #1301 still replays byte-identically onto the tree before
95a40be, with no local ref written and the worktree detached; the same
replay is unaffected by a pre-existing backport/v1/pr-1301 checked out
elsewhere, which is left untouched; a merge commit missing the PR's
files is refused; a prose mention of the trailer no longer matches while
a real trailer still does; --skip-gomod gives the same 5 files from the
repository root and from tool/; #1252 still leaves its .rej files.
baptmont added a commit that referenced this pull request Aug 25, 2026
Three blockers, four that were filed as smaller but fail silently and
permanently, and four cosmetic.

The rebase-merge guard refused ordinary squash merges anywhere but CI.
jq sorts by codepoint and sort(1) by locale, and under en_US.UTF-8 --
the normal workstation setting, and mine, which is why the guard's own
test passed -- comm reports present files as missing and the run tells
the operator to cherry-pick a healthy squash by hand. Both sides are now
sorted and compared under LC_ALL=C, with core.quotePath off so a
non-ASCII path cannot break the same comparison on the runner. The file
list is also paginated now; gh pr view --json files stops at 100.

The gh pr create redirect sat on the assignment rather than inside the
substitution, so ${err} was always empty, the grep could never match,
and the "Allow GitHub Actions to create and approve pull requests"
guidance -- the one diagnosis the workflow header promises, on the
failure most likely to happen first -- was dead code. The operator got a
warning ending in a colon. Moving it inside also removes an ordering
hazard: a failed mktemp made 2>"" an ambiguous redirect that fired after
the PR was created, and the failure branch then deleted its branch.

Both recovery procedures said git add -A immediately after git apply
--reject wrote .rej files next to the sources, so following them put
reject files on the backport branch and then on v1. They now delete the
rejects, stage with -u, and carry the (cherry picked from commit <sha>)
trailer, without which a hand-finished backport is invisible to the
exclusion check once its branch is deleted.

in_flight passed a bare name to ls-remote, which matches the tail of
every ref, so somebody's <user>/backport/v1/pr-<n> would read as this
backport already being under way and drop the PR from the queue for
good. Fully qualified as refs/heads/ now.

The patch pipeline's status was unchecked, and with errexit inert inside
this function a failed git show left an empty file that was read as
"nothing to backport" and tallied as a benign skip on a green run. Of
the four return codes 2 was the one absorbing tooling failure; it now
has to be earned.

The conflict marker is matched only on comments by the bot or the
authenticated user, so it can no longer be pre-posted by anyone to
suppress the only signal a human gets. The job takes timeout-minutes,
since it holds a serialized concurrency group and the default is six
hours.

Also: an explicit PR number that matches nothing now says so and why,
rather than claiming the whole queue is empty on the command the
conflict comment tells contributors to run; the return-code docstring
lists all six paths that return 3; and the push guard's comment says it
is an assertion rather than the thing holding the invariant.

Verified: the locale false positive reproduces on this PR's own three
files before the change and is gone after; the setting-specific
guidance fires now that stderr is captured; the qualified ref still
matches the real branch and no longer matches a prefixed one; a failed
patch pipeline returns 3; the recovery text prints commands that work;
and #1301 still replays byte-identically onto the tree before 95a40be.
baptmont added a commit that referenced this pull request Sep 3, 2026
* ci: automate v1 backports from the v2 label

Backporting a fix to the maintenance branch meant opening a second pull
request by hand and resolving the same conflicts every time. Six of the
fifteen commits on v1 are backports, several of them rolling up five main
PRs, so the work was already being batched to make the cost bearable.

The obstacle is not the cherry-pick, it is the module path. main is
google.golang.org/adk/v2 and v1 is google.golang.org/adk, so every Go
file's import block differs and any patch touching imports conflicts.
That difference is a pure string rewrite, so the patch is rewritten
before it is applied and the common case now lands untouched. Verified
against history rather than assumed: replaying #1195 fails as a raw
cherry-pick and applies cleanly rewritten, and replaying #1301 reproduces
the hand-made backport in 95a40be byte for byte.

Genuine drift is left alone. Replaying #1252 rejects the hunks touching
IsolationScope, a field v1 never had, which is the correct outcome; the
script applies what it can, leaves .rej files, and prints the commands to
finish. The workflow then comments on the original PR asking for a manual
backport rather than failing silently.

A clean apply is still not a correct backport. The hand-made #1195 also
added a helper that existed on main but never on v1, so a patch can apply
and not compile. The script says so and prints the verify commands.

Two details worth recording. The job needs a PAT or App token rather than
the built-in GITHUB_TOKEN, because pull requests opened with GITHUB_TOKEN
do not trigger workflow runs and the backport PR would arrive with no CI
and no way to merge it; the run refuses to start without one and confirms
afterwards that the checks actually registered. And it triggers on push
rather than pull_request_target, which would hand a token that can push
and open pull requests to a workflow running in a pull request's context;
the work is driven off the label queue, so it does not need the event
payload at all.

The queue clears itself: a PR drops off once its number is referenced by
a commit on v1 or by an open PR targeting v1.

* fix(backport): read only the places that record a backport

The queue skipped any PR whose number appeared anywhere in a v1 commit
message, subject and body alike. Bodies are prose, and prose is full of
numbers that mean something else. "Fixes #1152" names an
issue. One commit explains that it bumped dependencies "rather than a
cherry-pick of main's Dependabot commits (#1021, #1144, #1192, #1215,
#1219, #1242, #1275)", naming seven PRs precisely because they were not
backported. Fourteen numbers in the current history are references of
that kind, and each one is a fix that would never appear in the queue and
never be reported missing.

A silent omission is the one failure this tool must not have, since not
forgetting is the whole point of it.

Read the two places a backport is actually recorded instead: the commit
subject, where GitHub's squash puts "(#N)" and where a batched backport
lists every number it carries, and the "* subject (#N)" bullets a squash
leaves in the body for the commits it folded in. That second one matters
and is easy to miss -- #1156 and #1217 are recorded only as bullets, and
reading subjects alone would have re-queued both and opened duplicates.

Checked against the whole v1 history: the twelve prose references become
queueable again, and all forty-five real backport records are still
matched.

Open pull requests are matched on title alone. Every title this script
writes carries its numbers, in the trailing "(#N)" of a single backport
or the "(#a, #b, #c)" of a batch.

* fix(backport): stop a failed API call from looking like an empty queue

backported_prs ended in `| grep -oE '#[0-9]+' | tr -d '#' || true`, and
that `|| true` covered the whole group feeding the pipe, the `gh pr list`
call included. A network or auth blip while listing open v1 pull requests
would therefore not fail; it would return a short exclusion set, and a
short exclusion set means backporting something that is already in
flight. Nightly, unattended, with nobody reading the log.

The call is made up front now, and a failure stops the run. Refusing to
act on an incomplete picture is the right instinct here: the queue is
self-clearing, so a run skipped today is retried tomorrow at no cost,
while a duplicate pull request has to be noticed and closed by a person.

Also scopes the loop variable in the skip filter, which was assigning to
a global, and drops a stale claim in confirm_checks that this only ever
runs under a human's credentials. It runs in CI too now.

* fix(backport): do not lose the pull request to a missing label

The new pull request was labelled through `gh pr create --label`, which
fails outright when the repository has no such label rather than skipping
it. By that point the branch has been pushed, so a rename of `v1` would
leave the replayed commits on the remote with nothing pointing at them
and the run red for a reason that reads nothing like "the label moved".

Label as a separate step and warn if it does not take. A backport that
arrives unlabelled is a small annoyance; one that never arrives is the
failure this tool exists to prevent.

The label names also stop borrowing the branch constants, which they only
happened to match. V2_LABEL is what the queue filters merged main PRs on
and V1_LABEL is what goes on the PRs this opens; neither has any reason
to change when a branch name does.

* ci: say so when the backport token is not usable yet

The token check only asked whether the secret was non-empty. A
fine-grained PAT scoped to an organization is non-empty, and
authenticates, from the moment it is created -- but it cannot see the
repository until an org admin approves the request. The run would get
past the check and fail at actions/checkout instead, reporting that the
repository could not be read, which points at everything except the
pending approval that actually caused it.

Ask the API what the token can do, before the checkout. Unreachable names
approval as the likely cause and says where to look; readable but not
pushable names the two permissions to grant. Both beat inferring it from
a checkout failure.

Verified against four cases: empty secret, a working token, a malformed
token, and a valid token pointed at a repository it cannot see, which is
the pending-approval case.

* ci: drive the queue off v1-needed instead of v2

The labels changed: v2-only is gone, and v1-needed replaces it as the
signal that a change on main still owes a 1.x equivalent. v1 and v2 stay
on as information about which branch a pull request targets.

That inverts what the queue reads. v2 was an opt-in that also happened to
describe the branch, which made it do two jobs at once and left v2-only
carrying the "no" case as a second label to remember. One label with
behaviour attached and two that only describe things is easier to get
right at review time, and it is the label the queue now filters on.

The constant is BACKPORT_LABEL rather than V2_LABEL, since the name no
longer has anything to do with a branch. V1_LABEL is untouched: a
backport pull request targets v1, so the informational label still
applies to the ones this opens.

Verified against the live repository: the queue query returns #1328, the
one pull request currently carrying v1-needed, with the exclusion set and
output formatting intact around it.

* ci: run the backport on GITHUB_TOKEN instead of a PAT

The workflow refused to start without a BACKPORT_TOKEN secret on the
grounds that a pull request opened with the built-in 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 pull request opened by
github-actions[bot] now does trigger its pull_request workflows, in an
approval-required state: the runs are created and wait for someone with
write access to click "Approve workflows to run".

So the backport PR can get its CI without the repository holding a
personal access token that is bound to one person's account, outlives
the job, and needs an organization approval before it works at all. The
cost is one click on a pull request a human reviews and merges anyway.

What this needs instead is the "Allow GitHub Actions to create and
approve pull requests" repository setting. A workflow token cannot read
it, so it is not checked up front; gh pr create fails on it and the
failure handler names it rather than leaving a pushed branch and a
403 to interpret.

confirm_checks no longer fails the run when nothing has registered. The
branch is pushed and the PR is open by then, so exiting non-zero would
report a backport that succeeded as broken; held runs are now the
expected case, and it prints how to release them.

* ci: one backport PR per pull request, not one per run

The review found three blockers and three near-blockers, and five of the
six came from two design choices rather than from the code being wrong.

Batching every pending backport onto one branch made a single conflict
fatal to the whole run: apply_pr exited from inside the loop, so the PRs
behind the conflicting one were never attempted and the ones that had
already applied went away with the runner. It also made the batch title
enumerate PRs that might have contributed no commit, and made the branch
name a date that collides on the second batch of a day.

Working out what was already backported by reading PR numbers out of
commit subjects and PR titles meant parsing prose. An open v1 PR titled
"address review feedback from #1301" silently dropped #1301 from the
queue forever, and the die() protecting that lookup sat behind a pipeline
in a command substitution, where errexit does not reach it.

So: one branch and one pull request per original PR, replayed in its own
worktree. A conflict now costs one backport. The branch name is a
function of the PR number, so it cannot collide and it doubles as the
in-flight check. And "is this already on v1" is answered by searching v1
for the "(cherry picked from commit <sha>)" trailer the script itself
writes -- an exact match on something the automation owns, with no prose
in the loop.

What is left of the review is small and is fixed here: the push refuses
any branch outside backport/v1/, a branch with no commits is never
pushed, inherit_errexit is on, and an empty patch no longer counts as a
backport. Failure modes are now distinguished -- a conflict is a normal
outcome that comments once and keeps the PR queued, while a failed push
or PR call fails the run, because nobody has been told.

Dropped with the batching: --branch, --worktree, --force, --watch and the
two-step resume flow, which existed to make a failed batch recoverable.
--list, --pr and --skip-gomod remain.

Verified against real history: replaying #1301 onto the tree before its
hand-made backport reproduces 95a40be byte for byte, #1252 rejects
exactly the IsolationScope hunks and leaves .rej files, and #1195 needs
--skip-gomod. Note that #1195 does not apply cleanly with or without this
change -- the claim in the original description was wrong, and is
corrected there.

* ci: make the backport script fail loudly instead of quietly

Every one of these is the same shape: the script did the wrong thing
and reported success.

The replay is no longer tied to a named local branch. `checkout -B` was
the only thing binding backport/v1/pr-<n> to the replayed commit, its
exit status was unchecked, and the push resolved the name rather than
the commit -- so with that branch already checked out in the clone, the
commit landed on a detached HEAD and the push sent the pre-existing
branch instead, inside a run reporting "backported 1 ... failed 0". The
worktree now stays detached from start to finish, the push sends
HEAD:refs/heads/<branch>, and nothing writes or deletes a local ref. The
guards before the push check HEAD, which is now the thing being pushed.

`git worktree add` and `git commit` are checked. `|| status=$?` at the
call site makes errexit inert for the whole function, so a failing
worktree add fell through to `git apply`, failed because the directory
was not there, and posted "this change does not replay cleanly" on a
merged pull request whose patch was fine.

`die` no longer appears in the per-PR path. It is `exit 1`, which
`|| status=$?` does not catch, so it aborted the drain mid-queue: every
later PR skipped, no summary line. Those paths return 3, which main
already collects into a red run after draining the rest.

A failed `gh pr create` now takes the pushed branch back down. in_flight
treats a pushed branch as a backport already under way, so leaving it
behind removed that PR from the queue permanently -- one red run, then
silence. The likeliest trigger is the "Actions cannot create pull
requests" case the script already handles by name.

The comment the bot posts on a conflict described a recovery procedure
that destroys the work it asks for: "re-run with --pr" starts from a
clean replay and rm -rf's the worktree first. It now says what the
terminal already said -- finish it in the worktree, push HEAD.

Rebase merges are asserted against rather than assumed away. The
repository allows them, and for a rebase merge mergeCommit.oid is only
the last commit of the branch, so a multi-commit PR would be backported
in part, apply cleanly, and clear itself from the queue on the trailer.
The PR's file set is compared against the merge commit's, and a merge
commit that does not carry the whole PR is refused.

Also: the trailer test is an exact line match, so a v1 commit quoting
the phrase in prose no longer suppresses an unrelated backport; the
--skip-gomod pathspec is anchored with :(top), so a run from a
subdirectory no longer silently truncates the patch; the module rewrite
leaves `<path> v2.x` in go.mod require lines alone rather than producing
a go.mod the go tool rejects; the remote match is anchored, so it no
longer accepts google/adk-go-experimental; the workflow input is matched
with [[ =~ ]] rather than a line-oriented grep a newline gets past; the
job is skipped outside google/adk-go; bash >= 4.4 is checked by name
rather than failing on `shopt: invalid option`; and mktemp is given a
template so it works on BSD.

CONTRIBUTING no longer claims the current checkout is untouched, and
notes that the backport PR owner is now the bot, so git blame on v1
points there with the human author in the trailer.

Verified: #1301 still replays byte-identically onto the tree before
95a40be, with no local ref written and the worktree detached; the same
replay is unaffected by a pre-existing backport/v1/pr-1301 checked out
elsewhere, which is left untouched; a merge commit missing the PR's
files is refused; a prose mention of the trailer no longer matches while
a real trailer still does; --skip-gomod gives the same 5 files from the
repository root and from tool/; #1252 still leaves its .rej files.

* ci: lint the shell scripts

The lint job is golangci-lint matrixed over Go modules, so nothing in CI
reads scripts/backport.sh or .github/scripts/apidiff.sh. shellcheck with
--enable=all is what surfaces the set -e suppression class -- a git call
whose failure is swallowed while the script carries on -- which was
behind three of the findings on this pull request.

shellcheck ships on the runner image, so this is a job rather than an
install step.

* ci: fail closed when the v1 trailer search fails

Ran shellcheck --enable=all once, as a temporary CI job, to answer the
review point that nothing checks these files. Two of its findings were
real; the job is removed again rather than kept, since a permanent shell
lint across the repository is a separate decision from this change.

The one that matters is the same failure mode already fixed once for
`gh pr list`. already_backported piped `git log` into the loop through a
process substitution, where a failure is invisible: it reads as "no
candidates", meaning not yet backported, and opens a duplicate pull
request. The output is now captured and checked, so a failed search
stops the run instead of guessing.

The other was an unquoted expansion in the rebase-merge guard's message,
which would glob a path list against the working directory.

The remaining findings are deliberate: the boolean predicates in
pending_queue and the `|| status=$?` collecting per-PR outcomes both
suppress errexit on purpose, and that suppression is now handled by
checking each git call explicitly rather than relying on it.

* fix(backport): print a push command that works from a detached worktree

The conflict path's terminal instructions still said
`git push <remote> <branch>`, which was correct while the worktree
checked out a local branch of that name and stopped being correct when
it went detached. From a detached HEAD there is no such ref, so a
contributor following those steps gets 'src refspec does not match any'
after doing the resolution work, and `gh pr create` without --head has
nothing to infer from either.

Same wording as the comment the bot posts, which was already fixed.

Found while tracing what happens to a backport whose patch depends on an
earlier one: it conflicts, and this is the text it prints.

* fix(backport): close the round-three findings

Three blockers, four that were filed as smaller but fail silently and
permanently, and four cosmetic.

The rebase-merge guard refused ordinary squash merges anywhere but CI.
jq sorts by codepoint and sort(1) by locale, and under en_US.UTF-8 --
the normal workstation setting, and mine, which is why the guard's own
test passed -- comm reports present files as missing and the run tells
the operator to cherry-pick a healthy squash by hand. Both sides are now
sorted and compared under LC_ALL=C, with core.quotePath off so a
non-ASCII path cannot break the same comparison on the runner. The file
list is also paginated now; gh pr view --json files stops at 100.

The gh pr create redirect sat on the assignment rather than inside the
substitution, so ${err} was always empty, the grep could never match,
and the "Allow GitHub Actions to create and approve pull requests"
guidance -- the one diagnosis the workflow header promises, on the
failure most likely to happen first -- was dead code. The operator got a
warning ending in a colon. Moving it inside also removes an ordering
hazard: a failed mktemp made 2>"" an ambiguous redirect that fired after
the PR was created, and the failure branch then deleted its branch.

Both recovery procedures said git add -A immediately after git apply
--reject wrote .rej files next to the sources, so following them put
reject files on the backport branch and then on v1. They now delete the
rejects, stage with -u, and carry the (cherry picked from commit <sha>)
trailer, without which a hand-finished backport is invisible to the
exclusion check once its branch is deleted.

in_flight passed a bare name to ls-remote, which matches the tail of
every ref, so somebody's <user>/backport/v1/pr-<n> would read as this
backport already being under way and drop the PR from the queue for
good. Fully qualified as refs/heads/ now.

The patch pipeline's status was unchecked, and with errexit inert inside
this function a failed git show left an empty file that was read as
"nothing to backport" and tallied as a benign skip on a green run. Of
the four return codes 2 was the one absorbing tooling failure; it now
has to be earned.

The conflict marker is matched only on comments by the bot or the
authenticated user, so it can no longer be pre-posted by anyone to
suppress the only signal a human gets. The job takes timeout-minutes,
since it holds a serialized concurrency group and the default is six
hours.

Also: an explicit PR number that matches nothing now says so and why,
rather than claiming the whole queue is empty on the command the
conflict comment tells contributors to run; the return-code docstring
lists all six paths that return 3; and the push guard's comment says it
is an assertion rather than the thing holding the invariant.

Verified: the locale false positive reproduces on this PR's own three
files before the change and is gone after; the setting-specific
guidance fires now that stderr is captured; the qualified ref still
matches the real branch and no longer matches a prefixed one; a failed
patch pipeline returns 3; the recovery text prints commands that work;
and #1301 still replays byte-identically onto the tree before 95a40be.

* fix(backport): make the merge-shape guard fail closed

Self-review of the previous commit, before asking for another round.

The guard swallowed a failed API call: `gh api | sort` inside an `if`
meant an unreachable API produced an empty list, which read as "no files
to compare" and skipped the check entirely. A guard whose purpose is to
stop a silent half-backport must not disable itself silently -- if the
file list cannot be fetched the merge shape is unknown, and unknown is
not the same as fine. It now refuses.

Raised the job timeout from 10 to 30 minutes. Ten was tuned for
unwedging quickly and ignored what a kill costs: the job dies mid-drain,
and a kill landing 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 and drops that PR from the queue. A full-history
checkout plus a backlog of replays needs room.

* fix(backport): stop the recovery steps dropping files the patch adds

`git apply --reject` runs without --index, so a file the patch adds is
left untracked in the worktree. Both recovery procedures then said
`git add -u`, which stages tracked paths only, so following the printed
steps verbatim committed the modified file and silently dropped the
added one -- with the cherry-pick trailer intact, so already_backported
matched it and the pull request left the queue for good. A partial
change on v1, reached by doing exactly what the tool said.

Reproduced on a patch that modifies one tracked file and adds another:
the commit carried [existing.txt] and added.txt was gone. Back to
`git add -A`, which is now both safe and complete because the
`find -name '*.rej' -delete` added last round runs first. Verified on
the same fixture: commit carries [added.txt existing.txt], no .rej.

The posted comment's trailer was a literal placeholder where the
terminal text interpolated the real sha, because comment_conflict was
never given it. It takes the sha now.

open_backport_pr ended on cleanup_worktree, so a worktree that refused
to be removed made a landed backport return 1 and be tallied as a
conflict, telling the contributor to redo by hand a pull request that
was already open. Returns 0 explicitly.

Settled the `gh api user` question by measurement: on any API error
`gh api --jq` prints the response body to stdout, so 2>/dev/null does
not suppress it and `|| echo ''` only appends. A bad token yields a
JSON blob, which interpolated into the jq filter makes jq refuse to
parse, `gh pr view` exit non-zero, and the handler warn and return 0 --
the conflict comment silently never posts. That is the CI path, since
GITHUB_TOKEN is an installation token and GET /user wants user-to-server
auth. The login is validated against ^[A-Za-z0-9-]{1,39}$ now, so
anything else becomes empty and matches no author.

in_flight keys on an open pull request rather than on the branch.
Branch-existence conflated "under way" with "attempted and died", and
only the second is silent: a run killed between the push and
`gh pr create` left a branch that excluded its PR from every later run.
CONTRIBUTING documents the new contract.

Costing that alongside the fail-closed guard, as raised: paginating the
file list up front made a 547-file pull request ~19 requests, each able
to fail into a refusal. The guard now compares file counts first -- one
request -- and fetches the full list only when they differ, to name what
is missing. A squash carries the whole PR so the counts match; a rebase
merge records one commit so they do not. Residual noted in the comment:
equal counts over different sets would pass.

* docs(backport): record the real residual of the merge-shape guard

The comment said equal counts over different sets would pass. 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. Comparing lists rather
than content is what allows it, and the count step neither introduced
nor widened it -- but the recorded residual should be the real one.

What covers it is unchanged and now stated: the last commit's patch is
written against a parent that is not on v1, so it fails to apply rather
than landing silently.

---------

Co-authored-by: João Westerberg <baptmont@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

skill.Frontmatter.AllowedTools does not follow the spec

3 participants