feat(dispatch): route /fs-plan-tests to the qualityflow stage - #6284
feat(dispatch): route /fs-plan-tests to the qualityflow stage#6284guyoron1 wants to merge 1 commit into
Conversation
Custom agents registered via config.yaml agents[] cannot be triggered by comment in per-org mode: the bash router only knows the built-in commands, and the harness CEL path (harness-dispatch) reads .fullsend/config.yaml from the caller repo, which per-org enrolled repos do not have. A /fs-plan-tests comment therefore routes nowhere and the run exits with "No stage matched". Adds the case branch this file documents as the way to add a stage, plus the qualityflow -> coder role mapping (declared by its harness) and the existing fork gate, since the agent commits generated tests to the PR branch. Interim until harness CEL dispatch becomes primary (fullsend-ai#2902), which removes the bash routing this branch lives in. Signed-off-by: guy oron <goron@redhat.com>
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
PR Summary by QodoRoute /fs-plan-tests comments to the qualityflow dispatch stage
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1. /fs-plan-tests routing out-of-sync
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| /fs-plan-tests) | ||
| # QualityFlow custom agent. Mirrors its harness CEL trigger | ||
| # (change_proposal, non-fork); fork gate is in "Resolve PR | ||
| # head" below. Interim until CEL dispatch is primary (#2902). | ||
| if [[ "${COMMENT_USER_TYPE}" != "Bot" ]] && is_authorized \ | ||
| && [[ "${ISSUE_HAS_PR}" == "true" ]]; then | ||
| STAGE="qualityflow" | ||
| fi |
There was a problem hiding this comment.
1. /fs-plan-tests routing out-of-sync 📘 Rule violation ⚙ Maintainability
internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml adds routing for /fs-plan-tests to STAGE="qualityflow", but .github/workflows/reusable-dispatch.yml does not implement the same routing (and related stage handling), violating the requirement to keep the per-org and per-repo dispatch workflows in sync. This drift makes /fs-plan-tests behave differently across install modes and can result in per-repo dispatch being skipped when STAGE remains empty.
Agent Prompt
## Issue description
The per-org dispatcher scaffold (`internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml`) now routes the `/fs-plan-tests` slash command to `STAGE=qualityflow`, but the per-repo dispatcher (`.github/workflows/reusable-dispatch.yml`) does not include the same `/fs-plan-tests` routing and associated stage handling (role mapping and relevant gating). This creates stage-routing drift between the two dispatch workflows and causes per-repo installations to ignore `/fs-plan-tests` (logging that no stage matched and skipping dispatch).
## Issue Context
This repository maintains two dispatch workflows:
- Per-org dispatcher scaffold: `internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml`
- Per-repo dispatcher: `.github/workflows/reusable-dispatch.yml`
Compliance requires jq payload/stage routing/secret threading logic to remain in sync across these dispatch workflows unless an explicit inline comment documents an intentional divergence. When introducing a new built-in command/stage, both dispatchers should support the same slash-command surface; otherwise behavior drifts across install modes.
## Fix Focus Areas
- internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml[166-173]
- .github/workflows/reusable-dispatch.yml[185-245]
- .github/workflows/reusable-dispatch.yml[440-454]
- .github/workflows/reusable-dispatch.yml[494-520]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review findings (review-only sweep; not requesting changes).
CRITICAL — PR breaks go test ./internal/scaffold/... (two pre-existing assertions now fail)
internal/scaffold/workflow_call_alignment_test.go:975 (file not touched by this diff, so it can't be anchored inline)
Reproduced locally at HEAD (a1724c5): go test ./internal/scaffold/... fails with --- FAIL: TestDispatchPRHeadResolution and --- FAIL: TestRoleCheckCaseBranches, both in the scaffold/dispatch.yml sub-case (the reusable-dispatch.yml sub-case still passes).
TestDispatchPRHeadResolution's regex expects the oldfix|review)guard text in the "Resolve PR head for issue_comment events" step; this PR changed it to^(fix|review|qualityflow)$, so the regex no longer matches.TestRoleCheckCaseBranchesasserts the literal substringcode|fix) STAGE_ROLE="coder"exists in dispatch.yml (test line 975). This PR changed that line (dispatch.yml:437) tocode|fix|qualityflow) STAGE_ROLE="coder", so the exact-substring match fails withdoes not contain "code|fix) STAGE_ROLE=\"coder\"".
The PR description's verification table only covers a manually-extracted routing script, not the repo's actual Go test suite, so this regression isn't caught there.
Suggestion: update workflow_call_alignment_test.go alongside the workflow change — relax TestDispatchPRHeadResolution's regex to tolerate the added |qualityflow alternative (e.g. (?s)fix\|review(\|qualityflow)?\).*exit 1), and update TestRoleCheckCaseBranches' expected substring to code|fix|qualityflow) STAGE_ROLE="coder". Re-run go test ./internal/scaffold/... before pushing — it currently fails deterministically on this branch.
MEDIUM — New /fs-plan-tests slash command and its auth requirement are undocumented
docs/ADRs/0054-require-authorization-on-all-agent-dispatch-paths.md:41 (file not touched by this diff, so it can't be anchored inline)
This ADR maintains an explicit table of every slash-command dispatch path and its authorization requirement (lines 41-42 list /fs-review and /fs-fix), presented as the canonical enumeration of gated paths, but has no row for the new /fs-plan-tests -> write-level is_authorized path this PR adds. Grepping docs/ for fs-plan-tests also turns up zero references anywhere else in docs/ (docs/agents/, docs/guides/, docs/glossary.md), unlike /fs-fix and /fs-review which are documented in multiple places. Since this ADR explicitly claims to enumerate all gated dispatch paths and this PR adds a new one with a real security-relevant auth requirement, the omission is a genuine gap, not merely stylistic.
Suggestion: add a row for /fs-plan-tests (Yes, is_authorized write) to the ADR-0054 authorization table, and add a brief docs/agents/ or glossary mention of the command, or explicitly note in the PR that documentation is deferred pending #2902 given the command's interim/bash-routing lifetime.
| # QualityFlow custom agent. Mirrors its harness CEL trigger | ||
| # (change_proposal, non-fork); fork gate is in "Resolve PR | ||
| # head" below. Interim until CEL dispatch is primary (#2902). | ||
| if [[ "${COMMENT_USER_TYPE}" != "Bot" ]] && is_authorized \ |
There was a problem hiding this comment.
MEDIUM — /fs-plan-tests gate order runs the GitHub permission-API call before the cheap ISSUE_HAS_PR check
Because bash && short-circuits left-to-right, is_authorized (a gh api repos/.../collaborators/<user>/permission network call) runs for every non-bot /fs-plan-tests comment regardless of whether the comment is even on a PR, since the cheap ISSUE_HAS_PR check is evaluated last. The existing /fs-review and /fs-fix cases in this same file put ISSUE_HAS_PR as the outer/first condition specifically to skip the API call when it can't route anywhere, so this is a real deviation from the established pattern, adding avoidable GitHub API load on every plain-issue use of the command.
Suggestion: reorder to check ISSUE_HAS_PR first, matching /fs-review and /fs-fix:
if [[ "${ISSUE_HAS_PR}" == "true" ]]; then
if [[ "${COMMENT_USER_TYPE}" != "Bot" ]] && is_authorized; then
STAGE="qualityflow"
fi
fi
| STAGE="prioritize" | ||
| fi | ||
| ;; | ||
| /fs-plan-tests) |
There was a problem hiding this comment.
MEDIUM — No test coverage added for the new /fs-plan-tests routing or its gates
The diff only touches dispatch.yml and the lint-workflow-size max-lines directive; internal/scaffold/workflow_call_alignment_test.go gains no new test cases. The PR description substitutes an ad hoc, out-of-repo script run (a manual verification table) for actual in-repo coverage, and offers to port it into the behaviour-test harness later. As-is, nothing in the Go test suite asserts /fs-plan-tests routes to stage qualityflow, requires write-level (not triage) auth, requires ISSUE_HAS_PR, fails closed on PR-head fetch failure for the qualityflow stage, or is blocked on fork PRs by the extended regex.
Suggestion: add unit test cases mirroring existing /fs-fix/-review coverage in workflow_call_alignment_test.go: assert /fs-plan-tests requires write-level auth, requires ISSUE_HAS_PR == true, fails closed (exit 1) on PR-head fetch failure, and is blocked on fork PRs via the same regex as fix/qualityflow.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Requesting changes — this adds a new feature to the deprecated per-org path, and puts a third-party agent's command into the shared upstream scaffold. Neither should ship. The good news is the capability you need already exists on the per-repo path and needs no upstream change.
Which file this touches
Confirmed from the shim templates on main:
internal/scaffold/fullsend-repo/templates/shim-workflow-call.yaml:58→__ORG__/.fullsend/.github/workflows/dispatch.yml@main— per-org, a deployed copy of the file this PR editsinternal/scaffold/fullsend-repo/templates/shim-per-repo.yaml:46→__REUSABLE_DISPATCH__withinstall_mode: per-repo— per-repo,reusable-dispatch.yml, untouched here
So this change lands exclusively on the per-org dispatcher and does nothing for per-repo users. Run 32015419992 confirms the same routing: the shim forwards to fullsend-ai/.fullsend/.github/workflows/dispatch.yml@main, and that deployed copy has no harness-dispatch job, no fullsend dispatch, and no --config-dir at all — so the CEL path isn't evaluating against a missing .fullsend/ directory as the description says, it simply doesn't exist on that path. harness-dispatch lives only in reusable-dispatch.yml (per-repo).
Why this shouldn't land
1. New feature on the immediate-removal path. ADR 0044 (Accepted 2026-06-16) makes per-repo the sole supported install model. #2302 plus the 2026-08-06 decision (#5995) dropped the phased deprecation in favour of immediate removal. #6113 was filed precisely because agents keep generating org-mode content — "do not add org-mode-specific content to docs or code." Extending the org dispatcher is that, in the routing layer.
2. A third-party agent's command and role in the shared scaffold. /fs-plan-tests belongs to redhat-community-ai-tools/qualityflow-fullsend, and the change also hardcodes code|fix|qualityflow → coder into the generic router. A fullsend release shouldn't ship one team's fleet extension as a default — that's what agents[] registration exists to avoid.
3. Concrete defect for per-org orgs that don't have QualityFlow. Check agent is enabled only skips on an explicit enabled: false name match, and registered agents are source: URLs with no name key — so nothing matches and nothing skips. The coder role gate passes on any standard defaults.roles. Stage discovery then finds no # fullsend-stage: qualityflow marker and the job hits ::error::No workflows found for stage: $STAGE → exit 1. Result: a failed dispatch run for any write+ user who types /fs-plan-tests on a PR in such an org — and it only reaches them after a manual sync-scaffold, since nothing auto-propagates this file (#6013).
The supported path already covers this
The QualityFlow harness documents the intended consumption model itself:
Consuming teams reference this via
base:inheritance in their.fullsendrepo
That's the same layering the code/fix agents use to pick up remote skills — base composition is implemented in internal/harness/compose.go (LoadWithBase), and the harness already carries its own skills:, policy, image, and role: coder. This belongs in the consuming repo's config, extending the default fleet from fullsend-ai/agents, not in the action release.
The trigger side needs nothing upstream either. Your CEL trigger is comment-shaped, not PR-shaped:
event.transition.kind == "comment_added"
&& event.transition.comment.command == "/fs-plan-tests"
&& has(event.state.change_proposal)
&& !event.state.change_proposal.is_fork
internal/dispatch already routes exactly this — see TestHarnessRouter_SlashCommandCustomAgent, which asserts /fs-custom-agent routes to a config-registered agent, alongside the unknown-agent case. A per-repo install that registers QualityFlow in its own .fullsend/config.yaml gets /fs-plan-tests working today. The trigger registered in fullsend-ai/.fullsend#93 is correct; the reason nothing consumes it is that fullsend-ai is still on per-org mode, where the CEL path doesn't exist.
Suggested path forward
- Register QualityFlow in the consuming repo's own
.fullsend/config.yamlviabase:extend with its remote skills. Supported today, no upstream change. - For the
fullsend-aidogfood gap: the fix is migrating off per-org (#5815repos migrate), not extending the dispatcher that #2302 deletes. - If you need it working before that migration lands: the org's
.fullsend/dispatch.ymlis an org-local copy and nothing auto-overwrites it (#6013) — the same repo where the QualityFlow stage workflow already lives. Carrying the case branch there unblocks you now without the command entering the upstream scaffold.
On the two points in the description: the "case branch is the documented way to add a stage" note is about built-in stages — org-specific custom agents are exactly the case agents[]/CEL registration was built for. And "interim, #2902 removes it" doesn't quite hold either, since #2902 rewrites reusable-dispatch.yml (per-repo), while this branch lives in the per-org copy that #2302 deletes on a separate track.
The verification table is genuinely thorough, and the diagnosis that nothing consumes the registered trigger is correct — it's the placement of the fix I'm objecting to, not the problem statement.
|
Closing per the review above: per-org mode is deprecated (ADR 0044, #2302) and this change only lands on that path, while the custom-agent capability it's after is already supported per-repo via |
|
🤖 Finished Retro · ✅ Success · Started 5:41 PM UTC · Completed 5:55 PM UTC Commit: |
Retro: PR #6284 — Route
|
|
Thanks @waynesun09 — agreed on all counts, closing this. I verified both blocking findings independently rather than taking them on faith: The defect for orgs without QualityFlow is real. Every entry in The test breakage reproduces at the line you cited. I've also taken the ADR 0044 / #2302 point: extending the per-org dispatcher is adding to the path that's being deleted, and Following your suggested path — the case branch now lives only in the org-local copy in fullsend-ai/.fullsend#167, alongside the stage workflow it needs, so nothing enters the upstream scaffold and no other org is affected. I've corrected the |
2 participants
Problem
A custom agent registered via
config.yamlagents[]cannot be triggered by comment in per-org mode.Concretely,
/fs-plan-testson #6010 fired run 32015419992 and the dispatch job logged:Two reasons it routes nowhere:
config.yamlagents[](and cannot — routing runs before the config repo is checked out).reusable-dispatch.yml→harness-dispatch) runsfullsend dispatch --config-dir .fullsendagainst the caller repo. Per-org enrolled repos have no.fullsend/directory, so CEL triggers never evaluate. QualityFlow's trigger is registered in Add QualityFlow custom agent with CEL trigger .fullsend#93 and is correct — nothing consumes it yet.Change
This adds the case branch that this file documents as the way to add a stage:
/fs-plan-tests→STAGE=qualityflow, gated PR-only + write+ + non-Bot, mirroring the harness CEL trigger (has(change_proposal) && !is_fork)qualityflow→coderrole, which its harness declares (role: coder)qualityflow, since the agent commits generated tests to the PR branch (same exposure asfix), and a failed PR-context fetch is fatal rather than a warningmax-lines610 → 625, the documented path for a justified increaseThe companion stage workflow lives in
.fullsend(org-local, likescribe.yml) and is discovered via its# fullsend-stage: qualityflowmarker.Interim by design: #2902 removes the bash routing this branch lives in, and this case goes with it.
Verification
Extracted the real "Determine stage" script from the workflow and ran it against a stubbed
gh(permission API), asserting the routed stage:/fs-plan-testson PR, write+ memberqualityflow/fs-plan-tests PROJ-123(trailing arg)qualityflow/fs-plan-testson an issue (no PR)/fs-plan-testsfrom a Bot/fs-plan-testsfrom read-only user/fs-plan-testsfrom triage-only user/fs-review,/fs-code,/fs-triage/fs-bogus10/10, and the same 10 pass against the currently-deployed copy in
.fullsendwith the same patch applied. Happy to port these into the behaviour-test harness (#2891–#2895) if you'd prefer them in-repo.