fix(#5188): recognize org bot actors in dispatch routing#5415
Conversation
PR Summary by QodoFix dispatch routing for org GitHub App bots in pull_request_target
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
|
🤖 Finished Review · ✅ Success · Started 1:48 PM UTC · Completed 2:07 PM UTC |
Site previewPreview: https://5d94649f-site.fullsend-ai.workers.dev Commit: |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
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
Low
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
Labels: PR modifies dispatch routing logic and adds shell script testing infrastructure Previous run (13)ReviewFindingsMedium
Low
Previous run (14)ReviewFindingsMedium
Low
Previous run (15)ReviewFindingsMedium
Low
Previous run (16)ReviewFindingsMedium
Previous run (17)ReviewFindingsHigh
Medium
Low
Labels: PR modifies dispatch routing logic and fixes a bug in bot actor recognition |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
component/dispatch
46045f3 to
989bf7a
Compare
|
🤖 Review · ❌ Terminated · Started 2:46 PM UTC · Ended 2:59 PM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Superseded by updated review
fullsend-ai-review
Bot
added
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 2:46 PM UTC · Completed 2:59 PM UTC |
989bf7a to
1f1a1d5
Compare
|
🤖 Finished Review · ✅ Success · Started 4:15 PM UTC · Completed 4:32 PM UTC |
1f1a1d5 to
dabfa50
Compare
|
🤖 Finished Review · ✅ Success · Started 4:43 PM UTC · Completed 4:58 PM UTC |
dabfa50 to
efd0d27
Compare
|
🤖 Finished Review · ✅ Success · Started 5:04 PM UTC · Completed 5:20 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
efd0d27 to
2b6bf19
Compare
|
🤖 Finished Review · ✅ Success · Started 5:46 PM UTC · Completed 6:00 PM UTC |
|
🤖 Finished Review · ✅ Success · Started 1:59 PM UTC · Completed 2:16 PM UTC |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review squad pass 4 (Claude x2 + Grok, 3 agents). All three round-3 findings confirmed fixed via direct mutation testing against the live rendered files — the ||-anchored regexes, TestReusableFixWorkflowContent/TestReviewBotIdentitiesStayInSync, and the ADR prompt-injection addendum all hold up as intended. Two new findings this round, both surfaced by mutation-testing the new drift-guard tests themselves (HIGH: TestReviewBotIdentitiesStayInSync gives asymmetric protection across the three files it covers; MEDIUM: TestIsOrgBotConstantsStayInSync doesn't cover the want_role guard's comparison operator), plus one low-priority process note (no tracking issue for the deferred performed_via_github_app.id fix). Full detail inline.
bdbf027 to
6725415
Compare
|
🤖 Finished Review · ✅ Success · Started 2:22 PM UTC · Completed 2:38 PM UTC |
6725415 to
43020c6
Compare
|
🤖 Finished Review · ✅ Success · Started 4:23 PM UTC · Completed 4:40 PM UTC |
43020c6 to
a1635e3
Compare
|
🤖 Finished Review · ✅ Success · Started 4:55 PM UTC · Completed 5:10 PM UTC |
The collaborator permission API does not recognize GitHub App bot
accounts (role_name comes back empty), so any authorization check that
delegates to it always fails for a bot actor — even though the bot has
legitimate access via its app installation grant. The identical root
cause independently broke five places across the dispatch/fix pipeline
that assumed a fullsend agent bot's identity is always
"${ORG_NAME}-<role>[bot]":
- pull_request_target.opened/synchronize/ready_for_review: PRs authored
by fullsend's own agents never received automatic review, including
after every fix-agent iteration (fullsend-ai#5188).
- issues.opened/edited: issues filed or edited by fullsend's own agents
(e.g. the retro agent's proposal issues) never received automatic
triage (fullsend-ai#2636).
- pull_request_review (changes_requested) -> fix auto-dispatch: checked
REVIEW_USER_LOGIN == "${ORG_NAME}-review[bot]" directly.
- reusable-fix.yml's "Pre-fetch review body" step: same jq filter
mistake, causing a hard failure ("nothing to fix") when the review
came from the shared bot.
- pre-fetch-prior-review.sh: same mistake, silently treating every
dispatch as a first review (no delta review, no prior SHA anchoring)
when the prior review came from the shared bot.
All five assumed the bot's own installing-org name (ORG_NAME /
github.repository_owner) is baked into its identity. That's wrong for
fullsend's default deployment model: per ADR 0029/0059/0068, every
adopting org installs the SAME shared, vendor-owned fullsend-ai-<role>
Apps, so the bot identity is fixed regardless of which repo it operates
on. Each of these checks only "worked" by coincidence on
fullsend-ai/fullsend itself, where org name and vendor-App prefix
happen to collide, and was a no-op for every other adopting org. Found
via a human reviewer (waynesun09) on the first instance; a repo-wide
grep for the same pattern turned up the other four.
One fix, applied everywhere the bug appeared: is_org_bot() recognizes
two exact-match identities, never a prefix/suffix wildcard, optionally
restricted to one specific role via a second argument (needed at the
fix-dispatch gate, which must match specifically the review bot):
1. fullsend-ai-<role>[bot] for the five known agent roles (coder,
review, triage, retro, prioritize), unconditionally, independent of
ORG_NAME.
2. ${ORG_NAME}-<role>[bot] for the same five roles, for self-managed
orgs running their own private per-role Apps (ADR 0029/0033).
Both paths match only a fixed, known set of role suffixes — never a
bare wildcard — so a third party can't forge the bypass by registering
a GitHub App named "${ORG_NAME}-<anything>[bot]" or
"fullsend-ai-<anything>[bot]". The same two-identity check is inlined
directly wherever jq can't call the bash helper (three separate jq
filters). ADR 0054 documents this as an explicit platform-constraint
exception.
Extract the routing helpers into scripts/dispatch-route-helpers.sh with
unit tests so is_org_bot and friends are testable outside the workflow
YAML, and compact the remaining inline helpers to keep both dispatch.yml
(per-org) and reusable-dispatch.yml (per-repo) under the workflow size
limit (ADR-0005). gh-api error text is sanitized before being embedded
in a GitHub Actions ::warning:: annotation, since an unsanitized "::"
sequence could otherwise be read as a workflow command.
Test coverage locks in all three bypass call sites (including the
review-role-restricted one) and, per a human reviewer's suggestion,
verifies the security-relevant is_org_bot constants (role list and both
identity match patterns) stay identical across the three hand-copied
implementations, guarding against the kind of silent drift that
produced earlier review rounds' divergence findings.
Signed-off-by: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Greg Allen <gallen@redhat.com>
a1635e3 to
1ba11e9
Compare
|
🤖 Finished Review · ✅ Success · Started 6:31 PM UTC · Completed 6:47 PM UTC |
ifireball
left a comment
ifireball
left a comment
There was a problem hiding this comment.
We have discussed this issue multiple times - the solution for triggering on the bot actions is to have the bot prove they have sufficient access by setting labels! I do not thing we should merge this PR.
Also if we do end up agreeing to go down this path, at this point I would expect such a change to also be applied to the CEL-based dispatch path.
| [[ -z "${username}" ]] && return 1 | ||
| for role in ${FULLSEND_AGENT_ROLES}; do | ||
| [[ -n "${want_role}" && "${role}" != "${want_role}" ]] && continue | ||
| [[ "${username}" == "fullsend-ai-${role}[bot]" ]] && return 0 |
There was a problem hiding this comment.
this assumes the defaults set of GH apps - people can still use other apps.
There was a problem hiding this comment.
Fair point — verified this is real: --app-set accepts any string (ValidateAppSet has no format requirement tying it to the org), and docs/guides/infrastructure/advanced-setup.md explicitly documents registering "multiple app sets on the same mint," which by definition can't all equal a single ${ORG_NAME}. Neither this check nor the collaborator-permission-API check it replaces has ever been able to recognize a non-conventional app-set bot (GitHub Apps aren't collaborators regardless of name), so this narrows an existing gap rather than introducing one. Properly closing it means threading the actual configured app-set prefix through as a repo variable at install time (nothing analogous to the existing FULLSEND_MINT_URL/FULLSEND_GCP_REGION variables exists for this yet) — that's installer + scaffold plumbing beyond this PR's routing-bug-fix scope, so I filed #5480 to track it and added a short note to the ADR (pending push, commit ae9930ed) acknowledging the gap where the ${ORG_NAME} assumption is stated.
There was a problem hiding this comment.
The need to recognise bots by name evaporates if we require then to prove their access by having them proactively set labels instead of trying to sniff it via the GH API. This had been our approach for a long time, and I'm not sure why are we diverging from it now. (I wrote this on my main review comment as well, but those seem to always get lost in the noise)
| if [[ -n "${ORG_NAME:-}" ]]; then | ||
| for role in ${FULLSEND_AGENT_ROLES}; do | ||
| [[ -n "${want_role}" && "${role}" != "${want_role}" ]] && continue | ||
| [[ "${username}" == "${ORG_NAME}-${role}[bot]" ]] && return 0 |
There was a problem hiding this comment.
this still makes string-based assumptions on the bot name, in practice bots can have strange names if people so desire.
There was a problem hiding this comment.
Same root concern as your comment on line 176 above — this pattern is string/name-based by necessity (GitHub Apps aren't identifiable any other way in these event payloads, and performed_via_github_app.id pinning is tracked separately as #5463 for the review-bot-specific gate). The specific gap you're pointing at here — that the name doesn't have to match ${ORG_NAME} at all — is now tracked as #5480, with an ADR note added (commit ae9930ed, pending push) acknowledging that a self-hosted operator can register an app set under any name.
| Both `is_authorized` (slash commands) and `is_event_actor_authorized` | ||
| (event triggers) delegate to a shared `has_write_permission` helper that | ||
| calls the collaborator permission API. This ensures consistent behavior | ||
| across all paths. |
There was a problem hiding this comment.
Why are we (massively) updating an accepted ADR instead of sending-in a new one when we want to make drastic design changes?
There was a problem hiding this comment.
Fair question. The convention here is append-only dated notes ("Note (YYYY-MM-DD, #issue):" blockquotes) rather than editing the original decision text — this predates this PR: the #5223 note above (2026-07-17) already used this pattern to extend the authorization mechanism (adding the triage permission tier) without touching the accepted body text. This PR's notes follow the same convention to document a platform-constraint exception (bot identities aren't resolvable via the collaborator-permission API this ADR originally specified), not to reverse or rewrite the original decision — repository permissions are still the authorization source in both cases, just resolved two different ways.
That said, I think your broader point stands as a real design question: if the is_org_bot() pattern keeps needing amendment (as this thread and #5480 show), at what point does it stop being a footnote on a permission-check ADR and become its own decision worth a fresh ADR with its own context/consequences section? I don't have a strong view on where that line is, but if #5480's fix ends up being substantial (new install-time plumbing, a repo variable, etc.), that's a reasonable candidate for its own ADR rather than another note here.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review squad pass 5 (Claude + Grok, 2 agents). All three round-4 fixes verified effective via direct content inspection against the real files, not just re-reading the diff. One new finding this round: the wantRoleGuard assertion only anchors half of a compound && condition, leaving an -n→-z mutation undetected (full detail inline). Grok's independent assessment: this PR has reached a natural stopping point — rounds 1-4 fixed real gaps in the core fix and its test coverage, and this round's findings (including the one above) are narrow test-meta-coverage gaps rather than defects in the shipped fix itself. Recommend merging once the inline finding is addressed, without further review rounds on test-suite meta-coverage.
| // the pull_request_review -> fix gate. | ||
| const wantRoleGuard = `"${role}" != "${want_role}"` | ||
| for name, content := range sources { | ||
| assert.Contains(t, content, wantRoleGuard, |
There was a problem hiding this comment.
[medium] test-coverage
Confirmed against the real is_org_bot() guard in reusable-dispatch.yml/dispatch.yml: the actual guard clause is the compound condition [[ -n "${want_role}" && "${role}" != "${want_role}" ]] && continue, but wantRoleGuard only anchors the second half ("${role}" != "${want_role}"). Mutating -n to -z in the two production YAML copies (not dispatch-route-helpers.sh, which is separately covered by dispatch-route-helpers-test.sh's executable tests) would not be caught by any test in this suite, and inverts behavior in both call shapes:
- Called with no role arg (
is_org_bot "${ISSUE_USER_LOGIN}", etc.):want_roleis empty, so-zis true; sinceroleis always non-empty,!=is always true too, socontinuealways fires —is_org_botnever matches anything, silently regressing back to the original Review agent skipped on bot-authored PRs due to collaborator permission check failing for GitHub App accounts #5188/dispatch: retro-filed proposal issues skip auto-triage under ADR 0054 authorization gate #2636 failure. - Called with a role arg (
is_org_bot "${REVIEW_USER_LOGIN}" review):-z "review"is false, so the&&is false and the role filter is never applied — any of the 5 agent-role bots (not justreview) would satisfy thepull_request_review → fixgate, exactly the escalation the role-filter argument exists to prevent.
Suggested fix: extend wantRoleGuard to anchor the full compound condition, e.g. `-n "${want_role}" && "${role}" != "${want_role}"`, across all three sources.
| # both exact logins are inlined here directly. | ||
| gh api "repos/${SOURCE_REPO}/pulls/${PR_NUM}/reviews" \ | ||
| --jq "[.[] | select(.state == \"CHANGES_REQUESTED\" and .user.login == \"${REVIEW_BOT}\")] | last | .body // \"\"" \ | ||
| --jq "[.[] | select(.state == \"CHANGES_REQUESTED\" and (.user.login == \"fullsend-ai-review[bot]\" or .user.login == \"${ORG_NAME}-review[bot]\"))] | last | .body // \"\"" \ |
There was a problem hiding this comment.
[medium] prompt-injection
Pre-existing: the Pre-fetch review body steps match review comments by .user.login string, which on the self-managed org path could be spoofed by a third party squatting the GitHub App slug. This path feeds potentially attacker-authored text into the fix agent prompt. Tracked as #5463 and marked out of scope for #5188/#2636. pre-fetch-prior-review.sh already performs performed_via_github_app.client_id provenance validation downstream.
| --jq '.role_name' 2>"${api_err}") || { | ||
| echo "::warning::Permission API call failed for ${username}: $(cat "${api_err}")" >&2 | ||
| sanitized_err=$(tr '\n' ' ' < "${api_err}" | sed 's/::/: /g') | ||
| echo "::warning::Permission API call failed for ${username}: ${sanitized_err}" >&2 |
There was a problem hiding this comment.
[low] workflow-command-injection
Pre-existing (improved): ${username} is interpolated unsanitized into ::warning:: workflow commands in has_repo_permission. GitHub login names are restricted to [a-zA-Z0-9-] plus [bot], preventing injection in practice. The PR improves the situation by adding sed sanitization to the API error output.
| is_issue_author() { | ||
| [[ "${COMMENT_USER_LOGIN:-}" == "${ISSUE_USER_LOGIN:-}" ]] | ||
| } | ||
|
|
There was a problem hiding this comment.
[low] edge-case
Pre-existing: when both COMMENT_USER_LOGIN and ISSUE_USER_LOGIN are unset or empty, is_issue_author returns true because empty-string equality evaluates to true in bash [[ ]]. Not practically reachable.
|
Closing this in favor of #5551, which fixes #5188 via label re-triggering instead of actor-identity recognition. Summary of the outcome: @ifireball's review here was right — this PR's The actual root cause of #5188 (review not re-dispatching after a fix-agent push) turned out to be simpler than this PR assumed: @waynesun09 — the review-body content-attribution bug you found during this PR's review (the Thanks both for catching this before it merged. |
|
🤖 Finished Retro · ✅ Success · Started 10:07 PM UTC · Completed 10:26 PM UTC |
Retro: PR #5415 — recognize org bot actors in dispatch routingOutcome: Closed without merging after 2 days, 17 force-pushes, 18 review agent runs, and 5 human review-squad rounds. A human reviewer (ifireball) identified a fundamental design conflict that no agent caught: the PR's identity-based bot recognition approach ( Cascading failure from wrong triage recommendationThe failure cascaded through the full pipeline:
Evidence for existing open issuesThis retro provides concrete, high-impact evidence for several existing proposals. No new proposals are warranted — the improvement areas are already well-identified:
What went well
|
pull_request_target.synchronize fires when the fix agent pushes, but its actor-identity authorization check is gated on PR_USER_LOGIN — the PR's original author, which for agent-authored PRs is the code agent's bot account regardless of who triggered this fix run. GitHub App bots have no collaborator role, so that check always fails closed and review is never re-dispatched after a fix-agent push (fullsend-ai#5188). post-fix.sh now removes then re-adds the ready-for-review label after a successful push, forcing a fresh labeled webhook event. That path has no actor-authorization gate at all — label application itself already requires write access, so it needs no separate identity check — mirroring post-code.sh's identical handling of the PR-open case. GitHub does not fire a new labeled event when a label already present is re-added, hence the remove-then-add sequence. This supersedes fullsend-ai#5415, which attempted the same fix via an actor-identity-recognition function (is_org_bot()) extended across several dispatch-authorization gates. That approach was closed after review: it reintroduced a design (recognizing bots by name for dispatch authorization) that issue fullsend-ai#2669 had already evaluated and rejected in favor of label-based gating — "actors can be spoofed and the approach is fragile across workflow changes" — a decision PR fullsend-ai#2679 already implemented and shipped for the retro-to-triage handoff (closing fullsend-ai#2636). The label-based fix here needs no changes to the CEL-based dispatch path (internal/harnessdispatch), which already trusts label-added events unconditionally, and needs no forge-specific bot-identity logic, since labels work identically across GitHub and GitLab. A separate, unrelated bug that fullsend-ai#5415 also touched — the fix agent's own review-body content-attribution lookups missing the shared fullsend-ai-review[bot] identity — is tracked independently as fullsend-ai#5550, since it is not a dispatch-authorization question and is unaffected by this change. Signed-off-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Greg Allen <gallen@redhat.com>
3 participants
Summary
The collaborator permission API does not recognize GitHub App bot accounts (
role_namecomes back empty), so any authorization check that delegates to it always fails for a bot actor — even though the bot has legitimate access via its app installation grant. This broke two independent dispatch paths silently:pull_request_target.opened/synchronize/ready_for_review: PRs authored by the org's own agents (e.g.fullsend-ai-coder[bot]) never received automatic review, including after every fix-agent iteration (Review agent skipped on bot-authored PRs due to collaborator permission check failing for GitHub App accounts #5188).issues.opened/edited: issues filed or edited by the org's own agents (e.g. the retro agent's proposal issues) never received automatic triage (dispatch: retro-filed proposal issues skip auto-triage under ADR 0054 authorization gate #2636) — previously worked around by having the filer also pre-apply a routing label, rather than fixing the actor check itself.This is the same bug recurring at two call sites, not two different bugs, so it gets one fix.
Fixes #5188
Changes
is_org_bot()recognizing any org-prefixed bot identity (${ORG_NAME}-*[bot]— coder, review, triage, retro, prioritize, present and future). Third-party bots (renovate[bot],dependabot[bot], etc.) still fall through to the normal collaborator-permission check.is_org_botas a bypass on bothpull_request_target.opened/synchronize/ready_for_review(review dispatch) andissues.opened/edited(triage dispatch), in bothdispatch.yml(per-org) andreusable-dispatch.yml(per-repo).internal/scaffold/fullsend-repo/scripts/dispatch-route-helpers.shwith unit tests (dispatch-route-helpers-test.sh) so this logic is testable outside the workflow YAML, and compact the remaining inline helpers to keep both workflow files under the size limit (ADR-0005).Makefilescript-testtarget.Testing
dispatch-route-helpers-test.sh— 22/22 passing (is_org_botacross org/third-party bots and all agent roles,has_labelincl. aset -unounset regression test,is_issue_author)go test ./internal/scaffold/...— passing, including updatedTestDispatchWorkflowContentpre-commit run— all hooks pass, including ShellCheck andlint-workflow-size(dispatch.ymlat 492/500 lines)scan-secretscleanReview history
This PR went through two review rounds (fullsend-ai-review + Qodo) that flagged and got fixed: a CI-breaking stale test assertion, a
set -unounset bug inhas_label, dead code, a misleading file header, and several doc/consistency gaps. All inline threads were replied to and resolved. The remaining open finding is a protected-path note (.github/changes always require human approval regardless of content) — not an actionable defect.Context
Originally scoped narrowly to #5188 (code agent's own PRs never reviewed). While preparing this PR, found the identical root cause independently breaks issue-triage dispatch too (#2636, previously papered over with a label-based workaround rather than a real fix) — broadened the fix to cover both rather than filing a near-duplicate follow-up.