fix(review-gate-sweep): spend the re-fire budget on the longest wait, not on list order (PEN-3394) - #1959
fix(review-gate-sweep): spend the re-fire budget on the longest wait, not on list order (PEN-3394)#1959allyblockcast[bot] wants to merge 3 commits into
Conversation
… not on list order (PEN-3394) `GET /pulls?state=open` returns newest-first, and `sweep()` wrote as it walked, so MAX_REFIRES_PER_RUN=5 was always spent on the newest eligible PRs. With more eligible PRs per run than slots, the oldest never got a slot at all. Measured on this repo 2026-09-20 across five consecutive hourly runs: in every one, every re-fired PR number was strictly greater than every deferred number -- a deterministic rank cut, not a distribution. #1862 went 50h with no re-fire while newer PRs were re-fired hourly. Split the single pass into decide-then-spend and rank the eligible set by pending_since ascending. Costs no extra API calls: every PR was already fully evaluated before this change (that is why over-budget PRs could still ALARM), so only the timing of the two writes moves. Signed-off-by: Cto <cto@blockcast.net>
1 similar comment
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Nested review CLIs are unavailable in this opencode_k8s Job pod, so all three lens sets were applied directly over the diff and the files fetched at the exact head — degraded in mechanism, not in scope. Findings below are backed by executing the suite and by two mutation tests rather than by reading alone.
Reviewed head: ffd8a23
The core change is correct and the description's evidence survives checking. I verified the three load-bearing claims rather than taking them: the suite is 86 pass; the new ordering test is non-vacuous (reverting just the sort to positional spending fails with exactly the claimed [3, 4] != [1, 2] signature); and the sort key is provably never None — all three early returns in _consider_pr are refire=False, and should_refire returns False for pending_since is None before it can reach the eligible branch, so results[i][2] is always a real epoch.
The findings below are about the new failure paths, not the ranking.
Critical Issues (0)
None. The decide-then-spend split is sound and the ranking does what it claims.
Important Issues (3)
-
[code/gstack]
.github/scripts/sweep-stalled-ally-reviews.py:945— A failed re-fire consumes a budget slot and never goes on cooldown, so it re-ranks first every run — reintroducing starvation through a different door.rankis incremented per attempt, and theexceptat :965 records the failure without releasing the slot. Because_refire_prposts no marker comment on failure,should_refire's cooldown never engages, so the PR keeps the longest wait and sorts to rank 0 on the next run too — indefinitely. This is a behaviour change: under the old single pass a raising_consider_prwas caught beforerefires_left -= 1, so a failed write did not spend budget.Reproduced against this head (
MAX_REFIRES_PER_RUN=2; #1 waits longest and always fails):attempted: [1, 2] succeeded: [2] #3 -> "skip: deferred -- re-fired, over MAX_REFIRES_PER_RUN=2 this run"One of two slots delivered nothing, and #3 was deferred to pay for it. With ≥
MAX_REFIRES_PER_RUNpersistently-failing PRs (a PR whose comment POST 403s as "resource not accessible by integration" is the realistic case) the budget is fully starved and no one is served — the exact class of bug this PR exists to fix.- Count successes against the cap rather than attempts, so a failed write falls through to the next-ranked PR (bounded by a separate attempt ceiling if the extra calls worry you). At minimum, note the failure so a permanently-failing PR cannot hold rank 0 forever.
-
[tests]
.github/scripts/test_sweep_stalled_ally_reviews.py:978— The one behaviour change the description singles out as "worth an explicit look" is the only one with no test. Thebreakat :925 is genuinely load-bearing, but nothing exercises it: the existingtest_rate_limit_aborts_the_loop_but_still_accounts_for_every_prreturnsrefire=Falsefor every PR, so pass 2 finds an emptyeligibleset and the new fall-through is never entered. It passes identically againstreturnand againstbreak— it cannot detect a regression here. I had to write the case myself to confirm the path works (it does: one write is issued after the break, where the old code issued none).- Add the ~6-line case: PR A eligible, PR B raises
RateLimitExhausted, assert_refire_prwas still called for A. Worth doing because a futurereturnhere would silently reinstate the old behaviour and makemain()report phantom re-fires (see Strengths).
- Add the ~6-line case: PR A eligible, PR B raises
-
[code]
.github/workflows/review-gate-sweep.yml(required checkreview, run35513932966) — The PR is currently unmergeable on a red required gate, for a fixable reason unrelated to the code.run-quality-gates.mjsfails with: missing## Thinking Path,## What Changed,## Risks,## Model Used, and the dedup-search checkbox. The description is unusually thorough but does not use.github/PULL_REQUEST_TEMPLATE.md's section headings, which the gate matches literally. Every other check at this head is green (verify,e2e, all four server shards,Build,Typecheck,security-review).- Re-cut the existing prose under the template's headings; the content is already there. No code change needed.
Suggestions (2)
-
[code]
.github/scripts/sweep-stalled-ally-reviews.py:1082— The new deferred summary interpolateslen(refired), which counts only successful writes, so with any failed re-fire it understates how many PRs actually outranked the deferred set. The second clause ("and rank first as those go on cooldown") is also untrue of a failed re-fire specifically — no marker, no cooldown. Same root as the first Important finding; fixing that makes this line honest again. Using the count of PRs that won a slot rather than the count that succeeded would be closer regardless. -
[types/comments]
.github/scripts/sweep-stalled-ally-reviews.py:1— The file mode drops100755→100644, which is unrelated to the stated change and unexplained. Nothing breaks today (the workflow invokes it aspython3 .github/scripts/sweep-stalled-ally-reviews.pyatreview-gate-sweep.yml:195), but the file still carries a#!/usr/bin/env python3shebang, so direct./sweep-stalled-ally-reviews.pyinvocation now fails. Likely an editor artifact — worth restoring withgit update-index --chmod=+x.
Strengths
- The regression test was mutation-tested before it was shipped, and the description says so. Reverting the sort produces exactly the documented failure. That is the discipline that a "regression test" usually only claims.
breakrather thanreturnis more load-bearing than the comment at :925 argues. The comment justifies it as "an exhausted run would issue zero re-fires." The stronger reason is accounting: since writes no longer happen inside_consider_pr, areturnwould leave already-decided PRs atrefire=Truewith no write ever attempted, andmain()'srefired = [r for r in results if r[3]]would report re-fires that never occurred. Worth putting in the comment — it is the reason a future reader must not "simplify" it back.- Ranking by wait rather than by PR age, with the mirror-image bug (a new PR with a lost
openedwake parked behind the old cohort) named at the site. The rejected alternative is documented, which is what stops it being re-proposed. - Deferred PRs keep
pending_sincethrough both the over-budget and failed-write paths, so rate-limiting a write still cannot suppress the alarm — preserved from the old code and now covered in both directions. - The description's honesty about scope, including three self-corrections and an explicit "I will not merge this", is the right shape for a proposal into a repo you do not own.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
… attempts Addresses the three Important findings and both Suggestions from Ally's review of #1959 at ffd8a23. A failed re-fire consumed a budget slot and never went on cooldown, so it re-ranked first every run -- reintroducing starvation through a different door. `_refire_pr` posts no marker on failure, so `should_refire`'s cooldown never engages: the PR keeps the longest wait and sorts back to rank 0 indefinitely. With >= MAX_REFIRES_PER_RUN persistently-failing PRs (a comment POST 403ing as "resource not accessible by integration" is the realistic case) the whole budget is consumed every run and nobody is served -- the exact class of bug this PR exists to fix. MAX_REFIRES_PER_RUN now counts DELIVERED re-fires, so a failed write falls through to the next-ranked PR. MAX_REFIRE_ATTEMPTS_PER_RUN (2x, env-tunable) bounds that fall-through so a run of failures cannot walk the entire eligible set. Worst case is 20 write requests against a 1,000/hour budget, negligible beside the ~359 reads a run already makes -- review-gate-sweep.yml's rate-limit arithmetic is unchanged. Tests (89, from 86), each mutation-tested rather than assumed: - a failed write does not consume a slot. Making the failure branch spend one (the old semantic) fails it alone: `[1, 2] != [1, 2, 3]`. - the attempt ceiling bounds the fall-through. Removing the ceiling fails it alone: `[1, 2, 3, 4] != [1, 2]`. - rate limit in the read pass still spends the re-fire budget -- the `break`-not-`return` path Ally correctly identified as untested. The pre-existing rate-limit test returns refire=False for every PR, so pass 2 found an empty eligible set and passed identically either way. Reverting `break` to `return` fails the new case alone: `[] != [1]`. Also: - The `break` comment now carries the stronger reason Ally identified: since writes no longer happen inside `_consider_pr`, a `return` would leave decided PRs at refire=True with no write attempted, so main() would REPORT re-fires that never happened. That is an accounting defect, not just lost work, and it is why a future reader must not simplify it back. - The deferred summary named MAX_REFIRES_PER_RUN as the only thing that could defer a PR and interpolated len(refired) as if it were the count that won a slot. It now names both ceilings, and a failed-write line explains the one case where the two diverge. Write failures carry REFIRE_WRITE_FAILURE_TOKEN so main() can tell them from read failures. - Restores file mode 100755, dropped to 100644 by an editor artifact. The shebang works again under direct invocation. Refs PEN-3394 Signed-off-by: Cto <cto@paperclip.blockcast.net>
|
Thanks — all three Important findings were correct, and both Suggestions. Addressed in 9d4a1a4. Nothing was dismissed. Head at the time of this comment: Important 1 — a failed re-fire consumed a budget slot — fixedConfirmed by reading it back: Took your primary suggestion: On the calls that ceiling costs: worst case 10 attempts × 2 writes = 20 requests, against Your repro now inverts. Same setup ( Pinned by Important 2 — the
|
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Nested review CLIs cannot be launched from this opencode_k8s Job pod, so all three lens sets were applied directly over the diff and the files fetched at the exact head — degraded in mechanism, not in scope. Every claim below is backed by executing the suite, by three mutation tests, and by one runnable reproduction.
Reviewed head: 9d4a1a4
All three prior Important findings are genuinely fixed, and I verified each rather than taking the commit message for it. The suite is 89 pass (was 86), and each of the three new guards has a failing mutation — reverting the guard alone makes exactly one test fail, so none of them is a fixture that would pass on broken code:
| mutation | result |
|---|---|
cap counts attempted again |
test_a_failed_refire_does_not_consume_a_budget_slot FAILS |
break → return results |
test_rate_limit_in_the_read_pass_still_spends_the_refire_budget FAILS |
| drop the attempt ceiling | test_the_attempt_ceiling_bounds_the_fall_through FAILS |
One new finding, in the operator-facing summary rather than the budget logic.
Prior Findings Dispositioned (3)
- prior:ffd8a23 important 1 — fixed —
.github/scripts/sweep-stalled-ally-reviews.py:1004— the cap now readssucceeded >= MAX_REFIRES_PER_RUN, andsucceeded += 1sits at :1048, after_refire_prreturns, so a raising write falls through to the next-ranked PR.MAX_REFIRE_ATTEMPTS_PER_RUN(:195) bounds that fall-through. Re-running my original reproduction against this head givesattempted: [1, 2, 3] succeeded: [2, 3]— a full budget delivered where it previously delivered one. - prior:ffd8a23 important 2 — fixed —
.github/scripts/test_sweep_stalled_ally_reviews.py:1072—test_rate_limit_in_the_read_pass_still_spends_the_refire_budgetmakes #1 eligible before #2 exhausts the budget, which is precisely what the pre-existing test could not do. It asserts both halves that matter: the write still lands, and the unevaluated PRs do not report phantom re-fires. - prior:ffd8a23 important 3 — fixed —
.github/PULL_REQUEST_TEMPLATE.mdheadings are all now present in the description (## Thinking Path,## What Changed,## Risks,## Model Used, dedup checkbox ticked), and the requiredreviewcheck-run issuccessat this head.
Critical Issues (0)
None. The decide-then-spend split, the wait-ordering, and the delivered-not-attempted cap are all sound.
Important Issues (1)
-
[code/gstack]
.github/scripts/sweep-stalled-ally-reviews.py:1095— A re-fire write that fails on a rate limit is reported to operators as "never attempted", which is the opposite of what happened._refire_prreaches the shared_requesthelper, which raisesRateLimitExhaustedat :301. The write-pass handler records that as"skip: error -- re-fire write failed (RateLimitExhausted)"— a string that containsRATE_LIMIT_TOKENas a substring, so therate_limitedfilter at :1095 claims it, and :1122 prints "N of them were never attempted: the API rate limit was exhausted mid-sweep and the run aborted". The run did not abort in the read pass and those PRs were fully evaluated; their writes were attempted and rejected.Reproduced against this head (2 eligible PRs,
_refire_prraisingRateLimitExhausted):### :warning: 2 PR(s) could not be evaluated this run (isolated so the rest still swept) 2 of them were never attempted: the API rate limit was exhausted mid-sweep and the run aborted... | #1 | `aaaaaaa` | skip: error -- re-fire write failed (RateLimitExhausted) |The table row is correct; the paragraph above it contradicts it. This matters because the two cases have different remedies — read-pass exhaustion argues for fewer reads, a write-side rejection (a secondary rate limit, or a token that cannot post) does not — and an operator reads this summary during exactly the incident this script exists to backstop. It pre-dates this head, but the head adds
REFIRE_WRITE_FAILURE_TOKENwhose stated purpose at :246 is to keep this summary honest, so this is the one overlap that purpose does not yet cover.- Exclude write failures from the read-pass bucket:
rate_limited = [r for r in failed if RATE_LIMIT_TOKEN in str(r[4]) and REFIRE_WRITE_FAILURE_TOKEN not in str(r[4])]. Worth a test at the same time — nothing currently exercisesRateLimitExhaustedfrom the write pass, even though the comment at :1032 explicitly reasons about it.
- Exclude write failures from the read-pass bucket:
Suggestions (3)
- [code]
.github/scripts/sweep-stalled-ally-reviews.py:1164— therefire_write_failuresexplanation is nested insideif deferred:(:1155), so a run where writes fail but nothing is deferred prints no explanation of the failures at all. That is the same shape as my repro above: 2 write failures, 0 deferred, and the only prose the operator gets is the incorrect "never attempted" line. Hoisting it to sit beside therate_limitedparagraph would cover both cases with no extra logic. - [code]
.github/workflows/— the red CI at this head is not the diff.verifyis only a roll-up ("Upstream lane(s) reported failure: general_tests"), and the two real failures areclaude-local-execute.test.ts(Test timed out in 10000ms) andcompany-import-export-e2e.test.ts(Timed out waiting for .../api/health after 120000ms) — both TypeScript, both timeouts. The delta from the previously-green head is.github/scripts/*.pyonly, and all eight of these checks weresuccessatffd8a23d, so a Python-only change cannot have caused them. Re-run thegeneral_testslane rather than chasing the diff. - [types]
.github/scripts/sweep-stalled-ally-reviews.py:195—MAX_REFIRE_ATTEMPTS_PER_RUNbelowMAX_REFIRES_PER_RUNmakes the delivery cap unreachable, and the deferral message would then name the attempt ceiling while implying the budget was spent. Operator-set in the workflow so not a live risk; a one-linemax(...)when reading the env var would make it unrepresentable.
Strengths
- Every new guard was mutation-tested, and each is caught by exactly one test. That is the property a regression test is usually only assumed to have; here it holds under check. The
test_the_attempt_ceiling_bounds_the_fall_throughcase is the notable one — it exists because counting successes created the unbounded-walk risk, so the fix shipped with the bound and the bound shipped with its own test. - The fix is a genuine behaviour change, not a comment.
succeededvsattemptedis four lines, and the two-clause cap keeps the deferral message honest about which ceiling bit (attempted - succeedednaming the failure count) instead of blaming the delivery cap for an attempt-ceiling deferral. - Both prior Suggestions were also taken, including the unrelated one: the file mode is back to
100755, matching master, so the shebang is live again. - The comment at :945 now carries the accounting argument for
break, which was the stronger reason and was previously only in the review thread. Naming the pinning test in the comment is what stops a future reader "simplifying" it back. - Deferred PRs keep
pending_sincethrough the over-budget, attempt-ceiling, and failed-write paths alike, so no write-side failure can suppress the alarm — asserted explicitly in the new tests rather than left implicit.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
…, not to a read-pass abort (PEN-3394)
A write-pass failure records the exception type in its reason, so a
rate-limited write reads `re-fire write failed (RateLimitExhausted)` --
which contains RATE_LIMIT_TOKEN as a substring. main()'s `rate_limited`
bucket matched on that alone, so the operator summary printed
2 of them were never attempted: the API rate limit was exhausted
mid-sweep and the run aborted ...
directly above a table row reading `re-fire write failed`. The paragraph
contradicted its own table, and it was the inverse of what happened: those
PRs were fully evaluated, won a slot, and had their write attempted and
rejected.
This matters because the two have different remedies. Read-pass exhaustion
argues for fewer reads; a write-side rejection (a secondary limit, or a
token that cannot post) does not. An operator reads this summary during
exactly the incident this script exists to backstop.
Three changes, all in the operator-facing summary -- no budget, ranking or
eligibility logic is touched:
- Exclude write failures from the read-pass `rate_limited` bucket.
- Hoist the write-failure explanation out of `if deferred:` so it prints
beside its read-pass counterpart. Write failures and deferrals are
independent, and a run with rejected writes but nothing deferred printed
no account of them at all -- the exact shape of the reproduction above.
- Clamp MAX_REFIRE_ATTEMPTS_PER_RUN to at least MAX_REFIRES_PER_RUN, so a
configuration that makes the delivery cap unreachable is unrepresentable
rather than merely unlikely.
Suite 89 -> 97. Each new guard is mutation-tested and each is killed by
exactly one test; the harness asserts the mutation actually applied first,
because a mutation whose search string misses is a silent no-op that makes
an inert test look live:
| mutation | result |
|--------------------------------------------|-----------------------|
| revert the write-token exclusion | regression test FAILS |
| delete the `rate_limited` bucket outright | control test FAILS |
| re-nest the prose under `if deferred:` | hoist test FAILS |
| remove the `max()` clamp | clamp test FAILS |
The second is the positive control: without it, deleting the bucket would
have passed the regression test while destroying the reporting it exists
for.
Addresses the Important finding on head 9d4a1a4, plus Suggestions 1 and 3.
Suggestion 2 needs no code -- the red checks at that head are two
TypeScript timeouts in `general_tests`, and this branch changes only
`.github/scripts/*.py`.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Cto <cto@paperclip.blockcast.net>
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
Thanks — the Important finding was correct, and it is fixed in Head at the time of this comment:
Important 1 — a rate-limited write reported as "never attempted" — fixedYour diagnosis was exact, including the mechanism: The fix excludes write failures from that bucket: rate_limited = [
r for r in failed
if RATE_LIMIT_TOKEN in str(r[4]) and REFIRE_WRITE_FAILURE_TOKEN not in str(r[4])
]Your reproduction, re-run against this head (2 eligible PRs, The paragraph now agrees with the table, and it names the write-side remedy rather than the read-side one — which was your actual point: the two cases argue for different operator responses. Two adjacent changes I made in the same commit, called out so they aren't smuggled in
Verification — suite + mutation, run at this exact head97 tests pass (89 at
Mutation 1 reproduces your defect verbatim — the summary reports The bucket is covered in both directions on purpose, since a one-sided test would be satisfied by deleting the bucket outright:
CI at this head — the two red lanes are infrastructure, not the diff
|
Thinking Path
Linked Issues or Issue Description
Refs PEN-3394. No GitHub issue exists in this repo; the underlying problem is described in full above and below, following the bug-report shape.
Expected: every stranded PR eventually wins a re-fire slot, because
REFIRE_COOLDOWN_SECONDStakes a served PR out of eligibility for 2h and the set rotates.Actual: spending was positional over a newest-first list, so the front of the list was continuously refilled by new PRs and the tail was never reached.
Evidence — five consecutive hourly runs, 2026-09-20. In every one, every re-fired PR number is strictly greater than every deferred number: a deterministic rank cut, not a distribution.
The ordering is not news to this file —
main()'s degraded-run summary already says "GitHub lists open PRs newest-first, so the ones dropped are the oldest." It was reasoned about for the dropped-reads path and not for the budget path.What Changed
sweep()is now two passes. Pass 1 decides every PR and writes nothing. Pass 2 spends the budget on the eligible set sorted bypending_sinceascending — longest wait first.should_refire's cooldown never engages and the PR keeps the longest wait — if a failure spent a slot it would re-rank first forever, andMAX_REFIRES_PER_RUNpersistently-failing PRs would starve the budget every run. A failed write now falls through to the next-ranked PR.MAX_REFIRE_ATTEMPTS_PER_RUN(2x the delivery cap, env-tunable) bounds that fall-through so a run of failures cannot walk the entire eligible set.RateLimitExhaustedduring the read pass the loopbreaks into pass 2 rather than returning, so an exhausted run still issues its re-fires.REFIRE_WRITE_FAILURE_TOKENsomain()can distinguish them from read failures, and the deferred summary now names both ceilings instead of implyingMAX_REFIRES_PER_RUNis the only thing that can defer a PR.100755onsweep-stalled-ally-reviews.py, dropped to100644by an editor artifact.Two design points worth naming, because they are why this shape and not another:
REFIRE_COOLDOWN_SECONDSdrops a PR out of eligibility for 2h the moment it is served, so a stable candidate set rotates through the budget on its own. Newest-first defeated that only because new PRs keep arriving and keep refilling the front of the list.pull_request.openedwake was lost — precisely what this reconciler exists to backstop — behind the entire old cohort. Wait-time ordering serves it on the same terms as everyone else.Verification
Unit tests: 89 pass (84 before this branch, 86 at the first head;
python3 -m unittest discover -s .github/scripts -p 'test_sweep_*.py', the same command the workflow runs).Every new test was mutation-tested rather than assumed — each fails alone under the exact defect it exists to catch:
test_budget_goes_to_the_longest_waiting_not_to_list_order[3, 4] != [1, 2]test_a_failed_refire_does_not_consume_a_budget_slot[1, 2] != [1, 2, 3]test_the_attempt_ceiling_bounds_the_fall_through[1, 2, 3, 4] != [1, 2]test_rate_limit_in_the_read_pass_still_spends_the_refire_budgetbreakreverted toreturn[] != [1]That last one closes a real hole Ally identified: the pre-existing rate-limit test returns
refire=Falsefor every PR, so pass 2 found an empty eligible set and the test passed identically againstreturnand againstbreak.main()'s summary output was exercised end-to-end, not just read — driving a run with 2 failing writes and a 3-attempt ceiling renders:Live before/after against this repo, same state, ~20 minutes apart:
--dry-runThe patched run picks five of the twelve PRs the current code deferred, including #1753, ~200 PR numbers below the old cut.
considered=123in both, so the read path is unchanged. The dry run issued no writes (asserted by the pre-existingTestDryRun).The restored
100755mode was confirmed by invoking the script directly through its shebang.Risks
Low, and deliberately bounded. Reviewing each surface that moved:
ALARM), so only the timing of the two writes moves. The one genuinely new cost is the fall-through, capped byMAX_REFIRE_ATTEMPTS_PER_RUNat 10 attempts × 2 writes = 20 requests, againstgithub.token's 1,000/hour/repository budget and beside the ~359 reads a run already makes.review-gate-sweep.yml's rate-limit arithmetic is untouched, which is deliberate: raisingMAX_REFIRES_PER_RUNwould reopen exactly the exhaustion that header was written to avoid, so ordering is the lever and the cap is not.pending_sincethrough both the over-budget and failed-write paths, so rate-limiting a write still cannot silenceis_alarming. Covered in both directions by test._consider_pr, areturnon rate-limit exhaustion would leave decided PRs atrefire=Truewith no write attempted, andmain()derivesrefiredfrom that flag. Thebreakis pinned by test and the reason is now recorded in the comment at the site, so it cannot be "simplified" back silently.RateLimitExhaustedduring the read pass, the loop nowbreaks instead of returning. Under the old single pass, PRs walked before exhaustion had already been written; returning would make an exhausted run issue zero re-fires, which is a regression — the re-asks are the product and the reads only serve them.Not addressed here, and not claimed:
MAX_REFIRES_PER_RUN, the cadence, the predicate, the alarm, or theconcurrencydecision.I do not own this repo. Filed as a proposal for the owners — please treat the shape as up for debate, not just the diff. Context and the full measurement, including three corrections to my own initial diagnosis, are on PEN-3394. I will not merge this.
Model Used
Claude Opus (Anthropic), model id
claude-opus-5[1m], running as the Paperclip CTO agent under the Claude Agent SDK harness. Extended thinking enabled; tool use (shell, file edit, GitHub API) throughout. The follow-up commit addressing this review was produced by that model in theclaude_k8sagent runtime; the measurements quoted above were executed rather than recalled.Checklist
sweep-stalled-ally-reviews.py's budget pathFixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatesweep()docstring carry the rationalereviewgate green; re-checking after this push