fix(dispatch): distinguish permission-check failures from unauthorized skips#5219
fix(dispatch): distinguish permission-check failures from unauthorized skips#5219Roming22 wants to merge 1 commit into
Conversation
PR Summary by QodoLog skip reasons for comment-triggered dispatches
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Site previewPreview: https://84d85a8a-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}
8632175 to
1210e7d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1210e7d to
1956dc5
Compare
1956dc5 to
6d2813e
Compare
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 8:24 PM UTC · Completed 8:39 PM UTC |
ReviewFindingsHigh
Medium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
Roming22
changed the title
6d2813e to
c24376e
Compare
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review squad pass (3 agents: Claude coder, Claude researcher, Grok). Fail-closed behavior verified correct for all code paths — return 1 after esac is the load-bearing line. Both workflow files are structurally identical. 2 MEDIUM findings posted inline (3/3 agent consensus on both). The stale CHANGES_REQUESTED from fullsend-ai-review[bot] (commit 6d2813e) is resolved in the current head — both files now use case + return 1.
Assisted-by: Claude (review), Grok (review)
| # membership regardless of visibility (private vs public). | ||
| # See: github/gh-aw-mcpg#2862 | ||
| # Returns 0 if username has write access, 1 if not, 2 on operational failure | ||
| # (mktemp/gh api errors). Callers must distinguish 1 vs 2 when messaging. |
There was a problem hiding this comment.
[MEDIUM] Event-actor auth callers don't distinguish exit codes, violating the new docstring contract
The new docstring declares "Callers must distinguish 1 vs 2 when messaging." But is_event_actor_authorized() is still called in plain boolean if context at 3 call sites (lines ~278, ~282, ~301 in this file; same in scaffold), emitting no skip notices and treating exit codes 1 and 2 identically:
if is_event_actor_authorized "${ISSUE_USER_LOGIN}"; then
STAGE="triage"
fiFail-closed is correct — dispatch is blocked for all non-zero. The issue is observability: when has_write_permission returns 2 (API failure) on the event-triggered path, there's zero audit trail, unlike the comment path which now logs distinct notices. The docstring creates a false expectation that all callers handle the distinction.
Suggestion: Either (a) narrow the docstring to "Comment-path callers should distinguish 1 vs 2 for observability; event-path callers currently treat all non-zero as skip", or (b) create an event_from_authorized_actor() wrapper with case-based notices matching comment_from_authorized_user().
Flagged by 3 agents (Claude x2, Grok) — consensus
| ;; | ||
| esac | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
[LOW] Missing * catch-all in case statement for unexpected exit codes (downgraded from MEDIUM after July 20 re-review at head afebea02 — all 3 agents now assess LOW)
case "${auth_rc}" handles 0, 1, and 2 but has no * default. Unexpected exit codes (e.g., 127 for gh not in PATH, 128+N for signal kill) fall through to return 1 with no notice — correctly fail-closed, but a silent skip in miniature. Downgraded because both failure arms in has_write_permission are caught by || { ...; return 2; }, so codes other than 0/1/2 cannot occur today; the exposure is limited to future refactors regressing silently.
Same pattern in the scaffold copy at internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml.
Suggestion: Add a catch-all before esac:
*)
echo "::warning::Unexpected auth exit code ${auth_rc} for ${COMMENT_USER_LOGIN}" >&2
;;Previous review (superseded)
[MEDIUM] Missing * catch-all in case statement for unexpected exit codes
case "${auth_rc}" handles 0, 1, and 2 but has no * default. Unexpected exit codes (e.g., 127 for gh not in PATH, 128+N for signal kill) fall through to return 1 with zero audit trail. The return 1 after esac correctly maintains fail-closed, but the exact observability gap the PR is trying to fix (distinct notices per denial reason) repeats for codes > 2.
Same pattern in the scaffold copy at internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml.
Suggestion: Add a catch-all before esac:
*)
echo "::warning::Unexpected auth exit code ${auth_rc} for ${COMMENT_USER_LOGIN}" >&2
;;Flagged by 3 agents (Claude x2, Grok) — consensus
…d skips Return exit code 2 from has_write_permission on mktemp/API failures so callers do not treat check errors as unauthorized. Emit matching skip notices in comment_from_authorized_user and keep both dispatch workflows aligned. Signed-off-by: Romain Arnaud <rarnaud@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
c24376e to
afebea0
Compare
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review squad pass (3 agents: Claude coder, Claude researcher, Grok) against head afebea02. Fail-closed behavior re-verified correct on every path — an operational failure (exit 2) can never result in a dispatch, and the is_authorized || auth_rc=$? capture is safe under set -euo pipefail in if-condition context. Both workflow copies are identical modulo the pre-existing ISSUE_IS_PR/ISSUE_HAS_PR naming drift.
Posting 2 new MEDIUM findings inline (3/3 agent consensus on both). Not re-posted: the event-path 1-vs-2 gap from the July 17 round still applies unchanged at this head; the missing * catch-all comment has been updated in place — all 3 agents now assess it LOW.
| echo "::warning::Permission API call failed for ${username}: $(cat "${api_err}")" >&2 | ||
| rm -f "${api_err}" | ||
| return 1 | ||
| return 2 |
There was a problem hiding this comment.
[MEDIUM] premature-decision: HTTP 404 from the permission API lands in exit 2 "operational failure", conflating an authorization-relevant outcome
The docstring asserts every gh api failure is operational, but the endpoint's error taxonomy says otherwise. Verified empirically against the live API:
- Real non-collaborator on a public repo → HTTP 200,
role_name: "read"→ correctly exit 1 - Real non-collaborator on a private repo → HTTP 200,
role_name: ""→ correctly exit 1 - Nonexistent user (deleted/renamed account between comment and workflow run) → HTTP 404 →
ghexits non-zero → exit 2
So the common unauthorized case is classified correctly, but a deleted/renamed account gets the "failed to verify permissions" notice, which suggests a retryable infra problem when it is not. Fail-closed either way — message accuracy only.
Suggestion: Match HTTP 404 in the captured stderr and return 1 for it, reserving return 2 for network/5xx/auth errors and the mktemp path — or note in the function comment that unknown-user 404s land in exit 2 by design.
Same pattern in internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml:72-77.
Flagged by 3 agents (Claude x2, Grok) — consensus
| assert.Contains(t, s, `Skipping dispatch for bot comment`) | ||
| assert.Contains(t, s, `Skipping dispatch for unauthorized comment`) | ||
| assert.Contains(t, s, `Skipping dispatch: failed to verify permissions`) | ||
| assert.Contains(t, s, `return 2`) |
There was a problem hiding this comment.
[MEDIUM] Test assertions pin strings anywhere in the file, not the exit-code behavior being fixed
All new assertions are file-wide assert.Contains checks. assert.Contains(t, s, "return 2") passes with the string anywhere in the rendered YAML — a regression like 0|2) return 0 ;; (treating operational failure as authorized) would leave every assertion green, and the notice strings would too. Nothing executes the routing script, so the exact bug class this PR fixes (wrong exit-code mapping) would not be caught if reintroduced. Additionally, only the scaffold copy is covered — .github/workflows/reusable-dispatch.yml carries an identical hand-mirrored routing block with no drift guard.
Suggestion: Assert on compound snippets that tie exit codes to their branches (e.g. the literal is_authorized || auth_rc=$? line plus a 2) arm adjacent to its notice), and/or drive the extracted script with a fake gh on PATH asserting emitted notices and STAGE for rc 0/1/2. A normalizing drift test comparing the shared helper region of both workflow files (analogous to the existing scan-secrets sync check) would close the duplication gap.
Flagged by 3 agents (Claude x2, Grok) — consensus; severity settled at MEDIUM (assessed HIGH by one agent, LOW by another)
2 participants
Summary
has_write_permissiondistinct exit codes:1for insufficient permission,2for operational failures (mktemp/gh api), so callers do not treat check failures as unauthorized.comment_from_authorized_userin both dispatch workflows to emit the correct skip notice for each case (and fail closed on unexpected codes).case-based handling).Related Issue
N/A
Changes
has_write_permission()returns2on mktemp/API failures (still returns1for no write access).comment_from_authorized_user()inspects the auth exit code and logs either unauthorized or permission-verification-failure notices..github/workflows/reusable-dispatch.ymlandinternal/scaffold/fullsend-repo/.github/workflows/dispatch.yml.Testing
make lintpasses (stage changes first, then run)Checklist
!for breaking changes)