Repository navigation
fix: bound daily AIC quota recovery and snapshot discovery - #70968
Conversation
Restart complete accounting phases once after a bounded server quota reset while retaining attempt-1 identity and fail-closed authorization. Limit optional snapshot discovery without weakening authoritative window accounting. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ 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.
|
|
✅ 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.
|
|
✅ PR Code Quality Reviewer completed the code quality 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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
No actionable blocking issues found in the changed lines. The quota recovery, bounded snapshot discovery, and cache-attempt validation changes are internally consistent; the advisory findings about budget reuse and snapshot candidate breadth do not establish defects from the inspected implementation.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · 2.51 AIC · ⌖ 0.55 AIC · ⊞ 1.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design (pr-triage was not invoked directly; I applied the same bug-fix/hardening classification heuristic manually against the pre-fetched diff — noting this per the fallback rules). One minor cleanup suggestion posted inline; otherwise this is solid, well-evidenced, well-tested work.
📋 Key Themes & Highlights
Key Themes
- Dead code left behind by refactor:
MAX_RESTORE_RUNS/MAX_RESTORE_PAGES/SCAN_WINDOW_MSinconstants.cjsare now unused in production (onlyrestore_aic_scan_cache.cjsconsumed them, and it now defines its own local bounds) but remain exported and asserted inconstants.test.cjs. Flagged inline.
Verified Correctness
- Traced
withQuotaResetRecovery(daily_aic_api_budget.cjs) againstgetGitHubAPIDiagnostic/getGitHubAPIQuota: the one-shot deferral correctly excludesauthentication_failed/permission_denied, requiresGITHUB_RUN_ATTEMPT === "1", bounds the wait tomaxWaitMs(10 min) + 60s timer tolerance, and deliberately does not re-catch a second failure ("renewed exhaustion cannot defer again") — this matches the PR's stated contract exactly. - Confirmed the
recoveryoption wiring end-to-end:check_daily_aic_workflow_guardrail.cjs→scanDailyAIC→withQuotaResetRecovery, and separatelyrestore_aic_scan_cache.cjsmainWithPaths→withQuotaResetRecovery(() => restoreWithBudget(...)). - Verified the bounded snapshot-discovery change (
MAX_SNAPSHOT_PRODUCERS=2,MAX_SNAPSHOT_HISTORY_PAGES=2, 48hCACHE_RETENTION_MSwindow) is exercised by tests asserting the exactper_page: 2call and the page-by-page trusted-candidate counting/early-break logic inrestoreWithBudget.
Positive Highlights (/diagnosing-bugs, /tdd)
- ✅ Root-caused against real production evidence (6 failed scheduled runs + 1 successful-but-wasteful baseline cited in the PR body), not just patching a symptom.
- ✅ Strong regression coverage: descriptive spec-like test names ("restarts the complete phase only after the later server deadline", "does not defer HTTP 401/403 access failures even when quota headers are exhausted") covering unknown/invalid/expired/long resets, rerun vs. original attempt, early/late timer wake, and renewed exhaustion.
- ✅ Spec doc (
ai-credits-specification.md) updated in lockstep with the code, keeping the bounded-discovery and deferral contract documented.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 198.6 AIC · ⌖ 15.1 AIC · ⊞ 10.2K
Comment /matt to run again
There was a problem hiding this comment.
🔵 Needs a closer look
Timing-sensitive quota recovery in the startup guardrail has a confirmed deadline-selection defect and needs maintainer validation.
2 open issues
What changed in this PR
This PR updates gh-aw’s daily AI Credits startup guardrail to bound optional snapshot discovery and introduce limited recovery from GitHub API quota exhaustion.
Changes:
- Limits snapshot discovery by retention window, producer count, and history pages.
- Adds bounded, first-attempt quota-reset recovery that restarts the complete accounting phase.
- Documents the recovery contract and adds regression coverage.
| File | Description |
|---|---|
docs/src/content/docs/specs/ai-credits-specification.md |
Defines snapshot bounds and quota-recovery requirements. |
actions/setup/js/restore_aic_scan_cache.cjs |
Bounds snapshot discovery and adds restore-phase recovery. |
actions/setup/js/daily_aic_scan.test.cjs |
Covers phase restarts, refreshed history, changed attempts, and bounded misses. |
actions/setup/js/daily_aic_scan.cjs |
Wraps authoritative accounting in complete-phase recovery. |
actions/setup/js/daily_aic_api_budget.test.cjs |
Tests recovery deadlines, elapsed-time bounds, and fail-closed conditions. |
actions/setup/js/daily_aic_api_budget.cjs |
Implements a single bounded quota-reset deferral. |
actions/setup/js/check_daily_aic_workflow_guardrail.cjs |
Passes recovery options into authoritative accounting. |
Review effort: Max
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 98ed4ff
|
Track configured reserve stops independently of secondary restrictions, and list ten completed runs while limiting trusted snapshot inspections to two. Add deadline and untrusted-run regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
In response to #70968 (comment): No code change was requested by this completed skills review. The two actionable inline findings were fixed separately with regression coverage in 75d423d. |
|
In response to #70968 (comment): No code change is needed for this reported_no_action notice, and I am not treating it as proof of validation. The combined corrections passed 363 focused tests and strict TypeScript checking locally. |
|
In response to #70968 (comment): This reported_no_action notice contains no actionable finding, so no additional code change is needed. The quota and snapshot corrections are covered by explicit regression tests rather than relying on the no-op outcome. |
|
In response to #70968 (comment): The completed code-quality review requests no specific change. No additional change is needed beyond the two separately addressed inline findings. |
|
In response to #70968 (comment): This reported_no_action design-gate notice requests no ADR or design change. The narrow corrections retain the existing fail-closed accounting design, and the specification now distinguishes secondary deadlines and listed runs from trusted producer inspections. |
|
In response to #70968 (comment): Accepted: updated from main using make merge-main, fixed both inline findings in 75d423d, pushed the batch, and replied to and resolved both threads. Local validation passed (363 focused tests, strict TypeScript, lint, final progress gate, and 346/346 workflow recompilations); no CI dispatch or rerun was requested, and CI for the new head plus human review remain unverified. |
…egram Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Summary
Evidence and scope
Six inspected scheduled dispatcher failures on October 15, 16, and 18–21 stopped on the first snapshot-restore request with core quota remaining 0/15000. The October 17 successful baseline spent 11 requests discovering no usable snapshot, followed by only three authoritative accounting requests. This change reduces that amplification and provides bounded same-activation recovery; it does not attribute the shared quota consumers or guarantee API availability.
No schedule, compiler, generated-lock, workflow configuration, credential, queue, or deployment changes. No dispatches or reruns. Existing noon schedule mitigation remains unchanged.
Validation
make fmt-cjsandmake lint-cjspassed.make agent-report-progresspassed using installed modern Bash (macOS system Bash lacksmapfile): build, strict TypeScript check, JavaScript lint, schema freshness, impacted tests. No changed Go files; impacted Go tests correctly skipped. Existing lint warnings remain; no newly introduced warning.Residual live confirmation
Observe the next naturally scheduled run after deployment. Recovery is limited to ten minutes per phase, with one minute of timer scheduling tolerance; longer or unknown reset deadlines and renewed exhaustion deliberately still block startup. The activation job has no compiled timeout override; the agent-step timeout is unchanged. No stale-cache or partial-total authorization is introduced.