Skip to content

fix(dispatch): distinguish permission-check failures from unauthorized skips #5219

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Roming22 wants to merge 1 commit into from
+87 −21
Closed

fix(dispatch): distinguish permission-check failures from unauthorized skips #5219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 39 additions & 9 deletions .github/workflows/reusable-dispatch.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,8 @@ jobs:
# Uses the collaborator permission API which correctly resolves org
# 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.

@waynesun09 waynesun09 Jul 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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"
fi

Fail-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

has_write_permission() {
local username="${1:-}"
if [[ -z "${username}" ]]; then
Expand All @@ -137,13 +139,13 @@ jobs:
local role api_err
api_err=$(mktemp) || {
echo "::warning::Failed to create temp file for permission check of ${username}" >&2
return 1
return 2
}
role=$(gh api "repos/${GITHUB_REPOSITORY}/collaborators/${username}/permission" \
--jq '.role_name' 2>"${api_err}") || {
echo "::warning::Permission API call failed for ${username}: $(cat "${api_err}")" >&2
rm -f "${api_err}"
return 1
return 2

@waynesun09 waynesun09 Jul 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 → gh exits 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

}
rm -f "${api_err}"
case "${role}" in
Expand Down Expand Up @@ -176,6 +178,34 @@ jobs:
return 1
}

# Helper: check if the comment is from a user
comment_from_user() {
if [[ "${COMMENT_USER_TYPE}" == "Bot" ]]; then
echo "::notice::Skipping dispatch for bot comment"
return 1
fi
return 0
}

# Helper: check if the comment is from an authorized user
comment_from_authorized_user() {
if ! comment_from_user; then
return 1
fi
local auth_rc=0
is_authorized || auth_rc=$?
case "${auth_rc}" in
0) return 0 ;;
1)
echo "::notice::Skipping dispatch for unauthorized comment from ${COMMENT_USER_LOGIN}"
;;
2)
echo "::notice::Skipping dispatch: failed to verify permissions for ${COMMENT_USER_LOGIN}"
;;
esac
return 1
}

@waynesun09 waynesun09 Jul 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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


COMMAND=""
if [[ -n "${COMMENT_BODY:-}" ]]; then
COMMAND="$(printf '%s\n' "${COMMENT_BODY}" | head -1 | tr -d '\r' | awk '{print $1}')"
Expand All @@ -185,34 +215,34 @@ jobs:
issue_comment)
case "${COMMAND}" in
/fs-triage)
if [[ "${COMMENT_USER_TYPE}" != "Bot" ]] && is_authorized; then
if comment_from_authorized_user; then
STAGE="triage"
fi
;;
/fs-code)
if [[ "${ISSUE_IS_PR}" == "false" ]]; then
if [[ "${COMMENT_USER_TYPE}" != "Bot" ]] && is_authorized; then
if comment_from_authorized_user; then
STAGE="code"
fi
fi
;;
/fs-review)
if [[ "${ISSUE_IS_PR}" == "true" ]]; then
if [[ "${COMMENT_USER_TYPE}" != "Bot" ]] && is_authorized; then
if comment_from_authorized_user; then
STAGE="review"
fi
fi
;;
/fs-fix)
if [[ "${ISSUE_IS_PR}" == "true" ]]; then
if [[ "${COMMENT_USER_TYPE}" != "Bot" ]] && is_authorized; then
if comment_from_authorized_user; then
STAGE="fix"
TRIGGER_SOURCE="${COMMENT_USER_LOGIN}"
fi
fi
;;
/fs-retro|/fullsend)
if [[ "${COMMENT_USER_TYPE}" != "Bot" ]] && is_authorized; then
if comment_from_authorized_user; then
if [[ "${COMMAND}" == "/fullsend" ]]; then
SECOND_WORD="$(printf '%s\n' "${COMMENT_BODY}" | head -1 | tr -d '\r' | awk '{print $2}')"
if [[ "${SECOND_WORD}" == "retro" ]]; then
Expand All @@ -224,7 +254,7 @@ jobs:
fi
;;
/fs-prioritize)
if [[ "${COMMENT_USER_TYPE}" != "Bot" ]] && is_authorized; then
if comment_from_authorized_user; then
STAGE="prioritize"
fi
;;
Expand All @@ -233,7 +263,7 @@ jobs:
# re-trigger triage by providing clarification on needs-info
# issues. Full write-permission check is not required here.
if has_label "needs-info" && ! has_label "feature"; then
if [[ "${COMMENT_USER_TYPE}" != "Bot" ]]; then
if comment_from_user; then
if [[ "${COMMENT_AUTHOR_ASSOC}" != "NONE" ]] || is_issue_author; then
STAGE="triage"
fi
Expand Down
50 changes: 40 additions & 10 deletions internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
# lint-workflow-size: max-lines=500
# lint-workflow-size: max-lines=550
# Dispatcher workflow that routes events to agent workflows based on stage.
# Routing logic determines the stage from event context — the shim only
# forwards the raw event. Adding a new stage requires only a case branch
Expand Down Expand Up @@ -57,6 +57,8 @@ jobs:
# Uses the collaborator permission API which correctly resolves org
# 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.
has_write_permission() {
local username="${1:-}"
if [[ -z "${username}" ]]; then
Expand All @@ -65,13 +67,13 @@ jobs:
local role api_err
api_err=$(mktemp) || {
echo "::warning::Failed to create temp file for permission check of ${username}" >&2
return 1
return 2
}
role=$(gh api "repos/${GITHUB_REPOSITORY}/collaborators/${username}/permission" \
--jq '.role_name' 2>"${api_err}") || {
echo "::warning::Permission API call failed for ${username}: $(cat "${api_err}")" >&2
rm -f "${api_err}"
return 1
return 2
}
rm -f "${api_err}"
case "${role}" in
Expand Down Expand Up @@ -107,6 +109,34 @@ jobs:
return 1
}

