feat(dispatch): allow triage role for observation stages - #5244
Conversation
Extend has_repo_permission so triage-role collaborators can trigger triage and review dispatch without gaining code/fix (write+) access. Closes #5223 Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 2:25 PM UTC · Completed 2:39 PM UTC |
PR Summary by QodoDispatch: allow GitHub triage role for observation stages
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1.
|
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
|
Block triage→fix escalation via review-bot changes_requested on human PRs, restore the 475-line workflow size cap, and align ADR 0054 Consequences with stage-dependent auth. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · ❌ Terminated · Started 4:26 PM UTC · Ended 4:45 PM UTC |
Site previewPreview: https://3538b882-site.fullsend-ai.workers.dev Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad — 3 verified findings (MEDIUM)
Agents: 3 (Claude ×2, Grok) · Models: Claude, Grok
1. MEDIUM — Label-triggered ready-to-code bypasses per-stage authorization boundary
File: .github/workflows/reusable-dispatch.yml:255-256 (and scaffold equivalent)
The issues.labeled path sets STAGE="code" (mutation stage) when ready-to-code is applied, with no permission check. ADR 0054's Consequences section now claims "All dispatch paths require repository permission at a stage-appropriate minimum," but GitHub's triage role can apply labels — a triage-role collaborator could apply ready-to-code to trigger the code agent without write-level permission.
Pre-existing on main — unchanged by this PR — but the updated ADR language makes the gap visible.
Suggestion: Add is_event_actor_authorized "${EVENT_SENDER_LOGIN}" write to the ready-to-code labeled path, or update the ADR to acknowledge that label-triggered paths use a different authorization model. The ready-for-triage / ready-for-review label paths are fine (observation stages).
Flagged by: Claude (×2), Grok — consensus
2. MEDIUM — PR claims Closes #5154 but does not implement requested configurability
#5154 is titled "Support configurable minimum permission level for /fs-review in reusable dispatch" and proposes per-repo opt-in configurability (Option A). This PR implements a hardcoded platform-level floor (Option B). While Option B is valid, it locks every consumer repo into the triage floor for observation stages with no way to keep a stricter write-only gate.
Suggestion: Either keep #5154 open for the configurability aspect (only close #5223), or comment on #5154 explaining why Option B was chosen and whether per-repo override is planned as follow-up.
Flagged by: Claude (researcher) — single agent
3. MEDIUM — Retro stage on PR close has no permission check
File: .github/workflows/reusable-dispatch.yml:279
pull_request_target.closed unconditionally sets STAGE="retro" — any user who closes a PR triggers a retro run, including fork PR authors with no repo permission. While retro is read-only, it consumes API quota / LLM tokens and is inconsistent with the ADR's "all paths authorized" claim.
Pre-existing on main — unchanged by this PR.
Suggestion: Add if is_event_actor_authorized "${EVENT_SENDER_LOGIN}" triage; then STAGE="retro"; fi, or document this as an intentional exception.
Flagged by: Grok — single agent
Assisted-by: Claude (review), Grok (review)
|
🤖 Finished Review · ❌ Failure · Started 4:26 PM UTC · Completed 4:45 PM UTC |
Require write+ (or bot) for ready-to-code label dispatch, document ungated retro-on-close as intentional, and restore ADR 0054 Consequences wording per immutability. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
@waynesun09 Addressed your Review Squad findings in 492e9ac:
Also reverted the ADR Consequences rewrite per ADR immutability, and restored the collaborator-API rationale comment. |
|
Merged latest E2E failure on the previous run ( |
|
🤖 Finished Review · ✅ Success · Started 5:33 PM UTC · Completed 5:52 PM UTC |
|
CI after merging |
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad — 1 new finding (MEDIUM)
Agents: 4 (Claude ×2, Grok, Codex) · Models: Claude, Grok, Codex
Consensus: 3/4 agents flagged the test precision issue independently
All 6 prior findings (3 from review squad round 1, 3 from qodo) confirmed addressed — not re-raised.
Guard bot bypass and write-default min so the mutation-stage label gate cannot silently regress to triage. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 6:49 PM UTC · Completed 7:04 PM UTC |
waynesun09
left a comment
There was a problem hiding this comment.
Authorization boundary is correct — observation stages at triage+, mutation stages at write+, bot handoffs preserved, fail-closed on errors. All prior findings (11) addressed across 5 review rounds.
One remaining nit: the ADR 0054 Consequences rewrite ("write-level" → "write-level for mutation, triage+ for observation") is a substantive edit to an Accepted ADR per AGENTS.md policy. The Note annotation above Consequences already documents the evolution correctly — consider reverting the bullet to its original wording. Not blocking.
Assisted-by: Claude (review), Grok (review), Codex (review)
Resolve workflow_call_alignment_test.go conflict by keeping both per-stage auth and PR-head resolution tests. Revert ADR 0054 Consequences wording per AGENTS.md immutability (Note remains). Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
@waynesun09 Addressed your latest nit in 99d840d:
|
|
🤖 Finished Review · ✅ Success · Started 8:29 PM UTC · Completed 8:46 PM UTC |
|
🤖 Finished Retro · ✅ Success · Started 9:19 PM UTC · Completed 9:34 PM UTC |
Retro: PR #5244 — Allow triage role for observation stagesPR lifecycle: 8 review agent runs (5 success, 2 failure), 3 human review rounds, 11 findings all resolved, merged after ~3 days. What the review agent did well
Key gap: missed the two most critical security findingsThe review agent ran 8 times but never flagged the PR's two privilege escalation paths — the highest-impact findings in the entire review:
Both required reasoning across multiple dispatch routing branches rather than analyzing each changed line in isolation. The review agent's security sub-agent analyzed the Severity calibration gapThe review agent flagged the implicit Infrastructure failures
Evidence for existing issues (not filing new proposals)
Proposals filed
|
Summary
has_repo_permission(user, min)so GitHub Triage role can trigger observation stages (/fs-triage, auto-triage,/fs-review, auto-review) without gaining mutation access (/fs-code,/fs-fixstay write+)reusable-dispatch.ymland scaffolddispatch.ymlready-to-codeto write+ labelers (bots still allowed for agent handoff)pull_request_target.closed→ retroCloses #5223
Related: #5154 (per-repo configurable min permission left open — this PR hardcodes the triage floor for observation stages)
Test plan
go test ./internal/scaffold/ -run 'TestDispatchPerStageAuthorization|TestDispatchWorkflowContent'make lint(after bootstrap pinact/lychee)/fs-triageand open issues/PRs that auto-dispatch/fs-code,/fs-fix, or trigger code viaready-to-code