Allow PR Sous Chef to approve CJS and CGO action-required runs - #53679
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| max: 8 | ||
| allowed-workflows: [cjs.yml, cgo.yml] | ||
| allowed-pull-requests: ${{ steps.fetch-prs.outputs.eligible_pull_request_numbers }} | ||
| fork: true |
There was a problem hiding this comment.
Addressed in 7aafd4a by setting approve-workflow-run to fork: false and recompiling the workflow lock file.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. This PR only modifies workflow files (.github/workflows/pr-sous-chef.lock.yml and .github/workflows/pr-sous-chef.md). Test Quality Sentinel skipped.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #53679 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Request changes
This still introduces an unsafe auto-approval path for forked workflow runs.
Blocking theme
The new approve-workflow-run config is explicitly enabled with fork: true, which means PR Sous Chef can waive GitHub's manual approval gate for workflows originating from forks. Limiting approvals to cjs.yml and cgo.yml is not enough, because those workflows still execute the PR's code after approval.
That is a security boundary change, not just a convenience tweak, and it needs to be removed or replaced with a stronger trust policy before this can merge.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 11.1 AIC · ⌖ 6.88 AIC · ⊞ 4.5K
Comment /review to run again
| target: "*" | ||
| github-token: ${{ secrets.AWI_MAINTENANCE_TOKEN || secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }} | ||
| approve-workflow-run: | ||
| max: 8 |
There was a problem hiding this comment.
This enables fork approvals for workflow runs even though the existing review already called out fork false!, so the workflow is still authorizing execution of untrusted fork code rather than closing that risk.
💡 Why this blocks merge
approve-workflow-run is the mechanism that explicitly lifts GitHub's approval gate for fork-originated workflow runs. Setting fork: true means PR Sous Chef can now approve those runs automatically, which is exactly the high-risk path the review feedback was trying to stop. Restricting by workflow filename is not a meaningful safety boundary here: cjs.yml and cgo.yml still execute repository code from the PR branch after approval.
Please remove fork approval entirely or gate this behind a much stronger trust decision than "workflow name matches". At minimum, this needs to stay off for forked PRs until there is a reviewed policy proving those workflows are safe to auto-approve.
There was a problem hiding this comment.
The changes are well-scoped and internally consistent.
approve_workflow_runis narrowly gated tocjs.yml/cgo.yml, fork PRs are handled viafork: true, andprotected_files+allowed_pull_requests(dynamically scoped to eligible PRs from the prefilter) prevent over-approval.- The WAITING reclassification is coherent: WAITING no longer blocks the nudge timer (removed from the pending list) but surfaces as
ACTION_REQUIREDin failed-checks so the agent knows to act on it. actions: writepermission is correctly added in both the compile and handler jobs.
No blocking issues found. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 24.4 AIC · ⌖ 8.79 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design — requesting changes on two issues.
📋 Key Themes & Highlights
Issues
-
fork: trueonapprove-workflow-run(line 273) — already flagged in thread. Approving workflows from fork PRs defeats the "action required" security gate. Should befork: false. -
Missing run ID discovery guidance (line 366) — Step 0 tells the agent to call
approve_workflow_run --run_id <RUN_ID>but never explains how to obtain that ID. The agent must querygh run listfiltering bypathandheadBranchto resolve it.
Positive Highlights
- ✅ Clean eligibility guard:
allowed-pull-requestsscoped to the prefilter output prevents approvals outside the eligible set. - ✅ Graceful failure path:
approve_workflow_run_failedskip reason keeps the per-PR loop going rather than aborting the run. - ✅ Correct skip condition for the nudge comment after a clean approval-only run.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 37.5 AIC · ⌖ 10 AIC · ⊞ 7.8K
Comment /matt to run again
|
|
||
| 0. **Run formatters and push if needed** | ||
| 0. **Approve allowed action-required workflow runs** | ||
| - If the compact JSON `failed_checks` list contains `ACTION_REQUIRED`, inspect the PR's waiting workflow runs and call `safeoutputs approve_workflow_run --run_id <RUN_ID>` only for matching CJS/CGO runs. |
There was a problem hiding this comment.
[/diagnosing-bugs] Step 0 instructs the agent to call approve_workflow_run --run_id <RUN_ID> but gives no guidance on how to discover that run ID — the agent will likely stall or produce an incorrect approval.
💡 Suggested addition
Add an explicit lookup step, for example:
- To find the run IDs, query:
`gh run list --json databaseId,path,status,headBranch --jq '[.[] | select(.status == "action_required")]'`
Filter by `path` matching `.github/workflows/cjs.yml` or `.github/workflows/cgo.yml` and `headBranch` matching the PR's head branch.Without this, the agent has no deterministic path to resolve <RUN_ID> and may either silently fail or approve the wrong run.
@copilot please address this.
| max: 8 | ||
| allowed-workflows: [cjs.yml, cgo.yml] | ||
| allowed-pull-requests: ${{ steps.fetch-prs.outputs.eligible_pull_request_numbers }} | ||
| fork: true |
There was a problem hiding this comment.
[/codebase-design] fork: true is set on approve-workflow-run, meaning the safe-output handler will approve workflows from fork PRs. This is flagged by the existing review comment on this line — it is almost certainly incorrect, as approving fork-originated workflow runs defeats the security purpose of the "action required" gate.
💡 Why this matters
The "action required" status exists specifically to prevent untrusted fork code from running with repository secrets. Setting fork: true bypasses that protection. For PRs from forks, workflow approval should remain manual.
Change to fork: false (or remove the field, since false is the safer default).
@copilot please address this.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Enables PR Sous Chef to approve eligible CJS/CGO workflow runs awaiting approval.
Changes:
- Treats
WAITINGchecks as agent-inspectable. - Adds scoped
approve-workflow-runsafe output. - Reports successful workflow approvals.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/pr-sous-chef.md |
Adds approval eligibility, configuration, and agent instructions. |
.github/workflows/pr-sous-chef.lock.yml |
Regenerates the compiled workflow. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
| approve-workflow-run: | ||
| max: 8 | ||
| allowed-workflows: [cjs.yml, cgo.yml] | ||
| allowed-pull-requests: ${{ steps.fetch-prs.outputs.eligible_pull_request_numbers }} |
| - Otherwise, attempt `update_pull_request` with `update_branch: true` and a minimal append body marker including `pr-sous-chef` and the run URL. | ||
|
|
||
| 2. **Post exactly one combined nudge comment** (at most ONE `add_comment` per PR per run) | ||
| 3. **Post exactly one combined nudge comment** (at most ONE `add_comment` per PR per run) |
|
@copilot Please address the blocking review feedback on this PR, then run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in
Validation: Stale/addressed review threads that can be resolved or dismissed: the fork-approval threads, missing run-ID guidance thread, safe-output allowlist propagation thread, and unconditional nudge thread. |
|
🎉 This pull request is included in a new release. Release: |
PR Sous Chef skipped PRs blocked only by approval-gated workflows. It now allows eligible PRs through when the pending workflow can be safely approved, limited to CJS and CGO.
Safe output
approve-workflow-runto PR Sous Chef.cjs.ymlandcgo.yml.Eligibility
WAITINGcheck runs eligible for agent inspection.Agent behavior