fix: use union of caller + worker permissions for call-workflow jobs#41387
fix: use union of caller + worker permissions for call-workflow jobs#41387Copilot wants to merge 6 commits into
Conversation
The call-workflow job's permission envelope is now computed as the union of the caller's declared permissions and the worker's job-level permissions. Previously the compiler only set the caller's declared permissions on the call-workflow job and warned when they were insufficient for the worker. GitHub rejects a workflow_call invocation at startup when any worker job requests a permission level greater than the caller job grants, causing 100% startup_failure on the Smoke Call Workflow since 2026-06-23. The fix: after extracting worker permissions, clone the caller's permissions and merge the worker's permissions in. The resulting union becomes the call-workflow job's effective permission envelope, ensuring it always covers what the worker needs. Smoke Call Workflow lock regenerated: call-smoke-workflow-call now grants actions: read, contents: read, issues: write, pull-requests: write (union of caller's contents: read, pull-requests: read and the worker's issues: write, pull-requests: write, actions: read). Closes #41355 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…owJobs Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. TQS analysis for PR #41387 already completed in this workflow run (28151256398): comment posted (score 80/100 PASS) and APPROVE review submitted. Both add_comment and submit_pull_request_review limits were consumed. No further action needed. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (148 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 Score: 80/100 ✅ ExcellentPR: fix: use union of caller + worker permissions for call-workflow jobs 1 new test function added, 5 existing tests updated to match the new behavioral contract. All 6 tests verify observable compiler output (job-level permissions in the compiled lockfile), making them high-value behavioral contract tests. The test inflation ratio is notable (27.5×) but contextually appropriate — a focused 4-line logic fix required broad test-suite alignment. 📊 Metrics & Classification
Test-function classification table
🔍 Guidelines ComplianceBuild tags: ✅ Mock libraries: ✅ None. No Assertion messages: ✅ All testify assertions carry descriptive message arguments.
|
![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 /diagnose and /tdd — approving. The fix correctly addresses the root cause and ships a well-targeted regression test. Three non-blocking suggestions below.
📋 Key Themes & Highlights
Key Themes
findUncoveredWorkerPermissionsis now dead in production — the function exists incall_workflow_permissions.goand has test coverage, but is no longer called from any non-test code path. Worth resolving (repurpose for a diagnostic info message, or remove).- Silent elevation — the old code warned users via stderr when caller permissions were insufficient; the new code auto-promotes silently. A brief compile-time info message would preserve transparency without blocking the build.
callerPerms == nil+ worker-has-perms — themerged = NewPermissions()fallback branch is untested directly.
Positive Highlights
- ✅
TestBuildCallWorkflowJobs_WorkerPermissionsElevateCallerPermissionsdirectly names and pins the invariant that was broken — exactly the right regression test structure. - ✅
callerPerms.Clone()beforeMergeis the correct defensive pattern — originaldata.CachedPermissionsis never mutated. - ✅ PR description is detailed and includes before/after YAML, making it easy to reason about the change.
- ✅ Lock file regeneration confirms the fix is end-to-end correct for the Smoke Call Workflow.
- ✅ Test comment updates accurately reflect the new union semantics throughout.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 90.2 AIC · ⌖ 8.85 AIC · ⊞ 6.5K
Comments that could not be inline-anchored
pkg/workflow/compiler_safe_output_jobs.go:329
[/diagnose] Silent elevation — users lose compiler feedback when permissions are auto-promoted.
The old code emitted a visible warning to stderr when the caller's declared permissions were insufficient. The new code silently elevates and only logs internally. A caller who deliberately set a minimal permissions: block (for least-privilege reasons) will have no compile-time signal that the block was expanded.
<details>
<summary>💡 Suggestion: emit an info-level compiler message on eleva…
pkg/workflow/call_workflow_permissions.go:33
[/diagnose] findUncoveredWorkerPermissions is no longer called from any production code path — only from test assertions.
With the new union-based approach, Merge subsumes this function's role. The function still documents the ranking semantics clearly, but its continued existence without a production caller can be confusing. Consider one of:
<details>
<summary>💡 Options</summary>
- Repurpose it for a compile-time info message (see comment on
compiler_safe_output_jobs.go) — t…
pkg/workflow/call_workflow_permissions_test.go:454
[/tdd] Missing edge case: caller has no permissions: block but the worker declares permissions.
The existing test checks caller=nil + worker=no-perms (empty merge → no permissions on job). But the merged = NewPermissions() branch (line 326 of compiler_safe_output_jobs.go) handles caller=nil + worker=has-perms — and this path isn't directly exercised.
<details>
<summary>💡 Suggested test skeleton</summary>
func TestBuildCallWorkflowJobs_NilCallerPerms_WorkerPermsApplied(t *tes…
</details>
<details><summary>pkg/workflow/compiler_safe_output_jobs.go:307</summary>
**[/zoom-out]** `effectivePerms` is aliased to `callerPerms` as the initial value, then conditionally replaced by `merged`. This works correctly, but the intent is slightly obscured — a reader needs to trace both branches to understand when `effectivePerms != callerPerms`.
<details>
<summary>💡 Optional readability improvement</summary>
The current pattern:
```go
effectivePerms := callerPerms
if markdownPath != "" {
...
effectivePerms = merged
}An alternative that makes the defa…
pkg/workflow/call_workflow_permissions_test.go:392
✅ [/tdd] TestBuildCallWorkflowJobs_WorkerPermissionsElevateCallerPermissions is exactly the right regression test — it names the invariant in the function signature, documents the real-world scenario in comments, and asserts the specific permission scopes that the bug was silently violating. Well structured with clear Arrange / Act / Assert flow.
One small sharpening opportunity: asserting NotContains(t, job.Permissions, "pull-requests: read") alongside the existing `Contains(..., "pul…
There was a problem hiding this comment.
Pull request overview
This PR updates the gh-aw compiler’s handling of reusable workflow_call invocations emitted by the call-workflow safe output. The goal is to prevent GitHub Actions startup_failure when a called (worker) workflow’s jobs require higher GITHUB_TOKEN permissions than the caller job grants, by computing call-job permissions as a merged envelope.
Changes:
- Update
buildCallWorkflowJobsto compute call-job permissions from the caller + extracted worker permissions, and emit the merged permissions onto thecall-*job. - Update permission-extraction docs and expand tests to assert the new union behavior (including elevation when worker needs more than the caller declares).
- Regenerate
smoke-call-workflow.lock.ymlso the call job grants the worker-required permissions.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_safe_output_jobs.go | Merges caller + worker permission requirements to set call-job permissions for workflow_call jobs. |
| pkg/workflow/call_workflow_permissions.go | Updates doc comments to reflect permission extraction feeding merge behavior. |
| pkg/workflow/call_workflow_permissions_test.go | Updates and adds tests for union/elevation behavior for call-workflow job permissions. |
| .github/workflows/smoke-call-workflow.lock.yml | Regenerated lockfile reflecting elevated call-job permissions needed by the worker. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 4
| effectivePerms := callerPerms | ||
| if markdownPath != "" { | ||
| workerPerms, permErr := extractCallWorkflowPermissions(workflowName, markdownPath) | ||
| if permErr != nil { | ||
| // Non-fatal: log and continue. The worker file may not exist yet (it may be |
| // the caller job always grants at least what the worker requires, preventing GitHub from | ||
| // rejecting the run at startup when the worker requests a level higher than the caller granted. | ||
| // | ||
| // Returns nil when no workflow file is found or no permissions are declared. |
| var merged *Permissions | ||
| if callerPerms != nil { | ||
| merged = callerPerms.Clone() | ||
| } else { | ||
| merged = NewPermissions() | ||
| } | ||
| merged.Merge(workerPerms) | ||
| effectivePerms = merged |
There was a problem hiding this comment.
REQUEST_CHANGES — three issues must be addressed before merge: silent escalation destroys the audit trail, the error fallback silently reintroduces the exact startup_failure class this PR targets, and findUncoveredWorkerPermissions is now dead production code.
Blocking themes
Silent permission escalation removes the audit trail
The c.IncrementWarningCount() call and stderr warning were the only user-visible signal that a compiled lockfile grants more scope than the caller's markdown declares. Removing them means a worker update can silently widen every caller job's GITHUB_TOKEN with no indication to the author or security reviewer. The fix should keep the auto-elevation but restore an info-level message so the grant is auditable.
Error fallback bypasses the fix silently
When extractCallWorkflowPermissions fails (malformed worker YAML, path error), the call job falls back to caller-only permissions — the exact insufficient-permissions condition that caused startup_failure. This failure is logged internally but invisible to the user. A compiler warning (c.IncrementWarningCount()) is needed here so authors know auto-elevation was skipped.
findUncoveredWorkerPermissions is now dead production code
Removing the production call-site without removing the function leaves a function whose unit tests give false confidence that permission-coverage analysis is active during compilation. Either delete it (and its tests) or wire it back in as part of the audit-trail fix above.
🔎 Code quality review by PR Code Quality Reviewer · 103.2 AIC · ⌖ 10.5 AIC · ⊞ 5.2K
| // This is used to validate (not modify) the caller's permission envelope: callers | ||
| // control their own permission surface, and the compiler only warns when the declared | ||
| // permissions are insufficient for a worker the caller invokes. | ||
| func findUncoveredWorkerPermissions(caller, worker *Permissions) []string { |
There was a problem hiding this comment.
findUncoveredWorkerPermissions is dead production code — its only production call-site was removed in this PR, leaving a function exercised solely by its own unit tests.
💡 Details
The production call in compiler_safe_output_jobs.go was removed by this PR. The function now appears only in call_workflow_permissions_test.go, where tests validate the function's own internal logic — not any active compilation behavior.
Consequences:
- Future maintainers have no signal that this function is inert; there is no production call-site reference anywhere.
- Unit tests for this function create false confidence that permission-coverage analysis runs during compilation.
- This function can be deleted tomorrow and no production behavior changes.
Fix: Either remove the function and its tests, or explicitly document it as a utility kept for future use.
| } | ||
| } | ||
|
|
||
| if effectivePerms != nil { |
There was a problem hiding this comment.
Silent permission escalation removes the only user-visible audit signal — a worker update can now silently widen every caller job's GitHub token with no warning, no counter increment, and no feedback to the workflow author.
💡 Details
The old code emitted a stderr warning and called c.IncrementWarningCount() when the caller's declared permissions were insufficient. That was the mechanism by which authors could discover that their compiled lockfile was granting more scope than their source declared.
The new code silently elevates. An author who writes:
permissions:
contents: read
pull-requests: readgets a lockfile with issues: write, pull-requests: write, and actions: read — and receives zero indication this happened.
Why this matters:
- Worker-driven escalation: Any worker update that adds a new permission scope silently widens every caller job's GITHUB_TOKEN without the caller author's knowledge or review.
- Auditability: Security reviewers examining the markdown source see one permission envelope; the compiled artifact has a different, broader one.
- POLA: The caller's declared scope is no longer the effective scope.
Suggested fix: Keep the auto-elevation (it fixes the startup_failure) but restore the audit signal as an info-level message:
merged.Merge(workerPerms)
effectivePerms = merged
if elevated := findUncoveredWorkerPermissions(callerPerms, workerPerms); len(elevated) > 0 {
fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "info",
fmt.Sprintf("call-workflow target '%s' requires additional permissions (%s); auto-added to call-workflow job.",
workflowName, strings.Join(elevated, ", "))))
}This preserves the findUncoveredWorkerPermissions function (which would otherwise be dead code) and gives authors an audit trail.
| compilerSafeOutputJobsLog.Printf("Could not extract worker permissions for call-workflow job '%s' (validation skipped): %v", jobName, permErr) | ||
| // compiled in the same batch), in which case we fall back to the caller's | ||
| // own declared permissions. | ||
| compilerSafeOutputJobsLog.Printf("Could not extract worker permissions for call-workflow job '%s' (falling back to caller-only permissions): %v", jobName, permErr) |
There was a problem hiding this comment.
Error fallback silently reintroduces the startup_failure class — when worker permission extraction fails, the call job gets caller-only permissions and the PR's fix is bypassed with no user-visible signal.
💡 Details
When extractCallWorkflowPermissions returns a non-nil error, effectivePerms stays as callerPerms (the fallback on line 307). This is exactly the caller-insufficient-permissions scenario that caused the startup_failure this PR claims to fix. The failure is only emitted as a debug-level internal log — not surfaced to the user.
This can happen when:
- A worker
.lock.ymlfile is present but contains malformed YAML - A path resolution error occurs inside
findWorkflowFile - File permissions prevent reading the worker file
In all these cases, the caller job compiles silently with insufficient permissions, and the run fails at startup — the same class of failure the PR description says is now fixed.
Suggested fix: Surface the extraction failure as a compiler warning so the author knows the auto-elevation was skipped:
if permErr != nil {
fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning",
fmt.Sprintf("Could not extract worker permissions for '%s' (falling back to caller-only; startup_failure possible): %v",
workflowName, permErr)))
c.IncrementWarningCount()
}| // Validate (without modifying) that the caller's declared permissions cover what | ||
| // the worker requires. Emit a warning when they do not, so the user can widen the | ||
| // caller's `permissions:` block. This never alters the compiled permissions. | ||
| effectivePerms := callerPerms |
There was a problem hiding this comment.
effectivePerms aliases data.CachedPermissions without cloning in the non-merge path — any future mutation through this reference will silently corrupt the shared cache across loop iterations.
💡 Details
effectivePerms := callerPerms // direct alias; callerPerms may point to data.CachedPermissionsWhen workerPerms == nil or markdownPath == "", effectivePerms is never replaced with the cloned merged value, so it retains the alias. Today RenderToYAML() is read-only and there is no bug. But the pattern is a trap: any future effectivePerms.Set(...) or effectivePerms.Merge(...) call on the fallback path would silently mutate data.CachedPermissions, corrupting permissions for every subsequent workflow in the outer loop.
Note the merge path correctly clones: merged = callerPerms.Clone(). The fallback path should match that discipline:
var effectivePerms *Permissions
if callerPerms != nil {
effectivePerms = callerPerms.Clone()
}This makes ownership explicit and eliminates the shared-state aliasing risk entirely.
| // automatically promotes the call-workflow job's permissions to the union of both. | ||
| // This prevents GitHub's startup_failure when a reusable workflow job requests a | ||
| // permission level greater than the caller grants. | ||
| func TestBuildCallWorkflowJobs_WorkerPermissionsElevateCallerPermissions(t *testing.T) { |
There was a problem hiding this comment.
New test only covers the happy path — there is no test asserting that a permissions extraction error falls back to caller-only permissions without crashing or silently emitting empty permissions.
💡 Details
buildCallWorkflowJobs has a specific error-recovery branch: when extractCallWorkflowPermissions returns a non-nil error, it logs and continues with effectivePerms = callerPerms. This is new behavior (the fallback path changed semantics in this PR), but it has no test coverage.
A regression here could silently produce a call job with empty permissions (e.g., if effectivePerms were nil after the error) or panic on a nil dereference if callerPerms itself is nil after a parse failure.
Suggested addition: A test that writes a malformed .lock.yml for the worker and asserts:
buildCallWorkflowJobsreturns no error.- The generated call job carries the caller's declared permissions (not empty).
func TestBuildCallWorkflowJobs_WorkerExtractionError_FallsBackToCallerPerms(t *testing.T) {
// Write a malformed worker file so extraction fails
require.NoError(t, os.WriteFile(
filepath.Join(workflowsDir, "worker-bad.lock.yml"),
[]byte("not: valid: yaml: [::"), 0644))
// ... build jobs, assert caller permissions are set, no panic
}
|
@copilot run pr-finisher skill |
…style Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran the pr-finisher skill and addressed all four review threads from the automated reviewer in commit 852c819:
Local validation ( |
|
@copilot please summarize the remaining blockers and post the next concrete step.
|
Smoke Call Workflowhas been 100%startup_failuresince 2026-06-23. GitHub rejects aworkflow_callinvocation at startup when any worker job requests a permission level greater than what the calling job grants — the compiler was only setting the caller's declared permissions (contents: read,pull-requests: read) on thecall-smoke-workflow-calljob, while the worker requiredissues: writeandpull-requests: write.Changes
Compiler (
pkg/workflow/compiler_safe_output_jobs.go)Tests (
pkg/workflow/call_workflow_permissions_test.go)TestBuildCallWorkflowJobs_WorkerPermissionsElevateCallerPermissions— directly covers the scenario where the caller declares less than the worker needs.Comments (
pkg/workflow/call_workflow_permissions.go)extractCallWorkflowPermissionsdoc to reflect it now feeds the merge, not just validation.findUncoveredWorkerPermissions.Run: https://github.com/github/gh-aw/actions/runs/28156255243