refactor(dispatch): require PR context for review triggers - #2473
Conversation
Review and fix agents need PR context. Drop the issues labeled ready-for-review route and gate /fs-review to PR comments only, leaving pull_request_target and pull_request_review auto-triggers unchanged. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
PR Summary by QodoRequire PR context for review dispatch triggers Description
Diagram
High-Level Assessment
Files changed (9)
|
Site previewPreview: https://ac16fb6a-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 3:26 PM UTC · Completed 3:39 PM UTC |
Code Review by Qodo
1.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous runReviewFindingsHigh
Medium
Low
Previous run (2)ReviewFindingsHigh
Low
Previous run (3)ReviewFindingsHigh
Low
Previous run (4)ReviewFindingsHigh
Low
Previous run (5)ReviewFindingsHigh
Medium
Low
Previous run (6)ReviewFindingsHigh
Medium
Low
Previous run (7)ReviewFindingsHigh
Low
Previous run (8)ReviewFindingsHigh
Medium
Low
Previous run (9)ReviewFindingsHigh
Medium
Low
Labels: PR modifies dispatch workflow routing and updates multiple documentation files. Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsHigh
Medium
Low
Previous run (3)ReviewFindingsHigh
Low
Previous run (4)ReviewFindingsHigh
Low
Previous run (5)ReviewFindingsHigh
Low
Previous run (6)ReviewFindingsHigh
Medium
Low
Previous run (7)ReviewFindingsHigh
Medium
Low
Previous run (8)ReviewFindingsHigh
Low
Previous run (9)ReviewFindingsHigh
Medium
Low
Previous run (10)ReviewFindingsHigh
Medium
Low
Labels: PR modifies dispatch workflow routing and updates multiple documentation files. |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
fullsend-ai-review
Bot
added
component/dispatch
Restore the ready-for-review label dispatch route gated on ISSUE_HAS_PR, matching the /fs-review PR-context guard. Standalone issues no longer enqueue review; PR label application still can. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 5:52 AM UTC · Completed 6:04 AM UTC |
…drop-issue-review-fix-triggers Signed-off-by: Barak Korren <bkorren@redhat.com>
Tighten review agent doc wording per review, clarify ADR notes that fix dispatch was already PR-only, and annotate ADR 0002. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 6:51 AM UTC · Completed 7:04 AM UTC |
|
🤖 Finished Review · ✅ Success · Started 3:06 PM UTC · Completed 3:18 PM UTC |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
[medium] test-regex-fragility
The (?s)/fs-review\).*?ISSUE_IS_PR and (?s)ready-for-review.*?ISSUE_IS_PR regexes assert that ISSUE_IS_PR appears somewhere after the trigger string in the file — not that it gates the specific case block. If the /fs-review case block lost its gate but ISSUE_IS_PR still appeared later (e.g., in the /fs-fix block), the test would false-pass. Consider tightening to match within the case block structure, e.g.:
assert.Regexp(t, `(?s)/fs-review\)\s*\n\s+if \[\[ "\$\{ISSUE_IS_PR\}"`, s)
assert.Regexp(t, `(?s)ready-for-review"\s*\]\];\s*then\s*\n\s+if \[\[ "\$\{ISSUE_IS_PR\}"`, s)Routing logic and docs LGTM otherwise.
Assisted-by: Claude (review), Gemini (review)
Match ISSUE_IS_PR guards within the /fs-review and ready-for-review case blocks so the test cannot false-pass via a later reference. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 6:16 AM UTC · Completed 6:29 AM UTC |
| STAGE="code" | ||
| fi | ||
| ;; | ||
| /fs-review) |
There was a problem hiding this comment.
[low] silent-skip
When /fs-review is issued on an issue without a linked PR, STAGE remains empty and the workflow silently skips dispatch. The user receives no feedback. This is consistent with existing patterns (/fs-code silently skips when ISSUE_IS_PR is true), so this is not a regression.
| } | ||
| } | ||
|
|
||
| // TestReusableDispatchRoutingContent validates PR-context gating in per-repo |
There was a problem hiding this comment.
[low] test-naming-inconsistency
TestReusableDispatchRoutingContent validates routing content via regex matching but lives in workflow_call_alignment_test.go, which contains structural validation tests. Content validation tests (e.g., TestDispatchWorkflowContent) live in scaffold_test.go.
Suggested fix: Move this test to scaffold_test.go to align with the established pattern, or rename to reflect structural validation purpose.
fullsend-ai-review
Bot
added
the
requires-manual-review
Signed-off-by: Barak Korren <bkorren@redhat.com>
Align with TestDispatchWorkflowContent naming convention per review. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 7:02 AM UTC · Completed 7:17 AM UTC |
|
|
||
| // TestReusableDispatchWorkflowContent validates PR-context gating in per-repo | ||
| // reusable-dispatch.yml routing (per-org dispatch.yml unchanged). | ||
| func TestReusableDispatchWorkflowContent(t *testing.T) { |
There was a problem hiding this comment.
[low] test-naming-inconsistency
TestReusableDispatchWorkflowContent validates workflow content via regex matching but lives in workflow_call_alignment_test.go, which contains structural validation tests. Content validation tests (TestDispatchWorkflowContent) live in scaffold_test.go.
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Finished Retro · ✅ Success · Started 7:55 AM UTC · Completed 8:04 AM UTC |
Retro: PR #2473 — refactor(dispatch): require PR context for review triggersWhat happenedHuman-authored PR (ifireball + Cursor) modifying 9 files (44 additions, 17 deletions) to gate review dispatch on PR context in per-repo mode. The review bot ran 8 times over 2 days:
The PR was merged Jun 23 after the bot's verdict finally softened. AssessmentThe core issues — bot over-requesting changes on human PRs with only medium/low findings, not incorporating human approval signals, and re-raising the same unresolvable findings — are well-covered by the existing issue backlog. The most impactful existing issues for this pattern are:
Positive notes
ProposalsNo new proposals — the improvement opportunities are already tracked by existing issues. Prioritizing #2115 and #1551 would have the highest impact on this class of PR. |
Signed-off-by: Barak Korren <bkorren@redhat.com>
…e-review-fix-triggers refactor(dispatch): require PR context for review triggers
Signed-off-by: Barak Korren <bkorren@redhat.com>
5 participants
Summary
Part of #981 (per-repo only). Require PR context for review dispatch in per-repo installs via
reusable-dispatch.yml. Per-orgdispatch.ymlis intentionally unchanged — per-org mode is being retired (ADR ending that work); per-repo is the supported path going forward./fs-reviewonISSUE_IS_PR(PR comments only; preserves stale-head re-dispatch inpost-review.sh)ready-for-reviewlabel onISSUE_IS_PR(PR label application only; standalone issues skipped)pull_request_reviewand PR-gated/fs-fix/fs-revieworready-for-reviewlabelpull_request_target;ready-for-reviewon a PR;/fs-reviewon PR commentspull_request_review;/fs-fixon PR commentsManual re-review without a push: apply
ready-for-reviewto the PR or post/fs-reviewon the PR.ADR annotations
Test plan
go test ./internal/scaffold/...make lintready-for-reviewis applied to a standalone issueready-for-reviewon a PR dispatches review/fs-reviewon a PR comment still dispatches reviewpull_request_targeton PR open/push