fix(dispatch): label-aware concurrency prevents routing-label cancellation - #5472
Conversation
PR Summary by QodoFix dispatch concurrency for labeled events to prevent routing cancellation
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
|
🤖 Review · |
Site previewPreview: https://2bf35781-site.fullsend-ai.workers.dev Commit: |
4ec46ff to
761aadf
Compare
|
🤖 Finished Review · ✅ Success · Started 4:01 PM UTC · Completed 4:19 PM UTC |
Code Review by Qodo
1. PR label whitelist mismatch
|
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsMedium
Low
Previous run (5)ReviewFindingsMedium
Low
Previous run (6)ReviewFindingsMedium
Low
Previous run (7)ReviewFindingsMedium
Previous run (8)ReviewFindingsMedium
Low
Previous run (9)ReviewFindingsMedium
Low
Previous run (10)ReviewFindingsMedium
Low
Previous run (11)ReviewFindingsMedium
Low
Previous run (12)ReviewFindingsMedium
Low
Previous run (13)ReviewFindingsMedium
Low
Labels: PR adds 138 lines of Go test code validating the new shim label filtering and concurrency key behavior. Previous run (14)ReviewFindingsMedium
Low
Previous run (15)ReviewFindingsMedium
Low
Previous run (16)ReviewFindingsMedium
Low
Previous run (17)ReviewFindingsMedium
Low
Previous run (18)ReviewFindingsMedium
Low
Previous run (19)ReviewFindingsMedium
Low
Previous run (20)ReviewFindingsMedium
Low
Previous run (21)ReviewFindingsMedium
Low
Previous run (22)ReviewFindingsMedium
Low
Previous run (23)ReviewFindingsMedium
Low
Previous run (24)ReviewFindingsMedium
Low
Previous run (25)ReviewFindingsMedium
Previous run (26)ReviewFindingsMedium
Previous run (27)ReviewFindingsMedium
Previous run (28)ReviewFindingsMedium
Previous run (29)ReviewFindingsMedium
Previous run (30)ReviewFindingsMedium
Low
Previous run (31)ReviewFindingsMedium
Previous run (32)ReviewFindingsMedium
Low
Previous run (33)ReviewFindingsMedium
Low
Previous run (34)ReviewFindingsMedium
Low
Previous run (35)ReviewFindingsMedium
Low
Labels: PR modifies dispatch workflow routing (concurrency groups and labeled-event filtering) |
761aadf to
e909805
Compare
|
🤖 Finished Review · ✅ Success · Started 4:45 PM UTC · Completed 4:58 PM UTC |
waynesun09
left a comment
There was a problem hiding this comment.
Multi-agent review squad pass (4 reviewers: Claude x2, Grok, Gemini) — no Critical/High findings survived verification. 7 Medium findings posted inline covering: unverified core fix behavior in a live GH Actions environment, a weak test assertion (substring-only, not semantic), a fragile positional test slice, a stale ADR invariant re: per-role concurrency, an ADR update that overgeneralizes which shim gained label-aware concurrency, an unacknowledged "Confidence: Medium" root cause from the source issue, and hand-copied if:/group: blocks with no single source of truth. Two prior review threads (label-whitelist mismatch, weak concurrency test) are already resolved/declared-intentional and not re-flagged.
e909805 to
aed6789
Compare
|
🤖 Finished Review · ✅ Success · Started 7:00 PM UTC · Completed 7:14 PM UTC |
aed6789 to
9ea85ce
Compare
|
🤖 Finished Review · ✅ Success · Started 12:34 PM UTC · Completed 12:47 PM UTC |
|
🤖 Finished Review · ✅ Success · Started 3:45 PM UTC · Completed 4:01 PM UTC |
waynesun09
left a comment
There was a problem hiding this comment.
Multi-agent review squad pass (3 reviewers: Claude x2, Grok) against b24d7d4, plus a re-verification of the previous round.
The blocker from the last round is cleared. Three things resolved well:
- ADR immutability — the diff is now purely additive (
6+/0-), original Consequences bullet restored verbatim, short blockquote Note appended matching the existing pattern. This is the right shape perCONTRIBUTING.md:61, and it also corrects an earlier ask of mine that pointed the wrong way: I asked for the parent bullet to be rewritten because it contradicted its footnote, when the convention is to leave the historical text alone and annotate. That's on me — what shipped is correct and my earlier request was not. - The
unlabeleddoc contradiction — no false claim remains in either the ADR or the glossary. - "Agent workflows re-resolve current state … stale-payload races are benign" — removed entirely. That unverified, load-bearing claim was flagged by every reviewer in all three rounds; good to see it go rather than get re-hedged.
Posting 4 Medium findings that are new substance on lines this PR adds, plus one on the PR description below. Nothing outstanding is a behavior defect — this is test-hardening and doc accuracy. Low findings held back (the guard regex still hard-pins || immediately after != 'labeled', so a future unlabeled conjunct would fail the test; the glossary trim dropped the startsWith case-insensitivity asymmetry, which was accurate and didn't move to the guide; ready-for-merge is still missing from the glossary's review-outcome list, raised in round 1). Happy to add any of those.
[MEDIUM] — PR description no longer matches the shipped code, and it becomes the squash-merge message
Three drifts at b24d7d4:
- "
if:guard skips the dispatch job entirely for labeled events where the label isn'tready-to-code,ready-for-review, orready-for-triage" — the code implementsstartsWith('ready-'), a strictly weaker prefix match, not that three-label allowlist. - "Applied to all three shim files" listing
shim-per-repo.yaml"(if:guard only)" — that file is no longer in the changed-file set, andTestShimLabeledEventFiltering'sno-label-guardsubtest now asserts the opposite, that it must not carry the filter. - Test plan: "validates all three shims have the routing-label whitelist" — neither "all three" nor "whitelist" describes the test as written.
After eight rounds this body is the artifact a future reader reaches for first, and it's the commit message that lands on main. Worth a rewrite to: prefix filter, two shims changed, per-repo deliberately exempt for BYOA-harness compatibility.
Also unchecked: the one end-to-end validation ([ ] Apply 5+ labels to a test issue in rapid succession) is still open. That's been acknowledged as a post-merge item, so noting it for the record rather than re-litigating.
|
🤖 Finished Review · ✅ Success · Started 7:03 PM UTC · Completed 7:19 PM UTC |
waynesun09
left a comment
There was a problem hiding this comment.
Review pass: 1 new finding posted inline below (design-alternative note on the shim concurrency block). The other candidate findings from this pass overlapped with points already raised and resolved/acknowledged in the existing review threads and the most recent review round, so they were held back to avoid duplicating the record.
|
🤖 Review · |
|
🤖 Finished Review · ✅ Success · Started 7:55 PM UTC · Completed 8:10 PM UTC |
waynesun09
left a comment
There was a problem hiding this comment.
Multi-agent review squad pass against 8be7d13 (3 dispatched, 2 completed — the Grok reviewer terminated without producing findings, so this round is Claude x2 plus my own verification).
No CRITICAL findings, and the shipped behavior is sound. One reviewer hand-evaluated all 12 subscribed event/action pairs and killed 9 of 12 mutations against the guard and concurrency expressions. actionlint passes, both new tests pass, and all three routers agree on the TRIGGERING_LABEL set (ready-for-triage, ready-to-code, ready-for-review) — so TestRoutingLabelPrefixDrift is green for the right reason, not by accident.
Posting 1 High + 3 Medium. Everything is test-hardening or docs; nothing below is a behavior defect.
Deliberately not re-raised: both reviewers flagged the ready- prefix being promoted to a public contract as a premature decision. That's been declared intentional four times across earlier rounds, so I'm treating it as settled rather than re-litigating it.
Low findings held back (happy to add any): the ADR blockquote sits at column 0 between two list items, which splits the Consequences <ul> into two lists — indenting it two spaces would nest it under the bullet it annotates; the ADR Note attributes the per-repo shim's missing concurrency group to "BYOA compat", but shim-per-repo.yaml:15-16, scaffold_test.go:126, and this PR's own test comment at workflow_call_alignment_test.go:713-714 all say it's because per-role groups live in reusable-dispatch.yml — BYOA compat is why it's exempt from the prefix guard, a different thing; the glossary's new link has no anchor and the target has no matching heading; and the concurrency assertions don't pin the fullsend-dispatch- namespace or the - separator (dropping the separator to yield fullsend-dispatch-12ready-to-code survives the full package).
One observation across rounds. Each round's test fix has been syntactic, so each one closes the specific mutation raised and leaves a differently-shaped hole — Contains → Regexp → parens pinned → join operator pinned → broadened matcher, and the two findings below are the next shape. The durable fix is the one suggested a few times and not yet taken: a table-driven evaluator over (event_name, action, label.name) tuples asserting the guard's boolean result and the rendered group string, plus a self-test feeding each non-conforming comparison style through labelPatterns to assert it's recognized. That would end this particular treadmill.
Still open from earlier rounds: the PR description hasn't been updated (three-label allowlist vs. shipped prefix filter; "all three shim files" vs. two; test plan claiming the test validates a "whitelist" on all three shims, which the /no-label-guard subtest asserts the opposite of). It becomes the squash-merge message. The manual burst-test checkbox also remains unticked, which both reviewers noted as the one thing static assertions can't establish.
|
🤖 Finished Review · ✅ Success · Started 10:25 PM UTC · Completed 10:41 PM UTC |
|
🤖 Review · |
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass focused on the new ADR 0034 amendment note. One finding below.
|
🤖 Review · |
|
/fs-review |
|
🤖 Review · |
|
🤖 Finished Review · ✅ Success · Started 3:53 PM UTC · Completed 4:06 PM UTC |
|
🤖 Review · |
|
🤖 Finished Review · ✅ Success · Started 6:09 PM UTC · Completed 6:24 PM UTC |
waynesun09
left a comment
There was a problem hiding this comment.
Review pass: 1 new finding posted inline below. A second candidate (PR description misdescribing shim-per-repo.yaml's guard status and calling the ready- prefix filter a "whitelist") was held back — already raised and left open in an earlier review round on this PR, so not re-posted here.
|
🤖 Finished Review · ❌ Failure · Started 7:11 PM UTC · Completed 7:12 PM UTC |
|
/fs-review |
…ation (fullsend-ai#2452) Signed-off-by: Greg Allen <gallen@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Greg Allen <gallen@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 9:39 PM UTC · Completed 9:54 PM UTC |
|
🤖 Retro · Started 10:12 PM UTC |
Summary
Fixes #2452. When triage applies multiple labels rapidly, all
issues/labeledevents shared the same concurrency group (fullsend-dispatch-{number}). GitHub Actions cancels intermediate pending runs, so theready-to-codeevent could be cancelled while a non-routing label (type/bug) survives — the code agent never dispatches.Two complementary fixes:
labeledevents, include the label name in the group soready-to-codeandtype/bugget separate concurrency slots. Non-labeled events share a commondispatchsuffix to preserve existing queuing behavior.if:guard skips the dispatch job entirely for labeled events where the label isn'tready-to-code,ready-for-review, orready-for-triage. Prevents non-routing labels from consuming concurrency slots or wasting runner time.Applied to all three shim files:
.github/workflows/fullsend.yaml(this repo)internal/scaffold/fullsend-repo/templates/shim-workflow-call.yaml(per-org template)internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml(per-repo template,if:guard only — no concurrency group)Test plan
TestShimLabeledEventFiltering— validates all three shims have the routing-label whitelist and the workflow-call shims have label-aware concurrency keysactionlintpasses onfullsend.yamlready-to-codeas a non-final label). Verify theready-to-coderun completes with "Routed to stage: code" rather than being cancelled🤖 Generated with Claude Code