Allow add-labels to opt out of pull-requests: write#49477
add-labels to opt out of pull-requests: write#49477Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
add-labels to opt out of pull-requests: write
There was a problem hiding this comment.
Pull request overview
Adds least-privilege permission toggles for safe-outputs.add-labels while preserving existing defaults.
Changes:
- Adds
issuesandpull-requestsconfiguration toggles. - Applies toggles to job and GitHub App token permissions.
- Adds schema support and regression tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/add_labels.go |
Defines toggles and permission computation. |
pkg/workflow/safe_output_handlers.go |
Uses computed label permissions. |
pkg/workflow/safe_outputs_handler_registry.go |
Emits toggles in runtime configuration. |
pkg/parser/schemas/main_workflow_schema.json |
Validates and describes the new options. |
pkg/workflow/safe_outputs_permissions_test.go |
Tests permission aggregation. |
pkg/workflow/safe_outputs_app_test.go |
Tests least-privilege App token generation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Balanced
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49477 does not have the 'implementation' label and has 99 new lines of code in business logic directories (≤100 threshold). |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 92/100 — Excellent
📊 Metrics (3 tests)
📋 Test Details
Well-designed with both positive and negative assertions covering the behavioral contract. Table-driven test cases in
Complement each other with good boundary coverage. Assessment NotesTest Inflation: safe_outputs_app_test.go shows 3:1 ratio (+48 test lines vs +16 prod lines). This is justified because the integration test requires full workflow markdown setup, YAML parsing, temporary I/O, and compiler pipeline invocation — inherent to end-to-end testing, not a sign of poor design. Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 92/100. 0% implementation tests (threshold: 30%). No violations. All tests are design-contract oriented with comprehensive edge-case coverage. The suite correctly validates both unit-level permission logic and end-to-end workflow compilation.
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
The implementation is clean and correct.
buildAddLabelsPermissionscorrectly defaults to both permissions when config fields arenil, preserving backward compatibility.- Merging an empty
*Permissions(both opts set tofalse) into the aggregate is safe —Merge(other)with an empty map is a no-op that leaves prior permissions untouched. - The hyphen in
AddBoolPtr("pull-requests", ...)is consistent with the existing"target-repo"key pattern in the same file. - Schema, struct fields, handler wiring, and tests are all aligned.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 40 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on two correctness issues before merge.
📋 Key Themes & Highlights
Key Issues
-
Empty permissions not guarded (
add_labels.go): setting bothissues: falseandpull-requests: falsesilently compiles a workflow whose App token has no write scope foradd-labels. This fails at runtime, not at compile time. A validation error should be raised. -
Runtime config keys not consumed (
safe_outputs_handler_registry.go): the newissuesandpull-requestskeys are threaded into the runtime handler config butadd_labels.cjsnever reads them. If enforcement is compile-time only (App token lacks the scope), consider omitting the keys from the runtime payload to avoid confusion and key-naming inconsistency (other keys are snake_case). -
Missing edge-case test: no test covers the
issues: false+pull-requests: falsecombination, leaving the silent-failure scenario undocumented.
Positive Highlights
- ✅ Nil-coalescing defaults correctly preserve backward compatibility for existing workflows
- ✅ Schema, compile-time logic, and handler registry updated together — no partial wiring
- ✅ Regression test covers the specific failure scenario that motivated the fix (#48877)
- ✅
buildAddLabelsPermissionsmirrors the existing pattern fromadd-comment, keeping the codebase consistent
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 78.5 AIC · ⊞ 7K
Comment /matt to run again
| // buildAddLabelsPermissions computes the permissions for add_labels based on config. | ||
| // Issues: nil or true → issues:write (default: true) | ||
| // PullRequests: nil or true → pull-requests:write (default: true) | ||
| func buildAddLabelsPermissions(config *AddLabelsConfig) *Permissions { |
There was a problem hiding this comment.
[/diagnosing-bugs] No guard against issues: false + pull-requests: false — both disabled yields an empty permissions map. The compiled workflow and App token will have no write scope for add-labels, failing silently at runtime instead of producing a compile-time error.
💡 Suggested fix
Add validation in parseAddLabelsConfig or a dedicated validator alongside the existing max-field check:
if config.Issues != nil && !*config.Issues &&
config.PullRequests != nil && !*config.PullRequests {
return errors.New("add-labels: at least one of 'issues' or 'pull-requests' must be enabled")
}This surfaces the misconfiguration early with a clear error message rather than a silent runtime token failure.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 6b8e111. Added validateAddLabelsPermissions in add_labels.go that returns a compile-time error when both issues: false and pull-requests: false are set simultaneously:
safe-outputs.add-labels: at least one of 'issues' or 'pull-requests' must be enabled
The validation is registered in compiler_validators.go alongside the other safe-outputs validators.
| AddStringSlice("blocked", c.Blocked). | ||
| AddBoolPtr("issue_intent", c.IssueIntent). | ||
| AddBoolPtr("issues", c.Issues). | ||
| AddBoolPtr("pull-requests", c.PullRequests). |
There was a problem hiding this comment.
[/codebase-design] The runtime config key "pull-requests" (kebab-case) differs from all other handler config keys for add_labels which use snake_case (issue_intent, required_labels, required_title_prefix, etc.). The add_labels.cjs runtime handler currently reads config.issue_intent but has no code path that reads config['pull-requests'] or config.issues, so these flags are threaded into the compiled config payload but never consumed.
💡 Detail
If the runtime handler is intended to enforce the scope restriction at execution time (e.g. refuse to label a PR when pull-requests: false), the CJS handler needs to be updated to read and act on these flags. If enforcement is compile-time only (the App token simply lacks the scope), document that explicitly and consider whether to omit these keys from the runtime config to avoid confusion. If the key must be included, prefer snake_case for consistency: "pull_requests".
@copilot please address this.
There was a problem hiding this comment.
Fixed in 6b8e111. The issues and pull-requests flags are compile-time only — they control which permission scopes are included in the minted App token. The CJS runtime handler never reads them, so including them in the handler config payload was misleading. Removed both AddBoolPtr("issues", ...) and AddBoolPtr("pull-requests", ...) from the add_labels registry builder.
| Issues: ptrBool(false), | ||
| }, | ||
| }, | ||
| expected: map[PermissionScope]PermissionLevel{ |
There was a problem hiding this comment.
[/tdd] The two new test cases cover each opt-out scenario individually but there is no test for the issues: false + pull-requests: false combination, which is the edge case most likely to cause a silent runtime failure (empty permissions map). Consider adding a third case:
💡 Suggested test case
{
name: "add-labels with both issues:false and pull-requests:false - empty permissions",
safeOutputs: &SafeOutputsConfig{
AddLabels: &AddLabelsConfig{
BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("5")},
Issues: ptrBool(false),
PullRequests: ptrBool(false),
},
},
// Expected: either a compile error, or an empty permission map
// depending on whether validation is added
expected: map[PermissionScope]PermissionLevel{},
},This test documents the current (unguarded) behaviour and will break usefully if validation is added later.
@copilot please address this.
There was a problem hiding this comment.
Added in 6b8e111. Added TestValidateAddLabelsPermissions covering:
- nil config /
AddLabelsnot configured — no error - both nil (defaults) — no error
issues:false, pull-requests:true— no errorissues:true, pull-requests:false— no errorissues:false, pull-requests:false— error with expected message
Since validation now errors on the both-false case, the test expects a compile error rather than an empty permissions map.
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review: least-privilege opt-out for add-labels permissions
Approach mirrors the existing add-comment permission-toggle pattern and is implemented consistently, but it inherits (and slightly extends) a validation gap: nothing ties the new issues/pull-requests opt-outs to target, so pull-requests: false combined with a PR-eligible target can compile cleanly and fail at runtime with a GitHub API permissions error. Also, setting both flags false silently yields a zero-permission handler with no warning.
💡 Themes
- Structural consistency:
buildAddLabelsPermissionsclosely mirrorsbuildAddCommentPermissions(added in a very recent prior commit), so this change follows established convention rather than introducing a new pattern. - Runtime vs compile-time gap: GitHub's REST API for adding labels requires both
issues:writeandpull-requests:writewhen the target could be a PR. No cross-validation exists between these new opt-out flags and thetargetfield, so a misconfigured workflow will only fail when it actually runs, not when it is compiled. - Both-false edge case: setting
issues: falseandpull-requests: falsetogether produces a technically valid but functionally useless (zero-permission) handler with no compiler feedback. - Test coverage: new tests cover each flag in isolation (
pull-requests: falsealone,issues: falsealone) plus an app-token end-to-end case, but do not cover the combined-false or target-interaction scenarios called out above.
Not blocking merge since this mirrors an already-accepted pattern in add-comment, but flagged as a real correctness gap worth addressing (ideally for both call sites) in a follow-up.
🔎 Code quality review by PR Code Quality Reviewer · auto · 193.3 AIC · ⊞ 7.8K
Comment /review to run again
| // buildAddLabelsPermissions computes the permissions for add_labels based on config. | ||
| // Issues: nil or true → issues:write (default: true) | ||
| // PullRequests: nil or true → pull-requests:write (default: true) | ||
| func buildAddLabelsPermissions(config *AddLabelsConfig) *Permissions { |
There was a problem hiding this comment.
No validation prevents issues: false together with pull-requests: false, silently minting zero permissions for add-labels.
There was a problem hiding this comment.
Fixed in 6b8e111. Added validateAddLabelsPermissions — returns a compile error when both are false.
| // buildAddLabelsPermissions computes the permissions for add_labels based on config. | ||
| // Issues: nil or true → issues:write (default: true) | ||
| // PullRequests: nil or true → pull-requests:write (default: true) | ||
| func buildAddLabelsPermissions(config *AddLabelsConfig) *Permissions { |
There was a problem hiding this comment.
Disabling pull-requests has no compile-time link to target; if the workflow can label PRs (e.g. target: "*" or triggered on a PR) but sets pull-requests: false, the runtime issues.addLabels call will fail with a permissions error the compiler never warns about.
💡 Details
GitHub's REST docs list POST /repos/{owner}/{repo}/issues/{issue_number}/labels as requiring both issues:write and pull_requests:write for GitHub App tokens when the target could be a PR (a PR is an issue under the hood). This PR lets users independently opt out of either permission without validating that against target/trigger context, so add-labels + pull-requests: false + a PR-eligible target is a real foot-gun that will only surface at runtime, not at gh aw compile time.
Suggested mitigation: add a validation check (e.g., in safe_outputs_validation.go) that errors/warns when pull-requests: false is combined with a target value that can resolve to a pull request, similar to how the analogous add-comment config shares this same gap today.
There was a problem hiding this comment.
Acknowledged as a real gap. However, this is a pre-existing limitation shared by other handlers (add-comment has the same gap today) — the compiler has no target-type inference, so validating pull-requests: false against a potentially-PR-eligible target requires a broader cross-handler effort. Tracking this as follow-up work rather than expanding scope here.
PR Triage
Note: Permission opt-out for add-labels safe output; 1 CI failure; active review (5 reviews)
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…e config, add tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot quick triage for this PR:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed in the latest push:
|
|
Caution agentic threat detected DetailsPotential security threats were detected in the agent output. Review the workflow run logs for details. @copilot quick triage for this PR:
|
pr-finisher pass complete.
Re: the Hand-off: CI must be re-triggered by a maintainer (close/reopen PR or |
|
@copilot must also support nested github app in individual safe outputs. Add tests |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added in the latest commit. Three new tests in
|
PR TriageCategory: feature · Risk: medium · Priority: medium · Score: 64/100 (impact 30, urgency 18, quality 16) Recommended action: Batch: Lets
|
safe-outputs.add-labelsalways mintedpull-requests: write, even for issue-only workflows using a GitHub App with onlyissues: write. This made least-privilege App installs fail token minting despite never targeting PRs.Permission model
issuesandpull-requeststoggles tosafe-outputs.add-labelsadd-labels.pull-requests: falsenow removespull-requests: writefrom the computed safe-output/job/App-token permissionsCompiler / handler wiring
issues: write + pull-requests: writepermission builder foradd-labelsSchema
add-labelskeys validate cleanlyRegression coverage
add-labelsopts out of PR scopeExample:
With this shape,
add-labelscan compile and mint an App token withissues: writeonly.add-labelssafe-output always mintspull-requests: write; nopull-requests: falseopt-out likeadd-comment#48877Caution
agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.
Details
Potential security threats were detected in the agent output.
Review the workflow run logs for details.
run: https://github.com/github/gh-aw/actions/runs/30692749654