Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .changeset/patch-fix-add-comment-perms.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions .github/workflows/agent-performance-analyzer.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions .github/workflows/archie.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions .github/workflows/brave.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions .github/workflows/ci-doctor.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions .github/workflows/daily-assign-issue-to-user.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions .github/workflows/daily-cli-performance.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions .github/workflows/daily-fact.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions .github/workflows/dev-hawk.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions .github/workflows/discussion-task-miner.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions .github/workflows/issue-monster.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions .github/workflows/pdf-summary.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions .github/workflows/scout.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions .github/workflows/smoke-temporary-id.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions .github/workflows/smoke-test-tools.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions .github/workflows/sub-issue-closer.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions .github/workflows/workflow-health-manager.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions pkg/workflow/add_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,11 +116,12 @@ func (c *Compiler) buildCreateOutputAddCommentJob(data *WorkflowData, mainJobNam

// Determine permissions based on discussions field
// Default (nil or true) includes discussions:write for GitHub Apps with Discussions permission
// Note: PR comments are issue comments, so only issues:write is needed, not pull_requests:write
var permissions *Permissions
if data.SafeOutputs.AddComments.Discussions != nil && !*data.SafeOutputs.AddComments.Discussions {
permissions = NewPermissionsContentsReadIssuesWritePRWrite()
permissions = NewPermissionsContentsReadIssuesWrite()
} else {
permissions = NewPermissionsContentsReadIssuesWritePRWriteDiscussionsWrite()
permissions = NewPermissionsContentsReadIssuesWriteDiscussionsWrite()
}

// Use the shared builder function to create the job
Expand Down
4 changes: 2 additions & 2 deletions pkg/workflow/compiler_safe_outputs_job_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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", "discussions: write"},
},
{
name: "create pull requests with patch",
Expand Down Expand Up @@ -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", "discussions: write"},
},
{
name: "with threat detection enabled",
Expand Down
8 changes: 3 additions & 5 deletions pkg/workflow/notify_comment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -216,16 +216,14 @@ func TestConclusionJob(t *testing.T) {
}

// Check permissions based on what safe-outputs are configured
// When add-comment is configured, it requires issues, pull-requests, and discussions permissions
// When add-comment is configured, it requires issues and discussions permissions
// (PR comments are issue comments, so only issues: write is needed, not pull-requests: write)
// When only missing_tool/noop is configured, minimal permissions are needed
if tt.addCommentConfig {
// add-comment requires full write permissions
// add-comment requires issues and discussions write permissions
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")
}
Expand Down
10 changes: 6 additions & 4 deletions pkg/workflow/safe_outputs_permissions.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,11 @@ func computePermissionsForSafeOutputs(safeOutputs *SafeOutputsConfig) *Permissio
safeOutputsPermissionsLog.Print("Adding permissions for add-comment")
// Check if discussions permission should be excluded (discussions: false)
// Default (nil or true) includes discussions:write for GitHub Apps with Discussions permission
// Note: PR comments are issue comments, so only issues:write is needed, not pull_requests:write
if safeOutputs.AddComments.Discussions != nil && !*safeOutputs.AddComments.Discussions {
permissions.Merge(NewPermissionsContentsReadIssuesWritePRWrite())
permissions.Merge(NewPermissionsContentsReadIssuesWrite())
} else {
permissions.Merge(NewPermissionsContentsReadIssuesWritePRWriteDiscussionsWrite())
permissions.Merge(NewPermissionsContentsReadIssuesWriteDiscussionsWrite())
}
}
if safeOutputs.CloseIssues != nil {
Expand Down Expand Up @@ -105,10 +106,11 @@ func computePermissionsForSafeOutputs(safeOutputs *SafeOutputsConfig) *Permissio
safeOutputsPermissionsLog.Print("Adding permissions for hide-comment")
// Check if discussions permission should be excluded (discussions: false)
// Default (nil or true) includes discussions:write for GitHub Apps with Discussions permission
// Note: Hiding comments (issue/PR/discussion) only needs issues:write, not pull_requests:write
if safeOutputs.HideComment.Discussions != nil && !*safeOutputs.HideComment.Discussions {
permissions.Merge(NewPermissionsContentsReadIssuesWritePRWrite())
permissions.Merge(NewPermissionsContentsReadIssuesWrite())
} else {
permissions.Merge(NewPermissionsContentsReadIssuesWritePRWriteDiscussionsWrite())
permissions.Merge(NewPermissionsContentsReadIssuesWriteDiscussionsWrite())
}
}
if safeOutputs.DispatchWorkflow != nil {
Expand Down
24 changes: 10 additions & 14 deletions pkg/workflow/safe_outputs_permissions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,9 @@ func TestComputePermissionsForSafeOutputs(t *testing.T) {
},
},
expected: map[PermissionScope]PermissionLevel{
PermissionContents: PermissionRead,
PermissionIssues: PermissionWrite,
PermissionPullRequests: PermissionWrite,
PermissionDiscussions: PermissionWrite,
PermissionContents: PermissionRead,
PermissionIssues: PermissionWrite,
PermissionDiscussions: PermissionWrite,
},
},
{
Expand All @@ -94,9 +93,8 @@ func TestComputePermissionsForSafeOutputs(t *testing.T) {
},
},
expected: map[PermissionScope]PermissionLevel{
PermissionContents: PermissionRead,
PermissionIssues: PermissionWrite,
PermissionPullRequests: PermissionWrite,
PermissionContents: PermissionRead,
PermissionIssues: PermissionWrite,
},
},
{
Expand All @@ -107,10 +105,9 @@ func TestComputePermissionsForSafeOutputs(t *testing.T) {
},
},
expected: map[PermissionScope]PermissionLevel{
PermissionContents: PermissionRead,
PermissionIssues: PermissionWrite,
PermissionPullRequests: PermissionWrite,
PermissionDiscussions: PermissionWrite,
PermissionContents: PermissionRead,
PermissionIssues: PermissionWrite,
PermissionDiscussions: PermissionWrite,
},
},
{
Expand All @@ -122,9 +119,8 @@ func TestComputePermissionsForSafeOutputs(t *testing.T) {
},
},
expected: map[PermissionScope]PermissionLevel{
PermissionContents: PermissionRead,
PermissionIssues: PermissionWrite,
PermissionPullRequests: PermissionWrite,
PermissionContents: PermissionRead,
PermissionIssues: PermissionWrite,
},
},
{
Expand Down
Loading