Skip to content

feat(#1662): ADR 0054 — implement is_authorized on all agent dispatch paths - #1688

Merged
ascerra merged 21 commits into
mainfrom
agent/1662-adr-auth-slash-commands
Jun 24, 2026
Merged

feat(#1662): ADR 0054 — implement is_authorized on all agent dispatch paths#1688
ascerra merged 21 commits into
mainfrom
agent/1662-adr-auth-slash-commands

Conversation

@fullsend-ai-coder

@fullsend-ai-coder fullsend-ai-coder Bot commented May 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements ADR 0054: all agent dispatch paths now require write-level repository permission before dispatching.

Authorization mechanism

Both slash commands (is_authorized) and event triggers (is_event_actor_authorized) delegate to a shared has_write_permission() helper that calls the collaborator permission API (GET /repos/{owner}/{repo}/collaborators/{username}/permission) and checks .role_name. Users with admin, maintain, or write role are authorized.

This replaces the previous author_association-based check which was unreliable for users with private org membership (github/gh-aw-mcpg#2862).

Changes

  • reusable-dispatch.yml + scaffold dispatch.yml: unified authorization via has_write_permission() using .role_name
  • Gated all previously ungated paths: /fs-triage, /fs-code, /fs-review, issues.opened/edited, pull_request_target.opened/synchronize
  • ADR 0054: accepted, documents the decision, rationale, and mechanism
  • Agent docs (code.md, fix.md, review.md, retro.md, triage.md, prioritize.md): updated authorization requirement
  • architecture.md: added ADR 0054 reference to slash-command parser section
  • bugfix-workflow.md, e2e-testing.md: updated authorization descriptions

Design notes

  • The needs-info re-triage path intentionally uses a weaker gate (author_association != NONE) to allow external reporters to provide clarification
  • Bot-to-bot handoffs are unaffected (they use label-based triggers which implicitly require write access)
  • API errors are logged via ::warning:: annotations (fail-closed)

Closes #1662

@github-actions

github-actions Bot commented May 29, 2026

Copy link
Copy Markdown

Site preview

Preview: https://91a07774-site.fullsend-ai.workers.dev

Commit: a2628e1795f8324bd30213167f4df781e5188f9e

@fullsend-ai-review

fullsend-ai-review Bot commented May 29, 2026

Copy link
Copy Markdown

Review

Findings

Medium

  • [protected-path] .github/workflows/reusable-dispatch.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The change is justified by linked issue ADR: require is_authorized check on all agent slash commands #1662 and ADR 0054, but human review is always required for protected-path changes regardless of context.

  • [logic-error] .github/workflows/reusable-dispatch.yml:63 — The route job permissions are contents: read and pull-requests: read. The new has_write_permission() function calls the collaborator permission API (GET /repos/{owner}/{repo}/collaborators/{username}/permission), which may require broader token scopes for private repositories. The function is fail-closed (returns 1 on API failure, emits ::warning::), so insufficient permissions would block all authorized users from dispatching rather than allowing unauthorized access. Same concern applies to the scaffold dispatch.yml.
    Remediation: Verify empirically in a test run that the collaborator permission endpoint is accessible with the current GITHUB_TOKEN scopes across public and private repos.

  • [intent-alignment] — The PR scope extends beyond issue ADR: require is_authorized check on all agent slash commands #1662's request to "require is_authorized on all agent slash commands." In addition to gating ungated paths, the PR replaces the author_association-based authorization mechanism with the collaborator permission API and gates automatic event triggers (issues.opened/edited, pull_request_target.opened/synchronize). The scope expansion is documented and justified in ADR 0054 (private org membership bug in author_association), but represents a broader architectural change than the issue authorized.

  • [naming-consistency] internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml:39 — Pre-existing variable naming inconsistency: the scaffold dispatch.yml uses ISSUE_HAS_PR while reusable-dispatch.yml uses ISSUE_IS_PR for the identical purpose. Per AGENTS.md, these files must share identical routing logic. This inconsistency predates the PR but is worth addressing since both files are being modified. Same inconsistency in the test at scaffold_test.go:206.
    Remediation: Standardize on ISSUE_IS_PR in the scaffold dispatch.yml and update the test assertion.

  • [stale-documentation] docs/superpowers/plans/2026-05-04-retro-agent.md:959 — Plan document contains an outdated authorization pattern using author_association == 'OWNER'|'MEMBER'|'COLLABORATOR' in the dispatch-retro-command job example. With the migration to the collaborator permission API (has_write_permission with admin|maintain|write), this example no longer reflects the current authorization mechanism.
    Remediation: Update the authorization example in Task 10 to reflect the new has_write_permission() pattern, or add a note that the snippet is simplified.

Previous run

Review

Findings

Medium

  • [protected-path] .github/workflows/reusable-dispatch.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The change is justified by linked issue ADR: require is_authorized check on all agent slash commands #1662 and ADR 0054, but human review is always required for protected-path changes regardless of context.

  • [logic-error] .github/workflows/reusable-dispatch.yml:63 — The route job permissions are contents: read and pull-requests: read. The new has_write_permission() function calls the collaborator permission API (GET /repos/{owner}/{repo}/collaborators/{username}/permission), which may require token scopes beyond what is declared. The function is fail-closed (returns 1 on API failure, emits a ::warning::), so insufficient permissions would block all authorized users from dispatching rather than allowing unauthorized access. The github.token likely has sufficient access for same-repo collaborator queries, but this should be verified empirically for both public and private repos. Same concern applies to the scaffold dispatch.yml.
    Remediation: Verify empirically in a test run that the collaborator permission endpoint is accessible with the current GITHUB_TOKEN scopes across all deployment modes.

Low

  • [test-inadequate] internal/scaffold/scaffold_test.go:209 — The test does not assert that GH_TOKEN or EVENT_SENDER_LOGIN are present in the dispatch workflow env block. Without GH_TOKEN, gh api would fail at runtime (fail-closed). Without EVENT_SENDER_LOGIN, issues.edited authorization would use an empty string (also fail-closed). A regression removing either would not be caught by tests. Same pattern in the scaffold dispatch.yml.
    Remediation: Add assert.Contains(t, s, "GH_TOKEN") and assert.Contains(t, s, "EVENT_SENDER_LOGIN") to TestDispatchWorkflowContent.

  • [authorization-gap] .github/workflows/reusable-dispatch.yml — The issues.labeled paths for ready-to-code and ready-for-review have no explicit authorization gate. The ADR documents this as "Already implicit (requires write access)" — label application requires write access, making this a documented design decision. Users with triage permission can also apply labels, but triage is only grantable by repo admins and the downstream agents (code, review) are non-destructive. Same pattern in the scaffold dispatch.yml.

  • [authorization-gap] .github/workflows/reusable-dispatch.yml — The pull_request_target.closed path triggers retro without an explicit authorization gate. A PR author can close their own PR without write access, which would trigger the retro agent. The blast radius is minimal (retro is read-only analysis), and the ADR documents this as "Already implicit (requires write access)." Pre-existing behavior, unchanged by this PR. Same pattern in the scaffold dispatch.yml.

  • [error-handling-consistency] .github/workflows/reusable-dispatch.yml — The has_write_permission() function uses manual rm -f "${api_err}" for tempfile cleanup. Other scripts in the codebase use trap "rm -f ..." RETURN for automatic cleanup, which is more robust against early returns. Minor hygiene issue on a short-lived CI runner. Same pattern in the scaffold dispatch.yml.
    Remediation: Replace manual cleanup with trap "rm -f '${api_err}'" RETURN after creating the tempfile.

  • [workflow-command-injection] .github/workflows/reusable-dispatch.yml — The has_write_permission() function interpolates ${username} and $(cat "${api_err}") into ::warning:: messages. GitHub usernames cannot contain :: or newlines (alphanumeric and hyphens only). The ::warning:: messages are redirected to stderr (>&2), and GitHub Actions only processes workflow commands on stdout, so no injection is possible via this path. Residual risk is nil. Same pattern in the scaffold dispatch.yml.

Previous run

Review

Findings

Medium

  • [protected-path] .github/workflows/reusable-dispatch.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The change is justified by linked issue ADR: require is_authorized check on all agent slash commands #1662 and ADR 0054, but human review is always required for protected-path changes regardless of context.

  • [logic-error] .github/workflows/reusable-dispatch.yml:63 — The route job permissions are contents: read and pull-requests: read. The new has_write_permission() function calls the collaborator permission API (GET /repos/{owner}/{repo}/collaborators/{username}/permission), which may require token scopes beyond what is declared. The function is fail-closed (returns 1 on API failure, emits a ::warning::), so insufficient permissions would block all authorized users from dispatching rather than allowing unauthorized access. The github.token likely has sufficient access for same-repo collaborator queries, but this should be verified empirically for both public and private repos. Same concern applies to the scaffold dispatch.yml.
    Remediation: Verify empirically in a test run that the collaborator permission endpoint is accessible with the current GITHUB_TOKEN scopes across all deployment modes.

Low

  • [test-inadequate] internal/scaffold/scaffold_test.go:209 — The test does not assert that GH_TOKEN or EVENT_SENDER_LOGIN are present in the dispatch workflow env block. Without GH_TOKEN, gh api would fail at runtime (fail-closed). Without EVENT_SENDER_LOGIN, issues.edited authorization would use an empty string (also fail-closed). A regression removing either would not be caught by tests. Same pattern in the scaffold dispatch.yml.
    Remediation: Add assert.Contains(t, s, "GH_TOKEN") and assert.Contains(t, s, "EVENT_SENDER_LOGIN") to TestDispatchWorkflowContent.

  • [authorization-gap] .github/workflows/reusable-dispatch.yml — The issues.labeled paths for ready-to-code and ready-for-review have no explicit authorization gate. The ADR documents this as "Already implicit (requires write access)" — label application requires write access, making this a documented design decision. Users with triage permission can also apply labels, but triage is only grantable by repo admins and the downstream agents (code, review) are non-destructive. Same pattern in the scaffold dispatch.yml.

  • [authorization-gap] .github/workflows/reusable-dispatch.yml — The pull_request_target.closed path triggers retro without an explicit authorization gate. A PR author can close their own PR without write access, which would trigger the retro agent. The blast radius is minimal (retro is read-only analysis), and the ADR documents this as "Already implicit (requires write access)." Pre-existing behavior, unchanged by this PR. Same pattern in the scaffold dispatch.yml.

  • [error-handling-consistency] .github/workflows/reusable-dispatch.yml — The has_write_permission() function uses manual rm -f "${api_err}" for tempfile cleanup. Other scripts in the codebase use trap "rm -f ..." RETURN for automatic cleanup, which is more robust against early returns. Minor hygiene issue on a short-lived CI runner. Same pattern in the scaffold dispatch.yml.
    Remediation: Replace manual cleanup with trap "rm -f '${api_err}'" RETURN after creating the tempfile.

  • [workflow-command-injection] .github/workflows/reusable-dispatch.yml — The has_write_permission() function interpolates ${username} and $(cat "${api_err}") into ::warning:: messages. GitHub usernames cannot contain :: or newlines (alphanumeric and hyphens only). The ::warning:: messages are redirected to stderr (>&2), and GitHub Actions only processes workflow commands on stdout, so no injection is possible via this path. Residual risk is nil. Same pattern in the scaffold dispatch.yml.

Previous run (2)

Review

Findings

Medium

  • [protected-path] .github/workflows/reusable-dispatch.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The change is justified by linked issue ADR: require is_authorized check on all agent slash commands #1662 and ADR 0054, but human review is always required for protected-path changes regardless of context.

  • [logic-error] .github/workflows/reusable-dispatch.yml — For issues.edited, the authorization check uses ISSUE_USER_LOGIN (the issue author) rather than github.actor (the user who performed the edit). If a maintainer edits an issue originally opened by an external user, the check verifies the external user's permissions (which will fail) rather than the maintainer's. The practical impact is low since maintainers rarely edit external users' issues and can use /fs-triage instead, but the check does not match the semantic intent of "authorize the actor who triggered the event." Same pattern in the scaffold dispatch.yml.
    Remediation: Add EVENT_ACTOR: ${{ github.actor }} as an env var and use it for the issues.edited path, or document this as an intentional simplification.

Low

  • [logic-error] .github/workflows/reusable-dispatch.yml:63 — The route job permissions (contents: read, pull-requests: read) may not include sufficient scope for the collaborator permission API (GET /repos/{owner}/{repo}/collaborators/{username}/permission). The function is fail-closed (returns 1 on any failure, emits a ::warning::), so insufficient permissions would block all authorized users from dispatching rather than allowing unauthorized access. The github.token likely has sufficient access for same-repo collaborator queries, but this should be verified empirically. Same concern applies to the scaffold dispatch.yml.

  • [test-integrity] internal/scaffold/scaffold_test.go — The test verifies has_write_permission, .role_name, and admin|maintain|write are present but does not assert that GH_TOKEN is set in the routing step's env block. Since has_write_permission() relies on gh api which needs GH_TOKEN, a regression removing the env var would pass all tests.
    Remediation: Add assert.Contains(t, s, "GH_TOKEN").

  • [authorization-gap] .github/workflows/reusable-dispatch.yml — The pull_request_target.closed path triggers retro without an explicit authorization gate. A PR author can close their own PR without write access, which would trigger the retro agent. The blast radius is minimal (retro is read-only analysis), and the ADR documents this as "Already implicit (requires write access)." Pre-existing behavior, unchanged by this PR. Same pattern in the scaffold dispatch.yml.

  • [documentation-implementation-gap] docs/ADRs/0054-require-authorization-on-all-agent-dispatch-paths.md — The "Visible feedback" section says the dispatch script "should provide" visible response for unauthorized users. The Consequences section correctly acknowledges this as "desirable future work." The implementation silently drops unauthorized commands. The ADR is internally consistent on this point (improved from prior review where it said "must").

  • [error-handling-consistency] .github/workflows/reusable-dispatch.yml — The has_write_permission() function uses manual rm -f "${api_err}" for tempfile cleanup. Other scripts in the codebase (e.g., scripts/lib/github-api-csma.sh) use trap "rm -f ..." RETURN for automatic cleanup, which is more robust against early returns. Same pattern in the scaffold dispatch.yml.
    Remediation: Replace manual cleanup with trap "rm -f '${api_err}'" RETURN after creating the tempfile.

Previous run (3)

Review

Findings

High

  • [ADR-number-collision] docs/ADRs/0051-require-authorization-on-all-agent-dispatch-paths.md — Main already has 0051-agent-eval-harness-for-test-infrastructure.md and 0052-functional-tests-for-agent-pipelines.md (merged since this branch was created). This PR adds a different ADR 0051, creating a number collision. At merge time two files with the same ADR number will coexist, and references to "ADR 0051" in docs/architecture.md and docs/guides/dev/e2e-testing.md become ambiguous. The next available number is 0053.
    Remediation: Renumber this PR's ADR to 0053 (or the next available number) and update all references in docs/architecture.md, docs/guides/dev/e2e-testing.md, and the ADR file's title/heading.

Medium

  • [protected-path] .github/workflows/reusable-dispatch.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The change is justified by linked issue ADR: require is_authorized check on all agent slash commands #1662 and ADR 0051, but human review is always required for protected-path changes regardless of context.

Low

  • [logic-error] .github/workflows/reusable-dispatch.yml:63 — The route job permissions (contents: read, pull-requests: read) may not be sufficient for the has_write_permission() function's call to the collaborator permission API. The function is fail-closed (returns 1 on any failure, emits a ::warning::), so insufficient permissions would block all authorized users from dispatching rather than allowing unauthorized access. The github.token likely has sufficient access for same-repo collaborator queries, but this should be verified empirically. Same concern applies to the scaffold dispatch.yml.

  • [documentation-code-inconsistency] docs/ADRs/0051-require-authorization-on-all-agent-dispatch-paths.md — The ADR's code snippet for has_write_permission shows simplified error handling (2>/dev/null || echo "none") while the actual implementation uses mktemp + error capture + ::warning:: logging. A future reader may implement the simplified pattern instead of the actual one.
    Remediation: Add a note to the ADR that the snippet is simplified and the actual implementation includes error logging.

  • [GHA-workflow-command-injection] .github/workflows/reusable-dispatch.yml:118 — The has_write_permission function emits ::warning:: containing unsanitized ${username} and $(cat "${api_err}") values. The output is redirected to stderr (>&2), and GitHub Actions only processes workflow commands on stdout, so no injection is possible via this path today. However, the ::warning:: prefix is cosmetically misleading since it serves no purpose on stderr.

  • [documentation] docs/ADRs/0051-require-authorization-on-all-agent-dispatch-paths.md — The Decision section says the dispatch script "must provide some form of visible response" for unauthorized users, but the implementation silently drops commands. The Consequences section correctly acknowledges this as "desirable future work," creating an internal consistency gap between the normative Decision and the descriptive Consequences.
    Remediation: Adjust the Decision section language from "must" to "should" for the visible response requirement, or note it as planned follow-up rather than a current requirement.

  • [authorization-gap] .github/workflows/reusable-dispatch.yml:199 — The pull_request_target.closed path triggers retro without an explicit authorization gate. A PR author can close their own PR without write access, which would trigger the retro agent. The blast radius is minimal (retro is read-only analysis). Pre-existing behavior, unchanged by this PR. Same pattern in the scaffold dispatch.yml.

  • [authorization-inconsistency] .github/workflows/reusable-dispatch.yml:170 — The needs-info auto-triage dispatch path uses COMMENT_AUTHOR_ASSOC != 'NONE' rather than the new has_write_permission(). This is likely intentional (allowing issue authors and contributors to re-trigger triage by providing clarification), but the different authorization standard is not documented in the code or ADR.
    Remediation: Add a brief code comment explaining why this path intentionally uses a weaker authorization check.


Labels: PR modifies dispatch workflow authorization logic, adds a security-focused ADR, and updates agent documentation

Previous run (4)

Review

Findings

Medium

  • [logic-error] .github/workflows/reusable-dispatch.yml:63 — The route job permissions (contents: read, pull-requests: read) may not be sufficient for the new has_write_permission() function's call to the collaborator permission API (GET /repos/{owner}/{repo}/collaborators/{username}/permission). If the endpoint requires broader scopes, all dispatch paths will be blocked for all users. The function is fail-closed with ::warning:: output, making failure safe and immediately visible in the Actions UI. The same concern applies to the scaffold dispatch.yml. Note: the prior review's error-handling concern (silent 2>/dev/null || echo "none") is now resolved — errors are captured to a temp file and logged.
    Remediation: Verify empirically in a test run that the collaborator permission endpoint is accessible with the current GITHUB_TOKEN scopes. If it fails, add the required permission to both dispatch files.

  • [protected-path] .github/workflows/reusable-dispatch.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The change is justified by linked issue ADR: require is_authorized check on all agent slash commands #1662 and ADR 0051, but human review is always required for protected-path changes regardless of context.

Low

  • [authorization-gap] .github/workflows/reusable-dispatch.yml:199 — The pull_request_target.closed path triggers retro without an explicit authorization gate. A PR author can close their own PR without write access, which would trigger the retro agent. The ADR documents this as "already implicit (requires write access)" — technically incorrect for self-closed PRs, but the blast radius is minimal (retro is read-only analysis). Pre-existing behavior, unchanged by this PR. Same pattern in the scaffold dispatch.yml.

  • [documentation] .github/workflows/reusable-dispatch.yml:170 — The needs-info auto-triage dispatch path uses COMMENT_AUTHOR_ASSOC != 'NONE' rather than the new has_write_permission(). This is likely intentional (allowing issue authors and contributors to re-trigger triage by providing clarification), but the different authorization standard is not documented in the code or ADR.
    Remediation: Add a brief code comment explaining why this path intentionally uses a weaker authorization check.

  • [documentation] docs/ADRs/0051-require-authorization-on-all-agent-dispatch-paths.md — The Decision section says the dispatch script "must provide some form of visible response" for unauthorized users, but the implementation silently drops commands. The Consequences section correctly acknowledges this as "desirable future work," creating an internal consistency gap between the normative Decision and the descriptive Consequences.
    Remediation: Adjust the Decision section language from "must" to "should" for the visible response requirement, or note it as planned follow-up rather than a current requirement.


Labels: PR modifies dispatch workflow authorization logic and adds a security-focused ADR requiring authorization on all agent dispatch paths.

Previous run (5)

Review

Findings

Medium

  • [logic-error] docs/architecture.md:200 — The diff changes the display text from [ADR 0050] to [ADR 0051] in two places (lines 200 and 205) while keeping the link target as ADRs/0050-distributed-tracing-instrumentation.md. The surrounding content is about distributed tracing (ADR 0050's topic), not authorization (ADR 0051's topic), so the display text change is incorrect — readers see "ADR 0051" but the link still navigates to the tracing ADR.
    Remediation: Revert the display text back to [ADR 0050] in both places, keeping the link target unchanged.

  • [logic-error] .github/workflows/reusable-dispatch.yml:63 — The route job declares permissions contents: read and pull-requests: read. The new has_write_permission() function calls the collaborator permission API (GET /repos/{owner}/{repo}/collaborators/{username}/permission). This endpoint may require token scopes beyond what is declared. The 2>/dev/null || echo "none" fallback silently treats API errors as "no permission", causing has_write_permission() to return 1 for ALL users if the token lacks sufficient access. This is fail-closed (safe from a security perspective) but could silently disable all gated dispatch paths. The same pattern exists in the scaffold dispatch.yml.
    Remediation: Verify that github.token with the declared permissions can access the collaborator permission API endpoint on both public and private repositories. Add error differentiation so API failures are logged distinctly from "user has no permission."

  • [protected-path] .github/workflows/reusable-dispatch.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The change is justified by linked issue ADR: require is_authorized check on all agent slash commands #1662 and ADR 0051, but human review is always required for protected-path changes regardless of context.

Low

  • [logic-error] .github/workflows/reusable-dispatch.yml:170 — The needs-info auto-triage dispatch path (the * case in issue_comment) still uses COMMENT_AUTHOR_ASSOC != 'NONE' instead of the new has_write_permission(). This is pre-existing behavior not changed by this PR. The weaker gate is intentionally different — it allows the issue author (who may be external) to re-trigger triage by providing clarification on needs-info issues. The ADR's table does not mention this path, suggesting it was considered out of scope.

  • [logic-error] docs/ADRs/0051-require-authorization-on-all-agent-dispatch-paths.md:263 — The ADR states the dispatch script "must provide some form of visible response" for unauthorized users, but the implementation provides no feedback — STAGE remains empty and the script silently exits. The Consequences section (line 303) claims "Unauthorized slash command attempts get visible feedback (reaction + comment)" which contradicts the implementation. The fail-closed behavior is correct for security; the gap is in user-facing feedback.

  • [authorization-gap] .github/workflows/reusable-dispatch.yml:199 — The pull_request_target.closed path triggers retro without authorization. The ADR claims closing requires write access (implicit gate), but a PR author can close their own PR without write access. The blast radius is limited — retro is a read-only analysis. Pre-existing behavior, not introduced by this PR.

  • [error-handling] .github/workflows/reusable-dispatch.yml:112 — The has_write_permission() function silences all API errors with 2>/dev/null || echo "none". If the API call fails for transient reasons (rate limiting, network timeout, GitHub outage), all users are silently treated as unauthorized with no logging. This makes operational debugging difficult.
    Remediation: Log stderr or the HTTP status code before falling back to "none".

  • [stale-doc] docs/guides/dev/e2e-testing.md:38 — The e2e testing guide describes authorization using author_association with OWNER, MEMBER, or COLLABORATOR values. This pattern was replaced in agent dispatch paths (ADR 0051) with the collaborator permission API. The e2e guide already documents the author_association limitation (line 41-42), but a cross-reference to ADR 0051 would help readers understand the relationship between the two approaches.


Labels: PR modifies dispatch workflow authorization logic and adds a security-focused ADR.

Previous run (6)

Review

Findings

Medium

  • [logic-error] docs/architecture.md:200 — The diff changes the display text from [ADR 0050] to [ADR 0051] in two places (lines 200 and 205) while keeping the link target as ADRs/0050-distributed-tracing-instrumentation.md. The surrounding content is about distributed tracing (ADR 0050's topic), not authorization (ADR 0051's topic), so the display text change is incorrect — readers see "ADR 0051" but the link still navigates to the tracing ADR.
    Remediation: Revert the display text back to [ADR 0050] in both places, keeping the link target unchanged.

  • [protected-path] .github/workflows/reusable-dispatch.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The change is justified by linked issue ADR: require is_authorized check on all agent slash commands #1662 and ADR 0051, but human review is always required for protected-path changes regardless of context.

Low

  • [logic-error] docs/ADRs/0051-require-authorization-on-all-agent-dispatch-paths.md:147 — The ADR section "Visible feedback for unauthorized users" states the dispatch script "must provide some form of visible response" but the implementation provides no feedback — STAGE remains empty and the script silently exits with "No stage matched — skipping dispatch." The ADR frames this as a requirement ("must provide"), not a future enhancement.
    Remediation: Either implement visible feedback (e.g., a GitHub reaction or comment) or weaken the ADR language from "must provide" to "should provide" / mark as future work.

  • [logic-error] .github/workflows/reusable-dispatch.yml:169 — The needs-info auto-triage dispatch path (the * case in the issue_comment handler) allows any commenter with COMMENT_AUTHOR_ASSOC != 'NONE' or who is the issue author to trigger triage. This is a weaker gate than is_authorized (OWNER|MEMBER|COLLABORATOR). Pre-existing behavior, not introduced by this PR.

  • [missing-test-coverage] internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml:61 — The scaffold dispatch.yml gains is_event_actor_authorized(), PR_AUTHOR_ASSOC, and ISSUE_AUTHOR_ASSOC, but TestDispatchWorkflowContent in internal/scaffold/scaffold_test.go is not updated to assert their presence. The existing assert.Contains(t, s, "is_authorized") substring-matches is_event_actor_authorized, but there are no dedicated assertions for PR_AUTHOR_ASSOC or ISSUE_AUTHOR_ASSOC.

  • [stale-doc] docs/glossary.md:155 — The glossary entry for "Slash Command" mentions "gated by an ACL" but does not reference ADR 0051 which now formalizes the authorization policy. A cross-reference would help readers find the concrete policy.


Labels: PR modifies dispatch workflow authorization logic and adds security-focused ADR

Previous run (7)

Review

Findings

Medium

  • [logic-error] docs/architecture.md:200 — The diff changes the display text from [ADR 0050] to [ADR 0051] in two places (lines 200 and 205) while keeping the link target as ADRs/0050-distributed-tracing-instrumentation.md. The surrounding content is about distributed tracing (ADR 0050's topic), not authorization (ADR 0051's topic), so the display text change is incorrect — readers see "ADR 0051" but the link still navigates to the tracing ADR.
    Remediation: Revert the display text changes: keep [ADR 0050] in both the Observability bullet and the strikethrough open question.

  • [logic-error] docs/ADRs/0051-require-authorization-on-all-agent-dispatch-paths.md:147 — The ADR section "Visible feedback for unauthorized users" states the dispatch script "must provide some form of visible response" but the implementation provides no feedback — STAGE remains empty and the script silently exits with "No stage matched — skipping dispatch." The ADR frames this as a requirement ("must provide"), not a future enhancement.
    Remediation: Either implement visible feedback (e.g., a GitHub reaction or comment) or weaken the ADR language from "must provide" to "should provide" / mark as future work.

  • [protected-path] .github/workflows/reusable-dispatch.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The change is justified by linked issue ADR: require is_authorized check on all agent slash commands #1662 and ADR 0051, but human review is always required for protected-path changes regardless of context.

Low

  • [logic-error] docs/ADRs/0051-require-authorization-on-all-agent-dispatch-paths.md:113 — The ADR's automatic event triggers table claims pull_request_target.closed is "Already implicit (requires write access)." However, a PR author can close their own PR without write access to the repository. An external contributor closing their own PR would trigger retro without passing authorization. The blast radius is limited (retro is read-only analysis), but the stated rationale is factually incorrect.

  • [logic-error] .github/workflows/reusable-dispatch.yml:169 — The needs-info auto-triage dispatch path (the * case in the issue_comment handler) allows any commenter with COMMENT_AUTHOR_ASSOC != 'NONE' or who is the issue author to trigger triage. This is a weaker gate than is_authorized (OWNER|MEMBER|COLLABORATOR). The ADR states all dispatch paths require authorization but this path is not gated with is_authorized and not listed in the ADR's trigger table. Pre-existing behavior, not introduced by this PR.

  • [missing-test-coverage] internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml:61 — The scaffold dispatch.yml gains is_event_actor_authorized(), PR_AUTHOR_ASSOC, and ISSUE_AUTHOR_ASSOC, but TestDispatchWorkflowContent in internal/scaffold/scaffold_test.go is not updated to assert their presence. The existing assert.Contains(t, s, "is_authorized") substring-matches is_event_actor_authorized, but there are no dedicated assertions for PR_AUTHOR_ASSOC or ISSUE_AUTHOR_ASSOC.

  • [authorization bypass] .github/workflows/reusable-dispatch.yml:184 — The issues > labeled paths (ready-to-code, ready-for-review) have no is_event_actor_authorized check in either reusable-dispatch.yml or the scaffold dispatch.yml. GitHub's Triage role can apply labels without being OWNER, MEMBER, or COLLABORATOR. The ADR documents this as "Already implicit (requires write access)," and label application is a deliberate trust boundary.

Info

  • [permission-scope] .github/workflows/reusable-dispatch.yml:97 — New env vars PR_AUTHOR_ASSOC and ISSUE_AUTHOR_ASSOC are sourced from server-side computed GitHub event fields (not user-controlled). The is_event_actor_authorized function fails closed on empty/missing input (the * case returns 1), which is the correct security posture.

Labels: PR modifies dispatch workflow authorization logic and adds security-focused ADR

Previous run (8)

Review

Findings

Medium

  • [protected-path] .github/workflows/reusable-dispatch.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The change is justified by linked issue ADR: require is_authorized check on all agent slash commands #1662 and ADR 0051, but human review is always required for protected-path changes regardless of context.

Low

  • [missing-test-coverage] internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml:61 — The scaffold dispatch.yml gains is_event_actor_authorized(), PR_AUTHOR_ASSOC, and ISSUE_AUTHOR_ASSOC, but TestDispatchWorkflowContent in internal/scaffold/scaffold_test.go is not updated to assert their presence. The existing assert.Contains(t, s, "is_authorized") will substring-match is_event_actor_authorized, providing partial implicit coverage, but there are no dedicated assertions for the new function name or the two new env vars.

  • [logic-error] docs/ADRs/0051-require-authorization-on-all-agent-dispatch-paths.md:113 — The ADR's automatic event triggers table claims pull_request_target.closed is "Already implicit (requires write access)." However, a PR author can close their own PR without write access to the repository. An external contributor who opens a PR and then closes it would trigger the retro agent without passing any authorization check. The blast radius is limited (retro is a read-only analysis), but the ADR's claim is technically inaccurate.

  • [missing-implementation] .github/workflows/reusable-dispatch.yml — The ADR's "Visible feedback for unauthorized users" section requires that unauthorized slash command attempts receive visible feedback (reaction, comment, or both). The implementation silently drops the command — STAGE remains empty and the workflow exits with "No stage matched." The same gap exists in the scaffold dispatch.yml. The fail-closed behavior is correct for security, but the UX gap should be tracked as follow-up work.

  • [authorization bypass] .github/workflows/reusable-dispatch.yml:184 — The issues > labeled path (ready-to-code, ready-for-review) has no explicit is_event_actor_authorized check. The ADR documents this as "Already implicit (requires write access)," but GitHub's Triage role can also apply labels, which is broader than the OWNER/MEMBER/COLLABORATOR requirement enforced on other paths. This is a conscious trust boundary rather than an exploitable bypass.

Info

  • [permission-scope] .github/workflows/reusable-dispatch.yml:97 — New env vars PR_AUTHOR_ASSOC and ISSUE_AUTHOR_ASSOC are sourced from server-side computed GitHub event fields (not user-controlled). The is_event_actor_authorized function fails closed on empty/missing input (the * case returns 1), which is the correct security posture.

Labels: PR modifies dispatch workflow authorization logic and adds a security-focused ADR with documentation updates.

Previous run (9)

Review

Findings

Medium

  • [protected-path] .github/workflows/reusable-dispatch.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The change is justified by linked issue ADR: require is_authorized check on all agent slash commands #1662 and ADR 0050, but human review is always required for protected-path changes regardless of context.

  • [missing-test-coverage] internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml:61 — The scaffold dispatch.yml gains is_event_actor_authorized(), PR_AUTHOR_ASSOC, and ISSUE_AUTHOR_ASSOC, but TestDispatchWorkflowContent in internal/scaffold/scaffold_test.go is not updated to assert their presence. The existing assert.Contains(t, s, "is_authorized") at line 208 will substring-match is_event_actor_authorized, but there are no dedicated assertions for the new function name or the two new env vars. A future regression removing event-level authorization from the scaffold would pass all tests.

Low

  • [logic-error] docs/ADRs/0050-require-authorization-on-all-agent-dispatch-paths.md:113 — The ADR's automatic event triggers table claims pull_request_target.closed is "Already implicit (requires write access)." However, a PR author can close their own PR without write access to the repository. An external contributor who opens a PR and then closes it would trigger the retro agent without passing any authorization check. The blast radius is limited (retro is a read-only analysis), but the ADR's claim is technically inaccurate.

  • [missing-implementation] .github/workflows/reusable-dispatch.yml — The ADR's "Visible feedback for unauthorized users" section requires that unauthorized slash command attempts receive visible feedback (reaction, comment, or both). The implementation silently drops the command — STAGE remains empty and the workflow exits with "No stage matched." The same gap exists in the scaffold dispatch.yml. The fail-closed behavior is correct for security, but the UX gap should be tracked as follow-up work.

  • [authorization bypass] .github/workflows/reusable-dispatch.yml:184 — The issues > labeled path (ready-to-code, ready-for-review) has no explicit is_event_actor_authorized check. The ADR documents this as "Already implicit (requires write access)," but GitHub's Triage role can also apply labels, which is broader than the OWNER/MEMBER/COLLABORATOR requirement enforced on other paths. This is a conscious trust boundary rather than an exploitable bypass.

  • [comment-consistency] internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml — The scaffold dispatch.yml uses # Helper: comment prefixes on all helper functions (lines 54, 62, 67), while reusable-dispatch.yml does not use this prefix on any helper function. The new is_event_actor_authorized perpetuates this pre-existing cross-file style inconsistency.

Info

  • [permission-scope] .github/workflows/reusable-dispatch.yml:97 — New env vars PR_AUTHOR_ASSOC and ISSUE_AUTHOR_ASSOC are sourced from server-side computed GitHub event fields (not user-controlled). The is_event_actor_authorized function fails closed on empty/missing input (the * case returns 1), which is the correct security posture.

Labels: PR modifies dispatch workflow authorization logic and adds security gates to agent dispatch paths.

Previous run (10)

Review

Findings

Medium

  • [protected-path] .github/workflows/reusable-dispatch.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The change is justified by linked issue ADR: require is_authorized check on all agent slash commands #1662 and ADR 0049, but human review is always required for protected-path changes regardless of context.

  • [logic-error] docs/ADRs/0049-require-authorization-on-all-agent-dispatch-paths.md:113 — The ADR's automatic event triggers table claims pull_request_target.closed is "Already implicit (requires write access)." However, a PR author can close their own PR without write access to the repository. An external contributor who opens a PR and then closes it would trigger the retro agent without passing any authorization check. This contradicts the ADR's stated goal that all agent dispatch paths require authorization.

  • [missing-test-coverage] internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml:61 — The scaffold dispatch.yml gains is_event_actor_authorized(), PR_AUTHOR_ASSOC, and ISSUE_AUTHOR_ASSOC, but scaffold_test.go is not updated to assert their presence. The existing test asserts is_authorized and COMMENT_AUTHOR_ASSOC but not the new additions. A future regression removing event-level authorization from the scaffold would pass all tests.

Low

  • [missing-implementation] .github/workflows/reusable-dispatch.yml:141 — The ADR's "Visible feedback for unauthorized users" section requires that unauthorized slash command attempts receive visible feedback (reaction, comment, or both). The implementation silently drops the command — STAGE remains empty and the workflow exits with "No stage matched." The same gap exists in the scaffold dispatch.yml. The fail-closed behavior is correct for security, but the UX gap should be tracked as follow-up work.

  • [authorization bypass] .github/workflows/reusable-dispatch.yml:184 — The issues > labeled path (ready-to-code, ready-for-review) has no explicit is_event_actor_authorized check. The ADR documents this as "Already implicit (requires write access)," but GitHub's Triage role can also apply labels, which is broader than the OWNER/MEMBER/COLLABORATOR requirement enforced on other paths. This is a conscious trust boundary rather than an exploitable bypass.

  • [comment-consistency] internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml — The new is_event_actor_authorized function has a # Helper: comment prefix in the scaffold dispatch.yml but lacks it in reusable-dispatch.yml. The existing pattern in the scaffold file is that all helper functions have # Helper: prefixes.

Info

  • [permission-scope] .github/workflows/reusable-dispatch.yml:97 — New env vars PR_AUTHOR_ASSOC and ISSUE_AUTHOR_ASSOC are sourced from server-side computed GitHub event fields (not user-controlled). The is_event_actor_authorized function fails closed on empty/missing input (the * case returns 1), which is the correct security posture.
Previous run (11)

Review

Findings

Medium

  • [protected-path] .github/workflows/reusable-dispatch.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The change is justified by linked issue ADR: require is_authorized check on all agent slash commands #1662 and ADR 0049, but human review is always required for protected-path changes regardless of context.

Low

  • [missing-implementation] .github/workflows/reusable-dispatch.yml:141 — The ADR's "Visible feedback for unauthorized users" section requires that unauthorized slash command attempts receive visible feedback (reaction, comment, or both). The implementation silently drops the command — STAGE remains empty and the workflow exits with "No stage matched." The same gap exists in the scaffold dispatch.yml. The fail-closed behavior is correct for security, but the UX gap should be tracked as follow-up work.

  • [authorization bypass] .github/workflows/reusable-dispatch.yml:184 — The issues > labeled path (ready-to-code, ready-for-review) has no explicit is_event_actor_authorized check. The ADR documents this as "Already implicit (requires write access)," but GitHub's Triage role can also apply labels, which is broader than the OWNER/MEMBER/COLLABORATOR requirement enforced on other paths. However, Triage is a trusted role explicitly granted by repository admins, making this a conscious trust boundary rather than an exploitable bypass.

  • [authorization bypass] .github/workflows/reusable-dispatch.yml:198 — The pull_request_target > closed path triggers retro without authorization. The ADR documents this as intentionally ungated: closing a PR requires write access. Triage role can also close PRs, so retro can be triggered by a broader set than OWNER/MEMBER/COLLABORATOR.

  • [stale-doc] docs/ADRs/0034-centralized-shim-routing-via-dispatch.md:129 — ADR 0034 states "Author association checks for /fs-fix, /fs-retro, /fs-fix-stop move to dispatch.yml's routing step" — now incomplete since ADR 0049 extends authorization to all slash commands. Per AGENTS.md, Accepted ADRs are point-in-time records; a minor cross-reference annotation to ADR 0049 would help readers.

  • [missing-authorization-details] docs/glossary.md:155 — The glossary definition of "Slash Command" mentions "gated by an ACL" but does not specify the OWNER/MEMBER/COLLABORATOR requirement. Now that ADR 0049 formalizes the policy, a cross-reference would help readers find the concrete policy.

Info

  • [permission-scope] .github/workflows/reusable-dispatch.yml:97 — New env vars PR_AUTHOR_ASSOC and ISSUE_AUTHOR_ASSOC are sourced from server-side computed GitHub event fields (not user-controlled). The is_event_actor_authorized function fails closed on empty/missing input (the * case returns 1), which is the correct security posture.
Previous run (12)

Review

Findings

Medium

  • [protected-path] .github/workflows/reusable-dispatch.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The change is justified by linked issue ADR: require is_authorized check on all agent slash commands #1662 and ADR 0049, but human review is always required for protected-path changes regardless of context.

  • [scope-authorization-mismatch] docs/agents/triage.md:28 — The PR adds authorization requirement notes to agent docs but does not update the auto-trigger statements. docs/agents/triage.md line 28 still says "Triage also runs automatically when a new issue is opened" and docs/agents/review.md line 30 says the review agent "runs automatically when a PR is opened." With the new is_event_actor_authorized gate, these statements are only true for issues/PRs opened by OWNER, MEMBER, or COLLABORATOR users. External contributor issues and PRs will no longer trigger automatic triage/review.
    Remediation: Qualify the auto-trigger statements to note the authorization requirement, e.g., "Triage also runs automatically when a new issue is opened by a repository owner, member, or collaborator."

  • [authorization bypass] .github/workflows/reusable-dispatch.yml:200 — The issues > labeled path (ready-to-code, ready-for-review) has no explicit authorization gate. The ADR documents this as "Already implicit (requires write access)," but GitHub's Triage role can also apply labels, which is broader than the OWNER/MEMBER/COLLABORATOR requirement enforced on other paths. This is a pre-existing design choice not changed by this PR, but now more notable with the formalized policy. See also: same pattern in internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml.

Low

  • [missing-implementation] .github/workflows/reusable-dispatch.yml:143 — The ADR's "Visible feedback for unauthorized users" section requires that unauthorized slash command attempts receive visible feedback (reaction, comment, or both). The implementation silently drops the command — STAGE remains empty and the workflow exits with "No stage matched." The same gap exists in the scaffold dispatch.yml. The fail-closed behavior is correct for security, but the UX gap should be tracked as follow-up work.

  • [scope-creep] .github/workflows/reusable-dispatch.yml:196 — Issue ADR: require is_authorized check on all agent slash commands #1662's title scopes to slash commands, but the PR extends authorization to event triggers (issues.opened/edited, pull_request_target.opened/synchronize/ready_for_review). This expansion is deliberate — the ADR title says "all agent dispatch paths" and documents the rationale. The scope broadening through an ADR is a legitimate process, not an oversight.

  • [authorization bypass] .github/workflows/reusable-dispatch.yml:218 — The pull_request_target > closed path triggers retro without authorization. The ADR documents this as intentionally ungated: closing a PR requires write access, which is an implicit authorization gate. Same pattern in scaffold dispatch.yml.

  • [missing-doc] docs/agents/fix.md:26, docs/agents/retro.md:26, docs/agents/prioritize.md:22 — These agent docs do not mention the OWNER/MEMBER/COLLABORATOR authorization requirement for their slash commands. Pre-existing gap (authorization existed before this PR), but now more notable with the formalized policy in ADR 0049.

  • [missing-context] docs/ADRs/0049-require-authorization-on-all-agent-dispatch-paths.md — The PR both proposes ADR 0049 and implements it in the same changeset. This is an accepted pattern in this codebase (per AGENTS.md: "approval happens at merge"), but the PR description could be clearer that this is a new decision being proposed, not implementation of a pre-existing one.

Info

  • [test-coverage-expectation] internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml:61 — No test assertions for the new is_event_actor_authorized function or PR_AUTHOR_ASSOC/ISSUE_AUTHOR_ASSOC env vars. Existing TestDispatchWorkflowContent checks is_authorized and OWNER|MEMBER|COLLABORATOR but not the new additions.

  • [comment-style-inconsistency] .github/workflows/reusable-dispatch.yml:111 — The new is_event_actor_authorized function lacks the # Helper: ... comment prefix present in the scaffold dispatch.yml and on other helpers. Minor style gap.

  • [permission-scope] .github/workflows/reusable-dispatch.yml:97 — New env vars (PR_AUTHOR_ASSOC, ISSUE_AUTHOR_ASSOC) are sourced from GitHub-provided event fields (github.event.pull_request.author_association, github.event.issue.author_association). These are server-side computed values, not user-controllable. The is_event_actor_authorized function fails closed on empty/missing input (the * case returns 1), which is the correct security posture.

Previous run (13)

Review

Findings

Medium

  • [ADR-technical-inaccuracy] docs/ADRs/0049-require-authorization-on-all-agent-dispatch-paths.md:165 — The Consequences section states: "Bot-to-bot orchestration (e.g., triage → code handoff) is unaffected because bot accounts bypass the human authorization check." The word "bypass" is misleading. Bots do not bypass the check; they are blocked by it for slash commands (the COMMENT_USER_TYPE != "Bot" condition short-circuits before is_authorized). Bot-to-bot orchestration works because it uses label-based triggers that have no authorization check. The Decision section (lines 131–137) correctly explains this mechanism, but the Consequences summary contradicts it.
    Remediation: Change to: "Bot-to-bot orchestration (e.g., triage → code handoff) is unaffected because it uses label-based triggers, which require write access and do not pass through the slash command authorization gate."

  • [logic-error] docs/guides/user/bugfix-workflow.md:67 — The added text states "Bot accounts bypass this check to preserve agent-to-agent handoffs." This is factually incorrect. Bot accounts do NOT bypass the authorization check — the COMMENT_USER_TYPE != "Bot" guard explicitly blocks bots from using slash commands. Agent-to-agent handoffs are preserved because they use label-based triggers, not slash commands. See also: [ADR-technical-inaccuracy] finding above (same conceptual issue, different file).
    Remediation: Replace with: "Bot-to-bot agent handoffs are not affected because they use label-based triggers, not slash commands."

  • [protected-path] .github/workflows/reusable-dispatch.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The change is justified by linked issue ADR: require is_authorized check on all agent slash commands #1662 and ADR 0049, but human review is always required for protected-path changes regardless of context.

Low

  • [missing-implementation] .github/workflows/reusable-dispatch.yml:143 — The ADR specifies visible feedback for unauthorized slash command users ("the dispatch script must provide some form of visible response"), but the implementation does not provide any feedback. When is_authorized returns false, STAGE remains empty and the workflow silently exits. The route job has read-only permissions, so adding feedback would require permission changes. Track as follow-up work.

  • [authorization] .github/workflows/reusable-dispatch.yml:170 — The non-command issue_comment catch-all uses COMMENT_AUTHOR_ASSOC != 'NONE' || is_issue_author, which is less restrictive than the new is_authorized check on slash commands. This is pre-existing behavior not changed by this PR, and is functionally appropriate (triage re-evaluation on needs-info issues should fire for any non-anonymous commenter). Noting the asymmetry for documentation purposes.

  • [authorization] .github/workflows/reusable-dispatch.yml:184 — The issues.labeled path has no explicit authorization check. The ADR documents this as intentional: label application requires write access, which is an implicit authorization gate.

  • [missing-doc] docs/agents/fix.md:26 — The /fs-fix command documentation does not mention the OWNER/MEMBER/COLLABORATOR authorization requirement. Pre-existing gap (authorization existed before this PR), but now more notable with the formalized policy in ADR 0049.

  • [missing-doc] docs/agents/retro.md:26 — The /fs-retro command documentation does not mention the authorization requirement. Same pre-existing gap.

  • [missing-doc] docs/agents/prioritize.md:22 — The /fs-prioritize command documentation does not mention the authorization requirement. Same pre-existing gap.

  • [incomplete-doc] docs/guides/user/bugfix-workflow.md:56 — The blanket authorization note added at line 66–67 covers /fs-retro, but the note itself contains the factual error about bot bypass (see medium finding above).

Info

  • [test-coverage-expectation] internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml:61 — No test assertions for the new is_event_actor_authorized function or PR_AUTHOR_ASSOC/ISSUE_AUTHOR_ASSOC env vars. Existing TestDispatchWorkflowContent checks is_authorized and OWNER|MEMBER|COLLABORATOR but not the new additions.

  • [comment-style-inconsistency] .github/workflows/reusable-dispatch.yml:111 — Helper functions in reusable-dispatch.yml lack the # Helper: ... comments present in the scaffold dispatch.yml. The new is_event_actor_authorized has a comment in dispatch.yml but not in reusable-dispatch.yml.

Previous run (14)

Review

Findings

Medium

  • [ADR-technical-inaccuracy] docs/ADRs/0049-require-authorization-on-all-agent-dispatch-paths.md:131 — The ADR section "Bot-to-bot workflows are preserved" states that bot accounts bypass the is_authorized gate via slash commands. This is incorrect: the guard COMMENT_USER_TYPE != "Bot" && is_authorized blocks bots from slash commands because the != "Bot" check fails first when COMMENT_USER_TYPE is "Bot", so the entire condition is false and STAGE is never set. Bot-to-bot handoffs actually work via label-based triggers (ready-to-code, ready-for-review via the issues.labeled path), which have no bot or authorization check. The ADR's stated mechanism is wrong even though the conclusion (handoffs are preserved) happens to be correct.
    Remediation: Rewrite the "Bot-to-bot workflows are preserved" section to describe the actual mechanism: bot-to-bot handoffs are preserved because they use label-based triggers (issues.labeled with ready-to-code/ready-for-review), not slash commands. Label application requires write access, which serves as an implicit authorization gate. If bot slash command invocation is actually needed, the guard logic must change to if [[ "${COMMENT_USER_TYPE}" == "Bot" ]] || is_authorized.

  • [missing-doc] docs/architecture.md — AGENTS.md requires updating docs/architecture.md when a new accepted ADR is added in the same PR. ADR 0049 is new with status Accepted but architecture.md is not updated to reference it.
    Remediation: Update the "Slash-command parser + ACL" building block section in docs/architecture.md to reference ADR 0049.

  • [protected-path] .github/workflows/reusable-dispatch.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The change is justified by linked issue ADR: require is_authorized check on all agent slash commands #1662 and ADR 0049, but human review is always required for protected-path changes regardless of context.

Low

  • [ADR-technical-inaccuracy] docs/ADRs/0049-require-authorization-on-all-agent-dispatch-paths.md:107 — The ADR's automatic event triggers table states that pull_request_target.ready_for_review is "Already implicit (requires write access)." In the implementation, ready_for_review is in the same opened|synchronize|ready_for_review case branch and is explicitly gated by is_event_actor_authorized. The table should reflect that it is explicitly gated, not implicitly.

  • [missing-implementation] docs/ADRs/0049-require-authorization-on-all-agent-dispatch-paths.md:147 — The ADR specifies visible feedback for unauthorized slash command users ("the dispatch script must provide some form of visible response"), but neither workflow file implements any feedback mechanism. When authorization fails, the script silently falls through to "No stage matched" and exits 0. The fail-closed behavior is correct for security, but the UX gap should be tracked as follow-up work.

Info

  • [incomplete-doc] docs/glossary.md:155 — The Slash Command glossary entry mentions "gated by an ACL" but does not reference ADR 0049 which now formalizes the authorization policy. A cross-reference would help readers find the concrete policy.

  • [test-coverage-expectation] internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml — No test assertions for the new is_event_actor_authorized function or PR_AUTHOR_ASSOC/ISSUE_AUTHOR_ASSOC env vars. Existing TestDispatchWorkflowContent checks is_authorized and OWNER|MEMBER|COLLABORATOR but not the new additions.

  • [comment-style-inconsistency] .github/workflows/reusable-dispatch.yml:104 — Helper functions in reusable-dispatch.yml lack the # Helper: ... comments present in the scaffold dispatch.yml. The new is_event_actor_authorized has a comment in dispatch.yml but not in reusable-dispatch.yml.

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label May 29, 2026
@ascerra
ascerra force-pushed the agent/1662-adr-auth-slash-commands branch from eee399d to 27584e3 Compare May 29, 2026 14:27
@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels May 29, 2026
Comment thread docs/ADRs/0043-require-authorization-on-all-slash-commands.md Outdated
Comment thread docs/ADRs/0043-require-authorization-on-all-slash-commands.md Outdated
Comment thread docs/ADRs/0043-require-authorization-on-all-slash-commands.md Outdated
@ascerra
ascerra force-pushed the agent/1662-adr-auth-slash-commands branch 2 times, most recently from c6d4382 to dca8138 Compare May 29, 2026 15:59
@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels May 29, 2026

@ralphbean ralphbean left a comment

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.

A few non-blocking notes inline. The PR title still says "slash commands" but the ADR covers all dispatch paths now — worth updating since the merge commit title feeds release notes.

Comment thread docs/ADRs/0043-require-authorization-on-all-agent-dispatch-paths.md Outdated
Comment thread docs/ADRs/0054-require-authorization-on-all-agent-dispatch-paths.md
Comment thread docs/ADRs/0051-require-authorization-on-all-agent-dispatch-paths.md Outdated
Comment thread docs/ADRs/0051-require-authorization-on-all-agent-dispatch-paths.md Outdated
@ascerra

ascerra commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Updated with the implementation — all dispatch paths (slash commands + event triggers) now gated with is_authorized. Also updated PR title to match the ADR scope (dispatch paths, not just slash commands). Changes include:

  • ADR status: Proposed → Accepted (frontmatter + body)
  • Implementation note for non-comment event authorization (is_event_actor_authorized helper)
  • Future work bullet for rate-limited external auto-triage (vouch reference)
  • reusable-dispatch.yml: gated /fs-triage, /fs-code, /fs-review + pull_request_target.opened
  • dispatch.yml (scaffold): same slash command gates + issues.opened/edited + pull_request_target.opened

@ascerra ascerra changed the title docs(#1662): ADR 0043 — require authorization on all slash commands docs(#1662): ADR 0043 — require authorization on all agent dispatch paths Jun 16, 2026
@ascerra ascerra changed the title docs(#1662): ADR 0043 — require authorization on all agent dispatch paths feat(#1662): ADR 0043 + implement is_authorized on all agent dispatch paths Jun 16, 2026
@github-actions

github-actions Bot commented Jun 16, 2026

Copy link
Copy Markdown

E2E tests did not run

The authorization check failed (GitHub API error). Re-run the workflow when the API is available; if this persists, contact a maintainer.

See E2E testing guide for details.

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 24, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 12:28 PM UTC · Completed 12:42 PM UTC
Commit: a2628e1 · View workflow run →

@fullsend-ai-retro

fullsend-ai-retro Bot commented Jun 24, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 12:57 PM UTC · Completed 1:13 PM UTC
Commit: a2628e1 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #1688 — ADR 0054, authorize all agent dispatch paths

Timeline: 26 days (May 29 – Jun 24), 21 commits, 13 files changed across workflow code, ADR, agent docs, scaffold, and tests.

Workflow graph: triage → /fs-code (ADR-only output) → human implementation (ascerra, 20 commits) → 3 human review rounds (ralphbean, ifireball, waynesun09 Review Squad) → merge. No fix agent runs; all fixes were manual.

What went well:

  • The Review Squad (waynesun09) caught two HIGH-severity issues: maintain as dead code in the permission case statement, and a missing ISSUE_IS_PR guard on /fs-review in the scaffold template.
  • The post-review protected-path downgrade correctly prevented auto-approval on workflow file changes (requires-manual-review label applied).
  • Multiple human reviewers provided substantive design feedback (ifireball's objection to blocking external reporters, ralphbean catching the incorrect e2e fix).

What could go better:

Proposals filed: 1 new proposal (review run cancellation on merge). Other findings map to existing open issues.

Proposals filed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/dispatch Workflow dispatch and triggers requires-manual-review Review requires human judgment security Security threat model and related concerns

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ADR: require is_authorized check on all agent slash commands

6 participants