Add dispatching to support easy adding/replacement of agents - #390
Conversation
Site previewPreview: https://04cbc032-site.fullsend-ai.workers.dev Commit: |
There was a problem hiding this comment.
Review: #390
Head SHA: ef268d9
Timestamp: 2026-04-24T00:00:00Z
Outcome: approve
Summary
This PR correctly implements a dispatch-based architecture that decouples the shim workflow from individual agent workflows. The shim now routes all stages through dispatch-agent.yml, which emits repository_dispatch events consumed by the existing agent workflows. The ternary-like &&/|| expression pattern used to select between repository_dispatch and workflow_dispatch inputs is valid GitHub Actions syntax and behaves correctly for both trigger types. Security properties are preserved: the dispatcher uses a GitHub App token (not a PAT) for repository_dispatch calls, the stage input is constrained to a choice type, and agent workflows retain their three-layer input validation. Tests are updated appropriately.
Findings
Low
-
[correctness]
dispatch-agent.yml— The dispatcher forwardssource_repowithout validating its format before emittingrepository_dispatch. While the downstream agent workflows all validatesource_repo(format check, owner check, allowlist check), validating in the dispatcher would short-circuit invalid requests earlier and reduce unnecessaryrepository_dispatchevents.
Remediation: Consider adding the samesource_repoformat regex check (^[a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+$) in the dispatcher before emitting the event. -
[style/conventions]
code.yml,review.yml,triage.yml— The concurrency group expressions duplicate the full ternary pattern (e.g.,fromJSON(github.event_name == 'repository_dispatch' && github.event.client_payload.event_payload || inputs.event_payload).issue.number). This is unavoidable since workflow-levelconcurrencycannot reference job-levelenvvars, but the resulting expressions are long and harder to read, particularly inreview.ymlwhere the expression appears twice with a||fallback.
Remediation: No action required — this is a GitHub Actions limitation. A comment explaining the pattern would aid future maintainers.
Footer
Outcome: approve
This review applies to SHA ef268d913e0510d4a4cf55139234e863e5ea2dd8. Any push to the PR head clears this review and requires a new evaluation.
|
When you're ready for review, take this out of draft. |
|
I think this is a great feature. |
ef268d9 to
7b74862
Compare
There was a problem hiding this comment.
Review: #390
Head SHA: 7b74862
Timestamp: 2026-04-28T00:00:00Z
Outcome: approve
Summary
This PR cleanly implements the dispatch-based architecture requested in #335. The shim now routes all stages through a single dispatch-agent.yml dispatcher, which emits repository_dispatch events that agent workflows (triage, code, review) listen for alongside the existing workflow_dispatch trigger. The credential separation is well-documented and correctly implemented: the PAT stays on enrolled repos for workflow_dispatch, while the orchestrator app token is used only for the repository_dispatch POST. Input escaping via jq --arg prevents JSON injection. The concurrency, validation, and token-scoping patterns are all sound. No critical or high findings.
Findings
Medium
- [Platform security]
dispatch-agent.yml:49-68— The dispatcher forwardssource_repoto therepository_dispatchclient_payload without any validation (format check, org-owner check, enrollment allowlist). While downstream agent workflows all perform these checks before acting onsource_repo, adding validation here would follow the defense-in-depth principle — catching invalid inputs before generating an orchestrator app token and emitting a dispatch event for a repo that will be immediately rejected.
Remediation: Consider replicating the format/owner/allowlist validation from the agent workflows (or extracting it into a shared script) and running it before theEmit repository_dispatchstep.
Low
- [Style/conventions]
code.yml:17,review.yml:17,triage.yml:17— The concurrency group expressions using inline ternary logic (github.event_name == 'repository_dispatch' && github.event.client_payload.event_payload || inputs.event_payload) are dense and hard to scan. This is a GitHub Actions limitation (concurrency groups can't reference step outputs or job-level env vars), so there's no clean alternative. Consider adding a YAML comment above each concurrency block explaining the expression's purpose for future maintainers.
Remediation: Add a comment like# Selects event_payload from repository_dispatch client_payload or workflow_dispatch inputs.
Info
-
[Correctness]
scaffold_test.go— Tests are updated to cover the newdispatch-agent.ymlfile, the shim's use of it, and therepository_dispatchtrigger in triage.yml. The existingTestFullsendRepoFilesExistlist now includes the new file, andTestShimTemplateContentasserts the new routing. Test coverage is adequate for the change scope. -
[Intent alignment] — The PR scope matches #335's request: shim → dispatcher → repository_dispatch → agent workflows. Backward compatibility with direct
workflow_dispatchis preserved. No scope creep detected. -
[Injection defense] — PR body and commit messages contain no non-rendering Unicode, bidirectional overrides, or prompt injection patterns. The
jq -n --argusage indispatch-agent.ymlcorrectly escapes all user-controlled values as JSON strings before posting to the API.
Footer
Outcome: approve
This review applies to SHA 7b7486206929be0e3eda1a53021b8e7c5c2cf91f. Any push to the PR head clears this review and requires a new evaluation.
There was a problem hiding this comment.
Review: #390
Head SHA: 4027cdd
Timestamp: 2026-04-28T00:00:00Z
Outcome: comment-only
Summary
This PR switches the shim-to-agent dispatch mechanism from gh workflow run (workflow_dispatch) to repository_dispatch via gh api repos/.../dispatches, decoupling the shim from specific workflow filenames and enabling easier agent addition or replacement. The change is well-structured: agent workflows gain a repository_dispatch trigger alongside the existing workflow_dispatch, job-level env vars normalize input access across both trigger types, and the shim constructs properly-typed dispatch payloads using jq. Tests are updated accordingly. No critical or high findings; the change is functionally correct and preserves the existing security controls (source_repo validation, owner check, allowlist). A few readability and robustness observations are noted below.
Findings
Medium
- [Style/conventions]
review.ymlconcurrency group — The concurrency group key duplicates the full ternary expression twice on a single line (~250 chars), making it very difficult to read and maintain. Consider extracting this into a reusable pattern or a workflow-level env var evaluated before the concurrency block (though note GitHub Actions evaluates concurrency before job env vars, so this may require a different approach like a YAML anchor or accepting the duplication with a clarifying comment).
Low
-
[Correctness]
shim-workflow.yamlcomment header — The old comment said "Routes events to per-role agent dispatch workflows in .fullsend." The new comment says "Routes events to agent workflows in .fullsend via repository_dispatch" but omits thatworkflow_dispatchis preserved as a fallback trigger in the agent workflows. A reader of only the shim may not realize the dual-trigger design. -
[Correctness] Robustness of
fromJSON()in concurrency groups — If arepository_dispatchevent arrives with a missing or malformedclient_payload.event_payload, the ternary falls through toinputs.event_payload(also empty for dispatch events), andfromJSON('')will cause a workflow evaluation error. This is a reasonable failure mode (malformed payloads should fail), but it may produce a confusing error message. A comment noting this intentional fail-fast behavior would help future maintainers.
Info
-
[Style/conventions] Step names were changed from "Dispatch triage" → "Dispatch triage stage", etc. This is cosmetic and consistent across all three dispatch jobs.
-
[Correctness] The
client_payloadincludes both the dispatch event type (event_type: "fullsend-triage") and the original GitHub event type (event_typeinsideclient_payload). The field name collision between the top-levelevent_type(used by GitHub's dispatch type filter) andclient_payload.event_type(the original event name) is not a bug — GitHub resolves them from different namespaces — but could confuse readers. A rename likeoriginal_event_typein the client_payload would improve clarity.
Footer
Outcome: comment-only
This review applies to SHA 4027cdd7bddc296bcc3602ed4602c1559313e8e7. Any push to the PR head clears this review and requires a new evaluation.
f796c89 to
a44dd3d
Compare
There was a problem hiding this comment.
Review: #390
Head SHA: a44dd3d
Timestamp: 2026-04-28T00:00:00Z
Outcome: request-changes
Summary
This PR introduces a dispatcher workflow (dispatch.yml) that decouples the shim from individual agent workflows by routing events based on # fullsend-stage: markers. The architectural change is sound — it enables adding or replacing agent workflows without modifying the shim template. However, the PR branch is based on a stale merge base: the GCP auth conditionals were already updated on main from secrets.FULLSEND_GCP_WIF_PROVIDER to vars.FULLSEND_GCP_AUTH_MODE == 'wif', but this PR changes them to the non-existent vars.FULLSEND_USE_WIF == 'true'. If merged, this would silently regress the auth condition across all three agent workflows (code, review, triage), potentially breaking GCP authentication.
Findings
High
- [Correctness]
internal/scaffold/fullsend-repo/.github/workflows/code.yml,review.yml,triage.yml— Stale branch causes GCP auth condition regression. The PR changesif: secrets.FULLSEND_GCP_WIF_PROVIDER != ''toif: vars.FULLSEND_USE_WIF == 'true', but currentmainalready usesvars.FULLSEND_GCP_AUTH_MODE == 'wif'. Merging this PR would replace the correctFULLSEND_GCP_AUTH_MODEvariable with the incorrectFULLSEND_USE_WIFvariable, breaking GCP authentication for any org that has configuredFULLSEND_GCP_AUTH_MODE.
Remediation: Rebase onto currentmainand usevars.FULLSEND_GCP_AUTH_MODE == 'wif'(and!= 'wif') consistently, matching the existing convention.
Medium
-
[Correctness]
internal/scaffold/fullsend-repo/.github/workflows/code.yml:102-106— NewGCP_OIDC_TOKEN_FILEworkaround step creates an empty file at/tmp/empty-oidc-tokenwith no explanatory comment. It's unclear what downstream component requires this env var and what happens when it points to an empty file. If this is needed, it should also be present onmainfor the non-dispatched path; if not, it may be dead code after rebase.
Remediation: Add a comment explaining which component readsGCP_OIDC_TOKEN_FILEand why an empty file is an acceptable fallback. Verify this step is still needed after rebasing onto currentmain. -
[Correctness]
internal/scaffold/fullsend-repo/.github/workflows/code.yml,review.yml,triage.yml— TheDISPATCH_SOURCE_REPOandDISPATCH_EVENT_PAYLOADenv var indirection (job-level env set frominputs.*, then referenced asenv.DISPATCH_*) is functionally equivalent to usinginputs.*directly. The motivation isn't documented. If this is preparation for a future change where dispatch provides these values through a different mechanism, a code comment would clarify intent. Otherwise it adds indirection without benefit.
Remediation: Add a brief comment explaining why the indirection exists, or simplify by usinginputs.*directly.
Low
- [Platform security]
internal/forge/github/types.go:7,67— Addingactions: "write"to the fullsend app permissions is necessary for the dispatcher to trigger workflows viagh workflow run. This is a correct and justified permission expansion. Note that this means the fullsend app token can now create, cancel, and re-run any workflow in repositories it's installed on — ensure this aligns with the principle of least privilege for the orchestrator role.
Info
-
[Style]
internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml— The dispatcher's grep-based stage matching (grep -E '^# fullsend-stage:') is simple and effective. Consider documenting the# fullsend-stage:convention in a comment withindispatch.ymlor in the repo docs so future contributors know how to register new agent workflows. -
[Intent alignment] No linked issue. The PR title describes the intent clearly, and the changes are appropriately scoped to the dispatching concern. The
appsetup.goURL extraction refactor is a minor cleanup bundled in — acceptable but unrelated.
Footer
Outcome: request-changes
This review applies to SHA a44dd3db51e794ad81f2f3429cb77298749682dd. Any push to the PR head clears this review and requires a new evaluation.
a44dd3d to
6d9f91f
Compare
There was a problem hiding this comment.
Review: #390
Head SHA: 6d9f91f
Timestamp: 2026-04-28T00:00:00Z
Outcome: approve
Summary
This PR introduces a central dispatch layer (dispatch.yml) that replaces direct workflow-to-workflow routing with a stage-based fan-out pattern. The shim workflow now targets a single dispatch.yml entry point with a stage parameter, and the dispatcher scans for # fullsend-stage: markers in workflow files to determine which workflows to trigger. Agent workflows (code.yml, review.yml, triage.yml) are updated to use job-level env vars for source_repo and event_payload, enabling them to be triggered either directly or via the dispatcher. The actions: write permission is correctly added to the fullsend app to support gh workflow run. All changes are well-structured, security-sensitive patterns (env-var-based input passing, source_repo validation in downstream workflows, script injection prevention) are preserved, and tests are updated consistently. No critical or high findings.
Findings
Critical
None.
High
None.
Medium
None.
Low
- [correctness]
internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml:46— Thestageinput is an unconstrained string with no validation against a known set (triage, code, review). An unrecognized stage silently results in zero dispatches. Consider adding a validation step or usingchoicetype to catch typos early and improve observability.
Remediation: Add a validation step before the scan loop:if [[ ! "$STAGE" =~ ^(triage|code|review)$ ]]; then echo "::error::Unknown stage: $STAGE"; exit 1; fi, or change the input type tochoice.
Info
-
[style]
internal/scaffold/fullsend-repo/.github/workflows/code.yml:120-122,review.yml:111-113,triage.yml:90-92— TheGCP_OIDC_TOKEN_FILEworkaround (creating an empty file for non-WIF auth) lacks a comment explaining what downstream component requires this variable and why an empty file is acceptable. Future maintainers may not understand the purpose.
Remediation: Add a brief comment explaining which tool or SDK readsGCP_OIDC_TOKEN_FILEand why an empty sentinel works. -
[style]
internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml— The dispatcher does not validatesource_repoformat, unlike all downstream workflows. This is safe because validation happens at the point of use, but the asymmetry could be confusing. A comment noting that validation is deferred to downstream workflows would help.
Footer
Outcome: approve
This review applies to SHA 6d9f91ffd4c2fe7960b2f93a7e9b3d265959cf6a. Any push to the PR head clears this review and requires a new evaluation.
Security Audit Report: Dispatch Workflow Architecture ChangesDate: 2026-04-28 Executive SummaryOverall Security Posture: MAINTAINED with MINOR IMPROVEMENTS The new dispatch workflow architecture maintains the existing security model while introducing a centralized dispatch pattern. One new permission ( Architecture ChangesBeforeAfterSecurity Analysis by Dimension1. Credential Distribution & Blast RadiusStatus: NO CHANGE (remains secure) Before:
After:
Analysis: 2. Permission BoundariesStatus: MINOR EXPANSION (justified and scoped) New Permission Added:
Before: After: Justification: Risk Assessment:
3. Attack Surface & Injection VulnerabilitiesStatus: NO REGRESSION (existing protections maintained) Injection Protection (MAINTAINED):
Conclusion: 4. Token Exposure & LifetimeStatus: IMPROVED FULLSEND_DISPATCH_TOKEN (in target repos):
GitHub App Tokens (in .fullsend):
Analysis: 5. Workflow Triggering ControlsStatus: EQUIVALENT with IMPROVED MAINTAINABILITY Authorization Check (BOTH versions): # Shim validates:
if: >-
github.event_name == 'issue_comment' && (
github.event.comment.body == '/triage' ||
startsWith(github.event.comment.body, '/triage ') ||
(permissions check for auto-triage)
)✅ Same authorization logic in both versions Agent Workflow Triggering:
Security Implication:
6. Cross-Repository SecurityStatus: MAINTAINED Repository Isolation:
Validation Steps (present in BOTH versions): # 1. Format check (prevents path traversal, injection)
if [[ ! "$SOURCE_REPO" =~ ^[a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+$ ]]; then
exit 1
fi
# 2. Owner check (prevents cross-org access)
REPO_OWNER="${SOURCE_REPO%%/*}"
if [[ "$REPO_OWNER" != "$GITHUB_REPOSITORY_OWNER" ]]; then
exit 1
fi
# 3. Allowlist check (prevents unauthorized repo access)
ENABLED=$(yq ".repos.\"$REPO_NAME\".enabled" config.yaml 2>/dev/null)
if [[ "$ENABLED" != "true" ]]; then
exit 1
fi✅ Three-layer defense maintained 7. Input ValidationStatus: MAINTAINED Shim → dispatch.yml: inputs:
stage: (string, required)
event_type: (string, required)
source_repo: (string, required)
event_payload: (string, required)✅ All inputs typed and required dispatch.yml → agent workflows: -f event_type="$EVENT_TYPE"
-f source_repo="$SOURCE_REPO"
-f event_payload="$EVENT_PAYLOAD"✅ Variables properly quoted 8. Code Execution PathsStatus: ONE NEW EXECUTION PATH (secure) New Execution Path: Script Analysis (dispatch.yml lines 48-69): for workflow in .github/workflows/*.yml .github/workflows/*.yaml; do
[[ -f "$workflow" ]] || continue # Safe: checks file existence
workflow_stage=$(grep -E '^# fullsend-stage:' "$workflow" | head -1 | cut -d: -f2 | tr -d ' ')
# ^ Safe: regex anchored to line start, simple string extraction
[[ -z "$workflow_stage" ]] && continue # Safe: empty check
[[ "$workflow_stage" != "$STAGE" ]] && continue # Safe: string comparison
workflow_name=$(basename "$workflow") # Safe: basename prevents path traversal
gh workflow run "$workflow_name" \
--repo "$GITHUB_REPOSITORY" \ # Safe: GitHub-provided variable
-f event_type="$EVENT_TYPE" \ # Safe: quoted, from workflow input
-f source_repo="$SOURCE_REPO" \ # Safe: quoted, validated downstream
-f event_payload="$EVENT_PAYLOAD" # Safe: quoted, opaque data
doneSecurity Properties: New Attack Scenarios ConsideredScenario 1: Malicious Workflow InjectionAttack: Attacker adds malicious workflow to .fullsend with Mitigations: Risk: EQUIVALENT to old architecture Scenario 2: Stage Name SpoofingAttack: Attacker modifies shim to call Mitigations: Risk: NOT APPLICABLE (attacker would need merge access) Scenario 3: dispatch.yml CompromiseAttack: Attacker modifies dispatch.yml scanning logic Mitigations: Risk: EQUIVALENT to old architecture Scenario 4: Unauthorized Workflow Triggering via Actions PermissionAttack: Compromised fullsend app token triggers arbitrary workflows Mitigations: Risk: LOW (token is short-lived, scoped, and workflow content is protected) Findings Summary✅ No Security Regressions
|
|
@ralphbean, this is ready for review finally. I had to wrestle with Github a bit to get things to work, so we now have our own dispatch workflow. |
There was a problem hiding this comment.
Review: #390
Head SHA: 6d9f91f
Timestamp: 2026-04-28T00:00:00Z
Outcome: approve
Summary
This PR introduces a dispatch layer that decouples the shim workflow from individual agent workflows, allowing new agents to be added by simply creating a workflow file with a # fullsend-stage: marker. The architecture is sound: the dispatcher generates a scoped GitHub App token, scans only base-branch workflow files (preventing PR-based manipulation of stage markers), and correctly avoids self-triggering. The Actions: "write" permission is appropriately scoped to the fullsend orchestrator role only. Tests are updated consistently across all affected assertions. No critical, high, or blocking findings.
Findings
Medium
- [Correctness]
internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml:52-67— The dispatch loop has no error handling. Ifgh workflow runfails for one workflow (e.g., rate limit, transient API error), the loop silently continues and subsequent workflows may still fire. A partial dispatch (some agents triggered, others not) could be difficult to diagnose in production.
Remediation: Addset -euo pipefailat the top of the run block, or capture the exit code of eachgh workflow runcall and emit a::warningor::errorannotation on failure.
Low
- [Correctness]
internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml:56— Stage marker parsing (cut -d: -f2 | tr -d ' ') is simple but fragile — a stage name containing a colon would be silently truncated. Current stage names (triage, code, review) are safe, but this could surprise future contributors adding new stages.
Remediation: Consider using a regex capture group instead (e.g.,sed -n 's/^# fullsend-stage: *\(.*\)/\1/p'), or document the constraint that stage names must not contain colons.
Info
- [Style/conventions]
internal/scaffold/fullsend-repo/.github/workflows/code.yml:25-27— TheDISPATCH_SOURCE_REPO/DISPATCH_EVENT_PAYLOADenv var indirection replaces directinputs.*references. Functionally equivalent, but creates a consistent pattern for workflows that may be triggered either directly or via the dispatcher. This is a reasonable convention worth documenting for future contributors.
Footer
Outcome: approve
This review applies to SHA 6d9f91ffd4c2fe7960b2f93a7e9b3d265959cf6a. Any push to the PR head clears this review and requires a new evaluation.
ralphbean
left a comment
There was a problem hiding this comment.
Review: #390
The dispatch architecture is a solid improvement — decoupling the shim from individual workflow filenames eliminates a maintenance burden when adding/replacing agents. The # fullsend-stage: convention is lightweight and auditable. Security properties (credential scoping, input validation, pull_request_target protections) are preserved.
One minor change requested: the dispatch job should declare explicit permissions to match the least-privilege convention used in all sibling workflows.
Also noting (but not blocking on): the dispatch loop silently succeeds when zero workflows match a stage, which could mask typos or missing markers in production. Consider adding a counter and failing on zero matches in a follow-up.
Address feedback from PR fullsend-ai#390 review: - Add explicit permissions block (actions: write, contents: read, id-token: write) to dispatch job as requested by ralphbean - Add error handling with set -euo pipefail - Track dispatched workflow count and fail with clear error when zero workflows match a stage - Improve stage marker parsing using sed for more robust extraction - Update tests to verify new permissions and error handling features - Add actions: write permission to fullsend app in test fixture Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
waynesun09
left a comment
There was a problem hiding this comment.
Approved — all previous findings resolved
Re-reviewed with 4 agents (security, quality, cursor, gemini). All 12 findings from rounds 1 and 2 are confirmed resolved. The dispatch architecture is solid: proper input validation, self-dispatch guard, permissions scoping, is_bot_user() helper extraction, and stage-gated trigger_source forwarding.
One small recommendation for a follow-up
TRIGGER_USER null guard in shim — The dispatch-fix-bot and dispatch-fix-human jobs set TRIGGER_USER from event context (github.event.review.user.login / github.event.comment.user.login) without checking for null. If the field were ever missing, an empty trigger_source would pass dispatch.yml (optional input), trigger fix.yml, and only fail at pre-fix.sh — wasting a workflow run. A simple guard before the gh workflow run call would fail fast:
if [[ -z "$TRIGGER_USER" ]]; then
echo "::error::Could not determine trigger user from event"
exit 1
fiLow risk since GitHub always populates these fields for pull_request_review and issue_comment events — this is pure defense-in-depth. Fine to defer to a follow-up.
Rename file and title from "repository_dispatch" to "stage-based dispatch" to match the actual implementation (Option C uses workflow_dispatch with stage-marker scanning, not repository_dispatch). Mark status as Accepted now that PR fullsend-ai#390 has merged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@/tmp/pr-body.md