Repository navigation
feat: implement issue #1994 — Rate-limited PR reviews wait up to ~7h after the limit resets — the retry relies on GitHub's best-effort cron, which fires far less often than every 15 min - #1999
Conversation
…after the limit resets — the retry relies on GitHub's best-effort cron, which fires far less often than every 15 min
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #1999 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but I can't mark it done yet: required check |
|
No description provided. |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a deterministic delayed rate-limit retry mechanism via a new workflow and script to handle deferred PR reviews near their rate-limit reset times, reducing reliance on the best-effort cron schedule. It also updates the sweep script, documentation, and adds comprehensive unit tests. The review feedback suggests improving the robustness of the shell script by adding error handling to the mktemp command and utilizing optional chaining with fallback values in jq to prevent potential crashes under set -e.
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-01T20:14:09Z. |
Dev-Lead — review-changes (partial)A commit was pushed, but not every requested change was applied. Per requested item:
The unaddressed items above still need work. |
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-01T20:19:51Z. |
Superseded by automated re-review at
|
|
Acknowledged — this is a Codex usage-limit notice from the external code-review bot, not a finding against this PR's diff. No code action is warranted; advisory-bot availability is an org billing/quota concern tracked outside this PR, and the pr-review-agent already posted a rate-limit note. Recording as informational. |
|
Acknowledged — this is a Qodo trial-ended / billing notice, not a finding against the diff. No code action applies; resuming Qodo reviews is an org billing decision outside this PR. Recording as informational. |
|
Acknowledged — this is CodeRabbit's auto-generated "review in progress" placeholder (no findings posted yet); it self-updates once the bot finishes. Nothing actionable in the diff to address here. Recording as informational. |
|
Acknowledged — SonarCloud reports Quality Gate passed with 0 new issues for this PR. This is a passing-status notice, not a finding; no code action required. Recording as informational. |
Dev-Lead — waiting on PR blockers (intent: fix-reviews)PR: #1999 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
No description provided. |
Superseded by automated re-review at
|
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-01T20:36:01Z. |
|
dev-lead is withholding action on this item. It is labeled To re-enable automated pickup: remove the |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
don-petry
removed
the
needs-human-review
Claude
…erive the sleep ceiling Addresses the open review threads on #1999: - Marker at wake time (cubic P2). still_applicable now also requires the armed head's rate-limit marker. Without it, a retry whose marker was removed fell through the delegated sweep's REVIEW_REQUIRED + green-CI path and launched an ordinary review. - Failed arms spend the budget (cubic P2). A failed `gh workflow run` in arm_delayed_retry now counts against MAX_DISPATCH, so a transient dispatch error cannot fan out across every remaining candidate. - Sleep ceiling derived (cubic P3). DELAYED_RETRY_MAX_SLEEP_SEC now defaults to the sweep's DELAYED_RETRY_HORIZON_SEC + DELAYED_RETRY_BUFFER_SEC (same variables and defaults). An explicit lower value is raised to that floor with a warning, so the retry never wakes before reset+buffer. - Docs (cubic P3). docs/agentic-interaction-model.md and the sweep comment no longer call the cron a guaranteed <=15-min backstop. - Tests (graphite). write_pr fixtures carry `state`; new cases cover a merged PR, a closed PR, a missing marker, a marker for another head, the raised ceiling, and a failed arm. The marker, ceiling and failed-arm cases fail without the fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fdDZDb4mhT65MU1izeA4R
1efea90 wrapped the Bridge B sentence in docs/agentic-interaction-model.md so a line began with "#1952),". markdownlint reads that as an ATX heading without a space and fails the Lint check. Reworded so the issue reference sits mid-line. markdownlint-cli2 is clean over the repo. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fdDZDb4mhT65MU1izeA4R
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-01T21:39:46Z. |
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
CI checks on this PR are still running. The PR-review sweep re-reviews this PR automatically once the checks complete — no action is needed. Posted by the donpetry-bot PR-review cascade. |
cubic on #1999: - P2: the digit-only checks admit a leading zero, which bash arithmetic reads as octal (0600 -> 384). DELAYED_RETRY_HORIZON_SEC, DELAYED_RETRY_BUFFER_SEC and DELAYED_RETRY_MAX_SLEEP_SEC are now normalized with 10#, so the ceiling matches the sweep's horizon. - P3: the sweep reads rate-limit markers from reviews AND comments, but the wake-time check only read comments, so a marker in a review body would silently strand the retry. still_applicable now fetches `reviews` and searches both. Both new tests fail without the change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fdDZDb4mhT65MU1izeA4R
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
cubic P3: the test name says the retry re-dispatches but it only checked the no-marker message was absent. It now asserts the pr-review-trigger.yml dispatch for that PR, like the other happy-path cases. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fdDZDb4mhT65MU1izeA4R
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-01T22:14:06Z. |
Automated review — human attention neededThis PR has been through 3 automated review cycles since the last approval or escalation (cap: 3) without converging. Further automated review has been paused to avoid infinite loops. Please take a look manually, or close this PR if it's no longer needed. To re-engage the automated cascade with a fresh cycle budget, a human should remove the Posted by the donpetry-bot PR-review cascade. |
don-petry
removed
the
needs-human-review
Claude
|
CI checks on this PR are still running. The PR-review sweep re-reviews this PR automatically once the checks complete — no action is needed. Posted by the donpetry-bot PR-review cascade. |
|
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 481142d140a34fa5b7c4ffef37afdd2b1938f8c0
Review mode: triage-approved (single reviewer)
Summary
Fixes #1994. When the sweep defers a rate-limited PR whose reset is within a configurable horizon (default 3600s), it now dispatches a new workflow_dispatch-only workflow, pr-review-delayed-retry.yml. That workflow checks the PR is open, still at the armed head, and still carries the marker. It sleeps until reset+120s (capped at horizon+buffer), checks the same things again, then hands the PR back to the sweep with ARM_DELAYED_RETRY=false. The sweep re-checks the marker, any standing verdict, holds and CI before it re-dispatches through the normal trigger, so the retry never forces a review. Overstated "guaranteed ≤15-min backstop" wording is corrected throughout. Risk is MEDIUM: this is new workflow and orchestration logic. It uses the same PAT pattern as pr-review-sweep.yml: the don-petry identity guard, workflow_dispatch inputs passed only through env: (never interpolated into run:), contents: read, and a pinned checkout. It adds no new secret handling or third-party surface.
Linked issue analysis
Every acceptance criterion in #1994 is met:
- Near-reset arming, configurable horizon, wait to reset+buffer, same re-dispatch path: done (
arm_delayed_retry,DELAYED_RETRY_HORIZON_SEC, delegation to the sweep throughSWEEP_PRS_FILE). - Concurrency: each (PR, head SHA) gets its own group, so retries for different PRs never cancel each other or the full sweep.
- Idempotent and bounded: a dedup check matches the run-name (
gh run listdisplayTitle) and skips re-arming when a retry is already queued or in progress. A newer push makes the retry exit without acting (checked before and after the sleep). Hold, escalation and standing-verdict exclusions are re-applied by the delegated sweep. Arms share the singleMAX_DISPATCHbudget, including failed attempts. The retry cannot re-arm itself. - Comment corrections: done in the workflow, script, AGENTS.md, the interaction-model doc and the interaction contract.
- Tests: near reset arms one retry; far reset arms none; SHA change, standing verdict, merged, closed and missing marker all exit without acting; per-PR concurrency group; DRY_RUN; MAX_DISPATCH sharing; dedup; sleep floor; base-10 parsing.
One small gap (not blocking): the AC asks to "state the choice and the runner-minute cost". The design is documented in code comments and AGENTS.md, but the PR body does not state the cost explicitly (up to about 62 minutes of runner sleep per armed retry, bounded by timeout-minutes: 70).
Findings
Prior cascade findings (cycle 3/3 at ad82ff67), all resolved by later commits:
- Vacuous DRY_RUN arming assertion → the test now asserts "would arm delayed retry".
- Sleep, buffer and cap paths untested → a near-future reset test exercises the sleep; tests cover the max-sleep floor and leading-zero/base-10 parsing.
- Lane missing from the
concurrency_laneregistry → added tointeraction-contracts/pr-review.yml. - Re-arm churn on every sweep tick →
delayed_retry_in_flightdedup via run-name/displayTitle, which fails open. - Arms starving
MAX_DISPATCH→ arms and dispatches now share one budget, and a failed arm still uses a slot. - Retry waking before the sweep's reset check → the sleep floor is forced to horizon+buffer, so an armed retry always wakes after reset.
- 65-minute timeout too tight → now 70 minutes.
- No pre-sleep supersession check →
still_applicable pre-sleepadded; it also requires the marker and an OPEN state. - Marker only searched in comments → now searches reviews and comments.
- Unresolved bot threads → none remain unresolved (GraphQL check: 0).
Non-blocking notes:
- info: Dedup keys on (PR, head), not (PR, head, reset). If a second marker with a later reset appears at the same head while a retry sleeps, the first retry wakes, the sweep defers again without re-arming, and the next sweep tick arms the new reset. The cron backstop covers this; it is harmless.
- info:
delayed_retry_in_flightmatches onlyqueued/in_progress. A run inwaiting/pendingwould not count, but withcancel-in-progress: truethe worst outcome is one redundant arm. - info:
shellcheck --severity=warningis clean on both scripts (checked locally).
CI status
All required checks pass: SonarCloud, CodeQL, agent-shield / AgentShield, dependency-audit / Detect ecosystems, duplicate-decl-gate. The other completed checks pass too, including Lint, bats, shellcheck, unit-tests, unit, actionlint, validate-interaction-contracts, validate-interaction-model, gh-aw-compile and Secret scan (gitleaks).
Two non-required guard checks (Test-Deletion Guard, Sync Scope Guard) have been queued for about 25 minutes because of a runner backlog; their first runs at this SHA were cancelled when the merge commit replaced them. Neither applies to this diff: it deletes no test lines, and it is not a standards-sync PR. The rest of the pending rollup entries (add-to-project, dev-lead dispatch, pr-auto-review) are automation triggers, not CI.
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 5.5 [opus 4.8, opus 4.7]). Reply if you need a human review.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 481142d140a34fa5b7c4ffef37afdd2b1938f8c0
Review mode: triage-approved (single reviewer)
Summary
Fixes #1994. When the sweep defers a PR on a rate-limit marker whose reset is within DELAYED_RETRY_HORIZON_SEC (default 3600s), it now arms a new dispatch-only workflow, pr-review-delayed-retry.yml. The armed run checks that the PR is open, still at the armed head and still carries that head's marker. It then sleeps until reset+buffer (never less than horizon+buffer), runs the same checks again, and hands the single PR back to sweep-stuck-reviews.sh with ARM_DELAYED_RETRY=false. The sweep applies its usual hold, standing-verdict, reset and CI gates before it dispatches through the normal trigger, so the retry never forces a review and cannot re-arm itself. Comments that called the cron a "guaranteed ≤15-min backstop" are corrected across the workflow, script, AGENTS.md, the interaction-model doc and the interaction contract.
Risk is MEDIUM: this adds new orchestration logic and a new workflow. The workflow uses the same don-petry PAT as pr-review-sweep.yml and in the same way: the identical PAT-presence and gh api user identity guard, contents: read, and a SHA-pinned checkout. workflow_dispatch inputs reach the script only through env: and are never interpolated into run:. No new secret, third-party action or reusable workflow is introduced. I confirmed this triage-approved assessment independently.
Linked issue analysis
Every #1994 acceptance criterion is substantively met:
- Near-reset arming, configurable horizon, wait until reset+buffer, same re-dispatch path:
arm_delayed_retryin the defer branch,DELAYED_RETRY_HORIZON_SEC/DELAYED_RETRY_BUFFER_SEC, and delegation to the sweep throughSWEEP_PRS_FILE. - Design choice and runner-minute cost: now stated in the PR body (≤~62 runner-minutes of sleep per armed retry, capped by
timeout-minutes: 70). This closes the one gap the earlier review at this SHA noted. - Concurrency: a per-(PR, head SHA) group, so retries never cancel each other or the sweep's group.
- Idempotent and bounded: a run-name/displayTitle dedup that fails open; supersession checks before and after the sleep; exclusions (holds per #946/#1550, standing verdict) re-applied by the sweep. I checked in
sweep-stuck-reviews.shat head that they are evaluated before the defer/arm branch. Arms and dispatches share oneMAX_DISPATCHbudget, failed attempts included, and there is no force-review. - Tests: near reset arms exactly one retry; far reset arms none; SHA change, standing verdict, merged, closed and missing marker all no-op; concurrency group; DRY_RUN; shared budget; dedup; sleep floor; base-10 parsing.
Note: #1994 was closed manually as completed (2026-10-01T21:46Z) before this PR merged. The PR's Closes #1994 is harmless, but the issue state does not yet reflect a merged fix.
Findings
No blocking findings. Unresolved review threads: 0. No unanswered human questions; all later don-petry comments are automation dispositions.
Non-blocking notes:
- info: horizon/ceiling coupling. The sweep reads
DELAYED_RETRY_HORIZON_SECfrom its own env. The retry script derives its sleep floor from its own copy of the variable (default 3600), which is not passed as a workflow input. If someone later raises the horizon only on the sweep side, the retry could wake before reset. It would also hittimeout-minutes: 70once horizon+buffer gets above ~68 min. Either way the delegated sweep would re-defer without re-arming, which falls back to the cron. Not reachable with today's defaults (neither workflow overrides the variable). Consider passing the horizon through as an input, or adding a comment that both sides must change together. - info: Dedup keys on (PR, head), not (PR, head, reset), and only counts
queued/in_progressruns (notwaiting/pending). The worst case is one redundant arm, whichcancel-in-progresscollapses, or a cron pickup. - info:
run-nameandconcurrency.groupinterpolateinputs.pr_url/inputs.head_sha. These are not shell sinks, and only users with write access can dispatch, so there is no injection surface.
CI status
All substantive checks pass at 481142d1: Lint, bats, shellcheck/ShellCheck, unit-tests, unit, actionlint, CodeQL (actions, python), SonarCloud (Quality Gate passed, 0 new issues), Secret scan (gitleaks), AgentShield / Agent Security Scan, validate-interaction-contracts, validate-interaction-model, template-drift, reusable-pin-compliance, toplevel-permissions, concurrency-config and gh-aw-compile.
The guard / sync-scope-guard entries show one cancelled run, but a later run of each succeeded. Still-queued rollup entries (dev-lead dispatch/resume, Dismiss, pr-auto-review, review / review) are automation triggers with completed-success siblings, not CI gates.
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 5.5 [opus 4.8, opus 4.7]). Reply if you need a human review.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 481142d140a34fa5b7c4ffef37afdd2b1938f8c0
Review mode: triage-approved (single reviewer)
Summary
Fixes #1994. When the sweep defers a PR whose rate-limit reset is within DELAYED_RETRY_HORIZON_SEC (default 3600s), it now dispatches a new workflow_dispatch-only workflow, pr-review-delayed-retry.yml. That workflow sleeps until reset + buffer, checks that the PR is still open, at the same head and still carrying the rate-limit marker, then hands the PR back to the sweep with ARM_DELAYED_RETRY=false. It also corrects the docs and contract wording that called the cron a 'guaranteed ≤15-min backstop'. Confirms the triage-approved assessment: MEDIUM, approvable.
Linked issue analysis
#1994's acceptance criteria are addressed:
- Near reset arms a retry; far reset does not: arming happens only when 0 < delta ≤ horizon, and a reset past the horizon is left to the cron.
- Not schedule-based, with stated cost: dispatch-only workflow; the PR body gives the design choice and the runner-minute cost (≤ ~62 min sleep, capped by timeout-minutes: 70).
- Concurrency: the group is per-(pr_url, head_sha), so retries for different PRs never cancel each other or the full sweep. A gh run list displayTitle check skips re-arming while one is queued or running.
- Idempotent and bounded:
- SHA supersession guard runs both before and after the sleep.
- The marker re-check stops a plain review from being forced.
- Delegating to the sweep keeps its existing exclusions and standing-verdict checks.
- Arms count against the shared MAX_DISPATCH budget.
- No force flag is passed.
- Comment correction: the 'guaranteed ≤15 min' wording is fixed in pr-review-sweep.yml, AGENTS.md, docs/agentic-interaction-model.md and interaction-contracts/pr-review.yml.
- Tests: new tests/test_delayed_rate_limit_retry.bats plus additions to tests/test_sweep_stuck_reviews.bats, wired into lint.yml.
Note: #1994 already shows CLOSED although this PR is not merged. The PR's Closes #1994 is still correct.
Findings
No blocking findings.
- Security: The PAT handling in the new workflow is the same as the existing pr-review-sweep.yml:
GH_PAT_DON_PETRY || GH_PAT_WORKFLOWSpassed through env, with a fail-closed check that the PAT is bound to don-petry. Permissions arecontents: read, and checkout is pinned to the same v7.0.1 SHA used elsewhere in the repo. Workflow inputs reach the script only through env (run-name and concurrency use them only as expressions), so there is no shell-injection path. - Correctness:
- The sleep ceiling has a floor of horizon + buffer, so the retry always wakes after reset.
- The base-10 coercion avoids octal misreads.
- An unparseable NOT_BEFORE proceeds immediately, so the PR is not stranded.
- The in-flight dedup query fails open.
- A failed arm still counts against MAX_DISPATCH, which prevents a retry storm.
- Review threads: none unresolved. Earlier codeant, gemini, cubic and graphite comments were addressed in prior cycles, and cubic reports 'All reported issues were addressed'.
- Docs: AGENTS.md now lists the new repo-specific workflow as an exception, and the §4 classification table and interaction contract are updated.
- Secret scan: the MCP run_secret_scanning tool is not available in this session. The gitleaks CI check passed.
CI status
All substantive checks pass: Lint, bats, unit-tests, ShellCheck, actionlint, CodeQL, SonarCloud (Quality Gate passed, 0 new issues), gitleaks, Agent Security Scan, and the stub/permissions/contract validators. Earlier cancelled runs of guard and sync-scope-guard have later successful runs. The only queued or pending entries are the review automation itself (pr-auto-review, review/review, dev-lead dispatch/resume), not CI gates.
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 5.5 [opus 4.8, opus 4.7]). Reply if you need a human review.
2 participants
Problem
Rate-limited PR reviews wait up to ~7h after the limit resets — the retry relies on GitHub's best-effort cron, which fires far less often than every 15 min
From the issue: When pr-review defers a PR because an advisory reviewer is rate-limited, it writes:
<!-- pr-review-agent rate-limited v1 sha=<HEAD> status=rate-limited reset=<ISO> -->fromscripts/lib/advisory-review-gate.sh(~L340). The only thing that re-dispatches the review afterresetispr-review-sweep.yml/scripts/sweep-stuck-reviews.sh(~L300).Design choice and runner-minute cost
Choice: this PR uses a dispatched, sleeping one-shot workflow (
pr-review-delayed-retry.yml). It does not shorten or duplicate the cron. When the sweep defers a PR whose reset falls withinDELAYED_RETRY_HORIZON_SEC(default 3600s), it arms one retry for that PR and head SHA. The retry sleeps until reset plusDELAYED_RETRY_BUFFER_SEC(default 120s). It then re-checks that the PR is open, still at the same head, and still carries the marker, and hands the PR to the normal sweep path withARM_DELAYED_RETRY=false.Runner-minute cost: each armed retry uses at most ~62 runner-minutes of sleep (horizon + buffer = 3720s), plus about a minute of setup and the one-PR sweep. The job is capped at
timeout-minutes: 70.MAX_DISPATCHbudget, since arms count against it.Risk
Medium — changes GitHub Actions workflow behavior, which is exercised only post-merge; verify via the affected workflow runs.
Test plan
Tests added/updated:
tests/test_delayed_rate_limit_retry.bats,tests/test_sweep_stuck_reviews.bats. Verification:bash scripts/dev-lead-lint.sh(shellcheck --severity=warning) ran pre-commit; the bats suite runs in CI.Rollback
Revert this PR. No non-revertible side effects (no tags, migrations, or external state).
Monitoring
Watch the affected workflow run(s) in the Actions tab and this PR's Lint check for regressions.
Closes #1994
<source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg">