Consolidate copilot-requests: write permission checks on Permissions

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.*

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 {

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")

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 {

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)

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.