-
Notifications
You must be signed in to change notification settings - Fork 374
fix: grant pull-requests: write for issue_comment reactions (slash_command PR comment fix) #28854
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -14,7 +14,7 @@ The `gh aw compile` command generates a lock file that includes an activation jo | |||||
|
|
||||||
| ### Decision | ||||||
|
|
||||||
| We will derive activation job permissions by parsing the `on:` section YAML at compile time, filtering out known metadata trigger fields, and granting only the write scopes required by the real GitHub event types that are configured. `issues: write` is granted only when `issues`, `issue_comment`, or `pull_request` events are present (since reactions and status comments on issues/PRs use the Issues REST API). `pull-requests: write` is granted only when `pull_request_review_comment` events are present. `discussions: write` is granted only when `discussion` or `discussion_comment` events are present. A fallback to the previous broad-grant behavior is preserved for synthetic or test `WorkflowData` instances where the `on:` section is empty. | ||||||
| We will derive activation job permissions by parsing the `on:` section YAML at compile time, filtering out known metadata trigger fields, and granting only the write scopes required by the real GitHub event types that are configured. `issues: write` is granted only when `issues`, `issue_comment`, or `pull_request` events are present (since reactions and status comments on issues/PRs use the Issues REST API). `pull-requests: write` is granted when `pull_request` or `pull_request_review_comment` events are present, or when `issue_comment` is present with PR reactions enabled (because `issue_comment` fires for PR comments and GitHub requires `pull-requests: write` to react to PR comments). `discussions: write` is granted only when `discussion` or `discussion_comment` events are present. A fallback to the previous broad-grant behavior is preserved for synthetic or test `WorkflowData` instances where the `on:` section is empty. | ||||||
|
|
||||||
| ### Alternatives Considered | ||||||
|
|
||||||
|
|
@@ -55,7 +55,7 @@ Request a minimal token at activation time and escalate permissions lazily when | |||||
| ### Activation Permission Derivation | ||||||
|
|
||||||
| 1. Implementations **MUST** derive activation job write permissions from the set of real GitHub event types present in the `on:` section, not from the presence of `reaction` or `status-comment` configuration alone. | ||||||
| 2. Implementations **MUST NOT** grant `pull-requests: write` in the activation job unless `pull_request_review_comment` is among the configured trigger events. | ||||||
| 2. Implementations **MUST NOT** grant `pull-requests: write` in the activation job unless `pull_request`, `pull_request_review_comment`, or `issue_comment` is among the configured trigger events and the reaction/status-comment configuration includes pull requests. (`issue_comment` events fire for both issue comments and PR comments; since PR comments require `pull-requests: write` for reactions, the presence of `issue_comment` with PR reactions enabled mandates this permission.) | ||||||
|
||||||
| 2. Implementations **MUST NOT** grant `pull-requests: write` in the activation job unless `pull_request`, `pull_request_review_comment`, or `issue_comment` is among the configured trigger events and the reaction/status-comment configuration includes pull requests. (`issue_comment` events fire for both issue comments and PR comments; since PR comments require `pull-requests: write` for reactions, the presence of `issue_comment` with PR reactions enabled mandates this permission.) | |
| 2. Implementations **MUST NOT** grant `pull-requests: write` in the activation job unless `pull_request`, `pull_request_review_comment`, or `issue_comment` is among the configured trigger events and the reaction configuration includes pull requests. (`issue_comment` events fire for both issue comments and PR comments; since PR comments require `pull-requests: write` for reactions, the presence of `issue_comment` with PR reactions enabled mandates this permission.) |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -164,8 +164,10 @@ func addActivationInteractionPermissionsMap( | |||||||||||
| if needsIssuesWriteForReaction { | ||||||||||||
| permsMap[PermissionIssues] = PermissionWrite | ||||||||||||
| } | ||||||||||||
| // Reactions on pull requests and PR review comments require pull-requests:write. | ||||||||||||
| if reactionIncludesPullRequests && (hasPullRequestEvent || hasPullRequestReviewCommentEvent) { | ||||||||||||
| // Reactions on pull requests and PR review comments require pull-requests: write. | ||||||||||||
| // issue_comment events also fire for PR comments (slash_command with events:[pull_request_comment] | ||||||||||||
| // compiles to issue_comment), so pull-requests: write is also needed when issue_comment is present. | ||||||||||||
| if reactionIncludesPullRequests && (hasPullRequestEvent || hasPullRequestReviewCommentEvent || hasIssueCommentEvent) { | ||||||||||||
|
Comment on lines
+169
to
+170
|
||||||||||||
| // compiles to issue_comment), so pull-requests: write is also needed when issue_comment is present. | |
| if reactionIncludesPullRequests && (hasPullRequestEvent || hasPullRequestReviewCommentEvent || hasIssueCommentEvent) { | |
| // compiles to issue_comment), but the reaction step only runs for issue_comment when | |
| // issue reactions are enabled, so permission derivation must match that gating. | |
| if reactionIncludesPullRequests && (hasPullRequestEvent || hasPullRequestReviewCommentEvent || (reactionIncludesIssues && hasIssueCommentEvent)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the Decision paragraph, the wording implies
pull-requests: writeis granted wheneverpull_request/pull_request_review_commentevents are present. In the implementation, that permission is only added when reactions are enabled for pull requests (and now also forissue_commentin a PR-comment context). Consider tightening this text to explicitly include the reaction target gating so the ADR matches the current behavior.