diff --git a/.github/workflows/ai-moderator.lock.yml b/.github/workflows/ai-moderator.lock.yml index d8b5425059..ac50515f40 100644 --- a/.github/workflows/ai-moderator.lock.yml +++ b/.github/workflows/ai-moderator.lock.yml @@ -866,7 +866,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1026,7 +1025,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/archie.lock.yml b/.github/workflows/archie.lock.yml index 748f2dbd91..00caa28295 100644 --- a/.github/workflows/archie.lock.yml +++ b/.github/workflows/archie.lock.yml @@ -848,7 +848,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1119,7 +1118,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/brave.lock.yml b/.github/workflows/brave.lock.yml index 550608718c..90b23842a7 100644 --- a/.github/workflows/brave.lock.yml +++ b/.github/workflows/brave.lock.yml @@ -840,7 +840,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1108,7 +1107,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/ci-doctor.lock.yml b/.github/workflows/ci-doctor.lock.yml index 4d966720d0..b413900223 100644 --- a/.github/workflows/ci-doctor.lock.yml +++ b/.github/workflows/ci-doctor.lock.yml @@ -1016,7 +1016,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1275,7 +1274,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/cloclo.lock.yml b/.github/workflows/cloclo.lock.yml index 1d5f1a35ec..254a3ff016 100644 --- a/.github/workflows/cloclo.lock.yml +++ b/.github/workflows/cloclo.lock.yml @@ -1172,7 +1172,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: write - discussions: write issues: write pull-requests: write outputs: @@ -1484,7 +1483,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: write - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/contribution-check.lock.yml b/.github/workflows/contribution-check.lock.yml index 85f1db559b..84653dc46d 100644 --- a/.github/workflows/contribution-check.lock.yml +++ b/.github/workflows/contribution-check.lock.yml @@ -892,7 +892,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1100,7 +1099,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/craft.lock.yml b/.github/workflows/craft.lock.yml index 8ea46f2b1d..f91749789b 100644 --- a/.github/workflows/craft.lock.yml +++ b/.github/workflows/craft.lock.yml @@ -875,7 +875,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: write - discussions: write issues: write pull-requests: write outputs: @@ -1143,7 +1142,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: write - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/daily-assign-issue-to-user.lock.yml b/.github/workflows/daily-assign-issue-to-user.lock.yml index 891f8aaf37..89e3f7b302 100644 --- a/.github/workflows/daily-assign-issue-to-user.lock.yml +++ b/.github/workflows/daily-assign-issue-to-user.lock.yml @@ -837,7 +837,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1045,7 +1044,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/daily-cli-performance.lock.yml b/.github/workflows/daily-cli-performance.lock.yml index ba2e87a938..8839e56475 100644 --- a/.github/workflows/daily-cli-performance.lock.yml +++ b/.github/workflows/daily-cli-performance.lock.yml @@ -1070,7 +1070,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1351,7 +1350,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/daily-fact.lock.yml b/.github/workflows/daily-fact.lock.yml index df3df936db..3c9ee19780 100644 --- a/.github/workflows/daily-fact.lock.yml +++ b/.github/workflows/daily-fact.lock.yml @@ -779,7 +779,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -970,7 +969,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/dev-hawk.lock.yml b/.github/workflows/dev-hawk.lock.yml index 1a6b6115ab..26fd3920a3 100644 --- a/.github/workflows/dev-hawk.lock.yml +++ b/.github/workflows/dev-hawk.lock.yml @@ -907,7 +907,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1147,7 +1146,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/discussion-task-miner.lock.yml b/.github/workflows/discussion-task-miner.lock.yml index b4c3188137..572bf0fe25 100644 --- a/.github/workflows/discussion-task-miner.lock.yml +++ b/.github/workflows/discussion-task-miner.lock.yml @@ -945,7 +945,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1227,7 +1226,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/draft-pr-cleanup.lock.yml b/.github/workflows/draft-pr-cleanup.lock.yml index e631b35237..2e9b94b616 100644 --- a/.github/workflows/draft-pr-cleanup.lock.yml +++ b/.github/workflows/draft-pr-cleanup.lock.yml @@ -866,7 +866,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1075,7 +1074,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/issue-monster.lock.yml b/.github/workflows/issue-monster.lock.yml index 6d6a0885bc..0a87a66fbc 100644 --- a/.github/workflows/issue-monster.lock.yml +++ b/.github/workflows/issue-monster.lock.yml @@ -862,7 +862,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1128,7 +1127,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/issue-triage-agent.lock.yml b/.github/workflows/issue-triage-agent.lock.yml index c6d03da5df..5dc42d3766 100644 --- a/.github/workflows/issue-triage-agent.lock.yml +++ b/.github/workflows/issue-triage-agent.lock.yml @@ -804,7 +804,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1011,7 +1010,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/pr-triage-agent.lock.yml b/.github/workflows/pr-triage-agent.lock.yml index 28352f7854..e0cbd222b8 100644 --- a/.github/workflows/pr-triage-agent.lock.yml +++ b/.github/workflows/pr-triage-agent.lock.yml @@ -949,7 +949,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1227,7 +1226,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/q.lock.yml b/.github/workflows/q.lock.yml index f663c462d6..ddbdcec0d9 100644 --- a/.github/workflows/q.lock.yml +++ b/.github/workflows/q.lock.yml @@ -1020,7 +1020,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: write - discussions: write issues: write pull-requests: write outputs: @@ -1312,7 +1311,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: write - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/refiner.lock.yml b/.github/workflows/refiner.lock.yml index bac192a794..a0c6456b03 100644 --- a/.github/workflows/refiner.lock.yml +++ b/.github/workflows/refiner.lock.yml @@ -881,7 +881,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: write - discussions: write issues: write pull-requests: write outputs: @@ -1136,7 +1135,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: write - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/scout.lock.yml b/.github/workflows/scout.lock.yml index 6b2c8cb1d7..7a9f3e33ab 100644 --- a/.github/workflows/scout.lock.yml +++ b/.github/workflows/scout.lock.yml @@ -1011,7 +1011,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1306,7 +1305,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/smoke-claude.lock.yml b/.github/workflows/smoke-claude.lock.yml index ed228c9e25..10e2cdfe47 100644 --- a/.github/workflows/smoke-claude.lock.yml +++ b/.github/workflows/smoke-claude.lock.yml @@ -2082,7 +2082,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: write - discussions: write issues: write pull-requests: write outputs: @@ -2374,7 +2373,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: write - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/smoke-codex.lock.yml b/.github/workflows/smoke-codex.lock.yml index 3c9b1991b8..50037b2db6 100644 --- a/.github/workflows/smoke-codex.lock.yml +++ b/.github/workflows/smoke-codex.lock.yml @@ -1266,7 +1266,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1533,7 +1532,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/smoke-copilot-sdk.lock.yml b/.github/workflows/smoke-copilot-sdk.lock.yml index a3a0fd9d0f..41026a1576 100644 --- a/.github/workflows/smoke-copilot-sdk.lock.yml +++ b/.github/workflows/smoke-copilot-sdk.lock.yml @@ -1170,7 +1170,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1454,7 +1453,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/smoke-opencode.lock.yml b/.github/workflows/smoke-opencode.lock.yml index 3d45c2fa93..0655d14d7f 100644 --- a/.github/workflows/smoke-opencode.lock.yml +++ b/.github/workflows/smoke-opencode.lock.yml @@ -1489,7 +1489,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1810,7 +1809,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/smoke-project.lock.yml b/.github/workflows/smoke-project.lock.yml index 5f4c68a760..33732d89df 100644 --- a/.github/workflows/smoke-project.lock.yml +++ b/.github/workflows/smoke-project.lock.yml @@ -1276,7 +1276,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: write - discussions: write issues: write pull-requests: write outputs: @@ -1566,7 +1565,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: write - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/smoke-temporary-id.lock.yml b/.github/workflows/smoke-temporary-id.lock.yml index f7cac5d6e6..be51e4c541 100644 --- a/.github/workflows/smoke-temporary-id.lock.yml +++ b/.github/workflows/smoke-temporary-id.lock.yml @@ -932,7 +932,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1207,7 +1206,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/smoke-test-tools.lock.yml b/.github/workflows/smoke-test-tools.lock.yml index 6704345387..a57aaf7fe5 100644 --- a/.github/workflows/smoke-test-tools.lock.yml +++ b/.github/workflows/smoke-test-tools.lock.yml @@ -826,7 +826,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1066,7 +1065,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/sub-issue-closer.lock.yml b/.github/workflows/sub-issue-closer.lock.yml index 37b5f5da5a..0f3636327b 100644 --- a/.github/workflows/sub-issue-closer.lock.yml +++ b/.github/workflows/sub-issue-closer.lock.yml @@ -879,7 +879,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1087,7 +1086,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/technical-doc-writer.lock.yml b/.github/workflows/technical-doc-writer.lock.yml index 1878965370..d029fb02f5 100644 --- a/.github/workflows/technical-doc-writer.lock.yml +++ b/.github/workflows/technical-doc-writer.lock.yml @@ -960,7 +960,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: write - discussions: write issues: write pull-requests: write outputs: @@ -1184,7 +1183,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: write - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/unbloat-docs.lock.yml b/.github/workflows/unbloat-docs.lock.yml index 27317da4d3..f90c008267 100644 --- a/.github/workflows/unbloat-docs.lock.yml +++ b/.github/workflows/unbloat-docs.lock.yml @@ -1104,7 +1104,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: write - discussions: write issues: write pull-requests: write outputs: @@ -1403,7 +1402,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: write - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/.github/workflows/workflow-health-manager.lock.yml b/.github/workflows/workflow-health-manager.lock.yml index 00e7a2128d..c01767240a 100644 --- a/.github/workflows/workflow-health-manager.lock.yml +++ b/.github/workflows/workflow-health-manager.lock.yml @@ -1009,7 +1009,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write outputs: @@ -1316,7 +1315,6 @@ jobs: runs-on: ubuntu-slim permissions: contents: read - discussions: write issues: write pull-requests: write timeout-minutes: 15 diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 9409c952d5..60ed14747a 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -4802,9 +4802,8 @@ }, "discussion": { "type": "boolean", - "const": true, - "description": "DEPRECATED: This field is deprecated and will be removed in a future version. The add_comment handler now automatically detects whether to target discussions based on context (discussion/discussion_comment events) or the item_number field provided by the agent. Remove this field from your workflow configuration.", - "deprecated": true + "description": "Enable discussion comment support. Set to true to enable discussions permission for commenting on discussions.", + "default": false }, "hide-older-comments": { "type": "boolean", @@ -5729,6 +5728,11 @@ "type": "string", "description": "Target repository in format 'owner/repo' for cross-repository comment hiding. Takes precedence over trial target repo settings." }, + "discussion": { + "type": "boolean", + "description": "Enable discussion comment support. Set to true to enable discussions permission for hiding discussion comments.", + "default": false + }, "allowed-reasons": { "type": "array", "description": "List of allowed reasons for hiding comments. Default: all reasons allowed (spam, abuse, off_topic, outdated, resolved).", diff --git a/pkg/workflow/add_comment.go b/pkg/workflow/add_comment.go index ffd52ea953..bd117ba188 100644 --- a/pkg/workflow/add_comment.go +++ b/pkg/workflow/add_comment.go @@ -20,7 +20,7 @@ type AddCommentsConfig struct { Target string `yaml:"target,omitempty"` // Target for comments: "triggering" (default), "*" (any issue), or explicit issue number TargetRepoSlug string `yaml:"target-repo,omitempty"` // Target repository in format "owner/repo" for cross-repository comments AllowedRepos []string `yaml:"allowed-repos,omitempty"` // List of additional repositories that comments can be added to (additionally to the target-repo) - Discussion *bool `yaml:"discussion,omitempty"` // Target discussion comments instead of issue/PR comments. Must be true if present. + Discussion *bool `yaml:"discussion,omitempty"` // Enable discussion comment support. Set to true to enable discussions permission. HideOlderComments bool `yaml:"hide-older-comments,omitempty"` // When true, minimizes/hides all previous comments from the same workflow before creating the new comment AllowedReasons []string `yaml:"allowed-reasons,omitempty"` // List of allowed reasons for hiding older comments (default: all reasons allowed) } @@ -113,6 +113,15 @@ func (c *Compiler) buildCreateOutputAddCommentJob(data *WorkflowData, mainJobNam needs = append(needs, createPullRequestJobName) } + // Determine permissions based on whether discussions are enabled + // Default is false (no discussion permission) unless explicitly set to true + var permissions *Permissions + if data.SafeOutputs.AddComments.Discussion != nil && *data.SafeOutputs.AddComments.Discussion { + permissions = NewPermissionsContentsReadIssuesWritePRWriteDiscussionsWrite() + } else { + permissions = NewPermissionsContentsReadIssuesWritePRWrite() + } + // Use the shared builder function to create the job return c.buildSafeOutputJob(data, SafeOutputJobConfig{ JobName: "add_comment", @@ -121,7 +130,7 @@ func (c *Compiler) buildCreateOutputAddCommentJob(data *WorkflowData, mainJobNam MainJobName: mainJobName, CustomEnvVars: customEnvVars, Script: getAddCommentScript(), - Permissions: NewPermissionsContentsReadIssuesWritePRWriteDiscussionsWrite(), + Permissions: permissions, Outputs: outputs, Condition: jobCondition, Needs: needs, @@ -158,11 +167,5 @@ func (c *Compiler) parseCommentsConfig(outputMap map[string]any) *AddCommentsCon return nil // Invalid configuration, return nil to cause validation error } - // Validate discussion field - must be true if present - if config.Discussion != nil && !*config.Discussion { - addCommentLog.Print("Invalid discussion: must be true if present") - return nil // Invalid configuration, return nil to cause validation error - } - return &config } diff --git a/pkg/workflow/add_comment_hide_comment_parsing_test.go b/pkg/workflow/add_comment_hide_comment_parsing_test.go new file mode 100644 index 0000000000..d30be443a5 --- /dev/null +++ b/pkg/workflow/add_comment_hide_comment_parsing_test.go @@ -0,0 +1,279 @@ +//go:build !integration + +package workflow + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestParseHideCommentConfig tests that all fields are properly parsed +func TestParseHideCommentConfig(t *testing.T) { + tests := []struct { + name string + inputMap map[string]any + expected *HideCommentConfig + isNil bool + }{ + { + name: "all fields parsed correctly", + inputMap: map[string]any{ + "hide-comment": map[string]any{ + "max": 10, + "target-repo": "owner/repo", + "discussion": true, + "allowed-reasons": []any{"spam", "outdated"}, + }, + }, + expected: &HideCommentConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{ + Max: 10, + }, + SafeOutputTargetConfig: SafeOutputTargetConfig{ + TargetRepoSlug: "owner/repo", + }, + Discussion: ptrBool(true), + AllowedReasons: []string{"spam", "outdated"}, + }, + }, + { + name: "discussion false is preserved", + inputMap: map[string]any{ + "hide-comment": map[string]any{ + "max": 5, + "discussion": false, + }, + }, + expected: &HideCommentConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{ + Max: 5, + }, + Discussion: ptrBool(false), + }, + }, + { + name: "no discussion field defaults to nil", + inputMap: map[string]any{ + "hide-comment": map[string]any{ + "max": 3, + }, + }, + expected: &HideCommentConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{ + Max: 3, + }, + Discussion: nil, + }, + }, + { + name: "default max of 5 when not specified", + inputMap: map[string]any{ + "hide-comment": map[string]any{}, + }, + expected: &HideCommentConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{ + Max: 5, + }, + }, + }, + { + name: "nil config sets default max", + inputMap: map[string]any{ + "hide-comment": nil, + }, + expected: &HideCommentConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{ + Max: 5, + }, + }, + }, + { + name: "wildcard target-repo returns nil", + inputMap: map[string]any{ + "hide-comment": map[string]any{ + "target-repo": "*", + }, + }, + isNil: true, + }, + { + name: "allowed-repos is parsed", + inputMap: map[string]any{ + "hide-comment": map[string]any{ + "allowed-repos": []any{"owner/repo1", "owner/repo2"}, + }, + }, + expected: &HideCommentConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{ + Max: 5, + }, + SafeOutputTargetConfig: SafeOutputTargetConfig{ + AllowedRepos: []string{"owner/repo1", "owner/repo2"}, + }, + }, + }, + { + name: "missing hide-comment key returns nil", + inputMap: map[string]any{}, + isNil: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := &Compiler{} + result := compiler.parseHideCommentConfig(tt.inputMap) + + if tt.isNil { + assert.Nil(t, result, "Expected nil config") + return + } + + require.NotNil(t, result, "Expected non-nil config") + + assert.Equal(t, tt.expected.Max, result.Max, "Max should match") + assert.Equal(t, tt.expected.TargetRepoSlug, result.TargetRepoSlug, "TargetRepoSlug should match") + + if tt.expected.Discussion != nil { + require.NotNil(t, result.Discussion, "Discussion should not be nil") + assert.Equal(t, *tt.expected.Discussion, *result.Discussion, "Discussion value should match") + } else { + assert.Nil(t, result.Discussion, "Discussion should be nil") + } + + assert.Equal(t, tt.expected.AllowedReasons, result.AllowedReasons, "AllowedReasons should match") + assert.Equal(t, tt.expected.AllowedRepos, result.AllowedRepos, "AllowedRepos should match") + }) + } +} + +// TestParseAddCommentsConfig tests that all fields are properly parsed +func TestParseAddCommentsConfig(t *testing.T) { + tests := []struct { + name string + inputMap map[string]any + expected *AddCommentsConfig + isNil bool + }{ + { + name: "all fields parsed correctly", + inputMap: map[string]any{ + "add-comment": map[string]any{ + "max": 3, + "target": "*", + "target-repo": "owner/repo", + "allowed-repos": []any{"owner/repo1", "owner/repo2"}, + "discussion": true, + "hide-older-comments": true, + "allowed-reasons": []any{"spam", "resolved"}, + }, + }, + expected: &AddCommentsConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{ + Max: 3, + }, + Target: "*", + TargetRepoSlug: "owner/repo", + AllowedRepos: []string{"owner/repo1", "owner/repo2"}, + Discussion: ptrBool(true), + HideOlderComments: true, + AllowedReasons: []string{"spam", "resolved"}, + }, + }, + { + name: "discussion false is preserved", + inputMap: map[string]any{ + "add-comment": map[string]any{ + "max": 1, + "discussion": false, + }, + }, + expected: &AddCommentsConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{ + Max: 1, + }, + Discussion: ptrBool(false), + }, + }, + { + name: "no discussion field defaults to nil", + inputMap: map[string]any{ + "add-comment": map[string]any{ + "max": 2, + }, + }, + expected: &AddCommentsConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{ + Max: 2, + }, + Discussion: nil, + }, + }, + { + name: "default max of 1 when not specified", + inputMap: map[string]any{ + "add-comment": map[string]any{}, + }, + expected: &AddCommentsConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{ + Max: 1, + }, + }, + }, + { + name: "nil config sets default max", + inputMap: map[string]any{ + "add-comment": nil, + }, + expected: &AddCommentsConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{ + Max: 1, + }, + }, + }, + { + name: "wildcard target-repo returns nil", + inputMap: map[string]any{ + "add-comment": map[string]any{ + "target-repo": "*", + }, + }, + isNil: true, + }, + { + name: "missing add-comment key returns nil", + inputMap: map[string]any{}, + isNil: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := &Compiler{} + result := compiler.parseCommentsConfig(tt.inputMap) + + if tt.isNil { + assert.Nil(t, result, "Expected nil config") + return + } + + require.NotNil(t, result, "Expected non-nil config") + + assert.Equal(t, tt.expected.Max, result.Max, "Max should match") + assert.Equal(t, tt.expected.Target, result.Target, "Target should match") + assert.Equal(t, tt.expected.TargetRepoSlug, result.TargetRepoSlug, "TargetRepoSlug should match") + assert.Equal(t, tt.expected.AllowedRepos, result.AllowedRepos, "AllowedRepos should match") + assert.Equal(t, tt.expected.HideOlderComments, result.HideOlderComments, "HideOlderComments should match") + assert.Equal(t, tt.expected.AllowedReasons, result.AllowedReasons, "AllowedReasons should match") + + if tt.expected.Discussion != nil { + require.NotNil(t, result.Discussion, "Discussion should not be nil") + assert.Equal(t, *tt.expected.Discussion, *result.Discussion, "Discussion value should match") + } else { + assert.Nil(t, result.Discussion, "Discussion should be nil") + } + }) + } +} diff --git a/pkg/workflow/compiler_safe_outputs_job_test.go b/pkg/workflow/compiler_safe_outputs_job_test.go index a51c1f3204..0441f3da15 100644 --- a/pkg/workflow/compiler_safe_outputs_job_test.go +++ b/pkg/workflow/compiler_safe_outputs_job_test.go @@ -52,7 +52,7 @@ func TestBuildConsolidatedSafeOutputsJob(t *testing.T) { }, expectedJobName: "safe_outputs", checkPermissions: true, - expectedPerms: []string{"contents: read", "issues: write", "pull-requests: write", "discussions: write"}, + expectedPerms: []string{"contents: read", "issues: write", "pull-requests: write"}, }, { name: "create pull requests with patch", @@ -83,7 +83,7 @@ func TestBuildConsolidatedSafeOutputsJob(t *testing.T) { }, expectedJobName: "safe_outputs", checkPermissions: true, - expectedPerms: []string{"contents: read", "issues: write", "pull-requests: write", "discussions: write"}, + expectedPerms: []string{"contents: read", "issues: write", "pull-requests: write"}, }, { name: "with threat detection enabled", diff --git a/pkg/workflow/hide_comment.go b/pkg/workflow/hide_comment.go index 694d0e1581..7ff419740c 100644 --- a/pkg/workflow/hide_comment.go +++ b/pkg/workflow/hide_comment.go @@ -10,48 +10,36 @@ var hideCommentLog = logger.New("workflow:hide_comment") type HideCommentConfig struct { BaseSafeOutputConfig `yaml:",inline"` SafeOutputTargetConfig `yaml:",inline"` + Discussion *bool `yaml:"discussion,omitempty"` // Enable discussion comment support. Set to true to enable discussions permission. AllowedReasons []string `yaml:"allowed-reasons,omitempty"` // List of allowed reasons for hiding comments (default: all reasons allowed) } // parseHideCommentConfig handles hide-comment configuration func (c *Compiler) parseHideCommentConfig(outputMap map[string]any) *HideCommentConfig { + // Check if the key exists + if _, exists := outputMap["hide-comment"]; !exists { + return nil + } + hideCommentLog.Print("Parsing hide-comment configuration") - if configData, exists := outputMap["hide-comment"]; exists { - hideCommentConfig := &HideCommentConfig{} - - if configMap, ok := configData.(map[string]any); ok { - hideCommentLog.Print("Found hide-comment config map") - - // Parse target config (target-repo) with validation - targetConfig, isInvalid := ParseTargetConfig(configMap) - if isInvalid { - return nil // Invalid configuration (e.g., wildcard target-repo), return nil to cause validation error - } - hideCommentConfig.SafeOutputTargetConfig = targetConfig - - // Parse allowed-reasons - if allowedReasons, exists := configMap["allowed-reasons"]; exists { - if reasonsArray, ok := allowedReasons.([]any); ok { - for _, reason := range reasonsArray { - if reasonStr, ok := reason.(string); ok { - hideCommentConfig.AllowedReasons = append(hideCommentConfig.AllowedReasons, reasonStr) - } - } - } - } - - // Parse common base fields with default max of 5 - c.parseBaseSafeOutputConfig(configMap, &hideCommentConfig.BaseSafeOutputConfig, 5) - - hideCommentLog.Printf("Parsed hide-comment config: max=%d, target_repo=%s", - hideCommentConfig.Max, hideCommentConfig.TargetRepoSlug) - } else { - // If configData is nil or not a map, still set the default max - hideCommentConfig.Max = 5 - } - - return hideCommentConfig + + // Unmarshal into typed config struct + var config HideCommentConfig + if err := unmarshalConfig(outputMap, "hide-comment", &config, hideCommentLog); err != nil { + hideCommentLog.Printf("Failed to unmarshal config: %v", err) + // For backward compatibility, handle nil/empty config + config = HideCommentConfig{} + } + + // Set default max if not specified + if config.Max == 0 { + config.Max = 5 + } + + // Validate target-repo (wildcard "*" is not allowed) + if validateTargetRepoSlug(config.TargetRepoSlug, hideCommentLog) { + return nil // Invalid configuration, return nil to cause validation error } - return nil + return &config } diff --git a/pkg/workflow/notify_comment_test.go b/pkg/workflow/notify_comment_test.go index 892379204a..01d228490b 100644 --- a/pkg/workflow/notify_comment_test.go +++ b/pkg/workflow/notify_comment_test.go @@ -219,16 +219,13 @@ func TestConclusionJob(t *testing.T) { // When add-comment is configured, it requires issues, pull-requests, and discussions permissions // When only missing_tool/noop is configured, minimal permissions are needed if tt.addCommentConfig { - // add-comment requires full write permissions + // add-comment requires write permissions for issues and PRs (discussions only if explicitly enabled) if !strings.Contains(job.Permissions, "issues: write") { t.Error("Expected 'issues: write' permission when add-comment is configured") } if !strings.Contains(job.Permissions, "pull-requests: write") { t.Error("Expected 'pull-requests: write' permission when add-comment is configured") } - if !strings.Contains(job.Permissions, "discussions: write") { - t.Error("Expected 'discussions: write' permission when add-comment is configured") - } } // No need to check for specific permissions when only noop/missing_tool is configured // as they don't require write permissions on their own diff --git a/pkg/workflow/safe_outputs_permissions.go b/pkg/workflow/safe_outputs_permissions.go index a8f1faf61b..ffcafc6a9d 100644 --- a/pkg/workflow/safe_outputs_permissions.go +++ b/pkg/workflow/safe_outputs_permissions.go @@ -29,8 +29,15 @@ func computePermissionsForSafeOutputs(safeOutputs *SafeOutputsConfig) *Permissio permissions.Merge(NewPermissionsContentsReadIssuesWriteDiscussionsWrite()) } if safeOutputs.AddComments != nil { - safeOutputsPermissionsLog.Print("Adding permissions for add-comment") - permissions.Merge(NewPermissionsContentsReadIssuesWritePRWriteDiscussionsWrite()) + // Check if add-comment is configured to target discussions + // Default is false (no discussion permission) unless explicitly set to true + if safeOutputs.AddComments.Discussion != nil && *safeOutputs.AddComments.Discussion { + safeOutputsPermissionsLog.Print("Adding permissions for add-comment (with discussions support)") + permissions.Merge(NewPermissionsContentsReadIssuesWritePRWriteDiscussionsWrite()) + } else { + safeOutputsPermissionsLog.Print("Adding permissions for add-comment") + permissions.Merge(NewPermissionsContentsReadIssuesWritePRWrite()) + } } if safeOutputs.CloseIssues != nil { safeOutputsPermissionsLog.Print("Adding permissions for close-issue") @@ -96,8 +103,15 @@ func computePermissionsForSafeOutputs(safeOutputs *SafeOutputsConfig) *Permissio permissions.Merge(NewPermissionsContentsReadPRWrite()) } if safeOutputs.HideComment != nil { - safeOutputsPermissionsLog.Print("Adding permissions for hide-comment") - permissions.Merge(NewPermissionsContentsReadIssuesWritePRWriteDiscussionsWrite()) + // Check if hide-comment is configured to support discussions + // Default is false (no discussion permission) unless explicitly set to true + if safeOutputs.HideComment.Discussion != nil && *safeOutputs.HideComment.Discussion { + safeOutputsPermissionsLog.Print("Adding permissions for hide-comment (with discussions support)") + permissions.Merge(NewPermissionsContentsReadIssuesWritePRWriteDiscussionsWrite()) + } else { + safeOutputsPermissionsLog.Print("Adding permissions for hide-comment") + permissions.Merge(NewPermissionsContentsReadIssuesWritePRWrite()) + } } if safeOutputs.DispatchWorkflow != nil { safeOutputsPermissionsLog.Print("Adding permissions for dispatch-workflow") diff --git a/pkg/workflow/safe_outputs_permissions_test.go b/pkg/workflow/safe_outputs_permissions_test.go index c570e6deb4..b2d9fb336d 100644 --- a/pkg/workflow/safe_outputs_permissions_test.go +++ b/pkg/workflow/safe_outputs_permissions_test.go @@ -72,12 +72,26 @@ func TestComputePermissionsForSafeOutputs(t *testing.T) { }, }, { - name: "add-comment includes all write permissions including discussions", + name: "add-comment only - no discussions permission (default)", safeOutputs: &SafeOutputsConfig{ AddComments: &AddCommentsConfig{ BaseSafeOutputConfig: BaseSafeOutputConfig{Max: 1}, }, }, + expected: map[PermissionScope]PermissionLevel{ + PermissionContents: PermissionRead, + PermissionIssues: PermissionWrite, + PermissionPullRequests: PermissionWrite, + }, + }, + { + name: "add-comment with discussion: true requires discussions permission", + safeOutputs: &SafeOutputsConfig{ + AddComments: &AddCommentsConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: 1}, + Discussion: ptrBool(true), + }, + }, expected: map[PermissionScope]PermissionLevel{ PermissionContents: PermissionRead, PermissionIssues: PermissionWrite, @@ -86,10 +100,38 @@ func TestComputePermissionsForSafeOutputs(t *testing.T) { }, }, { - name: "hide-comment includes all write permissions including discussions", + name: "add-comment with discussion: false - no discussions permission", + safeOutputs: &SafeOutputsConfig{ + AddComments: &AddCommentsConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: 1}, + Discussion: ptrBool(false), + }, + }, + expected: map[PermissionScope]PermissionLevel{ + PermissionContents: PermissionRead, + PermissionIssues: PermissionWrite, + PermissionPullRequests: PermissionWrite, + }, + }, + { + name: "hide-comment only - no discussions permission (default)", + safeOutputs: &SafeOutputsConfig{ + HideComment: &HideCommentConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: 1}, + }, + }, + expected: map[PermissionScope]PermissionLevel{ + PermissionContents: PermissionRead, + PermissionIssues: PermissionWrite, + PermissionPullRequests: PermissionWrite, + }, + }, + { + name: "hide-comment with discussion: true - includes discussions permission", safeOutputs: &SafeOutputsConfig{ HideComment: &HideCommentConfig{ BaseSafeOutputConfig: BaseSafeOutputConfig{Max: 1}, + Discussion: ptrBool(true), }, }, expected: map[PermissionScope]PermissionLevel{ @@ -99,6 +141,20 @@ func TestComputePermissionsForSafeOutputs(t *testing.T) { PermissionDiscussions: PermissionWrite, }, }, + { + name: "hide-comment with discussion: false - no discussions permission", + safeOutputs: &SafeOutputsConfig{ + HideComment: &HideCommentConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: 1}, + Discussion: ptrBool(false), + }, + }, + expected: map[PermissionScope]PermissionLevel{ + PermissionContents: PermissionRead, + PermissionIssues: PermissionWrite, + PermissionPullRequests: PermissionWrite, + }, + }, { name: "add-labels only - no discussions permission", safeOutputs: &SafeOutputsConfig{ @@ -237,6 +293,42 @@ func TestComputePermissionsForSafeOutputs(t *testing.T) { PermissionContents: PermissionWrite, }, }, + { + name: "add-comment with create-discussion includes discussions permission", + safeOutputs: &SafeOutputsConfig{ + AddComments: &AddCommentsConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: 1}, + }, + CreateDiscussions: &CreateDiscussionsConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: 1}, + }, + }, + expected: map[PermissionScope]PermissionLevel{ + PermissionContents: PermissionRead, + PermissionIssues: PermissionWrite, + PermissionPullRequests: PermissionWrite, + PermissionDiscussions: PermissionWrite, + }, + }, + { + name: "hide-comment with update-discussion includes discussions permission", + safeOutputs: &SafeOutputsConfig{ + HideComment: &HideCommentConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: 1}, + }, + UpdateDiscussions: &UpdateDiscussionsConfig{ + UpdateEntityConfig: UpdateEntityConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: 1}, + }, + }, + }, + expected: map[PermissionScope]PermissionLevel{ + PermissionContents: PermissionRead, + PermissionIssues: PermissionWrite, + PermissionPullRequests: PermissionWrite, + PermissionDiscussions: PermissionWrite, + }, + }, { name: "create-code-scanning-alert requires security-events write", safeOutputs: &SafeOutputsConfig{