Handle GitHub reaction rate limits gracefully - #58610
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
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. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the implementation label and has ≤100 new lines of code in business logic directories.
|
|
❌ Ponytail Reviewer failed. Please review the logs for details. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review. Completed PR review with one blocking comment and submitted overall review.
|
![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 — small, well-scoped fix for GitHub reaction rate-limit 403s, mirrored across both reaction paths with matching regression tests.
📋 Key Themes & Highlights
Key Themes
- Scope question: in
add_reaction_and_edit_comment.cjs, theisRateLimitErrorcheck sits in the sharedtry/catchcovering both the reaction call and the subsequent comment-edit call, so a rate-limit-shaped failure from the comment-edit step would also be downgraded to a warning — unlikeadd_reaction.cjswhere the check is scoped only to the reaction handler. Worth confirming this is intentional.
Positive Highlights
- ✅ Reuses the existing
isLockedErrorpattern consistently for the newisRateLimitErrorcheck - ✅ Regression tests added for both call sites, asserting
core.warningfires andcore.setFaileddoes not - ✅
error_helpers.cjscentralizes the rate-limit detection logic with clear docstring
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 22.7 AIC · ⌖ 15.6 AIC · ⊞ 10.3K
Comment /matt to run again
| return; | ||
| } | ||
| const errorMessage = getErrorMessage(error); | ||
| if (isRateLimitError(error)) { |
There was a problem hiding this comment.
[/codebase-design] This isRateLimitError check sits in the outer try/catch that wraps both the reaction call and the subsequent addCommentWithWorkflowLink call. A rate-limited comment-edit failure will now also be silently downgraded to a warning, not just a rate-limited reaction — is that the intended scope, or should the rate-limit check be scoped to the reaction step only (mirroring add_reaction.cjs, where it only guards handleReactionError)?
💡 Why it matters
If a genuinely important comment-edit failure (e.g. workflow-run link update) starts throwing a rate-limit-shaped error message, it would now fail silently with only a warning instead of setFailed, which could mask a real issue for maintainers monitoring workflow health. The existing test only exercises the reaction-request path (mockGithub.request.mockRejectedValueOnce), so this broader catch scope is untested.
@copilot please address this.
There was a problem hiding this comment.
🟡 Changes recommended
The classifier misses common status-, header-, and secondary-rate-limit response shapes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Handles reaction API rate-limit failures without blocking workflow activation.
Changes:
- Adds non-fatal warning handling for recognized rate limits.
- Preserves locked-resource and unrelated-error behavior.
- Adds regression tests for installation rate limits.
File summaries
| File | Description |
|---|---|
actions/setup/js/add_reaction.cjs |
Handles recognized reaction rate limits. |
actions/setup/js/add_reaction.test.cjs |
Tests standalone rate-limit handling. |
actions/setup/js/add_reaction_and_edit_comment.cjs |
Handles recognized combined-path rate limits. |
actions/setup/js/add_reaction_and_edit_comment.test.cjs |
Tests combined-path rate-limit handling. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
| return; | ||
| } | ||
| const errorMessage = getErrorMessage(error); | ||
| if (isRateLimitError(error)) { |
| return; | ||
| } | ||
| const errorMessage = getErrorMessage(error); | ||
| if (isRateLimitError(error)) { |
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
The new fail-open path for reaction writes is reasonable, but this PR still needs a tighter contract around what gets downgraded to a warning. Right now the changed code relies entirely on message text matching for rate-limit detection, so any future GitHub error shape drift here turns into either a silent false negative or a silently swallowed real failure.
Blocking theme
The implementation only proves the happy-path wording for one installation rate-limit message. These scripts are handling workflow activation side effects, so the downgrade path needs tests that lock down the exact boundary between transient throttling and real 403/API failures across both REST and GraphQL call sites.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 17.9 AIC · ⌖ 7.41 AIC · ⊞ 23.5K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/add_reaction.cjs:18
This downgrade path is only guarded by free-form message matching, so a GitHub error wording change can flip a real API failure into a warning or start failing genuine throttling again without any test catching it.
<details><summary>💡 Tighten the contract</summary>
The new behavior is intentionally fail-open, which makes boundary coverage more important than before. Please add table-driven coverage around isRateLimitError for the concrete response shapes you expect from both REST and Graph…
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
@copilot Please take the next forward-progress pass on this PR.
|
GitHub reaction requests can return
403when API rate limits are exhausted. These transient failures currently fail workflow activation.Changes