From 0e9ae4a2d86e1e70f08064f1c103871b89c180d3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 10 Apr 2026 10:13:31 +0000 Subject: [PATCH 1/3] Initial plan From f540b14744e766d0c7dbb2b9a9ee6e84e7d7da14 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 10 Apr 2026 10:33:15 +0000 Subject: [PATCH 2/3] Fix codemod not to convert id-token and copilot-requests write permissions to read Agent-Logs-Url: https://github.com/github/gh-aw/sessions/fd1159b5-5841-4727-8f39-ffe63a11b37d Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/codemod_permissions_test.go | 121 +++++++++++++++++++++++++++ pkg/cli/codemod_permissions_write.go | 23 ++++- 2 files changed, 143 insertions(+), 1 deletion(-) diff --git a/pkg/cli/codemod_permissions_test.go b/pkg/cli/codemod_permissions_test.go index 4f0d93cd9b2..46db8b2a807 100644 --- a/pkg/cli/codemod_permissions_test.go +++ b/pkg/cli/codemod_permissions_test.go @@ -428,3 +428,124 @@ This workflow needs permissions.` assert.Contains(t, result, "# Test Workflow") assert.Contains(t, result, "This workflow needs permissions.") } + +func TestWritePermissionsCodemod_SkipsIdToken(t *testing.T) { + codemod := getMigrateWritePermissionsToReadCodemod() + + content := `--- +on: workflow_dispatch +permissions: + contents: read + id-token: write +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "permissions": map[string]any{ + "contents": "read", + "id-token": "write", + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.False(t, applied, "Should not be applied when only write-only permissions have write") + assert.Equal(t, content, result) + assert.Contains(t, result, "id-token: write") + assert.NotContains(t, result, "id-token: read") +} + +func TestWritePermissionsCodemod_SkipsCopilotRequests(t *testing.T) { + codemod := getMigrateWritePermissionsToReadCodemod() + + content := `--- +on: workflow_dispatch +permissions: + contents: read + copilot-requests: write +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "permissions": map[string]any{ + "contents": "read", + "copilot-requests": "write", + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.False(t, applied, "Should not be applied when only write-only permissions have write") + assert.Equal(t, content, result) + assert.Contains(t, result, "copilot-requests: write") + assert.NotContains(t, result, "copilot-requests: read") +} + +func TestWritePermissionsCodemod_MixedWithIdToken(t *testing.T) { + codemod := getMigrateWritePermissionsToReadCodemod() + + content := `--- +on: workflow_dispatch +permissions: + contents: write + issues: write + id-token: write +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "permissions": map[string]any{ + "contents": "write", + "issues": "write", + "id-token": "write", + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "contents: read") + assert.Contains(t, result, "issues: read") + // id-token must remain write — "read" is not a valid value for it + assert.Contains(t, result, "id-token: write") + assert.NotContains(t, result, "id-token: read") +} + +func TestWritePermissionsCodemod_MixedWithCopilotRequests(t *testing.T) { + codemod := getMigrateWritePermissionsToReadCodemod() + + content := `--- +on: workflow_dispatch +permissions: + contents: write + copilot-requests: write +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "permissions": map[string]any{ + "contents": "write", + "copilot-requests": "write", + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "contents: read") + // copilot-requests must remain write — "read" is not a valid value for it + assert.Contains(t, result, "copilot-requests: write") + assert.NotContains(t, result, "copilot-requests: read") +} diff --git a/pkg/cli/codemod_permissions_write.go b/pkg/cli/codemod_permissions_write.go index 88623b7a511..3cd4fe419b3 100644 --- a/pkg/cli/codemod_permissions_write.go +++ b/pkg/cli/codemod_permissions_write.go @@ -9,6 +9,13 @@ import ( var writePermissionsCodemodLog = logger.New("cli:codemod_permissions") +// writeOnlyPermissions are permission scopes that only accept "write" or "none" as valid values. +// These must never be converted to "read" since "read" is not a valid value for them. +var writeOnlyPermissions = map[string]bool{ + "id-token": true, // OIDC token: only write or none + "copilot-requests": true, // Copilot authentication token: only write or none +} + // getMigrateWritePermissionsToReadCodemod creates a codemod for converting write permissions to read func getMigrateWritePermissionsToReadCodemod() Codemod { return Codemod{ @@ -35,7 +42,12 @@ func getMigrateWritePermissionsToReadCodemod() Codemod { // Handle map format if mapValue, ok := permissionsValue.(map[string]any); ok { - for _, value := range mapValue { + for key, value := range mapValue { + // Skip write-only permissions (e.g. id-token, copilot-requests) since + // "read" is not a valid value for them — they only accept "write" or "none" + if writeOnlyPermissions[key] { + continue + } if strValue, ok := value.(string); ok && strValue == "write" { hasWritePermissions = true break @@ -91,8 +103,17 @@ func getMigrateWritePermissionsToReadCodemod() Codemod { parts := strings.SplitN(line, ":", 2) if len(parts) >= 2 { key := parts[0] + permKey := strings.TrimSpace(key) valueAndComment := parts[1] + // Skip write-only permissions (e.g. id-token, copilot-requests) since + // "read" is not a valid value for them — they only accept "write" or "none" + if writeOnlyPermissions[permKey] { + result[i] = line + writePermissionsCodemodLog.Printf("Skipping write-only permission %q on line %d", permKey, i+1) + continue + } + // Replace "write" with "read" in the value part newValueAndComment := strings.Replace(valueAndComment, " write", " read", 1) result[i] = fmt.Sprintf("%s:%s", key, newValueAndComment) From e1b439c0f1eec8a0abf183f2bc287a1845e87a4a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 10 Apr 2026 11:18:40 +0000 Subject: [PATCH 3/3] Add missing assertion messages to new codemod permission tests Agent-Logs-Url: https://github.com/github/gh-aw/sessions/34d4cfee-ccd4-46b0-b132-c091443990ed Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/agentdrain/data/default_weights.json | 56 ++++-------------------- pkg/cli/codemod_permissions_test.go | 38 ++++++++-------- 2 files changed, 28 insertions(+), 66 deletions(-) diff --git a/pkg/agentdrain/data/default_weights.json b/pkg/agentdrain/data/default_weights.json index f232d2e905d..74ea4e2b819 100644 --- a/pkg/agentdrain/data/default_weights.json +++ b/pkg/agentdrain/data/default_weights.json @@ -100,12 +100,7 @@ ], "config": { "Depth": 4, - "ExcludeFields": [ - "session_id", - "trace_id", - "span_id", - "timestamp" - ], + "ExcludeFields": ["session_id", "trace_id", "span_id", "timestamp"], "MaskRules": [ { "Name": "uuid", @@ -151,21 +146,12 @@ "id": 1, "size": 100, "stage": "finish", - "template": [ - "stage=finish", - "\u003c*\u003e", - "tokens=\u003cNUM\u003e" - ] + "template": ["stage=finish", "\u003c*\u003e", "tokens=\u003cNUM\u003e"] } ], "config": { "Depth": 4, - "ExcludeFields": [ - "session_id", - "trace_id", - "span_id", - "timestamp" - ], + "ExcludeFields": ["session_id", "trace_id", "span_id", "timestamp"], "MaskRules": [ { "Name": "uuid", @@ -211,21 +197,12 @@ "id": 1, "size": 58, "stage": "plan", - "template": [ - "stage=plan", - "errors=\u003cNUM\u003e", - "turns=\u003cNUM\u003e" - ] + "template": ["stage=plan", "errors=\u003cNUM\u003e", "turns=\u003cNUM\u003e"] } ], "config": { "Depth": 4, - "ExcludeFields": [ - "session_id", - "trace_id", - "span_id", - "timestamp" - ], + "ExcludeFields": ["session_id", "trace_id", "span_id", "timestamp"], "MaskRules": [ { "Name": "uuid", @@ -269,12 +246,7 @@ "clusters": null, "config": { "Depth": 4, - "ExcludeFields": [ - "session_id", - "trace_id", - "span_id", - "timestamp" - ], + "ExcludeFields": ["session_id", "trace_id", "span_id", "timestamp"], "MaskRules": [ { "Name": "uuid", @@ -318,12 +290,7 @@ "clusters": null, "config": { "Depth": 4, - "ExcludeFields": [ - "session_id", - "trace_id", - "span_id", - "timestamp" - ], + "ExcludeFields": ["session_id", "trace_id", "span_id", "timestamp"], "MaskRules": [ { "Name": "uuid", @@ -1448,12 +1415,7 @@ ], "config": { "Depth": 4, - "ExcludeFields": [ - "session_id", - "trace_id", - "span_id", - "timestamp" - ], + "ExcludeFields": ["session_id", "trace_id", "span_id", "timestamp"], "MaskRules": [ { "Name": "uuid", @@ -1493,4 +1455,4 @@ }, "next_id": 15 } -} \ No newline at end of file +} diff --git a/pkg/cli/codemod_permissions_test.go b/pkg/cli/codemod_permissions_test.go index 46db8b2a807..7d177070bcf 100644 --- a/pkg/cli/codemod_permissions_test.go +++ b/pkg/cli/codemod_permissions_test.go @@ -451,11 +451,11 @@ permissions: result, applied, err := codemod.Apply(content, frontmatter) - require.NoError(t, err) + require.NoError(t, err, "codemod should not return an error") assert.False(t, applied, "Should not be applied when only write-only permissions have write") - assert.Equal(t, content, result) - assert.Contains(t, result, "id-token: write") - assert.NotContains(t, result, "id-token: read") + assert.Equal(t, content, result, "codemod result should be unchanged when only write-only permissions have write") + assert.Contains(t, result, "id-token: write", "id-token permission should remain write after codemod") + assert.NotContains(t, result, "id-token: read", "id-token should never be converted to read") } func TestWritePermissionsCodemod_SkipsCopilotRequests(t *testing.T) { @@ -480,11 +480,11 @@ permissions: result, applied, err := codemod.Apply(content, frontmatter) - require.NoError(t, err) + require.NoError(t, err, "codemod should not return an error") assert.False(t, applied, "Should not be applied when only write-only permissions have write") - assert.Equal(t, content, result) - assert.Contains(t, result, "copilot-requests: write") - assert.NotContains(t, result, "copilot-requests: read") + assert.Equal(t, content, result, "codemod result should be unchanged when only write-only permissions have write") + assert.Contains(t, result, "copilot-requests: write", "copilot-requests permission should remain write after codemod") + assert.NotContains(t, result, "copilot-requests: read", "copilot-requests should never be converted to read") } func TestWritePermissionsCodemod_MixedWithIdToken(t *testing.T) { @@ -511,13 +511,13 @@ permissions: result, applied, err := codemod.Apply(content, frontmatter) - require.NoError(t, err) - assert.True(t, applied) - assert.Contains(t, result, "contents: read") - assert.Contains(t, result, "issues: read") + require.NoError(t, err, "codemod should not return an error") + assert.True(t, applied, "codemod should be applied when non-write-only permissions have write") + assert.Contains(t, result, "contents: read", "contents permission should be downgraded from write to read") + assert.Contains(t, result, "issues: read", "issues permission should be downgraded from write to read") // id-token must remain write — "read" is not a valid value for it - assert.Contains(t, result, "id-token: write") - assert.NotContains(t, result, "id-token: read") + assert.Contains(t, result, "id-token: write", "id-token permission should remain write after codemod") + assert.NotContains(t, result, "id-token: read", "id-token should never be converted to read") } func TestWritePermissionsCodemod_MixedWithCopilotRequests(t *testing.T) { @@ -542,10 +542,10 @@ permissions: result, applied, err := codemod.Apply(content, frontmatter) - require.NoError(t, err) - assert.True(t, applied) - assert.Contains(t, result, "contents: read") + require.NoError(t, err, "codemod should not return an error") + assert.True(t, applied, "codemod should be applied when non-write-only permissions have write") + assert.Contains(t, result, "contents: read", "contents permission should be downgraded from write to read") // copilot-requests must remain write — "read" is not a valid value for it - assert.Contains(t, result, "copilot-requests: write") - assert.NotContains(t, result, "copilot-requests: read") + assert.Contains(t, result, "copilot-requests: write", "copilot-requests permission should remain write after codemod") + assert.NotContains(t, result, "copilot-requests: read", "copilot-requests should never be converted to read") }