Repository navigation
Clarify GitHub API quota failures and reset diagnostics - #68737
Conversation
Distinguish exhausted and reserved API quota from authentication failures using failed-response metadata. Keep daily accounting fail-closed and review dismissal actor validation unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ 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.
|
|
🧠 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.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
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.
Request changes
The new diagnostics are moving in the right direction, but two regressions here still break the failure reporting this patch is trying to improve: wrapped artifact quota failures never pick up the new diagnostic, and plain permission 403s can be mislabeled as quota exhaustion when x-ratelimit-remaining happens to be zero.
Blocking themes
- Wrapped
artifact_clientfailures are not unwrapped before deriving diagnostics, so restore and guardrail errors still losequota_exhaustedandretry_not_beforecontext on artifact redirect and download paths. remaining === 0is being treated as a sufficient signal for rate limiting on every 403, which overwrites genuine permission failures with the wrong classification.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 40.9 AIC · ⌖ 5.8 AIC · ⊞ 21.3K
Comment /review to run again
There was a problem hiding this comment.
🟡 Changes recommended
Malformed Retry-After values can be accepted, and some restore failures omit the promised step summary.
2 open findings
What changed in this PR
Adds secret-safe GitHub API quota diagnostics to distinguish quota, authentication, permission, and rate-limit failures.
Changes:
- Introduces validated quota metadata and failure classifications.
- Adds restore summaries and review-fetch diagnostics.
- Expands documentation and test coverage.
| File | Description |
|---|---|
docs/src/content/docs/reference/rate-limiting-controls.md |
Documents quota diagnostics. |
actions/setup/js/restore_aic_scan_cache.cjs |
Reports restore failures. |
actions/setup/js/github_api_diagnostics.cjs |
Implements diagnostic parsing. |
actions/setup/js/github_api_diagnostics.test.cjs |
Tests diagnostic behavior. |
actions/setup/js/dismiss_pull_request_review.cjs |
Adds review-fetch diagnostics. |
actions/setup/js/dismiss_pull_request_review.test.cjs |
Tests review failure classifications. |
actions/setup/js/daily_aic_scan.test.cjs |
Tests restore summaries. |
actions/setup/js/daily_aic_api_budget.cjs |
Integrates quota metadata. |
actions/setup/js/daily_aic_api_budget.test.cjs |
Tests quota handling. |
actions/setup/js/check_daily_aic_workflow_guardrail.cjs |
Adds guardrail diagnostics. |
actions/setup/js/check_daily_aic_workflow_guardrail.test.cjs |
Tests guardrail messages. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs on the new github_api_diagnostics.cjs module. Requesting changes: a real classification bug where the exact scenario this PR aims to clarify (a protective-reserve stop on HTTP 403 with non-zero remaining quota) gets mislabeled permission_denied instead of quota_reserve, and the gap isn't covered by the new tests.
📋 Key Themes & Highlights
Key Themes
- Classification precedence bug: in
getGitHubAPIDiagnostic, thestatus === 403 && !limited ? "permission_denied"branch runs before thequota ? ... : quotaStopped ...branch is reached, so aDAILY_AIC_QUOTA_STOPfailure at HTTP 403 with non-zero remaining is reported aspermission_deniedeven thoughquota: true. This directly undermines the PR's goal of distinguishing reserve stops from permission failures — see inline comment onactions/setup/js/github_api_diagnostics.cjs:46. - Test coverage gap: existing tests for the
quotaStopped+permission_deniedinteraction only usex-ratelimit-remaining: "100"with status 401/403 and no response headers set beyond that in one case, but don't specifically assert the 403+non-zero-remaining+quota_stop path producesquota_reserve. The triage above is the regression test that should be added.
Positive Highlights
- ✅ Careful secret-safety discipline throughout: header values are strictly validated (regex +
Number.isSafeInteger), and tests explicitly assert that raw payloads/URLs/auth headers never leak into diagnostic messages. - ✅ Good separation of concerns:
getGitHubAPIQuota(pure header parsing) vsgetGitHubAPIDiagnostic(classification) is a clean, testable split. - ✅ Solid regression coverage for malformed/overflow header values (
Infinity,1e300,9999999999999999, etc.) and for retry/reset deadline precedence. - ✅ Consistent, minimal integration across
daily_aic_api_budget.cjs,check_daily_aic_workflow_guardrail.cjs,restore_aic_scan_cache.cjs, anddismiss_pull_request_review.cjs— each call site reuses the shared helper rather than reimplementing classification logic.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 152.9 AIC · ⌖ 14.9 AIC · ⊞ 10.4K
Comment /matt to run again
Address review feedback with bounded cause traversal, sanitized access-failure evidence, HTTP-date Retry-After validation, and summaries for every attributed restore read failure. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Why
Installation API quota failures currently obscure which request failed and can be confused with authentication failures or daily AI Credits overruns. Failure Investigator stopped on its first business API request with 0 of 15,000 requests remaining; PR Sous Chef failed fetching a review despite a separate client's healthy quota precheck. Neither observation establishes avoidable excessive consumption by the failed operation.
Approach
Credentials, actor-bound review validation, daily credit accounting, quota reserves, and retry bounds remain unchanged. No workflow sources, generated locks, or PR Sous Chef prefilter credential bindings changed. Shared installation quota remains an infrastructure limit; reset timestamps do not guarantee available capacity or schedule automatic retries.
Validation
make agent-report-progresspassed build, typecheck, scoped lint, and 247 impacted JavaScript tests.make fmt-cjsandmake lint-cjspassed. Seven pre-existing warnings on untouched lines remain; the new diagnostic helper is warning-free.