Conversation
…ns:write Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot add a field discussions: false that allows to disable discussions: write permissions. discussions: write as default. For add-comments and hide-comments. |
…-comment and hide-comment Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added Default behavior (field omitted or Example: safe-outputs:
add-comment:
target: "*"
max: 1
discussions: false # Exclude discussions:writeImplementation in commit 7d34838:
Verified compilation: workflows with Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Pull request overview
Adds a configurable opt-out (discussions: false) for add-comment / hide-comment safe outputs so workflows can avoid requesting discussions:write and prevent 422s for GitHub Apps without Discussions permission.
Changes:
- Adds optional
Discussions *booltoAddCommentsConfigandHideCommentConfig, and updates permission computation to conditionally includediscussions:write. - Updates schema/docs (
main_workflow_schema.json,safe_outputs_tools.json) and extends permission tests. - Regenerates
.github/workflows/ci-doctor.lock.ymlpermissions output.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/safe_outputs_permissions.go | Conditionally merges discussions:write for add/hide comment based on new config flag. |
| pkg/workflow/safe_outputs_permissions_test.go | Adds test cases for default vs discussions:false. |
| pkg/workflow/add_comment.go | Updates standalone add_comment job permissions to respect the new flag. |
| pkg/workflow/hide_comment.go | Adds Discussions field to config struct (but see review comment re: parsing). |
| pkg/workflow/js/safe_outputs_tools.json | Documents discussions:false opt-out behavior in tool descriptions. |
| pkg/parser/schemas/main_workflow_schema.json | Adds discussions boolean to add-comment / hide-comment schema definitions. |
| .github/workflows/ci-doctor.lock.yml | Updates generated job permissions (notably removes discussions: write). |
Comments suppressed due to low confidence (1)
.github/workflows/ci-doctor.lock.yml:1279
- This workflow's source (
ci-doctor.md) enablessafe-outputs.add-commentwithout settingdiscussions: false, but this lock file no longer requestsdiscussions: writefor thesafe_outputsjob. If the default behavior is still to includediscussions:write, either regenerate the lock file or explicitly opt out viasafe-outputs.add-comment.discussions: falsein the source workflow.
runs-on: ubuntu-slim
permissions:
contents: read
issues: write
pull-requests: write
timeout-minutes: 15
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| AllowedReasons []string `yaml:"allowed-reasons,omitempty"` // List of allowed reasons for hiding comments (default: all reasons allowed) | ||
| Discussions *bool `yaml:"discussions,omitempty"` // When false, excludes discussions:write permission. Default (nil or true) includes discussions:write for GitHub Apps with Discussions permission. | ||
| } |
There was a problem hiding this comment.
HideCommentConfig now has a Discussions field, but parseHideCommentConfig never reads discussions from the hide-comment config map. As a result, safe-outputs.hide-comment.discussions: false will be silently ignored and the workflow will still request discussions:write. Parse and set this field (only when the key is present) so the opt-out works as intended.
| permissions: | ||
| contents: read | ||
| discussions: write | ||
| issues: write | ||
| pull-requests: write | ||
| outputs: |
There was a problem hiding this comment.
This workflow's source (ci-doctor.md) enables safe-outputs.add-comment without setting discussions: false, but this lock file no longer requests discussions: write for the conclusion job. If the default behavior is still to include discussions:write (as described/tests indicate), either the lock file needs regeneration or the workflow config should explicitly set safe-outputs.add-comment.discussions: false to justify omitting the permission.
This issue also appears on line 1274 of the same file.
| // Determine permissions based on discussions field | ||
| // Default (nil or true) includes discussions:write for GitHub Apps with Discussions permission | ||
| var permissions *Permissions | ||
| if data.SafeOutputs.AddComments.Discussions != nil && !*data.SafeOutputs.AddComments.Discussions { | ||
| permissions = NewPermissionsContentsReadIssuesWritePRWrite() | ||
| } else { | ||
| permissions = NewPermissionsContentsReadIssuesWritePRWriteDiscussionsWrite() | ||
| } |
There was a problem hiding this comment.
The standalone add_comment job now conditionally drops discussions:write based on safe-outputs.add-comment.discussions, but there isn't test coverage asserting the generated job permissions for the default vs discussions:false cases (existing tests for buildCreateOutputAddCommentJob only check needs/env). Add a unit test that verifies the job's rendered permissions include/exclude discussions: write accordingly.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
add-commentandhide-commentsafe outputs unconditionally requesteddiscussions:writepermission, causing 422 errors when GitHub Apps lack Discussions permission. This occurred even when no discussion-related safe outputs were configured.Solution
Added an optional
discussionsfield toadd-commentandhide-commentconfigurations that allows users to control whetherdiscussions:writepermission is requested.Default behavior (field omitted or
discussions: true): Includesdiscussions:writepermission for maximum compatibilityOpt-out (
discussions: false): Excludesdiscussions:writepermission for GitHub Apps without Discussions permissionChanges
1. Config Structs (
add_comment.go,hide_comment.go):Discussions *boolfield toAddCommentsConfigandHideCommentConfigtruewhen omitted (backward compatible)2. Permission Logic (
safe_outputs_permissions.go,add_comment.go):computePermissionsForSafeOutputs()to check theDiscussionsfielddiscussions: false: usesNewPermissionsContentsReadIssuesWritePRWrite()NewPermissionsContentsReadIssuesWritePRWriteDiscussionsWrite()3. Schema Updates (
main_workflow_schema.json):discussionsboolean field to bothadd-commentandhide-commentschemas4. Tool Descriptions (
safe_outputs_tools.json):add_commentandhide_commenttool descriptions to document thediscussionsfielddiscussions: false5. Test Coverage (
safe_outputs_permissions_test.go):discussions:write)discussions: false(excludesdiscussions:write)Usage Example
Behavior
Default (maintains backward compatibility):
contents: readissues: writepull-requests: writediscussions: write✅With
discussions: false(for apps without Discussions permission):contents: readissues: writepull-requests: writeThe
discussions:writepermission is still independently added when discussion-related safe outputs (create-discussion,close-discussion,update-discussion) are configured.Original prompt
This section details on the original issue you should resolve
<issue_title>add-comment and hide-comment safe outputs unconditionally request discussions:write permission</issue_title>
<issue_description>🤖 AI Assisted Bug Report
Problem
PR #15518 (fixing #15516) introduced
computePermissionsForSafeOutputsto compute minimal permissions for thesafe_outputsandconclusionjobs. However, theadd-commentandhide-commententries still useNewPermissionsContentsReadIssuesWritePRWriteDiscussionsWrite(), which unconditionally includesdiscussions: write— even when no discussion-related safe outputs (create-discussion,close-discussion,update-discussion) are configured.This causes the same 422 error that #15516 reported:
…when the GitHub App installation doesn't have the Discussions permission granted.
Root Cause
In
pkg/workflow/safe_outputs_permissions.go:The rationale is that
add_comment.cjscan comment on discussions, not just issues/PRs. But if no discussion-related safe-output is configured, the agent will never target a discussion, sodiscussions: writeis unnecessary.The same pattern also exists in
pkg/workflow/add_comment.goL124 (standalone handler permissions).Impact
Any workflow using
add-commentorhide-commentwith a GitHub App that doesn't have Discussions permission will fail at the token generation step in thesafe_outputsandconclusionjobs. The agent job completes successfully, but its output (comments, labels, assignments) is never applied.Affected compiled output (lock.yml):
permissions:block includesdiscussions: writecreate-github-app-tokenstep includespermission-discussions: writeWorkaround: Manually remove
discussions: writeandpermission-discussions: writefrom the compiled lock file after everygh aw compile.Suggested Fix
In
safe_outputs_permissions.go, useNewPermissionsContentsReadIssuesWritePRWrite()(already exists atpermissions_factory.goL83) instead:discussions: writeis already independently added when discussion-related safe outputs are configured:So workflows that actually use discussions will still get the permission via those entries.
Same fix should be applied to
add_comment.goL124 for the standalone handler path.Reproduction
add-commentbut no discussion safe-outputs:gh aw compilediscussions: writein the lock file'ssafe_outputsandconclusionjobsEnvironment
Related
discussions: writewhen workflow has no discussion-related safe-outputs #15516 — original report (closed by Fix: Compute minimal permissions for conclusion/safe_outputs jobs based on configured safe-outputs #15518, but the fix was incomplete)computePermissionsForSafeOutputsbut didn't fixadd-comment/hide-comment)</issue_description>
Comments on the Issue (you are @copilot in this section)
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.