fix(skilltoolset): accept scalar allowed-tools frontmatter - #1301
Conversation
|
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. |
|
CLA details have been updated. @googlebot rescan |
kdroste-google
left a comment
There was a problem hiding this comment.
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?
|
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:
Separator-only input such as The build, race tests, lint, and module tidy checks all pass locally after the changes. Thanks again for helping improve the implementation. |
kdroste-google
left a comment
There was a problem hiding this comment.
@ktsoator, thank you for those fixes!
I'm glad to hear that you find ADK-GO useful - waiting for your next contributions!
|
Thanks both for the fast turnaround on this one! |
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.
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.
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.
* 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>
Link to Issue or Description of Change
Closes: #1296
skill.Frontmatter.AllowedToolspreviously accepted only YAML sequences, while the Agent Skills specification definesallowed-toolsas a scalar string.This change:
AllowedTools []stringAPI unchanged;Testing Plan
go build -mod=readonly workgo test -race -mod=readonly -count=1 -shuffle=on workgolangci-lint runin the root andplugin/agentanalyticsmodulesgo mod tidy -diffin both modules