Fail fast on approve_workflow_run permission denials to stop safe-outputs API storm - #55957
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
approve_workflow_run permission denials to stop safe-outputs API storm
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR #55957 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This change turns the first approval permission failure into a global skip for the rest of the handler instance. That avoids the API storm, but it also drops all later approval messages without even checking whether they target a different repository/token context where approval would succeed.
Blocking theme
The new approvePermissionDenied latch is process-wide state for the handler, not state scoped to the repository/token/run that actually failed. After one Resource not accessible response, every subsequent approve_workflow_run message is rejected before any per-run validation or API read happens. If a single safe-outputs pass can process approvals across multiple installations/repos/tokens, one transiently unsupported approval poisons unrelated approvals and silently leaves valid runs unapproved.
That needs to be scoped more narrowly (for example per token/repo capability) or derived from configuration up front instead of short-circuiting all later messages.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 4.16 AIC · ⌖ 6.87 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design — commenting with a few targeted suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Error matching precision:
isPermissionDeniedErrormatches on a partial substring; tightening it reduces the risk of false positives or silent misses if GitHub changes the error wording. - Closure lifecycle:
approvePermissionDeniedflag lifecycle is correct for the intended one-call-per-pass contract but is undocumented, which could confuse future maintainers. processedCountordering: decrement-before-guard pattern is subtly fragile.
Positive Highlights
- ✅ Fail-fast guard is a clean, simple closure — no shared global state.
- ✅ Tests cover both the initial denial and the short-circuit path.
- ✅
reasonCodegives callers a stable string to key on. - ✅
skipped: true(non-fatal) is the right semantic for a permission denial.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 26.2 AIC · ⌖ 10.1 AIC · ⊞ 7.6K
Comment /matt to run again
There was a problem hiding this comment.
Review: Fail fast on approve_workflow_run permission denials
The approach is sound — a closure-scoped flag (approvePermissionDenied) short-circuits all subsequent calls after a permission error, preventing an API storm. Two issues worth addressing:
Blocking
processedCount off-by-one on permission denial
In the catch block, processedCount-- fires before the permission check. When the denial path is taken, the count is decremented even though the run was skipped (not failed processing). If maxCount depends on this counter, downstream runs may be over-processed.
Non-blocking
isPermissionDeniedError string matching is narrow
Only "resource not accessible" is matched. Octokit errors for 403 responses include other messages ("Must have admin rights", plain 403 status). Checking error.status === 403 as a fallback would make the guard more resilient.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 22.2 AIC · ⌖ 9.31 AIC · ⊞ 6.2K
Comments that could not be inline-anchored
actions/setup/js/approve_workflow_run.cjs:17
[harden] Narrow permission-error detection may miss other 403 variants
The current check only matches "resource not accessible". GitHub also returns permission-related errors that don't contain this string, for example "Must have admin rights to Repository." or a raw 403 HTTP status code on the Octokit error object.
A more robust guard would also inspect the HTTP status if available:
function isPermissionDeniedError(message, status) {
const normalized = String(message || ""…
</details>
<details><summary>actions/setup/js/approve_workflow_run.cjs:396</summary>
**[harden] `processedCount--` is decremented before the permission check, leaving the count incorrect**
In the catch block at line ~378–396:
```js
} catch (error) {
processedCount--; // decremented here
const errorMessage = getErrorMessage(error);
if (isPermissionDeniedError(errorMessage)) {
approvePermissionDenied = true;
...
return { success: false, skipped: true, ... };
}
throw error;
}When a permission denial is caught, processedCount h…
There was a problem hiding this comment.
Pull request overview
Adds handler-level circuit breaking for workflow-run approval permission failures.
Changes:
- Detects permission-denied approval errors.
- Skips subsequent approval attempts in the same pass.
- Adds focused behavioral tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/approve_workflow_run.cjs |
Adds permission classification and fail-fast state. |
actions/setup/js/approve_workflow_run.test.cjs |
Tests skip and short-circuit behavior. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
|
Please do one focused follow-up pass:
Run: https://github.com/github/gh-aw/actions/runs/32943696113
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Pushed follow-up commit |
|
Triage update: Category: bug | Risk: high | Score: 64/100 (impact 36 + urgency 18 + quality 10) | Action: fast_track | Batch: workflow-runtime
|
|
Triage update: Category: bug | Risk: high | Score: 64/100 (impact 36 + urgency 18 + quality 10) | Action: fast_track | Batch: workflow-runtime
|
|
$ Please do one focused follow-up pass:
Run: https://github.com/github/gh-aw/actions/runs/32972030991
|
…ix-approve-workflow-run Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Completed the focused PR-finisher pass and pushed branch refresh commit |
PR Sous Chefbegan attemptingapprove_workflow_runon fork PR runs with a token that lacks approval permissions, producingResource not accessible by personal access tokenand repeatedly burning API quota. This change makes permission-class approval failures non-fatal and short-circuits subsequent approval attempts in the same safe-outputs pass.Approval error classification (handler-level)
resource not accessible).skippedwith a dedicated reason code:APPROVE_WORKFLOW_RUN_PERMISSION_DENIED.Fail-fast guard to prevent repeated expensive attempts
approvePermissionDenied).approve_workflow_runmessages return immediately as skipped (no additional API approval attempts).Behavioral contract coverage