Remove unnecessary contents: read from safe-output handlers - #48913
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
contents: read from safe-output handlers
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Removes unnecessary contents: read access from output-only safe-output handlers.
Changes:
- Adds minimal permission factories and rewires handlers.
- Keeps content permissions for repository-mutating operations.
- Updates permission tests and specification v1.27.0.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_permissions_test.go |
Updates expected minimal permissions. |
pkg/workflow/safe_outputs_app_test.go |
Verifies App tokens omit contents access. |
pkg/workflow/safe_output_handlers.go |
Rewires handler permission factories. |
pkg/workflow/permissions_factory.go |
Adds contents-free permission factories. |
pkg/workflow/compiler_safe_outputs_job_test.go |
Updates compiled job expectations. |
pkg/workflow/compile_outputs_label_test.go |
Updates label workflow permissions. |
pkg/workflow/compile_outputs_comment_test.go |
Updates comment workflow permissions. |
pkg/workflow/add_comment.go |
Removes baseline contents permission. |
docs/src/content/docs/specs/safe-outputs-specification.md |
Publishes revised permission specification. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 4
- Review effort level: Medium
| return NewPermissionsChecksWritePRRead() | ||
| } | ||
| return NewPermissionsContentsReadChecksWrite() | ||
| return NewPermissionsChecksWrite() |
| **Version 1.27.0** (2026-07-29): | ||
|
|
||
| - **Removed**: `contents: read` from the required permissions of all output-only safe-output handlers. The permission was an unconditional baseline with no functional purpose in the `safe_outputs` job for handlers that only call issue, pull-request, discussion, checks, or security-events APIs. Operators who use tightly scoped GitHub Apps no longer need to justify a repository-contents read grant for pure output workloads. | ||
| - **Updated**: Permission tables for `create_issue`, `add_comment`, `close_issue`, `update_issue`, `assign_milestone`, `assign_to_user`, `unassign_from_user`, `assign_to_agent`, `link_sub_issue`, `set_issue_type`, `set_issue_field`, `comment_memory`, `create_discussion`, `update_discussion`, `close_discussion`, `hide_comment`, `add_labels`, `remove_labels`, `replace_label`, `add_reviewer`, `close_pull_request`, `mark_pull_request_as_ready_for_review`, `dismiss_pull_request_review`, `create_pull_request_review_comment`, `submit_pull_request_review`, `reply_to_pull_request_review_comment`, `resolve_pull_request_review_thread`, `update_pull_request` (without `update-branch`), `create_code_scanning_alert`, `autofix_code_scanning_alert`, `create_check_run`, `update_project`, `create_project`, and `create_project_status_update`. |
| **Notes**: | ||
|
|
||
| - Most complex permission set - requires security-events, contents, pull-requests, and actions scopes | ||
| - Most complex permission set — GitHub Actions Token requires security-events and actions scopes; GitHub App additionally requires contents and pull-requests scopes for branch/PR creation |
| // NewPermissionsIssuesWrite creates permissions with issues: write only. | ||
| // Used for output-only handlers (create-issue, close-issue, etc.) that call the | ||
| // issues API without accessing repository file contents. | ||
| func NewPermissionsIssuesWrite() *Permissions { |
|
✅ Test Quality Sentinel completed test quality analysis. Caution agentic threat detected DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. No new or modified behavioral test functions in this PR. The PR only contains minor updates to existing test assertions (1-8 line changes per test file) to align with production code refactoring of safe-output permissions handling. No new behavioral test coverage was added. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The permission reduction is correct and well-scoped. All changed handlers call only write APIs — no repository content reads. Key observations:
- Each new NewPermissions* factory is narrowly named and documented.
- upload_asset PermissionBuilder removal is correct — that handler belongs to publish_assets, not safe_outputs.
- autofix-code-scanning-alert spec note correctly distinguishes GitHub Actions token vs GitHub App permissions.
- Tests and spec updated consistently.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 32.8 AIC · ⌖ 4.72 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /improve-codebase-architecture — clean, well-scoped security improvement. Approving.
📋 Key Themes & Highlights
Key Themes
- Principle of least privilege: Removing
contents: readfrom ~25 output-only handlers is exactly the right call. Thesafe_outputsjob has never needed repository file access for issue/PR/discussion writes. - Factory proliferation: 12 new factory functions added. This is a valid trade-off (explicit, searchable, documented) but worth watching as the set grows.
- Spec alignment:
safe-outputs-specification.mdupdated to v1.27.0 with a thorough changelog — spec and implementation stay in sync.
Positive Highlights
- ✅ Every handler change is paired with a corresponding test assertion (
NotContains "permission-contents: read") - ✅
upload_assetPermissionBuilder removal is safe:PermissionBuilder == nilis already guarded insafe_outputs_permissions.go - ✅
update-pull-requestcorrectly preservescontents: writewhenupdate-branch: true— the conditional path is untouched - ✅ Project handlers retain
issues: readvia the newNewPermissionsOrganizationProjWriteIssuesRead()factory - ✅ Changelog entry accurately lists every affected handler
Minor Observations (non-blocking)
- The factory naming scheme (
NewPermissionsIssuesWriteDiscussionsWritePRWrite) grows verbose at three-permission combinations. A builder pattern (NewPermissions().With(Issues, Write).With(Discussions, Write)) could scale better for future additions — out of scope here. - No test explicitly covers the
upload_assethandler contributing zero permissions afterPermissionBuilderremoval. Indirect coverage exists via the nil-guard insafe_outputs_permissions.go, but a targeted test case would add confidence.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 46.6 AIC · ⌖ 4.92 AIC · ⊞ 7K
Comment /matt to run again
There was a problem hiding this comment.
Review: no blocking issues found
This is a clean, mechanical permission-scoping change. Verified: all ~25 handler rewires correctly map to the new minimal-permission factories; upload-asset's PermissionBuilder removal is safe (git write lives in the separate publish_assets job, NewPermissionsContentsRead/NewPermissionsContentsReadProjectsWrite remain used elsewhere so no dead code); update-pull-request's conditional contents:write (only with update-branch: true) is correctly preserved and tested; new factory functions in permissions_factory.go are consistent with existing naming/behavior and cover all needed combinations without duplication; spec and test updates match the code changes.
💡 What I checked
- Cross-referenced every handler diff in
safe_output_handlers.goagainst its new factory function inpermissions_factory.go. - Confirmed no handler that lost
contents: readactually reads repository file contents (all are pure API-output handlers: issues, PRs, discussions, checks, security-events, projects). - Verified
NewPermissionsContentsReadandNewPermissionsContentsReadProjectsWriteare still referenced elsewhere in the codebase (compiler_main_job_helpers.go, compiler_pre_activation_job.go, compiler_unlock_job.go, evals_job.go, threat_detection_job.go, tools.go), so removing theupload-assetPermissionBuilderdoes not orphan these functions or cause build issues. - Test assertions in
safe_outputs_permissions_test.goandsafe_outputs_app_test.go(NotContains "permission-contents: read") correctly validate the negative case. - Ran a background grumpy-coder sub-agent pass over the diff; it independently reported no issues, corroborating this assessment.
🔎 Code quality review by PR Code Quality Reviewer · aut00 · 68 AIC · ⌖ 7.13 AIC · ⊞ 7.8K
Comment /review to run again
…y safe-output handlers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (165 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot run pr-finisher skill |
- compiler_safe_outputs_steps.go: use target-aware minimal permissions for create-check-run GitHub App token (NewPermissionsChecksWrite or NewPermissionsChecksWritePRRead when target is set) - safe_outputs_permissions.go: add stepsRequireContentsRead to detect actions/checkout in user-provided safe-output steps and conditionally add contents: read to avoid private-repo checkout failures - spec: remove incorrect contents: write and pull-requests: write from autofix_code_scanning_alert GitHub App permissions (handler calls code-scanning API only, no branch/PR creation) - spec: add issues: read to create_project and update_project permission tables to match NewPermissionsOrganizationProjWriteIssuesRead factory - Tests: add TestComputePermissionsForSafeOutputs_Checkout and TestSafeOutputsCreateCheckRunAppTokenMinimalPermissions Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot update safe outputs specification, add tests for minimal permissions |
|
@copilot recompile |
…ct-status-update, merge-pull-request, update-release, and create-agent-session handlers; fix spec note for create_project_status_update Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit Tests added to
Spec fix: The |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — recompiled all 266 workflow lock files in commit |
…owlist to match bash: [\"*\"] wildcard Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. |
|
🎉 This pull request is included in a new release. Release: |
Nearly every
safe_outputsjob handler requestedcontents: readas an unconditional baseline, documented as "always included for repository context access." The safe_outputs job never reads repository file contents — the permission had no functional basis and forced operators with tightly-scoped GitHub Apps to justify a repository-contents read grant for pure output workloads.Changes
Permission factory functions (
permissions_factory.go)Added 12 new factory functions omitting
contents: read:NewPermissionsIssuesWrite(),NewPermissionsPRWrite(),NewPermissionsDiscussionsWrite()and variants for issue+PR, issue+discussion, checks, security-events, and org-projects combinations.Handler rewiring (
safe_output_handlers.go,add_comment.go)upload-assetPermissionBuilderremoved —safe_outputsnever processes this handler type; actual git operations live in the separatepublish_assetsjob (contents: write).update-pull-requestwithoutupdate-branchnow usespull-requests: writeonly; withupdate-branchkeepscontents: write.Handlers retaining
contentsaccesscreate-pull-request,merge-pull-request,push-to-pull-request-branch,update-pull-request(update-branch: true),update-release— all legitimately needcontents: write.Spec (
safe-outputs-specification.md→ v1.27.0)contents: readfrom all output-only handler permission tables.create_check_runchangelog entry to reflect the corrected permission set.Tests
Updated
safe_outputs_permissions_test.go,safe_outputs_app_test.go,compile_outputs_comment_test.go,compile_outputs_label_test.go, andcompiler_safe_outputs_job_test.goto assert the new minimal permission sets. App token tests now assertNotContains "permission-contents: read"for output-only handler combinations.contents: readfor repository file access? #48882