Repository navigation
feat(#8726): let a CEL trigger decide review routing - #8729
Closed
waynesun09 wants to merge 1 commit into
Closed
waynesun09 wants to merge 1 commit into
waynesun09 wants to merge 1 commit into
Conversation
Add `fullsend dispatch --route review`, which resolves the review harness the way `fullsend run` does (a config entry with source:, else the agents-repo fallback at the lockstep ref) and prints one JSON line: run, skip, or bash when the resolved harness has no trigger. The route job evaluates it when the repository or organization variable FULLSEND_CEL_ROUTING_REVIEW is shadow or on, for pull request and comment events. shadow logs the CEL decision next to the bash stage; on lets it set or clear STAGE=review. The review job still runs the review. With the variable off or unset every new step is skipped and the bash stage is unchanged. Move the OWNERS-aware authorization gate out of Dispatch into AuthorizeEvent so both paths share it. Add a parity test of the default review trigger against the bash router, a test that runs the Determine stage script per mode, and a behaviour step that sets a repository variable and restores it at cleanup. Closes #8726 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
PR Summary by QodoLet review harness CEL triggers decide review routing
AI Description
Diagram
High-Level Assessment
Files changed (20)
|
|
🤖 Finished Review · ✅ Success · Started 9:14 PM UTC · Completed 9:25 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $3.18 |
Member
Author
Code Review by Qodo
1. The guide starts without prerequisites
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+732
to
+740
| ## Choosing when review runs | ||
|
|
||
| By default, the dispatch workflow decides when the review agent runs: when a | ||
| pull request is opened, pushed to or marked ready, when the | ||
| `ready-for-review` label is added, and on a `/fs-review` comment. To change | ||
| that, set the repository or organization variable | ||
| `FULLSEND_CEL_ROUTING_REVIEW` and give the review harness a `trigger` | ||
| expression. The review still runs in the same `Review` job; the trigger only | ||
| decides whether that job starts. |
There was a problem hiding this comment.
1. The guide starts without prerequisites 📜 Skill insight ✧ Quality
The new review-routing procedure in customizing-agents.md adds setup commands without a labeled prerequisites section. Readers reach the configuration steps without being told what access, repository setup, or tools they need beforehand.
Agent Prompt
## Issue description
The guide adds a review-routing procedure but does not provide a clearly labeled prerequisites section before the procedure begins.
## Fix Focus Areas
- docs/guides/user/customizing-agents.md[732-740]
## Recommended Fix
Add a `## Prerequisites` section before `## Choosing when review runs` that lists the required repository or organization access, an enrolled repository, and the GitHub CLI or equivalent variable-management capability needed to follow the example.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+755
to
+758
| ### Example: review only on `/fs-review` | ||
|
|
||
| To stop automatic reviews on every pull request but keep `/fs-review`, | ||
| override the review harness with a narrower trigger: |
There was a problem hiding this comment.
6. Procedure prose hides the routing steps 📜 Skill insight ✧ Quality
The review-routing example explains how to change behavior in prose and then places the commands in an unnumbered code block. Readers cannot follow the setup as the required ordered procedure, and later edits can obscure the intended sequence.
Agent Prompt
## Issue description
The new review-routing procedure uses prose paragraphs and an unnumbered command block instead of numbered steps.
## Fix Focus Areas
- docs/guides/user/customizing-agents.md[755-783]
## Recommended Fix
Rewrite the example instructions as an ordered list, with each command or action at the start of its numbered step followed by any explanation. Keep the YAML examples as supporting configuration blocks under the relevant steps.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+809
to
+814
| Two cases are not yet authorized on the trigger path, so with `on` they do | ||
| not start a review: pull requests and `/fs-review` comments from triage-role | ||
| users ([#8724](https://github.com/fullsend-ai/fullsend/issues/8724)), and pull | ||
| requests opened or pushed by a bot without a `write` collaborator role, | ||
| including fullsend's own agent bots. Check both in `shadow` before you switch | ||
| to `on`. |
There was a problem hiding this comment.
4. Triage users lack trigger-route access 📜 Skill insight ≡ Correctness
The guide states that triage-role users and some bot-authored events are not yet authorized on the trigger path while routing is enabled. Because the text describes future authorization work only through issue links and not the required planned-feature callout, readers can mistake the limitation for an undocumented permanent behavior.
Agent Prompt
## Issue description
The guide documents trigger-path authorization gaps as not-yet-authorized cases without using the required planned-feature callout format.
## Fix Focus Areas
- docs/guides/user/customizing-agents.md[809-814]
## Recommended Fix
Convert this limitation into a `> **Planned:**` callout that explicitly links to the relevant issue(s), and state that triage-role and bot-authored trigger routing is not currently supported rather than presenting the future work as ordinary guidance.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+738
to
+740
| `FULLSEND_CEL_ROUTING_REVIEW` and give the review harness a `trigger` | ||
| expression. The review still runs in the same `Review` job; the trigger only | ||
| decides whether that job starts. |
There was a problem hiding this comment.
7. The review guide leaves cel undefined 📜 Skill insight ✧ Quality
The new guide uses CEL in the routing variable and decision logs without expanding the acronym or linking it to a glossary or reference on first use. A reader unfamiliar with the expression language cannot tell what the routing mechanism means before following the configuration example.
Agent Prompt
## Issue description
The review-routing guide introduces CEL-based routing without defining `CEL` or linking the term to the trigger reference.
## Fix Focus Areas
- docs/guides/user/customizing-agents.md[738-740]
- docs/guides/user/customizing-agents.md[790-800]
## Recommended Fix
Expand CEL as Common Expression Language on its first occurrence and link it to `cel-triggers-reference.md` or the existing CEL specification reference. Use the same linked or defined terminology in the decision-log example.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| cmd.Flags().StringVar(&eventName, "event-name", "", "GitHub event name (default: GITHUB_EVENT_NAME)") | ||
| cmd.Flags().StringVar(&eventAction, "event-action", "", "GitHub event action") | ||
| cmd.Flags().StringVar(&forgeToken, "forge-token", "", "Forge API token (default: GH_TOKEN, GITHUB_TOKEN, or gh auth token)") | ||
| cmd.Flags().StringVar(&route, "route", "", "Decide routing for one built-in agent (review) and print one JSON line with decision run, skip or bash; --output-driver is ignored") |
There was a problem hiding this comment.
8. Users cannot find the new route flag 📘 Rule violation ⚙ Maintainability
newDispatchCmd adds --route and introduces FULLSEND_CEL_ROUTING_REVIEW, but the general CLI references and repository-variable table are not updated, and the PR does not state that docs/contributing/documentation.md was consulted. Users consulting those references lack the flag’s usage and the variable’s behavior and defaults, while later maintainers lack a record that the documentation checklist was followed.
Agent Prompt
## Issue description
The new `fullsend dispatch --route review` flag and `FULLSEND_CEL_ROUTING_REVIEW` variable are missing from the required general documentation references, and the PR does not record consultation of `docs/contributing/documentation.md`.
## Fix Focus Areas
- internal/cli/dispatch.go[59-59]
- docs/guides/getting-started/operations.md[1-1]
- docs/guides/dev/cli-internals.md[1-1]
- docs/cli/README.md[1-1]
- docs/guides/infrastructure/infrastructure-reference.md[352-352]
## Recommended Fix
Document `--route review`, its output format, and its ignored output-driver semantics in the applicable CLI reference and CLI internals documentation. Add `FULLSEND_CEL_ROUTING_REVIEW` to the repository-variable table with its `off`, `shadow`, and `on` values, default behavior, and a link to the review-routing guide. Follow the documentation guide’s discovery and cross-reference requirements, and note in the PR description that the guide was consulted.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+117
to
+119
| harnessPath, deps, err := resolveAgentSource(ctx, absConfigDir, o.agent, forgeClient, cfg, composeOpts, printer) | ||
| if err != nil { | ||
| return d, err |
There was a problem hiding this comment.
9. Denied events still fetch review harnesses 🐞 Bug ➹ Performance
routeBuiltinAgent resolves and loads the harness before calling AuthorizeEvent, so authorization does not protect those fetches. An unauthorized PR or comment event can repeatedly fetch a configured URL source or remote base before receiving a skip decision, unlike ordinary dispatch, which authorizes before harness enumeration.
Agent Prompt
## Issue description
The route path performs potentially remote harness resolution before applying its event authorization gate.
## Fix Focus Areas
- internal/cli/dispatch_route.go[117-137]
## Recommended Fix
Apply event authorization before resolving or loading remote harness content where a denied event needs no harness. Preserve the no-trigger compatibility decision explicitly when reorganizing this flow.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+103
to
+107
| // Resolve forge: blocks for the event's forge, as `fullsend run` does. | ||
| switch o.event.Source.System { | ||
| case normevent.SystemGitHub, normevent.SystemGitLab: | ||
| composeOpts.ForgePlatform = string(o.event.Source.System) | ||
| } |
There was a problem hiding this comment.
10. Configured forge can block cel routing 🐞 Bug ≡ Correctness
routeBuiltinAgent chooses the harness forge platform from the event source instead of using config.forge precedence as fullsend run does. A repository configured for a different forge can therefore load a different overlay or fail a forge-specific harness load, making route mode fall back to bash rather than deciding against the harness that review execution uses.
Agent Prompt
## Issue description
Route mode derives the harness forge platform from the event even when repository configuration specifies a different platform for run mode.
## Fix Focus Areas
- internal/cli/dispatch_route.go[96-107]
- internal/cli/run.go[6338-6365]
## Recommended Fix
Select `ForgePlatform` with the same configuration and environment precedence used by `fullsend run`, then test route mode with `config.forge` and a forge-specific harness block.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+186
to
+194
| - name: Guard vendored install for CEL review route | ||
| if: steps.cel-config.outcome == 'success' && steps.cel-cli-mode.outputs.mode == 'vendored' | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
| if [[ ! -f cel-route/.fullsend/bin/fullsend ]]; then | ||
| echo "::error::broken --vendor install: vendored marker present but .fullsend/bin/fullsend is missing" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
3. A broken cli install in shadow mode stops all dispatch 🐞 Bug ☼ Reliability
None of the new install steps in the Route job have continue-on-error: not the CEL config checkout, the vendored guard (which runs exit 1 itself), either install step, the upstream action checkout, or the cache-artifact cp. Only "Evaluate CEL review route" and the cache save do. If any of those steps fails while the variable is shadow or on, every later step is skipped, including "Determine stage" (it runs only when earlier steps succeeded). The Route job then fails, so no stage runs (triage, code, fix, retro or review) for that PR or comment event, even though shadow is meant to be risk-free.
Agent Prompt
## Issue description
The CEL review-route install steps in the Route job can fail the whole job. When that happens "Determine stage" is skipped and no agent stage is dispatched, even in shadow mode.
## Fix Focus Areas
- .github/workflows/reusable-dispatch.yml[155-260]
## Recommended Fix
Add `continue-on-error: true` to every cel-* step: checkout, mode detect, vendored guard, cache restore, both installs, the upstream action checkout, expose and prepare. Gate "Evaluate CEL review route" on the install having succeeded, for example by checking that `command -v fullsend` works inside the step, so that "Determine stage" always runs and sees a non-success outcome. That outcome keeps the bash stage with ::warning:: in shadow or ::error:: in on.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+605
to
+610
| CEL_ROUTE_SOURCE="cel" | ||
| case "${CEL_REVIEW_DECISION}" in | ||
| run) STAGE="review" ;; | ||
| skip) STAGE="" ;; | ||
| *) CEL_ROUTE_SOURCE="bash" ;; | ||
| esac |
There was a problem hiding this comment.
11. A trigger can start review on a plain issue comment 🐞 Bug ≡ Correctness
In on mode, the run branch sets STAGE=review for any issue_comment without checking ISSUE_IS_PR, and the documented /fs-review trigger checks the command and actor but not whether the comment is on a pull request. A /fs-review comment on an ordinary issue can therefore reach the Review job with the issue’s URL and number as PR context, without a PR-head lookup or pull_request payload.
Agent Prompt
## Issue description
In `on` mode, a CEL `run` decision can select the PR-only Review job for a comment on an ordinary issue. The documented `/fs-review` example trigger also permits those comments.
## Fix Focus Areas
- .github/workflows/reusable-dispatch.yml[593-610]
- docs/guides/user/customizing-agents.md[760-769]
## Recommended Fix
In the `on` branch, map `run` to `STAGE=review` only for `pull_request_target` events or `issue_comment` events with `ISSUE_IS_PR == true`. Otherwise, log a warning and leave `STAGE` empty. Add `&& (event.entity.kind == "change_proposal" || has(event.entity.linked_change_proposal))` to the documented example trigger.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| with: | ||
| mode: upstream | ||
| workflow_repository: ${{ job.workflow_repository }} | ||
| workflow_sha: ${{ job.workflow_sha }} |
There was a problem hiding this comment.
13. Reviews can use a different trigger 🔗 Cross-repo conflict ≡ Correctness
The CEL route installs and caches the CLI at job.workflow_sha, while the Review job uses inputs.fullsend_version || job.workflow_sha; because the CLI build determines the agents ref, they can resolve different review-harness triggers. When a caller overrides fullsend_version, CEL can return run or skip for a trigger the executed harness does not have, return run where bash is appropriate, or fall back to bash if the workflow-SHA CLI does not support --route.
Agent Prompt
## Issue description
An explicit `fullsend_version` changes the release used by Review but not the CLI used for CEL routing, so routing can evaluate a different review harness or use a CLI without `--route`.
## Fix Focus Areas
- .github/workflows/reusable-dispatch.yml[198-260]
- .github/workflows/reusable-dispatch.yml[1216-1223]
## Recommended Fix
Install the route CLI using the same effective version as Review, `inputs.fullsend_version || job.workflow_sha`, and include that value in the route CLI cache key so it cannot reuse a binary built for another version. Test routing with a caller-supplied `fullsend_version`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Review skipped — this PR is already closed. The Posted by fullsend post-review check |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Lets the review harness
triggerdecide whether theReviewjob starts, behind the rollout variableFULLSEND_CEL_ROUTING_REVIEW(offdefault /shadow/on). TheReviewjob still runs the review unchanged. With the variable off or unset, routing is unchanged.Related Issue
Closes #8726. Part of #2888 (routing half of #2898).
Changes
fullsend dispatch --route reviewprints one JSON line{"agent","decision":"run|skip|bash","reason","harness"}. It resolves the review harness likefullsend run(config entry withsource:, else the agents-repo fallback at the lockstep ref) and loads it withLoadWithBase. A resolved harness with notrigger→bash(compatibility guard). Otherwise the event goes through the dispatch auth gate, then the trigger.AuthorizeEvent: the OWNERS-aware gate moved out ofDispatchso both paths share it. No behaviour change.Route job: new
cel-*steps, gated on the variable beingshadow/onand the event beingpull_request_targetorissue_comment. They do a sparse checkout intocel-route/(same trusted ref), install the CLI with theharness-dispatchcache key, and evaluate withcontinue-on-error. A guarded tail in "Determine stage" applies the decision:shadowbash=… cel=…, warns on a mismatch.onrun→STAGE=review,skip→STAGE="",bashkeeps the bash stage. Never replaces another bash stage. A CLI error keeps the bash stage with::error::.Behaviour step
the repository variable "<NAME>" is "<value>": restores the previous value at scenario cleanup.Docs:
customizing-agents.md: new section "Choosing when review runs".cel-triggers-reference.md: updated coexistence section.Not in this PR
harness-dispatchmatrix (fix(dispatch): don't fan out built-in agents from GitHub CEL dispatch #8725). Until it lands, areviewoverride with a trigger can also run viaharness-run.onreviewoverride with a trigger also runs viaharness-rungha-eventinput does not run the ADR 0107 bot-role lookup yet, so bot-authored PRs are skippedshadowhas no dependency.Known differences on the CEL path (from the parity test)
/fs-reviewcommenterwritecollaborator rolegha-eventinput does not run the ADR 0107 bot-role lookup yetissueslabeledready-for-reviewon a PRpull_request_targetandissue_commentrun the CEL step), so the bash stage is keptTesting
make lintpasses (pre-commit run --from-ref origin/main --to-ref HEAD)--route(each guard row, kill switch, disabled, auth denied, fallback unavailable, one-line stdout).gha-event→AuthorizeEvent→testdata/review-trigger.cel, compared with the bash decision.off/unset match a run without the CEL env.cel-*step depends on the variable./fs-review-only override, off, double-run check) follows once feat(dispatch): apply per-harness permission threshold on the CEL path #8724/fix(dispatch): don't fan out built-in agents from GitHub CEL dispatch #8725 merge.Checklist
!for breaking changes)🤖 Generated with Claude Code