Repository navigation
Fix Windows Grower reusable workflow permissions - #70020
Conversation
Grant the Windows probe caller exactly the callee's job-permission union and add regression coverage preserving sibling-agent permissions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
Non-blocking, but the new regression test has a blind spot around inherited callee permissions.
Theme
The review found one medium-signal maintainability issue: the test computes the reusable caller grant set from per-job permission blocks only, so a future refactor that moves shared scopes to the callee workflow-level `permissions` block can make this check pass while the caller is still missing required scopes.🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 45.2 AIC · ⌖ 7.95 AIC · ⊞ 12.7K
Comment /review to run again
|
|
||
| required := NewPermissions() | ||
| for _, job := range compilers["windows"].jobManager.GetAllJobs() { | ||
| required.Merge(NewPermissionsParser(job.Permissions).ToPermissions()) |
There was a problem hiding this comment.
This test only unions explicit job.Permissions, so it will miss scopes inherited from the callee's workflow-level permissions block and can still pass while windows_probe is under-privileged.
💡 Why this matters
Ifwindows.lock.yml ever moves shared scopes to the reusable workflow's top-level permissions, job.Permissions will be empty for those jobs and this loop will undercount the caller grant set. That leaves you with a false-green regression test for the exact failure this PR is trying to prevent.
Consider either asserting that the callee workflow-level permissions stay permissions: {} or merging that workflow-level baseline into each job before building required, so future refactors can't silently weaken the check.
There was a problem hiding this comment.
🔵 Needs a closer look
The reported Go-lint publication gate remains incomplete, requiring final human review.
No open issues 🎉
What changed in this PR
Fixes Windows Grower’s reusable workflow permissions without broadening workflow-level or sibling-agent permissions.
Changes:
- Grant
windows_probethe callee’s exact permission union and regenerate the lock. - Add regression coverage for permissions, rendering, dependencies, and secret inheritance.
| File | Description |
|---|---|
pkg/workflow/repository_workflows_test.go |
Adds caller/callee permission regression coverage. |
.github/workflows/windows-grower.md |
Grants permissions only to windows_probe. |
.github/workflows/windows-grower.lock.yml |
Reflects the job-scoped permissions in generated YAML. |
Review effort: Max
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs to this permission-startup-failure fix.
The fix itself is correct and well-tested: I reverted the working tree to the pre-fix state and reran TestWindowsGrowerProbePermissions, confirming it fails without the permissions: block on windows_probe and passes with it. The regression test also recomputes the required union from the compiled windows workflow (not a hardcoded copy), so it will catch future drift if windows.md gains new job permissions.
📋 Key Themes & Highlights
Key Theme
- Root cause vs. symptom: the PR fixes
windows-growerspecifically by hand-writing the permission union, but the compiler already has machinery (addCallWorkflowPermissions/extractJobPermissionsFromParsedWorkflow) that computes this automatically for safe-outputscall-*jobs. Customjobs.<name>.uses:entries (likewindows_probe) don't get that same automatic treatment, so every future reusable-workflow caller in the repo is exposed to the same class of bug. See inline comment for a concrete follow-up suggestion.
Positive Highlights
- ✅ Minimal, surgical change — only grants what the callee's jobs actually need, no blanket broadening
- ✅ Regression test verifies the agent job's own permissions are untouched (no scope creep into the OpenAI-backed grower agent)
- ✅ Test recomputes the expected union from the compiled callee rather than hardcoding expected values twice
No blocking issues — approving in spirit, filing one actionable architectural suggestion inline for a possible compiler-level fix.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 93.1 AIC · ⌖ 14.9 AIC · ⊞ 10.1K
Comment /matt to run again
| copilot-requests: write | ||
| discussions: write | ||
| issues: write | ||
| pull-requests: write |
There was a problem hiding this comment.
[/diagnosing-bugs] This fix addresses the symptom for windows-grower only — the hardcoded permission union will silently drift if windows.md later adds a job needing a new scope, reproducing the exact startup_failure this PR fixes. The compiler already has addCallWorkflowPermissions/extractJobPermissionsFromParsedWorkflow (pkg/workflow/compiler_safe_output_jobs.go, call_workflow_permissions.go) that compute this union automatically for safe-outputs call-* jobs, but configureCustomReusableWorkflow (compiler_custom_job_execution.go) never calls it for custom jobs.<name>.uses: entries like this one.
💡 Suggested follow-up
The regression test does catch drift today (it recomputes the union from the compiled windows workflow and diffs it against the static string in windows-grower.md), so this isn't silently broken — but it relies on every future editor remembering to run/update that test rather than the compiler enforcing it. Consider a follow-up that wires addCallWorkflowPermissions (or equivalent) into configureCustomReusableWorkflow, so any jobs.<name>.uses: reusable-workflow call gets the same automatic permission union that call-* safe-output jobs already get — removing the need for hand-maintained permission lists entirely.
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
Why
Windows Grower fails before any jobs start because its reusable
windows_probecall inherits the workflow-levelpermissions: {}default. The Windows callee requests permissions that the caller has not granted, as reported in run 39114886057.Change
Grant only
windows_probethe exact union of the current callee's job permissions. Keep the workflow-level deny-by-default policy, sibling-agent permissions, activation dependency, and existing secret inheritance unchanged. Regenerate the lock and add a regression that compiles the actual caller/callee, checks the exact permission union and rendered caller block, and verifies the grower agent retains its original permissions.No compiler-wide permission inference, approval bypass, or workflow/job renaming is introduced.
Validation
Passed:
make buildandmake fmtmake recompile: 345 of 345 workflows compiledgh aw compile windows-grower --dry-run: zero warnings; shellcheck passedgo test ./pkg/workflow -run '^TestWindowsGrowerProbePermissions$' -count=1Known limitation:
make agent-report-progressdid not complete its Go-lint group. Installed golangci-lint v2.14.0 stalled beyond its configured timeout; isolating its temporary directory/cache removed a parallel-runner lock error but did not resolve the stall. Checksum-verified repository-pinned v2.13.2 also exceeded a hard 360-second deadline. No linter rules were weakened. This draft is published at the user's explicit request with that gate still unresolved.No workflow was dispatched or rerun, and repository settings and approval protections were not changed.