Skip write permissions for staged safe output handlers#21903
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/faa27e25-cf6d-4c9f-84d7-d56982dd015b
…Outputs When a safe output handler is marked as staged (either globally via safeOutputs.Staged or per-handler via BaseSafeOutputConfig.Staged), it only emits preview output and does not make real GitHub API calls. Therefore, such handlers should not contribute write permissions. - Add isHandlerStaged() helper to check staged state - Modify ComputePermissionsForSafeOutputs to skip permission merges for staged handlers - Add TestComputePermissionsForSafeOutputs_Staged with 8 test cases Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/faa27e25-cf6d-4c9f-84d7-d56982dd015b
There was a problem hiding this comment.
Pull request overview
Updates safe-output permission computation so staged handlers (global or per-handler) don’t contribute write permissions, aligning permissions with “preview-only” staged behavior.
Changes:
- Add
isHandlerStaged(globalStaged, handlerStaged)helper and use it to skip permission merges for staged handlers inComputePermissionsForSafeOutputs. - Add staged-mode test cases covering global staged, per-handler staged, mixed staged/non-staged, and PR review operations.
- Update the
smoke-codex.lock.ymldynamic tool schema and step inputs for adding the “smoked” label.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/workflow/safe_outputs_permissions.go | Skips permission merges for handlers that are effectively staged (global/per-handler). |
| pkg/workflow/safe_outputs_permissions_test.go | Adds coverage for staged permission computation scenarios. |
| .github/workflows/smoke-codex.lock.yml | Tightens dynamic tool schema and changes action inputs to labels/number parsed from JSON payload. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -118,23 +131,23 @@ func ComputePermissionsForSafeOutputs(safeOutputs *SafeOutputsConfig) *Permissio | |||
| permissions.Merge(NewPermissionsContentsWritePRWrite()) | |||
| } | |||
| } | |||
| if safeOutputs.PushToPullRequestBranch != nil { | |||
| if safeOutputs.PushToPullRequestBranch != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.PushToPullRequestBranch.Staged) { | |||
| safeOutputsPermissionsLog.Print("Adding permissions for push-to-pull-request-branch") | |||
| permissions.Merge(NewPermissionsContentsWritePRWrite()) | |||
| } | |||
There was a problem hiding this comment.
Skipping permission merges for staged create-pull-request / push-to-pull-request-branch removes even contents: read, but the safe_outputs job still adds actions/checkout + git setup steps whenever these handlers are configured (see usesPatchesAndCheckouts + buildSharedPRCheckoutSteps). With workflow-level permissions: {} this means secrets.GITHUB_TOKEN will have no scopes and the checkout step (and create_pull_request’s pre-preview checkoutManager.switchTo) can fail, preventing staged previews from running unless a separate PAT secret is configured. Consider either (a) keeping the minimal read permissions needed for staged PR handlers (e.g., contents: read and any required read scopes), or (b) skipping the checkout/git steps when the effective handler is staged so the job can run with no token permissions.
See below for a potential fix:
if safeOutputs.CreatePullRequests != nil {
if isHandlerStaged(safeOutputs.Staged, safeOutputs.CreatePullRequests.Staged) {
// Staged create-pull-request still requires minimal read permissions so that
// checkout/git operations (e.g., pre-preview checkout) can succeed even when
// the workflow uses permissions: {} at the top level.
safeOutputsPermissionsLog.Print("Adding minimal read permissions for staged create-pull-request")
permissions.Merge(NewPermissionsContentsRead())
} else {
// Check fallback-as-issue setting to determine permissions
if getFallbackAsIssue(safeOutputs.CreatePullRequests) {
safeOutputsPermissionsLog.Print("Adding permissions for create-pull-request with fallback-as-issue")
permissions.Merge(NewPermissionsContentsWriteIssuesWritePRWrite())
} else {
safeOutputsPermissionsLog.Print("Adding permissions for create-pull-request")
permissions.Merge(NewPermissionsContentsWritePRWrite())
}
}
}
if safeOutputs.PushToPullRequestBranch != nil {
if isHandlerStaged(safeOutputs.Staged, safeOutputs.PushToPullRequestBranch.Staged) {
// Staged push-to-pull-request-branch still requires minimal read permissions
// so the shared checkout/git steps can run with a token that has contents: read.
safeOutputsPermissionsLog.Print("Adding minimal read permissions for staged push-to-pull-request-branch")
permissions.Merge(NewPermissionsContentsRead())
} else {
safeOutputsPermissionsLog.Print("Adding permissions for push-to-pull-request-branch")
permissions.Merge(NewPermissionsContentsWritePRWrite())
}
}
| func TestComputePermissionsForSafeOutputs_Staged(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| safeOutputs *SafeOutputsConfig | ||
| expected map[PermissionScope]PermissionLevel | ||
| }{ | ||
| { | ||
| name: "global staged=true - no permissions for any handler", | ||
| safeOutputs: &SafeOutputsConfig{ | ||
| Staged: true, | ||
| CreateIssues: &CreateIssuesConfig{}, | ||
| CreateDiscussions: &CreateDiscussionsConfig{}, | ||
| AddLabels: &AddLabelsConfig{}, | ||
| }, | ||
| expected: map[PermissionScope]PermissionLevel{}, | ||
| }, | ||
| { | ||
| name: "per-handler staged=true - staged handler contributes no permissions", | ||
| safeOutputs: &SafeOutputsConfig{ | ||
| CreateIssues: &CreateIssuesConfig{ | ||
| BaseSafeOutputConfig: BaseSafeOutputConfig{Staged: true}, | ||
| }, | ||
| AddLabels: &AddLabelsConfig{}, | ||
| }, | ||
| // create-issue is staged so it contributes nothing; add-labels is not staged | ||
| expected: map[PermissionScope]PermissionLevel{ | ||
| PermissionContents: PermissionRead, | ||
| PermissionIssues: PermissionWrite, | ||
| PermissionPullRequests: PermissionWrite, | ||
| }, | ||
| }, | ||
| { | ||
| name: "all handlers per-handler staged - no permissions", | ||
| safeOutputs: &SafeOutputsConfig{ | ||
| CreateIssues: &CreateIssuesConfig{ | ||
| BaseSafeOutputConfig: BaseSafeOutputConfig{Staged: true}, | ||
| }, | ||
| CreateDiscussions: &CreateDiscussionsConfig{ | ||
| BaseSafeOutputConfig: BaseSafeOutputConfig{Staged: true}, | ||
| }, | ||
| }, | ||
| expected: map[PermissionScope]PermissionLevel{}, | ||
| }, | ||
| { | ||
| name: "global staged=true overrides per-handler staged=false", | ||
| safeOutputs: &SafeOutputsConfig{ | ||
| Staged: true, | ||
| CreatePullRequests: &CreatePullRequestsConfig{ | ||
| BaseSafeOutputConfig: BaseSafeOutputConfig{Staged: false}, | ||
| }, | ||
| DispatchWorkflow: &DispatchWorkflowConfig{}, | ||
| }, | ||
| expected: map[PermissionScope]PermissionLevel{}, | ||
| }, | ||
| { | ||
| name: "global staged=false, one handler staged=true", | ||
| safeOutputs: &SafeOutputsConfig{ | ||
| Staged: false, | ||
| CreatePullRequests: &CreatePullRequestsConfig{ | ||
| BaseSafeOutputConfig: BaseSafeOutputConfig{Staged: true}, | ||
| }, | ||
| CloseIssues: &CloseIssuesConfig{}, | ||
| }, | ||
| // create-pull-request is staged; close-issue is not | ||
| expected: map[PermissionScope]PermissionLevel{ | ||
| PermissionContents: PermissionRead, | ||
| PermissionIssues: PermissionWrite, | ||
| }, | ||
| }, | ||
| { | ||
| name: "global staged=true - upload-asset staged, no contents:write", | ||
| safeOutputs: &SafeOutputsConfig{ | ||
| Staged: true, | ||
| UploadAssets: &UploadAssetsConfig{}, | ||
| }, | ||
| expected: map[PermissionScope]PermissionLevel{}, | ||
| }, | ||
| { | ||
| name: "pr review operations - all staged via global flag", | ||
| safeOutputs: &SafeOutputsConfig{ | ||
| Staged: true, | ||
| CreatePullRequestReviewComments: &CreatePullRequestReviewCommentsConfig{}, | ||
| SubmitPullRequestReview: &SubmitPullRequestReviewConfig{}, | ||
| }, | ||
| expected: map[PermissionScope]PermissionLevel{}, | ||
| }, | ||
| { | ||
| name: "pr review operations - one staged, one not", | ||
| safeOutputs: &SafeOutputsConfig{ | ||
| CreatePullRequestReviewComments: &CreatePullRequestReviewCommentsConfig{ | ||
| BaseSafeOutputConfig: BaseSafeOutputConfig{Staged: true}, | ||
| }, | ||
| SubmitPullRequestReview: &SubmitPullRequestReviewConfig{}, | ||
| }, | ||
| // submit-pull-request-review is not staged, so PR write permissions are added | ||
| expected: map[PermissionScope]PermissionLevel{ | ||
| PermissionContents: PermissionRead, | ||
| PermissionPullRequests: PermissionWrite, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| permissions := ComputePermissionsForSafeOutputs(tt.safeOutputs) | ||
| require.NotNil(t, permissions, "Permissions should not be nil") | ||
|
|
||
| // Check that all expected permissions are present | ||
| for scope, expectedLevel := range tt.expected { | ||
| actualLevel, exists := permissions.Get(scope) | ||
| assert.True(t, exists, "Permission scope %s should exist", scope) | ||
| assert.Equal(t, expectedLevel, actualLevel, "Permission level for %s should match", scope) | ||
| } | ||
|
|
||
| // Check that no unexpected permissions are present | ||
| for scope := range permissions.permissions { | ||
| _, expected := tt.expected[scope] | ||
| assert.True(t, expected, "Unexpected permission scope: %s", scope) | ||
| } | ||
| }) |
There was a problem hiding this comment.
The staged-permissions tests assert the internal permissions map is empty, but they don’t validate the YAML rendering/behavior of “no permissions” vs. “explicit empty permissions”. Since job-level permissions are omitted when empty, it would be safer to add assertions around RenderToYAML() for staged scenarios to ensure the workflow ends up with the intended permissions configuration (especially given the workflow-level permissions: {} behavior).
| "payload": { | ||
| "description": "JSON-encoded payload to pass to the action", | ||
| "labels": { | ||
| "description": "The labels' name to be added. Must be separated with line breaks if there're multiple labels.", |
There was a problem hiding this comment.
Grammar in the tool input schema description is off (e.g., “The labels' name … if there're multiple labels”). Consider rephrasing for clarity (e.g., “Label names to add. Separate multiple labels with newlines.”).
| "description": "The labels' name to be added. Must be separated with line breaks if there're multiple labels.", | |
| "description": "Label names to add. Separate multiple labels with newlines.", |
|
@copilot add integration tests in pkg/cli/workflows |
…ssions - Add test-staged-permissions-global.md: workflow with global staged:true - Add test-staged-permissions-per-handler.md: workflow with per-handler staged:true - Add TestCompileStagedSafeOutputsPermissionsGlobal: verifies no write permissions when safe-outputs.staged=true globally - Add TestCompileStagedSafeOutputsPermissionsPerHandler: verifies only non-staged handlers contribute write permissions - Add TestCompileStagedSafeOutputsPermissionsAllHandlersStaged: verifies no write permissions when all handlers are individually staged Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/1afcbdcd-09a8-4d0b-b876-34a29e28d315
Added in commit 9efbae8: New workflow files in
New integration tests in
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:
|
22 links across blog posts, guides, and setup pages were missing trailing slashes, causing the Starlight link validator to fail the Doc Build CI job. Add trailing slashes to: - /gh-aw/setup/creating-workflows -> /gh-aw/setup/creating-workflows/ - /gh-aw/guides/agentic-authoring -> /gh-aw/guides/agentic-authoring/ - /gh-aw/agent-factory-status -> /gh-aw/agent-factory-status/ Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/79f8e4d7-5e14-498f-9e1e-3d0fbfd1b3ea
Fixed in commit eadce7b. The Doc Build was failing because 22 internal links in blog posts and docs pages were missing trailing slashes (required by the Starlight link validator). Added trailing slashes to |
| - agent | ||
| if: ((!cancelled()) && (needs.agent.result != 'skipped')) && (needs.agent.outputs.detection_success == 'true') | ||
| runs-on: ubuntu-slim | ||
| permissions: |
There was a problem hiding this comment.
Fixed in commit f799c0c. The safe_outputs job in poem-bot.lock.yml now has explicit permissions: {} (line 1488). This was done by:
- Updating
ComputePermissionsForSafeOutputsto returnNewPermissionsEmpty()(renders aspermissions: {}) when all handlers are staged - Updating
usesPatchesAndCheckoutsto returnfalsewhen all PR handlers are staged, so no checkout steps are added for staged-only workflows
There was a problem hiding this comment.
Thanks for confirming! The explicit permissions: {} approach is the right call here. 🤖 (Smoke test agent checking in — all systems go!)
Note
🔒 Integrity filtering filtered 1 item
Integrity filtering activated and filtered the following item during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.
- pr:Skip write permissions for staged safe output handlers #21903 (
pull_request_read: Resource 'pr:Skip write permissions for staged safe output handlers #21903' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.)
📰 BREAKING: Report filed by Smoke Copilot
This comment has been minimized.
This comment has been minimized.
|
Hey One thing to tighten up before this fully crosses the finish line:
If you'd like a hand splitting that out, here's a ready-to-use prompt:
|
- usesPatchesAndCheckouts: respect staged status for create-pull-request
and push-to-pull-request-branch, returning false when all PR handlers
are staged (no checkout/git steps added for staged-only configs)
- ComputePermissionsForSafeOutputs: return NewPermissionsEmpty() when
safe_outputs is configured but all handlers are staged, so the compiled
safe_outputs job renders "permissions: {}" explicitly
- Tests: add staged cases to TestUsesPatchesAndCheckouts, add YAML
rendering assertions to TestComputePermissionsForSafeOutputs_Staged,
add TestComputePermissionsForSafeOutputs_StagedYAMLRendering
- Recompile: poem-bot.lock.yml safe_outputs job now has permissions: {}
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/d9d7cf59-7cee-4a99-8c2d-a04ad9fa45a9
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. 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:
|
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
✅ All tools validated successfully! Agent Container Smoke Test confirms agent container is ready. |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
|
Smoke Test Results for run §23330853729
Overall: ✅ PASS — PR by Note 🔒 Integrity filtering filtered 1 itemIntegrity filtering activated and filtered the following item during workflow execution.
|
There was a problem hiding this comment.
Quick review of PR #21903 — the staged handler skip logic is well-implemented. The isHandlerStaged helper is clear and the pattern is applied consistently. See inline comments for minor observations.
Note
🔒 Integrity filtering filtered 1 item
Integrity filtering activated and filtered the following item during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.
📰 BREAKING: Report filed by Smoke Copilot
| if safeOutputs.CreateIssues != nil { | ||
| // Merge permissions for all handler-managed types. | ||
| // Staged handlers are skipped because they do not make real API calls. | ||
| if safeOutputs.CreateIssues != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.CreateIssues.Staged) { |
There was a problem hiding this comment.
The staged-handler check pattern is applied consistently across all handler types — nice. Consider extracting the repeated !isHandlerStaged(safeOutputs.Staged, handler.Staged) pattern into a small loop over a slice of handlers in the future if more handler types are added, to avoid repetition.
| // (i.e., it will only emit preview output, not make real API calls). A handler is | ||
| // staged when either the global safe-outputs staged flag is true, or the | ||
| // per-handler staged flag is true. Staged handlers do not require write permissions. | ||
| func isHandlerStaged(globalStaged, handlerStaged bool) bool { |
There was a problem hiding this comment.
Clean and readable helper! One minor note: this could equivalently be return globalStaged || handlerStaged without the named function, but having it as a named helper makes the intent explicit and improves readability across all call sites. 👍
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
Commit pushed:
|
Smoke Test (Codex)PRs: #21907 Warning
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
Note
🔒 Integrity filtering filtered 2 items
Integrity filtering activated and filtered the following items during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.
- pr:#21903 (
pull_request_read: Resource 'pr:#21903' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.) - pr:#21903 (
pull_request_read: Resource 'pr:#21903' has lower integrity than agent requires. Agent would need to drop integrity tags [approved:all unapproved:all] to trust this resource.)
💥 [THE END] — Illustrated by Smoke Claude
| // staged when either the global safe-outputs staged flag is true, or the | ||
| // per-handler staged flag is true. Staged handlers do not require write permissions. | ||
| func isHandlerStaged(globalStaged, handlerStaged bool) bool { | ||
| return globalStaged || handlerStaged |
There was a problem hiding this comment.
The isHandlerStaged helper is clean and well-placed. Consider adding a test case where both globalStaged and handlerStaged are true to document that either being true is sufficient.
| // permissions that are required by the configured safe outputs. | ||
| // Handlers that are staged (globally or per-handler) are skipped because | ||
| // staged mode only emits preview output and does not make any API calls. | ||
| func ComputePermissionsForSafeOutputs(safeOutputs *SafeOutputsConfig) *Permissions { |
There was a problem hiding this comment.
Good use of early return when safeOutputs == nil. The principle-of-least-privilege approach here is solid — staged handlers correctly skip permission requests.
Staged safe output handlers only emit step summary previews — they make no real GitHub API calls and therefore need no write permissions.
ComputePermissionsForSafeOutputswas incorrectly merging write permissions for staged handlers regardless.Changes
safe_outputs_permissions.go: AddedisHandlerStaged(globalStaged, handlerStaged bool) boolhelper; gates every permission merge inComputePermissionsForSafeOutputsbehind!isHandlerStaged(...), covering both the globalsafeOutputs.Stagedflag and per-handlerBaseSafeOutputConfig.Staged. When all handlers are staged and no permissions are accumulated, returnsNewPermissionsEmpty()so the compiled safe_outputs job renders an explicitpermissions: {}block rather than omitting it (which would inherit from the workflow-level permissions).safe_outputs_runtime.go: UpdatedusesPatchesAndCheckoutsto returnfalsewhen all PR handlers (create-pull-request,push-to-pull-request-branch) are effectively staged — preventing unnecessary checkout/git steps from being added for staged-only configs.safe_outputs_permissions_test.go: AddedTestComputePermissionsForSafeOutputs_Stagedcovering global staged, per-handler staged, mixed staged/non-staged, and the multi-handler PR review operations case, with YAML rendering assertions verifyingpermissions: {}for fully-staged configs. AddedTestComputePermissionsForSafeOutputs_StagedYAMLRenderingfor dedicated YAML rendering coverage.safe_outputs_config_helpers_test.go: Added staged test cases toTestUsesPatchesAndCheckoutscovering globally staged, per-handler staged, and mixed staged/non-staged PR handlers.pkg/cli/workflows/test-staged-permissions-global.md: New integration test workflow with globalstaged: trueacross multiple handler types.pkg/cli/workflows/test-staged-permissions-per-handler.md: New integration test workflow with per-handlerstaged: truemixed with a non-staged handler.pkg/cli/compile_integration_test.go: AddedTestCompileStagedSafeOutputsPermissionsGlobal,TestCompileStagedSafeOutputsPermissionsPerHandler, andTestCompileStagedSafeOutputsPermissionsAllHandlersStagedto verify that the compiled lock file's safe_outputs job contains no write permissions for staged scenarios.Behavior
With these configs,
ComputePermissionsForSafeOutputsnow returns explicit empty permissions (permissions: {}) for fully-staged configs, and omits only the staged handler's contribution when mixing staged and non-staged handlers. When all PR handlers are staged,usesPatchesAndCheckoutsreturnsfalseso no checkout steps are injected. The integration tests confirm this is reflected correctly in the compiled lock file'spermissions:block — for example,poem-bot.lock.yml's safe_outputs job now renderspermissions: {}explicitly.Original prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.
✨ PR Review Safe Output Test - Run 23330853717
Note
🔒 Integrity filtering filtered 2 items
Integrity filtering activated and filtered the following items during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.
pull_request_read: Resource 'pr:Skip write permissions for staged safe output handlers #21903' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.)pull_request_read: Resource 'pr:Skip write permissions for staged safe output handlers #21903' has lower integrity than agent requires. Agent would need to drop integrity tags [approved:all unapproved:all] to trust this resource.)