fix: stop unbounded pagination in check_rate_limit when run exceeds threshold - #48972
Conversation
…than threshold Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #48972 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). The single changed file is not in a monitored business logic directory. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Stops rate-limit pagination once workflow runs exceed the time window, preventing costly full-history scans.
Changes:
- Detects the first stale run in newest-first results.
- Stops processing and pagination immediately.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/check_rate_limit.cjs |
Terminates pagination at the time-window boundary. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Medium
| continue; | ||
| core.info(` Stopping pagination - run ${run.id} created before threshold (${run.created_at})`); | ||
| hasMore = false; | ||
| break; |
|
✅ 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. No behavioral tests were added or modified in this PR. The change is a production-only bug fix to check_rate_limit.cjs. Test Quality Sentinel review completed and comment posted. No test quality verdict applicable. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and minimal. Since listWorkflowRuns returns runs newest-first, the first run older than thresholdTime guarantees all remaining runs (and future pages) are also past the threshold. Replacing continue with hasMore = false; break correctly short-circuits the outer pagination loop, eliminating the unbounded scan that was causing 250-page walks and 37% CI failures. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 11.3 AIC · ⌖ 5.92 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — one actionable finding, otherwise clean.
📋 Key Themes & Highlights
Key Themes
- Missing regression test: The fix correctly stops pagination but there is no test that asserts
listWorkflowRunsis called only once when a stale run is encountered. The existing threshold test validates the count of matched runs, not whether pagination halts — so a futurecontinuereintroduction would pass silently.
Positive Highlights
- ✅ Root cause is fully addressed: the API ordering guarantee (newest-first) is now leveraged to stop early, not just skip.
- ✅
hasMore = false+breakis the correct dual-signal idiom for this loop structure — both the innerforand the outerwhileare terminated. - ✅ Updated log message (
Stopping paginationvs.Skipping run) clearly communicates the new intent. - ✅ Surgical 4-line change with zero risk of collateral damage.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 26.3 AIC · ⌖ 5 AIC · ⊞ 7K
Comment /matt to run again
| continue; | ||
| core.info(` Stopping pagination - run ${run.id} created before threshold (${run.created_at})`); | ||
| hasMore = false; | ||
| break; |
There was a problem hiding this comment.
[/tdd] The existing test "should exclude runs older than the time window" does not assert that pagination stops — it only checks the count. With the old continue the test would still pass, meaning this regression can silently reappear.
💡 Suggested regression test
it("should stop pagination when a run predates the threshold", async () => {
// First page has one stale run (older than window)
mockGithub.rest.actions.listWorkflowRuns
.mockResolvedValueOnce({
data: {
workflow_runs: [{
id: 999,
run_number: 1,
created_at: new Date(Date.now() - 120 * 60 * 1000).toISOString(),
actor: { login: "test-user" },
status: "completed",
}],
},
})
.mockResolvedValue({ data: { workflow_runs: [] } });
await checkRateLimit.main();
// Must only call once — page 2 should never be fetched
expect(mockGithub.rest.actions.listWorkflowRuns).toHaveBeenCalledTimes(1);
expect(mockCore.info).toHaveBeenCalledWith(
expect.stringContaining("Stopping pagination")
);
});Without this, a future continue reintroduction would go undetected.
@copilot please address this.
|
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: N/A — No Test Changes
Summary
ContextThis PR fixes an unbounded pagination bug in Verdict
|
There was a problem hiding this comment.
Verdict: Approve with a minor test-coverage suggestion
The fix is logically correct: since listWorkflowRuns returns runs newest-first, hitting one run older than the threshold means all subsequent runs are also stale, so hasMore = false; break correctly halts pagination instead of the old continue which scanned the full run history.
💡 Themes
- No regression test added for the specific incident (unbounded pagination). See inline comment.
- The newest-first ordering assumption from the GitHub API is relied upon implicitly; worth a comment or assertion in tests.
- Change is minimal, scoped, and matches the described root cause.
🔎 Code quality review by PR Code Quality Reviewer · aut00 · 27.6 AIC · ⌖ 7.11 AIC · ⊞ 7.8K
Comment /review to run again
| core.info(` Skipping run ${run.id} - created before threshold (${run.created_at})`); | ||
| continue; | ||
| core.info(` Stopping pagination - run ${run.id} created before threshold (${run.created_at})`); | ||
| hasMore = false; |
There was a problem hiding this comment.
Correct fix logically, but no regression test was added for this exact incident (unbounded pagination when a run predates the threshold) — the bug that caused a 37% failure rate for 6h will have no test guarding against reintroduction.
💡 Add a targeted test
The fix relies on the undocumented-in-code assumption that listWorkflowRuns returns runs newest-first (true by API default, but not enforced/asserted anywhere). A test mocking a paginated response where an early run in page 1 is older than thresholdTime, followed by mock pages that would need many more calls if pagination continued, would catch a future regression to continue and also validate this ordering assumption. Consider asserting github.rest.actions.listWorkflowRuns is called exactly once in that scenario.
|
@copilot please run the skill, investigate the failed check below, and rerun the relevant checks once fixed. Failed checks: Run: https://github.com/github/gh-aw/actions/runs/30488631737
|
|
@copilot run pr-finisher skill |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed in commit |
|
🎉 This pull request is included in a new release. Release: |
listWorkflowRunsreturns runs newest-first, so the first run found older than the threshold means all remaining runs (current page and beyond) are also past it. The loop wascontinue-ing anyway, causing every AI Moderator invocation to page through the entire run history — confirmed reaching page 250 with runs back to January 2026 before CI force-cancelled the step (37% failure rate over 6h).Change
In
actions/setup/js/check_rate_limit.cjs, replacecontinuewithhasMore = false; breakwhen a run predates the threshold:Run: https://github.com/github/gh-aw/actions/runs/30488631737