Retain jobs.*.permissions for built-in safe_outputs and conclusion jobs#50642
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation is correct and well-tested. applyBuiltinJobPermissionsAugmentation uses the existing Merge API correctly — compiler-computed permissions are preserved and user-declared scopes (e.g. id-token: write) are layered on top additively. The error-path logic for augmentedField reporting is correctly extended to cover the .permissions-only case. The integration test covers the stated scenario end-to-end. No blocking issues found.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 25.8 AIC · ⌖ 7.4 AIC · ⊞ 5.4K
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (107 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report
📊 Metrics (1 test)
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — commenting, no blocking issues but three improvements recommended.
📋 Key Themes & Highlights
Key Themes
- Unreachable nil guard (
compiler_custom_jobs.go:873) —ToPermissions()never returns nil; the guard silently swallows invalidpermissionsvalues. - Test doesn't isolate the actual regression (
builtin_job_permissions_integration_test.go:50) — the test fixture declaresid-token: writeat both the top-level and job level, so the compiler may compute it anyway; the test doesn't prove the augmentation path is what preserved it. - Error message ambiguity (
compiler_custom_jobs.go:793) — whenneeds+permissionsare both set on a non-existent job, the error field collapses to the bare job name instead of naming the offending fields.
Positive Highlights
- ✅ Additive merge semantics are correct — compiler-computed scopes are preserved, user scopes added on top.
- ✅ Clean extraction into a standalone
applyBuiltinJobPermissionsAugmentationhelper makes the logic independently testable. - ✅
hasPermissionsdetection is correctly placed before the early-continue, so the early-exit is only skipped when needed. - ✅ Integration test exercises the full compile path end-to-end.
| if userPermissions == nil { | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
[/tdd] userPermissions == nil is unreachable — ToPermissions() always returns a non-nil *Permissions. This guard silently swallows a misconfigured value (e.g. permissions: true) as a no-op instead of surfacing an error.
💡 Suggested fix
Either remove the nil check, or validate with an explicit empty/invalid check and return an error:
userPermissions := NewPermissionsParserFromValue(permissionsValue).ToPermissions()
// Remove the nil guard; ToPermissions() always returns non-nilIf you want to guard against a scalar value being passed, detect that before calling ToPermissions().
@copilot please address this.
|
|
||
| workflowFile := filepath.Join(tmpDir, "builtin-job-permissions-augmentation.md") | ||
| require.NoError(t, os.WriteFile(workflowFile, []byte(workflowContent), 0644)) | ||
| require.NoError(t, compiler.CompileWorkflow(workflowFile)) |
There was a problem hiding this comment.
[/tdd] The test only asserts the happy path where the user-declared permissions match the compiler-computed ones. It doesn't cover the additive-merge case where the user declares a scope not already present in the compiler-computed permissions (the core bug scenario: id-token: write being dropped because the compiler doesn't compute it).
💡 Suggested additional assertion
Add a separate sub-test (or extend the existing one) where the compiler-computed job would have contents: read, issues: write but id-token is absent, then assert that after augmentation id-token: write appears. For example:
// workflow only declares id-token at job level, not at top level
// compiler should NOT compute id-token — verify it is added by augmentation
assert.Equal(t, "write", perms["id-token"], "id-token should be injected by augmentation, not compiler")This is the actual regression the PR is fixing; the current test doesn't isolate it.
@copilot please address this.
| } else { | ||
| augmentedField = configuredJobName + ".permissions" | ||
| } | ||
| } else if augmentedIf != "" || hasPermissions { |
There was a problem hiding this comment.
[/diagnosing-bugs] The error-field reporting when needs is set has ambiguous logic. When len(augmentedNeeds) > 0 and hasPermissions is true but augmentedIf == "", the field is reported as just configuredJobName (no sub-field), which hides which specific field caused the augmentation attempt.
💡 Details
The original code reported the exact field (needs, if, or the combined job name). The new || hasPermissions in else if augmentedIf != "" || hasPermissions means that when only needs and permissions are set (no if), the error message points to the bare job name rather than naming the offending field. Consider listing all present fields explicitly, e.g. configuredJobName + ".needs+permissions", or enumerating them for clarity in the error.
@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Adds retention of user-declared permissions for generated built-in jobs.
Changes:
- Merges built-in job permissions with compiler-computed permissions.
- Improves missing-job error reporting.
- Adds integration coverage for OIDC permissions.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_custom_jobs.go |
Implements permission augmentation and errors. |
pkg/workflow/builtin_job_permissions_integration_test.go |
Tests retained OIDC permissions. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (1)
pkg/workflow/compiler_custom_jobs.go:792
- When both
.ifand.permissionsare configured and the target job is absent, this branch reports only.if, even though the nearby comment and PR description require reporting the configured field(s). Use the job path when multiple augmentation fields are present, and a field-specific path only when exactly one is present.
if augmentedIf != "" {
augmentedField = configuredJobName + ".if"
} else {
augmentedField = configuredJobName + ".permissions"
}
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Balanced
| userPermissions := NewPermissionsParserFromValue(permissionsValue).ToPermissions() | ||
| if userPermissions == nil { | ||
| return nil | ||
| } |
| _, hasPermissions := configMap["permissions"] | ||
| if len(augmentedNeeds) == 0 && augmentedIf == "" && !hasPermissions { |
| perms, ok := job["permissions"].(map[string]any) | ||
| require.True(t, ok, "expected %s permissions to be a map", jobName) | ||
| assert.Equal(t, "write", perms["id-token"], "%s should retain id-token: write from jobs.%s.permissions", jobName, jobName) |
There was a problem hiding this comment.
Review: Retain jobs.*.permissions for built-in safe_outputs/conclusion jobs
No blocking issues found. The additive merge (write-over-read, via Permissions.Merge) correctly preserves compiler-computed least-privilege scopes while layering in user-declared ones, and the new integration test exercises the exact regression scenario from the linked issue.
💡 Verification notes
I independently compiled several edge-case workflows to stress-test the merge logic:
permissions: write-allshorthand on a built-in job → expands correctly, all compiler scopes retained at write.issues: noneon a scope the compiler had already set to write → correctly downgraded to none (author intent honored).actions: none/models: noneon scopes the compiler never touched → correctly added as none.- Invalid types (
permissions: 123,all: write) are already rejected by the JSON schema before this code path runs, so no schema hole here.
The error-message branch in applyBuiltinJobAugmentations (deciding whether to report .needs, .if, .permissions, or the bare job name when the job does not exist) is a little convoluted with the added hasPermissions condition, but it is only diagnostic text and was verified to produce sensible output for all combinations I tried — not blocking.
|
@copilot please address the remaining blockers on this PR:
Run: https://github.com/github/gh-aw/actions/runs/31034308996
|
PR Triage: #50642Category: bug | Risk: high | Priority Score: 82/100 (impact 45, urgency 22, quality 15)
|
Permissions declared under
jobs.<built-in>.permissions(e.g.safe_outputs,conclusion) were dropped from the compiled lock file, since built-in job permissions come solely from least-privilege computation and thejobs.<builtin>block was only consulted forneeds/if. Scopes likeid-token: writewent missing, breaking OIDC token minting.Changes
compiler_custom_jobs.go—applyBuiltinJobAugmentationsnow also detects apermissionsblock underjobs.<builtin>and merges it into the built-in job via a new helperapplyBuiltinJobPermissionsAugmentation. The merge is additive over the compiler-computed permissions (write overrides read), so no compiler-required scope is lost..permissionswhen that is the only field configured.builtin_job_permissions_integration_test.gocompiles the issue scenario and assertsid-token: writesurvives on bothsafe_outputsandconclusion.Example
Compiled
safe_outputs/conclusionnow include: