Fix add-comment and hide-comment to conditionally request discussions:write permission#16501
Fix add-comment and hide-comment to conditionally request discussions:write permission#16501
Conversation
…mission unconditionally Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot the add-comment safe output needs to have the |
…ion: true Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in ef1b6a5. The Similarly, 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:
|
|
@copilot update the default value of |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated in a2d255f. The Behavior:
29 workflows were recompiled and now include 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:
|
|
@copilot also handle hide_comment the same way |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated in d06ddbe. The Behavior:
The parser was also simplified to use 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:
|
|
@copilot change to the original default discussion |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Reverted in dcc228c. The Behavior:
30 workflows were recompiled to remove 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:
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot review changes and make sure the parsing of add-comments, hide-comments did not lose fields. Ensure proper testing. |
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where add-comment and hide-comment safe outputs unconditionally requested discussions:write permission, causing failures when GitHub App installations don't have the Discussions permission granted. The fix makes discussion support opt-in by requiring users to explicitly set discussion: true.
Changes:
- Modified permission logic to only request
discussions:writewhendiscussion: trueis explicitly set - Updated field documentation to reflect the new opt-in behavior
- Removed validation that required
discussion: trueif present (allowingdiscussion: false) - Added comprehensive test coverage for all three scenarios (unset, true, false)
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/safe_outputs_permissions.go | Updated to conditionally include discussions permission based on Discussion field |
| pkg/workflow/add_comment.go | Updated standalone handler permissions and field documentation; removed validation |
| pkg/workflow/hide_comment.go | Added Discussion field and migrated parser to use unmarshalConfig |
| pkg/parser/schemas/main_workflow_schema.json | Updated schemas to add/modify discussion field with default: false |
| pkg/workflow/safe_outputs_permissions_test.go | Added comprehensive tests for all permission scenarios |
| pkg/workflow/compiler_safe_outputs_job_test.go | Updated tests to reflect new permission expectations |
| pkg/workflow/notify_comment_test.go | Removed test assertion for discussions permission |
| .github/workflows/*.lock.yml | Removed discussions permission from compiled workflows |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Reviewed and verified in a2e59c4. Added comprehensive parsing tests to ensure no fields were lost during the migration to Fields verified for Fields verified for Test coverage added:
All workflow package tests pass (20s runtime). 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:
|
Both
add-commentandhide-commentsafe outputs no longer unconditionally requestdiscussions: writepermission. Users must now explicitly setdiscussion: trueto enable discussion support and request the discussions permission.Changes
pkg/workflow/safe_outputs_permissions.go:add-comment: Only includediscussions: writepermission whendiscussion: trueis explicitly sethide-comment: Only includediscussions: writepermission whendiscussion: trueis explicitly setpkg/workflow/add_comment.go:discussionfielddiscussion: trueif presentpkg/workflow/hide_comment.go:Discussion *boolfield toHideCommentConfigstructunmarshalConfigfor automatic field parsing (consistent withadd-comment)pkg/parser/schemas/main_workflow_schema.json:const: trueconstraint foradd-commentdiscussion fielddiscussionfield tohide-commentschema withdefault: falseadd-commentandhide-comment, plus parsing tests to verify all fields are correctly handledPermission Impact
After (add-comment or hide-comment without discussion field - default behavior):
After (add-comment or hide-comment with discussion: true - opt-in):
After (add-comment or hide-comment with discussion: false - explicit opt-out):
The
discussions: writepermission is only requested when:discussion: trueis explicitly set in theadd-commentorhide-commentconfigurationTesting
add-commentandhide-commentunmarshalConfigpreserves all existing functionalityMigration Note
Workflows using
add-commentorhide-commentwill not includediscussions: writepermission by default. To enable discussion comment support, adddiscussion: trueto theadd-commentorhide-commentconfiguration.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)
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.