diff --git a/docs/adr/26693-reaction-target-scoping-and-union-activation-permission-derivation.md b/docs/adr/26693-reaction-target-scoping-and-union-activation-permission-derivation.md new file mode 100644 index 00000000000..351c172c793 --- /dev/null +++ b/docs/adr/26693-reaction-target-scoping-and-union-activation-permission-derivation.md @@ -0,0 +1,91 @@ +# ADR-26693: Reaction Target Scoping and Union Activation Permission Derivation + +**Date**: 2026-04-16 +**Status**: Draft +**Deciders**: pelikhan, Copilot + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +ADR-26535 established that activation job permissions are derived at compile time from the real GitHub event types in the `on:` section, scoped to only the write permissions actually needed. However, that approach treated `on.reaction` as a monolithic toggle — if reactions were enabled, all event groups (issues, pull requests, discussions) were eligible for reactions and their associated permissions were granted based solely on the real event types present. This caused a 403 error during activation on `pull_request` events: the reaction step ran unconditionally on PR events but the permission model from ADR-26535 did not account for configurations where PR reactions should be explicitly disabled. The existing `on.status-comment` configuration already supported per-target boolean toggles (`issues`, `pull-requests`, `discussions`); `on.reaction` had no equivalent, creating an inconsistency and a correctness gap. + +### Decision + +We will extend `on.reaction` to support an object form with optional `issues`, `pull-requests`, and `discussions` boolean target toggles, mirroring the existing `on.status-comment` object syntax. When the object form is used, only the enabled targets participate in reaction step condition generation and in permission derivation. We will compute activation job permissions as the union of permissions needed for the enabled reaction targets and the enabled status-comment targets, so that disabling PR reactions (`reaction.pull-requests: false`) removes `pull-requests: write` even when `pull_request` events are configured. Scalar reaction forms (`"eyes"`, `"rocket"`, `1`, etc.) remain unchanged and continue to imply all targets enabled. + +### Alternatives Considered + +#### Alternative 1: Add a Single `pull-requests` Toggle to `on.reaction` + +Add only a `pull-requests: false` boolean directly on `on.reaction` (without object refactoring) to unblock the immediate 403 issue. This would fix the specific failure mode but would create a non-orthogonal API — `on.reaction` would support one target toggle but not `issues` or `discussions`. It was rejected because `on.status-comment` already demonstrates the value of full per-target control, and a partial API would require a follow-up breaking change to reach parity. + +#### Alternative 2: Suppress Reaction Steps for Events Not in the Permission Set + +Automatically derive reaction step conditions from the activation permission scope — if `pull-requests: write` is not granted, exclude PR events from the reaction step condition. This approach avoids a new configuration surface but ties UI behavior (which events get a reaction) to a security concern (which permissions are granted), making the coupling non-obvious. Authors who want reactions on issues but not PRs would have no explicit way to express that intent. It was rejected in favor of explicit configuration. + +#### Alternative 3: Unify `reaction` and `status-comment` into a Single `interactions` Object + +Replace both `on.reaction` and `on.status-comment` with a single top-level `on.interactions` object that has `reaction` and `status-comment` sub-keys, each with their own target toggles. This would produce a cleaner schema but is a breaking change to all existing workflows that use `on.reaction` or `on.status-comment`. The migration cost and churn outweighed the schema elegance benefit, so it was deferred. + +### Consequences + +#### Positive +- The 403 activation error on `pull_request` events when `reaction.pull-requests: false` is resolved: the reaction step condition no longer includes PR events, so the step does not run and no PR permission is required. +- `on.reaction` and `on.status-comment` now have symmetric per-target APIs, making the workflow configuration model easier to understand and document. +- Permission derivation is more accurate: disabling a reaction target explicitly removes the corresponding write scope from the activation job, reducing the blast radius of a compromised token. +- All three target dimensions (issues, pull-requests, discussions) are controllable independently for both reactions and status comments. + +#### Negative +- The `addActivationInteractionPermissionsMap` and `addBroadActivationInteractionPermissions` functions now accept three additional `bool` parameters each (`reactionIncludesIssues`, `reactionIncludesPullRequests`, `reactionIncludesDiscussions`), increasing function arity and making call sites more verbose. Tests that use these functions directly require updates when the signature changes again. +- A configuration where all reaction targets are disabled (`issues: false, pull-requests: false, discussions: false`) is rejected at parse time as invalid — this may surprise authors who expect a no-op behavior. The validation error message must be clear enough to guide authors to use `reaction: none` instead. +- `parseReactionValue` is now superseded by `parseReactionConfig`, which returns five values including three nullable booleans. Callers must handle the `nil`-means-default convention correctly or risk subtle bugs where the scalar form (returning `nil` pointers) is incorrectly treated as "disabled". + +#### Neutral +- `WorkflowData` gains three new nullable `*bool` fields (`ReactionIssues`, `ReactionPullRequests`, `ReactionDiscussions`), consistent with the existing `StatusCommentIssues`, `StatusCommentPullRequests`, `StatusCommentDiscussions` fields. `nil` means "inherit default (true)". +- The JSON schema for `on.reaction` is extended with a new `oneOf` branch for the object form. Existing scalar usages are unaffected. +- `BuildReactionCondition()` now delegates to the new `BuildReactionConditionForTargets(true, true, true)` — it remains the public API for callers that do not need target scoping, so no callers outside the compiler are broken. + +--- + +## Part 2 — Normative Specification (RFC 2119) + +> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119). + +### Reaction Configuration + +1. Implementations **MUST** accept `on.reaction` in both scalar form (string or integer reaction type) and object form (`{type?, issues?, pull-requests?, discussions?}`). +2. In object form, implementations **MUST** default `type` to `"eyes"` when the `type` key is absent. +3. In object form, implementations **MUST** default each of `issues`, `pull-requests`, and `discussions` to `true` when the key is absent. +4. Implementations **MUST** reject a `reaction` object where all three target toggles are explicitly set to `false`, returning a parse-time error; authors **SHOULD** use `reaction: none` to disable reactions entirely. +5. Implementations **MUST NOT** change the behavior of scalar reaction forms — a scalar value **MUST** be treated as if all three target toggles are `true`. + +### Reaction Step Condition Generation + +1. Implementations **MUST** generate the reaction step `if:` condition using only the event names corresponding to the enabled reaction targets. +2. Implementations **MUST NOT** include `pull_request` or `pull_request_review_comment` event names in the reaction step condition when `reaction.pull-requests` is `false`. +3. Implementations **MUST NOT** include `issues` or `issue_comment` event names in the reaction step condition when `reaction.issues` is `false`. +4. Implementations **MUST NOT** include `discussion` or `discussion_comment` event names in the reaction step condition when `reaction.discussions` is `false`. + +### Activation Permission Derivation + +1. Implementations **MUST** compute the activation job permission set as the union of permissions required for enabled reaction targets and permissions required for enabled status-comment targets, both evaluated against the real GitHub event types in the `on:` section. +2. Implementations **MUST NOT** grant `issues: write` for reaction purposes unless `reaction.issues` is `true` (or unset) AND at least one of `issues`, `issue_comment`, or `pull_request` event types is configured. +3. Implementations **MUST NOT** grant `pull-requests: write` for reaction purposes unless `reaction.pull-requests` is `true` (or unset) AND `pull_request_review_comment` event type is configured. +4. Implementations **MUST NOT** grant `discussions: write` for reaction purposes unless `reaction.discussions` is `true` (or unset) AND at least one of `discussion` or `discussion_comment` event types is configured. +5. Implementations **MUST** apply the same target-scoped permission derivation to both the activation job `permissions` block and the GitHub App token minting permissions (consistent with ADR-26535). + +### Fallback Behavior + +1. When the `on:` section is absent or unparseable at permission derivation time, implementations **MUST** fall back to the broad permission grant defined in ADR-26535, respecting the target toggle booleans for the broad-grant path as well. +2. Implementations **MUST NOT** ignore the `reactionIncludesPullRequests` flag in the fallback path — a configuration with `reaction.pull-requests: false` **MUST** suppress `pull-requests: write` even under the broad fallback. + +### Conformance + +An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance. This ADR extends ADR-26535; conformance with this ADR implies conformance with ADR-26535 except where this ADR explicitly supersedes it. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/24526383923) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index a88692a5b64..2997b224ad6 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -1879,11 +1879,43 @@ "type": "integer", "enum": [1, -1], "description": "YAML parses +1 and -1 without quotes as integers. These are converted to +1 and -1 strings respectively." + }, + { + "type": "object", + "additionalProperties": false, + "properties": { + "type": { + "oneOf": [ + { + "type": "string", + "enum": ["+1", "-1", "laugh", "confused", "heart", "hooray", "rocket", "eyes", "none"] + }, + { + "type": "integer", + "enum": [1, -1], + "description": "YAML parses +1 and -1 without quotes as integers. These are converted to +1 and -1 strings respectively." + } + ], + "description": "Reaction type. Defaults to 'eyes' when omitted." + }, + "issues": { + "type": "boolean", + "description": "Whether reactions are allowed for issue triggers (issues, issue_comment)." + }, + "pull-requests": { + "type": "boolean", + "description": "Whether reactions are allowed for pull request triggers (pull_request, pull_request_review_comment)." + }, + "discussions": { + "type": "boolean", + "description": "Whether reactions are allowed for discussion and discussion_comment triggers." + } + } } ], "default": "eyes", - "description": "AI reaction to add/remove on triggering item (one of: +1, -1, laugh, confused, heart, hooray, rocket, eyes, none). Use 'none' to disable reactions. Defaults to 'eyes' if not specified.", - "examples": ["eyes", "rocket", "+1", 1, -1, "none"] + "description": "AI reaction to add/remove on triggering item. Scalar form accepts one of: +1, -1, laugh, confused, heart, hooray, rocket, eyes, none. Object form implies enabled reactions and supports optional `issues`, `pull-requests`, and `discussions` fields to control trigger groups independently; use `type` to choose the reaction emoji (defaults to `eyes` when omitted). Use 'none' to disable reactions.", + "examples": ["eyes", "rocket", "+1", 1, -1, "none", { "type": "rocket", "pull-requests": false }, { "issues": false, "discussions": true }] }, "status-comment": { "oneOf": [ diff --git a/pkg/workflow/activation_permissions_scope_test.go b/pkg/workflow/activation_permissions_scope_test.go index f5418e296f0..f03db293571 100644 --- a/pkg/workflow/activation_permissions_scope_test.go +++ b/pkg/workflow/activation_permissions_scope_test.go @@ -78,6 +78,44 @@ engine: copilot assert.NotContains(t, activationJobSection, "discussions: write", "activation job should not include discussions: write for PR review comment reactions") } +func TestActivationPermissionsReactionPullRequestsDisabled(t *testing.T) { + tmpDir := testutil.TempDir(t, "activation-perms-reaction-pr-disabled") + testFile := filepath.Join(tmpDir, "reaction-pr-disabled.md") + testContent := `--- +on: + reaction: + type: eyes + pull-requests: false + status-comment: false + pull_request: + types: [opened] +engine: copilot +--- + +# Reaction pull_request target disabled +` + + 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, "Add eyes reaction for immediate feedback", "activation job should include reaction step") + assert.Contains(t, activationJobSection, "github.event_name == 'issues'", "reaction condition should still include issues when reaction.issues is enabled") + assert.Contains(t, activationJobSection, "github.event_name == 'discussion'", "reaction condition should still include discussions when reaction.discussions is enabled") + assert.NotContains(t, activationJobSection, "github.event_name == 'pull_request'", "reaction condition should exclude pull_request when reaction.pull-requests is false") + assert.NotContains(t, activationJobSection, "github.event_name == 'pull_request_review_comment'", "reaction condition should exclude pull_request_review_comment when reaction.pull-requests is false") + assert.NotContains(t, activationJobSection, "issues: write", "activation job should not include issues: write when pull request reactions are disabled") + assert.NotContains(t, activationJobSection, "pull-requests: write", "activation job should not include pull-requests: write when pull request reactions are disabled") + assert.NotContains(t, activationJobSection, "discussions: write", "activation job should not include discussions: write when no discussion triggers are configured") +} + func TestActivationPermissionsStatusCommentDiscussionsDisabled(t *testing.T) { tmpDir := testutil.TempDir(t, "activation-perms-status-comment-discussions-disabled") testFile := filepath.Join(tmpDir, "status-comment-discussions-disabled.md") @@ -116,7 +154,16 @@ engine: copilot func TestAddActivationInteractionPermissionsMapFallsBackOnInvalidOnYAML(t *testing.T) { permsMap := map[PermissionScope]PermissionLevel{} - addActivationInteractionPermissionsMap(permsMap, "on: [", true, true, true, true, true) + addActivationInteractionPermissionsMap(permsMap, "on: [", + /* hasReaction */ true, + /* reactionIncludesIssues */ true, + /* reactionIncludesPullRequests */ true, + /* reactionIncludesDiscussions */ true, + /* hasStatusComment */ true, + /* statusCommentIncludesIssues */ true, + /* statusCommentIncludesPullRequests */ true, + /* statusCommentIncludesDiscussions */ true, + ) assert.Equal(t, PermissionWrite, permsMap[PermissionIssues], "fallback should include issues:write") assert.Equal(t, PermissionWrite, permsMap[PermissionPullRequests], "fallback should include pull-requests:write") @@ -126,7 +173,16 @@ func TestAddActivationInteractionPermissionsMapFallsBackOnInvalidOnYAML(t *testi func TestAddActivationInteractionPermissionsMapFallbackRespectsStatusCommentDiscussionsToggle(t *testing.T) { permsMap := map[PermissionScope]PermissionLevel{} - addActivationInteractionPermissionsMap(permsMap, "name: no-on-key", false, true, true, true, false) + addActivationInteractionPermissionsMap(permsMap, "name: no-on-key", + /* hasReaction */ false, + /* reactionIncludesIssues */ true, + /* reactionIncludesPullRequests */ true, + /* reactionIncludesDiscussions */ true, + /* hasStatusComment */ true, + /* statusCommentIncludesIssues */ true, + /* statusCommentIncludesPullRequests */ true, + /* statusCommentIncludesDiscussions */ false, + ) assert.Equal(t, PermissionWrite, permsMap[PermissionIssues], "fallback should include issues:write for status comments") _, hasPullRequests := permsMap[PermissionPullRequests] @@ -175,7 +231,16 @@ engine: copilot func TestAddActivationInteractionPermissionsMapFallbackRespectsStatusCommentIssuesToggle(t *testing.T) { permsMap := map[PermissionScope]PermissionLevel{} - addActivationInteractionPermissionsMap(permsMap, "name: no-on-key", false, true, false, false, true) + addActivationInteractionPermissionsMap(permsMap, "name: no-on-key", + /* hasReaction */ false, + /* reactionIncludesIssues */ true, + /* reactionIncludesPullRequests */ true, + /* reactionIncludesDiscussions */ true, + /* hasStatusComment */ true, + /* statusCommentIncludesIssues */ false, + /* statusCommentIncludesPullRequests */ false, + /* statusCommentIncludesDiscussions */ true, + ) _, hasIssues := permsMap[PermissionIssues] _, hasPullRequests := permsMap[PermissionPullRequests] @@ -249,7 +314,16 @@ engine: copilot func TestAddActivationInteractionPermissionsMapFallbackRespectsStatusCommentPullRequestsToggle(t *testing.T) { permsMap := map[PermissionScope]PermissionLevel{} - addActivationInteractionPermissionsMap(permsMap, "name: no-on-key", false, true, false, false, true) + addActivationInteractionPermissionsMap(permsMap, "name: no-on-key", + /* hasReaction */ false, + /* reactionIncludesIssues */ true, + /* reactionIncludesPullRequests */ true, + /* reactionIncludesDiscussions */ true, + /* hasStatusComment */ true, + /* statusCommentIncludesIssues */ false, + /* statusCommentIncludesPullRequests */ false, + /* statusCommentIncludesDiscussions */ true, + ) _, hasIssues := permsMap[PermissionIssues] _, hasPullRequests := permsMap[PermissionPullRequests] diff --git a/pkg/workflow/compiler_activation_job.go b/pkg/workflow/compiler_activation_job.go index 07a7d70aa50..729f92c8484 100644 --- a/pkg/workflow/compiler_activation_job.go +++ b/pkg/workflow/compiler_activation_job.go @@ -117,6 +117,9 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate // Compute reaction/comment/label flags early so the app token and reaction steps can be // inserted right after generate_aw_info for fast user feedback. hasReaction := data.AIReaction != "" && data.AIReaction != "none" + reactionIncludesIssues := shouldIncludeIssueReactions(data) + reactionIncludesPullRequests := shouldIncludePullRequestReactions(data) + reactionIncludesDiscussions := shouldIncludeDiscussionReactions(data) hasStatusComment := data.StatusComment != nil && *data.StatusComment statusCommentIncludesIssues := shouldIncludeIssueStatusComments(data) statusCommentIncludesPullRequests := shouldIncludePullRequestStatusComments(data) @@ -144,6 +147,9 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate appPerms, data.On, hasReaction, + reactionIncludesIssues, + reactionIncludesPullRequests, + reactionIncludesDiscussions, hasStatusComment, statusCommentIncludesIssues, statusCommentIncludesPullRequests, @@ -171,7 +177,11 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate // possible. generate_aw_info runs first so its data is captured even if the reaction fails. // This runs in the activation job so it can use any configured github-token or github-app. if hasReaction { - reactionCondition := BuildReactionCondition() + reactionCondition := BuildReactionConditionForTargets( + reactionIncludesIssues, + reactionIncludesPullRequests, + reactionIncludesDiscussions, + ) steps = append(steps, fmt.Sprintf(" - name: Add %s reaction for immediate feedback\n", data.AIReaction)) steps = append(steps, " id: react\n") @@ -581,6 +591,9 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate permsMap, data.On, hasReaction, + reactionIncludesIssues, + reactionIncludesPullRequests, + reactionIncludesDiscussions, hasStatusComment, statusCommentIncludesIssues, statusCommentIncludesPullRequests, @@ -643,6 +656,9 @@ func addActivationInteractionPermissions( perms *Permissions, onSection string, hasReaction bool, + reactionIncludesIssues bool, + reactionIncludesPullRequests bool, + reactionIncludesDiscussions bool, hasStatusComment bool, statusCommentIncludesIssues bool, statusCommentIncludesPullRequests bool, @@ -656,6 +672,9 @@ func addActivationInteractionPermissions( permsMap, onSection, hasReaction, + reactionIncludesIssues, + reactionIncludesPullRequests, + reactionIncludesDiscussions, hasStatusComment, statusCommentIncludesIssues, statusCommentIncludesPullRequests, @@ -670,6 +689,9 @@ func addActivationInteractionPermissionsMap( permsMap map[PermissionScope]PermissionLevel, onSection string, hasReaction bool, + reactionIncludesIssues bool, + reactionIncludesPullRequests bool, + reactionIncludesDiscussions bool, hasStatusComment bool, statusCommentIncludesIssues bool, statusCommentIncludesPullRequests bool, @@ -686,6 +708,9 @@ func addActivationInteractionPermissionsMap( addBroadActivationInteractionPermissions( permsMap, hasReaction, + reactionIncludesIssues, + reactionIncludesPullRequests, + reactionIncludesDiscussions, hasStatusComment, statusCommentIncludesIssues, statusCommentIncludesPullRequests, @@ -700,6 +725,9 @@ func addActivationInteractionPermissionsMap( addBroadActivationInteractionPermissions( permsMap, hasReaction, + reactionIncludesIssues, + reactionIncludesPullRequests, + reactionIncludesDiscussions, hasStatusComment, statusCommentIncludesIssues, statusCommentIncludesPullRequests, @@ -717,15 +745,20 @@ func addActivationInteractionPermissionsMap( if hasReaction { // Reactions on issues, issue comments, and pull requests all use issues endpoints. - if hasIssuesEvent || hasIssueCommentEvent || hasPullRequestEvent { + // Both issue and pull request reactions require issues:write because PR reactions + // are created via /issues/{number}/reactions. + needsIssuesWriteForIssueEvents := reactionIncludesIssues && (hasIssuesEvent || hasIssueCommentEvent) + needsIssuesWriteForPullRequestEvents := reactionIncludesPullRequests && hasPullRequestEvent + needsIssuesWriteForReaction := needsIssuesWriteForIssueEvents || needsIssuesWriteForPullRequestEvents + if needsIssuesWriteForReaction { permsMap[PermissionIssues] = PermissionWrite } // Reactions on PR review comments use pull request review comment endpoints. - if hasPullRequestReviewCommentEvent { + if reactionIncludesPullRequests && hasPullRequestReviewCommentEvent { permsMap[PermissionPullRequests] = PermissionWrite } // Reactions on discussions use GraphQL discussion APIs. - if hasDiscussionEvent || hasDiscussionCommentEvent { + if reactionIncludesDiscussions && (hasDiscussionEvent || hasDiscussionCommentEvent) { permsMap[PermissionDiscussions] = PermissionWrite } } @@ -746,6 +779,9 @@ func addActivationInteractionPermissionsMap( func addBroadActivationInteractionPermissions( permsMap map[PermissionScope]PermissionLevel, hasReaction bool, + reactionIncludesIssues bool, + reactionIncludesPullRequests bool, + reactionIncludesDiscussions bool, hasStatusComment bool, statusCommentIncludesIssues bool, statusCommentIncludesPullRequests bool, @@ -755,17 +791,40 @@ func addBroadActivationInteractionPermissions( return } - if hasReaction || statusCommentIncludesIssues || statusCommentIncludesPullRequests { + needsIssuesWriteForReaction := hasReaction && (reactionIncludesIssues || reactionIncludesPullRequests) + needsIssuesWriteForStatusComment := statusCommentIncludesIssues || statusCommentIncludesPullRequests + if needsIssuesWriteForReaction || needsIssuesWriteForStatusComment { permsMap[PermissionIssues] = PermissionWrite } - if hasReaction { + if hasReaction && reactionIncludesPullRequests { permsMap[PermissionPullRequests] = PermissionWrite } - if hasReaction || statusCommentIncludesDiscussions { + if (hasReaction && reactionIncludesDiscussions) || statusCommentIncludesDiscussions { permsMap[PermissionDiscussions] = PermissionWrite } } +func shouldIncludeIssueReactions(data *WorkflowData) bool { + if data == nil || data.ReactionIssues == nil { + return true + } + return *data.ReactionIssues +} + +func shouldIncludePullRequestReactions(data *WorkflowData) bool { + if data == nil || data.ReactionPullRequests == nil { + return true + } + return *data.ReactionPullRequests +} + +func shouldIncludeDiscussionReactions(data *WorkflowData) bool { + if data == nil || data.ReactionDiscussions == nil { + return true + } + return *data.ReactionDiscussions +} + func shouldIncludeIssueStatusComments(data *WorkflowData) bool { if data == nil || data.StatusCommentIssues == nil { return true diff --git a/pkg/workflow/compiler_safe_outputs.go b/pkg/workflow/compiler_safe_outputs.go index ebc092ebee9..fb3b7e8eb0e 100644 --- a/pkg/workflow/compiler_safe_outputs.go +++ b/pkg/workflow/compiler_safe_outputs.go @@ -50,7 +50,7 @@ func (c *Compiler) parseOnSection(frontmatter map[string]any, workflowData *Work // Extract reaction from on section if reactionValue, hasReactionField := onMap["reaction"]; hasReactionField { hasReaction = true - reactionStr, err := parseReactionValue(reactionValue) + reactionStr, reactionIssues, reactionPullRequests, reactionDiscussions, err := parseReactionConfig(reactionValue) if err != nil { return err } @@ -60,6 +60,9 @@ func (c *Compiler) parseOnSection(frontmatter map[string]any, workflowData *Work } // Set AIReaction even if it's "none" - "none" explicitly disables reactions workflowData.AIReaction = reactionStr + workflowData.ReactionIssues = reactionIssues + workflowData.ReactionPullRequests = reactionPullRequests + workflowData.ReactionDiscussions = reactionDiscussions } // Extract status-comment from on section diff --git a/pkg/workflow/compiler_safe_outputs_test.go b/pkg/workflow/compiler_safe_outputs_test.go index 47deeb4b578..6735e333247 100644 --- a/pkg/workflow/compiler_safe_outputs_test.go +++ b/pkg/workflow/compiler_safe_outputs_test.go @@ -1066,24 +1066,27 @@ func TestApplyDefaultToolsComplexScenarios(t *testing.T) { // TestParseOnSectionReactionMapFormat tests reaction with map format func TestParseOnSectionReactionMapFormat(t *testing.T) { - // This test covers the case where reaction might be provided as a map - // though the current implementation expects string or int c := &Compiler{} workflowData := &WorkflowData{} - // Test that invalid type (map) is handled frontmatter := map[string]any{ "on": map[string]any{ "reaction": map[string]any{ - "type": "heart", + "type": "heart", + "pull-requests": false, }, }, } err := c.parseOnSection(frontmatter, workflowData, "/path/to/test.md") - - // The parseReactionValue function should return an error for map type - assert.Error(t, err, "Should error on map type reaction") + require.NoError(t, err, "reaction map format should be accepted") + assert.Equal(t, "heart", workflowData.AIReaction, "reaction type should be parsed from reaction.type") + require.NotNil(t, workflowData.ReactionIssues, "reaction issue target flag should be set") + assert.True(t, *workflowData.ReactionIssues, "reaction issues target should default to true") + require.NotNil(t, workflowData.ReactionPullRequests, "reaction pull request target flag should be set") + assert.False(t, *workflowData.ReactionPullRequests, "reaction pull request target should match parsed value") + require.NotNil(t, workflowData.ReactionDiscussions, "reaction discussion target flag should be set") + assert.True(t, *workflowData.ReactionDiscussions, "reaction discussions target should default to true") } // TestCompilerNeedsGitCommandsAllOutputTypes tests all safe output types for git command requirements diff --git a/pkg/workflow/compiler_types.go b/pkg/workflow/compiler_types.go index 0f1b9009c8d..93de8a1b233 100644 --- a/pkg/workflow/compiler_types.go +++ b/pkg/workflow/compiler_types.go @@ -436,6 +436,9 @@ type WorkflowData struct { LabelCommandOtherEvents map[string]any // for merging label-command with other events LabelCommandRemoveLabel bool // whether to automatically remove the triggering label (default: true) AIReaction string // AI reaction type like "eyes", "heart", etc. + ReactionIssues *bool // whether reactions are allowed on issues/issue_comment triggers (default: true) + ReactionPullRequests *bool // whether reactions are allowed on pull_request/pull_request_review_comment triggers (default: true) + ReactionDiscussions *bool // whether reactions are allowed on discussion/discussion_comment triggers (default: true) StatusComment *bool // whether to post status comments (default: true when ai-reaction is set, false otherwise) StatusCommentIssues *bool // whether status comments are allowed on issues/issue_comment triggers (default: true) StatusCommentPullRequests *bool // whether status comments are allowed on pull_request/pull_request_review_comment triggers (default: true) diff --git a/pkg/workflow/expression_builder.go b/pkg/workflow/expression_builder.go index 2a07fc1b08d..61ab6ca52cc 100644 --- a/pkg/workflow/expression_builder.go +++ b/pkg/workflow/expression_builder.go @@ -65,7 +65,18 @@ func BuildAnd(left ConditionNode, right ConditionNode) ConditionNode { // BuildReactionCondition creates a condition tree for the add_reaction job func BuildReactionCondition() ConditionNode { expressionBuilderLog.Print("Building reaction condition for multiple event types") - return buildReactionLikeCondition(true, true, true) + return BuildReactionConditionForTargets(true, true, true) +} + +// BuildReactionConditionForTargets creates a condition tree for reactions scoped to target groups. +func BuildReactionConditionForTargets(includeIssues bool, includePullRequests bool, includeDiscussions bool) ConditionNode { + expressionBuilderLog.Printf( + "Building reaction condition: includeIssues=%t includePullRequests=%t includeDiscussions=%t", + includeIssues, + includePullRequests, + includeDiscussions, + ) + return buildReactionLikeCondition(includeIssues, includePullRequests, includeDiscussions) } // BuildStatusCommentCondition creates a condition tree for activation status comments. diff --git a/pkg/workflow/expressions_test.go b/pkg/workflow/expressions_test.go index 7f20fa216ea..27de712715d 100644 --- a/pkg/workflow/expressions_test.go +++ b/pkg/workflow/expressions_test.go @@ -176,6 +176,21 @@ func TestBuildReactionCondition(t *testing.T) { } } +func TestBuildReactionConditionForTargetsExcludesPullRequests(t *testing.T) { + result := BuildReactionConditionForTargets(true, false, true) + rendered := result.Render() + + if strings.Contains(rendered, "github.event_name == 'pull_request'") { + t.Errorf("Expected pull_request event to be excluded when pull request reactions are disabled, got: %s", rendered) + } + if strings.Contains(rendered, "github.event_name == 'pull_request_review_comment'") { + t.Errorf("Expected pull_request_review_comment event to be excluded when pull request reactions are disabled, got: %s", rendered) + } + if !strings.Contains(rendered, "github.event_name == 'issues'") { + t.Errorf("Expected issues event to remain when issue reactions are enabled, got: %s", rendered) + } +} + func TestFunctionCallNode_Render(t *testing.T) { tests := []struct { name string diff --git a/pkg/workflow/reactions.go b/pkg/workflow/reactions.go index 1271343e3c3..a85dab15666 100644 --- a/pkg/workflow/reactions.go +++ b/pkg/workflow/reactions.go @@ -1,6 +1,7 @@ package workflow import ( + "errors" "fmt" "github.com/github/gh-aw/pkg/logger" @@ -82,6 +83,62 @@ func parseReactionValue(value any) (string, error) { } } +// parseReactionConfig parses reaction configuration from frontmatter. +// Supported formats: +// - scalar (string/int): reaction type only +// - object: {type, issues, pull-requests, discussions} +func parseReactionConfig(value any) (string, *bool, *bool, *bool, error) { + if reactionMap, ok := value.(map[string]any); ok { + reactionType := "eyes" + if typeValue, hasType := reactionMap["type"]; hasType { + parsedType, err := parseReactionValue(typeValue) + if err != nil { + return "", nil, nil, nil, err + } + reactionType = parsedType + } + + reactionIssues := true + if issuesValue, hasIssues := reactionMap["issues"]; hasIssues { + issuesBool, ok := issuesValue.(bool) + if !ok { + return "", nil, nil, nil, fmt.Errorf("reaction.issues must be a boolean value, got %T", issuesValue) + } + reactionIssues = issuesBool + } + + reactionPullRequests := true + if pullRequestsValue, hasPullRequests := reactionMap["pull-requests"]; hasPullRequests { + pullRequestsBool, ok := pullRequestsValue.(bool) + if !ok { + return "", nil, nil, nil, fmt.Errorf("reaction.pull-requests must be a boolean value, got %T", pullRequestsValue) + } + reactionPullRequests = pullRequestsBool + } + + reactionDiscussions := true + if discussionsValue, hasDiscussions := reactionMap["discussions"]; hasDiscussions { + discussionsBool, ok := discussionsValue.(bool) + if !ok { + return "", nil, nil, nil, fmt.Errorf("reaction.discussions must be a boolean value, got %T", discussionsValue) + } + reactionDiscussions = discussionsBool + } + + if !reactionIssues && !reactionPullRequests && !reactionDiscussions { + return "", nil, nil, nil, errors.New("reaction object requires at least one target to be enabled (issues, pull-requests, or discussions)") + } + + return reactionType, &reactionIssues, &reactionPullRequests, &reactionDiscussions, nil + } + + reactionType, err := parseReactionValue(value) + if err != nil { + return "", nil, nil, nil, err + } + return reactionType, nil, nil, nil, nil +} + // intToReactionString converts an integer to a reaction string. // Only 1 (+1) and -1 are valid integer values for reactions. func intToReactionString(v int64) (string, error) { diff --git a/pkg/workflow/reactions_test.go b/pkg/workflow/reactions_test.go index a4b69f67d97..f08b8984b1f 100644 --- a/pkg/workflow/reactions_test.go +++ b/pkg/workflow/reactions_test.go @@ -174,3 +174,39 @@ func TestIntToReactionString(t *testing.T) { }) } } + +func TestParseReactionConfigMap(t *testing.T) { + reaction, issues, pullRequests, discussions, err := parseReactionConfig(map[string]any{ + "type": "rocket", + "issues": false, + "pull-requests": true, + "discussions": false, + }) + if err != nil { + t.Errorf("parseReactionConfig(map) returned unexpected error: %v", err) + } + if reaction != "rocket" { + t.Errorf("Expected reaction type 'rocket', got %q", reaction) + } + if issues == nil || *issues { + t.Errorf("Expected issues target false, got %v", issues) + } + if pullRequests == nil || !*pullRequests { + t.Errorf("Expected pull-requests target true, got %v", pullRequests) + } + if discussions == nil || *discussions { + t.Errorf("Expected discussions target false, got %v", discussions) + } +} + +func TestParseReactionConfigMapAllTargetsDisabled(t *testing.T) { + _, _, _, _, err := parseReactionConfig(map[string]any{ + "type": "eyes", + "issues": false, + "pull-requests": false, + "discussions": false, + }) + if err == nil { + t.Fatal("Expected parseReactionConfig to fail when all reaction targets are disabled") + } +}