From 4ec6d1dd3778670e8800a9a678393a0bef47d4ec Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 10 Jul 2026 05:01:31 +0000 Subject: [PATCH 1/3] Initial plan From 05a320e3241fbc4a910feae9e49dfe2d98e9c64b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 10 Jul 2026 05:16:47 +0000 Subject: [PATCH 2/3] Consolidate copilot requests write check Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/workflow_secrets.go | 3 +- pkg/workflow/compiler_validators_test.go | 63 +++++++++++++++++++ .../permissions_compiler_validator.go | 3 +- pkg/workflow/permissions_operations.go | 13 +++- pkg/workflow/permissions_operations_test.go | 58 +++++++++++++++++ 5 files changed, 134 insertions(+), 6 deletions(-) diff --git a/pkg/cli/workflow_secrets.go b/pkg/cli/workflow_secrets.go index 49464815842..7d288581c95 100644 --- a/pkg/cli/workflow_secrets.go +++ b/pkg/cli/workflow_secrets.go @@ -126,8 +126,7 @@ func hasCopilotRequestsWritePermission(frontmatter map[string]any) bool { return false } perms := workflow.NewPermissionsParserFromValue(permissionsValue).ToPermissions() - level, exists := perms.Get(workflow.PermissionCopilotRequests) - return exists && level == workflow.PermissionWrite + return perms.HasCopilotRequestsWrite() } func filterOutSecretRequirement(reqs []SecretRequirement, secretName string) []SecretRequirement { diff --git a/pkg/workflow/compiler_validators_test.go b/pkg/workflow/compiler_validators_test.go index c510603127f..8fcfbba3a2e 100644 --- a/pkg/workflow/compiler_validators_test.go +++ b/pkg/workflow/compiler_validators_test.go @@ -471,6 +471,69 @@ func TestValidatePermissions_EmitsCopilotRequestsTipOncePerMarkdownPath(t *testi assert.Equal(t, 1, strings.Count(stderr, tipText), "copilot-requests tip should be emitted only once per markdown path") } +func TestShouldEmitCopilotRequestsEnableTip(t *testing.T) { + tests := []struct { + name string + data *WorkflowData + perms *Permissions + expected bool + }{ + { + name: "nil workflow data", + data: nil, + perms: NewPermissions(), + expected: false, + }, + { + name: "non copilot engine", + data: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "claude"}, + }, + perms: NewPermissions(), + expected: false, + }, + { + name: "copilot engine without permission emits tip", + data: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot"}, + }, + perms: NewPermissionsContentsRead(), + expected: true, + }, + { + name: "copilot engine with permission suppresses tip", + data: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot"}, + }, + perms: func() *Permissions { + p := NewPermissionsContentsRead() + p.Set(PermissionCopilotRequests, PermissionWrite) + return p + }(), + expected: false, + }, + { + name: "explicit none suppresses tip", + data: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot"}, + }, + perms: func() *Permissions { + p := NewPermissionsContentsRead() + p.Set(PermissionCopilotRequests, PermissionNone) + return p + }(), + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := shouldEmitCopilotRequestsEnableTip(tt.data, tt.perms) + assert.Equal(t, tt.expected, got) + }) + } +} + func TestValidateToolConfiguration_EmitsSandboxWarningBeforeThreatDetectionError(t *testing.T) { tmpDir := testutil.TempDir(t, "tool-warning-test") markdownPath := filepath.Join(tmpDir, "test.md") diff --git a/pkg/workflow/permissions_compiler_validator.go b/pkg/workflow/permissions_compiler_validator.go index d799739a1ce..caec149d8bb 100644 --- a/pkg/workflow/permissions_compiler_validator.go +++ b/pkg/workflow/permissions_compiler_validator.go @@ -218,8 +218,7 @@ func shouldEmitCopilotRequestsEnableTip(workflowData *WorkflowData, workflowPerm if level, exists := workflowPermissions.GetExplicit(PermissionCopilotRequests); exists && level == PermissionNone { return false } - level, exists := workflowPermissions.Get(PermissionCopilotRequests) - return !exists || level != PermissionWrite + return !workflowPermissions.HasCopilotRequestsWrite() } func validateOIDCPermissions(workflowData *WorkflowData, workflowPermissions *Permissions) error { diff --git a/pkg/workflow/permissions_operations.go b/pkg/workflow/permissions_operations.go index 893e7727a5a..6a6a5fdadcc 100644 --- a/pkg/workflow/permissions_operations.go +++ b/pkg/workflow/permissions_operations.go @@ -56,6 +56,16 @@ func (p *Permissions) HasContentsReadAccess() bool { return false } +// HasCopilotRequestsWrite returns true if the permissions grant copilot-requests: write. +func (p *Permissions) HasCopilotRequestsWrite() bool { + if p == nil { + return false + } + + level, ok := p.Get(PermissionCopilotRequests) + return ok && level == PermissionWrite +} + // hasCopilotRequestsWritePermission returns true when workflow permissions include // copilot-requests: write. This controls whether engines should use ${{ github.token }} // for Copilot authentication instead of requiring COPILOT_GITHUB_TOKEN. @@ -70,8 +80,7 @@ func hasCopilotRequestsWritePermission(workflowData *WorkflowData) bool { if perms == nil { return false } - level, ok := perms.Get(PermissionCopilotRequests) - return ok && level == PermissionWrite + return perms.HasCopilotRequestsWrite() } // filterJobLevelPermissions takes a raw permissions YAML string (as stored in WorkflowData.Permissions) diff --git a/pkg/workflow/permissions_operations_test.go b/pkg/workflow/permissions_operations_test.go index f29f2d84cbd..235003f310e 100644 --- a/pkg/workflow/permissions_operations_test.go +++ b/pkg/workflow/permissions_operations_test.go @@ -875,6 +875,64 @@ func TestPermissions_HasContentsReadAccess(t *testing.T) { } } +func TestPermissions_HasCopilotRequestsWrite(t *testing.T) { + tests := []struct { + name string + perms *Permissions + expected bool + }{ + { + name: "nil permissions returns false", + perms: nil, + expected: false, + }, + { + name: "write-all grants copilot requests write", + perms: NewPermissionsWriteAll(), + expected: true, + }, + { + name: "read-all does not grant copilot requests write", + perms: NewPermissionsReadAll(), + expected: false, + }, + { + name: "explicit copilot requests write grants access", + perms: NewPermissionsFromMap(map[PermissionScope]PermissionLevel{ + PermissionCopilotRequests: PermissionWrite, + }), + expected: true, + }, + { + name: "explicit copilot requests none denies access", + perms: NewPermissionsFromMap(map[PermissionScope]PermissionLevel{ + PermissionCopilotRequests: PermissionNone, + }), + expected: false, + }, + { + name: "all write with explicit copilot requests none denies access", + perms: func() *Permissions { + p := NewPermissions() + p.hasAll = true + p.allLevel = PermissionWrite + p.Set(PermissionCopilotRequests, PermissionNone) + return p + }(), + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := tt.perms.HasCopilotRequestsWrite() + if result != tt.expected { + t.Errorf("HasCopilotRequestsWrite() = %v, expected %v", result, tt.expected) + } + }) + } +} + func TestFilterJobLevelPermissionsWithCache(t *testing.T) { // Verify that passing a pre-parsed *Permissions produces the same result // as parsing from the raw YAML string. From 415bd336c4e97e9a4940e5572c62c86336a9f63e Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 10 Jul 2026 05:28:52 +0000 Subject: [PATCH 3/3] docs(adr): add draft ADR-44650 for consolidating copilot-requests write check --- ...idate-copilot-requests-write-permission.md | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 docs/adr/44650-consolidate-copilot-requests-write-permission.md diff --git a/docs/adr/44650-consolidate-copilot-requests-write-permission.md b/docs/adr/44650-consolidate-copilot-requests-write-permission.md new file mode 100644 index 00000000000..052062c4438 --- /dev/null +++ b/docs/adr/44650-consolidate-copilot-requests-write-permission.md @@ -0,0 +1,48 @@ +# ADR-44650: Consolidate `copilot-requests: write` Permission Check on `*Permissions` + +**Date**: 2026-07-10 +**Status**: Draft +**Deciders**: pelikhan (PR author), copilot-swe-agent (implementor) + +--- + +### Context + +The `copilot-requests: write` permission check — determining whether a workflow has granted Copilot token-auth enablement — was implemented inline at three separate call sites: `hasCopilotRequestsWritePermission` in `pkg/workflow/permissions_operations.go`, `shouldEmitCopilotRequestsEnableTip` in `pkg/workflow/permissions_compiler_validator.go`, and `hasCopilotRequestsWritePermission` in `pkg/cli/workflow_secrets.go`. Each re-derived the same `perms.Get(PermissionCopilotRequests) && level == PermissionWrite` logic independently. The `*Permissions` type already hosted an analogous method, `HasContentsReadAccess()`, establishing a prior pattern for centralising permission predicates on the type. + +### Decision + +We will add `(*Permissions).HasCopilotRequestsWrite() bool` as the single source of truth for the `copilot-requests: write` check. All three existing call sites are updated to delegate to this method. The method handles nil receivers, consistent with the existing `HasContentsReadAccess()` pattern. + +### Alternatives Considered + +#### Alternative 1: Keep Inline Checks at Each Call Site (Status Quo) + +Each call site continues to call `perms.Get(PermissionCopilotRequests)` and compare the result to `PermissionWrite`. No shared helper is introduced. + +This was rejected because the three implementations can silently diverge over time (e.g., one site adds nil-guard logic that the others miss), making the effective permission rule difficult to reason about and audit. + +#### Alternative 2: Extract a Package-Level Helper Function + +A package-private function `hasCopilotRequestsWrite(p *Permissions) bool` is extracted rather than adding a method to `*Permissions`. + +This was rejected because a method on `*Permissions` is consistent with the existing `HasContentsReadAccess()` pattern, keeps the nil-safety concern inside the type, and is discoverable by callers who already hold a `*Permissions` value. + +### Consequences + +#### Positive +- Single location for the `copilot-requests: write` business rule — future changes to how this permission is evaluated affect all call sites uniformly. +- Built-in nil safety — callers no longer need to guard against a nil `*Permissions` before checking the permission. +- Follows the established `Has*` method convention on `*Permissions`, improving API consistency. + +#### Negative +- Increases the public API surface of the `Permissions` type by one exported method. +- A bug introduced in `HasCopilotRequestsWrite()` now propagates to all three callers simultaneously, whereas previously each call site could only affect its own code path. + +#### Neutral +- No change in observable behaviour at the call sites — the refactor is behaviour-preserving by construction. +- New unit tests are added for `HasCopilotRequestsWrite()` directly, covering nil, shorthand write-all, read-all, explicit write, explicit none, and write-all-with-explicit-none override cases. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*