Prevent invalid id-token: read emission when merging permissions: all: read with explicit overrides#43297
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
id-token: read emission when merging permissions: all: read with explicit overrides
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43297 does not have the 'implementation' label and has only 13 new lines of code in business logic directories (threshold: 100). |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This pull request fixes a GitHub Actions compatibility bug in gh-aw’s permissions compiler: when permissions: all: read is merged/materialized into an explicit permissions map (via Permissions.Set()), it could previously synthesize id-token: read, which GitHub Actions rejects. The change aligns Set()’s “all: read” expansion behavior with existing guards in Get()/Merge()/RenderToYAML() so invalid id-token levels are never emitted.
Changes:
- Updated
Permissions.Set()to skip expandingid-tokenwhen convertingall: readinto an explicit permissions map. - Added a regression test to ensure
id-tokenis not present after conversion and rendered YAML never containsid-token: read.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/permissions_operations.go | Prevents Set() from materializing an invalid id-token: read entry when expanding all: read. |
| pkg/workflow/permissions_operations_test.go | Adds a regression test covering all: read plus explicit overrides (e.g., copilot-requests: write) and asserts the rendered YAML is valid. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Low
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Prevent invalid id-token: read emission in Set()
The fix is correct, minimal, and consistent with the existing pattern across the codebase. All other expansion paths (Merge, RenderToYAML, Get, shorthand expansion) already guard against id-token: read; the Set() hasAll branch was the only missing site.
Correctness:
- The guard
s == PermissionIdToken && p.allLevel == PermissionReadexactly mirrors lines 164, 281, 296, 338, and 380 in the same file — this is the correct pattern. p.hasAll = false/p.allLevel = ""are cleared after expansion (lines 187-188), soGet()andRenderToYAML()correctly see the map-only state post-Set().- The
all: writepath is unaffected;id-token: writeis valid.
Test coverage: The new test case directly exercises the regression: NewPermissionsAllRead() + Set(PermissionCopilotRequests, PermissionWrite), then asserts id-token absent from map and YAML. Both assertions are necessary and sufficient.
No issues found.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 37 AIC · ⌖ 5.73 AIC · ⊞ 4.9K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (1 test scenario)
AnalysisThe new test block added to Two assertions validate complementary perspectives:
Both carry descriptive failure messages. Build tag The minor inflation flag (2.25:1) is marginal and appropriate given the two-assertion coverage style for a targeted bug fix. Verdict
References: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — commenting with minor improvement suggestions. The fix and test coverage are solid; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Test assertion precision: The YAML assertion checks for
id-token: readspecifically but should check for anyid-token:entry to fully capture the intent. - Test structure consistency: The existing permission tests use a well-structured table-driven pattern; the new inline case could be aligned with that style.
- Comment precision: The implementation comment could be made more concrete by citing the GitHub Actions constraint explicitly.
Positive Highlights
- ✅ Root cause correctly identified and fixed: the
all: readexpansion path inSet()was the last gap not covered by existing guards. - ✅ Fix is minimal and surgical — 4 lines, exactly mirroring the already-correct guard in the shorthand expansion path above it.
- ✅ Regression test is present and directly reproduces the failure scenario (
all: read+copilot-requests: write). - ✅ Both the map state and rendered YAML are asserted, giving full coverage of the bug's two observable symptoms.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 63 AIC · ⌖ 5.53 AIC · ⊞ 6.6K
Comment /matt to run again
| if _, exists := p3.Get(PermissionIdToken); exists { | ||
| t.Error("expected id-token to be excluded when converting all: read to explicit map") | ||
| } | ||
| if yaml := p3.RenderToYAML(); strings.Contains(yaml, "id-token: read") { |
There was a problem hiding this comment.
[/tdd] The YAML assertion only guards against id-token: read, but the intent is that id-token should be entirely absent — any other accidental level (e.g. none) would slip through.
💡 Stronger assertion
Change the assertion to check for any id-token: entry, not just the read level:
if yaml := p3.RenderToYAML(); strings.Contains(yaml, "id-token:") {
t.Errorf("RenderToYAML() should not contain any id-token entry, got:\n%s", yaml)
}This matches the intent of the fix — id-token should be completely absent from the output when starting from all: read.
@copilot please address this.
| t.Errorf("expected issues: write, got %v (exists: %v)", level, exists) | ||
| } | ||
|
|
||
| p3 := NewPermissionsAllRead() |
There was a problem hiding this comment.
[/tdd] The new test case is embedded flat inside TestPermissionsSet, while the existing test for the shorthand conversion case (TestPermissionsSetPreservesShorthandPermissions) uses a structured table-driven pattern. Embedding p3 inline makes it harder to see what scenario is being tested.
💡 Suggested refactor
Either move this case into TestPermissionsSetPreservesShorthandPermissions as a new table row, or at minimum add a descriptive comment block:
// all: read + explicit write scope must not expand id-token to any level
p3 := NewPermissionsAllRead()
p3.Set(PermissionCopilotRequests, PermissionWrite)Even better, consider adding a parallel entry in TestPermissionsSetPreservesShorthandPermissions that tests the all: read case with an assertion for PermissionIdToken absent, keeping all permission-preservation tests in one place.
@copilot please address this.
| for _, s := range GetAllPermissionScopes() { | ||
| if _, exists := p.permissions[s]; !exists { | ||
| // id-token does not support the read level | ||
| if s == PermissionIdToken && p.allLevel == PermissionRead { |
There was a problem hiding this comment.
[/diagnosing-bugs] The guard is correct and well-placed. One minor note: the comment says id-token does not support the read level — it would be more precise to say it supports only write and none (not any non-write level), which matches how GitHub Actions actually validates permissions.
💡 Suggested comment wording
// id-token only supports "write" and "none"; emitting "read" causes GitHub Actions to reject
// the workflow. Skip it entirely when expanding from all: read.This mirrors the comment in permissions_rendering.go (if one exists) and makes the rule concrete for future readers — clarifying that this isn't a gh-aw policy but a GitHub Actions constraint.
@copilot please address this.
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · 223.2 AIC · ⌖ 5.63 AIC · ⊞ 5.4K
Comment /review to run again
| if s == PermissionIdToken && p.allLevel == PermissionRead { | ||
| continue | ||
| } | ||
| p.permissions[s] = p.allLevel |
There was a problem hiding this comment.
PermissionDiscussions has the same gap this PR just closed for PermissionIdToken: Set() expands all: read into an explicit map but does not skip discussions, causing discussions: read to materialize in the explicit map where the RenderToYAML guard no longer fires.
💡 Explanation and suggested fix
When Set() is called on an all: read Permissions, it expands all scopes into p.permissions. This PR correctly skips id-token during that expansion. However, RenderToYAML also suppresses discussions: read (GHE compatibility) — but only when rendering through the hasAll branch. Once Set() has materialized discussions: read into the explicit map, the RenderToYAML guard on the hasAll branch never runs, and the scope leaks into the final YAML.
Add the matching guard alongside the id-token one in the hasAll block:
// id-token does not support the read level
if s == PermissionIdToken && p.allLevel == PermissionRead {
continue
}
// discussions: read is suppressed for GHE compatibility (only include if explicitly set)
if s == PermissionDiscussions && p.allLevel == PermissionRead {
continue
}The test added by this PR should also assert that discussions is absent from the converted map.
| } | ||
| if yaml := p3.RenderToYAML(); strings.Contains(yaml, "id-token: read") { | ||
| t.Errorf("RenderToYAML() should not contain id-token: read, got:\n%s", yaml) | ||
| } |
There was a problem hiding this comment.
Test does not assert the boundary case: all: write still correctly expands id-token: write: The test only verifies that id-token is absent after all: read conversion, but does not verify the complementary case — that the guard is not applied when allLevel == PermissionWrite, which could silently regress.
💡 Suggested additional assertion
Add a case verifying the guard is not over-applied:
// Guard must NOT fire at write level — id-token: write is valid
p4 := &Permissions{hasAll: true, allLevel: PermissionWrite, permissions: make(map[PermissionScope]PermissionLevel)}
p4.Set(PermissionCopilotRequests, PermissionWrite)
if level, exists := p4.Get(PermissionIdToken); !exists || level != PermissionWrite {
t.Errorf("expected id-token: write after all: write expansion, got level=%q exists=%v", level, exists)
}Without this, a future change that over-broadly guards PermissionIdToken for all levels would pass the current test suite.
|
@copilot merge main, recompile, lint go |
…read-scope Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in |
github-actions
Bot
added
pr-priority:high
pr-risk:low
pr-type:bug
pr-batch:compiler-fixes
labels
🤖 PR Triage
Score breakdown: Impact 38 (permissions correctness — prevents GHA runtime rejections) + Urgency 22 + Quality 12 (small fix, github-actions approved) Fixes compiler emitting
|
permissions: all: readwas safe on its own, but when merged with explicit scopes (e.g.copilot-requests: write) the compiler could materialize an explicit permissions map containingid-token: read, which GitHub Actions rejects. This change aligns theall: readexpansion path inSet()with existing merge/render guards so invalidid-tokenlevels are never emitted.What changed
Permissions.Set(pkg/workflow/permissions_operations.go) to skipPermissionIdTokenwhen convertinghasAll + allLevel=readinto explicit scope entries.id-token: readduring map materialization.Regression coverage
TestPermissionsSet(pkg/workflow/permissions_operations_test.go) with anall: read+copilot-requests: writecase.PermissionIdTokenis not present after conversion.id-token: read.