diff --git a/docs/adr/26535-event-scoped-activation-permission-derivation.md b/docs/adr/26535-event-scoped-activation-permission-derivation.md index 49fdbdf33e7..e5315c33442 100644 --- a/docs/adr/26535-event-scoped-activation-permission-derivation.md +++ b/docs/adr/26535-event-scoped-activation-permission-derivation.md @@ -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.) 3. Implementations **MUST NOT** grant `discussions: write` in the activation job unless `discussion` or `discussion_comment` is among the configured trigger events. 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. 5. Implementations **MUST** apply the same permission derivation logic to both the activation job `permissions` block and the GitHub App token minting permissions. diff --git a/pkg/workflow/activation_permissions_scope_test.go b/pkg/workflow/activation_permissions_scope_test.go index 4a400c86dd1..cc8a299ee5b 100644 --- a/pkg/workflow/activation_permissions_scope_test.go +++ b/pkg/workflow/activation_permissions_scope_test.go @@ -362,3 +362,64 @@ func TestAddActivationInteractionPermissionsMapFallbackRespectsStatusCommentPull assert.False(t, hasPullRequests, "fallback should omit pull-requests:write when reactions are disabled") assert.Equal(t, PermissionWrite, permsMap[PermissionDiscussions], "fallback should include discussions:write when status-comment.discussions is true") } + +// TestActivationPermissionsIssueCommentReactionRequiresPullRequestsWrite verifies that +// issue_comment triggers with PR reactions grant pull-requests:write. This covers the +// slash_command case (events:[pull_request_comment] compiles to issue_comment) where +// reactions on PR comments require pull-requests:write even though the API uses /issues/comments. +func TestActivationPermissionsIssueCommentReactionRequiresPullRequestsWrite(t *testing.T) { + permsMap := map[PermissionScope]PermissionLevel{} + + onSection := "on:\n issue_comment:\n types: [created]\n" + addActivationInteractionPermissionsMap(permsMap, onSection, + /* hasReaction */ true, + /* reactionIncludesIssues */ true, + /* reactionIncludesPullRequests */ true, + /* reactionIncludesDiscussions */ false, + /* hasStatusComment */ false, + /* statusCommentIncludesIssues */ false, + /* statusCommentIncludesPullRequests */ false, + /* statusCommentIncludesDiscussions */ false, + ) + + assert.Equal(t, PermissionWrite, permsMap[PermissionIssues], "issue_comment reaction should include issues:write") + assert.Equal(t, PermissionWrite, permsMap[PermissionPullRequests], "issue_comment reaction should include pull-requests:write because PR comments use issue_comment event") + _, hasDiscussions := permsMap[PermissionDiscussions] + assert.False(t, hasDiscussions, "issue_comment reaction should not include discussions:write") +} + +// TestActivationPermissionsSlashCommandPRCommentReactionRequiresPullRequestsWrite verifies +// end-to-end that a slash_command workflow with events:[pull_request_comment] produces an +// activation job with pull-requests:write. slash_command compiles to issue_comment, and +// GitHub requires pull-requests:write to react to PR comments (#26720 follow-up). +func TestActivationPermissionsSlashCommandPRCommentReactionRequiresPullRequestsWrite(t *testing.T) { + tmpDir := testutil.TempDir(t, "activation-perms-slash-command-pr-comment") + testFile := filepath.Join(tmpDir, "slash-command-pr-comment.md") + testContent := `--- +on: + slash_command: + name: review + events: [pull_request_comment] + reaction: eyes + status-comment: false +engine: copilot +--- + +# Slash command PR comment reaction permissions +` + + err := os.WriteFile(testFile, []byte(testContent), 0644) + require.NoError(t, err, "failed to write test workflow") + + compiler := NewCompiler() + err = compiler.CompileWorkflow(testFile) + require.NoError(t, err, "failed to compile workflow") + + lockContent, err := os.ReadFile(stringutil.MarkdownToLockFile(testFile)) + require.NoError(t, err, "failed to read generated lock file") + + activationJobSection := extractJobSection(string(lockContent), string(constants.ActivationJobName)) + assert.Contains(t, activationJobSection, "issues: write", "activation job should include issues:write for PR comment reactions via issue_comment event") + assert.Contains(t, activationJobSection, "pull-requests: write", "activation job should include pull-requests:write for slash_command PR comment reactions") + assert.NotContains(t, activationJobSection, "discussions: write", "activation job should not include discussions:write for slash_command PR comment reactions") +} diff --git a/pkg/workflow/compiler_activation_job.go b/pkg/workflow/compiler_activation_job.go index bf4f9d9c343..dec20f84efd 100644 --- a/pkg/workflow/compiler_activation_job.go +++ b/pkg/workflow/compiler_activation_job.go @@ -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) { permsMap[PermissionPullRequests] = PermissionWrite } // Reactions on discussions use GraphQL discussion APIs.