Preserve job-level permission shorthands in custom and safe jobs#47471
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Preserves job-level permission shorthands during workflow compilation.
Changes:
- Uses the shared permissions parser for custom and safe jobs.
- Retains raw safe-job permission values.
- Adds integration coverage for object and shorthand forms.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_custom_jobs.go |
Parses custom-job permissions uniformly. |
pkg/workflow/safe_jobs.go |
Preserves and renders raw safe-job permissions. |
pkg/workflow/custom_job_permissions_integration_test.go |
Tests permission compilation forms. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Medium
| if err != nil { | ||
| return fmt.Errorf("failed to convert permissions to YAML for job '%s': %w", jobName, err) | ||
| } | ||
| formattedPerms := NewPermissionsParserFromValue(permissions).ToPermissions().RenderToYAML() |
| if len(jobConfig.Permissions) > 0 { | ||
| // Build Permissions struct from map | ||
| if jobConfig.RawPermissions != nil { | ||
| job.Permissions = NewPermissionsParserFromValue(jobConfig.RawPermissions).ToPermissions().RenderToYAML() |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — one actionable finding; no blocking issues.
📋 Key Themes & Highlights
Finding
- Dead code in
safe_jobs.go: Theelse if len(jobConfig.Permissions) > 0branch (lines 347–352) is unreachable becauseRawPermissionsis always set wheneverpermissionsis present in the job config. The old map-building path should be removed.
Positive Highlights
- ✅ Clean use of the shared
NewPermissionsParserFromValuepath — consistent with how the rest of the compiler handles permissions - ✅ Good integration test coverage: all four cases (object/shorthand × custom/safe) are tested
- ✅ The fix in
compiler_custom_jobs.gois minimal and correct - ✅
RawPermissions anyyaml:"-"`` correctly avoids double-serialisation
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 21 AIC · ⌖ 4.65 AIC · ⊞ 6.7K
Comment /matt to run again
| // Build Permissions struct from map | ||
| if jobConfig.RawPermissions != nil { | ||
| job.Permissions = NewPermissionsParserFromValue(jobConfig.RawPermissions).ToPermissions().RenderToYAML() | ||
| } else if len(jobConfig.Permissions) > 0 { |
There was a problem hiding this comment.
[/codebase-design] Dead code: the else if len(jobConfig.Permissions) > 0 branch (lines 347–352) is unreachable — RawPermissions is always set alongside Permissions when any permissions key exists (see parseSafeJobsConfig lines 116–126).
This silently makes the old map-building path a no-op; NewPermissionsParserFromValue already handles both object and shorthand forms. Safe to delete the else if block entirely.
💡 Suggested fix
if jobConfig.RawPermissions != nil {
job.Permissions = NewPermissionsParserFromValue(jobConfig.RawPermissions).ToPermissions().RenderToYAML()
}The else if block duplicates logic already covered by NewPermissionsParserFromValue and will never execute.
@copilot please address this.
🧪 Test Quality Sentinel Report
📊 Metrics (2 tests)
|
There was a problem hiding this comment.
One medium-severity issue to address
The fix is correct for the bug it targets. One maintainability issue needs cleanup before this is clean.
Details
Dead branch (medium): The else if len(jobConfig.Permissions) > 0 path in buildSafeJobs is unreachable because RawPermissions is always set when permissions exists. The Permissions map[string]string field is populated but its rendering path is permanently bypassed. See inline comment.
Silent failure on unknown shorthands (low / by-design): NewPermissionsParserFromValue marks any string as isShorthand=true, but ToPermissions() returns an empty Permissions (no output) for unrecognized strings via the default case. This is consistent with the old behaviour (old code only handled map[string]any and also silently dropped unrecognized strings), so this is not a regression — but worth knowing.
Integration test coverage is good for the happy paths. No issues with the test helpers.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 37.9 AIC · ⌖ 4.66 AIC · ⊞ 5.7K
Comment /review to run again
| // Build Permissions struct from map | ||
| if jobConfig.RawPermissions != nil { | ||
| job.Permissions = NewPermissionsParserFromValue(jobConfig.RawPermissions).ToPermissions().RenderToYAML() | ||
| } else if len(jobConfig.Permissions) > 0 { |
There was a problem hiding this comment.
Dead else if branch — safeJob.Permissions map is populated but never used for rendering.
The else if len(jobConfig.Permissions) > 0 branch at line 347 is unreachable: RawPermissions is always set unconditionally at line 117 whenever permissions key exists, so the if jobConfig.RawPermissions != nil guard always fires first.
💡 Details and suggested fix
The safeJob.Permissions map[string]string is still parsed and stored (lines 119–124) but never used for rendering. This creates a misleading dual-path: future maintainers may trust that field as the canonical representation and produce wrong output silently.
Options:
- Remove the
else ifblock and thesafeJob.Permissionsmap-population loop entirely. - Or keep it with an explicit comment:
// Unreachable: RawPermissions is always set when permissions key exists. - Or remove
Permissions map[string]stringfromSafeJobConfigentirely if it has no other readers.
There was a problem hiding this comment.
Review: Preserve job-level permission shorthands
The changes correctly fix the dropped permission shorthands for both custom jobs and safe jobs. The approach — routing permissions through NewPermissionsParserFromValue instead of the type-asserted map path — is consistent with how permissions are handled elsewhere in the compiler.
One non-blocking observation: In safe_jobs.go, the else if len(jobConfig.Permissions) > 0 branch (lines 347–352) is now dead code. RawPermissions is always set alongside Permissions in parseSafeJobsConfig, so the map-based fallback path can never be reached. It causes no harm but could be cleaned up to avoid confusion.
Test coverage via the new integration tests is thorough and well-structured. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 20.1 AIC · ⌖ 5.18 AIC · ⊞ 5K
…tom and safe jobs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (141 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
|
|
Hey The fix for preserving job-level permission shorthands (
The PR is ready for review and merge. Nice work! 🚀
|
|
@copilot please run the Unresolved review feedback:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
An explicit `permissions: {}` block is semantically distinct from an
omitted permissions field: it denies all token permissions rather than
inheriting from the workflow level. Previously the shared permissions
parser converted an empty map to an ordinary empty `Permissions` whose
renderer returned `""`, silently dropping the deny-all intent.
Changes:
- Add `isExplicitEmpty bool` to `PermissionsParser` to track when an
explicit empty map was the input.
- Set the flag in `NewPermissionsParserFromValue` when the map is
non-nil but contains no keys.
- Return `NewPermissionsEmpty()` from `ToPermissions()` when the flag
is set, so `RenderToYAML()` produces `"permissions: {}"`.
- Add unit test case in `TestPermissionsParser_ToPermissions` covering
the empty-map input.
- Add integration test cases for `permissions: {}` in both
`TestCustomJobPermissionsIntegration` and
`TestSafeJobPermissionsIntegration`.
- Refactor `TestSafeJobPermissionsIntegration` to use per-case
`notExpectedParts` instead of a hardcoded blanket assertion, allowing
the new explicit-empty case to coexist cleanly.
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed in 6d656bc. Both issues raised by the reviewer are addressed:
Integration test cases for |
|
🎉 This pull request is included in a new release. Release: |
Custom job
permissionswere only reliably preserved in object form. Shorthand values accepted by the workflow schema, such asread-all, were being dropped during compilation for both top-level custom jobs andsafe-outputs.jobs.Permission parsing
permissionsthrough the shared permissions parser instead of only handling map values.Safe-job permission handling
safe-outputs.jobs.<name>.permissionsvalue and render it through the same shared parser.Integration coverage
Example:
After this change, those job-level permissions are preserved in the compiled workflow instead of being omitted.
Run URL: https://github.com/github/gh-aw/actions/runs/29984131428