Fix AI credits throughput rate-limit errors being silently swallowed - #49360
Conversation
…ately from budget exhaustion Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49360 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Pull request overview
Fixes under-budget Copilot API throughput throttles so they produce actionable AI credits errors.
Changes:
- Reports all detected AI credits rate-limit signals.
- Selects separate budget-exhaustion and throughput-throttle templates.
- Adds regression coverage for under-budget throttling.
Show a summary per file
| File | Description |
|---|---|
actions/setup/md/ai_credits_rate_limit_throttle.md |
Adds throughput-throttle guidance. |
actions/setup/js/handle_agent_failure.cjs |
Selects the appropriate rate-limit template. |
actions/setup/js/handle_agent_failure_max_ai_credits_exceeded.test.cjs |
Updates budget-exhaustion tests. |
actions/setup/js/handle_agent_failure_ai_credits_rate_limit.test.cjs |
Tests throughput rendering. |
actions/setup/js/ai_credits_context.test.cjs |
Covers under-budget signals. |
actions/setup/js/ai_credits_context.cjs |
Reports rate-limit signals unconditionally. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
| > [!WARNING] | ||
| > **AI Credits Rate Limit** | ||
| > | ||
| > The Copilot API returned a rate limit response (HTTP 429) before the workflow's AI credits budget was exhausted.{metrics_summary} |
| function shouldReportAICreditsRateLimitError(hasRateLimitSignal) { | ||
| return hasRateLimitSignal; |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
The fix is correct and well-tested.
shouldReportAICreditsRateLimitErrorsimplification is sound — removing theaiCredits >= maxAICreditsguard means any genuine 429 signal is now surfaced regardless of budget position.- The
isBudgetExceededdiscriminator at both call sites correctly usesmaxAICreditsExceededto select the right template. - The new
ai_credits_rate_limit_throttle.mdtemplate appropriately omits the "Increase the limit" guidance that only applies to budget exhaustion. - Tests cover both the under-budget throttle scenario and the existing budget-exceeded paths.
One minor observation: suggestedCredits is computed even when isBudgetExceeded=false (throttle path), though it is unused in that template. The code comment acknowledges this as harmless — no action needed.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14.9 AIC · ⊞ 5.3K
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (4 tests)
i️ Inflation Note
Verdict
|
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Root cause addressed cleanly: the faulty
aiCredits >= maxAICreditsgate was doing double-duty as a budget check when it should only gate on the presence of the signal itself. The fix is correct and minimal. - Good dual-template split: separating throughput vs. budget-exhaustion messages prevents confusing "Increase the limit" advice from appearing in the wrong context.
- Minor gaps:
suggestedCreditsis still computed when unused (throughput path), and the new throttle template has an edge-case wording issue when no budget is configured. Both are low risk.
Positive Highlights
- ✅ Excellent regression test added at the exact scenario from the failing run (236/1000 AIC)
- ✅ Both call sites in
main()updated consistently — no risk of one path lagging behind - ✅ Existing tests updated to be explicit about
isBudgetExceeded=trueintent (see the comment added inhandle_agent_failure_max_ai_credits_exceeded.test.cjs) - ✅ Simplification of
shouldReportAICreditsRateLimitErroris welcome — the function is now a pure signal passthrough, removing hidden coupling to credit accounting
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 37 AIC · ⊞ 7K
Comment /matt to run again
| throw new Error(`failed to resolve template path for ${templateName} (${getErrorMessage(error)}); ensure RUNNER_TEMP or GH_AW_PROMPTS_DIR is set and the template file exists`, { cause: error }); | ||
| } | ||
|
|
||
| // Suggest a new limit: 2x current max, or 2x actual usage if max is unknown, or a reasonable default |
There was a problem hiding this comment.
[/diagnosing-bugs] suggestedCredits is computed unconditionally but is only consumed by the budget-exceeded template — the new throughput template does not use it. This is harmless today, but obscures the intent and could mislead future maintainers.
💡 Suggested fix
Guard the computation inside the budget-exceeded branch:
const suggestedCredits = isBudgetExceeded
? (baseForSuggestion > 0 ? Math.ceil(baseForSuggestion * 2) : 2000)
: undefined;This makes it explicit that suggestedCredits belongs to the budget-exceeded path only.
@copilot please address this.
|
|
||
| expect(rendered).toContain("AI Credits Rate Limit"); | ||
| expect(rendered).not.toContain("AI Credits Budget Exceeded"); | ||
| // inline metrics show usage without an overage |
There was a problem hiding this comment.
[/tdd] The test asserts toContain("Used \236.1` of `1K` max")— this implicitly tests the rounding/formatting logic that lives inhandle_agent_failure.cjs. A follow-on test covering the edge case where both aiCreditsandmaxAICredits` are empty strings (under the throughput path) would close the coverage gap left by the analogous budget-exceeded test at line 53 of the other test file.
💡 Suggested test
it("shows throughput message without metrics when no credit data is available", () => {
const rendered = buildAICreditsRateLimitErrorContext(true, "", "", "", false);
expect(rendered).toContain("AI Credits Rate Limit");
expect(rendered).not.toContain("Used");
});@copilot please address this.
| @@ -0,0 +1,14 @@ | |||
| > [!WARNING] | |||
| > **AI Credits Rate Limit** | |||
| > | |||
There was a problem hiding this comment.
[/diagnosing-bugs] The template text says "before the workflow's AI credits budget was exhausted" — this is slightly misleading when maxAICredits is not configured (i.e. there is no budget). Consider phrasing it as "before the configured AI credits budget was reached (or no budget was set)" to handle the zero-budget case clearly.
@copilot please address this.
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the follow-up review findings in |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please run the Unresolved review threads remain, newest first:
The latest branch update is requested via GitHub. Please confirm the branch is current, address the remaining review feedback, and then run the
|
Verified the branch is current with |
🤖 PR TriageCategory: bug | Risk: medium | Score: 78/100 (impact 35, urgency 25, quality 18) Recommended action: Real bug fix — a genuine 429 throughput throttle was being silently swallowed and surfaced as a generic engine-terminated error. Non-draft, 3 automated approvals, CI in progress. High-value fix; worth expediting human review once CI completes.
|
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please run the Unresolved review threads remain, newest first:
The latest branch update was requested via GitHub. Please confirm the branch is current, address the remaining review feedback, and then run the
|
Confirmed the branch is still current with |
|
🎉 This pull request is included in a new release. Release: |
shouldReportAICreditsRateLimitErrorgated onaiCredits >= maxAICredits, so a real Copilot API 429 throughput throttle (credits below budget) was suppressed and fell through to the generic "engine terminated unexpectedly" path instead of surfacing a meaningful error.Changes
ai_credits_context.cjs— Removed theaiCredits >= maxAICreditsguard;shouldReportAICreditsRateLimitErrornow returnshasRateLimitSignalunconditionally.handle_agent_failure.cjs— AddedisBudgetExceededparam tobuildAICreditsRateLimitErrorContextto select the correct template at both call sites. Whenfalse, renders the new throughput throttle template; whentrue, renders the existing budget-exhaustion template.ai_credits_rate_limit_throttle.md(new) — Separate error template for throughput 429s. Omits the "Increase the limit" guidance that only applies to budget exhaustion.Tests — Updated existing tests to pass
isBudgetExceeded=true; added new tests covering the under-budget throughput throttle case.Before / After
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Run: https://github.com/github/gh-aw/actions/runs/30660114445