Measure and log GitHub API rate limit around daily AIC guardrail#38256
Conversation
- fetchAndLogRateLimit now returns the core rate-limit snapshot
({remaining, limit, used, reset} or null) so callers can reuse the
single API call for both logging and in-memory tracking
- check_daily_aic_workflow_guardrail.cjs calls fetchAndLogRateLimit
with "daily-aic-guardrail-start" before the first API call to
establish a before-guardrail baseline in the JSONL log
- After the artifact inspection loop, calls fetchAndLogRateLimit with
"daily-aic-guardrail-end" and logs a consumption-delta entry
(rateLimitBeforeInspection, rateLimitAfterInspection, consumed) via
logDailyGuardrail so the rate limit hunger of the guardrail is
observable in run artifacts
- Tests: updated toBeUndefined → toBeNull; added return-value tests
for fetchAndLogRateLimit; added main() test verifying the start/end
logs and consumption delta are emitted
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds explicit GitHub API rate-limit snapshots around the daily AI Credits (AIC) guardrail so operators can quantify how much rate-limit budget the guardrail consumes, and records those snapshots in the existing JSONL artifact for post-run analysis.
Changes:
- Update
fetchAndLogRateLimit()to return a core rate-limit snapshot ({remaining, limit, used, reset} | null) so callers can reuse the same API call for logging and in-memory tracking. - Add “start” and “end” rate-limit snapshots to the daily AIC guardrail flow and emit a structured delta log.
- Expand unit tests to cover the new return shape and the daily guardrail delta logging behavior.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/github_rate_limit_logger.cjs | Make fetchAndLogRateLimit() return a core snapshot (or null) in addition to writing JSONL entries. |
| actions/setup/js/github_rate_limit_logger.test.cjs | Update and add tests for the new fetchAndLogRateLimit() return contract and null cases. |
| actions/setup/js/check_daily_aic_workflow_guardrail.cjs | Capture rate-limit snapshots at guardrail start/end and log a consumption delta. |
| actions/setup/js/check_daily_aic_workflow_guardrail.test.cjs | Add a main() test asserting snapshot calls occur and a delta log is emitted. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
actions/setup/js/check_daily_aic_workflow_guardrail.cjs:325
- The initial rate-limit snapshot is fetched but its return value is discarded, and
rateLimitis still populated via a secondrateLimit.get()call (getCoreRateLimitSnapshot). This defeats the newfetchAndLogRateLimit()API (single call for logging + in-memory tracking) and also means the laterrateLimitBeforeInspectiondelta is not necessarily derived from the same snapshot that was logged asdaily-aic-guardrail-start. Capture the returned snapshot and use it as the baseline (with a fallback togetCoreRateLimitSnapshotwhen null).
// Capture a before-guardrail rate-limit snapshot and log it to the JSONL
// so consumers can determine the baseline available quota before inspection starts.
await fetchAndLogRateLimit(githubClient, "daily-aic-guardrail-start");
const currentRun = await githubClient.rest.actions.getWorkflowRun({
owner,
repo,
run_id: context.runId,
});
const rateLimit = await getCoreRateLimitSnapshot(githubClient);
- Files reviewed: 4/4 changed files
- Comments generated: 2
| const rateLimitEnd = await fetchAndLogRateLimit(githubClient, "daily-aic-guardrail-end"); | ||
| logDailyGuardrail("GitHub API rate limit consumed by daily AIC guardrail", { | ||
| rateLimitBeforeInspection: rateLimit.remaining, | ||
| rateLimitAfterInspection: rateLimitEnd?.remaining ?? null, | ||
| consumed: rateLimit.remaining - (rateLimitEnd?.remaining ?? rateLimit.remaining), | ||
| limit: rateLimit.limit, | ||
| reset: rateLimit.reset, | ||
| }); |
There was a problem hiding this comment.
Updated in 2ac9616 to use the stored daily-aic-guardrail-start snapshot as the baseline when available, and to clamp consumed at 0 so resets cannot produce negative consumption.
| const coreData = resources.core; | ||
| if (!coreData || typeof coreData !== "object") return null; | ||
| return { | ||
| remaining: Number(coreData.remaining ?? 0), | ||
| limit: Number(coreData.limit ?? 0), | ||
| used: Number(coreData.used ?? 0), | ||
| reset: coreData.reset ? new Date(coreData.reset * 1000).toISOString() : "", | ||
| }; |
There was a problem hiding this comment.
Updated in 2ac9616: fetchAndLogRateLimit now returns null when required core fields are missing/non-numeric instead of coercing defaults, and tests now cover partial core payloads.
|
@copilot run pr-reviewer skill |
Ran the |
|
@copilot run pr-finisher skill |
|
🧠 Matt Pocock Skills Reviewer was skipped during the skills-based review. |
|
❌ Test Quality Sentinel was skipped during test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #38256 does not have the 'implementation' label and has 0 new lines of code in business logic directories (≤100 threshold). Neither enforcement condition is met. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES — measurement is inaccurate and misleading from day one
The feature direction is right, but two blocking correctness bugs mean the logged consumed delta will always be wrong in normal operation, and will silently report 0 on any API failure.
Blocking issues (must fix)
-
Line 318 — return value discarded, redundant API call, wrong baseline:
fetchAndLogRateLimitwas updated specifically so callers can skipgetCoreRateLimitSnapshot— but the return value is thrown away andgetCoreRateLimitSnapshotis still called.rateLimit.remaining(thebeforebaseline) therefore reflects the state after the start snapshot +getWorkflowRunhave already consumed quota, soconsumedis systematically too low. -
Line 474 —
consumedsilently reports0on failure, and goes negative on reset-mid-run: WhenrateLimitEndisnull, the expression yields0— indistinguishable from genuine zero consumption. If the hourly window resets between snapshots, the value goes negative.
Non-blocking observations
- Line 332 / 349 — early returns leave an orphaned
daily-aic-guardrail-startJSONL entry with no matching end. - Test line 280 —
>= 2bound passes even without the end-snapshot call; should be>= 3(or=== 2after removing the redundant snapshot call). - Test line 277 — assertions only check that key names appear in the serialized string; delta correctness is never validated.
🔎 Code quality review by PR Code Quality Reviewer · ⌖ 14 AIC
Comments that could not be inline-anchored
actions/setup/js/check_daily_aic_workflow_guardrail.cjs:318
Return value discarded — extra API call and wrong before baseline. fetchAndLogRateLimit now explicitly returns the core snapshot specifically so callers avoid a second rateLimit.get() call (see its docstring: "callers can use a single API call for both logging and in-memory rate-limit tracking"), but the return value is thrown away here and getCoreRateLimitSnapshot (line 324) issues an identical second call. This means rateLimit.remaining used as rateLimitBeforeInspection …
actions/setup/js/check_daily_aic_workflow_guardrail.cjs:474
consumed silently lies when the end snapshot fails or when the rate-limit window resets mid-run. Two independent bugs in this expression:
- When
rateLimitEndisnull(API error), the fallback israteLimit.remaining, soconsumed = 0. Zero looks like a successful measurement of no consumption — there is no signal distinguishing "guardrail used zero quota" from "we could not measure it". - If the hourly rate-limit window resets between the
beforeandaftersnapshots, `rate…
actions/setup/js/check_daily_aic_workflow_guardrail.cjs:332
Early return leaves the daily-aic-guardrail-start JSONL entry orphaned. fetchAndLogRateLimit("daily-aic-guardrail-start") was already called unconditionally, but this path exits before the matching "daily-aic-guardrail-end" entry or the consumption-delta log are ever emitted. The same problem applies to the maxInspectableRuns <= 0 early return at line 349.
<details>
<summary>💡 Suggested fix</summary>
Move fetchAndLogRateLimit("daily-aic-guardrail-start") to just before the cand…
actions/setup/js/check_daily_aic_workflow_guardrail.test.cjs:280
>= 2 lower bound cannot catch regression that removes the end-snapshot call. The actual call sequence is: fetchAndLogRateLimit-start (1) + getCoreRateLimitSnapshot (2) + fetchAndLogRateLimit-end (3) = minimum 3 calls. An assertion of >= 2 passes even if the end measurement is deleted entirely, so the test provides no regression guard for the feature it claims to test.
<details>
<summary>💡 Suggested fix</summary>
// Must be at least 3: start snapshot + getCoreRateLimitSnap…
</details>
<details><summary>actions/setup/js/check_daily_aic_workflow_guardrail.test.cjs:277</summary>
**Assertions only verify key names exist in the serialized JSON string, never the computed values.** `toContain("consumed")` passes for `consumed: 0`, `consumed: null`, `consumed: -500`, or any other value — the delta arithmetic is entirely untested.
<details>
<summary>💡 Suggested fix</summary>
Parse the log line and assert the numeric properties directly:
```js
const detailsMatch = consumptionLog.match(/\[daily-workflow-aic\].*?: (\{.*\})$/);
const details = JSON.parse(detailsMatch[1]);
ex…
</details>Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran the PR finisher flow and addressed the in-scope review threads with fixes in |
The daily AI Credits guardrail inspects prior workflow runs via the GitHub Actions API and downloads usage artifacts, but there's no visibility into how much GitHub API rate limit this consumes. Without before/after snapshots, it's impossible to determine whether the guardrail itself is a significant rate-limit consumer.
Changes
github_rate_limit_logger.cjsfetchAndLogRateLimitnow returns{remaining, limit, used, reset} | nullfor thecoreresource, allowing callers to reuse a singlerateLimit.get()call for both JSONL logging and in-memory tracking.check_daily_aic_workflow_guardrail.cjsfetchAndLogRateLimit(githubClient, "daily-aic-guardrail-start")as the first API operation inmain()— beforegetWorkflowRun— to write a baseline entry togithub_rate_limits.jsonl.fetchAndLogRateLimit(githubClient, "daily-aic-guardrail-end").The
github_rate_limits.jsonlartifact entries for"daily-aic-guardrail-start"and"daily-aic-guardrail-end"(both with"source": "rate_limit_api") also capture the full per-resource breakdown for deeper analysis.Tests
fetchAndLogRateLimitreturn-value assertions (toBeUndefined→toBeNullon error path); added three new tests covering success return, absent resources, and absent core resource.main()test verifiesfetchAndLogRateLimitis called at least twice and the consumption-delta log entry is emitted with expected fields.