Skip to content

Commit 115b573

Browse files
authored
fix: grant pull-requests: write for issue_comment reactions (slash_command PR comment fix) (#28854)
1 parent 38d1131 commit 115b573

3 files changed

Lines changed: 67 additions & 4 deletions

File tree

docs/adr/26535-event-scoped-activation-permission-derivation.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ The `gh aw compile` command generates a lock file that includes an activation jo
1414

1515
### Decision
1616

17-
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.
17+
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.
1818

1919
### Alternatives Considered
2020

@@ -55,7 +55,7 @@ Request a minimal token at activation time and escalate permissions lazily when
5555
### Activation Permission Derivation
5656

5757
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.
58-
2. Implementations **MUST NOT** grant `pull-requests: write` in the activation job unless `pull_request_review_comment` is among the configured trigger events.
58+
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.)
5959
3. Implementations **MUST NOT** grant `discussions: write` in the activation job unless `discussion` or `discussion_comment` is among the configured trigger events.
6060
4. Implementations **MUST NOT** grant `issues: write` solely for reaction/status-comment purposes unless `issues`, `issue_comment`, or `pull_request` is among the configured trigger events.
6161
5. Implementations **MUST** apply the same permission derivation logic to both the activation job `permissions` block and the GitHub App token minting permissions.

pkg/workflow/activation_permissions_scope_test.go

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -362,3 +362,64 @@ func TestAddActivationInteractionPermissionsMapFallbackRespectsStatusCommentPull
362362
assert.False(t, hasPullRequests, "fallback should omit pull-requests:write when reactions are disabled")
363363
assert.Equal(t, PermissionWrite, permsMap[PermissionDiscussions], "fallback should include discussions:write when status-comment.discussions is true")
364364
}
365+
366+
// TestActivationPermissionsIssueCommentReactionRequiresPullRequestsWrite verifies that
367+
// issue_comment triggers with PR reactions grant pull-requests:write. This covers the
368+
// slash_command case (events:[pull_request_comment] compiles to issue_comment) where
369+
// reactions on PR comments require pull-requests:write even though the API uses /issues/comments.
370+
func TestActivationPermissionsIssueCommentReactionRequiresPullRequestsWrite(t *testing.T) {
371+
permsMap := map[PermissionScope]PermissionLevel{}
372+
373+
onSection := "on:\n issue_comment:\n types: [created]\n"
374+
addActivationInteractionPermissionsMap(permsMap, onSection,
375+
/* hasReaction */ true,
376+
/* reactionIncludesIssues */ true,
377+
/* reactionIncludesPullRequests */ true,
378+
/* reactionIncludesDiscussions */ false,
379+
/* hasStatusComment */ false,
380+
/* statusCommentIncludesIssues */ false,
381+
/* statusCommentIncludesPullRequests */ false,
382+
/* statusCommentIncludesDiscussions */ false,
383+
)
384+
385+
assert.Equal(t, PermissionWrite, permsMap[PermissionIssues], "issue_comment reaction should include issues:write")
386+
assert.Equal(t, PermissionWrite, permsMap[PermissionPullRequests], "issue_comment reaction should include pull-requests:write because PR comments use issue_comment event")
387+
_, hasDiscussions := permsMap[PermissionDiscussions]
388+
assert.False(t, hasDiscussions, "issue_comment reaction should not include discussions:write")
389+
}
390+
391+
// TestActivationPermissionsSlashCommandPRCommentReactionRequiresPullRequestsWrite verifies
392+
// end-to-end that a slash_command workflow with events:[pull_request_comment] produces an
393+
// activation job with pull-requests:write. slash_command compiles to issue_comment, and
394+
// GitHub requires pull-requests:write to react to PR comments (#26720 follow-up).
395+
func TestActivationPermissionsSlashCommandPRCommentReactionRequiresPullRequestsWrite(t *testing.T) {
396+
tmpDir := testutil.TempDir(t, "activation-perms-slash-command-pr-comment")
397+
testFile := filepath.Join(tmpDir, "slash-command-pr-comment.md")
398+
testContent := `---
399+
on:
400+
slash_command:
401+
name: review
402+
events: [pull_request_comment]
403+
reaction: eyes
404+
status-comment: false
405+
engine: copilot
406+
---
407+
408+
# Slash command PR comment reaction permissions
409+
`
410+
411+
err := os.WriteFile(testFile, []byte(testContent), 0644)
412+
require.NoError(t, err, "failed to write test workflow")
413+
414+
compiler := NewCompiler()
415+
err = compiler.CompileWorkflow(testFile)
416+
require.NoError(t, err, "failed to compile workflow")
417+
418+
lockContent, err := os.ReadFile(stringutil.MarkdownToLockFile(testFile))
419+
require.NoError(t, err, "failed to read generated lock file")
420+
421+
activationJobSection := extractJobSection(string(lockContent), string(constants.ActivationJobName))
422+
assert.Contains(t, activationJobSection, "issues: write", "activation job should include issues:write for PR comment reactions via issue_comment event")
423+
assert.Contains(t, activationJobSection, "pull-requests: write", "activation job should include pull-requests:write for slash_command PR comment reactions")
424+
assert.NotContains(t, activationJobSection, "discussions: write", "activation job should not include discussions:write for slash_command PR comment reactions")
425+
}

pkg/workflow/compiler_activation_job.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,8 +164,10 @@ func addActivationInteractionPermissionsMap(
164164
if needsIssuesWriteForReaction {
165165
permsMap[PermissionIssues] = PermissionWrite
166166
}
167-
// Reactions on pull requests and PR review comments require pull-requests:write.
168-
if reactionIncludesPullRequests && (hasPullRequestEvent || hasPullRequestReviewCommentEvent) {
167+
// Reactions on pull requests and PR review comments require pull-requests: write.
168+
// issue_comment events also fire for PR comments (slash_command with events:[pull_request_comment]
169+
// compiles to issue_comment), so pull-requests: write is also needed when issue_comment is present.
170+
if reactionIncludesPullRequests && (hasPullRequestEvent || hasPullRequestReviewCommentEvent || hasIssueCommentEvent) {
169171
permsMap[PermissionPullRequests] = PermissionWrite
170172
}
171173
// Reactions on discussions use GraphQL discussion APIs.

0 commit comments

Comments
 (0)