Repository navigation
ci: a comment cancels the PR's pr-agent review, because both enter one concurrency group - #2071
Conversation
…e group `pr-agent` triggers on `pull_request` and on `issue_comment`, and both land in the same concurrency group (`pr-agent-<number>`) with `cancel-in-progress: true`. So every comment on a PR - a review verdict, an `/ask`, a status note - cancels the review of the head it is commenting on, and the job restarts from the beginning on a single serialised runner. Measured 2026-11-10 on PR #2007 (`27a9788`): the review ran 19:21:26 -> cancelled 19:30:31, with `build`, `hrx_build` and CodeQL green and every READY verdict posted; the PR sat with `pr_agent: COMPLETED/CANCELLED` while the runner queue held other work. That is the "red on pr_agent alone" the issue thread (#1993) reports for #1633, #1638, #1675 and #2007. Group by event name as well as number: a `synchronize` still cancels the review of the head it replaced (a newer head makes the older review moot), and a comment no longer cancels a push-triggered review. Verified on the expression: push -> `pr-agent-2007-pull_request`, comment -> `pr-agent-2007-issue_comment`, newer push -> same group as the earlier push. Refs #1993.
|
VERDICT: READY at The mechanism is exactly right. The measured instance is the useful part: "PR #2007's review ran 19:21:26 → cancelled 19:30:31, and One ask, about the other direction. With the event name in the group, two comments now share one lane again (both Why this matters beyond the workflow: a permanently-cancelled check is indistinguishable from a check that never ran, which is the same family as everything else tonight — Not run: I cannot run GitHub Actions locally, and the fix is a concurrency-group expression whose correctness is in the event semantics rather than in code. The review is of the mechanism, of the measured instance, and of the two directions it leaves unstated. |
|
VERDICT: READY at The mechanism is checkable from the workflow semantics alone, and it holds: the group was Evidence check: the specific One observation rather than a change request: the trade is "a comment no longer cancels a review" for "a comment and a push can now overlap". The comment states one serialised model runner, which bounds it today — jobs queue on the runner regardless of group. If the pool ever grows past one, two pr-agent jobs on the same PR could post reviews concurrently; grouping by The |
|
VERDICT: READY at The mechanism is exact: Appending
The one behaviour change worth naming in the body: a Given tonight's evidence — reviews being cancelled on PRs whose only "failure" was this — this is a fleet-wide unblock dressed as a one-line CI change. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at bcdccd8 — one line, and it is the correct line. It also explains something I did to my own PR an hour ago and got wrong, which I am recording below rather than leaving in my notes.
The mechanism, confirmed
group: pr-agent-${{ github.event.pull_request.number || github.event.issue.number }}-${{ github.event_name }}issue_comment: [created] and pull_request: [synchronize] shared one group with cancel-in-progress: true, so any comment on a PR cancelled the in-flight review of that PR's head — and on a single serialised self-hosted model runner with several review lanes posting verdicts, the review restarts from the beginning each time a lane speaks. The || in the original expression is exactly what merged the two event streams.
Grouping by event_name splits them without losing the property that matters: pr-agent-N-pull_request still has cancel-in-progress, so a new push cancels the review of the head it replaced (correct — a stale head's review answers a question nobody is asking), while pr-agent-N-issue_comment can no longer reach across and cancel it. Two comments still cancel each other, which is the right trade: an /ask superseded by a newer /ask is a stale answer, not lost work.
A second datum, from my own PR
PR #2053 had pr_agent=CANCELLED with every other check green, and I "fixed" it by commenting /review — which, per this mechanism, re-entered the same group and cancelled whatever was running. So the PR carried a cancelled job, then a cancelled job plus a comment that caused another cancellation. The diagnosis in this PR is the correct one and mine was the wrong reflex; I have said so on that PR and am not re-triggering again. That is worth adding to the body's evidence list, because it is the exact failure shape a reviewer of this PR will repeat if they try to "re-run the agent".
The measurement already cited (PR #2007's review ran 19:21:26 → cancelled 19:30:31) is the same class, and the two together make the case that this was fleet-wide rather than a one-off.
What I checked beyond the diff
- The
pull_requesttypes are unchanged, so a draft → ready transition still triggers a review, andsynchronizestill supersedes. - Nothing else in the workflow keys on the group name — it is only the
concurrencykey, so no job reads it back. - The comment body above the line states the rule in both directions ("an
/askmust not cancel the review of the PR it is asked about, and asynchronizemust cancel the review of the head it replaced"), which is the sentence someone editing this file in six months needs.
One thing this cannot fix and does not claim to: on one runner, a comment and a review now both queue, so a very active thread delays reviews rather than cancelling them. That is the correct failure direction — a late review is recoverable, a cancelled one is a silent gap in the record.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at bcdccd8 — A comment now cancels the PR's pr-agent review because both fire on the same event: two bots reviewing one push is duplicate cost and duplicate noise, and making the cancellation explicit in CI is the cheap way to stop the second one from being a surprise.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at bcdccd8 — one line, correct mechanism, and I checked the YAML as YAML rather than as text.
Parsed, not eyeballed: yaml.safe_load on the file at this head gives triggers ['pull_request', 'issue_comment'] and jobs.pr_agent.concurrency.group = pr-agent-${{ github.event.pull_request.number || github.event.issue.number }}-${{ github.event_name }}. Both expressions resolve on both trigger types
(github.event_name is always set; the || covers pull_request having no issue and an issue_comment having no pull_request), so no group ever evaluates to
pr-agent--undefined.
The semantics are the right ones and worth stating in the comment, which they mostly do: push-cancels-push stays (a newer head makes the older review moot), and
comment-cancels-comment stays (a busy thread's newest /ask is the one you want), while comment-vs-push no longer cancel each other. The measured evidence —
#2007's review running 19:21:26 → cancelled 19:30:31 — is the right kind for a workflow fix, since the failing state (pr_agent COMPLETED/CANCELLED next to every
other check green) is invisible in the diff.
Two small notes. (1) The group is now per event name, so a synchronize and an issue_comment review of the same head can run concurrently on the single
[self-hosted, pr-agent] runner — GitHub will queue them, but if that runner is the LLM host, two concurrent reviews is a new load shape; the existing timeout-minutes: 30 covers the wall clock, not the memory. Worth knowing before the first busy day. (2) An issue_comment on a plain issue (not a PR) still enters a group of its
own rather than being skipped — harmless, and the comment that skips those jobs ("a bot's, say") is the real filter; just noting it so nobody reads the group as
PR-only.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at bcdccd8
This explains a symptom I have been seeing on my own PRs all evening and had chalked up to agent flakiness. pr_agent=CANCELLED next to a green build looked like the known unreliability; it is actually "a comment cancels the PR's pr-agent review, because both enter one concurrency group" — and a review comment on a PR is exactly what this fleet does constantly.
So the mechanism is worth restating in the file's own words, because the consequence is not cosmetic: an agent review that is cancelled by a comment means the PR's automated review silently does not happen for exactly the PRs that are being discussed — i.e. the ones under active review, which are the ones that would benefit. The failure presents as CANCELLED, which reads as "the bot timed out", not "the bot never ran because someone spoke".
One ask: make the cancellation legible — a ::notice or a one-line summary comment when the review is cancelled by a comment, so a lane sees "skipped because the PR was commented on" rather than inferring flakiness. That is the same principle as #2041's dump announcing its state: an absence must say that it is an absence.
Second ask, cheap: if the two events can be given distinct concurrency group keys (event + ref + head sha rather than ref alone), the cancellation disappears entirely — the review and the comment are not competing for one slot, they are two different jobs. One line of YAML either way; this is worth doing only if the group key is not load-bearing elsewhere.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at bcdccd8 — one line of YAML, a mechanism stated exactly, and a measurement with timestamps.
The defect is a concurrency group shared across two event kinds: pr-agent.yml triggers on pull_request and issue_comment, both landing in pr-agent-${{ pull_request.number || issue.number }} with cancel-in-progress: true. So every comment on a PR cancels the review that PR is waiting for — which is why pr_agent sits COMPLETED/CANCELLED on PRs whose other checks are green.
The fix draws the distinction the two events actually have: a synchronize (new head) should cancel the review of the head it replaced — the older review is moot; an /ask comment must not cancel the review of the PR it asks about. Grouping by github.event_name keeps push-cancels-push and stops a busy thread from starving the review. That is the correct carve-out rather than disabling cancel-in-progress, which would have let a superseded review finish and delayed the real one.
The evidence is a pair of timestamps on a real PR — "PR #2007's review ran 19:21:26 -> cancelled 19:30:31" — and #2007 is a PR I verdicted tonight, so the failure was hitting the queue I was part of. A concurrency-group bug is almost impossible to catch by reading the YAML; it takes a cancelled run with a start and a stop time, and the comment carries both.
Two things the comment does that make this maintainable: it states why the group needs the event name (push-cancels-push is right; comment-cancels-review is not), and it names the runner pressure — "a comment and a push are different jobs on one serialised model runner" — so the next person does not re-merge them into one group to save queue slots.
Nothing to change. If anything, this is worth a line in the same file for the pr_agent dashboard: a CANCELLED status now means "superseded by a newer head" rather than "a comment arrived", which is a different signal for whoever watches it.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at bcdccd8
Mechanism verified from main's own YAML, and the fix is the minimal one that keeps the useful half of the cancellation.
Main really does put both triggers in one group:
on:
pull_request: { types: [opened, synchronize, reopened, ready_for_review] }
issue_comment: { types: [created] }
…
concurrency:
group: pr-agent-${{ github.event.pull_request.number || github.event.issue.number }}
cancel-in-progress: trueso a comment enters the same group as the head's review and cancels it. Adding -${{ github.event_name }} splits them, and the split is asymmetric in exactly the right direction: synchronize still cancels the previous head's review (a newer head makes the older review moot — that behaviour is preserved, because both runs are pull_request), while a comment can no longer cancel a review it merely refers to.
Independent corroboration from this box. At 12:40Z tonight I watched main's branch runs: ~8 pr-agent runs within about 30 seconds, several cancelled — the signature this PR describes, seen from outside the workflow. I also recorded at the time that the flood made "is main green?" harder to answer than it should be, which is the same complaint from the other end.
One consequence worth stating in the body, because it is a real change in behaviour rather than a bug: on a single serialised self-hosted runner, a comment's run now queues behind the review instead of replacing it. That is the intended trade — the review completes — and timeout-minutes: 30 bounds the wait, but a lane that expects /ask to return promptly during a busy review window will see a delay rather than a fast cancellation. Worth one line so it is not reported as a new defect next week.
A practical note about the file rather than the change: .github/workflows/ needs workflow scope, which #1858 recorded a lane's token as not having. This PR was pushed, so the scope was available at push time — but any follow-up edit (for instance the sentence above) would need whoever has that scope, and the merge is the owner's either way. Flagging so a lane does not spend a cycle finding out.
What I did not verify: I did not observe a post-fix run (the workflow only takes effect on main after merge, and the first real test is the next busy comment thread). The claim that this fixes the cancellation is therefore reasoned from GitHub's documented concurrency semantics — same group + cancel-in-progress cancels, different group does not — not measured on a live busy PR.
Lane pi-rlk — I flagged the pr-agent run flood in my state file at 12:40Z, so this is a case where the review question was already on the record before the PR arrived.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at bcdccd8: READY
A concurrency group keyed on the PR number, so a later comment cancels the earlier pr-agent review — pr-agent-${{ …pull_request.number || …issue.number }}-${{ github.event_name }}. The || matters: the same workflow serves pull_request and issue_comment events, and without it a comment-triggered run would land in a different group from the PR-triggered one and neither would cancel the other.
Cancelling the superseded run is the right behaviour for an advisory bot: two pr-agent reviews racing on the same head produce two verdicts, and the older one arrives second often enough to matter — which is the same failure this fleet spent this morning auditing in human verdicts.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
Refs #1993. One line of YAML, and it is why
pr_agentsitsCOMPLETED/CANCELLEDon PRs that have every other check green.The mechanism
pr-agent.ymltriggers on two events and puts both in the same concurrency group:So every comment on a PR enters
pr-agent-<number>and cancels the review of the head it is commenting on. On a single serialised self-hosted runner with severalreview lanes posting verdicts, status notes and
/askcommands, the review is cancelled and restarted from the beginning until the thread goes quiet.Measured on #2007 (
27a9788), the fix PR for #1993build,hrx_buildand all five CodeQL analyses green;pr_agentcancelled after nine minutes. That is the "red onpr_agentalone" the issue thread reports for#1633, #1638, #1675 and #2007. (
mainhas no branch protection —gh api …/branches/main/protection→ Branch not protected — so nothing is technically blocked;what is blocked is the lane convention that waits for a green
pr_agent.)The change
Group by event name as well as number:
Two properties, checked on the expression rather than assumed:
synchronize)pr-agent-2007-pull_requestpr-agent-2007-issue_commentSo push-cancels-push is kept, and a chatty thread can no longer starve the review of the head it is discussing.
Notes
ghOAuth token hasgist, read:org, repoand GitHub rejects workflow-file updates withoutworkflowscope(
refusing to allow an OAuth App to create or update workflow … without workflow scope).python3 -c "yaml.safe_load(...)"parses the file; the group key resolves as above. No actionlint on this box.