# Helper: check if the comment is from a user
comment_from_user() {
if [[ "${COMMENT_USER_TYPE}" == "Bot" ]]; then
echo "::notice::Skipping dispatch for bot comment"
return 1
fi
return 0
}

# Helper: check if the comment is from an authorized user
comment_from_authorized_user() {
if ! comment_from_user; then
return 1
fi
local auth_rc=0
is_authorized || auth_rc=$?
case "${auth_rc}" in
0) return 0 ;;
1)
echo "::notice::Skipping dispatch for unauthorized comment from ${COMMENT_USER_LOGIN}"
;;
2)
echo "::notice::Skipping dispatch: failed to verify permissions for ${COMMENT_USER_LOGIN}"
;;
esac
return 1
}

# Extract the first word of the comment as the command
COMMAND=""
if [[ -n "${COMMENT_BODY:-}" ]]; then
Expand All @@ -117,34 +147,34 @@ jobs:
issue_comment)
case "${COMMAND}" in
/fs-triage)
if [[ "${COMMENT_USER_TYPE}" != "Bot" ]] && is_authorized; then
if comment_from_authorized_user; then
STAGE="triage"
fi
;;
/fs-code)
if [[ "${ISSUE_HAS_PR}" == "false" ]]; then
if [[ "${COMMENT_USER_TYPE}" != "Bot" ]] && is_authorized; then
if comment_from_authorized_user; then
STAGE="code"
fi
fi
;;
/fs-review)
if [[ "${ISSUE_HAS_PR}" == "true" ]]; then
if [[ "${COMMENT_USER_TYPE}" != "Bot" ]] && is_authorized; then
if comment_from_authorized_user; then
STAGE="review"
fi
fi
;;
/fs-fix)
if [[ "${ISSUE_HAS_PR}" == "true" ]]; then
if [[ "${COMMENT_USER_TYPE}" != "Bot" ]] && is_authorized; then
if comment_from_authorized_user; then
STAGE="fix"
TRIGGER_SOURCE="${COMMENT_USER_LOGIN}"
fi
fi
;;
/fs-retro|/fullsend)
if [[ "${COMMENT_USER_TYPE}" != "Bot" ]] && is_authorized; then
if comment_from_authorized_user; then
if [[ "${COMMAND}" == "/fullsend" ]]; then
SECOND_WORD="$(printf '%s\n' "${COMMENT_BODY}" | head -1 | tr -d '\r' | awk '{print $2}')"
if [[ "${SECOND_WORD}" == "retro" ]]; then
Expand All @@ -156,7 +186,7 @@ jobs:
fi
;;
/fs-prioritize)
if [[ "${COMMENT_USER_TYPE}" != "Bot" ]] && is_authorized; then
if comment_from_authorized_user; then
STAGE="prioritize"
fi
;;
Expand All @@ -165,7 +195,7 @@ jobs:
# re-trigger triage by providing clarification on needs-info
# issues. Full write-permission check is not required here.
if has_label "needs-info" && ! has_label "feature"; then
if [[ "${COMMENT_USER_TYPE}" != "Bot" ]]; then
if comment_from_user; then
if [[ "${COMMENT_AUTHOR_ASSOC}" != "NONE" ]] || is_issue_author; then
STAGE="triage"
fi
Expand Down
10 changes: 8 additions & 2 deletions internal/scaffold/scaffold_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,9 +224,15 @@ func TestDispatchWorkflowContent(t *testing.T) {
assert.Contains(t, s, `COMMENT_AUTHOR_ASSOC`)
// Auto-triage requires assoc != NONE or issue author
assert.Contains(t, s, "is_issue_author")
// Bot filtering
// Bot filtering and skip notices via shared helpers
assert.Contains(t, s, `COMMENT_USER_TYPE`)
assert.Contains(t, s, `!= "Bot"`)
assert.Contains(t, s, `comment_from_user`)
assert.Contains(t, s, `comment_from_authorized_user`)
assert.Contains(t, s, `== "Bot"`)
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`)

@waynesun09 waynesun09 Jul 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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)

// No-fix label check (uses PR_LABELS for pull_request_review events)
assert.Contains(t, s, "fullsend-no-fix")
assert.Contains(t, s, "PR_LABELS")
Expand Down
Loading