Skip to content

fix(dispatch): per-role two-layer concurrency for per-repo (#981)#2465

Open
ifireball wants to merge 6 commits into
fullsend-ai:mainfrom
ifireball:fix/981-per-repo-concurrency
Open

fix(dispatch): per-role two-layer concurrency for per-repo (#981)#2465
ifireball wants to merge 6 commits into
fullsend-ai:mainfrom
ifireball:fix/981-per-repo-concurrency

Conversation

@ifireball

Copy link
Copy Markdown
Member

Summary

  • Remove the monolithic fullsend-dispatch-{issue|pr} concurrency group from the per-repo shim template — it serialized unrelated roles and could drop the wrong pending run when multiple label events arrived in quick succession (Concurrency group cancels code dispatch when triage applies multiple labels #2452).
  • Add per-role cancel-in-progress: true concurrency on all six stage jobs in reusable-dispatch.yml (triage, code, review, fix, retro, prioritize).
  • Mirror the same group keys on all reusable-{stage}.yml workflows (defense-in-depth for overlapping workflow_call invocations and manual re-triggers).

Roles operate independently: a review dispatch does not cancel triage, code, fix, etc.

History (why this is messy)

  1. Add concurrency group to dispatch-review job in fullsend.yaml #981 (May 2025): Old per-stage shim jobs had concurrency on dispatch-triage, dispatch-fix, etc., but dispatch-review and dispatch-code did not → PR refactor: unify mint provisioning and harden URL validation #930 saw 23 review dispatches, only 8 useful.
  2. ADR 0034: Collapsed to one shim dispatch job with a shared queue group; per-stage cancellation moved to per-org thin callers (review.yml, etc.).
  3. ADR 0033 / per-repo: reusable-dispatch.yml was documented to get per-stage concurrency (PR feat: add per-repo installation mode (ADR 0033) #799) but never did.
  4. PR feat(dispatch): synchronous workflow_call event dispatch (ADR 41) #1611 (open): Ports concurrency to reusable workflows only — misses the dispatch router layer and keeps the monolithic shim queue.

This PR implements the two-layer, per-role policy ADR 0033 intended for per-repo installs.

Issues

Closes #981
Closes #982
Closes #1357

Also addresses:

Not solved here (follow-ups):

Test plan

  • mise exec -- go test ./internal/scaffold/ -v
  • Manual: rapid pushes on a per-repo enrolled repo → only the latest in-flight review completes; triage/code on the same issue still dispatch independently

Made with Cursor

…ai#981)

Remove the monolithic per-repo shim concurrency group and add matching
cancel-in-progress groups on reusable-dispatch stage jobs plus all
reusable-{stage}.yml workflows so roles dedupe independently.

Signed-off-by: Barak Korren <bkorren@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Per-role concurrency for per-repo dispatch workflows
🐞 Bug fix ⚙️ Configuration changes 🧪 Tests 🕐 40+ Minutes

Grey Divider

Description

• Remove monolithic per-repo shim concurrency that serialized roles and dropped pending runs.
• Add per-role cancel-in-progress concurrency to all reusable-dispatch stage jobs.
• Mirror concurrency on reusable stage workflows and add tests to prevent regressions.
Diagram

graph TD
  A["GitHub event"] --> B["per-repo shim"] --> C["reusable-dispatch"] --> D["stage job (role)"] --> E["reusable-{stage} workflow"]
  D --> F["Concurrency group (role+repo+issue/pr)"]
  E --> F
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Only workflow-level concurrency (reusable-{stage}.yml)
  • ➕ Fewer moving parts; concurrency defined close to the agent implementation
  • ➕ Avoids repeating group logic in the dispatch router
  • ➖ Does not dedupe at the dispatch layer; multiple dispatch stage jobs can still churn before cancellation
  • ➖ Less robust against overlapping workflow_call invocations and manual retriggers
2. Single monolithic shim concurrency group

Recommendation: Keep the PR’s two-layer, per-role approach: job-level concurrency in reusable-dispatch prevents dispatch churn, while workflow-level concurrency in reusable-{stage}.yml provides defense-in-depth for overlapping workflow_call invocations and manual retriggers. The removed monolithic shim group is a known footgun for correctness and throughput.

Files changed (10) +224 / -18

Bug fix (2) +26 / -3
reusable-dispatch.ymlEnforce per-role concurrency on all dispatch stage jobs +22/-0

Enforce per-role concurrency on all dispatch stage jobs

• Adds per-stage job concurrency groups with cancel-in-progress for triage, code, review, fix, retro, and prioritize. Documents that roles operate independently and that concurrency is mirrored on reusable stage workflows.

.github/workflows/reusable-dispatch.yml

shim-per-repo.yamlRemove monolithic concurrency from per-repo shim template +4/-3

Remove monolithic concurrency from per-repo shim template

• Deletes the shared fullsend-dispatch concurrency group from the shim dispatch job. Adds documentation explaining why concurrency belongs to per-role stage jobs/workflows to avoid serializing roles and dropping pending runs (#2452).

internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml

Tests (2) +137 / -3
scaffold_test.goAdd regression test for per-repo shim concurrency removal +15/-0

Add regression test for per-repo shim concurrency removal

• Introduces TestShimPerRepoTemplateContent to ensure the per-repo shim starts with a YAML document marker, contains expected jobs/markers, and does not define a monolithic concurrency block. Asserts the template includes the explanatory concurrency comment.

internal/scaffold/scaffold_test.go

workflow_call_alignment_test.goTest per-role concurrency in dispatch and reusable stage workflows +122/-3

Test per-role concurrency in dispatch and reusable stage workflows

• Extends YAML parsing structs to include concurrency and adds expectation tables per stage. Adds tests to verify reusable-dispatch stage jobs and reusable-{stage}.yml workflows declare cancel-in-progress concurrency groups with the expected key fragments.

internal/scaffold/workflow_call_alignment_test.go

Other (6) +61 / -12
reusable-code.ymlAdd workflow-level per-role concurrency for Code agent +9/-2

Add workflow-level per-role concurrency for Code agent

• Adds a cancel-in-progress concurrency group keyed by source repo and issue number. Updates header comments to reflect dispatch-based calling and the mirrored concurrency policy.

.github/workflows/reusable-code.yml

reusable-fix.ymlAdd workflow-level per-role concurrency for Fix agent +16/-2

Add workflow-level per-role concurrency for Fix agent

• Adds cancel-in-progress concurrency keyed by source repo and PR/issue identifiers, with fallback to inputs.pr_number. Expands comments to clarify that newer human or bot fix requests preempt older runs.

.github/workflows/reusable-fix.yml

reusable-prioritize.ymlAdd workflow-level per-role concurrency for Prioritize agent +9/-2

Add workflow-level per-role concurrency for Prioritize agent

• Adds cancel-in-progress concurrency keyed by source repo and issue number. Updates comments to match the dispatch router architecture.

.github/workflows/reusable-prioritize.yml

reusable-retro.ymlAdd workflow-level per-role concurrency for Retro agent +9/-2

Add workflow-level per-role concurrency for Retro agent

• Adds cancel-in-progress concurrency keyed by source repo and PR/issue number. Updates documentation comments to align with reusable-dispatch calling semantics.

.github/workflows/reusable-retro.yml

reusable-review.ymlAdd workflow-level per-role concurrency for Review agent +9/-2

Add workflow-level per-role concurrency for Review agent

• Adds cancel-in-progress concurrency keyed by source repo and PR/issue number. Updates comments to describe the mirrored, per-role concurrency policy.

.github/workflows/reusable-review.yml

reusable-triage.ymlAdd workflow-level per-role concurrency for Triage agent +9/-2

Add workflow-level per-role concurrency for Triage agent

• Adds cancel-in-progress concurrency keyed by source repo and issue number. Updates comments to reflect dispatch-based invocation and the per-role policy.

.github/workflows/reusable-triage.yml

@github-actions

github-actions Bot commented Jun 21, 2026

Copy link
Copy Markdown

Site preview

Preview: https://c0c69ed0-site.fullsend-ai.workers.dev

Commit: c0ffc0245a3b1518ed51922bf2cb1934b7a4ebf8

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 21, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:39 AM UTC · Completed 8:53 AM UTC
Commit: 8e0e3c9 · View workflow run →

@qodo-code-review

qodo-code-review Bot commented Jun 21, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (1) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 58 rules
✅ Skills: writing-user-docs, writing-adrs

Grey Divider


Action required

1. ADR 0034 concurrency docs stale ✓ Resolved 📎 Requirement gap ⚙ Maintainability
Description
The implementation now uses per-role cancel-in-progress concurrency groups, but the docs still
describe a single shared concurrency group that cancels runs across stages. This leaves
post-ADR-0041 synchronous dispatch concurrency/cancellation semantics incorrect/ambiguous for
operators and integrators.
Code

internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml[R17-19]

+# Concurrency: per-role cancel-in-progress groups live in reusable-dispatch.yml
+# stage jobs and reusable-{stage}.yml workflows — not on this shim. A monolithic
+# shim group would serialize unrelated roles and drop pending runs (#2452).
Relevance

⭐⭐⭐ High

Team frequently accepts updating ADRs to match implementation (e.g., ADR updates accepted in PR
#730, #1215).

PR-#730
PR-#1215
PR-#1966

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The shim template and reusable dispatch workflow now explicitly implement per-role concurrency where
roles operate independently, but ADR 0034 still documents a single concurrency group that cancels
in-progress runs across stages. This violates the requirement to document post-ADR-0041 concurrency
and cancel-in-progress semantics for synchronous dispatch/fan-out.

Document post-ADR-0041 concurrency and cancel-in-progress semantics for synchronous dispatch and fan-out
internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml[17-19]
.github/workflows/reusable-dispatch.yml[6-8]
docs/ADRs/0034-centralized-shim-routing-via-dispatch.md[138-143]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR changes dispatch concurrency semantics to per-role, cancel-in-progress groups, but documentation still claims a single shared concurrency group that cancels across stages.

## Issue Context
This PR implements independent per-role dispatching (triage/code/review/fix/retro/prioritize). Existing ADR text describing a monolithic concurrency group is now misleading and conflicts with the new behavior.

## Fix Focus Areas
- docs/ADRs/0034-centralized-shim-routing-via-dispatch.md[134-143]
- docs/ADRs/0041-synchronous-workflow-call-event-dispatch.md[123-139]
- .github/workflows/reusable-dispatch.yml[1-12]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

2. Review concurrency missing HEAD SHA 📎 Requirement gap ☼ Reliability
Description
The new review concurrency groups are keyed only by repo + PR/issue number, not PR HEAD SHA, so
duplicate review dispatches for the same SHA can still produce multiple completed reviews
(especially when events arrive sequentially). This violates the requirement to deduplicate review
runs per PR HEAD SHA.
Code

.github/workflows/reusable-dispatch.yml[R390-392]

+    concurrency:
+      group: fullsend-review-${{ github.repository }}-${{ github.event.pull_request.number || github.event.issue.number }}
+      cancel-in-progress: true
Relevance

⭐ Low

Repo treats SHA dedupe as separate agent logic; PR #2048 only forwards head SHA, not concurrency.

PR-#2048
PR-#380

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The review job and review reusable workflow define concurrency groups without including the PR HEAD
SHA, despite the dispatch workflow extracting pull_request.head.sha into the event payload. The
checklist requires SHA-level deduplication (or an equivalent skip-if-already-reviewed guard), which
is not implemented by these group keys.

Review dispatch deduplicates runs for the same PR HEAD SHA
.github/workflows/reusable-dispatch.yml[321-326]
.github/workflows/reusable-dispatch.yml[390-392]
.github/workflows/reusable-review.yml[8-10]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Review dispatch deduplication is required at the PR HEAD SHA level, but the new concurrency groups do not include the SHA and there is no clear early-exit guard for already-reviewed SHAs.

## Issue Context
`reusable-dispatch.yml` already constructs an event payload containing `pull_request.head.sha`, but the review job concurrency group does not use it.

## Fix Focus Areas
- .github/workflows/reusable-dispatch.yml[316-326]
- .github/workflows/reusable-dispatch.yml[386-392]
- .github/workflows/reusable-review.yml[8-10]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Concurrency mirror test too weak 🐞 Bug ⚙ Maintainability
Description
The new concurrency tests only assert that concurrency.group contains a prefix and a few
substrings, so adding extra suffixes/segments or otherwise changing the group structure can still
pass while breaking the intended mirror between dispatch stage jobs and reusable stage workflows.
Code

internal/scaffold/workflow_call_alignment_test.go[R301-345]

+// TestReusableDispatchStageConcurrency validates per-role cancel-in-progress groups
+// on all stage jobs in reusable-dispatch.yml (#981, #982, ADR 0033).
+func TestReusableDispatchStageConcurrency(t *testing.T) {
+	content, err := os.ReadFile(filepath.Join("..", "..", ".github", "workflows", "reusable-dispatch.yml"))
+	require.NoError(t, err)
+
+	var caller callerWorkflow
+	require.NoError(t, yaml.Unmarshal(content, &caller))
+
+	for stage, expect := range dispatchStageConcurrencyExpectations {
+		t.Run(stage, func(t *testing.T) {
+			job, ok := caller.Jobs[stage]
+			require.True(t, ok, "job %q should exist", stage)
+			require.NotNil(t, job.Concurrency, "job %q should declare a concurrency group", stage)
+			assert.Contains(t, job.Concurrency.Group, expect.groupPrefix)
+			for _, fragment := range expect.groupMust {
+				assert.Contains(t, job.Concurrency.Group, fragment,
+					"job %q concurrency group should reference %q", stage, fragment)
+			}
+			assert.True(t, job.Concurrency.CancelInProgress,
+				"job %q should cancel in-progress runs when a newer dispatch arrives", stage)
+		})
+	}
+}
+
+// TestReusableWorkflowConcurrency validates workflow-level concurrency on all
+// reusable stage workflows (defense-in-depth; mirrors dispatch stage jobs).
+func TestReusableWorkflowConcurrency(t *testing.T) {
+	for stage, expect := range reusableStageConcurrencyExpectations {
+		t.Run(stage, func(t *testing.T) {
+			path := filepath.Join("..", "..", ".github", "workflows", fmt.Sprintf("reusable-%s.yml", stage))
+			content, err := os.ReadFile(path)
+			require.NoError(t, err)
+
+			var wf reusableStageWorkflow
+			require.NoError(t, yaml.Unmarshal(content, &wf))
+			require.NotNil(t, wf.Concurrency, "reusable-%s.yml should declare workflow-level concurrency", stage)
+			assert.Contains(t, wf.Concurrency.Group, expect.groupPrefix)
+			for _, fragment := range expect.groupMust {
+				assert.Contains(t, wf.Concurrency.Group, fragment,
+					"reusable-%s.yml concurrency group should reference %q", stage, fragment)
+			}
+			assert.True(t, wf.Concurrency.CancelInProgress,
+				"reusable-%s.yml should cancel in-progress runs", stage)
+		})
Relevance

⭐ Low

Similar requests to strengthen tests beyond substring/contains were rejected previously (PR #390).

PR-#390

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The tests only check that the group contains groupPrefix and each groupMust fragment, and never
assert the full group string (or equivalence between dispatch and stage layers), so group drift can
pass unnoticed.

internal/scaffold/workflow_call_alignment_test.go[301-345]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The concurrency validation tests use `assert.Contains` checks (prefix + required fragments) rather than verifying a strict expected group shape/equivalence. This allows semantically-breaking changes (e.g., appending an extra suffix while keeping the fragments) to pass CI, defeating the stated goal that groups are “mirrored”.

## Issue Context
These tests were added to enforce per-role cancel-in-progress groups across both layers.

## Fix Focus Areas
- internal/scaffold/workflow_call_alignment_test.go[301-345]

## Suggested fix approach
- Replace the `Contains`-based checks with stricter validation, e.g.:
 - For each stage, assert the group matches an explicit regex for the full string (not just prefix/fragments), or
 - Maintain a stage→expected-group-expression map and `assert.Equal` against the parsed YAML values.
- If exact-string equality is intentionally flexible, at least assert **no extra characters** beyond the expected template (e.g., `assert.Regexp` with `^...$`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml Outdated
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 21, 2026

Copy link
Copy Markdown

Review

Findings

Medium

Low

  • [edge-case] .github/workflows/reusable-dispatch.yml:412 — The review stage concurrency group uses github.event.pull_request.number || github.event.issue.number. When review is triggered by issues/labeled (ready-for-review), the key uses the issue number; when triggered by pull_request_target, it uses the PR number. For a linked issue/PR pair where the numbers differ (e.g., issue Add some harness engineering posts to the backlog #10 linking to PR Add performance and load impact verification problem document #15), the two triggers produce different concurrency keys and will not cancel each other. Two concurrent review runs are possible for the same logical work item. The same concern applies to fix and retro stages.

  • [edge-case] internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml — Existing per-repo installations retain their old shim with the monolithic cancel-in-progress: false concurrency group until re-scaffolded. After merge, they will simultaneously have the old shim-level queue and the new per-role cancel-in-progress: true groups on reusable-dispatch.yml stage jobs (delivered via @v0). The interaction is safe (shim serialization is more conservative), but the shim queue prevents per-role concurrency from taking full effect until re-scaffolded.

  • [stale-concurrency-reference] docs/ADRs/0033-per-repo-installation-mode.md:350 — ADR 0033 resolved question states "Concurrency groups are set at the per-stage job level inside reusable-dispatch.yml, matching the per-org behavior." The statement remains accurate after this PR (concurrency is at the per-stage job level), but does not describe the specific per-role naming pattern or two-layer structure now in place. A short annotation referencing Add concurrency group to dispatch-review job in fullsend.yaml #981 would maintain doc currency.

Previous run

Review

Findings

Medium

  • [protected-path] .github/workflows/reusable-*.yml, internal/scaffold/ — Seven workflow files under .github/ and scaffold templates are modified. The PR links to issues Add concurrency group to dispatch-review job in fullsend.yaml #981, Add concurrency group to dispatch-code job in fullsend.yaml #982, Enable cancel-in-progress for review dispatches on the same PR #1357 and explains the rationale for the concurrency changes. Human approval is always required for protected-path changes, regardless of context.

  • [scope-authorization] docs/ADRs/0034-centralized-shim-routing-via-dispatch.md:138 — The PR replaces 6 lines of ADR 0034's Consequences section (describing a single concurrency group) with 9 lines describing per-role concurrency groups. Per AGENTS.md, accepted ADRs are point-in-time records; substantial rewrites to Context, Decision, or Consequences sections are prohibited. This change exceeds the "minor annotation" exception (cross-references, short notes, typo fixes). The new concurrency model represents a material architectural change that supersedes the original ADR's documented behavior. Consider writing a new ADR that documents the per-role concurrency model and marks it as superseding ADR 0034's concurrency section, with ADR 0034 receiving only a short cross-reference annotation.

Low

  • [edge-case] .github/workflows/reusable-dispatch.yml:412 — The review stage concurrency group uses github.event.pull_request.number || github.event.issue.number. When review is triggered by issues/labeled (ready-for-review), the key uses the issue number; when triggered by pull_request_target, it uses the PR number. For a linked issue/PR pair where the numbers differ (e.g., issue Add some harness engineering posts to the backlog #10 linking to PR Add performance and load impact verification problem document #15), the two triggers produce different concurrency keys and will not cancel each other. Two concurrent review runs are possible for the same logical work item. The same concern applies to fix and retro stages. This is a pre-existing design limitation shared with the per-org thin callers.

  • [edge-case] internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml — Existing per-repo installations retain their old shim with the monolithic cancel-in-progress: false concurrency group until re-scaffolded. After merge, they will simultaneously have the old shim-level queue and the new per-role cancel-in-progress: true groups on reusable-dispatch.yml stage jobs (delivered via @v0). The interaction is safe (shim serialization is more conservative), but the shim queue prevents per-role concurrency from taking full effect until re-scaffolded.

  • [scope-authorization] docs/ADRs/0041-synchronous-workflow-call-event-dispatch.md:137 — The PR replaces a forward-looking consequence ("may need concurrency review") with a resolution statement. While closer to the "short note linking to newer decisions" exception that AGENTS.md permits than the ADR 0034 rewrite, it still modifies the original text rather than appending an annotation.

  • [concurrency-naming-inconsistency] .github/workflows/reusable-dispatch.yml — Concurrency groups use github.repository for the repo component, while the established convention in per-org thin caller workflows uses inputs.source_repo. In per-repo mode github.repository resolves correctly to the target repo (the shim runs there), so this is functionally correct but cosmetically inconsistent. Consider documenting the distinction.

Previous run (2)

Review

Findings

Medium

  • [protected-path] .github/workflows/reusable-*.yml — Seven workflow files under .github/ are modified. The PR links to issues Add concurrency group to dispatch-review job in fullsend.yaml #981, Add concurrency group to dispatch-code job in fullsend.yaml #982, Enable cancel-in-progress for review dispatches on the same PR #1357 and explains the rationale for the changes. Human approval is always required for protected-path changes, regardless of context.

  • [architectural-deviation] internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml — The diff removes the dispatch-level concurrency block from the per-repo shim entirely. The PR body describes this as intentional, with ADR 0033 stating "Concurrency groups are set at the per-stage job level inside reusable-dispatch.yml, matching the per-org behavior." The two-layer defense-in-depth model (dispatch stage jobs + reusable workflow level) replaces the old monolithic shim group. Worth confirming this is the intended evolution of the concurrency architecture.

Low

  • [edge-case] .github/workflows/reusable-dispatch.yml:389 — The review stage concurrency group uses github.event.pull_request.number || github.event.issue.number. When review is triggered by issues/labeled, the key uses the issue number; when triggered by pull_request_target, it uses the PR number. For a linked issue/PR pair (e.g., issue Add some harness engineering posts to the backlog #10 linking to PR Add performance and load impact verification problem document #15), these are different numbers, so the two triggers will not cancel each other. Two concurrent review runs are possible for the same logical work item.

  • [YAML-comment-style] .github/workflows/reusable-code.yml:1 — The updated header comment in reusable-code.yml and reusable-fix.yml uses "dispatch workflows" while the other four reusable workflows (triage, review, retro, prioritize) still say "thin callers in .fullsend repos". Minor comment inconsistency across the six files.

  • [behavioral-change] .github/workflows/reusable-code.yml — Adding workflow-level concurrency with cancel-in-progress: true to reusable workflows changes execution behavior for all per-repo callers immediately via the @v0 tag. Per-repo installations currently have no concurrency control at the stage level. Per-org callers already have this via thin callers and are unaffected.

  • [shim-template-drift] internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml — Existing per-repo installations keep their old shim (with dispatch-level cancel-in-progress: false) until re-scaffolded. They will have both old dispatch concurrency and new stage-level concurrency from updated reusable workflows — a transitional three-layer model. The old shim queuing is strictly more conservative, not harmful.

  • [YAML-multiline-style] .github/workflows/reusable-fix.yml:11 — The concurrency group uses >- (folded scalar) for a multi-line expression, while other stages use single-line group definitions. The fix workflow's more complex fallback logic justifies this, but the style differs.

  • [stale-reference] .github/workflows/fullsend.yaml:39 — The per-org mode shim still uses the monolithic fullsend-dispatch- concurrency group with cancel-in-progress: false. The per-org thin callers already provide per-stage concurrency downstream, so this is additive rather than harmful, but serializes unrelated roles unnecessarily.

  • [test-comment-coverage] internal/scaffold/scaffold_test.go:148 — Test comment references Concurrency group cancels code dispatch when triage applies multiple labels #2452 (which the PR body says it also addresses). A cross-reference to the primary issue Add concurrency group to dispatch-review job in fullsend.yaml #981 would improve traceability.


Labels: PR fixes dispatch concurrency groups across workflow files and scaffold templates.

Previous run (3)

Review

Findings

Medium

  • [protected-path] .github/workflows/reusable-*.yml, internal/scaffold/ — Seven workflow files under .github/ and scaffold templates are modified. The PR links to issues Add concurrency group to dispatch-review job in fullsend.yaml #981, Add concurrency group to dispatch-code job in fullsend.yaml #982, Enable cancel-in-progress for review dispatches on the same PR #1357 and explains the rationale for the concurrency changes. Human approval is always required for protected-path changes, regardless of context.

  • [architectural-deviation] internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml — The PR removes the dispatch-level monolithic concurrency group from the per-repo shim template. ADR 0034 previously documented "Enrolled repos gain a single concurrency group." The PR updates ADR 0034 to reflect the new per-role model, keeping code and documentation in sync within the same PR. However, the scope of the ADR Consequences edit raises the question in the next finding.

  • [scope-authorization] docs/ADRs/0034-centralized-shim-routing-via-dispatch.md — The PR replaces 6 lines of the Consequences section describing a single concurrency group with 9 lines describing per-role concurrency groups. Per AGENTS.md, accepted ADRs are point-in-time records; substantial rewrites to Consequences sections are prohibited. This change goes beyond minor annotations (cross-references, short notes, typo fixes). Consider writing a new ADR that supersedes ADR 0034's concurrency model, with ADR 0034 receiving only a short cross-reference annotation.

Low

  • [edge-case] .github/workflows/reusable-dispatch.yml:410 — The review stage concurrency group uses github.event.pull_request.number || github.event.issue.number. When review is triggered by issues/labeled (ready-for-review), the key uses the issue number; when triggered by pull_request_target, it uses the PR number. For a linked issue/PR pair (e.g., issue Add some harness engineering posts to the backlog #10 linking to PR Add performance and load impact verification problem document #15), these are different numbers, so the two triggers produce different concurrency keys and will not cancel each other. Two concurrent review runs are possible for the same logical work item.

  • [behavioral-change] .github/workflows/reusable-code.yml — Adding workflow-level concurrency with cancel-in-progress: true to all reusable workflows changes execution behavior for all per-repo callers immediately via the @v0 tag. This is the intended design (defense-in-depth), but existing per-repo installations gain stage-level concurrency upon merge without any action from those repos.

  • [edge-case] internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml — Existing per-repo installations keep their old shim (with dispatch-level cancel-in-progress: false) until re-scaffolded. They will have both old dispatch concurrency and new stage-level concurrency — a transitional state that is safe (old queuing is more conservative) but suboptimal.

  • [scope-authorization] docs/ADRs/0041-synchronous-workflow-call-event-dispatch.md — The PR replaces a forward-looking consequence ("may need concurrency review") with a resolution statement. This is close to the "short note linking to newer decisions" exception that AGENTS.md permits, but rewrites the bullet rather than annotating it.

Previous run (4)

Review

Findings

Medium

  • [protected-path] .github/workflows/reusable-*.yml — Seven workflow files under .github/ are modified. The PR links to issues Add concurrency group to dispatch-review job in fullsend.yaml #981, Add concurrency group to dispatch-code job in fullsend.yaml #982, Enable cancel-in-progress for review dispatches on the same PR #1357 and explains the rationale for the changes. Human approval is always required for protected-path changes, regardless of context.

  • [architectural-deviation] internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml — The diff removes the dispatch-level concurrency block from the per-repo shim entirely. The PR body describes this as intentional, with ADR 0033 stating "Concurrency groups are set at the per-stage job level inside reusable-dispatch.yml, matching the per-org behavior." The two-layer defense-in-depth model (dispatch stage jobs + reusable workflow level) replaces the old monolithic shim group. Worth confirming this is the intended evolution of the concurrency architecture.

Low

  • [edge-case] .github/workflows/reusable-dispatch.yml:389 — The review stage concurrency group uses github.event.pull_request.number || github.event.issue.number. When review is triggered by issues/labeled, the key uses the issue number; when triggered by pull_request_target, it uses the PR number. For a linked issue/PR pair (e.g., issue Add some harness engineering posts to the backlog #10 linking to PR Add performance and load impact verification problem document #15), these are different numbers, so the two triggers will not cancel each other. Two concurrent review runs are possible for the same logical work item.

  • [YAML-comment-style] .github/workflows/reusable-code.yml:1 — The updated header comment in reusable-code.yml and reusable-fix.yml uses "dispatch workflows" while the other four reusable workflows (triage, review, retro, prioritize) still say "thin callers in .fullsend repos". Minor comment inconsistency across the six files.

  • [behavioral-change] .github/workflows/reusable-code.yml — Adding workflow-level concurrency with cancel-in-progress: true to reusable workflows changes execution behavior for all per-repo callers immediately via the @v0 tag. Per-repo installations currently have no concurrency control at the stage level. Per-org callers already have this via thin callers and are unaffected.

  • [shim-template-drift] internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml — Existing per-repo installations keep their old shim (with dispatch-level cancel-in-progress: false) until re-scaffolded. They will have both old dispatch concurrency and new stage-level concurrency from updated reusable workflows — a transitional three-layer model. The old shim queuing is strictly more conservative, not harmful.

  • [YAML-multiline-style] .github/workflows/reusable-fix.yml:11 — The concurrency group uses >- (folded scalar) for a multi-line expression, while other stages use single-line group definitions. The fix workflow's more complex fallback logic justifies this, but the style differs.

  • [stale-reference] .github/workflows/fullsend.yaml:39 — The per-org mode shim still uses the monolithic fullsend-dispatch- concurrency group with cancel-in-progress: false. The per-org thin callers already provide per-stage concurrency downstream, so this is additive rather than harmful, but serializes unrelated roles unnecessarily.

  • [test-comment-coverage] internal/scaffold/scaffold_test.go:148 — Test comment references Concurrency group cancels code dispatch when triage applies multiple labels #2452 (which the PR body says it also addresses). A cross-reference to the primary issue Add concurrency group to dispatch-review job in fullsend.yaml #981 would improve traceability.


Labels: PR fixes dispatch concurrency groups across workflow files and scaffold templates.

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment component/dispatch Workflow dispatch and triggers type/bug Confirmed defect in existing behavior labels Jun 21, 2026
@ifireball ifireball self-assigned this Jun 21, 2026
ifireball and others added 2 commits June 21, 2026 12:52
Signed-off-by: Barak Korren <bkorren@redhat.com>
Align ADR 0034/0041 consequences with per-role cancel-in-progress groups
introduced for per-repo dispatch (fullsend-ai#981). Fixes gofmt CI failure.

Signed-off-by: Barak Korren <bkorren@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 21, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 9:56 AM UTC · Ended 10:11 AM UTC
Commit: 4e21a60 · View workflow run →

@codecov

codecov Bot commented Jun 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@ifireball

Copy link
Copy Markdown
Member Author

Babysit update

  • CI test: fixed (gofmt) — now green
  • Merged latest main (141 commits)
  • ADR docs: updated ADR 0034/0041 concurrency consequences to match per-role policy (addresses qodo review thread)
  • E2E: still failing — TestAdminInstallUninstall triage workflow concludes failure in ephemeral org (empty logs in artifact); TestVendorFromSubdirectory hit 422 Update is not a fast forward (org pool race). Re-run also failed. Main branch e2e was green earlier today — treating as likely infra flake unless triage logs show a regression from concurrency YAML.

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 21, 2026
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:56 AM UTC · Completed 10:11 AM UTC
Commit: 346776d · View workflow run →

ci: retrigger e2e after babysit fixes
Co-authored-by: Cursor <cursoragent@cursor.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 21, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 10:16 AM UTC · Ended 10:22 AM UTC
Commit: 4e21a60 · View workflow run →

…ncel

Per-role groups on workflow_call parents (thin callers and reusable-dispatch
stage jobs) share keys with reusable stage workflows. Duplicate groups with
cancel-in-progress cancel the parent immediately, breaking e2e triage (fullsend-ai#981).

Signed-off-by: Barak Korren <bkorren@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 21, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:25 AM UTC · Completed 10:40 AM UTC
Commit: 6cc890f · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

See the review comment for full details.

independently: a new dispatch now cancels any in-progress run for the
same issue/PR. In practice, only one agent should run per issue/PR at a
time, and the latest event takes priority.
- Enrolled repos gain per-role concurrency groups with `cancel-in-progress: true`

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[medium] scope-authorization

The PR replaces 6 lines of ADR 0034's Consequences section (describing a single concurrency group) with 9 lines describing per-role concurrency groups. Per AGENTS.md, accepted ADRs are point-in-time records; substantial rewrites to Consequences sections are prohibited. Consider writing a new ADR that supersedes ADR 0034's concurrency model.

Suggested fix: Write a new ADR documenting the per-role concurrency model and add a short cross-reference annotation to ADR 0034 instead of rewriting it.

@@ -398,6 +410,9 @@ jobs:
name: Review
needs: route
if: needs.route.outputs.stage == 'review'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] edge-case

The review stage concurrency group uses github.event.pull_request.number || github.event.issue.number. When triggered by issues/labeled vs pull_request_target, different concurrency keys are produced for linked issue/PR pairs with different numbers. Two concurrent review runs are possible for the same logical work item.

`dispatch.yml` directly; the single-file marker pattern from ADR 0026 is gone.
- Per-org and per-repo dispatch shapes converge; enrolled-repo shims may need
`needs:` / concurrency review ([#504](https://github.com/fullsend-ai/fullsend/issues/504)).
- Per-org and per-repo dispatch shapes converge; per-role concurrency is

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] scope-authorization

The PR replaces a forward-looking consequence with a resolution statement. While close to the annotation exception AGENTS.md permits, it rewrites the original text rather than appending a note.

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 21, 2026
Restore workflow-level cancel-in-progress on reusable-{stage}.yml using
distinct fullsend-{stage}-agent- group keys so workflow_call parents keep
their own dispatch groups. Revert ADR body edits; add short implementation
notes only (fullsend-ai#981).

Signed-off-by: Barak Korren <bkorren@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 21, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:53 AM UTC · Completed 11:06 AM UTC
Commit: c0ffc02 · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

See the review comment for full details.

name: Review
needs: route
if: needs.route.outputs.stage == 'review'
concurrency:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] edge-case

The review stage concurrency group uses github.event.pull_request.number || github.event.issue.number. When review is triggered by issues/labeled (ready-for-review), the key uses the issue number; when triggered by pull_request_target, it uses the PR number. For a linked issue/PR pair where the numbers differ, the two triggers produce different concurrency keys and will not cancel each other. Two concurrent review runs are possible for the same logical work item. The same concern applies to fix and retro stages.

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/dispatch Workflow dispatch and triggers requires-manual-review Review requires human judgment type/bug Confirmed defect in existing behavior

Projects

None yet

2 participants