feat(dispatch): synchronous workflow_call event dispatch (ADR 41)#1611
feat(dispatch): synchronous workflow_call event dispatch (ADR 41)#1611ifireball wants to merge 3 commits into
Conversation
Site previewPreview: https://40a32e4d-site.fullsend-ai.workers.dev Commit: |
ReviewFindingsHigh
Low
Info
Previous runReviewFindingsMedium
Low
Info
Previous run (2)ReviewFindingsMedium
Low
Info
Previous run (3)ReviewFindingsMedium
Low
Info
Previous run (4)ReviewFindingsMedium
Low
Info
Previous run (5)ReviewFindingsHigh
Medium
Low
Info
Previous run (6)ReviewFindingsMedium
Low
Info
Previous run (7)ReviewFindingsMedium
Low
Info
Previous run (8)ReviewFindingsHigh
Medium
Low
Info
Previous run (9)ReviewFindingsMedium
Low
Info
Previous run (10)ReviewFindingsMedium
Low
Info
Previous run (11)ReviewFindingsMedium
Info
Previous run (12)ReviewFindingsMedium
Low
Info
Previous run (13)ReviewFindingsMedium
Low
|
Update stale references to thin callers and per-org OIDC minting in dispatch.yml; address review feedback on PR fullsend-ai#1611. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
HIGH — Missing
code) STAGE_ROLE="coder" ;;Compare with code|fix) STAGE_ROLE="coder" ;;When a (This line isn't in the PR diff — it's existing code that was carried forward without the fix that HIGH — Same ternary kill-switch bug exists in stage: ${{ steps.role-check.outputs.skipped == 'true' && '' || steps.route.outputs.stage }}Same Suggestion: stage: ${{ steps.role-check.outputs.skipped != 'true' && steps.route.outputs.stage || '' }} |
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad — 10-agent deep-dive on mint/OIDC impact
1 CRITICAL, 3 HIGH, 3 MEDIUM findings (see inline comments + PR comment for items on unchanged lines).
The central issue: permissions: {} at workflow level in the per-org dispatch.yml scaffold blocks id-token: write from propagating to stage jobs, which breaks OIDC token minting in all reusable workflows. The old flow worked because thin callers were independent workflow runs with their own permission grants — this PR inlines them as jobs under dispatch.yml's permissions: {} umbrella.
Additional issues: ternary expression bug defeats the role-check kill switch (both per-org and per-repo), fix role mapping is missing in per-org, and secrets: inherit on prioritize is inconsistent with other stages.
Update stale references to thin callers and per-org OIDC minting in dispatch.yml; address review feedback on PR fullsend-ai#1611. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> (cherry picked from commit 21e5e0f)
034b8cb to
3df8b3d
Compare
|
Addressed review feedback from @waynesun09:
Commit: |
|
Rebased onto
Resolved the review threads for permissions, ternary, e2e failed again on org-pool lock acquisition ( @waynesun09 please take another look when you have a moment. |
waynesun09
left a comment
There was a problem hiding this comment.
All CRITICAL/HIGH findings from the prior review are confirmed fixed in 1f26c872. LGTM — one minor doc cleanup remaining (see inline comment).
|
[MEDIUM] Stale workflow references in
The PR correctly removed Suggestion: Update both lines to query |
1f26c87 to
1a31cc2
Compare
|
Follow-up on the latest review comments (@waynesun09):
Rebased onto Deferred without code change (mitigations documented in threads):
|
1a31cc2 to
21175c7
Compare
21175c7 to
70b0280
Compare
ralphbean
left a comment
There was a problem hiding this comment.
A couple things to fix before merge. See inline comments.
|
@ralphbean — addressed your inline comments in
e2e / Other checks green on this push. |
70b0280 to
f7bf57e
Compare
Replace per-org dispatch marker scan and gh workflow run with static workflow_call jobs to upstream reusable workflows. Remove thin stage workflows from scaffold; add workflow_call to prioritize.yml. Port per-stage concurrency from removed thin callers into reusable workflows. Update unit tests and e2e smoke test to poll dispatch.yml. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Update stale references to thin callers and per-org OIDC minting in dispatch.yml; align retro-analysis skill and layer comments with the synchronous workflow_call pattern. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
- Per-org stage jobs: explicit OIDC permissions; kill-switch ternary fix - Role mapping: code|fix → coder; prioritize workflow_call secrets - Per-repo reusable-dispatch: prioritize job via .fullsend/prioritize.yml - finding-agent-runs skill: dispatch.yml instead of removed thin workflows - reusable-fix: fail closed when gh pr view fails (bot eligibility) - pre-code-test: restore line-by-line extra_env parsing - e2e: capture issueCreatedAt before issue creation Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
f7bf57e to
bb09458
Compare
| pull-requests: read | ||
| outputs: | ||
| stage: ${{ steps.role-check.outputs.skipped == 'true' && '' || steps.route.outputs.stage }} | ||
| stage: ${{ steps.role-check.outputs.skipped != 'true' && steps.route.outputs.stage || '' }} |
There was a problem hiding this comment.
[info] correctness
Good bug fix: the old stage output expression always evaluated to stage regardless of skip status. The corrected expression properly returns empty string when the role is skipped.
ralphbean
left a comment
There was a problem hiding this comment.
A couple things to address. See inline comments.
| --json databaseId,status,conclusion,createdAt | ||
|
|
||
| gh run list --repo "${DISPATCH_REPO}" --workflow=code.yml --limit 5 \ | ||
| gh run list --repo "${DISPATCH_REPO}" --workflow=dispatch.yml --limit 5 \ |
There was a problem hiding this comment.
[important] With synchronous workflow_call, there are no separate dispatch.yml runs in .fullsend -- the whole chain (shim -> dispatch -> stage) is one run in the enrolled repo. The triage and code sections above get this right (gh run view <RUN_ID> --json jobs on the shim run), but from here down -- "Find the actual agent run", review, retro, and "Logs and artifacts" -- we're still pointing agents at ${DISPATCH_REPO} for runs that won't exist there.
The e2e test in this PR was updated to poll fullsend.yaml in the enrolled repo, so we already know this is the right model. Could we update these sections to match? Same issue in retro-analysis/SKILL.md.
| test each code path that uses the function. Different operations | ||
| (e.g., approve vs request-changes) often have different required fields. | ||
|
|
||
| When requirements are ambiguous, distinguish between "vague but actionable" |
There was a problem hiding this comment.
[moderate] This removes the "Verify API contracts per code path" checklist item, which doesn't seem related to ADR 41. Rebase artifact? It's caught real bugs before (different API operations having different required fields), so I think we should keep it.
|
FWIW - I don't want to hold this up. If you can address those things feel free to merge when you're back at keyboard. You can dismiss my review via the API. :) |
Summary
dispatch.yml→reusable-*.yml@v0(synchronousworkflow_call), replacing# fullsend-stage:scanning andgh workflow run.prioritize.ymlgainsworkflow_call.dispatch.ymlcompletion.Review fixes (from #1586)
config.yamlfromjob.workflow_repository(.fullsend), not the enrolled repo.secrets: inheriton the prioritizeworkflow_calljob.architecture.mdandretro-analysisskill for the dispatch.yml flow.Note on secrets: Stage jobs pass
${{ secrets.FULLSEND_* }}from the.fullsendrepo context (wheredispatch.ymlruns afterworkflow_callfrom the shim). Enrolled repos do not hold GCP secrets; the shim should not pass them.Migration
fullsend admin install <org>to refresh.fullsendscaffold.# fullsend-stage:workflowsworkflow_calljob indispatch.yml; remove markers.Test plan
go test ./internal/scaffold/... ./internal/layers/...Supersedes closed #1586 (fork head could not run e2e).
Made with Cursor